Four driver failures on CI: a crash in the connector, and three checks that read too early - #759
Merged
Merged
Conversation
… not sent on a closed channel The reader closes the session's updates when the worker's output ends, but it is not the only goroutine that emits. A prompt's writer finishing a canceled turn, and the policy check ending an unsafe one, both call lastWord from goroutines of their own: each reads the refusals Codex logged on its way out and emits an update for every one. Either can reach that after the reader has gone, and a send on a closed channel is a panic — the connector crashed with one on CI. The close and every send now take the session's lock, and an update emitted after the close is dropped, as an update onto a full buffer already is. The refusal itself is unaffected: it reaches the ledger before the update is offered. A test holds it. The worker's output ends while the worker lives on, so the reader waits out its grace and finishes having read a stderr with no refusal in it; the worker then logs its refusal as it is ended, and the last word is read from a goroutine that is not the reader's. On the previous emit it panics every run.
… disk is its latest A flood of permission requests is answered on a goroutine each, and each writes the whole record once it has its outcome. Two of those interleave: one takes its snapshot, a later one takes a fuller snapshot and renames it into place, and then the first renames its older one over that. The file's last word is then a record the agent has already moved past, and nothing writes again to correct it — so a test waiting for what the agent has done waits for a word that was said and unsaid. The two also shared the temporary file they renamed from. The snapshot and its rename are now one step, so what lands is never older than what landed before it. TestAFloodOfPermissionRequestsIsBounded is the test that spends 60 seconds on this and gives up: it waits for 52 of 60 requests to be answered, and 52 is exactly how many are ever answered, so losing one publication loses the whole wait. With the window between the snapshot and its publication widened to 5ms it fails 5 runs of 5, with the same message and the same 60 seconds CI reports; with that delay still in and the publication ordered, 0 of 10.
…whether the kernel has reaped it kill(pid, 0) succeeds on a zombie, so the test was asking for the whole group to be reaped at the instant NewSession returns. The connector promises something else, and something narrower: ConfirmGroupGone waits until no member of the group is running, and a zombie runs nothing. Reaping what is left of an orphan belongs to whoever adopted it — the agent's child outlives the agent it was started by, so its parent is init or a subreaper, not this process — and that wait is nobody's to promise. Gone is now asked the way the connector asks it, through driver.ProcessGone, and about processes identified while they were still running: a pid on its own is one the kernel may have given away by the time the question is asked. The assertion says which of the two was still there. Run under a subreaper that adopts orphans and never waits for them, the previous check fails all 4 runs, in 12.03 seconds and with the message CI reports; this one passes 3 runs of 3 under the same subreaper.
…han racing it The fake Codex writes the policy the driver judges, then names its thread, then says what the turn did. The driver starts the policy check at the thread and, finding the policy unsafe, kills the fake's process group at once — sometimes between the thread and the denial the test reads for. The refusal is then never written, and both the ledger and the result are empty. Nothing is lost by the connector: the session made no refusal to lose. The fake now holds the policy record back until its events are written, so the verdict cannot be reached before the denial is on the stream. The ordering is made rather than waited for, and no timeout is widened. The test also pins the verdict to the policy the fake applied. A check that gave up waiting for a record reads as ErrUnsafeMode too, and only one of the two is what this test is about. Under 128-way CPU load the old ordering fails 30 runs of 100, and 29 of 100 under -race; the new one, 0 of 200 and 0 of 100.
Contributor
There was a problem hiding this comment.
🟢 Approved
The synchronization changes are narrowly scoped and the identified race conditions have deterministic regression coverage.
Pull request overview
Fixes four CI-only driver races across Codex and ACP.
Changes:
- Synchronizes Codex update emission and channel closure.
- Makes Codex and ACP test fixtures publish state deterministically.
- Uses process identity-aware termination checks.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/connector/driver/codex/fake_test.go |
Orders policy publication after events. |
internal/connector/driver/codex/codex.go |
Prevents sends after update-channel closure. |
internal/connector/driver/codex/codex_test.go |
Adds deterministic refusal regression coverage. |
internal/connector/driver/acp/fakeagent_test.go |
Serializes record snapshot publication. |
internal/connector/driver/acp/acp_test.go |
Improves diagnostics and process-exit assertions. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Four failures in the two driver packages, all of them only on CI: one of them a crash in the connector, and three checks that read at a moment the thing they check is not yet due. They are four different causes, not one, and the durations say so — 60 seconds, 12 seconds, a panic, and 0.04 seconds are not four faces of the same mistake.
Originally tracked in Two driver tests fail on CI and never locally. The two carded ones were found while root-causing the flake fixed in The recovery harness reads a worker's word once the worker has said it; the other two turned up in the CI logs while chasing them.
A refusal emitted after the reader has gone crashed the connector
This is the only one that is a defect in shipping code, and it is the only one that can hurt anyone outside a test run.
A Codex session's reader closes the channel it publishes updates on when the worker's output ends. It is not the only goroutine that writes to it. A prompt's writer finishing a canceled turn, and the policy check ending an unsafe session, each read the refusals Codex logged on its way out and emit an update for every one — from goroutines of their own, either of which can reach that after the reader has gone. A send on a closed channel is a panic, and the connector took one on CI.
The close and the sends now take the same lock, and an update emitted after the close is dropped — which is what already happens to an update that arrives when nobody is reading fast enough. Nothing is lost that was not already best effort: a refusal reaches the ledger before its update is offered.
The fake ACP agent could unsay what it had said
TestAFloodOfPermissionRequestsIsBoundedsends sixty permission requests at once and waits for the fifty-two that are not stuck at the policy to be answered. The agent answers each on a goroutine of its own, and each writes the whole record to disk once it has its outcome. Two of those interleave: one takes its snapshot, a later one takes a fuller snapshot and renames it into place, and then the first renames its older one over the top. Nothing writes again to correct it, and fifty-two is exactly how many are ever answered, so losing one publication loses the whole wait — which is why this one spends its full sixty seconds and reports that the condition was never satisfied. Taking the word "never" literally was the thing that cracked it.The snapshot and its rename are one step now, so the record on disk is never older than a record already published.
A zombie is not a process the connector has to bury
TestAFailedHandshakeLeavesNoGroupBehindaskedkill(pid, 0)and read anything but "no such process" as the group still being there. A zombie answers that call as though it were alive. What the connector actually promises — and waits for, before it settles an attempt — is that no member of the group is still running; reaping what is left of an orphan belongs to whoever adopted it, which for the agent's child is init or a subreaper and not this process, and no amount of waiting by the connector can make that happen sooner.So the test asked for something nobody had promised. It now asks the way the connector asks, through the same
ProcessGonethe one-owner rule uses, and about processes identified while they were still running — a pid on its own is one the kernel may have given away by the time you ask about it.A session killed for its policy, before it could say what it refused
TestAnUnsafeSessionStillReportsItsRefusalswants a session that made a refusal and was then stopped for running under a policy it was not asked to run under. The fake wrote the policy first, then named its thread, then said what the turn did — and the driver, which starts its check at the thread, found the policy unsafe and killed the fake's process group in between. The denial was never written. That is why this one fails in 0.04 seconds rather than spending a budget: there is nothing to wait for.Nothing is lost by the connector here — the session made no refusal to lose. The fake now holds the policy record back until its events are written, so the verdict cannot be reached before the denial is on the stream. The ordering is made rather than waited for; no timeout is widened.
Evidence
Each behaviour change red first, then green, on this machine:
-race+ loadThe failed-handshake and flood reproductions each fail with CI's own message and CI's own duration, which is the part that says they are the same failure and not a lookalike.
TestCloseEndsTheWholeProcessGroupasks the same zombie question, and fails under the same subreaper. It is left alone: it waits up to ten seconds for the pid to go, so an ordinary reaper has all the time it needs, and CI has never reported it.go build ./...,go vet ./...,gofmt -l .,golangci-lint run --build-tags dev ./...andBASECAMP_NO_KEYRING=1 go test -tags dev ./...are all clean.