Skip to content

Turn cancellation is not bounded end-to-end: ceiling armed after unbounded setup, and turn/interrupt has no timeout #47

Closed

Description

@axisrow

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

  1. Pass timeoutMs (3-5s) to the turn/interrupt request at lib/codex.mjs:1206. One line, closes the unbounded-cleanup hole, highest value.
  2. 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.
  3. Bound the thread/start / thread/resume / validateReasoningSelection RPCs so setup cannot silently consume the whole budget.
  4. Consider escalating to broker termination when a confirmed cancellation cannot be obtained in the remaining margin.

Test plan

  • Fixture where turn/interrupt is 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 now 110000, 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 --background resolves 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions