Skip to content

fix(mcp): reject daemon port 0 at the flag and the environment variable - #290

Merged
StefanSteiner merged 1 commit into
tableau:mainfrom
StefanSteiner:fix/mcp-reject-daemon-port-zero
Sep 7, 2026
Merged

fix(mcp): reject daemon port 0 at the flag and the environment variable#290
StefanSteiner merged 1 commit into
tableau:mainfrom
StefanSteiner:fix/mcp-reject-daemon-port-zero

Conversation

@StefanSteiner

Copy link
Copy Markdown
Contributor

Fixes #275

Summary

4 of #275's 5 items were already fixed on main by PRs #278 (landed inside #286's squash, b54103c) and #279 (56bc0d0). I audited each against the code and posted the item-by-item evidence as a comment on the issue. This PR closes the one that was genuinely still live: item 3, port 0.

For reviewers who want the short version of the audit:

Item Status Landed in
1 — the "atomic" discovery write isn't atomic already fixed #278 / b54103cremove_file and the false Windows-rename premise both gone
2 — torn PID-file read fails instead of retrying already fixed at both ends #279 / 56bc0d0 (the polling-side retry this issue asked for) and #278 / b54103c (writer made atomic + wait_for_reported_pid_retries_past_a_torn_write)
3 — port 0 accepted this PR
4 — oversized-but-valid record classified Malformed already fixed, phrasing included #278 / b54103cRawDiscoveryRead::Oversized, doctor now says "valid but larger than any legitimate record should be", test renamed to assert the honest classification
5 — silent peer close reported as success already fixed #278 / b54103cUnexpectedEof, plus the same fix in the doctor's own read loop

Item 2 is worth calling out because #278's own description said it fixed the torn read "at the writer" and left the polling side open. It didn't need to — #279 had already added the polling-side retry. Both halves are in, and both should stay: the retry is what stops a writer that ever regresses to a plain fs::write from resurrecting the flake.

The fix (item 3)

--port promises an exact bind, and "0".parse::<u16>() succeeds, so 0 passed validation at both entry points and reached TcpListener::bind — which assigns an ephemeral port rather than port 0.

What makes this more than cosmetic is that the failure compounds instead of surfacing. With the resulting PortScan { base: 0, span: 1 }, every client's scan probes port 0, gets a connection error, and ProbeResult::Refused reports it free. So each client that misses the discovery fast path concludes no daemon exists and starts another daemon-and-hyperd pair, on another ephemeral port no scan can ever find. The pairs accumulate silently.

Both entry points now reject it, exactly as the issue proposed:

  • The flag: #[arg(long, global = true, value_parser = clap::value_parser!(u16).range(1..))]. clap reports a usage error and exits 2.
  • The environment variable: a .filter(|port| *port != 0) in the HYPERDB_DAEMON_PORT chain, so 0 takes the same default fallback that unparseable values already take.

The --port help text and resolve_port_scan's rustdoc now both say why, rather than leaving the next reader to rediscover that base: 0, span: 1 is unsatisfiable by construction.

Two things I learned by running the test against the unfixed binary

The CLI test drives the real binary, because Cli lives in main.rs and is not reachable from a test crate. My first version used Command::output(), and running it against the unfixed binary hung the suite outrightdaemon --port 0 is accepted and runs a foreground daemon forever, so output() never returns. That same daemon then published daemon.json over my live developer discovery record.

Both are fixed in the test, and I think both are worth keeping as stated requirements rather than incidental hygiene:

  • It cannot hang. Each child is waited on against a 20 s deadline via try_wait() and killed if it outlives it, so a reverted fix fails with a message naming what happened instead of wedging CI.
  • It cannot touch the real state directory. HOME, USERPROFILE and HYPERDB_STATE_DIR are pinned into a TempDir, and the test asserts no daemon.json was published — which is what actually caught the regression when I ran the red-proof, before the exit-code assertion got a chance to.

This is a small argument for the same kind of sandboxing in any future test that shells out to the daemon subcommand.

Verification

Isolated worktree, dedicated CARGO_TARGET_DIR, HYPERD_PATH=~/dev/bin/hyperd.

  • cargo fmt --all -- --check — exit 0, no diff.
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — exit 0, no warnings.
  • cargo test -p hyperdb-mcp645 passed, 0 failed, 16 ignored, exit 0.
  • RUSTDOCFLAGS="-D warnings" cargo doc --no-deps — exit 0.
  • npx markdownlint-cli2 — 68 files linted, 0 issues, exit 0.

Red-before-green, with the value_parser range and the .filter each reverted:

test resolve_port_scan_rejects_zero_and_falls_back_to_the_default_scan ... FAILED
  assertion `left == right` failed: HYPERDB_DAEMON_PORT=0 must fall back to the
  default scan, not pin an unsatisfiable base: 0 / span: 1
    left: PortScan { base: 0, span: 1 }
   right: PortScan { base: 7485, span: 16 }

test daemon_cli_rejects_port_zero ... FAILED
  no invocation in this test may publish a discovery record; port 0 was
  accepted and a daemon actually started

test result: FAILED. 0 passed; 2 failed; exit 101, finished in 20.19s

Both green after restoring the fix, and confirmed the bounded kill left no stray daemon or hyperd process behind.

Relationship to the other PRs in this batch

Independent of #289 (issue #274, the accept loop) — that PR touches health.rs, this one touches main.rs and discovery.rs, so they do not overlap and either can merge first. Both are based on 2f31b9e.

`--port` promises an exact bind, and `"0".parse::<u16>()` succeeds, so `0`
passed validation at both entry points and reached `TcpListener::bind`, which
assigns an *ephemeral* port rather than port 0.

The failure compounded instead of surfacing. With the resulting
`PortScan { base: 0, span: 1 }`, every client's scan probed port 0, got a
connection error, and read `ProbeResult::Refused` as "this port is free" — so
each client that missed the discovery fast path concluded no daemon existed
and started another daemon-and-hyperd pair, on another ephemeral port no scan
could ever find. The pairs accumulated silently.

`--port` now carries a `1..` range, so clap reports a usage error and exits 2,
and the `HYPERDB_DAEMON_PORT` chain filters `0` into the same default fallback
that unparseable values already take.

The CLI test drives the real binary, because `Cli` lives in `main.rs` and is
not reachable from a test crate. It waits on each child against a deadline and
kills it rather than calling `output()`, and pins HOME/USERPROFILE/
HYPERDB_STATE_DIR into a temp dir. Both guards are there because running it
against the unfixed binary hung the suite outright — `daemon --port 0` was
accepted and ran a foreground daemon forever — and that daemon published
`daemon.json` over the developer's live discovery record. A regression now
fails with a message, inside the sandbox.

Fixes tableau#275
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.

daemon cleanup: five small correctness and honesty defects

1 participant