fix(sse): stop combo path tripping whole-provider breaker on plain 429 (#6868)

* fix(sse): stop combo path tripping whole-provider breaker on plain 429

The combo path recorded a whole-provider circuit-breaker failure for a
plain rate-limit 429, opening the breaker after N consecutive 429s and
blocking every account+model on that provider. This contradicts the
single-model path and the documented RESILIENCE_GUIDE policy.

- Single-model path uses PROVIDER_BREAKER_FAILURE_STATUSES =
  Set([408, 500, 502, 503, 504]) (src/sse/handlers/chat.ts:206) — 429 excluded.
- Combo path gated shouldRecordProviderBreakerFailure on isProviderFailureCode
  (accountFallback.ts), whose PROVIDER_FAILURE_ERROR_CODES INCLUDES 429 for
  connection-cooldown scope — so a plain 429 wrongly tripped the whole-provider
  breaker.

Fix scopes tightly: comboPredicates now tests a local
PROVIDER_BREAKER_FAILURE_STATUSES set mirroring the single-model constant
(429 excluded), instead of isProviderFailureCode. The shared
isProviderFailureCode / PROVIDER_FAILURE_ERROR_CODES are deliberately left
untouched — they drive connection-cooldown / model-lockout logic where 429
must still count. A genuine quota/token-limit terminal 429 is handled
elsewhere; only the whole-provider breaker-recording gate changes.

Adds tests/unit/combo-breaker-429.test.ts covering the 429 exclusion,
the 408/5xx inclusion, and the sameProviderNext / skipProviderBreaker
suppression paths.

* test(quality): register combo-breaker-429.test.ts in stryker tap.testFiles

Fast Quality Gates' mutation-coverage drift check flagged this PR's new
covering test for comboPredicates.ts as unregistered.

Co-authored-by: Chirag Singhal <chirag127@users.noreply.github.com>

---------

Co-authored-by: Chirag Singhal <chirag127@users.noreply.github.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com>

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

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

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}`
+    );
+  }
+});