feat(codex): harden turn timeout — interrupt on deadline, bound stalled turn/start, options threading

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,15 +614,16 @@ 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,
     effort,
     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 }

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 {

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")

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
   });

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)"
+  );
+});