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.
This commit is contained in:
Bob.Hou
2026-08-11 03:34:23 -04:00
committed by GitHub
parent 66845d2e37
commit f10dca4318
5 changed files with 325 additions and 5 deletions

View File

@@ -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.",

View File

@@ -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.
*/

View File

@@ -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,

View File

@@ -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;

View File

@@ -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);
});