diff --git a/open-sse/services/combo/comboPredicates.ts b/open-sse/services/combo/comboPredicates.ts index f41c31c559..887709522f 100644 --- a/open-sse/services/combo/comboPredicates.ts +++ b/open-sse/services/combo/comboPredicates.ts @@ -6,7 +6,6 @@ * predicates are re-exported from combo.ts for backward compatibility. */ -import { isProviderFailureCode } from "../accountFallback.ts"; import { errorResponse } from "../../utils/error.ts"; import { parseModel } from "../model.ts"; import type { ResolvedComboTarget } from "./types.ts"; @@ -116,13 +115,28 @@ export function shouldSkipForPredictedTtft( ); } +/** + * Whole-provider circuit-breaker failure statuses for the combo path. Kept byte-identical + * to the single-model path's `PROVIDER_BREAKER_FAILURE_STATUSES` (src/sse/handlers/chat.ts:206) + * — the source of truth. 429 is deliberately EXCLUDED: a plain rate-limit must not open the + * whole-provider breaker (it's connection-cooldown / model-lockout scope). Defined locally + * rather than imported to avoid a cross-layer (open-sse → src/sse) import cycle. + */ +const PROVIDER_BREAKER_FAILURE_STATUSES = new Set([408, 500, 502, 503, 504]); + /** * Decide whether a failed combo target should record a whole-provider circuit-breaker * failure (#1731 / #2743 gap-d). This is the consumer side of `skipProviderBreaker`: * * - Stream-readiness failures (pre-flight zombie/ping probes) never count as provider * failures — they are a connection-readiness signal, not an upstream outage. - * - Only provider-level failure codes (408/429/5xx — see `isProviderFailureCode`) count. + * - Only whole-provider failure statuses (408/500/502/503/504) count. A plain rate-limit + * 429 is deliberately EXCLUDED — it belongs to connection cooldown / model lockout scope + * (a genuine quota/token-limit 429 is handled there), NOT the whole-provider breaker. This + * mirrors the single-model path's `PROVIDER_BREAKER_FAILURE_STATUSES` (src/sse/handlers/ + * chat.ts:206) — the source of truth — and the documented RESILIENCE_GUIDE policy. NOTE: + * this intentionally differs from `isProviderFailureCode` (accountFallback.ts), which + * INCLUDES 429 for connection-cooldown purposes and must not be changed here. * - When the next combo target is on the SAME provider, don't trip the provider breaker: * a different model on that provider may still succeed. * - G-02 / #2743: when the fallback result carries `skipProviderBreaker` (an embedded @@ -139,7 +153,7 @@ export function shouldRecordProviderBreakerFailure(args: { }): boolean { return ( !args.isStreamReadinessFailure && - isProviderFailureCode(args.status) && + PROVIDER_BREAKER_FAILURE_STATUSES.has(args.status) && !args.sameProviderNext && !args.skipProviderBreaker ); diff --git a/stryker.conf.json b/stryker.conf.json index 13142050be..14e704a2d3 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -118,6 +118,7 @@ "tests/unit/combo-499-abort.test.ts", "tests/unit/combo-account-allowlist-3266.test.ts", "tests/unit/combo-auto-candidate-expansion.test.ts", + "tests/unit/combo-breaker-429.test.ts", "tests/unit/combo-cache-invalidation.test.ts", "tests/unit/combo-config.test.ts", "tests/unit/combo-context-relay.test.ts", diff --git a/tests/unit/combo-breaker-429.test.ts b/tests/unit/combo-breaker-429.test.ts new file mode 100644 index 0000000000..98fbc966ed --- /dev/null +++ b/tests/unit/combo-breaker-429.test.ts @@ -0,0 +1,69 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +/** + * Combo path must NOT trip the whole-provider circuit breaker on a plain rate-limit 429. + * + * Documented policy (docs/architecture/RESILIENCE_GUIDE.md + CLAUDE.md): only + * 408/500/502/503/504 trip the whole-provider breaker. A plain 429 is connection-cooldown + * / model-lockout scope, never a whole-provider outage. The single-model path already + * excludes 429 via `PROVIDER_BREAKER_FAILURE_STATUSES` (src/sse/handlers/chat.ts:206). This + * asserts the combo predicate `shouldRecordProviderBreakerFailure` is aligned — it must NOT + * gate on `isProviderFailureCode` (accountFallback.ts), which INCLUDES 429 for the separate + * connection-cooldown scope. + */ + +const { shouldRecordProviderBreakerFailure } = + await import("../../open-sse/services/combo/comboPredicates.ts"); + +const OTHER_ARGS = { + isStreamReadinessFailure: false, + sameProviderNext: false, + skipProviderBreaker: false, +} as const; + +test("429 (plain rate limit) does NOT record a whole-provider breaker failure", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ ...OTHER_ARGS, status: 429 }), + false, + "a plain 429 must not open the whole-provider breaker (cooldown/lockout scope)" + ); +}); + +for (const status of [408, 500, 502, 503, 504]) { + test(`whole-provider failure status ${status} DOES record a breaker failure`, () => { + assert.equal( + shouldRecordProviderBreakerFailure({ ...OTHER_ARGS, status }), + true, + `status ${status} must trip the whole-provider breaker` + ); + }); +} + +test("sameProviderNext:true suppresses recording regardless of status", () => { + for (const status of [408, 429, 500, 502, 503, 504]) { + assert.equal( + shouldRecordProviderBreakerFailure({ + ...OTHER_ARGS, + sameProviderNext: true, + status, + }), + false, + `sameProviderNext must suppress recording for status ${status}` + ); + } +}); + +test("skipProviderBreaker:true suppresses recording regardless of status", () => { + for (const status of [408, 429, 500, 502, 503, 504]) { + assert.equal( + shouldRecordProviderBreakerFailure({ + ...OTHER_ARGS, + skipProviderBreaker: true, + status, + }), + false, + `skipProviderBreaker must suppress recording for status ${status}` + ); + } +});