diff --git a/prometheus/rules/alerts.yml b/prometheus/rules/alerts.yml index 7d4f5bdd8b..006d51a137 100644 --- a/prometheus/rules/alerts.yml +++ b/prometheus/rules/alerts.yml @@ -369,3 +369,36 @@ groups: summary: "gittensory p95 request latency above 1s" description: "p95 HTTP request latency is {{ $value | printf \"%.2f\" }}s over the last 5m (sustained 10m), breaching the 1s SLO." runbook: "Check whether slowness is queue/DB/AI-bound: correlate with gittensory_queue_pending and Qdrant/AI latency. A rising p95 with flat error rate usually means a saturated dependency, not a bug." + + # ── AI review reliability (dual-AI combiner + per-provider circuit breaker, #2540) ─ + - name: gittensory-ai-review + rules: + - alert: GittensoryAiReviewInconclusiveSpike + # `inconclusive` means the AI review pipeline could not produce a usable verdict (every reviewer + # opinion missing/unparseable, or a required opinion never came back) -- the review still runs + # deterministically, but dual-AI review is repeatedly failing to add value. Absolute-increase + # threshold (matching GittensoryDeadLetterJobsGrowing above): there's no clean matching-cardinality + # denominator (total review attempts aren't broken out per-mode the same way), so a ratio query + # would need an unrelated series. > 5 in 30m tolerates the occasional one-off degrade. + expr: increase(gittensory_ai_review_inconclusive_total[30m]) > 5 + for: 10m + labels: + severity: warning + annotations: + summary: "gittensory AI review is repeatedly inconclusive" + description: "{{ $value | printf \"%.0f\" }} AI review(s) came back inconclusive over the last 30m (sustained 10m). Dual-AI review is repeatedly failing to produce a usable verdict." + runbook: "Check provider health and circuit-breaker state (gittensory_ai_provider_failures_total / gittensory_ai_provider_circuit_open_total) and verify AI_PROVIDER credentials are still valid for every configured reviewer." + + - alert: GittensoryAiProviderCircuitOpen + # A provider's circuit breaker opens after AI_PROVIDER_FAILURE_THRESHOLD consecutive failures and + # short-circuits further attempts for a cooldown window -- ANY circuit-open event in 15m means that + # provider has been failing repeatedly and calls are being skipped fast rather than retried at full + # cost. Same absolute-increase style as GittensoryDeadLetterJobsGrowing: any occurrence is worth a look. + expr: increase(gittensory_ai_provider_circuit_open_total[15m]) > 0 + for: 5m + labels: + severity: warning + annotations: + summary: "gittensory AI provider {{ $labels.provider }} circuit breaker is open" + description: "Provider {{ $labels.provider }} has failed repeatedly and its circuit breaker is skipping calls fast during its cooldown (sustained 5m)." + runbook: "Check that provider's credentials/reachability (CLI auth for claude-code/codex, or the configured API key/base URL for HTTP providers) via gittensory_ai_provider_failures_total{provider=\"...\"} and recent selfhost_ai_provider_failed logs." diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 091311a987..ff0ac1ffe4 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -604,6 +604,20 @@ export function resetAiProviderHealthForTest(): void { aiConsecutiveFailures = 0; } +// Per-provider circuit breaker (#2540): a provider that is failing hard (bad credential, sustained outage) +// otherwise pays the FULL cost of a fresh attempt (a real HTTP call, or a real CLI subprocess spawn) on +// every single review during the outage. This is independent of `aiConsecutiveFailures` above -- that streak +// tracks whole-CHAIN exhaustion for /ready; this tracks one PROVIDER's own reliability so a known-broken +// 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(); + +/** Test-only reset so circuit state from one test can't leak into the next (module-level map). */ +export function resetAiProviderCircuitBreakerForTest(): void { + aiProviderCircuits.clear(); +} + /** Whether a missing-CLI boot check should force /ready unhealthy: only when EVERY configured provider is * among the missing-CLI set, i.e. the whole AI_PROVIDER chain has zero chance of working -- not just one * provider within a chain that has a working fallback (another present CLI, or an HTTP-based provider, @@ -674,16 +688,39 @@ function requestKind(options: AiRunOptions): "embedding" | "review" { return Array.isArray(options.text) ? "embedding" : "review"; } -function runProviderWithOtel( +async function runProviderWithOtel( provider: { name: string; ai: SelfHostAi }, model: string, options: AiRunOptions, ): Promise { - return withReviewSpan( - "selfhost.ai.provider", - { "ai.provider": provider.name, "ai.model": model || "default", "ai.request_kind": requestKind(options) }, - () => provider.ai.run(model, options), - ); + const circuit = aiProviderCircuits.get(provider.name); + if (circuit && circuit.cooldownUntil > Date.now()) { + incr("gittensory_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`, + ); + } + try { + const result = await withReviewSpan( + "selfhost.ai.provider", + { "ai.provider": provider.name, "ai.model": model || "default", "ai.request_kind": requestKind(options) }, + () => provider.ai.run(model, options), + ); + aiProviderCircuits.delete(provider.name); + return result; + } catch (error) { + incr("gittensory_ai_provider_failures_total", { provider: provider.name }); + // Re-read the map here rather than reusing the `circuit` captured above: that read happened BEFORE the + // `await` on the real provider call, so under concurrent same-provider calls it can be stale by the time + // 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; + aiProviderCircuits.set(provider.name, { + failures, + cooldownUntil: failures >= AI_PROVIDER_FAILURE_THRESHOLD ? Date.now() + AI_PROVIDER_COOLDOWN_MS : 0, + }); + throw error; + } } /** Build one provider adapter by name. Provider config stays explicit so dual-provider setups cannot accidentally @@ -798,6 +835,16 @@ export function resolveAiReviewerPlan( const names = resolveProviderNames(env); if (names.length === 0) return undefined; if (names.length === 1) return { reviewers: [{ model: names[0] as string }], combine: "single", onMerge: undefined }; + // Fail loud when the two SLOTS the dual-review plan actually uses (the first two names) are the same + // provider: routeProviders' `byName` map collapses duplicate provider names to one runtime instance, so + // "dual review" would silently become "one provider called twice" -- no independent second opinion, and + // that provider's outage takes down both slots. A THIRD+ duplicate further down the list is fine; only + // the first two matter because resolveAiReviewerPlan below caps reviewers at names.slice(0, 2). + if (names[0] === names[1]) { + throw new Error( + `ai_reviewer_providers_not_distinct: AI_PROVIDER lists "${names[0]}" for both dual-review reviewer slots — configure two distinct providers (e.g. AI_PROVIDER=claude-code,codex) for independent dual review, or a single provider (AI_PROVIDER=codex) for single-reviewer mode.`, + ); + } const rawCombine = (env.AI_COMBINE ?? "").trim().toLowerCase() as CombineStrategy; const combine: CombineStrategy = COMBINE_STRATEGIES.has(rawCombine) ? rawCombine : "synthesis"; const rawOnMerge = (env.AI_ON_MERGE ?? "").trim().toLowerCase() as OnMerge; diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 22d04a3969..09b5dd274b 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -25,6 +25,7 @@ import { sanitizePublicComment } from "../queue-intelligence"; import { defangReviewInput } from "../review/safety"; import { convergedFeatureActive } from "../review/feature-activation"; import { labelSelfHostReviewerModels, labelSelfHostReviewerNames, resolveConfiguredProviderNames } from "../selfhost/ai-config"; +import { incr } from "../selfhost/metrics"; import { errorMessage } from "../utils/json"; import type { ReviewProfile } from "../signals/focus-manifest"; @@ -1228,6 +1229,11 @@ export async function runGittensoryAiReview( reviewDiagnostics.some((diagnostic) => diagnostic.status === "unparseable_output")) ) inconclusive = true; + // Observability (#2540): the single canonical point where `inconclusive` reaches its final value for this + // review call -- increment exactly once here, never at the downstream consumers in queue/processors.ts that + // push an `ai_review_inconclusive` advisory finding off this same already-computed result (incrementing there + // too would double/triple-count one review). + if (inconclusive) incr("gittensory_ai_review_inconclusive_total", { mode: input.mode }); const advisoryNotes = reviewsForNotes.length > 0 ? (composeAdvisoryNotes(reviewsForNotes) ?? composeFallbackAdvisoryNotes(fallbackNotes)) diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 8d03225f0f..1d5b083117 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -6,6 +6,7 @@ import { type GittensoryAiReviewInput, } from "../../src/services/ai-review"; import { createTestEnv } from "../helpers/d1"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; const { parseModelReview, @@ -87,6 +88,7 @@ const baseInput: GittensoryAiReviewInput = { afterEach(() => { vi.unstubAllGlobals(); + resetMetrics(); }); describe("runGittensoryAiReview gating", () => { @@ -474,6 +476,8 @@ describe("runGittensoryAiReview block mode (consensus)", () => { expect(result.consensusDefect).toBeNull(); expect(result.inconclusive).toBe(true); // FAIL-CLOSED: a missing second opinion holds the PR, never passes it expect(result.advisoryNotes).not.toBeNull(); // notes still come from the one parseable opinion + // Observability (#2540): the single canonical increment fires once for this inconclusive review. + expect(await renderMetrics()).toContain('gittensory_ai_review_inconclusive_total{mode="block"} 1'); }); it("a clean dual review is NOT inconclusive (both models parsed, neither blocks → passes)", async () => { @@ -486,6 +490,8 @@ describe("runGittensoryAiReview block mode (consensus)", () => { }); expect(result.status === "ok" && result.consensusDefect).toBeNull(); expect(result.status === "ok" && result.inconclusive).toBe(false); + // A non-inconclusive review must NOT increment the inconclusive counter. + expect(await renderMetrics()).not.toContain("gittensory_ai_review_inconclusive_total"); }); it("block mode with BYOK: provider writes the advisory, the free Workers-AI pair drives consensus", async () => { diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index 8124c90fc4..f3b9731ec3 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -2,7 +2,7 @@ import { chmodSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { delimiter, join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { assertNoLegacySharedAiEnv, buildProvider, claudeErrorStatus, createAnthropicAi, createChainAi, createClaudeCodeAi, createCodexAi, createOpenAiCompatibleAi, createSelfHostAi, extractCliText, extractCliUsage, isAiProviderHealthy, markAiProviderUnhealthyAtBoot, resetAiProviderHealthForTest, resolveAiReviewerPlan, resolveClaudeCliTimeoutMs, resolveCodexCliTimeoutMs, resolveCodexEffort, resolveEffort, resolveModel, resolveProviderNames, resolveRequiredCliProviders, resolveSubscriptionCliPath, redactSecrets, routeProviders, shouldMarkAiProviderUnhealthyAtBoot, subscriptionCliEnv } from "../../src/selfhost/ai"; +import { assertNoLegacySharedAiEnv, buildProvider, claudeErrorStatus, createAnthropicAi, createChainAi, createClaudeCodeAi, createCodexAi, createOpenAiCompatibleAi, createSelfHostAi, extractCliText, extractCliUsage, isAiProviderHealthy, markAiProviderUnhealthyAtBoot, resetAiProviderCircuitBreakerForTest, resetAiProviderHealthForTest, resolveAiReviewerPlan, resolveClaudeCliTimeoutMs, resolveCodexCliTimeoutMs, resolveCodexEffort, resolveEffort, resolveModel, resolveProviderNames, resolveRequiredCliProviders, resolveSubscriptionCliPath, redactSecrets, routeProviders, shouldMarkAiProviderUnhealthyAtBoot, subscriptionCliEnv } from "../../src/selfhost/ai"; import { labelSelfHostReviewerModel } from "../../src/selfhost/ai-config"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; @@ -67,6 +67,7 @@ afterEach(() => { vi.unstubAllGlobals(); resetMetrics(); resetAiProviderHealthForTest(); + resetAiProviderCircuitBreakerForTest(); }); type SpawnResult = { stdout: string; code: number | null; stderr?: string }; @@ -195,6 +196,120 @@ describe("createChainAi (fallback)", () => { }); }); +describe("per-provider circuit breaker (#2540 — skip fast during a sustained outage)", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("opens the circuit after 3 consecutive failures; a call within the cooldown skips the real provider entirely", async () => { + const calls = vi.fn(async () => { + throw new Error("down"); + }); + const flaky = { name: "flaky-provider", ai: { run: calls } }; + // 3 consecutive failures via createChainAi (the shared chokepoint) opens the circuit. + for (let i = 0; i < 3; i += 1) { + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(); + } + expect(calls).toHaveBeenCalledTimes(3); + // A subsequent call within the cooldown window throws circuit_open WITHOUT invoking provider.ai.run again. + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(/circuit_open: provider "flaky-provider"/); + expect(calls).toHaveBeenCalledTimes(3); // unchanged — the real provider was never reached + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_ai_provider_circuit_open_total{provider="flaky-provider"} 1'); + expect(metrics).toContain('gittensory_ai_provider_failures_total{provider="flaky-provider"} 3'); + }); + + it("REGRESSION (gate finding): concurrent same-provider failures accumulate correctly (no lost-update race)", async () => { + // Each call previously captured the circuit's failure count BEFORE its own `await` on the real provider + // call, then wrote `(that stale count) + 1` back on failure. Firing several failing calls for the SAME + // provider concurrently (not sequentially) means every call reads the map before any of them have written — + // each one computes failures=1 from the same stale pre-await snapshot, and the last writer clobbers the + // rest, so the count never reaches the threshold no matter how many concurrent failures occur. Fixed by + // re-reading the map fresh inside the (synchronous, no-await) catch block right before the write. + const calls = vi.fn(async () => { + throw new Error("down"); + }); + const flaky = { name: "concurrent-flaky-provider", ai: { run: calls } }; + const results = await Promise.allSettled([ + createChainAi([flaky]).run("m", { prompt: "x" }), + createChainAi([flaky]).run("m", { prompt: "x" }), + createChainAi([flaky]).run("m", { prompt: "x" }), + ]); + expect(results.every((r) => r.status === "rejected")).toBe(true); + expect(calls).toHaveBeenCalledTimes(3); // all 3 concurrent calls reached the real (failing) provider + // The circuit must now be OPEN (3 accumulated failures met the threshold) — a 4th call is skipped fast. + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(/circuit_open: provider "concurrent-flaky-provider"/); + expect(calls).toHaveBeenCalledTimes(3); // unchanged — the 4th call never reached the real provider + }); + + it("lets a call through to the real provider again after the cooldown elapses", async () => { + vi.useFakeTimers(); + const calls = vi.fn(async () => { + throw new Error("down"); + }); + const flaky = { name: "flaky-provider-2", ai: { run: calls } }; + for (let i = 0; i < 3; i += 1) { + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(); + } + expect(calls).toHaveBeenCalledTimes(3); + // Still within cooldown: skipped. + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(/circuit_open/); + expect(calls).toHaveBeenCalledTimes(3); + // Advance past the 60s cooldown — the next call reaches the real provider again. + await vi.advanceTimersByTimeAsync(60_001); + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(/down/); + expect(calls).toHaveBeenCalledTimes(4); // the real provider WAS invoked this time + }); + + it("a success resets the failure count so one subsequent isolated failure does not reopen the circuit", async () => { + let shouldFail = true; + const calls = vi.fn(async () => { + if (shouldFail) throw new Error("down"); + return { response: "ok" }; + }); + const provider = { name: "recovering-provider", ai: { run: calls } }; + // Two failures (below the threshold of 3). + await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(); + await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(); + // A success resets the failure count to 0. + shouldFail = false; + await expect(createChainAi([provider]).run("m", { prompt: "x" })).resolves.toEqual({ response: "ok" }); + // One more isolated failure afterward must NOT immediately reopen the circuit (count was reset, not just decremented). + shouldFail = true; + await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(/down/); // real failure, not circuit_open + // A second call right after must still reach the real provider (only 1 failure since the reset, below threshold 3). + await expect(createChainAi([provider]).run("m", { prompt: "x" })).rejects.toThrow(/down/); + expect(calls).toHaveBeenCalledTimes(5); // every call above reached the real provider.ai.run + }); + + it("routeProviders' direct-address path (dual-review) shares the same circuit breaker as the fallback chain", async () => { + const calls = vi.fn(async () => { + throw new Error("down"); + }); + const cc = { name: "claude-code", ai: { run: calls } }; + const cx = { name: "codex", ai: { run: vi.fn(async () => ({ response: "ok" })) } }; + const route = routeProviders([cc, cx]); + for (let i = 0; i < 3; i += 1) { + await expect(route.run("claude-code", { prompt: "x" })).rejects.toThrow(); + } + expect(calls).toHaveBeenCalledTimes(3); + await expect(route.run("claude-code", { prompt: "x" })).rejects.toThrow(/circuit_open: provider "claude-code"/); + expect(calls).toHaveBeenCalledTimes(3); // unaffected by the circuit-open skip + // The OTHER provider (codex) is completely unaffected — no cross-provider bleed. + await expect(route.run("codex", { prompt: "x" })).resolves.toEqual({ response: "ok" }); + }); + + it("does not affect isAiProviderHealthy / aiConsecutiveFailures — independent whole-chain streak", async () => { + const flaky = { name: "flaky-provider-3", ai: { run: async () => { throw new Error("down"); } } }; + for (let i = 0; i < 3; i += 1) { + await expect(createChainAi([flaky]).run("m", { prompt: "x" })).rejects.toThrow(); + } + // The whole-chain exhaustion streak (a SEPARATE counter) also reaches its own threshold here since every + // call exhausted the (single-provider) chain — a circuit-open throw still counts as a chain exhaustion. + expect(isAiProviderHealthy()).toBe(false); + }); +}); + describe("isAiProviderHealthy (readiness streak, #2497)", () => { const failing = { name: "a", ai: { run: async () => { throw new Error("down"); } } }; const working = { name: "a", ai: { run: async () => ({ response: "ok" }) } }; @@ -217,13 +332,18 @@ describe("isAiProviderHealthy (readiness streak, #2497)", () => { }); it("a success resets the streak back to healthy", async () => { + vi.useFakeTimers(); for (let i = 0; i < 3; i += 1) { await expect(createChainAi([failing]).run("m", { prompt: "x" })).rejects.toThrow(); } expect(isAiProviderHealthy()).toBe(false); + // Provider "a" tripped its own circuit breaker (#2540) after those 3 failures; advance past its cooldown so + // the success below reaches the real (working) provider instead of a fast circuit_open rejection. + await vi.advanceTimersByTimeAsync(60_001); await expect(createChainAi([working]).run("m", { prompt: "x" })).resolves.toEqual({ response: "ok" }); expect(isAiProviderHealthy()).toBe(true); + vi.useRealTimers(); }); it("regression: markAiProviderUnhealthyAtBoot reports unhealthy immediately, before any AI call (#2497 follow-up)", () => { @@ -378,6 +498,24 @@ describe("resolveProviderNames + resolveAiReviewerPlan (#dual-ai-combiner)", () expect(resolveAiReviewerPlan({ AI_PROVIDER: "claude-code,codex,ollama" })?.reviewers).toEqual([{ model: "claude-code" }, { model: "codex" }]); // first two }); + it("resolveAiReviewerPlan: throws when the two dual-review slots resolve to the SAME provider (#2540)", () => { + // "codex,codex" → both dual-review slots are the literal same provider. routeProviders' byName map + // collapses duplicate names to one runtime instance, so this would silently degrade "dual review" into + // "one provider called twice" with no independent second opinion. Fail loud at plan-resolution time + // instead of degrading silently. + expect(() => resolveAiReviewerPlan({ AI_PROVIDER: "codex,codex" })).toThrow(/ai_reviewer_providers_not_distinct/); + expect(() => resolveAiReviewerPlan({ AI_PROVIDER: "codex,codex" })).toThrow(/"codex"/); + }); + + it("resolveAiReviewerPlan: a THIRD-slot duplicate does not throw (only the first two slots are actually used)", () => { + // "codex,ollama,codex" — the first two names (codex, ollama) are distinct, so the plan resolves normally; + // the trailing repeat of codex is never addressed because reviewers are capped at the first two. + expect(resolveAiReviewerPlan({ AI_PROVIDER: "codex,ollama,codex" })).toMatchObject({ + reviewers: [{ model: "codex" }, { model: "ollama" }], + combine: "synthesis", + }); + }); + it("labels explicit provider:model reviewer ids without consulting env defaults", () => { expect(labelSelfHostReviewerModel(" CODEX:gpt-5.5 ", { CODEX_AI_MODEL: "ignored" })).toBe("codex:gpt-5.5"); });