Skip to content

Commit bec139c

Browse files
axisrowclaude
andcommitted
fix: turn-timeout gaps — interrupt on deadline, arm before startRequest, thread through options (closes #27)
Three gaps from Codex cycle-review of PR #26: 1. Timed-out turn not interrupted: deadline rejected the await but turn/interrupt was never sent. The broker kept executing a write-capable turn after it was reported failed. Fix: catch deadline rejection, best-effort interruptAppServerTurn(cwd, {threadId, turnId}), re-throw. 2. Deadline armed after turn/start response: a stalled turn/start (app-server alive but not responding) was unbounded. Fix: arm the deadline BEFORE startRequest, race the entire lifecycle (startRequest + completion) against it. If the deadline fires during startRequest, there's no turnId to interrupt, but the process no longer hangs. 3. --turn-timeout-ms ignored for background: applyForegroundTurnBudget was foreground-only (env side-channel gated by if(!options.background)). Replaced with options threading: resolveTurnTimeoutMsFromOptions resolves the value from the flag/env/default (foreground=110s, background=600s), passes it through buildTaskRequest → handleTaskWorker → executeTaskRun → runAppServerTurn → captureTurn → resolveTurnTimeoutMs. Deleted applyForegroundTurnBudget (env side-channel gone). Env fallback (CODEX_TURN_TIMEOUT_MS) remains for operator-configured overrides. Also threads cwd into captureTurn options (needed by the interrupt path). Verified: 166 tests, 162 pass / 4 pre-existing (unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 97c5556 commit bec139c

4 files changed

Lines changed: 109 additions & 52 deletions

File tree

plugins/codex/scripts/codex-companion.mjs

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ const TASK_WORKER_RECORD_WAIT_TIMEOUT_MS = 1000;
7474
// structured error instead of being killed with an empty result.
7575
// Ported from @russjhammond's openai/codex-plugin-cc#376.
7676
const FOREGROUND_TURN_TIMEOUT_MS = 110000;
77+
// Background runs have no external Bash ceiling — give them the full default budget.
78+
const DEFAULT_TURN_TIMEOUT_MS = 600000;
7779
const REASONING_EFFORTS = ["none", "minimal", "low", "medium", "high", "xhigh", "max", "ultra"];
7880
const VALID_REASONING_EFFORTS = new Set(REASONING_EFFORTS);
7981
const MODEL_ALIASES = new Map([
@@ -419,6 +421,7 @@ async function executeReviewRun(request) {
419421
target: reviewTarget,
420422
model: request.model,
421423
effort: request.effort,
424+
turnTimeoutMs: request.turnTimeoutMs,
422425
onProgress: request.onProgress
423426
});
424427
const payload = {
@@ -544,6 +547,7 @@ async function executeTaskRun(request) {
544547
sandbox: request.write ? "workspace-write" : "read-only",
545548
onProgress: request.onProgress,
546549
persistThread: true,
550+
turnTimeoutMs: request.turnTimeoutMs,
547551
threadName: resumeThreadId ? null : buildPersistentTaskThreadName(request.prompt || DEFAULT_CONTINUE_PROMPT)
548552
});
549553

@@ -655,15 +659,16 @@ function buildTaskJob(workspaceRoot, taskMetadata, write) {
655659
});
656660
}
657661

658-
function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId }) {
662+
function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId, turnTimeoutMs }) {
659663
return {
660664
cwd,
661665
model,
662666
effort,
663667
prompt,
664668
write,
665669
resumeLast,
666-
jobId
670+
jobId,
671+
turnTimeoutMs
667672
};
668673
}
669674

@@ -694,19 +699,21 @@ async function executeTransfer(cwd, options = {}) {
694699
};
695700
}
696701

697-
// Set the per-turn budget for a FOREGROUND command (codex.mjs reads
698-
// CODEX_TURN_TIMEOUT_MS at call time). Precedence: explicit --turn-timeout-ms
699-
// flag > a pre-set CODEX_TURN_TIMEOUT_MS env > the foreground default just
700-
// under the host Bash ceiling. Only call on foreground path: a detached
701-
// background worker inherits the parent env, so capping here would shrink
702-
// the background budget too. Ported from openai#376.
703-
function applyForegroundTurnBudget(options) {
702+
// Resolve the per-turn timeout from CLI options. Precedence:
703+
// --turn-timeout-ms flag > CODEX_TURN_TIMEOUT_MS env > foreground/background default.
704+
// Foreground default is just under the Bash-tool ceiling (110s) so a stalled turn
705+
// returns a structured error instead of being SIGKILLed. Background gets the full
706+
// 600s default (no external ceiling to collide with).
707+
function resolveTurnTimeoutMsFromOptions(options) {
704708
const explicit = Number(options["turn-timeout-ms"]);
705709
if (Number.isFinite(explicit) && explicit > 0) {
706-
process.env.CODEX_TURN_TIMEOUT_MS = String(explicit);
707-
} else if (!process.env.CODEX_TURN_TIMEOUT_MS) {
708-
process.env.CODEX_TURN_TIMEOUT_MS = String(FOREGROUND_TURN_TIMEOUT_MS);
710+
return explicit;
709711
}
712+
const fromEnv = Number(process.env.CODEX_TURN_TIMEOUT_MS);
713+
if (Number.isFinite(fromEnv) && fromEnv > 0) {
714+
return fromEnv;
715+
}
716+
return options.background ? DEFAULT_TURN_TIMEOUT_MS : FOREGROUND_TURN_TIMEOUT_MS;
710717
}
711718

712719
function readTaskPrompt(cwd, options, positionals) {
@@ -808,9 +815,6 @@ async function handleReviewCommand(argv, config) {
808815
jobClass: "review",
809816
summary: metadata.summary
810817
});
811-
if (!options.background) {
812-
applyForegroundTurnBudget(options);
813-
}
814818
await runForegroundCommand(
815819
job,
816820
(progress) =>
@@ -822,6 +826,7 @@ async function handleReviewCommand(argv, config) {
822826
effort,
823827
focusText,
824828
reviewName: config.reviewName,
829+
turnTimeoutMs: resolveTurnTimeoutMsFromOptions(options),
825830
onProgress: progress
826831
}),
827832
{ json: options.json }
@@ -874,15 +879,15 @@ async function handleTask(argv) {
874879
prompt,
875880
write,
876881
resumeLast,
877-
jobId: job.id
882+
jobId: job.id,
883+
turnTimeoutMs: resolveTurnTimeoutMsFromOptions(options)
878884
});
879885
const { payload } = enqueueBackgroundTask(cwd, job, request);
880886
outputCommandResult(payload, renderQueuedTaskLaunch(payload), options.json);
881887
return;
882888
}
883889

884890
const job = buildTaskJob(workspaceRoot, taskMetadata, write);
885-
applyForegroundTurnBudget(options);
886891
await runForegroundCommand(
887892
job,
888893
(progress) =>
@@ -894,6 +899,7 @@ async function handleTask(argv) {
894899
write,
895900
resumeLast,
896901
jobId: job.id,
902+
turnTimeoutMs: resolveTurnTimeoutMsFromOptions(options),
897903
onProgress: progress
898904
}),
899905
{ json: options.json }

plugins/codex/scripts/lib/codex.mjs

Lines changed: 59 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -603,33 +603,19 @@ async function captureTurn(client, threadId, startRequest, options = {}) {
603603
});
604604

605605
try {
606-
const response = await startRequest();
607-
options.onResponse?.(response, state);
608-
state.turnId = response.turn?.id ?? null;
609-
if (state.turnId) {
610-
state.threadTurnIds.set(state.threadId, state.turnId);
611-
}
612-
for (const message of state.bufferedNotifications) {
613-
if (belongsToTurn(state, message)) {
614-
applyTurnNotification(state, message);
615-
} else {
616-
if (previousHandler) {
617-
previousHandler(message);
618-
}
619-
}
620-
}
621-
state.bufferedNotifications.length = 0;
622-
623-
if (response.turn?.status && response.turn.status !== "inProgress") {
624-
completeTurn(state, response.turn);
625-
}
606+
// Arm the deadline BEFORE startRequest so a stalled turn/start (app-server
607+
// alive but not responding) is also bounded. Finding #27-2.
608+
const turnTimeoutMs = resolveTurnTimeoutMs(options);
609+
let deadlineTimer = null;
610+
const deadline = new Promise((_resolve, reject) => {
611+
deadlineTimer = setTimeout(() => {
612+
reject(new Error(`codex turn exceeded the ${turnTimeoutMs}ms turn budget.`));
613+
}, turnTimeoutMs);
614+
deadlineTimer.unref?.();
615+
});
626616

627-
// Bound the await so it can never outlast a dead process or a runaway turn.
628-
// Ported from @russjhammond's openai/codex-plugin-cc#376.
629-
// 1. state.completion — resolves on turn/completed (or inferred). Wire the
630-
// previously-dead rejectCompletion to the client exit so an app-server
631-
// death AFTER startRequest resolved rejects the await immediately.
632-
// 2. deadline — hard per-turn budget (resolveTurnTimeoutMs).
617+
// Wire exitPromise to rejectCompletion so an app-server death at any point
618+
// (during startRequest OR during completion) rejects immediately.
633619
client.exitPromise.then(() => {
634620
if (state.completed) {
635621
return;
@@ -638,21 +624,55 @@ async function captureTurn(client, threadId, startRequest, options = {}) {
638624
client.exitError ?? new Error("codex app-server exited before the turn completed.")
639625
);
640626
});
641-
const turnTimeoutMs = resolveTurnTimeoutMs(options);
642-
let deadlineTimer = null;
643-
const deadline = new Promise((_resolve, reject) => {
644-
deadlineTimer = setTimeout(() => {
645-
reject(new Error(`codex turn exceeded the ${turnTimeoutMs}ms turn budget.`));
646-
}, turnTimeoutMs);
647-
deadlineTimer.unref?.();
648-
});
627+
628+
let result;
649629
try {
650-
return await Promise.race([state.completion, deadline]);
630+
// Race the entire lifecycle (startRequest + completion) against the deadline.
631+
result = await Promise.race([
632+
(async () => {
633+
const response = await startRequest();
634+
options.onResponse?.(response, state);
635+
state.turnId = response.turn?.id ?? null;
636+
if (state.turnId) {
637+
state.threadTurnIds.set(state.threadId, state.turnId);
638+
}
639+
for (const message of state.bufferedNotifications) {
640+
if (belongsToTurn(state, message)) {
641+
applyTurnNotification(state, message);
642+
} else {
643+
if (previousHandler) {
644+
previousHandler(message);
645+
}
646+
}
647+
}
648+
state.bufferedNotifications.length = 0;
649+
650+
if (response.turn?.status && response.turn.status !== "inProgress") {
651+
completeTurn(state, response.turn);
652+
}
653+
654+
return await state.completion;
655+
})(),
656+
deadline
657+
]);
658+
} catch (error) {
659+
// Finding #27-1: on deadline, interrupt the in-flight turn so the broker
660+
// stops executing a write-capable task after we've reported it failed.
661+
// Best-effort: if interrupt fails (broker gone, network down), the error
662+
// from the deadline still propagates — we just don't block on it.
663+
if (state.threadId && state.turnId && options.cwd) {
664+
await interruptAppServerTurn(options.cwd, {
665+
threadId: state.threadId,
666+
turnId: state.turnId
667+
}).catch(() => {});
668+
}
669+
throw error;
651670
} finally {
652671
if (deadlineTimer) {
653672
clearTimeout(deadlineTimer);
654673
}
655674
}
675+
return result;
656676
} finally {
657677
clearCompletionTimer(state);
658678
client.setNotificationHandler(previousHandler ?? null);
@@ -1095,6 +1115,8 @@ export async function runAppServerReview(cwd, options = {}) {
10951115
target: options.target
10961116
}),
10971117
{
1118+
cwd,
1119+
turnTimeoutMs: options.turnTimeoutMs,
10981120
onProgress: options.onProgress,
10991121
onResponse(response, state) {
11001122
if (response.reviewThreadId) {
@@ -1230,6 +1252,8 @@ export async function runAppServerTurn(cwd, options = {}) {
12301252
outputSchema: options.outputSchema ?? null
12311253
}),
12321254
{
1255+
cwd,
1256+
turnTimeoutMs: options.turnTimeoutMs,
12331257
onProgress: options.onProgress,
12341258
onResponse() {
12351259
if (!options.effort) {

tests/fake-codex-fixture.mjs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -538,6 +538,11 @@ rl.on("line", (line) => {
538538
}
539539
540540
case "turn/start": {
541+
if (BEHAVIOR === "stalled-turn-start") {
542+
// Never respond — simulates app-server alive but network stalled.
543+
// companion's deadline must timeout and reject.
544+
break;
545+
}
541546
if (BEHAVIOR === "turn-start-fails") {
542547
throw new Error("turn/start failed after thread resolution");
543548
}

tests/runtime.test.mjs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3029,3 +3029,25 @@ test("setup and status honor --cwd when reading shared session runtime", () => {
30293029
input: JSON.stringify({ hook_event_name: "SessionEnd", cwd: targetWorkspace })
30303030
});
30313031
});
3032+
3033+
test("task with stalled turn/start times out via --turn-timeout-ms instead of hanging forever", () => {
3034+
const repo = makeTempDir();
3035+
const binDir = makeTempDir();
3036+
installFakeCodex(binDir, "stalled-turn-start");
3037+
initGitRepo(repo);
3038+
fs.writeFileSync(path.join(repo, "README.md"), "hello\n");
3039+
run("git", ["add", "README.md"], { cwd: repo });
3040+
run("git", ["commit", "-m", "init"], { cwd: repo });
3041+
3042+
// 3s timeout — must reject within that, not hang forever.
3043+
const result = run("node", [SCRIPT, "task", "--turn-timeout-ms", "3000", "test prompt"], {
3044+
cwd: repo,
3045+
env: buildEnv(binDir)
3046+
});
3047+
3048+
// Must NOT hang — exit with a timeout error.
3049+
assert.notEqual(result.status, 0, "must exit non-zero on timeout, not hang");
3050+
assert.match(result.stderr, /turn budget/i, "error must mention the turn budget");
3051+
const storedJob = readPersistedJob(repo);
3052+
assert.equal(storedJob.status, "failed", "job must be marked failed");
3053+
});

0 commit comments

Comments
 (0)