fix(codex-bridge): make the failure cap reach and bound the re-arm rate (#906 link 3) - #941
Merged
Conversation
The consecutive-watch-once-failure cap (default 3) already exists (#209), so #906's "re-arm has no backoff" is not the whole story. Reproduced against the real bridge with a fake app-server, four scenarios: (a) every watch-once fails -> cap fires at 3 (already works) (b) fail, fail, wake, repeating -> cap NEVER reached: the wake reset the counter to 0, so a bridge that kept delivering also kept failing, forever (c) a flood of distinct wakes -> 2094 arms in 56 s (37/s), no throttle -- each arm forks watch-once's library sourcing, the fork pressure #906 saturated (d) a flood of one repeated wake -> stale-wake guard stops it (ok) Two ceilings, so both (b) and (c) are bounded and the next reader sees them as one subject rather than assuming the cap "was fixed": (1) A wake decays the failure counter by one instead of resetting it to zero. A wake is progress, not proof the host recovered; forgiving one failure per wake lets a genuinely-recovered bridge fall to zero while a fail/fail/wake churn still climbs to the cap. A clean deadline (exit 2) -- a full timeout that ran end to end -- is the stronger signal and still resets outright. (2) A minimum interval between arms (MIN_ARM_INTERVAL_MS), on armWatch, the one path every re-arm goes through. watch-once's own deadline paces the healthy case at one arm per --timeout, so the floor is only ever felt by a degenerate loop. After the fix, re-measured: (a) still 3 arms/stop, (b) now stops at ~7 arms, (c) ~1 arm/s (26 in 25 s, was 37/s), (d) still guarded. Tests pin (b) and (c) against the real bridge. Part of #906 (link 3).
…st [[ ]] The two message checks were `[[ "$output" =~ ... ]]` in a non-last position, which cannot fail on macOS bash 3.2 -- the enforced-assertions guard (#670) flags exactly that. Use `grep -q ... <<<"$output"`, a plain command that trips errexit on both shells.
fujibee
added a commit
that referenced
this pull request
Aug 22, 2026
…k 1) (#935) * fix(codex-bridge): exit when another writer owns the thread, don't proceed The thread/resume catch was written to tolerate a benign failure -- a Codex 0.142+ --remote session that never created a rollout, where turn/start still works from the threadId alone, so the bridge stays alive idle. But it swallowed a deterministic one too: "already has an active writer" (-32600) means another writer -- a co-resident Codex Desktop, or a second bridge -- owns this thread, and resume cannot succeed while that holds. A bridge that proceeds anyway still arms watchers and holds ~10 threads, so on a host with a per-user pid limit duplicates accumulate until the slice is saturated and every fork of every process of the user fails (#906, two incidents in one day on an HPC login node). Now the two are told apart. The JSON-RPC clients carried only message.error.message and dropped the code; both now attach it. The catch matches the "already has an active writer" message -- the condition itself, not the generic -32600 code, which is also returned for other invalid requests -- and die()s, so a bridge that cannot own its thread stops instead of lingering. Every other resume failure keeps the benign proceed-without-resume, unchanged (the #276 test still passes). Part of #906 (link 1 of the reported chain; the launcher orphan scan, re-arm backoff, and cross-host PID qualification are separate). * docs(codex-bridge): the resume-error code is diagnostic, not the gate Address a review nit: the client comment implied ensureThread needs the code to distinguish the active-writer case, but it decides on the message. Say the code is carried for diagnostics and future callers, not for that branch. * test(codex-bridge): assert the active-writer case with forms that fail on 3.2 The three checks were `[[ ]]` in non-last positions, which report ok with a false claim inside on macOS bash 3.2 -- the shell CI runs. So on the macOS shards those assertions were no-ops. Use grep for the positive and `[ "$(grep -c ...)" -eq 0 ]` for the negatives, which fail on both shells. No logic change. * test(codex-bridge): restore the brace dropped rebasing onto #941's tests Rebasing this branch onto main after #941 landed put #941's two #936 tests just before this branch's #906 test in the same file; resolving that conflict dropped the closing brace of #941's second test, leaving the file unparseable. Restore it. Tests only; no logic change.
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.
Part of #906 (link 3). On
main.The measurement first
#906 lists "re-arm loop has no backoff" as a root cause. Measured against the real bridge (a fake WS app-server whose
process/spawnexit codes are scripted), that is only half true -- the consecutive-failure cap (--max-failures, default 3) already exists (#209) and fires. Four scenarios, before this PR:stopping after 3 consecutive, exit 1So the incident's "~80 armed from one PID that never gives up" is (b): the cap is real but never reaches, because any wake resets it. And (c) is a latent second hole: the wake -> re-arm path has no rate floor at all. The fork storm the reporter saw (
800 PIDs/s) is each armed watch-once forking its own library sourcing under pressure; the re-arm loop does not create that rate, it just restarts it forever -- which is why making the cap reach (b) is the direct fix, and bounding the rate (c) is the belt-and-suspenders.The change (
codex-bridge.js), two ceilings in one place each(1) A wake decays the failure counter by one instead of resetting it to zero. A wake is progress, not proof the host recovered -- amid failures it is exactly the (b) churn. Forgiving one failure per wake lets a genuinely-recovered bridge (mostly wakes) fall to zero, while a fail/fail/wake churn nets +1 per cycle and reaches the cap. A clean deadline (exit 2 -- a full
--timeoutthat ran end to end) is the stronger signal and still resets outright.(2) A minimum interval between arms (
MIN_ARM_INTERVAL_MS, 1 s), enforced inarmWatch-- the one path every re-arm goes through (a clean deadline, a wake and its turn, an idle transition). watch-once's own deadline paces the healthy case at one arm per--timeout(300 s), so the floor is only ever felt by a degenerate loop; a deferred arm still runs, nothing is dropped.Both are ceilings on "how fast / how forgiving," kept together so the next reader sees (b) and (c) as one subject rather than reading "the cap was fixed" and forgetting (c).
Not done here: widening the stale-wake guard from "one repeated max_id" to "N wakes in a window" -- (d) shows the guard works as specified, and (2) already bounds the distinct-wake flood, so it is a separate judgment, not this PR.
After the fix, re-measured (same harness)
Verification
tests/test_codex_bridge.bats+2, driving the real bridge against the scripted fake: (b) the failure cap reaches even when wakes interleave (the bridge stops itself), and (c) a flood of distinct wakes is rate-limited (arm count over ~6 s stays in single digits, where the unbounded loop produced hundreds). The two unchanged scenarios (a)/(d) are already covered by existing tests and the failure-cap test above.test_codex_bridge.bats44/44 local.