fix(codex-bridge-launcher): reap a same-(project,role) orphan before spawning (#906 link 2) - #943
Merged
Conversation
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.
…ntity lease (#906 link 2) A launcher whose sibling bridge is live but whose pidfile was removed would spawn a second bridge for the same (project, role) and never converge. The launcher now reaps such an orphan before spawning, but identifying "same (project, role)" by reconstructing the target's argv from `ps` is fragile: a TAB or space inside a project path or role name, macOS `ps` escaping, and glob/word-split in the reconstruction each let one identity read as another. Replace argv reconstruction with an identity lease the bridge itself publishes: - codex-bridge.js writes `codex-bridge-lease.<pid>` atomically (temp + rename) from writeMeta(), BEFORE the app-server client starts, so a bridge is reapable the instant it exists. It stores SHA-1 hashes of the project and of the sorted pair set (order-independent, separator-proof), the host, the pid, and the process start time. Cleanup on exit removes only our own lease, gated on pid + start token so a recycled pid's new owner is never touched. - The launcher reaper enumerates live codex-bridge.js pids (pgrep, liveness only), reads each lease, and kills only when v=1 with every field present, the host matches, and the project and pair-set hashes match its own. It re-checks the host and `ps -o lstart=` start token immediately before the kill to reject a recycled pid or another machine, waits for exit (bounded), and refuses to spawn if a target will not exit. A live bridge with no lease (legacy) is left alone. - Spawns are throttled by a per-key mkdir lock around a windowed stamp file so a crash-respawn storm cannot fork unbounded bridges while converging. The argv parser is removed entirely. Reader is fail-closed on any malformed, truncated, wrong-host, or hash-mismatched lease.
fujibee
force-pushed
the
fix/launcher-orphan-scan
branch
from
August 22, 2026 06:10
2dbebc2 to
92fde54
Compare
Owner
Author
|
Reworked: the reap no longer identifies a target by reconstructing its argv from Instead the bridge now publishes a per-PID identity lease that the launcher reads: (1) (2) The launcher reaper enumerates live (3) Cleanup removes only our own lease, gated on pid + start token, so a recycled pid's new owner is never touched. (4) Spawns are throttled by a per-key The argv parser is removed entirely; the reader is fail-closed on any malformed, truncated, wrong-host, or hash-mismatched lease. All 22 launcher tests pass locally; the enforced-assertions check is at baseline (638). |
…iewed failures (#906 link 2) Address the safety findings on the lease reaper: (1) Remove two unconditional debug appends to a fixed /tmp path from the reaper -- they wrote on every launcher poll and were never observed by the tests (the tests assert lease content, not side effects). (2) Rate-limit correctly. The spawn reservation now keys on the full identity (project hash + pair-set hash), not the role-only bridge_key, so two projects sharing a role no longer share a cap. When the per-identity mkdir lock cannot be taken, THROTTLE (return 1, do not spawn) instead of proceeding lock-free -- the concurrent reservation is exactly the overshoot the lock exists to stop. Only the lock owner mutates the stamp file and rmdir's. A lock left by a crash is reclaimed only once provably stale (older than the sub-second critical section by a wide margin), and reclamation reads mtime GNU-first so Linux does not silently fail to reclaim. (3) Publish-or-die at the source. writeLease now throws on an empty start token, a missing hostname, or a failed write/rename; run() publishes the lease before client.start(), so the throw aborts startup rather than leaving a live bridge with no authority behind it -- the very orphan this is meant to prevent. (4) Parse the lease under an exact v=1 schema: exactly the seven expected keys, each once, no unknown/duplicate/extra line, 40-hex hashes, numeric pid, startsrc in {proc,ps}. Anything else fails closed (no kill). Adds survive tests for truncated, foreign-host, unknown-key, and duplicated-key leases. (5) Make the reuse guard lossless where the platform allows. The start token is /proc/<pid>/stat field 22 (clock ticks) on Linux -- so a recycled pid is always distinguishable -- and `ps -o lstart=` (second precision) elsewhere; the launcher and the bridge compute it the same way. The residual on a second-precision platform is a pid reused within the same second, in the sub-ms window before the new occupant overwrites its own lease, whose only possible victim is a same-(project,pair) bridge that convergence re-spawns.
…he pair hash locale-proof (#906 link 2) Follow-up to the lease review: (1) The spawn lock is no longer reclaimed on age alone. A live holder can stall past any timeout (SIGSTOP, scheduler, NFS I/O), and reclaiming its lock let two owners update the stamp file and overshoot the cap. The lock is now an atomic exclusive file (created via ln(2), so a reader never sees an empty one) whose content names the owner: host, pid, and the same start token the lease uses. It is reclaimed ONLY when its owner is on this host AND provably gone -- the pid is dead, or recycled (start token changed). A foreign-host, malformed, empty, or still-live owner throttles (return 1), never reclaims. (2) The stamp reservation is written temp + atomic rename, and any write or rename failure releases the lock and returns 1 rather than counting as a reservation and spawning unreserved (the function runs under set +e, so a bare printf failure would otherwise fall through to return 0). rename also keeps a crash mid-write from truncating the existing stamps to nothing. (4) The pair-set hash canonicalizes before hashing: each "team<TAB>name" pair is hashed, the hex hashes are sorted (pure ASCII, so a byte sort in the launcher and a code-unit sort in the bridge agree), then the joined list is hashed. A locale/Unicode sort-order gap can no longer make the same pair set hash differently on the two sides. Bridge, launcher, and the test mock match.
…elf-expiring markers (#906 link 2) The lock reclaim could not be made safe. Proving the owner is gone and then unlinking the lock is not atomic, so a reclaimer that read an old owner can delete a live lock a second launcher has since acquired at the same path (ABA), and a transient failure to read the owner's start token read as "owner gone". Three rounds of hardening each surfaced another way to misjudge the owner -- the same "identify the wrong thing" shape the reap itself kept hitting. So stop reclaiming. Each spawn now reserves by creating its own uniquely named marker file whose timestamp is IN THE NAME (an exclusive noclobber create, so two launchers never share one, and the file is empty so there is no create-then-write window to misread). The cap is just the count of unexpired markers; reserve-then-count makes a concurrent pair both back off, so the cap is never exceeded. Nothing is ever reclaimed: a crashed launcher's marker ages out of the window and any later pass prunes it. No lock, no owner check, no ABA. Also add a reuse-guard survive test: a lease whose recorded start token no longer matches the live pid's is left alone -- the fail-closed default the reap decision now turns on (a recycled pid, an unrelated bridge, an unreadable token: no kill).
…as the reap wait's liveness check (#906 link 2) A repo guard forbids shipped scripts from deciding liveness with a bare kill -0: it answers "this pid exists", not "this pid is the process I mean", so a recycled pid reads as alive -- the very hole the reap's kill decision is fail-closed against. The reap's post-kill wait loop still used it. Fold liveness into the identity read instead. A dead pid yields no start token (no /proc entry, no ps row), so `_start_token` succeeding is itself proof the pid is live, and its value being unchanged is proof it is still the same process -- both from one observation, with no window between a separate liveness check and the token read for a recycled pid to slip through, and none of kill -0's EPERM blind spot. The wait ends when the token stops reading or changes.
… -- 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).
…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.
… to spawn, so a gated tick never wipes the recorded binding (#906 link 2, #350) The rate and reap gates were inserted between two halves of one action: the rebind/dead-bridge path wiped the recorded binding (pidfile, app-server, thread files) and THEN, several lines later, the spawn rewrote them. A gate that bailed out in between -- a rate throttle, or a reap that could not prove exit and returned 1 -- left the binding wiped and never rewritten, so the #350 bound-thread record vanished until some later tick happened to spawn. On a mock that exits the instant it starts (so every tick re-enters the spawn path) this turned a rare pre-existing flake into a near-deterministic failure of the bound-thread test. Restore the invariant that the binding is wiped only when its replacement is about to be written: hold the mismatched pid in need_kill, run the gates while the old binding is still intact, and do the kill + rm immediately before the spawn. A throttled or proof-less tick now changes nothing and the next tick still has the binding to reuse or rebind from.
… kill, so a reused pid is never killed (#906 link 2) Deferring the rebind teardown past the gates fixed the wiped-binding bug but left a bare `kill $need_kill` of a raw pid. The gates can take seconds (the reap wait in particular), during which that pid can exit and be reused -- and the kill would then land on an unrelated process. Same wrong-kill class the reap decision is already fail-closed against. Carry the pid together with its start token, captured the moment we chose it (when the pidfile still proved it was our bridge). Just before the kill, re-read the token and fire only when it still reads and matches; unreadable or changed means a different or gone process, so do nothing. This also lets a legacy (lease-less) mismatched bridge still be retired -- the reaper spares it for want of a lease, but here we hold its token directly -- while never killing a recycled pid.
… 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.
…s start token, not by _agmsg_pid_alive (#906 link 2) The spawn guard used _agmsg_pid_alive's false as proof the old writer was gone. But that helper's false can be a transient ps failure (#954), i.e. a failed observation, not proof -- so a flaky read fell through to rm + spawn and double-started, the same trap relocated from the reap wait to the spawn guard. Prove it with the start token instead, the way the reap wait does. Stash the mismatched pid's start token at detection; after the gates, spawn only when the token now reads and DIFFERS -- the old pid hosts a different process, so the old writer is gone. A token that still matches (alive), or cannot be read (unproven), keeps the binding and retries; a normal exit is caught next tick by the existing dead-pid path at the top of the loop. No _agmsg_pid_alive in the new gate-after decision, no direct kill; a failed observation never authorises a spawn.
… does not also count 'alice2' (#937) _count_role_bridges matched the role name as a substring, so a count for "alice" also counted the "alice2" bridge the survive tests deliberately run alongside it. With both alive the steady-state count is 2, and the assertion `== 1` only passed by catching a transient window where the launcher's own bridge was momentarily down mid-reap. The reap wait now takes a bounded moment (it waits for proof of exit), which shifts that window and, on the slower CI runner, misses it -- so the tests failed for a timing reason while the reap logic was correct all along. Match the name at a word boundary (a trailing digit/letter excludes it), so the count means "bridges serving exactly this role" and the assertion holds in steady state regardless of reap timing. Verified: with the fix, the alice2 and pair- superset survive tests pass even with a 5s delay injected at the reap point, the condition that reproduced the failure.
Closed
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 (link 2). On
main.The leak
codex-bridge-launcher.sh's reuse check trusts only the pidfile PID: if the recorded PID is alive and bound to the current app-server/thread it reuses that bridge, otherwise it removes the pidfile and spawns a new one. What it never does is look for a LIVE bridge for this same (project, role) that the pidfile does not name — and that bridge exists whenever the pidfile was removed while the process stayed alive:kill … || true) on a mismatch, e.g. a cross-host PID over an NFS-shared$HOME(codex monitor mode: duplicate-bridge leak saturates the user's TasksMax (two incidents in one day) — bridge survives "active writer" failure, launcher respawns without orphan scan, re-arm loop has no backoff #906, Bridge run-state uses bare PIDs on an NFS-shared $HOME, so two login nodes collide #938), or a race;The launcher then spawns a duplicate beside the orphan. On a host with a per-user pid limit those accumulate until the slice saturates (#906, two incidents in one day).
Reproduced against the real launcher (the mock bridge in
tests/test_codex_bridge_launcher.bats): launch a bridge, remove its pidfile while it stays alive, and the polling child spawns a second — two live bridges for one role.The change
Before the spawn, reap any live codex-bridge for THIS exact (project, role), then bound how often we spawn.
Kill only, never adopt. Whether a found bridge is bound to the current app-server/thread cannot be told from the launcher (the files it would have written may be this launcher's, or gone), so a "still good" guess would keep alive exactly the stray this removes. Killing a good one costs one restart; adopting a bad one is the bug.
Exact match on BOTH
--projectand each--pair team\tname. The pidfile/run-file key is role-only (team.name), not project-scoped, so a same-role bridge in another project shares the key — matching must scope by project too, or it would kill an unrelated project's bridge. Matching reads the process argv: Linux/proc/PID/cmdline(NUL-separated, sogrep -Fxqmatches a whole argv element and a prefix cannot sneak in); elsewhereps -o args=, normalising the pair's TAB (macOS/BSDpsprints it as\011) and bounding each token with spaces sodevdoes not matchdev2. Exact where/procexists; best-effort for a project/role value that itself contains a space, whichpscannot disambiguate.Non-Windows only. Matching yields a PID that is only killable in this shell's own PID namespace, which on Windows (MSYS vs native) is the #458 mismatch, unsolved until that lands; there the scan does nothing rather than kill a PID it cannot address. If no process lister (
pgrep) is available (a minimal container), it also does nothing and the spawn proceeds — no worse than before.A spawn-rate ceiling (
_SPAWN_MAXin_SPAWN_WINDOWs, 5 in 30) bounds the remaining churn: a bridge that cannot stay up (the launcher dying before it records the pidfile) would reap-and-respawn every poll tick. A rate-limited tick changes nothing (it does not even reap), so a genuinely-stuck role backs off instead of spinning. Not a fix for the crash, a cap on its cost.Verification
tests/test_codex_bridge_launcher.bats+4, all driving the real launcher:bob) alive;alice) in another project alive;alicedoes not sweep the prefix-collidingalice2.The last three assert SURVIVAL — that the kill is scoped, not just that the orphan dies — because a too-broad kill passes an "it died" test but is the worst outcome here. Full
test_codex_bridge_launcher.batsgreen locally; no existing test regressed (the #485 dedup tests still pass).Not done here: the cross-host PID collision itself (#938 — a PID is only meaningful with a host attached) and the Windows PID-namespace mismatch (#458).