From e4af6f853b08158e2776523a44d3be9390bca08f Mon Sep 17 00:00:00 2001 From: russjhammond Date: Tue, 21 Jul 2026 22:37:26 -0400 Subject: [PATCH] =?UTF-8?q?feat(codex):=20harden=20turn=20timeout=20?= =?UTF-8?q?=E2=80=94=20interrupt=20on=20deadline,=20bound=20stalled=20turn?= =?UTF-8?q?/start,=20thread=20budget=20via=20options?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #376 (the turn-await hang fix), porting the three improvements axisrow found while adapting #376 into their fork, with attribution: 1. Arm the deadline BEFORE turn/start and race the whole turn lifecycle, so a stalled turn/start (app-server alive but never responding) is bounded too — not just the post-start completion await. 2. On the deadline (or any lifecycle error), best-effort interrupt the in-flight turn so the broker stops executing a write-capable task after the job is already reported failed. 3. Resolve the per-turn budget from options and thread it through the request (buildTaskRequest -> executeTaskRun/executeReviewRun -> runAppServerTurn/ Review -> captureTurn) instead of mutating process.env — retiring the foreground->background env-inheritance hazard. Background runs get the full default budget. Also drops the redundant exitRaceSettled guard (state.completed already covers the late-exit-after-completion race). Kept #376's exit-wiring placement AFTER startRequest resolves (commit 688192a): hoisting it with the deadline would reject an unobserved state.completion when startRequest itself rejects, reintroducing the unhandled-rejection that #376's review already fixed. The deadline alone bounds the stalled-turn/start case. Test: a foreground task whose turn/start never responds now times out on the turn budget instead of hanging (new stalled-turn-start fake-codex behavior). tests/helpers.mjs run() gains an optional timeout so the hang path can't block CI. Credits: axisrow/codex-plugin-cc#26 (port) and #28 (these three findings). Refs openai#49. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01A5iLDcuiEzAxs1vBEwafur --- plugins/codex/scripts/codex-companion.mjs | 48 ++++----- plugins/codex/scripts/lib/codex.mjs | 115 +++++++++++++--------- tests/fake-codex-fixture.mjs | 5 + tests/helpers.mjs | 4 + tests/runtime.test.mjs | 28 ++++++ 5 files changed, 133 insertions(+), 67 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index d511d405d..e3d99da27 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -380,6 +380,7 @@ async function executeReviewRun(request) { const result = await runAppServerReview(request.cwd, { target: reviewTarget, model: request.model, + turnTimeoutMs: request.turnTimeoutMs, onProgress: request.onProgress }); const payload = { @@ -423,6 +424,7 @@ async function executeReviewRun(request) { model: request.model, sandbox: "read-only", outputSchema: readOutputSchema(REVIEW_SCHEMA), + turnTimeoutMs: request.turnTimeoutMs, onProgress: request.onProgress }); const parsed = parseStructuredOutput(result.finalMessage, { @@ -499,6 +501,7 @@ async function executeTaskRun(request) { model: request.model, effort: request.effort, sandbox: request.write ? "workspace-write" : "read-only", + turnTimeoutMs: request.turnTimeoutMs, onProgress: request.onProgress, persistThread: true, threadName: resumeThreadId ? null : buildPersistentTaskThreadName(request.prompt || DEFAULT_CONTINUE_PROMPT) @@ -611,7 +614,7 @@ function buildTaskJob(workspaceRoot, taskMetadata, write) { }); } -function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId }) { +function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId, turnTimeoutMs }) { return { cwd, model, @@ -619,7 +622,8 @@ function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId prompt, write, resumeLast, - jobId + jobId, + turnTimeoutMs }; } @@ -719,20 +723,25 @@ function enqueueBackgroundTask(cwd, job, request) { }; } -// Set the per-turn budget for a FOREGROUND command (codex.mjs reads -// CODEX_TURN_TIMEOUT_MS at call time). Precedence: explicit --turn-timeout-ms -// flag > a pre-set CODEX_TURN_TIMEOUT_MS env (e.g. settings.json) > the -// foreground default just under the host Bash ceiling, so a stalled turn -// returns a structured timeout instead of being SIGKILLed with no output. -// Only call this on a foreground path: a detached background worker inherits -// the parent env, so capping it here would shrink the background budget too. -function applyForegroundTurnBudget(options) { +// Resolve the per-turn budget from CLI options and thread it through the request +// (no process.env mutation). Precedence: explicit --turn-timeout-ms flag > +// a pre-set CODEX_TURN_TIMEOUT_MS env (e.g. settings.json) > the mode default. +// Foreground default sits just under the host Bash ceiling so a stalled turn +// returns a structured timeout instead of being SIGKILLed with no output; +// background has no external ceiling, so it gets the full default budget. +// Threading via options (rather than mutating process.env, which a detached +// background worker would inherit) removes the foreground/background env-leak +// hazard entirely. (Follow-up to openai#376, per axisrow/codex-plugin-cc#28.) +function resolveTurnTimeoutMsFromOptions(options) { const explicit = Number(options["turn-timeout-ms"]); if (Number.isFinite(explicit) && explicit > 0) { - process.env.CODEX_TURN_TIMEOUT_MS = String(explicit); - } else if (!process.env.CODEX_TURN_TIMEOUT_MS) { - process.env.CODEX_TURN_TIMEOUT_MS = String(FOREGROUND_TURN_TIMEOUT_MS); + return explicit; } + const fromEnv = Number(process.env.CODEX_TURN_TIMEOUT_MS); + if (Number.isFinite(fromEnv) && fromEnv > 0) { + return fromEnv; + } + return options.background ? DEFAULT_TURN_TIMEOUT_MS : FOREGROUND_TURN_TIMEOUT_MS; } async function handleReviewCommand(argv, config) { @@ -765,9 +774,6 @@ async function handleReviewCommand(argv, config) { // Review turns run foreground (--wait) through the same path as tasks; give // them the same foreground budget so a stall returns a structured timeout // instead of hitting the host Bash ceiling with an empty result. - if (!options.background) { - applyForegroundTurnBudget(options); - } await runForegroundCommand( job, (progress) => @@ -778,6 +784,7 @@ async function handleReviewCommand(argv, config) { model: options.model, focusText, reviewName: config.reviewName, + turnTimeoutMs: resolveTurnTimeoutMsFromOptions(options), onProgress: progress }), { json: options.json } @@ -829,18 +836,14 @@ async function handleTask(argv) { prompt, write, resumeLast, - jobId: job.id + jobId: job.id, + turnTimeoutMs: resolveTurnTimeoutMsFromOptions(options) }); const { payload } = enqueueBackgroundTask(cwd, job, request); outputCommandResult(payload, renderQueuedTaskLaunch(payload), options.json); return; } - // 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); - const job = buildTaskJob(workspaceRoot, taskMetadata, write); await runForegroundCommand( job, @@ -853,6 +856,7 @@ async function handleTask(argv) { write, resumeLast, jobId: job.id, + turnTimeoutMs: resolveTurnTimeoutMsFromOptions(options), onProgress: progress }), { json: options.json } diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index 9232b0d2b..2daebd900 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -606,49 +606,10 @@ async function captureTurn(client, threadId, startRequest, options = {}) { }); try { - const response = await startRequest(); - options.onResponse?.(response, state); - state.turnId = response.turn?.id ?? null; - if (state.turnId) { - state.threadTurnIds.set(state.threadId, state.turnId); - } - for (const message of state.bufferedNotifications) { - if (belongsToTurn(state, message)) { - applyTurnNotification(state, message); - } else { - if (previousHandler) { - previousHandler(message); - } - } - } - state.bufferedNotifications.length = 0; - - if (response.turn?.status && response.turn.status !== "inProgress") { - completeTurn(state, response.turn); - } - - // Bound the await so it can never outlast a dead process or a runaway turn: - // 1. state.completion — resolves on turn/completed (or inferred). Wire the - // previously-dead rejectCompletion to the client exit so an app-server - // death AFTER startRequest resolved rejects the await immediately - // instead of hanging until the deadline. Registered HERE rather than - // before startRequest: if startRequest itself rejects (e.g. broker-busy - // from turn/start, or the app-server exiting while that request is - // pending), it propagates directly and state.completion is never - // observed — wiring the exit earlier would reject an unobserved promise - // and surface as an unhandled rejection. - // 2. deadline — hard per-turn budget (resolveTurnTimeoutMs: option > - // CODEX_TURN_TIMEOUT_MS env > default, resolved at call time). - let exitRaceSettled = false; - client.exitPromise.then(() => { - if (exitRaceSettled || state.completed) { - return; - } - exitRaceSettled = true; - state.rejectCompletion( - client.exitError ?? new Error("codex app-server exited before the turn completed.") - ); - }); + // Arm the deadline BEFORE startRequest so a stalled turn/start (app-server + // alive but never responding) is bounded too — the race below covers the + // whole turn lifecycle, not just the post-start completion await. + // (Follow-up to openai#376, based on axisrow/codex-plugin-cc#28 finding #27-2.) const turnTimeoutMs = resolveTurnTimeoutMs(options); let deadlineTimer = null; const deadline = new Promise((_resolve, reject) => { @@ -657,13 +618,75 @@ async function captureTurn(client, threadId, startRequest, options = {}) { }, turnTimeoutMs); deadlineTimer.unref?.(); }); + + let result; try { - return await Promise.race([state.completion, deadline]); + // Race the whole lifecycle (startRequest + completion) against the deadline. + result = await Promise.race([ + (async () => { + const response = await startRequest(); + options.onResponse?.(response, state); + state.turnId = response.turn?.id ?? null; + if (state.turnId) { + state.threadTurnIds.set(state.threadId, state.turnId); + } + for (const message of state.bufferedNotifications) { + if (belongsToTurn(state, message)) { + applyTurnNotification(state, message); + } else { + if (previousHandler) { + previousHandler(message); + } + } + } + state.bufferedNotifications.length = 0; + + if (response.turn?.status && response.turn.status !== "inProgress") { + completeTurn(state, response.turn); + } + + // Wire exitPromise → rejectCompletion only AFTER startRequest resolved, + // so an app-server death mid-completion rejects the await immediately + // instead of hanging until the deadline. Kept post-startRequest (not + // hoisted with the deadline): if the app-server dies DURING startRequest + // that RPC rejects directly, and state.completion is never awaited on + // that path — wiring the exit earlier would reject an unobserved promise + // and surface as an unhandled rejection (the regression fixed in + // openai#376 review, commit 688192a). `state.completed` guards a late + // exit that arrives after a normal completion. + client.exitPromise.then(() => { + if (state.completed) { + return; + } + state.rejectCompletion( + client.exitError ?? new Error("codex app-server exited before the turn completed.") + ); + }); + + return await state.completion; + })(), + deadline + ]); + } catch (error) { + // On deadline (or any lifecycle error), interrupt the in-flight turn so the + // broker stops executing a write-capable task after we've already reported + // it failed. Best-effort: a failed interrupt (broker gone, network down) + // must not mask the original error. Requires threadId + turnId (only set + // once startRequest resolved) and options.cwd. + // (axisrow/codex-plugin-cc#28 finding #27-1.) + if (state.threadId && state.turnId && options.cwd) { + await interruptAppServerTurn(options.cwd, { + threadId: state.threadId, + turnId: state.turnId + }).catch(() => {}); + } + throw error; } finally { if (deadlineTimer) { clearTimeout(deadlineTimer); } } + return result; } finally { clearCompletionTimer(state); client.setNotificationHandler(previousHandler ?? null); @@ -1089,6 +1112,8 @@ export async function runAppServerReview(cwd, options = {}) { target: options.target }), { + cwd, + turnTimeoutMs: options.turnTimeoutMs, onProgress: options.onProgress, onResponse(response, state) { if (response.reviewThreadId) { @@ -1200,7 +1225,7 @@ export async function runAppServerTurn(cwd, options = {}) { effort: options.effort ?? null, outputSchema: options.outputSchema ?? null }), - { onProgress: options.onProgress } + { cwd, turnTimeoutMs: options.turnTimeoutMs, onProgress: options.onProgress } ); return { diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index f83c96a0d..43239bbfd 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -437,6 +437,11 @@ rl.on("line", (line) => { } case "turn/start": { + if (BEHAVIOR === "stalled-turn-start") { + // Never respond to turn/start — app-server alive but stalled. + // The companion's deadline must fire and reject instead of hanging. + break; + } const thread = ensureThread(state, message.params.threadId); const prompt = (message.params.input || []) .filter((item) => item.type === "text") diff --git a/tests/helpers.mjs b/tests/helpers.mjs index d6981197a..6647315da 100644 --- a/tests/helpers.mjs +++ b/tests/helpers.mjs @@ -18,6 +18,10 @@ export function run(command, args, options = {}) { env: options.env, encoding: "utf8", input: options.input, + // Optional hard wall-clock cap (ms). Left undefined by default (unchanged + // behavior); a test that exercises a hang path passes it so a regression + // fails fast instead of blocking CI forever. + timeout: options.timeout, shell: options.shell ?? (process.platform === "win32" && !path.isAbsolute(command)), windowsHide: true }); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..4b4c8cb48 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -2257,3 +2257,31 @@ test("setup and status honor --cwd when reading shared session runtime", () => { assert.equal(payload.sessionRuntime.mode, "shared"); assert.equal(payload.sessionRuntime.endpoint, "unix:/tmp/fake-broker.sock"); }); + +test("a foreground task whose turn/start stalls times out on the turn budget instead of hanging", () => { + const binDir = makeTempDir(); + installFakeCodex(binDir, "stalled-turn-start"); + const repo = makeTempDir(); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + // The fake app-server accepts thread/start but never answers turn/start. + // With the deadline armed before turn/start, a 2s budget must reject rather + // than hang. The 30s `timeout` is only a CI backstop: if the fix regressed + // and it truly hung, spawnSync would kill it and the stderr assertion below + // (no "turn budget" message on a kill) would fail the test. + const result = run("node", [SCRIPT, "task", "--turn-timeout-ms", "2000", "test prompt"], { + cwd: repo, + env: buildEnv(binDir), + timeout: 30000 + }); + + assert.notEqual(result.status, 0, "must exit non-zero on the turn-budget timeout, not hang"); + assert.match( + result.stderr, + /turn budget/i, + "a proper deadline timeout surfaces the turn-budget message (a hang-then-kill would not)" + ); +});