fix(daemon-shutdown): sequence exit on response flush, not a 50ms sleep (1qn)#102
Merged
Merged
Conversation
…ep (1qn) Sheriff finding #9 [LAW:no-ambient-temporal-coupling]: handleRequest scheduled setTimeout(() => shutdown(0), 50) after a newer-client version mismatch and after the shutdown verb, encoding "the response has flushed by then" as a magic delay nobody owned. A slow flush meant the client saw a dead socket (transient/io_error) instead of the VERSION_MISMATCH diagnostic (permanent/version_mismatch) — exactly during upgrades. The flush completion now owns the ordering: handleRequest returns { resp, exitAfterFlush: number | null } — the exit wish as data [LAW:effects-at-boundaries] — and respond() at the socket boundary performs it via sock.end(frame, cb), whose callback fires on finish OR error (total signal; a vanished peer cannot strand the exit). The version-mismatch asymmetry is now literally the value `req.v > PROTOCOL_VERSION ? 0 : null` [LAW:dataflow-not-control-flow]. The SIGKILL backstop inside shutdown() is untouched. New integration test pins the contract: a v+1 client receives the flushed VERSION_MISMATCH frame, then the daemon exits within budget.
There was a problem hiding this comment.
Z.ai Coding Agent Review
No must-change items found. The diff is a clean elimination of [LAW:no-ambient-temporal-coupling] — the setTimeout(() => shutdown(0), 50) is replaced by returning the exit code as data (HandledRequest.exitAfterFlush) from handleRequest, sequenced on sock.end's write-completion callback. The old empty catch {} around sock.write is replaced with a logged catch that still fires the settle thunk, fixing a pre-existing [LAW:no-silent-failure] gap. The new version-mismatch test validates both the flushed response and the process exit within budget. The stay() helper for the common non-exit path and the HandledRequest interface are well-factored. No violations introduced.
✅ Approved
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
2 participants
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.
Summary
brandon-daemon-shutdown-1qn, finding feat(render): capture terminal width at the wire boundary (kz8.4) #9[LAW:no-ambient-temporal-coupling].setTimeout(() => shutdown(0), 50)guesses insrc/daemon/server.ts. Exit is now sequenced on the response flush completion:handleRequestreturns{ resp, exitAfterFlush: number | null }(the lifecycle wish as data), andrespond()— the socket boundary — performs it viasock.end(frame, cb). Node invokes that callback on'finish'or'error', so the signal is total: a peer that vanishes mid-flush still settles and the exit is never stranded.req.v > PROTOCOL_VERSION ? 0 : null.shutdown()is untouched, per the ticket's locked decision.Why it matters
The failure mode landed exactly when users debug an upgrade: a slow flush let the daemon exit first, so the client saw a dead socket (
transient/io_error, triggering a pointless kick) instead of the flushedVERSION_MISMATCHdiagnostic (permanent/version_mismatch).Verification
v+1client receives the flushedVERSION_MISMATCHframe (receiving it at all proves flush-before-exit — premature exit kills the socket frameless), then the process exits within budget.check:protocol12/12; lint, typecheck, build clean.setTimeout(... shutdown ...)remains (the 500ms SIGKILL backstop usesprocess.killand is the documented, owned safety).