Repository navigation
Description
Follow-up from the local review of #46 (both findings raised by the Codex reviewer at high confidence, then triaged as pre-existing rather than introduced by that PR). Neither is a regression from #46 — #46 narrows the gap — but both are real and share one root: the foreground cancellation path is not bounded end-to-end against the host's ~120s Bash SIGKILL.
Related: #42 (pre-existing orphan chain — broker socket-close, /cancel reconciliation). This is the client-side half of the same problem.
Finding 1: the hard ceiling is armed after an unbounded setup phase
captureTurn arms the wall-clock ceiling at lib/codex.mjs:747, but it is called at the end of runAppServerTurn — after withAppServer → connect, startThread/thread/resume, assertExplicitSandboxHonored and validateReasoningSelection. The 110s foreground ceiling is therefore measured from turn-start, not from process entry.
The handshake portion is bounded and additive on fallback: BROKER_CONNECT_TIMEOUT_MS=2000 + BROKER_INITIALIZE_TIMEOUT_MS=5000, then SPAWNED_INITIALIZE_TIMEOUT_MS=10000 (app-server.mjs:32-34) — up to ~17s.
Worse, the RPCs after it are unbounded. AppServerClientBase.request (app-server.mjs:101-131) arms a timer only when the caller passes timeoutMs, and the only callers that do are the two initialize paths (:269, :384). So thread/start, thread/resume and validateReasoningSelection have no deadline at all.
Realistically 110s + 1-3s setup ≈ 113s, inside the ~120s kill. In the broker-fallback or slow-thread/start tail it exceeds it, and with a hung thread/resume it exceeds it arbitrarily.
Note this property is not new — pre-#41 (c33cf49^:lib/codex.mjs:645-650) armed its 110s total budget at the identical point.
Finding 2: turn/interrupt itself has no timeout
interruptAppServerTurn (lib/codex.mjs:1180-1223) opens a fresh connection and then issues client.request("turn/interrupt", …) with no timeoutMs (:1206). The connect phase is bounded by the constants above; the request is not.
It is awaited inside captureTurn's catch (lib/codex.mjs:805-809), i.e. in a window that by construction has ~10s of headroom left. If the broker accepts the connection and then never replies while staying alive, the companion blocks until SIGKILL and the original write-capable turn keeps running with nobody able to reach it.
The preconditions are narrow (peer must accept, stay alive, and go silent), and the code is byte-identical to ed9b4e3 — but the cancellation guarantee is only as strong as its slowest link.
Why this was not fixed in #46
At #46's base the foreground had no wall-clock bound at all (#41 left the ceiling a fixed 45 min), so a delta-producing foreground turn was SIGKILLed with no interrupt attempted whatsoever. #46 converts that into "an interrupt is attempted inside the kill window" — strictly better. And the catch containing the unbounded interrupt was already reachable at base at the same ~110s via the idle deadline, so #46 adds a second trigger into an already-reachable path, not new reachability.
Fixing it properly means changing the deadline's anchor point, which is a design change well beyond that PR's scope.
Proposed fix, cheapest first
- Pass
timeoutMs(3-5s) to theturn/interruptrequest atlib/codex.mjs:1206. One line, closes the unbounded-cleanup hole, highest value. - Anchor an absolute deadline at command entry and subtract elapsed setup before arming the ceiling, reserving an explicit cleanup margin for the interrupt rather than relying on the implicit ~10s.
- Bound the
thread/start/thread/resume/validateReasoningSelectionRPCs so setup cannot silently consume the whole budget. - Consider escalating to broker termination when a confirmed cancellation cannot be obtained in the remaining margin.
Test plan
- Fixture where
turn/interruptis accepted but never answered → assert the companion still exits bounded, and does not sit until SIGKILL. - Fixture with a deliberately slow
thread/start→ assert the ceiling still fires within the host margin (i.e. the deadline accounts for setup). - Both are red pre-fix.
Also deferred from #46's review (minor)
- The idle deadline reports
codex turn exceeded the {n}ms turn budget.while the ceiling reports…{n}ms hard wall-clock ceiling.; in the foreground both numbers are now110000, so they differ only by the trailing phrase. Renaming the first to "idle budget" is clearer, but three tests assert on/turn budget/i(runtime.test.mjs:3203,3253,3422) and would need updating together. - No test covers that
--backgroundresolves the ceiling to the 45-minute default (the foreground case is covered twice). Needs a seam — the resolver is module-private and the value is not in user-facing output.
Activity
fix: bound the broker client's close() against a peer that ignores FIN