Fix the two intermittent failures, both real bugs - #10
Conversation
Neither was a test defect. internal/mcp: reaperLoop calls cmd.Wait, and os/exec closes the pipes it created once Wait sees the child exit. A read still in flight then fails with "read |0: file already closed" instead of reporting EOF -- the reader simply lost a race. That error was recorded as the cause of death, and because markDead is first-writer-wins the reaper's clean exit status could never replace it, so Close reported a shutdown failure for a server that had stopped perfectly normally. On quit a user would see "mcp: server exited: read |0: file already closed" for a healthy server. os.ErrClosed is now classified exactly like EOF: provisional, leaving the reaper's status to win. The decision moves into exitReasonFromRead, a pure function, so it is tested directly rather than by losing the race on purpose. internal/acp: the session's active flag was cleared in a deferred function, which runs after the prompt response is already on the wire. A client that reads the response and immediately sends session/load -- which is what switching sessions looks like, and what the test does -- could be rejected with "session already has an active prompt". It is now cleared immediately before each terminal response, with the defer kept as a safety net for any future path that returns without sending one. The ordering the defer existed for is preserved: every trailing update is already written by then, so a replay still cannot interleave with this prompt's final updates. Evidence in both directions. With the acp clear reverted to defer-only, 300 iterations reproduce the exact CI failure; with the fix, 300 iterations pass and 100 more under -race. The mcp shutdown test passes 40 consecutive runs and the package passes three times under -race, and two subtests of the new classifier test fail when the ErrClosed handling is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 933dd92c50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clearActive := func() { | ||
| state.mu.Lock() | ||
| state.active = false | ||
| state.mu.Unlock() |
There was a problem hiding this comment.
If another session/prompt or session/load is already queued when a turn enters any terminal path, this clears active before sendResult/sendError acquires writeMu. The request loop can consequently accept the next operation, whose worker or history replay may write updates before the preceding prompt's terminal response, violating the one-active-prompt ordering guarantee and potentially replacing the runtime before the old worker returns. Clear the flag and serialize the terminal response as one synchronized transition rather than exposing an idle session between these calls.
Useful? React with 👍 / 👎.
1 participant
The two tests that had been failing at random on CI. Neither turned out to be a test defect — both are production bugs that the tests were correctly catching, intermittently.
internal/mcp— a healthy server reported as a failed shutdownObserved as:
reaperLoopcallscmd.Wait(), andos/execcloses the pipes it created onceWaitsees the child exit.StdoutPipe's own documentation says as much: "Wait will close the pipe after seeing the command exit, so most callers need not close it themselves; it is thus incorrect to call Wait before all reads from the pipe have completed."So a read still in flight fails with
os.ErrClosedinstead of reporting EOF. The reader had simply lost a race with the reaper. That error was then recorded as the cause of death — and becausemarkDeadis first-writer-wins:the reaper's clean exit path could never replace it.
closeExitErrsurfaced it, andClosereported a shutdown failure for a server that had stopped perfectly normally. This is not test-only: on quit, a user would seemcp: server exited: read |0: file already closedfor a healthy server.os.ErrClosedis now classified exactly like EOF — provisional, leaving the reaper's status to win. The existing code already had the right instinct for EOF, and its comment says why: EOF on stdout is not a cause of death, it is the first symptom.ErrClosedis the same symptom wearing a different coat.The decision moved into
exitReasonFromRead, a pure function, so it can be tested directly instead of by losing the race on purpose.internal/acp— a legitimatesession/loadrejected as busyObserved as:
The session's
activeflag was cleared in a deferred function:A
deferruns after the prompt response has already been written to the wire. That leaves a window in which the client has the response — from its point of view the prompt is over — but the server still considers the session busy. A client that reads the response and immediately sendssession/loadgets rejected.That is not a hypothetical client. It is what switching between sessions looks like, and it is exactly the sequence in the failing test: prompt → cancel → await the prompt response → re-load.
activeis now cleared immediately before each of the four terminal responses, with thedeferkept as a safety net for any future path that returns without sending one (clearing twice is harmless). The ordering the defer existed for is preserved — its comment worried about a replay interleaving with the prompt's final updates, and every trailing update is already written by the time each terminal response is sent.Evidence
Both directions, because "it stopped failing" is not evidence on its own:
re-load of an idle session should succeedTestServerLoadSession*, 100 iterations,-raceTestManager_Shutdown_AllClients, 40 consecutive runsinternal/mcppackage, 3 runs,-raceErrClosedhandling removedPlus the usual gates:
golangci-lint0 findings underGOOS=linux,darwinandwindows;go vetclean.One note on the full-suite run
A full
go test ./...on this branch also failedTestManager_ReadOnlyJobWithoutVerifyRootCannotSeeWorktree(internal/jobs, 88s) andTestEngine_RetryCapIsHard(internal/workflow). Both pass in isolation in 3.4s and 2.7s respectively. They were starved by three concurrentgolangci-lintpasses I was running at the same time, and neither package is touched by this change — but it is worth recording thatinternal/jobsin particular degrades badly under CPU pressure.🤖 Generated with Claude Code