fix(codex-bridge): exit when another writer owns the thread (#906 link 1) - #935
Merged
Conversation
This was referenced Aug 21, 2026
…oceed 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).
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.
…l 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.
fujibee
force-pushed
the
fix/bridge-die-on-active-writer
branch
from
August 22, 2026 01:34
a28ef47 to
61dc8fb
Compare
fujibee
added a commit
that referenced
this pull request
Aug 22, 2026
…e exit Three safety holes raised in static review, all the wrong-kill class: (1) The match tested only that PROJECT and each pair value appear somewhere in the argv as a whole element, not that they follow --project / --pair, and it accepted a candidate whose pairs are a SUPERSET of this role's. So a --workspace-root equal to the project, or an alice+bob bridge, was a target for an alice reaper. The match now reads the argv in order (NUL-delimited /proc, else ps), takes the value AFTER --project and after each --pair, and requires the project to match and the pair SET to be equal -- not a subset. (2) The ps fallback bounded tokens with spaces but still killed a space-containing project, which ps cannot tell from two arguments, so a project that is a space-prefix of another could match. It now refuses (and the caller spawns instead) when a value it would match on contains a space -- ps cannot reconstruct that boundary. A tab inside the pair separator is not a space and is kept. (3) The kill did not wait for the target to exit before spawning. The real bridge shuts down async on SIGTERM and holds its thread as writer until it does, so spawning immediately makes the new bridge lose thread/resume to the dying one (active-writer, exit 1, #935) and the old then exits too, leaving zero. The reaper now waits (bounded) for kill -0 to go false and, if a target will not exit, does NOT spawn this tick. Tests add the fixtures these need and assert SURVIVAL: a --workspace-root equal to the project, a pair-superset (alice+bob) bridge, a space- containing project on the ps path, and no duplicate spawned beside a bridge still shutting down. Full launcher suite green. Part of #906 (link 2). Follows review on #943.
fujibee
added a commit
that referenced
this pull request
Aug 22, 2026
… -- wait on positive proof only (#906 link 2) The previous fix over-corrected: it read a failed `_start_token` as "pid dead" and ended the wait. But that helper returns non-zero for a transient /proc or ps read failure too, not only for a gone pid, so a single bad observation would let the launcher spawn a replacement while the killed bridge still held the thread as writer -- reopening the #935 race the wait was added to close. Same shape as the bare kill -0 it replaced, just inverted: an observation FAILURE was read as STATE. Encode the rule and follow it: a failed observation proves nothing, and kill and spawn each need their own positive proof (on any failure, do nothing; the timeout bounds the wait). The wait now ends only on proof of EXIT -- either _agmsg_pid_alive_local reporting the pid ABSENT (the sanctioned helper: EPERM-aware, ps-cross-checked, erring to "alive" on ambiguity, so a false return is real absence; and the one liveness path allowed kill -0), or a start token that reads and now names a DIFFERENT process. A token that merely could not be read keeps the wait going, and no proof within the budget returns 1 (do not spawn).
fujibee
added a commit
that referenced
this pull request
Aug 22, 2026
…the reap wait; token replacement is the only exit proof (#906 link 2) _agmsg_pid_alive_local's false return is not a trustworthy absence proof: its ps cross-check pipeline has no pipefail, so a transient ps failure yields an empty stat and the helper returns "gone" (tracked separately as #954 -- it is a defect in the shared helper, 31 call sites, not something to change from inside this PR). Consulting it in the wait let a single flaky observation read as exit and spawn a replacement while the killed bridge still held the thread as writer (#935). Remove that consult. The only exit proof the reap wait trusts is a start token that reads and now names a DIFFERENT process (a replacement). A token that reads unchanged keeps waiting; a token that cannot be read proves nothing and also keeps waiting. A normal exit therefore falls through to the timeout and returns 1 -- we do not spawn this pass. The cost is one cycle, not a stall: the next pass no longer sees the exited pid in pgrep and spawns then. Fail-closed, as ruled.
fujibee
added a commit
that referenced
this pull request
Aug 22, 2026
… reuse-safe reaper do it and refuse to spawn beside a live one (#906 link 2) The deferred rebind kill still could not satisfy the fail-closed rule on the spawn side. Even token-guarded, an unreadable token skipped the kill but let the rm + spawn proceed, double-starting an old writer that was merely unobservable, not gone; and a token-matched kill did not wait for proof of exit before spawning, racing a bridge that holds the thread as writer through its async shutdown (#935). The deeper problem: the pidfile stores a bare pid with no identity, so no kill built on it can be made reuse-safe. So stop killing from the pidfile entirely. The lease-based reaper (which runs just above, matches on the identity hashes, re-checks the start token, and returns non-zero when it cannot prove a target exited) is the only reuse-safe path allowed to signal a same-identity bridge. If a mismatched-live bridge is somehow still alive after it -- the reaper spared a lease-less legacy bridge, or one is mid-shutdown -- do nothing this tick: keep the binding and retry, never spawning beside it. A lease-less bridge that never dies just lingers; orphan survival is acceptable, a wrong-kill or a double-start is not.
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.
Part of #906. On
main.The link
#906 reports a self-amplifying bridge leak that saturated an HPC login node's per-user
TasksMaxtwice in one day. The trigger it names first: with codex delivery modemonitor, whenthread/resumefails withalready has an active writer(JSON-RPC -32600 -- e.g. the role's thread is concurrently open in Codex Desktop), the bridge logsproceeding without resumeand keeps running. A bridge that cannot own its thread still arms watchers and holds ~10 threads, so the launcher's replacements accumulate instead of one converging.Why the catch swallowed it
ensureThread'scatchwas added for a genuinely benign failure (#276): a Codex 0.142+--remotesession may never create a rollout, sothread/resumefails outright, butturn/startneeds only thethreadId, so the bridge stays alive idle. That was right for that case. It just also caught the deterministic one -- "another writer owns this thread" -- which needs the opposite handling: retrying or proceeding cannot help while the other writer holds the thread.Two distinguishable things were sharing one branch.
The change (
codex-bridge.js)(1) Both JSON-RPC clients (
AppServerClient,WebSocketAppServerClient) rejected withnew Error(message.error.message)and dropped the error code. They now attachcodeto the rejected error.(2) The
ensureThreadcatch matches thealready has an active writermessage anddie()s (exit 1). It matches the message -- the condition itself -- rather than the bare -32600 code, which JSON-RPC also uses for other invalid requests; the code is carried for diagnostics. Every other resume failure keeps the benignproceeding without resume, unchanged.This does not by itself stop the launcher from respawning a bridge that keeps dying, or add re-arm backoff, or qualify PIDs across an NFS-shared
$HOME-- those are the other links of #906 and are separate.Verification
tests/test_codex_bridge.bats+1: a fake WS app-server answersthread/resumewith the-32600/already has an active writererror; the bridge exits non-zero, says why, and never logsarmedor reachesprocess/spawn-- i.e. it does not become one of the accumulating bridges.test_codex_bridge.bats43/43 local.