Make the fd-3 spawn rule uniform and enforce it - #565
Merged
Conversation
fujibee
force-pushed
the
fix/codex-monitor-fd-guard
branch
from
July 30, 2026 16:18
74e0ec8 to
411fe55
Compare
fujibee
changed the title
fujibee
force-pushed
the
fix/codex-monitor-fd-guard
branch
from
July 30, 2026 22:05
411fe55 to
041c92f
Compare
The rule was already applied in four places -- the sync engine in remote.sh, _session-start.sh, and codex-bridge-launcher.sh, which also closes both fds once at the top with exec. remote.sh is where the reasoning is written down: under bats, fd 3 is the TAP pipe, and a daemon inheriting it holds the whole test file open. codex-monitor.sh was the gap, and the worst place for one: its app-server is built to outlive its caller. Its two spawns close both fds now, as do the bounded-copy pair in remote.sh -- those are reaped on every normal path, but the EXIT trap only removes files, so a signal before curl opens the fifo leaves the copier blocked in open() forever. The test requires the guard on the spawn line itself, with no file-level exemption for a script-wide exec. Nothing depends on one, and a check for "this file contains an exec somewhere" would accept it inside a function, inside a branch that never runs, or after the spawn it claims to cover. Both that decoy and a spawn closing only fd 3 are pinned as failures, because earlier revisions of this test let each of them pass. This makes no claim about the CI hang. The shard that hung on this very PR reaches none of the spawns changed here.
fujibee
force-pushed
the
fix/codex-monitor-fd-guard
branch
from
July 30, 2026 22:10
041c92f to
bc2306b
Compare
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
1 participant
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.
This makes no claim about the CI hang. It was written while chasing one, and the chase failed — see the last section. What is left is a rule this repository already had, applied everywhere instead of at one call site, and enforced by a test instead of a comment.
The rule
remote.sh, where it starts the sync engine:That reasoning was written down once and applied once.
codex-monitor.shstarts two processes that outlive their caller without it:app-server— a daemon designed to persist: it writes a pidfile, a portfile and a version file, and its tests are named "reuses a live app-server" and "recreates a stale app-server".watch.sh's interval sleep is bounded and its parent is already guarded, so it was never at risk; it is included to make the rule uniform, because the alternative is deciding per site which daemons are "long-lived enough to matter" — and that judgement is precisely what left the app-server unguarded a few files away from the comment explaining why it should not be.The two
bounded-copyspawnsReaped on both normal paths — waited on success, killed and waited on failure — so they are short by construction. But the
EXITtrap only removes files; it does not kill the copier. A signal arriving before curl opens the fifo leaves it blocked inopen()with no writer ever coming. Narrow, but it is the one path where a bounded spawn stops being bounded, so they are guarded and the reasoning sits at the call site.Enforcement
tests/test_spawn_fd_guard.batsasserts that no background spawn underscripts/is missing the guard, and names the offending line when one is. A second case asserts the pattern still matches at least five spawns, so a pattern that quietly stops matching cannot leave a test that passes by finding nothing. Removing any single guard fails it.Why the hang claim is gone
This PR was opened believing the unguarded
app-serverexplained shards that report every testokand then sit silent until the job's 25-minute cap.Its own CI refuted that.
bats (macos-latest 4/4)hung here, with these guards in place, on a shard whose thirteen files include no test that reaches any spawn this PR touches —codex-monitor.shis not executed there at all. Every spawn reachable in that shard, inscripts/and in the tests, was already guarded.So the fd-3 inheritance story does not explain these jobs, and the correlation it rested on (hanging shards tending to contain a codex-reaching test) now has counterexamples including this one. The hang is being investigated separately, with diagnostics rather than another guess.
249 green across the suites the diff reaches.