diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 346499065..f9cc55ad5 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -598,6 +598,21 @@ async function isolatedCliCwd(): Promise { 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 { + 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 { 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 ); 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 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 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); } }, }; diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index eb7b39341..275a080dd 100644 --- a/src/services/ai-review.ts +++ b/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: ` 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; } } } diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 1f65e1a12..8088a27dd 100644 --- a/test/unit/ai-review.test.ts +++ b/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: ` 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) => { diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index f9c1ea547..cc63bd108 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/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.",