From f10dca43188792e48575b86c3e78aae714861a0e Mon Sep 17 00:00:00 2001 From: "Bob.Hou" Date: Tue, 11 Aug 2026 03:34:23 -0400 Subject: [PATCH] fix(combo): recover provider circuit breaker from HALF_OPEN on success (#9207) The combo success path called recordProviderSuccess (cooldown-only) without notifying the circuit breaker. When a provider breaker entered HALF_OPEN after repeated failures, successful probe requests never transitioned it back to CLOSED -- the breaker stayed stuck indefinitely. Production evidence: agy breaker HALF_OPEN with 699 requests at 98% success rate, never recovering. Root cause: combo.ts calls recordProviderSuccess from providerCooldownTracker.ts (resets cooldown failureCount only) but never calls breaker._onSuccess(). The failure path in accountFallback.ts calls breaker._onFailure(), creating an asymmetry. Fix: add recordProviderSuccess to accountFallback.ts as the symmetric counterpart of recordProviderFailure. Uses getProviderBreaker (not configureProviderBreaker) to avoid overwriting the breaker's resetTimeout with default profile values. Calls breaker._onSuccess() for all non-OPEN states (CLOSED/DEGRADED/HALF_OPEN), matching execute()'s behavior. --- config/quality/file-size-baseline.json | 1 + open-sse/services/accountFallback.ts | 42 +++ open-sse/services/combo.ts | 7 +- open-sse/services/providerCooldownTracker.ts | 5 + ...provider-breaker-halfopen-recovery.test.ts | 275 ++++++++++++++++++ 5 files changed, 325 insertions(+), 5 deletions(-) create mode 100644 tests/unit/provider-breaker-halfopen-recovery.test.ts diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index fccf4883be..42443bb7a0 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -1,4 +1,5 @@ { + "_rebaseline_2026_08_09_9207_breaker_halfopen_recovery": "PR #9207 own growth during the 2026-08-09 rebase: open-sse/services/accountFallback.ts 1978->2020 (+42 = recordProviderSuccess now also transitions the provider circuit breaker from HALF_OPEN to CLOSED when a request succeeds, so the breaker is not stuck half-open after repeated failures; the transition and its reset wiring grow the existing provider-success path, not extractable). Covered by tests/unit/provider-breaker-halfopen-recovery.test.ts.", "_rebaseline_2026_08_09_9296_adobe_media_capabilities": "PR #9296 (artickc, fix/adobe-firefly-model-capabilities) own growth: src/app/api/v1/models/catalog.ts 1590->1597 (+7). The image and video catalog serializers now expose the already-normalized Adobe Firefly discovery capability data (media_capabilities, plus the existing video modality/size fields) at their only response-emission chokepoints. The discovery parser and capability normalization remain in open-sse/services/adobeFireflyModels.ts; extracting these seven serialization fields would obscure the catalog contract. Covered by tests/unit/adobe-firefly.test.ts and tests/unit/image-upscale.test.ts.", "_rebaseline_2026_08_08_v3850_base_drift_batch_9757": "Base drift on release/v3.8.50, not own growth: the 08-06..08-08 merge batches grew 12 already-frozen (or newly-landed) files without carrying their rebaselines \u2014 the dedicated rebaseline PR #9616 was closed as 'superseded' but its file-size entries never actually reached the base, and later merges (#8894 combos page, #9539 EditConnectionModal, #8895 models route, #9294/#9293 catalog, #9541 db/core, #8970 tokenHealthCheck, #8925 mcp schemas+server, #8890 accountFallback, #9467 chat.ts, #8931 openai-to-kiro, ProxyRegistryManager) kept growing them. All 12 values re-measured on THIS branch's tree (= pure tip + this PR's 1-line chat.ts fix, which adds zero lines). This PR's own source changes (chat.ts identifier restore, stream.ts format carve-out) do not grow any frozen file past these values.", "_rebaseline_2026_08_08_migration_135_collision": "fix(db): resolve migration version 135 numbering collision \u2014 #9449's 135_connection_runtime_state.sql and #8908's 135_migrate_model_capability_max_token.sql both claimed version 135 (#9449 branched before #8908 merged and never got renumbered before landing on release/v3.8.50), which threw 'Migration version collision detected' the moment ANY code touched the database \u2014 a fresh install/deploy from this tip cannot even boot. Renumbered the later-landing file to 140 (next free slot) and added the matching isSchemaAlreadyApplied('140') retroactive guard, matching the established pattern already used for the prior 135/136 -> 137/138 renumber in the same file. Own growth: src/lib/db/migrationRunner.ts 1084->1094 (+10, the new case block) \u2014 irreducible, matches the existing per-case guard pattern exactly. Covered by tests/unit/migration-135-numbering-collision.test.ts (2/2), confirmed failing (reproducing the exact live crash) against the pre-fix colliding filenames, passing after.", diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index 06e1e003d5..42eb401159 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -31,6 +31,7 @@ import { looksLikeQuotaExhausted, type FailureKind, } from "../../src/shared/utils/classify429"; +import { recordProviderSuccess as resetCooldownFailureCount } from "./providerCooldownTracker.ts"; import { resolveProviderId } from "../../src/shared/constants/providers"; import { resolveUseUpstream429BreakerHints } from "../../src/shared/utils/providerHints"; import { getCodexModelScope } from "../config/codexQuotaScopes.ts"; @@ -1002,6 +1003,47 @@ export function recordProviderFailure( } } +/** + * Record a successful request for a provider. + * Symmetric counterpart of recordProviderFailure: + * - Resets cooldown failureCount (exponential backoff) for all non-OPEN states. + * - HALF_OPEN -> CLOSED (probe success), CLOSED/DEGRADED -> decay failureCount. + * + * When the breaker is OPEN (provider is failing), this is a no-op -- the + * cooldown stays intact and the breaker keeps its cooldown period. + * + * Matches execute()'s behavior: _onSuccess() is called for all non-OPEN states. + */ +export function recordProviderSuccess( + provider: string | null | undefined, + connectionId?: string | null +): void { + if (!provider || provider === "unknown") return; + + const breaker = getProviderBreaker(provider); + if (!breaker) return; + const breakerState = breaker.getStatus().state; + + // When breaker is OPEN, the provider is failing -- do not reset cooldown + // even if one request slipped through (dispatched before the open). + // The cooldown resets when the breaker reaches HALF_OPEN and the probe + // succeeds below. + if (breakerState === "OPEN") return; + + // Reset cooldown failureCount (exponential backoff) -- symmetric with + // recordProviderCooldown which increments it on each failure. + resetCooldownFailureCount(provider, connectionId ?? undefined); + + // Clear failure-dedup window so the next genuine failure is not suppressed. + if (connectionId) { + lastConnectionFailure.delete(`${provider}:${connectionId}`); + } + + // Transition breaker on success, matching execute()'s behavior: + // HALF_OPEN -> CLOSED (probe success), CLOSED/DEGRADED -> decay failureCount. + breaker._onSuccess(); +} + /** * Reset the shared provider breaker. */ diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 7b73108df5..618be5d42a 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -19,6 +19,7 @@ import { MODEL_ACCESS_DENIED_PATTERNS, recordModelLockoutFailure, recordProviderFailure, + recordProviderSuccess, selectLockoutCooldownMs, } from "./accountFallback.ts"; import { @@ -89,11 +90,7 @@ import { } from "./combo/promptCacheAffinity.ts"; import type { CompressionMode } from "./compression/types.ts"; import { getCachedProviderConnections } from "../../src/lib/db/readCache"; -import { - isProviderInCooldown, - recordProviderCooldown, - recordProviderSuccess, -} from "./providerCooldownTracker.ts"; +import { isProviderInCooldown, recordProviderCooldown } from "./providerCooldownTracker.ts"; import { resolveResilienceSettings, type ResilienceSettings, diff --git a/open-sse/services/providerCooldownTracker.ts b/open-sse/services/providerCooldownTracker.ts index f5a14d2006..13fbd17af5 100644 --- a/open-sse/services/providerCooldownTracker.ts +++ b/open-sse/services/providerCooldownTracker.ts @@ -171,6 +171,11 @@ export function getRemainingCooldownMs( /** * Record a successful request for a provider/connection. * Resets the failure count (but keeps the entry for reference). + * + * @deprecated Use accountFallback.recordProviderSuccess instead -- it also + * transitions the circuit breaker from HALF_OPEN to CLOSED. This function + * only resets the cooldown failureCount without touching the breaker, which + * leaves the breaker stuck in HALF_OPEN after repeated failures. */ export function recordProviderSuccess(provider: string, connectionId: string | undefined): void { if (!provider || provider === "unknown") return; diff --git a/tests/unit/provider-breaker-halfopen-recovery.test.ts b/tests/unit/provider-breaker-halfopen-recovery.test.ts new file mode 100644 index 0000000000..b3b50aea3e --- /dev/null +++ b/tests/unit/provider-breaker-halfopen-recovery.test.ts @@ -0,0 +1,275 @@ +/** + * Tests for circuit breaker HALF_OPEN -> CLOSED recovery via recordProviderSuccess. + * + * Production scenario: provider breaker stuck in HALF_OPEN with 699 requests + * at 98% success rate, never recovering. Root cause: combo.ts success path + * calls recordProviderSuccess (cooldown-only) but never calls breaker._onSuccess(). + * + * This test verifies that recordProviderSuccess in accountFallback.ts + * transitions the breaker from HALF_OPEN to CLOSED and resets failure count. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + getCircuitBreaker, + resetAllCircuitBreakers, +} from "../../src/shared/utils/circuitBreaker.ts"; +import { + recordProviderFailure, + recordProviderSuccess, +} from "../../open-sse/services/accountFallback.ts"; +import { + recordProviderCooldown, + isProviderInCooldown, + clearCooldownState, +} from "../../open-sse/services/providerCooldownTracker.ts"; + +const uniqueProvider = (suffix: string) => + `halfopen-test-${suffix}-${Date.now()}-${Math.floor(Math.random() * 1e6)}`; + +test("recordProviderSuccess transitions breaker from HALF_OPEN to CLOSED and resets failureCount", async () => { + const provider = uniqueProvider("recovery"); + + // Step 1: Open the breaker (failureThreshold: 1 -> one failure opens it) + recordProviderFailure(provider, undefined, undefined, { + failureThreshold: 1, + resetTimeoutMs: 150, + }); + + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "OPEN", "breaker should be OPEN after failure"); + assert.equal(breaker.failureCount, 1, "failureCount should be 1"); + + // Step 2: Wait past resetTimeout so _refreshOpenState transitions to HALF_OPEN + await new Promise((r) => setTimeout(r, 200)); + breaker.canExecute(); // triggers _refreshOpenState -> HALF_OPEN + assert.equal(breaker.state, "HALF_OPEN", "breaker should be HALF_OPEN after resetTimeout"); + + // Step 3: recordProviderSuccess should close the breaker + recordProviderSuccess(provider, undefined); + + assert.equal(breaker.state, "CLOSED", "breaker should be CLOSED after success"); + assert.equal(breaker.failureCount, 0, "failureCount should be reset to 0"); +}); + +test("recordProviderSuccess does not prematurely close an OPEN breaker before resetTimeout", () => { + const provider = uniqueProvider("no-premature-close"); + + // Open the breaker + recordProviderFailure(provider, undefined, undefined, { + failureThreshold: 1, + resetTimeoutMs: 60_000, // long timeout -- won't elapse during test + }); + + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "OPEN"); + + // Call success while still in OPEN (timeout hasn't elapsed) + recordProviderSuccess(provider, undefined); + + // Breaker should stay OPEN -- we don't want to bypass the cooldown + assert.equal(breaker.state, "OPEN", "breaker must stay OPEN when resetTimeout has not elapsed"); + assert.equal(breaker.failureCount, 1, "failureCount must not change while OPEN"); +}); + +test("recordProviderSuccess does not reset cooldown when breaker is OPEN", () => { + const provider = uniqueProvider("open-cooldown-guard"); + const connectionId = "conn-open"; + + // Build up cooldown first + for (let i = 0; i < 3; i++) { + recordProviderCooldown(provider, connectionId); + } + assert.equal(isProviderInCooldown(provider, connectionId), true); + + // Open the breaker + recordProviderFailure(provider, undefined, connectionId, { + failureThreshold: 1, + resetTimeoutMs: 60_000, + }); + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "OPEN"); + + // Success while OPEN should NOT reset cooldown (early-return guard) + recordProviderSuccess(provider, connectionId); + + assert.equal(breaker.state, "OPEN", "breaker stays OPEN"); + assert.equal( + isProviderInCooldown(provider, connectionId), + true, + "cooldown must NOT be cleared while breaker is OPEN" + ); +}); + +test("recordProviderSuccess on CLOSED breaker decays failureCount (gradual recovery)", () => { + const provider = uniqueProvider("closed-gradual"); + + // Get a breaker in CLOSED state (no failures) + const breaker = getCircuitBreaker(provider, { + failureThreshold: 3, + resetTimeoutMs: 30_000, + }); + assert.equal(breaker.state, "CLOSED"); + + // Add a failure via public API (but not enough to open: threshold=3) + recordProviderFailure(provider, undefined, undefined, { + failureThreshold: 3, + resetTimeoutMs: 30_000, + }); + assert.equal(breaker.failureCount, 1); + assert.equal(breaker.state, "CLOSED"); // still closed (threshold=3) + + // recordProviderSuccess on CLOSED decays failureCount by 1 (matching execute()) + recordProviderSuccess(provider, undefined); + + assert.equal(breaker.failureCount, 0, "failureCount decremented by 1 (gradual recovery)"); + assert.equal(breaker.state, "CLOSED", "should stay CLOSED"); +}); + +test("recordProviderSuccess with null/undefined provider is a safe no-op", () => { + // Should not throw + recordProviderSuccess(null, undefined); + recordProviderSuccess(undefined, undefined); + recordProviderSuccess("", undefined); +}); + +test("recordProviderSuccess is idempotent: multiple calls on HALF_OPEN are safe", async () => { + const provider = uniqueProvider("idempotent"); + + // Open the breaker + recordProviderFailure(provider, undefined, undefined, { + failureThreshold: 1, + resetTimeoutMs: 100, + }); + + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "OPEN"); + + // Wait for HALF_OPEN + await new Promise((r) => setTimeout(r, 150)); + breaker.canExecute(); + assert.equal(breaker.state, "HALF_OPEN"); + + // First success closes the breaker + recordProviderSuccess(provider, undefined); + assert.equal(breaker.state, "CLOSED"); + assert.equal(breaker.failureCount, 0); + + // Second success is a no-op (state is CLOSED, not HALF_OPEN) + recordProviderSuccess(provider, undefined); + assert.equal(breaker.state, "CLOSED"); + assert.equal(breaker.failureCount, 0); +}); + +test("full lifecycle: CLOSED -> OPEN -> HALF_OPEN -> CLOSED via success", async () => { + const provider = uniqueProvider("lifecycle"); + + // Start CLOSED + const breaker = getCircuitBreaker(provider, { + failureThreshold: 1, + resetTimeoutMs: 100, + }); + assert.equal(breaker.state, "CLOSED"); + + // Failure opens the breaker + recordProviderFailure(provider, undefined, undefined, { + failureThreshold: 1, + resetTimeoutMs: 100, + }); + assert.equal(breaker.state, "OPEN"); + assert.equal(breaker.failureCount, 1); + + // Still OPEN before timeout + recordProviderSuccess(provider, undefined); + assert.equal(breaker.state, "OPEN", "must not close before resetTimeout"); + + // Wait for HALF_OPEN transition + await new Promise((r) => setTimeout(r, 150)); + breaker.canExecute(); + assert.equal(breaker.state, "HALF_OPEN"); + + // Success recovers to CLOSED + recordProviderSuccess(provider, undefined); + assert.equal(breaker.state, "CLOSED"); + assert.equal(breaker.failureCount, 0); + + // Subsequent failure re-opens (proves breaker is functional after recovery) + recordProviderFailure(provider, undefined, undefined, { + failureThreshold: 1, + resetTimeoutMs: 100, + }); + assert.equal(breaker.state, "OPEN"); + assert.equal(breaker.failureCount, 1); +}); + +test("recordProviderSuccess resets cooldown failureCount (exponential backoff)", () => { + const provider = uniqueProvider("cooldown-reset"); + const connectionId = "conn-1"; + + // Build up 4 failures -> cooldown with exponential backoff + for (let i = 0; i < 4; i++) { + recordProviderCooldown(provider, connectionId); + } + + // Cooldown should be active (failureCount > 0) + assert.equal( + isProviderInCooldown(provider, connectionId), + true, + "provider should be in cooldown after 4 failures" + ); + + // Success should reset the cooldown + recordProviderSuccess(provider, connectionId); + + assert.equal( + isProviderInCooldown(provider, connectionId), + false, + "cooldown should be cleared after success (failureCount reset to 0)" + ); + + // Breaker should remain CLOSED (cooldown test does not open the breaker) + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "CLOSED", "breaker stays CLOSED (cooldown-only scenario)"); +}); + +test("recordProviderSuccess with connectionId transitions breaker and resets cooldown", async () => { + const provider = uniqueProvider("connid-breaker"); + const connectionId = "conn-abc"; + + // Open the breaker + recordProviderFailure(provider, undefined, connectionId, { + failureThreshold: 1, + resetTimeoutMs: 100, + }); + + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "OPEN"); + + // Build up cooldown with connectionId + for (let i = 0; i < 3; i++) { + recordProviderCooldown(provider, connectionId); + } + assert.equal(isProviderInCooldown(provider, connectionId), true); + + // Wait for HALF_OPEN + await new Promise((r) => setTimeout(r, 150)); + breaker.canExecute(); + assert.equal(breaker.state, "HALF_OPEN"); + + // Success with connectionId should close breaker AND reset cooldown + recordProviderSuccess(provider, connectionId); + + assert.equal(breaker.state, "CLOSED", "breaker CLOSED after success with connectionId"); + assert.equal(breaker.failureCount, 0, "failureCount reset"); + assert.equal(isProviderInCooldown(provider, connectionId), false, "cooldown cleared"); +}); + +test("resetAllCircuitBreakers cleans up test state", () => { + // Verify cleanup works (prevents state leaking between tests) + resetAllCircuitBreakers(); + clearCooldownState(); + const provider = uniqueProvider("cleanup"); + const breaker = getCircuitBreaker(provider); + assert.equal(breaker.state, "CLOSED"); + assert.equal(breaker.failureCount, 0); +});