Skip to content

fix(mcp): publish a restarted hyperd endpoint to daemon.json before STATUS - #286

Merged
StefanSteiner merged 3 commits into
tableau:mainfrom
StefanSteiner:fix/daemon-restart-endpoint-publish-order
Sep 6, 2026
Merged

fix(mcp): publish a restarted hyperd endpoint to daemon.json before STATUS#286
StefanSteiner merged 3 commits into
tableau:mainfrom
StefanSteiner:fix/daemon-restart-endpoint-publish-order

Conversation

@StefanSteiner

Copy link
Copy Markdown
Contributor

Fixes #284

Stacked on #278. This branch is based on #278's rebased head (236e957),
so its diff here includes #278's two commits. It targets main and should be
merged after #278; once #278 lands, this reduces to the single commit
fix(mcp): publish a restarted hyperd endpoint to daemon.json before STATUS.
Deliberately kept out of #278, which is a separate concern.

Why

test (ubuntu-latest) has been red on main since 56bc0d0, and because the
failure is a race it randomly reddens the ubuntu leg of every open PR —
including #278, whose own run was otherwise 16/17 green.

commit test (ubuntu-latest)
c9bacf2 success
06c5da1 success
56bc0d0 (#279 merge) failure
61fc766 (#280 merge) failure

56bc0d0 re-enabled eight daemon crash-and-restart tests on Linux and Windows
by narrowing #[ignore] to #[cfg_attr(target_os = "macos", ignore)]. Both
red runs fail identically: engine_recovers_after_hyperd_killed at
daemon_tests.rs:1473:67 with Connection refused against a hyperd
endpoint.

What was actually wrong

The prior investigation root-caused this as try_restart_hyperd publishing the
new endpoint to STATUS before rewriting daemon.json, with the test's STATUS
readiness gate racing Engine::new's file-based discover(). I could not
confirm that, and the measurements contradict it.
Two things are wrong here,
and the ordering hazard is not the one breaking CI.

1. The readiness gate never waits for the restart — this is the CI failure

wait_for_endpoint_change_or_recovery returned as soon as the endpoint STATUS
reported was TCP-connectable, never comparing it against the pre-kill endpoint
despite its name. For a few milliseconds after SIGKILL the doomed hyperd's
listening socket still completes inbound connections, and the callers poll
immediately after the kill — while the daemon's 5s monitor tick has not fired,
so STATUS still advertises the old process. The gate therefore returned on its
first attempt carrying the pre-kill endpoint, and the caller then raced the
kernel tearing that socket down.

The wall clock alone proves the failures precede any restart: every failing
run finished in ~0.53s, every passing run in ~5.47s
, against a 5s monitor
tick. Instrumented, the failing runs are explicit — and note the discovery file
and STATUS agreed, which is what rules out the ordering hypothesis:

DIAG pre_kill_endpoint=127.0.0.1:50993
DIAG gate_returned=127.0.0.1:50993 after=946.708µs same_as_pre_kill=true
DIAG discover_file_endpoint=Some("127.0.0.1:50993")
-> panicked ... Connection refused

The daemon in fact behaved correctly in every failure: a client that finds a
dead hyperd through discovery is supposed to get a connect error and report
it (Engine::new calls report_hyperd_error_to_daemon on exactly this path).
The test asserted success without waiting for the recovery it claimed to
await.

wait_for_live_hyperd_after_kill replaces it and gates in two phases: first
wait for the killed endpoint to stop accepting connections, proving the old
hyperd is gone; only then accept the endpoint STATUS advertises. It
deliberately does not require the endpoint to change — the OS may hand
the replacement the same port, and a changed-endpoint gate would then wait out
its whole timeout on a healthy restart.

2. The daemon published a restarted endpoint in an unsafe order — real, but latent

try_restart_hyperd copied the new endpoint into the in-memory DaemonInfo
that STATUS serves, released that lock, and only then rewrote daemon.json.
Since discovery::discover() reads the file, a client discovering inside that
window connected to the hyperd just dropped. The write failing was worse
than the window: the error return happened with the new endpoint already
published to STATUS and the replacement HyperProcess dropped on the way out,
leaving STATUS durably naming a hyperd the daemon itself had killed.

Now the discovery file is persisted first and DaemonInfo flipped second, both
under one info_arc lock, so observing the new endpoint through either channel
implies a live hyperd and no STATUS reader can catch an endpoint the file has
not committed. write_discovery_record is already atomic (temp file +
fs::rename), so this is purely an ordering change.

To be explicit: in 32 instrumented runs the file and STATUS agreed in every
case, including all failures.
This is fixed because it is a genuine
correctness bug in its own right, not because it reddens CI.

Measurements

Run on macOS via --ignored, driving the test binary in a loop. Baseline on
pristine main (61fc766); "after" on this branch.

test baseline after
engine_recovers_after_hyperd_killed 4 failures / 20 runs 0 failures / 50 runs
hyperd_monitor_detects_killed_hyperd_and_restarts 0 failures / 20 runs 0 failures / 20 runs
client_report_triggers_restart_after_kill 0 failures / 20 runs 0 failures / 20 runs

All four baseline failures were byte-identical and matched CI exactly.

The same gate was silently making a second test vacuous

hyperd_monitor_detects_killed_hyperd_and_restarts never failed, but 5 of
its 20 baseline runs finished in ~0.46s instead of ~5.4s
— the gate handed
back the pre-kill endpoint and the follow-up probe caught the socket before
teardown. Those runs asserted nothing about a restart: the test passed without
the behavior under test ever occurring. After the fix, 0 of 20 are sub-1s;
all take the full restart time. client_report_triggers_restart_after_kill
showed 0 vacuous runs only by accident — its REPORT_HYPERD_ERROR round-trip
happens to outlast the teardown window — and is fixed alongside the others.

Verification

cargo fmt --all -- --check                                          exit 0
cargo clippy --workspace --all-targets --all-features -- -D warnings exit 0
cargo test -p hyperdb-mcp --lib                     137 passed, 0 failed
cargo test -p hyperdb-api --test pool_tests          16 passed, 0 failed
cargo test -p hyperdb-mcp --test daemon_tests        52 passed, 0 failed, 8 ignored
npx markdownlint-cli2                                0 issues in 68 files

Because these eight tests are #[cfg_attr(target_os = "macos", ignore)], the
plain daemon_tests gate skips them on macOS (8 ignored above). To get
Linux-equivalent coverage locally I also ran the suite with
-- --include-ignored --test-threads=1 three times: 60 passed / 0 failed
twice, and one run failed on daemon_mode_engine_connects_to_shared_hyperd
with TestDaemon did not start within 150s — which is the pre-existing,
macOS-specific condition the original #[ignore] reason names verbatim
("daemon startup exceeds 150s timeout"), reproduced only because I forced the
macOS-ignored tests to run under load. It is unrelated to this change and is
not the failure mode seen on Linux CI.

Four correctness defects in hyperdb-mcp/src/daemon/discovery.rs:

- discover() deserialized the whole DaemonRecord (including the nested
  identity object) and then discarded it via record.info().clone(),
  buying strictness that cost forward compatibility. A daemon.json with
  a retyped identity, a reshaped executable_path, or an identity
  missing executable_path made a live, healthy daemon undiscoverable,
  defeating wait_for_daemon, status_degraded, and Engine::is_running.
  discover() now always extracts the legacy DaemonInfo fields (which
  ignore unknown fields) and only uses the stricter DaemonRecord parse
  to choose a debug-log message. The doctor's separate bounded raw
  reader is untouched and keeps the strict contract, since it
  deliberately wants to know about a malformed identity block.
  Fixes tableau#270.

- write_discovery_record deleted the existing daemon.json before
  renaming the temp file into place, on the false premise that
  std::fs::rename fails when the target exists on Windows. It doesn't:
  MoveFileExW (MOVEFILE_REPLACE_EXISTING) and the FileRenameInfoEx
  fallback both replace an existing target, matching Unix. The
  unnecessary delete opened a window where daemon.json didn't exist at
  all, observable by a concurrent discover() during try_restart_hyperd.
  The write is now a single rename, as the doc comment already claimed.

- The bounded raw discovery reader classified an oversized-but
  well-formed record as Malformed, sending users to fix JSON syntax
  that was never broken. RawDiscoveryRead gained an Oversized variant;
  the doctor now reports what actually happened.

- send_command_with_timeout returned Ok("") when a health-command peer
  closed the connection without writing anything, so daemon_stop
  printed "Daemon responded:" with nothing after it and exited 0. It
  now returns UnexpectedEof.

Fixes tableau#270. Fixes 3 of 5 items in tableau#275 (the atomic write, the
oversized-record misclassification, and the empty peer-close success).
Follow-up to the adversarial review of tableau#278.

The new tolerant `DaemonInfo` fallback in `discover()` yields a record where
the pre-change `Malformed` arm returned early, so control now reaches the
stale-cleanup `remove_file` it never used to reach. That lands squarely in
the scenario this PR exists to serve: an old client that fails a single
300 ms PING against a live *newer* daemon would delete that daemon's
discovery record, which the daemon only rewrites on `hyperd` restart —
hiding a healthy daemon from every client on the machine, not just the one
that could not parse it. It also contradicted the doctor's own operator
message, "The daemon discovery file was malformed; it was left unchanged."

`discover()` now tracks whether the record came from the tolerant fallback
and skips the cleanup in that case. A strictly parsed record on a dead port
is still stale-cleaned exactly as before; both halves are pinned by the new
`discover_preserves_a_leniently_parsed_stale_record` regression test.

Also in this change:

- The fallback no longer re-reads the file from disk.
  `RawDiscoveryRead::Malformed` and `Oversized` carry the bytes already
  read, removing both the redundant I/O and the window in which the strict
  and tolerant parses could observe different content. Those bytes are held
  in a `DiscoveryBytes` newtype whose `Debug` prints only a length:
  `RawDiscoveryRead` is `Debug`-formatted into diagnostics, and a derived
  `Debug` on a `Vec<u8>` would echo the whole unparsed file as a decimal
  byte list. `raw_discovery_read_is_non_mutating_and_distinguishes_io`
  already asserted that contents do not leak, but only as text, so it would
  not have caught that form; it now checks for the byte-list rendering too.
- `discover()`'s `Oversized` arm is folded into the fallback instead of
  returning `None`. The legacy reader applies no size cap, so the arm is
  unreachable today; folding it in means adding a cap later cannot silently
  turn a large-but-valid record into an undiscoverable daemon — the exact
  defect the fallback exists to prevent.
- The changelog described a design the code does not have (legacy fields
  "always" extracted, the strict shape used only to pick a log message).
  Rewritten to match the implementation: strict parse first, tolerant
  fallback second, and the preserved-record behavior documented.
- `send_doctor_command` now classifies a peer that closes without writing as
  `UnexpectedEof`, matching the `send_command_with_timeout` fix. This changes
  one operator-visible doctor warning: a recorded candidate that accepts the
  connection and then answers nothing is reported as
  `daemon_discovery_candidate_unreachable` rather than
  `daemon_status_malformed`, which is what actually happened. Pinned by
  `real_doctor_reports_a_silent_peer_close_as_unreachable_not_malformed`.

Test polish carried over from the tableau#279 review:

- `process_tests`' doc comment credited the parent with writing the PID
  report; the child writes it and the parent polls.
- The child now publishes that PID atomically (temp file plus `fs::rename`),
  so a poll cannot read a truncated-yet-parseable prefix and silently accept
  a wrong PID. The retry loop stays as defense in depth.
- `recovery_tests` reads `probe_valid` after `try_lock` rather than before.
  The flag is monotonic false->true, so reading it later can only produce a
  false *inconclusive*, never a false pass; reading it first left a window
  for the worker to return in between and mark a vacuous probe valid.
…TATUS

`try_restart_hyperd` copied the new endpoint into the in-memory `DaemonInfo`
that the health port's STATUS command serves, released that lock, and only
then rewrote `daemon.json`. `discovery::discover()` — the path every client's
`Engine::new` takes — reads the file, so a client discovering inside that
window connected to the `hyperd` the daemon had just dropped.

The write failing was worse than the window: the error return happened with
the new endpoint already published to STATUS and the replacement
`HyperProcess` dropped on the way out, leaving STATUS durably naming a
`hyperd` the daemon itself had killed.

Persist the discovery file first and flip `DaemonInfo` second, both under one
`info_arc` lock, so observing the new endpoint through either channel implies
a live `hyperd` behind it and no STATUS reader can catch an endpoint the file
has not committed. `write_discovery_record` is already atomic (temp file +
`fs::rename`), so this is purely an ordering change.

Also fix the readiness gate the restart tests share, which is what actually
reddened CI — the ordering hazard above is real but was not the cause, since
the file and STATUS agreed in all 32 instrumented runs.
`wait_for_endpoint_change_or_recovery` returned as soon as the endpoint STATUS
reported was TCP-connectable, never comparing it against the pre-kill one
despite its name. For a few milliseconds after SIGKILL the doomed hyperd's
listening socket still completes inbound connections, and the callers poll
immediately after the kill while the 5s monitor tick has not fired — so the
gate returned on its first attempt carrying the pre-kill endpoint, and the
caller then raced the kernel tearing that socket down.

Every failing run finished in ~0.53s against ~5.47s for a passing one, which
is proof the failures land before any restart could have completed. The
renamed `wait_for_live_hyperd_after_kill` waits for the killed endpoint to
stop accepting connections first, then for STATUS's endpoint to be reachable.
It deliberately does not require the endpoint to *change*, since the OS is
free to hand the replacement the same port and a changed-endpoint gate would
then wait out its whole timeout on a healthy restart.

Measured on macOS via `--ignored`, 20 runs each, before -> after:

  engine_recovers_after_hyperd_killed                4 failures -> 0
  hyperd_monitor_detects_killed_hyperd_and_restarts   0 failures -> 0
  client_report_triggers_restart_after_kill           0 failures -> 0

The monitor test was not failing but was passing *vacuously*: 5 of its 20
baseline runs finished in ~0.46s instead of ~5.4s, having verified no restart
at all. After the fix all 20 take the full restart time.

Fixes tableau#284
@StefanSteiner
StefanSteiner merged commit b54103c into tableau:main Sep 6, 2026
17 checks passed
StefanSteiner added a commit that referenced this pull request Sep 7, 2026
… budget (#309)

* test: make changelog contract robust to rollover; bump restart budget

Fix A: smoke_demo_and_changelog_contract asserted KV/export/routing claims
only against the `## [Unreleased]` slice of the embedded mcp CHANGELOG.
PR #307 (the rc.3 changelog rollover) moved every bullet into
`## [1.0.0-rc.3]`, emptying Unreleased and failing the test. Because #307
was docs-only, CI's `paths-ignore: **/*.md` skipped the Rust suite, so the
break surfaced only on a later code PR. Assert instead against the current
release window (`## [Unreleased]` + the most-recent dated section) via a new
`current_release_window` helper, so a future rollover can't silently
rebreak it. The window is bounded to the newest dated section so a stale
token in an ancient entry can't satisfy a deleted-claim check.

Fix B: engine_recovers_after_hyperd_killed and
hyperd_monitor_detects_killed_hyperd_and_restarts time out on
ubuntu-latest under CI load — a 5s monitor tick plus cold hyperd spawn
against a 12s readiness budget. Replace the three magic `12`s with a named
RESTART_READINESS_BUDGET_SECS = 45 and document the derivation. Budget bump
only; the macOS-only ignore is unchanged, so Linux/Windows crash-recovery
coverage from #279/#286 stays. See #305.

* test: gate RESTART_READINESS_BUDGET_SECS to cfg(unix)

The constant's only use sites are the three `#[cfg(unix)]` restart tests
and their Unix-only helpers, so on Windows it compiled but was never
referenced and `clippy -D warnings` (windows-latest) rejected it as dead
code. Gate the definition with `#[cfg(unix)]` so it exists exactly where
it's used — the honest fix, not `#[allow(dead_code)]`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(mcp): daemon restart advertises the new hyperd endpoint before persisting it, and the restart tests' readiness gate never waits for the restart

1 participant