fix(ai): stop a buffered-CLI timeout burning the full retry budget, and two subprocess leaks (#9476, #9479)

src/selfhost/ai.ts

@@ -598,6 +598,21 @@ async function isolatedCliCwd(): Promise<string> {
   return mkdtemp(join(tmpdir(), "loopover-ai-"));
 }
 
+/** #9479: remove a per-call temp dir once the subprocess is done with it. Nothing removed these: every AI review
+ *  call minted one (and, when repo review instructions are configured, wrote the composed system prompt into it),
+ *  so they accumulated on the container's writable overlay layer until it was recreated -- and left those repo
+ *  instructions on disk indefinitely. Best-effort by design: a cleanup failure must never turn a completed
+ *  review into a thrown error, and the next container recreation still collects anything missed. */
+async function removeIsolatedCliCwd(cwd: string | undefined): Promise<void> {
+  if (!cwd) return;
+  try {
+    const { rm } = await import("node:fs/promises");
+    await rm(cwd, { recursive: true, force: true });
+  } catch {
+    // best-effort -- see the doc comment.
+  }
+}
+
 /** Write `systemAppend` into `cwd` (the SAME per-call isolated temp dir already used for the subprocess's
  *  cwd, so it shares that directory's lifecycle) and return its path, for `--append-system-prompt-file`.
  *  Keeps repo review instructions out of argv/`ps aux` (#3951's concern) WITHOUT falling back to smuggling
@@ -903,6 +918,13 @@ async function defaultSpawn(): Promise<SpawnFn> {
         resolve({ stdout, code, stderr });
       });
       if (o.input != null) {
+        // #9479: `child.on("error")` catches SPAWN failures only -- it never receives stdio stream errors. If
+        // the CLI exits before draining stdin (an unknown flag on an upgraded binary, an immediate auth abort,
+        // an OOM kill), this ~250KB write fails with EPIPE on an emitter with no "error" listener, which Node
+        // escalates to an uncaught exception -> installSelfHostCrashHandlers -> exit(1), taking down every
+        // in-flight queue job in the container. The real failure is already surfaced by the exit-code and
+        // empty-output guards below, so swallowing the stream error here loses no diagnostic.
+        child.stdin?.on("error", () => undefined);
         child.stdin?.write(o.input);
         child.stdin?.end();
       }
@@ -997,6 +1019,7 @@ export function createClaudeCodeAi(parentEnv: Record<string, string | undefined>
       );
       let attempted = false;
       let stdoutForMetrics = "";
+      let cliCwd: string | undefined;
       try {
         if (!token) throw new Error("claude_code_no_oauth_token");
         // Usage telemetry (#claude-code-otel-passthrough): the allowlist deliberately excludes these -- they are
@@ -1019,7 +1042,8 @@ export function createClaudeCodeAi(parentEnv: Record<string, string | undefined>
         const systemAppend = normalizedSystemAppend(options);
         const prompt = toCliPrompt(options, systemAppend);
         const spawn = spawnImpl ?? (await defaultSpawn());
-        const cwd = await isolatedCliCwd();
+        cliCwd = await isolatedCliCwd();
+        const cwd = cliCwd;
         // Keep bypassPermissions (not "plan") only to avoid a headless approval prompt; the actual boundary is
         // tool removal. --tools "" removes every built-in tool, --strict-mcp-config prevents user/home MCP config
         // from loading, and mcp__* is a defense-in-depth deny for CLIs that still have MCP tools available. This
@@ -1088,6 +1112,7 @@ export function createClaudeCodeAi(parentEnv: Record<string, string | undefined>
         throw error;
       } finally {
         if (attempted) recordCliUsageMetrics("claude-code", claudeModel, effort, stdoutForMetrics);
+        await removeIsolatedCliCwd(cliCwd);
       }
     },
   };
@@ -1119,6 +1144,7 @@ export function createCodexAi(
       );
       let attempted = false;
       let stdoutForMetrics = "";
+      let cliCwd: string | undefined;
       try {
         assertCodexCredentialIsolation(parentEnv);
         await authCheckImpl(parentEnv);
@@ -1136,7 +1162,7 @@ export function createCodexAi(
           input: prompt,
           timeoutMs,
           firstOutputTimeoutMs,
-          cwd: await isolatedCliCwd(),
+          cwd: (cliCwd = await isolatedCliCwd()),
         });
         stdoutForMetrics = stdout;
         if (timedOut && stalledNoOutput) {
@@ -1190,6 +1216,7 @@ export function createCodexAi(
         throw error;
       } finally {
         if (attempted) recordCliUsageMetrics("codex", codexModel, effort, stdoutForMetrics);
+        await removeIsolatedCliCwd(cliCwd);
       }
     },
   };

src/services/ai-review.ts

@@ -1380,6 +1380,25 @@ function isSubscriptionCliTimeout(error: unknown): boolean {
   return error instanceof Error && error.message === "subscription_cli_timeout";
 }
 
+/**
+ * #9476: the CLI adapter's OTHER non-transient deadline signal, and in practice the one that actually fires.
+ * `claude --output-format json` buffers its whole response, so any run that exceeds its effort timeout has
+ * produced zero stdout bytes when the deadline lands -- which trips the first-output watchdog
+ * (`resolveClaudeFirstOutputTimeoutMs`, clamped to `timeoutMs - 1`) rather than the plain timeout. The adapter
+ * therefore throws `claude_stalled_no_output: <detail>` and `subscription_cli_timeout` is effectively
+ * unreachable for claude-code, so the strict-equality check above never matched and every timed-out review
+ * burned all three attempts: 3 x 180s at default effort, 3 x 600s at the top tier, before the fallback model
+ * was even tried. With QUEUE_CONCURRENCY defaulting to 8 that parks the whole queue during a provider
+ * slowdown, and the per-provider circuit breaker needs three FULL-LENGTH failures before it trips.
+ *
+ * Matched by PREFIX because these carry a `: detail` suffix -- the strict equality that missed this case is
+ * exactly the bug.
+ */
+function isStalledNoOutput(error: unknown): boolean {
+  if (!(error instanceof Error)) return false;
+  return error.message.startsWith("claude_stalled_no_output") || error.message.startsWith("codex_stalled_no_output");
+}
+
 /** True for a provider's own HTTP-429 signal (`src/selfhost/ai.ts`'s `claude_code_error_429` /
  *  `ai_http_429` / `anthropic_http_429`, and the generic Workers-AI equivalent). #5385-sentry
  *  (GITTENSORY-K/8): an immediate same-model retry against a rate limit that is still in its window has
@@ -1633,7 +1652,7 @@ async function runWorkersOpinion(
         // A structural config error (missing/expired credentials) is stronger still: it is DETERMINISTIC, not
         // just unlikely to clear in time -- the same model will fail the identical way on attempt 2 and 3 too,
         // confirmed live (GITTENSORY-K/8: 2094 + 544 events over 16 days from one never-fixed misconfiguration).
-        if (isSubscriptionCliTimeout(error) || isRateLimitError(error) || isStructuralProviderConfigError(error)) break;
+        if (isSubscriptionCliTimeout(error) || isStalledNoOutput(error) || isRateLimitError(error) || isStructuralProviderConfigError(error)) break;
       }
     }
   }

test/unit/ai-review.test.ts

@@ -3439,6 +3439,46 @@ describe("pure helpers", () => {
     expect(run).toHaveBeenCalledTimes(2); // 1 primary (timed out) + 1 fallback (succeeded on its first try).
   });
 
+  // #9476 regression: `claude --output-format json` buffers its whole response, so ANY run that exceeds its
+  // effort timeout has produced zero stdout when the deadline lands -- tripping the first-output watchdog and
+  // throwing `claude_stalled_no_output: <detail>` rather than `subscription_cli_timeout`. The break condition
+  // tested above used strict equality, so it never matched: every timed-out review burned all three attempts
+  // (3 x 180s at default effort, 3 x 600s at the top tier) before the fallback was even tried, and at
+  // QUEUE_CONCURRENCY=8 that parks the whole queue during a provider slowdown. The suffix is why prefix
+  // matching is required -- strict equality is the original bug.
+  it.each([
+    ["claude_stalled_no_output: no stdout within firstOutputTimeoutMs — claude likely hung"],
+    ["codex_stalled_no_output: no stdout within firstOutputTimeoutMs — codex likely hung reading stdin"],
+  ])("REGRESSION (#9476): runWorkersOpinion stops retrying after ONE %s", async (message) => {
+    let primaryAttempts = 0;
+    const run = vi.fn(async (model: string) => {
+      if (model === "fallback") return { response: reviewJson() };
+      primaryAttempts += 1;
+      throw new Error(message);
+    });
+    const env = createTestEnv({ AI: { run } as unknown as Ai });
+    const diagnostics: Array<{ status: string; model: string }> = [];
+    const parsed = await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never);
+    expect(parsed.review?.assessment).toContain("reasonable");
+    expect(primaryAttempts).toBe(1); // NOT 3 -- the stall short-circuits further retries of this model.
+    expect(run).toHaveBeenCalledTimes(2); // 1 primary (stalled) + 1 fallback (succeeded on its first try).
+  });
+
+  it("REGRESSION (#9476): a genuinely transient error still gets the FULL retry budget (the break is narrow)", async () => {
+    // Guards against over-broadening the break: only the non-transient deadline/rate-limit/config signals
+    // short-circuit. A dropped connection must still be retried up to the budget.
+    let primaryAttempts = 0;
+    const run = vi.fn(async (model: string) => {
+      if (model === "fallback") return { response: reviewJson() };
+      primaryAttempts += 1;
+      throw new Error("ECONNRESET");
+    });
+    const env = createTestEnv({ AI: { run } as unknown as Ai });
+    const diagnostics: Array<{ status: string; model: string }> = [];
+    await runWorkersOpinion(env, "primary", "fallback", "sys", "user", 256, diagnostics as never);
+    expect(primaryAttempts).toBe(3);
+  });
+
   it("REGRESSION (#5385-sentry, GITTENSORY-K/8): runWorkersOpinion stops retrying a model after ONE 429 rate-limit error, same as a CLI timeout", async () => {
     let primaryAttempts = 0;
     const run = vi.fn(async (model: string) => {

test/unit/selfhost-ai.test.ts

@@ -1,4 +1,4 @@
-import { chmodSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs";
+import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { delimiter, join } from "node:path";
 import { afterEach, describe, expect, it, vi } from "vitest";
@@ -1603,9 +1603,15 @@ describe("subscription CLI helpers + fail-safe", () => {
     const systemAppend = "REPOSITORY REVIEW INSTRUCTIONS: Follow async-error conventions.";
     let seen: string[] = [];
     let capturedInput = "";
+    // #9479: the per-call temp dir is now removed once the subprocess finishes, so the appended-prompt file
+    // must be read WHILE the CLI would still be running -- i.e. inside the spawn stub -- not after the call
+    // returns. Reading it here also matches reality more closely: the file exists exactly for the CLI's lifetime.
+    let capturedSystemAppendFile: string | undefined;
     const cap: StubSpawn = async (_c, a, o) => {
       seen = a;
       capturedInput = o.input ?? "";
+      const flagAt = a.indexOf("--append-system-prompt-file");
+      if (flagAt > -1) capturedSystemAppendFile = readFileSync(a[flagAt + 1] as string, "utf8");
       return { stdout: JSON.stringify({ type: "result", result: "ok" }), code: 0 };
     };
     await createClaudeCodeAi({ CLAUDE_CODE_OAUTH_TOKEN: "t" }, cap).run("", {
@@ -1625,8 +1631,10 @@ describe("subscription CLI helpers + fail-safe", () => {
     expect(capturedInput).toContain("Review this diff.");
     const flagIndex = seen.indexOf("--append-system-prompt-file");
     expect(flagIndex).toBeGreaterThan(-1);
-    const filePath = seen[flagIndex + 1] as string;
-    expect(readFileSync(filePath, "utf8")).toBe(systemAppend);
+    expect(capturedSystemAppendFile).toBe(systemAppend);
+    // ... and the directory holding it does not outlive the call (#9479): these dirs accumulated on the
+    // container's writable layer forever, with repo review instructions left on disk.
+    expect(existsSync(seen[flagIndex + 1] as string)).toBe(false);
 
     await createClaudeCodeAi({ CLAUDE_CODE_OAUTH_TOKEN: "t" }, cap).run("", {
       prompt: "Review this diff.",