Repository navigation
Conversation
…over `daemon start` exits once the daemon answers status.get, which it does from its socket claim on; admin.token lands only after that claim. An admin command run next read no file, connected as agent and got FORBIDDEN: token create printed no JSON and the teardown's daemon stop left the daemon running. Closes #404
3 findings fixed, comments only
Contributor
Author
Review notesNot blocking, not verified. Each is one reviewer's claim.
Written by an agent. |
V3RON
marked this pull request as ready for review
October 11, 2026 13:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
withDaemonhanded the test a daemon before the daemon'sadmin.tokenfile existed. Every admin CLI call after that raced the daemon's write of the file.withDaemon→startDaemon()ransimlock daemon startand returned when the command exited (e2e/helpers/env.ts:241on main).daemon startexits 0 oncestatus.getanswers (src/cli/index.ts:1618-1631). The daemon answershelloandstatus.getfrom the moment it claims its socket (src/daemon/server.ts:447). It writesadmin.tokenonly after that, inadminSecret.persist()(src/daemon/server.ts:454, ADR 0003 §5).token create --role worker(e2e/gateway-fast-fail.test.ts:35) reads its credential fromadmin.token, three tries 50 ms apart (src/cli/index.ts:219-232). If the file is still missing, the CLI connects asagentand prints the notice (src/cli/index.ts:294-300).token.createneedsadmin, so the daemon answersFORBIDDEN. stdout is empty,minted.jsonisundefined, and line 36 throwsCannot destructure property 'secret' of 'minted.json'.e2e/helpers/env.ts:360-361) records the pid holding the socket, then runsdaemon stop. That command reads the same missing file, connects asagent, and getsFORBIDDENfromdaemon.stop(src/daemon/server.ts:768-776). The daemon keeps running, so the grace window ends withwithDaemon teardown found stray daemon process(es) that outlived daemon stop.So the two errors in the issue have one cause:
admin.tokenwas not on disk when either command read it. The write is one temp-file write plus a rename. On a loaded box it can finish later than the CLI's 100 ms retry window, and nothing in the helper waited for it. The product does what ADR 0003 §5 and docs/CLI.md say it does here: a short retry, then an agent session with a notice. Architecture rule 11 needs that wait to be bounded. The false assumption was in the helper: "daemon starthas exited, so admin commands work".Fix
e2e/helpers/env.ts: whendaemon startexits 0,startDaemon()now waits untiladmin.tokenexists before it returns.restartDaemon()(andwithConfig()through it) now starts the daemon throughstartDaemon(), so the same wait covers restarts.e2e/gateway-fast-fail.test.ts:startGatewaychecksminted.codeand printsminted.stderrbefore it destructures. A failure now names the CLI's own error, which the issue asked for.Why this removes the race by construction:
writeFileAtomicrenames a complete temp file into place, so onceadmin.tokenexists it holds the full secret. Only the daemon that won the socket claim writes the file. It is removed only when that daemon stops gracefully.withDaemonno longer returns before the file exists, so every later CLI call in the test reads it and connects as admin, however long the write took. That includestoken createand the teardown'sdaemon stop. The ordering no longer depends on how fast the daemon's I/O is compared with the CLI's 100 ms retry.Why it is the simplest fix: it is one wait in the one helper that holds the false assumption. The alternatives change the product. One is to make
hellowait for the file, which contradicts ADR 0003 §5 ("a credential can be checked before the file lands"). Another is to makedaemon startor the CLI retry wait longer, which only moves a wall-clock bound.The wait uses
waitFor's default 30 s budget. It is a backstop for a daemon that never writes the file, not the mechanism that orders the calls.Evidence
Forcing the interleaving. A temporary patch, not committed, delays the daemon's write of
admin.tokenby 3 s. That keeps the file missing for the whole window in which the test callstoken createand the teardown callsdaemon stop:Unfixed code (main 8eb9c87) with the patch:
pnpm test:e2e e2e/gateway-fast-fail.test.ts -t "drained"These are both errors from the issue, word for word. The other failing test, "fails a request only a drained worker…", also matches
-t drainedand fails the same way. With only this PR'sminted.codeassertion added, the CLI's own output shows the mechanism:This PR with the same patch (every daemon's write delayed 3 s, gateway and workers):
pnpm test:e2e e2e/gateway-fast-fail.test.tsSanity check without the patch:
pnpm test:e2e e2e/gateway-fast-fail.test.ts e2e/contract-surface.test.ts e2e/held-lease-liveness.test.ts e2e/event-history.test.ts e2e/startup-reconcile.test.ts e2e/mcp-session.test.tspasses 38 of 38. These files callstartDaemon,restartDaemonorwithConfig.No permanent regression test: forcing a slow write needs a delay hook in the daemon, and the rules do not allow a production test hook for this. The diff above is how to re-apply the reproduction.
Assumptions
daemon startreports running always writesadmin.token(DaemonServer#startwrites it right after the socket claim, and a failed write fails the start). So waiting for the file cannot hang on a healthy daemon, and the 30 s budget only catches a broken one.killDaemon("SIGKILL")the dead daemon'sadmin.tokenstays behind. The wait sees that old file and returns at once, so a test that restarts after a SIGKILL still races the new daemon's write.Review
Spec review: 0 blocking, 0 fixed, 3 notes. Code review: 0 blocking, 0 fixed, 3 notes. Claims review: 1 blocking, 1 fixed, 1 notes. Comment review: 3 blocking, 2 fixed, 0 notes.
Mutate: 0 mutants, 0 alive.
Rejected:
Closes #404
Written by an agent.