Bound the Codex turn await: fix unbounded hang on stalled/dead app-server + make turn budget configurable#376
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5415c43684
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| state.rejectCompletion( | ||
| client.exitError ?? new Error("codex app-server exited before the turn completed.") | ||
| ); |
There was a problem hiding this comment.
When startRequest() rejects before the code reaches the Promise.race below (for example, a broker-busy error from turn/start/review/start, or the app-server exiting while that request is pending), withAppServer closes the client and exitPromise resolves, so these lines reject state.completion even though no handler has been attached to that promise. On supported Node versions this becomes an unhandled rejection that can terminate the CLI or preempt the intended direct retry/original error reporting; defer this rejection until the completion promise is being raced, or explicitly handle it on early failures.
Useful? React with 👍 / 👎.
| const explicitTurnTimeout = Number(options["turn-timeout-ms"]); | ||
| if (Number.isFinite(explicitTurnTimeout) && explicitTurnTimeout > 0) { | ||
| process.env.CODEX_TURN_TIMEOUT_MS = String(explicitTurnTimeout); | ||
| } else if (!process.env.CODEX_TURN_TIMEOUT_MS) { | ||
| process.env.CODEX_TURN_TIMEOUT_MS = String(FOREGROUND_TURN_TIMEOUT_MS); |
There was a problem hiding this comment.
This default foreground budget is only installed in handleTask, but handleReviewCommand still runs /codex:review --wait and /codex:adversarial-review --wait through the same foreground runForegroundCommand path without setting CODEX_TURN_TIMEOUT_MS. Those review turns therefore fall back to the 600000ms default in codex.mjs, so under Claude Code's ~120000ms Bash ceiling they can still be SIGKILLed with an empty result instead of returning the structured timeout this change is meant to provide.
Useful? React with 👍 / 👎.
|
Thanks for the review — both P2s addressed in
Verified: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 688192a624
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| deadlineTimer = setTimeout(() => { | ||
| reject(new Error(`codex turn exceeded the ${turnTimeoutMs}ms turn budget.`)); | ||
| }, turnTimeoutMs); |
There was a problem hiding this comment.
When the new deadline fires, this rejects out of captureTurn; runTrackedJob then takes its catch path and main().catch prints only the error message to stderr, so runForegroundCommand never writes a rendered result to stdout. In the foreground slash-command paths that return the command stdout verbatim, a timed-out turn still yields empty stdout instead of the structured timeout/partial output this budget is meant to surface; return a failed turn state/rendered result or render the timeout from the catch path.
Useful? React with 👍 / 👎.
|
Fair point — a scoping note on this PR vs. a follow-up. The primary goal here is eliminating the unbounded hang: a stalled or dead app-server previously blocked on Rendering the timeout (plus any partial turn output) to stdout as a structured result is a reasonable next step, but it touches the foreground render path ( |
…) (#26) Closes fork#25. If Codex CLI can't reach the OpenAI API (network down, DNS misconfigured), captureTurn's 'await state.completion' hangs forever — no turn-level timeout, rejectCompletion was dead code (zero call sites). Ported from @russjhammond's openai#376: - Wire rejectCompletion to client.exitPromise: app-server death after startRequest rejects the await immediately instead of hanging. - Add deadline via Promise.race: hard per-turn budget (default 600s bg, 110s foreground — just below Claude Code Bash tool ceiling). - resolveTurnTimeoutMs reads CODEX_TURN_TIMEOUT_MS at CALL time, not import time (companion sets env after import — import-time read was inert). - applyForegroundTurnBudget: foreground commands get 110s budget so a stalled turn returns a structured error instead of being SIGKILLed. - --turn-timeout-ms flag on review/adversarial-review/task for manual override. Verified: 166 tests, 162 pass / 4 pre-existing (unchanged). Refs fork#25, openai#49, openai#489, openai#376 (@russjhammond). Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@russjhammond — thank you for this PR. I forked openai/codex-plugin-cc and ported your fix into my fork (axisrow#26), so this comment is sharing back what I found while adapting it. Your diagnosis was spot-on —
My fork PR with attribution to this PR: axisrow#26. Happy to talk through any of it — and equally happy to leave it here. |
|
Follow-up on my earlier comment — the three items I flagged as "out of scope for this PR" are now closed in my fork, in case the notes are useful to anyone picking them up later. Following the scoping Russ drew in his comment above — this PR's job is eliminating the unbounded hang, and the three below were consciously deferred — I split them into a separate follow-up PR (axisrow#28) so #376's scope stays clean. No ask that you take any of it.
Verified TDD-first: a Fork PR with attribution back to this one: axisrow#28. Pure sharing — take it, adapt it, or set it aside. |
…rever captureTurn ended in `return await state.completion`, a Promise resolved only by completeTurn() (on turn/completed or a 250ms inferred fallback). Its rejectCompletion counterpart was wired into state but never called — dead code. So if the app-server died mid-turn or simply stalled, nothing rejected the await: the turn hung until the caller's external timeout SIGKILLed the process with no output. Separately, the per-turn budget was read at import time (`const TURN_TIMEOUT_MS = Number(process.env.CODEX_TURN_TIMEOUT_MS) || 600000`), so any value set at runtime (a foreground budget below the caller's ceiling, or --turn-timeout-ms) was ignored — the budget was frozen at the default and effectively inert. Fixes: - Wire rejectCompletion to client.exitPromise so a mid-turn process exit surfaces immediately as an error instead of hanging. - Race state.completion against a hard deadline timer (cleared in finally). - Resolve the turn budget at call time (resolveTurnTimeoutMs: option > CODEX_TURN_TIMEOUT_MS env > default) so the companion's foreground budget and --turn-timeout-ms actually take effect. - companion: set a foreground turn budget below the caller's ceiling and add a --turn-timeout-ms flag; background jobs keep the full default. Verified: node --check on both files; --turn-timeout-ms 3000 now aborts a long turn at ~3s with a structured error; a normal turn still completes cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two P2 fixes from the automated review: 1. codex.mjs — register the client.exitPromise -> rejectCompletion handler AFTER startRequest() resolves, not before. If startRequest() rejects (e.g. broker-busy from turn/start, or the app-server exiting while the request is pending), the error now propagates directly and state.completion is never observed. Previously the early-registered handler could reject an unobserved state.completion and surface as an unhandled rejection that can terminate the CLI or preempt the intended error reporting. The mid-turn-death coverage is unchanged (the handler is in place before the completion is awaited). 2. codex-companion.mjs — apply the foreground turn budget to review turns too. Extracted applyForegroundTurnBudget(options) and call it from both handleTask and handleReviewCommand. /codex:review --wait and adversarial-review --wait run foreground through runForegroundCommand but previously never set CODEX_TURN_TIMEOUT_MS, so they fell back to the 600000ms default and could be SIGKILLed at the host Bash ceiling with an empty result. Also accept --turn-timeout-ms on the review command. Guarded with !options.background so a detached worker still inherits the full default budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@axisrow — this is the good kind of open-source citizenship, and I appreciate it. Forking, porting through two adversarial review cycles, and then bringing the findings back here instead of quietly keeping them in your fork is exactly how this is supposed to work. Your three points all land:
I've picked this PR back up: rebased onto current |
|
Awesome — really glad this one's moving again, and thanks for the attribution. Happy it was useful. |
688192a to
b8b46ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8b46ee7a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Foreground turn budget (see applyForegroundTurnBudget). Runs only on the | ||
| // foreground path; the background branch returned above and its detached | ||
| // worker inherits the parent env unchanged (full default budget). | ||
| applyForegroundTurnBudget(options); |
There was a problem hiding this comment.
When the stop-review gate invokes this same task subcommand (plugins/codex/scripts/stop-review-gate-hook.mjs:105-109), it is not running under Claude Code's 120s Bash ceiling; it gives the child process STOP_REVIEW_TIMEOUT_MS (15 minutes). Applying the 110000ms foreground default here causes any enabled stop-gate review that legitimately runs longer than ~110s to fail/block early with a turn-budget error instead of honoring the hook's 15-minute timeout. Pass an explicit larger budget from the hook or avoid the Bash-specific default for that internal caller.
Useful? React with 👍 / 👎.
| async function handleTask(argv) { | ||
| const { options, positionals } = parseCommandInput(argv, { | ||
| valueOptions: ["model", "effort", "cwd", "prompt-file"], | ||
| valueOptions: ["model", "effort", "cwd", "prompt-file", "turn-timeout-ms"], |
There was a problem hiding this comment.
Accepting --turn-timeout-ms for task makes task --background --turn-timeout-ms 3000 look supported, but the background branch returns before applyForegroundTurnBudget() and buildTaskRequest() only stores cwd/model/effort/prompt/write/resumeLast/jobId, so the detached task-worker inherits no CLI override and falls back to CODEX_TURN_TIMEOUT_MS or the 600000ms default. Users trying to bound a background turn will still get the default budget unless they set the environment variable manually.
Useful? React with 👍 / 👎.
2 participants
Problem
A foreground Codex task can hang until the caller's external process timeout kills it with no output. Two distinct defects in
scripts/lib/codex.mjs:1. Unbounded, un-rejectable completion await.
captureTurnends inreturn await state.completion— a Promise resolved only bycompleteTurn()(on aturn/completednotification, or a 250ms inferred fallback). ItsrejectCompletioncounterpart is wired into the turn-capture state but has zero call sites — dead code. So oncestartRequest()resolves withinProgress, if the app-server dies or simply stalls, nothing rejects the await. It hangs until the caller (e.g. a host harness with a ~2-minute shell ceiling) SIGKILLs the process — surfacing as "hangs every time, returns nothing."2. Turn budget read at import time.
const TURN_TIMEOUT_MS = Number(process.env.CODEX_TURN_TIMEOUT_MS) || 600000is evaluated when the module is imported. The companion sets the env var after import (e.g. a foreground budget below the caller's ceiling, or via a flag), so the override never takes effect — the budget is frozen at the default and effectively inert.Fix
rejectCompletiontoclient.exitPromiseso a mid-turn process exit rejects the await immediately with a structured error instead of hanging.state.completionagainst a hard deadline timer (cleared in afinally).resolveTurnTimeoutMs(options): explicit option →CODEX_TURN_TIMEOUT_MSenv → default — so runtime overrides actually apply.codex-companion.mjs: set a foreground turn budget just below the caller's external ceiling and add a--turn-timeout-msflag. Background/detached jobs inherit the full default.Verification
node --checkpasses on both files.task --turn-timeout-ms 3000on a long-generation prompt now aborts at ~3s withcodex turn exceeded the 3000ms turn budget(previously ran unbounded — the flag was inert).🤖 Generated with Claude Code