Merge pull request #5980 from JSONbored/fix/ai-provider-structural-failure-cooldown

fix(selfhost): give structural AI-provider config errors a long circuit-breaker cooldown

Author: JSONbored

src/selfhost/ai.ts

@@ -6,6 +6,7 @@
 // review proceeds deterministically. Every path returns `{ response: string }` (or throws → the caller
 // records an error and degrades — never a silent wrong answer).
 
+import { isStructuralProviderConfigError } from "../services/ai-review";
 import type { AiContentBlock, CombineStrategy, OnMerge } from "../services/ai-review";
 import { isConfiguredSelfHostProvider, resolveConfiguredProviderNames } from "./ai-config";
 export { assertNoLegacySharedAiEnv } from "./ai-config";
@@ -1148,7 +1149,17 @@ function recordAiProvidersExhausted(): void {
 // provider can be skipped fast without affecting readiness semantics.
 const AI_PROVIDER_FAILURE_THRESHOLD = 3;
 const AI_PROVIDER_COOLDOWN_MS = 60_000;
-const aiProviderCircuits = new Map<string, { failures: number; cooldownUntil: number }>();
+// A STRUCTURAL failure (bad/missing credentials -- isStructuralProviderConfigError) is deterministic: unlike a
+// transient outage, the same provider will fail identically on the very next attempt too, so there is no reason
+// to wait for AI_PROVIDER_FAILURE_THRESHOLD consecutive failures before opening the circuit, and no reason to
+// re-check nearly as often once it's open. Confirmed live (GITTENSORY-K/8): a container with codex's credential
+// file simply never present generated 2094 + 544 events over 16 days under the 60s/3-failure defaults -- each
+// cooldown expiry let exactly one attempt through, which immediately re-failed and re-opened the circuit for
+// another 60s, forever. A much longer cooldown still re-checks periodically (so a fixed credential is picked
+// back up within the hour, not stuck until a manual restart) without hammering a known-broken provider on every
+// incoming review.
+const AI_PROVIDER_STRUCTURAL_COOLDOWN_MS = 60 * 60_000;
+const aiProviderCircuits = new Map<string, { failures: number; cooldownUntil: number; structural: boolean }>();
 const EXPECTED_EMBEDDING_ROUTING_ERRORS = new Set(["claude_code_no_embed", "codex_no_embed"]);
 
 /** Test-only reset so circuit state from one test can't leak into the next (module-level map). */
@@ -1250,7 +1261,9 @@ async function runProviderWithOtel(
   if (circuit && circuit.cooldownUntil > Date.now()) {
     incr("loopover_ai_provider_circuit_open_total", { provider: provider.name });
     throw new Error(
-      `circuit_open: provider "${provider.name}" is in cooldown after ${AI_PROVIDER_FAILURE_THRESHOLD} consecutive failures — skipping this attempt`,
+      circuit.structural
+        ? `circuit_open: provider "${provider.name}" has a structural config error (bad/missing credentials) — skipping until the cooldown expires; fix the underlying config, then restart`
+        : `circuit_open: provider "${provider.name}" is in cooldown after ${AI_PROVIDER_FAILURE_THRESHOLD} consecutive failures — skipping this attempt`,
     );
   }
   const requestKindLabel = requestKind(options);
@@ -1290,9 +1303,16 @@ async function runProviderWithOtel(
     // this catch runs, and computing `failures` from it would clobber a sibling call's write (lost-update race)
     // instead of accumulating. No `await` between this read and the `.set()` below, so it's race-free.
     const failures = (aiProviderCircuits.get(provider.name)?.failures ?? 0) + 1;
+    // A structural failure is deterministic (see AI_PROVIDER_STRUCTURAL_COOLDOWN_MS above) -- open the circuit
+    // on the FIRST failure, not after AI_PROVIDER_FAILURE_THRESHOLD consecutive ones, at the much longer cooldown.
+    const structural = isStructuralProviderConfigError(error);
     aiProviderCircuits.set(provider.name, {
       failures,
-      cooldownUntil: failures >= AI_PROVIDER_FAILURE_THRESHOLD ? Date.now() + AI_PROVIDER_COOLDOWN_MS : 0,
+      cooldownUntil:
+        structural || failures >= AI_PROVIDER_FAILURE_THRESHOLD
+          ? Date.now() + (structural ? AI_PROVIDER_STRUCTURAL_COOLDOWN_MS : AI_PROVIDER_COOLDOWN_MS)
+          : 0,
+      structural,
     });
     throw error;
   }

src/services/ai-review.ts

@@ -1020,6 +1020,18 @@ export function isRateLimitError(error: unknown): boolean {
   return error instanceof Error && /_(?:http|error)_429$/.test(error.message);
 }
 
+/** True for a provider's own STRUCTURAL misconfiguration signal (`src/selfhost/ai.ts`'s
+ *  `codex_auth_not_configured` / `codex_no_auth` — a missing or expired credential file). Unlike a transient
+ *  timeout or rate limit, this will fail identically on every future attempt until an operator re-runs
+ *  `codex auth` -- confirmed live (GITTENSORY-K/8: 2094 + 544 events over 16 days from one unfixed
+ *  misconfiguration, the credential file was never present the whole time). Mirrors
+ *  {@link isSubscriptionCliTimeout}/{@link isRateLimitError}'s identical non-transient-error short-circuit.
+ *  Exported so `src/selfhost/ai.ts`'s circuit breaker can give this failure class a much longer cooldown
+ *  than a genuinely transient one. */
+export function isStructuralProviderConfigError(error: unknown): boolean {
+  return error instanceof Error && /^codex_(?:auth_not_configured|no_auth):/.test(error.message);
+}
+
 /** Cap on the diagnostic prefix logged for an unparseable model response (#observability-unparseable) -- long
  *  enough to tell a markdown-fenced/truncated-mid-JSON/plain-prose response apart, short enough to never dump
  *  a large chunk of model output into Sentry/audit context. */
@@ -1148,7 +1160,10 @@ async function runWorkersOpinion(
         // this attempt will not have cleared by the next attempt a few hundred ms later, so an immediate
         // same-model retry burns the remaining budget for zero additional chance of success -- move straight
         // to the fallback model instead, which may be on a different account/provider entirely.
-        if (isSubscriptionCliTimeout(error) || isRateLimitError(error)) break;
+        // 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;
       }
     }
   }
@@ -1859,8 +1874,9 @@ async function runDualAiTieBreakJudgeCall(
           status: "provider_error",
           error: errorMessage(error),
         });
-        // See runWorkersOpinion's identical guard: a CLI timeout or 429 will not resolve by retrying the same model.
-        if (isSubscriptionCliTimeout(error) || isRateLimitError(error)) break;
+        // See runWorkersOpinion's identical guard: a CLI timeout, 429, or structural config error (bad/missing
+        // credentials) will not resolve by retrying the same model.
+        if (isSubscriptionCliTimeout(error) || isRateLimitError(error) || isStructuralProviderConfigError(error)) break;
       }
     }
   }

test/unit/ai-review.test.ts

@@ -4,6 +4,7 @@ import {
   BEST_REVIEW_MODELS,
   buildTestEvidencePromptSection,
   callAiProvider,
+  isStructuralProviderConfigError,
   resolveEffectiveAiReviewOnMerge,
   resolveEffectiveAiReviewPlan,
   runLoopOverAiReview,
@@ -2877,6 +2878,21 @@ describe("pure helpers", () => {
       expect(run).toHaveBeenCalledTimes(2); // 1 primary (rate-limited) + 1 fallback (succeeded on its first try).
     });
 
+    it("runDualAiTieBreakJudgeCall stops retrying a model after ONE structural codex-auth config error, same as a CLI timeout or 429 (GITTENSORY-K/8)", async () => {
+      let primaryAttempts = 0;
+      const run = vi.fn(async (model: string) => {
+        if (model === "fallback") return { response: '{"favored":"reviewer_1"}' };
+        primaryAttempts += 1;
+        throw new Error("codex_auth_not_configured: ~/.codex/auth.json not found");
+      });
+      const env = createTestEnv({ AI: { run } as unknown as Ai });
+      const diagnostics: Array<{ status: string; model: string }> = [];
+      const parsed = await runDualAiTieBreakJudgeCall(env, "primary", "fallback", blockedA, clean, false, diagnostics as never);
+      expect(parsed?.verdict).toBe("reviewer_1");
+      expect(primaryAttempts).toBe(1); // NOT 3 -- a structural config error is deterministic, so retrying is pointless.
+      expect(run).toHaveBeenCalledTimes(2); // 1 primary (structural failure) + 1 fallback (succeeded on its first try).
+    });
+
     it("resolveDualAiTieBreakWithOrderStability returns inconclusive when judge output never parses", async () => {
       const run = vi.fn(async () => ({ response: "not-json" }));
       const env = createTestEnv({ AI: { run } as unknown as Ai });
@@ -3043,6 +3059,31 @@ describe("pure helpers", () => {
     expect(run).toHaveBeenCalledTimes(2); // 1 primary (rate-limited) + 1 fallback (succeeded on its first try).
   });
 
+  it("runWorkersOpinion stops retrying a model after ONE structural codex-auth config error, same as a CLI timeout or 429 (GITTENSORY-K/8)", async () => {
+    let primaryAttempts = 0;
+    const run = vi.fn(async (model: string) => {
+      if (model === "fallback") return { response: reviewJson() };
+      primaryAttempts += 1;
+      throw new Error("codex_no_auth: auth.json missing or expired");
+    });
+    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 -- a structural config error is deterministic, so retrying is pointless.
+    expect(run).toHaveBeenCalledTimes(2); // 1 primary (structural failure) + 1 fallback (succeeded on its first try).
+  });
+
+  it("isStructuralProviderConfigError matches only codex's own structural-config error messages, not other Errors or non-Error throws (GITTENSORY-K/8)", () => {
+    expect(isStructuralProviderConfigError(new Error("codex_auth_not_configured: ~/.codex/auth.json not found"))).toBe(true);
+    expect(isStructuralProviderConfigError(new Error("codex_no_auth: auth.json missing or expired"))).toBe(true);
+    expect(isStructuralProviderConfigError(new Error("connection reset"))).toBe(false);
+    // Anchored ("^codex_...") -- a wrapped/rethrown message doesn't match, only the exact provider-level throw does.
+    expect(isStructuralProviderConfigError(new Error("wrapped: codex_auth_not_configured: nested"))).toBe(false);
+    expect(isStructuralProviderConfigError("codex_auth_not_configured: not an Error instance")).toBe(false);
+    expect(isStructuralProviderConfigError(undefined)).toBe(false);
+  });
+
   it("runWorkersOpinion still retries a genuinely transient (non-timeout, non-429) error up to the full budget", async () => {
     let attempts = 0;
     const run = vi.fn(async () => {

test/unit/selfhost-ai.test.ts

@@ -735,6 +735,63 @@ describe("per-provider circuit breaker (#2540 — skip fast during a sustained o
     // call exhausted the (single-provider) chain — a circuit-open throw still counts as a chain exhaustion.
     expect(isAiProviderHealthy()).toBe(false);
   });
+
+  it("opens the circuit on the very FIRST structural codex-auth failure, not after 3 (GITTENSORY-K/8 — a deterministic failure shouldn't pay for 3 real attempts)", async () => {
+    const calls = vi.fn(async () => {
+      throw new Error("codex_auth_not_configured: ~/.codex/auth.json not found");
+    });
+    const brokenAuth = { name: "codex", ai: { run: calls } };
+    await expect(createChainAi([brokenAuth]).run("m", { prompt: "x" })).rejects.toThrow(/codex_auth_not_configured/);
+    expect(calls).toHaveBeenCalledTimes(1);
+    // The very NEXT call is already skipped — no need to accumulate AI_PROVIDER_FAILURE_THRESHOLD failures first.
+    await expect(createChainAi([brokenAuth]).run("m", { prompt: "x" })).rejects.toThrow(
+      /circuit_open: provider "codex" has a structural config error/,
+    );
+    expect(calls).toHaveBeenCalledTimes(1); // unchanged — the real provider was never reached a 2nd time
+    const metrics = await renderMetrics();
+    expect(metrics).toContain('loopover_ai_provider_circuit_open_total{provider="codex"} 1');
+    expect(metrics).toContain('loopover_ai_provider_failures_total{provider="codex"} 1'); // NOT 3
+  });
+
+  it("uses the long structural cooldown (1h), not the 60s transient cooldown — still open just past 60s, reachable again only past 1h", async () => {
+    vi.useFakeTimers();
+    const calls = vi.fn(async () => {
+      throw new Error("codex_no_auth: auth.json missing or expired");
+    });
+    const brokenAuth = { name: "codex-2", ai: { run: calls } };
+    await expect(createChainAi([brokenAuth]).run("m", { prompt: "x" })).rejects.toThrow(/codex_no_auth/);
+    expect(calls).toHaveBeenCalledTimes(1);
+    // Past the ORDINARY 60s transient cooldown, the structural circuit must still be open.
+    await vi.advanceTimersByTimeAsync(60_001);
+    await expect(createChainAi([brokenAuth]).run("m", { prompt: "x" })).rejects.toThrow(/circuit_open/);
+    expect(calls).toHaveBeenCalledTimes(1);
+    // Past the full 1h structural cooldown, the real provider is reachable again (e.g. to notice a fixed credential).
+    await vi.advanceTimersByTimeAsync(3_600_000);
+    await expect(createChainAi([brokenAuth]).run("m", { prompt: "x" })).rejects.toThrow(/codex_no_auth/);
+    expect(calls).toHaveBeenCalledTimes(2); // the real provider WAS invoked this time
+  });
+
+  it("a success fully clears a structural circuit entry — a later transient failure starts fresh, not at the 1h structural cooldown", async () => {
+    vi.useFakeTimers();
+    let mode: "structural-fail" | "succeed" | "transient-fail" = "structural-fail";
+    const calls = vi.fn(async () => {
+      if (mode === "succeed") return { response: "ok" };
+      if (mode === "transient-fail") throw new Error("connection reset");
+      throw new Error("codex_auth_not_configured: ~/.codex/auth.json not found");
+    });
+    const provider = { name: "codex-3", ai: { run: calls } };
+    await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(/codex_auth_not_configured/);
+    // Credential fixed; jump past the 1h structural cooldown so the real provider is reachable again.
+    await vi.advanceTimersByTimeAsync(3_600_001);
+    mode = "succeed";
+    await expect(createChainAi([provider]).run("m", { prompt: "x" })).resolves.toEqual({ response: "ok" });
+    // A later, unrelated transient failure must start from a clean slate (1 failure, 60s-tier), not reopen
+    // immediately at the 1h structural cooldown left over from before the success.
+    mode = "transient-fail";
+    await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(/connection reset/);
+    await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(/connection reset/); // 2nd failure, still below threshold 3 — reaches the real provider again
+    expect(calls).toHaveBeenCalledTimes(4); // structural-fail, succeed, transient-fail x2 — all reached the real provider.ai.run
+  });
 });
 
 describe("isAiProviderHealthy (readiness streak, #2497)", () => {