Skip to content

fix: measure turn timeout as agent idle time, not total wall-clock - #41

Merged
axisrow merged 5 commits into
mainfrom
fix/idle-turn-timeout-40
Aug 2, 2026
Merged

fix: measure turn timeout as agent idle time, not total wall-clock#41
axisrow merged 5 commits into
mainfrom
fix/idle-turn-timeout-40

Conversation

@axisrow

@axisrow axisrow commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #40.

captureTurn's deadline (--turn-timeout-ms / CODEX_TURN_TIMEOUT_MS) currently bounds the entire turn, racing a single setTimeout armed before startRequest() against the whole lifecycle. That's the wrong metric: gpt-5.6-sol at effort=xhigh on adversarial-review routinely does real, progressing work for well over 10 minutes — it isn't stalled, it's just slow at high effort on a large diff. A fixed wall-clock budget forces an impossible choice: set it low enough to catch a genuinely dead app-server quickly and it kills legitimately long-running reviews mid-flight; set it high enough to tolerate long reviews and a truly stalled app-server now hangs just as long before surfacing.

Fix

Replace the fixed turn budget with an idle (inactivity) timeout:

  • captureTurn (plugins/codex/scripts/lib/codex.mjs): the one-shot deadline setTimeout is replaced with a resettable armIdleDeadline closure stored on the turn-capture state as state.resetIdleDeadline.
  • applyTurnNotification — the single choke point all turn notifications already flow through — calls state.resetIdleDeadline() on turn/started, item/started, and item/completed. A turn that keeps producing events can run indefinitely; only a gap of silence longer than the budget trips it.
  • applyTurnNotification also resets the deadline on in-item progress/delta notifications (item/commandExecution/outputDelta, item/fileChange/outputDelta, item/mcpToolCall/progress, item/agentMessage/delta, item/plan/delta, the reasoning deltas, command/exec/outputDelta, process/outputDelta). Without this a single long-running item — a big command execution or MCP tool call streaming output for minutes — emits no item boundary in between and is killed mid-flight despite being demonstrably alive.
  • A separate, generous wall-clock ceiling (HARD_WALL_CLOCK_CEILING_MS, 45 min) is kept as a second failsafe — a backstop against a turn that resets its own idle timer forever without ever completing (e.g. a runaway tool-call loop), independent of the tunable idle budget.
  • Comments / help text for --turn-timeout-ms updated: the meaning changes from "budget for the whole turn" to "how long the turn may go silent between events before it's considered dead."

This preserves the actual fix from upstream openai#376 (the completion await being reject-able at all — rejectCompletion was dead code — plus resolving the budget at call time instead of import time). Both are orthogonal to what the timer measures and are untouched.

Relation to other issues

Test plan

  • TDD-first: added three fixture behaviors to tests/fake-codex-fixture.mjsspaced-events-idle-ok (6 events spaced 700ms apart, ~4.2s total, each gap short), goes-silent-after-first-event (one event, then permanent silence), and single-item-progress-idle-ok (one item/started, then 6 item/commandExecution/outputDelta 700ms apart, then completion) — modeled on the existing stalled-turn-start behavior from fork PR fix: turn-timeout gaps — interrupt, early deadline, options threading (closes #27) #28.
  • tests/runtime.test.mjs, four assertions, each confirmed red-before / green-after:
  • npm test: 95 tests, 92 pass, 3 fail. The 3 failures are pre-existing status/result job-listing flakes — reproduce identically on unmodified fork/main (verified by stashing this branch's changes and re-running).
  • npm run build: passes (tsc -p tsconfig.app-server.json), no new TS errors.
  • npm run check-version: all version metadata is consistent.

Version

No version bump in this PR — the manifests stay at 1.0.6-fork.6. Suggestion for whoever cuts the next release: this is a behavioral change to a user-facing knob (--turn-timeout-ms / CODEX_TURN_TIMEOUT_MS stop bounding total turn wall-clock and start bounding agent idle time), so it likely deserves its own version rather than riding along unversioned.

Out of scope

Codex's adversarial review raised that a continuously-active foreground turn can now outlive Claude Code's Bash-tool SIGKILL ceiling (~120s), orphaning a live app-server turn that /codex:cancel cannot reach. The finding is real, but every link in that chain predates this PR — verified byte-identical against the base commit 15f22e6: the broker never issues turn/interrupt on socket close (broker-controller.mjs:353-370), /cancel filters queued|running after reconciliation flips the job to failed (job-control.mjs:326-329, state.mjs:151-196), and adversarial-review already runs foreground with the 600000ms default because it drops turnTimeoutMs (codex-companion.mjs:465, tracked as #38 / #39). Filed as #42 with the full chain and two concrete fix sites; deliberately not folded in here.

🤖 Generated with Claude Code

Closes #40.

captureTurn raced the whole turn lifecycle against a single deadline
armed before startRequest, so --turn-timeout-ms / CODEX_TURN_TIMEOUT_MS
bounded total turn duration rather than agent inactivity. That's the
wrong metric: gpt-5.6-sol at effort=xhigh on adversarial-review
routinely does real, progressing work for well over 10 minutes. A
fixed budget forces choosing between killing long-but-live reviews or
tolerating a truly stalled app-server for just as long.

Replace the one-shot deadline with a resettable idle timer, reset in
applyTurnNotification on turn/started, item/started, and
item/completed. A turn producing events can run indefinitely; only a
gap of silence longer than the budget trips it. A separate, generous
wall-clock ceiling (45 min) still backstops a turn that resets its own
idle timer forever without completing.

Test-first: added spaced-events-idle-ok (events spaced under the idle
budget but summing past it — must not time out) and
goes-silent-after-first-event (one event then silence — must time out
via the idle gap) fixture behaviors, alongside the existing
stalled-turn-start regression test.

npm test: 176/179 pass (3 pre-existing state-dir/job-listing flakes,
reproduce identically on fork/main). npm run build passes.
@axisrow

axisrow commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

🔍 Local review (cycle 1)

Reviewed locally (/review + Codex companion), no bots pinged.

Verdict Reviewer Finding Location
FIX codex In-item progress (item/commandExecution/outputDelta, item/mcpToolCall/progress) does not refresh the idle deadline — a single long-running item can be interrupted mid-flight despite continuous progress plugins/codex/scripts/lib/codex.mjs:606
SKIP claude No critical issues found — timer cleanup, promise wiring, and reset logic verified correct; tests and build pass

Triage note on the FIX finding: verified against the generated app-server protocol types (plugins/codex/.generated/app-server-types/ServerNotification.ts) — item/commandExecution/outputDelta and item/mcpToolCall/progress are real notification methods that fall through the default: break case in applyTurnNotification (codex.mjs:637) without calling state.resetIdleDeadline(). Confirmed the idle-timeout path actively interrupts the in-flight turn on deadline (codex.mjs:748-757), so a genuinely-active single long-running item (e.g. a large command execution or MCP tool call streaming output for minutes) can be killed mid-operation — exactly the failure mode this PR set out to eliminate. Proceeding to fix.

axisrow and others added 2 commits August 2, 2026 03:38
The idle deadline was only reset on turn/started, item/started, and
item/completed — not on the streaming progress notifications a single
long-running item emits while active (item/commandExecution/outputDelta,
item/mcpToolCall/progress, item/agentMessage/delta, etc). A large command
execution or MCP tool call that runs past the idle budget while
continuously streaming output was interrupted mid-flight despite being
genuinely active — exactly the failure mode this PR set out to eliminate.

Found by Codex adversarial review during cycle-review round 1.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e deadline

Follow-up to 93f2aee, which extended the idle-deadline reset to in-item
progress/delta notifications but left two doc comments listing only the
turn/item lifecycle events. Comment-only; no behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQBJirbWNKWhnpFUnWogB8
@axisrow

axisrow commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

🔍 Local review (cycle 2)

Reviewed locally (/review + Codex companion), no bots pinged.

Verdict Reviewer Finding Location
SKIP (out of scope → #42) codex Foreground turns can outlive their caller and continue writing without a cancellation path plugins/codex/scripts/lib/codex.mjs:715-719
SKIP claude No critical issues found in the round-1 delta

On the Codex finding

The finding is real and correctly diagnosed — I verified the whole chain in the code, and it is worse than a timeout question: the broker never issues turn/interrupt on socket close (broker-controller.mjs:353-370), reconciliation flips the job to failed (state.mjs:151-196), and /cancel filters queued|running after reconciling (job-control.mjs:326-329), so interruptAppServerTurn (codex.mjs:1166) is unreachable even though threadId/turnId are persisted and the broker has an interrupt bypass (broker-controller.mjs:298-323).

It is, however, not a regression from this PR. Verified against the base commit 15f22e6: the broker's socket-close handler, the /cancel status filter, and the missing turnTimeoutMs on the adversarial-review path are byte-identical there. A foreground turn could already outlive the Bash ceiling on main — that is exactly what adversarial-review does today, since it drops turnTimeoutMs (codex-companion.mjs:465) and runs with the 600000ms default in the foreground. This PR widens the window for actively-progressing turns; it creates none of the links.

Filed as #42 with the full chain, file:line references, and two concrete fix sites (broker interrupt-on-disconnect, and /cancel reachability for reconciled-failed jobs). The :465 bug is already tracked as #38 with #39 open against it, so it is deliberately left untouched here — #39 edits the same files and folding it in would conflict.

On the fix I tried and reverted

I first attempted a guard in captureTurn capping the wall-clock ceiling to the idle budget for "foreground-scale" timeouts. Reverted — it was wrong on three counts:

  1. It silently un-fixed Turn timeout should measure agent-idle time, not total turn wall-clock #40. Because the ceiling never resets, setting wallClockCeilingMs = idleTimeoutMs makes the effective behavior a fixed whole-turn budget — bit-for-bit the pre-PR semantics — for every call at or below the threshold, including the default foreground budget (110000ms). That is precisely what this PR exists to remove. Any cap that is a function of idleTimeoutMs alone collapses the same way, multiplier or not.
  2. The heuristic lies in both directions. --turn-timeout-ms 400000 in the foreground still got the full 45 minutes (hole wide open), while a background job with a modest explicit budget got its ceiling wrongly collapsed.
  3. The race cannot be won here. interruptAppServerTurn opens a new socket to the broker and performs a network request from inside a process that is already being SIGKILLed. No internal deadline makes that reliable — the fix belongs at the broker and /cancel layers, which is what SIGKILLed foreground companion orphans a live app-server turn; /cancel can't reach it #42 proposes.

An explicit background flag threaded into captureTurn was also considered and rejected: handleReviewCommand never backgrounds anything (--background is parsed at :801 but options.background is never read), so passing background: true there would be a literal falsehood, and buildTaskRequest (:664-676) is an allowlist that JSON-round-trips to the detached worker, where a new field would silently vanish.

Scope of this PR after cycle 2

Unchanged from cycle 1 plus one comment-only commit (75bdf6a) noting in-item progress/delta among the events that reset the idle deadline. npm run build passes; targeted idle-timeout tests are 4/4 green; the full suite is 95 tests / 92 pass / 3 fail, and those 3 are the pre-existing status/result job-listing flakes that reproduce identically on unmodified main.

axisrow and others added 2 commits August 2, 2026 09:53
--turn-timeout-ms / CODEX_TURN_TIMEOUT_MS change meaning in this release:
they now bound agent idle time (silence between turn events) rather than
total turn wall-clock. A behavioral change to a user-facing knob, so it
gets its own version.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQBJirbWNKWhnpFUnWogB8
@axisrow

axisrow commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

🔍 Local review (cycle 1)

Reviewed locally (/review + Codex companion gpt-5.6-sol / effort=xhigh), no bots pinged. Reviewed head 34c9096, base 15f22e6.

Verdict Reviewer Finding Location
FIX codex Resettable deadline lets a foreground turn outlive the ~120s Bash-host ceiling, so SIGKILL preempts the turn/interrupt catch block plugins/codex/scripts/lib/codex.mjs:765-775
FIX claude Three of the nine delta cases are opt-ed out at handshake and can never fire plugins/codex/scripts/lib/codex.mjs:631-634
UNVERIFIED claude Six delta method names are asserted only by this PR's own fixture, not by the protocol plugins/codex/scripts/lib/codex.mjs:628-636
UNVERIFIED claude Delta routing depends on params.threadId clearing belongsToTurn; also fixture-asserted plugins/codex/scripts/lib/codex.mjs:672-684
SKIP claude captureTurn comment still lists only the three lifecycle events, omitting the deltas plugins/codex/scripts/lib/codex.mjs:687-693
SKIP claude Idle-path error still reads "turn budget"; "inactivity" would be self-explaining plugins/codex/scripts/lib/codex.mjs:700

On the Codex finding vs. this PR's "Out of scope" section

The out-of-scope note argues every link in that chain predates the PR. Three of the four do, verified byte-identical against 15f22e6: the broker never issues turn/interrupt on socket close, /cancel filters after reconciliation, and adversarial-review already runs foreground on the 600s default (#38/#39).

The fourth link is new. At base, FOREGROUND_TURN_TIMEOUT_MS = 110000 was a non-resettable race against the whole turn, so a foreground turn was always cut at 110s — before the ~120s host kill — and the catch block reached interruptAppServerTurn in time. With the idle reset, a foreground turn that keeps producing events sails past 110s to the host SIGKILL, and the interrupt never runs. Reaching that state was unreachable before this PR; now it is the normal path for any foreground turn doing more than 110s of real work.

So the mechanism is pre-existing, but the reachability is introduced here. Fix direction: keep an independent, non-resettable foreground ceiling below the host kill and interrupt before it; let idle-reset govern the background/detached path, where no external ceiling applies.

On the two UNVERIFIED items

item/commandExecution/outputDelta, item/fileChange/outputDelta, item/mcpToolCall/progress, item/plan/delta, command/exec/outputDelta, and process/outputDelta appear nowhere else in the repo — not in app-server-protocol.d.ts, not in the opt-out list — only in the new switch and in the fixture this PR added. The green test proves the code is self-consistent, not that the app-server emits those methods. Same for the params.threadId shape the routing depends on. Confirming needs a real codex run that produces a long single item; the cycle stopped rather than treating "couldn't reproduce" as approval.

@axisrow

axisrow commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Review cycle stopped after cycle 1 by author decision. The two FIX findings, the resolution of both UNVERIFIED items (protocol research against codex-cli 0.146.0 binary + rust-v0.146.0 sources), and the two deferred SKIPs are tracked in follow-up #43 with concrete fix/test plans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Turn timeout should measure agent-idle time, not total turn wall-clock

1 participant