feat(selfhost): add a per-analyzer circuit breaker to review-enrichment

review-enrichment/src/analyzer-circuit-breaker.ts

@@ -0,0 +1,83 @@
+// Per-analyzer circuit breaker (#2541). Analyzers that depend on a third-party HTTP API (registry lookups,
+// GitHub API calls, endoflife.date, etc) have no memory of recent failures by default -- every incoming
+// enrichment request re-attempts a currently-unhealthy dependency from a cold state, even seconds after an
+// identical call just timed out or errored. Trip a short, in-process cooldown after a run of CONSECUTIVE
+// thrown failures (a timeout counts -- runWithTimeout's rejection is a thrown failure) and skip that analyzer
+// entirely -- no network/CLI call at all -- for the cooldown window, falling through the SAME plan.skipped
+// path any other skip reason already uses. In-process only (no persistence layer): review-enrichment is a
+// single long-lived process (Railway), matching the main app's equivalent per-provider AI circuit breaker
+// (src/selfhost/ai.ts's createChainAi).
+//
+// Half-open probing: review-enrichment serves CONCURRENT requests (one per in-flight PR review), so once the
+// cooldown expires, a burst of near-simultaneous requests would all see the circuit as "not open" and all
+// retry the same still-unhealthy dependency at once. `isAnalyzerCircuitOpen` claims a single probe slot the
+// first time it observes an expired cooldown; every other caller sees the circuit as still open until that
+// one probe resolves (recordAnalyzerCircuitSuccess/Failure). `releaseAnalyzerCircuitProbe` frees a claimed
+// slot without recording an outcome, for the case where the analyzer never actually ran (budget/timeout
+// capped in brief.ts before reaching the real call) -- otherwise a stuck claim would block re-probing forever.
+import type { AnalyzerName } from "./analyzers/types.js";
+
+const ANALYZER_CIRCUIT_FAILURE_STREAK = 3;
+const ANALYZER_CIRCUIT_COOLDOWN_MS = 5 * 60_000;
+
+interface AnalyzerCircuitState {
+  consecutiveFailures: number;
+  cooldownUntilMs: number;
+  probeClaimed: boolean;
+}
+
+const analyzerCircuits = new Map<AnalyzerName, AnalyzerCircuitState>();
+
+/** True while `name`'s breaker should skip the caller: either still within the full cooldown window, or past
+ *  it but another caller already claimed this cycle's single half-open probe. The FIRST caller to observe an
+ *  expired cooldown claims the probe as a side effect and gets `false` (proceed) -- this is the one function
+ *  planning calls to decide runnable vs skipped, so the claim has to happen here, not at execution time.
+ *
+ *  `cooldownUntilMs === 0` means the circuit has NEVER actually tripped (below the streak threshold) --
+ *  recordAnalyzerCircuitFailure only sets a non-zero cooldownUntilMs once consecutiveFailures reaches the
+ *  threshold, so this is a reliable "never opened" check. Without it, a caller after just 1-2 failures would
+ *  claim the half-open probe slot too, spuriously skipping a concurrent second caller as circuit_open even
+ *  though the breaker was never actually open. */
+export function isAnalyzerCircuitOpen(name: AnalyzerName, nowMs = Date.now()): boolean {
+  const state = analyzerCircuits.get(name);
+  if (state === undefined || state.cooldownUntilMs === 0) return false;
+  if (state.cooldownUntilMs > nowMs) return true;
+  if (state.probeClaimed) return true;
+  state.probeClaimed = true;
+  return false;
+}
+
+/** A completed run (whether a clean "ok" or a non-throwing "degraded"/"capped" partial result) resets the
+ *  streak -- the dependency responded, so it is not the failure mode this breaker guards against. */
+export function recordAnalyzerCircuitSuccess(name: AnalyzerName): void {
+  analyzerCircuits.delete(name);
+}
+
+/** A THROWN failure (including the analyzer_timeout rejection) is the signal this breaker tracks. Trips the
+ *  cooldown once the consecutive count reaches the streak threshold; stays open (extends nothing further --
+ *  the analyzer is simply skipped while open, so no additional failures accrue until it is tried again). A
+ *  half-open probe's failure re-extends the cooldown via this same threshold check, since consecutiveFailures
+ *  is already at/above it by the time a probe can be claimed -- no separate re-trip path needed. */
+export function recordAnalyzerCircuitFailure(name: AnalyzerName, nowMs = Date.now()): void {
+  const state = analyzerCircuits.get(name) ?? { consecutiveFailures: 0, cooldownUntilMs: 0, probeClaimed: false };
+  state.consecutiveFailures += 1;
+  state.probeClaimed = false;
+  if (state.consecutiveFailures >= ANALYZER_CIRCUIT_FAILURE_STREAK) {
+    state.cooldownUntilMs = nowMs + ANALYZER_CIRCUIT_COOLDOWN_MS;
+  }
+  analyzerCircuits.set(name, state);
+}
+
+/** Frees a claimed half-open probe WITHOUT recording success or failure -- for when the probing attempt never
+ *  actually reached the analyzer call (capped by budget/timeout in brief.ts first). Safe no-op when `name` has
+ *  no circuit state or no claimed probe, so callers can call this unconditionally on every capped early-return
+ *  without needing to know whether this particular call was the one that claimed the probe. */
+export function releaseAnalyzerCircuitProbe(name: AnalyzerName): void {
+  const state = analyzerCircuits.get(name);
+  if (state !== undefined) state.probeClaimed = false;
+}
+
+/** Test-only reset so circuit-breaker state from one test can't leak into the next (module-level Map). */
+export function resetAnalyzerCircuitsForTest(): void {
+  analyzerCircuits.clear();
+}

review-enrichment/src/brief.ts

@@ -14,6 +14,11 @@ import type {
   AnalyzerRunContext,
   AnalyzerCostClass,
 } from "./analyzers/types.js";
+import {
+  recordAnalyzerCircuitFailure,
+  recordAnalyzerCircuitSuccess,
+  releaseAnalyzerCircuitProbe,
+} from "./analyzer-circuit-breaker.js";
 import {
   createAnalysisContext,
   type AnalysisContext,
@@ -258,6 +263,10 @@ export async function buildBrief(
       };
       partial = true;
       analysis.metrics.recordCappedWork("analyzer_budget", 1);
+      // #2541: budget exhaustion, not a dependency-health signal -- if this call had claimed the circuit
+      // breaker's half-open probe (isAnalyzerCircuitOpen), free it so a later request can still probe rather
+      // than leaving the slot claimed forever with no outcome ever recorded.
+      releaseAnalyzerCircuitProbe(name);
       return;
     }
     const timeoutMs = analyzerTimeoutMs(
@@ -279,6 +288,8 @@ export async function buildBrief(
       };
       partial = true;
       analysis.metrics.recordCappedWork(`analyzer_${item.descriptor.cost}`, 1);
+      // #2541: same as above -- release a claimed half-open probe without recording an outcome.
+      releaseAnalyzerCircuitProbe(name);
       return;
     }
     try {
@@ -296,6 +307,11 @@ export async function buildBrief(
         },
       );
       findings[name] = result as never;
+      // #2541: the analyzer completed WITHOUT throwing -- whether "ok" or a non-throwing partial/degraded
+      // result (its own internal cap, not a dependency failure) -- so the dependency responded. Reset the
+      // circuit rather than only resetting on a clean "ok"; a benign internal partial must not itself count
+      // toward tripping the breaker.
+      recordAnalyzerCircuitSuccess(name);
       if (resultIsPartial(result) || diagnostics.partialStatus === "partial") {
         const status = statusFromDiagnostics(diagnostics, "degraded");
         const partialReason = publicPartialReason(
@@ -342,6 +358,10 @@ export async function buildBrief(
         };
       }
     } catch (error) {
+      // #2541: a THROWN failure (including the analyzer_timeout rejection from runWithTimeout) is the signal
+      // the circuit breaker tracks -- the dependency did not respond at all, unlike a non-throwing partial
+      // result above.
+      recordAnalyzerCircuitFailure(name);
       const status = timeoutStatus(error, diagnostics);
       const partialReason = publicPartialReason(diagnostics.partialReason, "analyzer_error");
       analyzerStatus[name] = status;
@@ -374,6 +394,18 @@ export async function buildBrief(
     }
   }
 
+  // #2541 (cost-class parallelization evaluated, NOT implemented): cost classes run strictly sequentially --
+  // this loop awaits each class's bounded worker pool (runWithConcurrency) before starting the next -- with
+  // cheaper/more-certain classes (local, then registry) always draining before expensive/less-essential ones
+  // (github-heavy, tooling). This is deliberate prioritization, not an oversight: it guarantees the cheap,
+  // always-safe signals are collected first, and a shrinking remainingMs budget (see analyzerTimeoutMs above)
+  // correctly starves LATER, less-essential classes first when time runs short -- never the reverse. Running
+  // every class's worker pool concurrently would sum EVERY class's concurrency limit at once (8+3+2+1+1 = 15
+  // simultaneous external calls on the "deep" profile instead of at most 8), spiking the third-party burst
+  // rate exactly when third-party health is already the concern this issue is about, and would let an
+  // expensive/uncertain "tooling" call start competing for budget with a cheap "local" one instead of only
+  // running once local has had its turn. That risk is not "low", so this stays sequential; the per-analyzer
+  // circuit breaker above is the intended fix for a specific unhealthy dependency, not a scheduling change.
   for (const cost of COST_ORDER) {
     const items = plan.runnable.filter((item) => item.descriptor.cost === cost);
     if (!items.length) continue;

review-enrichment/src/scheduler.ts

@@ -1,4 +1,5 @@
 import type { AnalysisContext } from "./analysis-context.js";
+import { isAnalyzerCircuitOpen } from "./analyzer-circuit-breaker.js";
 import {
   ANALYZER_NAMES,
   getAnalyzerDescriptor,
@@ -321,7 +322,19 @@ function skipReasonForAnalyzer(
     return "missing_github_token";
   }
 
-  return inputSkipReason(descriptor.name, analysis, req);
+  const inputSkip = inputSkipReason(descriptor.name, analysis, req);
+  if (inputSkip) return inputSkip;
+
+  // #2541: checked LAST, only once every other skip reason has cleared -- isAnalyzerCircuitOpen claims a
+  // single half-open probe as a side effect when the cooldown has expired, and that claim is only ever
+  // released inside runAnalyzer (brief.ts), which never runs for a plan.skipped item. Checking this any
+  // earlier could claim the probe for an analyzer that's about to be skipped for an UNRELATED reason (a
+  // missing head SHA, no dependency manifest, etc.), leaking the claim forever with no outcome ever recorded.
+  // An EXPLICIT request (req.analyzers) does not bypass this -- the circuit is about the dependency being
+  // down right now, which an explicit request can't fix.
+  if (isAnalyzerCircuitOpen(descriptor.name)) return "circuit_open";
+
+  return null;
 }
 
 function inputSkipReason(