fix(cli): flush stdout before exiting on a signal (OX-L8) - #58
Merged
Conversation
The last open oxaudit item that was a defect rather than a decision. Lifted from the audit/lane-a2 worktree, then rewritten - because the fix it carried does not work. OX-L8 was recorded rather than fixed, for a stated reason: SIGINT could not be exercised from the suite, so any change would ship unverified, and the fix contemplated at the time - dropping the forced exit and letting the loop drain - risked tokendamper mcp hanging on Ctrl+C instead. Both halves are answered here. Measured first, because the platform decides whether the bug exists. Node's stdout is synchronous for pipes on Windows and Linux, asynchronous on macOS, and the buffer only overflows on a large frame. A child writing 1 MB and exiting the way this handler does: Windows 1000046/1000046 complete, Linux 146176/1000046 truncated. Then end to end through the real product on Linux - a 900 kB MCP response with SIGINT delivered mid-stream - 365696 bytes arrived and the final frame did not parse, against 900819 bytes and a whole frame with the fix. lane-a2's fix was `await server.stop()`. It does not work: stop() is (): void, so the await yields one microtask, and a microtask does not run the I/O loop that drains a pipe. Measured at 146176/1000046 - byte for byte identical to no fix at all. Porting it as-is would have shipped something that reads as a fix in the changelog and changes nothing, which is the failure mode this review keeps finding. What works is asking the stream: an empty write queues behind everything pending, so its callback is the stream reporting its bytes reached the OS. The 2 s cap answers the hang objection - the exit is still forced, merely deferred, so a consumer that never reads cannot wedge shutdown. test/unit/cli/sigint-flush.test.ts pins the ordering deterministically by holding the write callback rather than waiting on a clock. All three cases fail against the unfixed handler, verified by reverting main.ts and re-running - a green from a test that cannot detect the defect would have been worthless here. 99 files / 918 tests pass, typecheck and lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes OX-L8, the last open oxaudit item that was a defect rather than a decision. Lifted from the
audit/lane-a2worktree — then rewritten, because the fix it carried does not work.The item, and why it was left open
process.exit()discards whatever a stream still holds, sotokendamper mcpexiting the instantstop()returned lost any JSON-RPC frame still buffered.It was recorded rather than fixed for a stated reason: "delivering SIGINT to exercise that is not something the suite can do here", so a change would ship unverified — and the fix contemplated at the time, dropping the forced exit to let the loop drain, risked
tokendamper mcphanging on Ctrl+C instead. Both halves are answered here.Measured first, because the platform decides whether this bug exists
Node's stdout is synchronous for pipes on Windows and Linux, asynchronous on macOS, and the buffer only overflows on a large frame. A child writing 1 MB and exiting the way this handler does:
Then end to end through the real product on Linux — a 900 kB MCP response with SIGINT delivered mid-stream:
lane-a2's fix does not work
The other worktree's change was
await server.stop().stop()is(): void, so the await yields one microtask — and a microtask does not run the I/O loop that drains a pipe. Measured: 146,176 / 1,000,046, byte for byte identical to no fix at all.Porting it as-is would have shipped something that reads as a fix in the changelog and changes nothing. That is the failure mode this review keeps finding, so it is recorded rather than repeated.
What actually works
Ask the stream. An empty
writequeues behind everything already pending, so its callback is the stream reporting that its bytes reached the OS.The 2 s cap answers the hang objection: the exit is still forced, merely deferred, so a consumer that never reads cannot wedge shutdown.
unrefkeeps the timer from holding the loop open.The test
test/unit/cli/sigint-flush.test.tspins the ordering deterministically — it holds the write callback rather than waiting on a clock, so it cannot pass by timing accident. Three cases: the flush defers the exit, the cap fires when no flush is reported, and the exit happens exactly once when both occur.All three fail against the unfixed handler, verified by reverting
main.tsand re-running (expected "spy" to not be called at all, but actually been called 1 times). A green from a test that cannot detect the defect would have been worthless here.Checks
99 test files / 918 tests pass, 2 skipped (POSIX-only). Typecheck and lint clean. CLI-only change, off the optimize route; no corpus run applies.
🤖 Generated with Claude Code