From e2d171c63ed6a5d77fd29ed04c428ddf523f64bf Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Sun, 14 Jun 2026 02:07:10 -0300 Subject: [PATCH] test(combo): cover skipProviderBreaker consumer gate (#2743 gap d) (#3832) --- CHANGELOG.md | 1 + file-size-baseline.json | 5 +- open-sse/services/combo.ts | 49 +++- ...kip-provider-breaker-consumer-2743.test.ts | 249 ++++++++++++++++++ 4 files changed, 294 insertions(+), 10 deletions(-) create mode 100644 tests/unit/skip-provider-breaker-consumer-2743.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index b3be4993c0..e844659b74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### 🐛 Fixed - **fix(intelligence): run pricing + models.dev sync from the live startup path** — like the Arena ELO sync (v3.8.24), the external **pricing sync** (`PRICING_SYNC_ENABLED`) and the **models.dev capability sync** (Settings → AI toggle) were only initialized from `server-init.ts`, which the Next standalone runtime never executes — and models.dev had no caller at all. Their toggles were inert in production. Both are now initialized from `instrumentation-node.ts` (self-gated, opt-in preserved, non-blocking, never fatal). (thanks @diegosouzapw) +- **test(combo): cover the `skipProviderBreaker` consumer gate** — the producer was tested but the consumer (whether a failed combo target trips the whole-provider circuit breaker) was not; the breaker decision is now an exported pure predicate (`shouldRecordProviderBreakerFailure`, behaviour-identical) with direct tests asserting a `connection_cooldown` 503 does not trip the breaker while a plain 503 does. Closes another deferred test gap from [#2743](https://github.com/diegosouzapw/OmniRoute/issues/2743). (thanks @diegosouzapw) - **fix(providers): surface the real Devin error + correct the Windsurf auth instructions** — Devin chat returned a generic 502 "Invalid SSE response for non-streaming request" that swallowed the real cause (e.g. "Devin CLI not found"): an error-only SSE chunk (no `choices`) is now propagated with its sanitized message. The Windsurf "Visit windsurf.com/show-auth-token" instruction (the bare URL shows no token without an IDE-supplied `?state=`) now directs users to the `Windsurf: Provide Auth Token` command-palette flow. ([#3324](https://github.com/diegosouzapw/OmniRoute/issues/3324) — thanks @mikmaneggahommie) - **fix(grok-web): clearer 403 message for anti-bot / IP-reputation blocks** — a Grok Web subscription validating from a flagged datacenter/VPS IP got a 403 that read like an invalid cookie, sending users to chase a cookie that was actually fine. A non-auth 403 (Cloudflare challenge / anti-bot body) now returns a message stating the cookie is likely OK and the block is IP-reputation-based — retry from a residential IP or configure a proxy (auth-shaped 403s keep the re-paste guidance). ([#3474](https://github.com/diegosouzapw/OmniRoute/issues/3474) — thanks @friedtofu1608) - **fix(db): make the mass-pending-migrations safety threshold env-overridable** — restoring a backup DB from an older version could trip "Detected N pending migrations … threshold is 50" with no way to override the hardcoded `50`. The threshold is now configurable via `OMNIROUTE_MAX_PENDING_MIGRATIONS` (resolved at startup; `0` disables the check). ([#3416](https://github.com/diegosouzapw/OmniRoute/issues/3416) — thanks @samuraiIT) diff --git a/file-size-baseline.json b/file-size-baseline.json index 156eb1003a..656b41e8c0 100644 --- a/file-size-baseline.json +++ b/file-size-baseline.json @@ -28,7 +28,7 @@ "open-sse/services/batchProcessor.ts": 828, "open-sse/services/browserBackedChat.ts": 850, "open-sse/services/claudeCodeCompatible.ts": 1202, - "open-sse/services/combo.ts": 5131, + "open-sse/services/combo.ts": 5162, "open-sse/services/rateLimitManager.ts": 1017, "open-sse/services/tokenRefresh.ts": 1997, "open-sse/services/usage.ts": 3394, @@ -119,5 +119,6 @@ "_rebaseline_2026_06_13_3758_chat_early_eof": "Re-baseline #3758 (#3817 mergeado): chat.ts 1392→1425 (+33). Crescimento = retry bounded de STREAM_EARLY_EOF no handleSingleModelChat (contador streamEarlyEofRetries + bloco de retry guardado por shouldRetryStreamEarlyEof). Lógica coesa no handler de chat; não-extraível. Reconciliação tardia — o bump foi esquecido no PR do fix (o de antigravity/models foi feito).", "_rebaseline_2026_06_13_3416_migration_threshold": "Re-baseline #3416 (threshold de migrações pendentes via env): migrationRunner.ts 1100→1125 (+25). Crescimento = helper resolveMaxPendingMigrations() que lê OMNIROUTE_MAX_PENDING_MIGRATIONS em call-time (valida finito+>=0, fallback 50) + JSDoc. Lógica coesa de config no runner; não-extraível.", "_rebaseline_2026_06_13_3474_grok_403": "Re-baseline #3474 (mensagem clara no 403 anti-bot do Grok): validation.ts 4302→4348 (+46). Crescimento = helper isGrokAntiBotBlock() + branch 403 de 3 tiers (auth-shaped / anti-bot-IP-reputation / upstream-error). Lógica coesa de classificação no validator; não-extraível.", - "_rebaseline_2026_06_13_3324_windsurf_devin": "Re-baseline #3324 (windsurf auth text + devin error propagation): route.ts 897→903 (+6, texto da instrução windsurf→fluxo command-palette) + sseParser.ts ADICIONADO como frozen 812 (era 746, +66 = helper extractSSEErrorMessage que faz surface do erro real SSE em vez do 502 genérico). 812 fica 12 acima do cap 800 — helper coeso no parser de SSE, congelado com justificativa (precedente providerLimits/useProviderConnections)." + "_rebaseline_2026_06_13_3324_windsurf_devin": "Re-baseline #3324 (windsurf auth text + devin error propagation): route.ts 897→903 (+6, texto da instrução windsurf→fluxo command-palette) + sseParser.ts ADICIONADO como frozen 812 (era 746, +66 = helper extractSSEErrorMessage que faz surface do erro real SSE em vez do 502 genérico). 812 fica 12 acima do cap 800 — helper coeso no parser de SSE, congelado com justificativa (precedente providerLimits/useProviderConnections).", + "_rebaseline_2026_06_13_2743d_skipbreaker": "Re-baseline #2743 gap-d (testar consumer do skipProviderBreaker): combo.ts 5131→5162 (+31). Crescimento = extração do boolean inline da decisão de circuit-breaker para o predicado puro EXPORTADO shouldRecordProviderBreakerFailure() (byte-idêntico) + JSDoc, para torná-lo unit-testável sem o harness completo de combo. Shrink estrutural segue com #3501." } diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 8bf2ac492b..5503e5e7e5 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -188,6 +188,35 @@ export function shouldSkipForPredictedTtft( ); } +/** + * 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. + * - 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 + * service supervisor outage signalled via `X-Omni-Fallback-Hint: connection_cooldown`) + * apply connection cooldown ONLY — never trip the whole-provider breaker. + * + * Pure predicate so the breaker decision is unit-testable without the full combo harness. + */ +export function shouldRecordProviderBreakerFailure(args: { + isStreamReadinessFailure: boolean; + status: number; + sameProviderNext: boolean; + skipProviderBreaker?: boolean; +}): boolean { + return ( + !args.isStreamReadinessFailure && + isProviderFailureCode(args.status) && + !args.sameProviderNext && + !args.skipProviderBreaker + ); +} + function resolveDelayMs(value: unknown, fallback: number): number { const numericValue = Number(value); if (!Number.isFinite(numericValue) || numericValue < 0) return fallback; @@ -1207,9 +1236,7 @@ export function resolveNestedComboTargets( for (const step of runtimeSteps) { if (step.kind === "combo-ref") { - resolved.push( - ...expandRuntimeStep(step, allCombos, new Set(visited), depth, path, maxDepth) - ); + resolved.push(...expandRuntimeStep(step, allCombos, new Set(visited), depth, path, maxDepth)); continue; } resolved.push(step); @@ -4348,10 +4375,12 @@ export async function handleComboChat({ const sameProviderNext = typeof nextTarget?.provider === "string" && nextTarget.provider === provider; if ( - !isStreamReadinessFailure && - isProviderFailureCode(result.status) && - !sameProviderNext && - !fallbackResult.skipProviderBreaker + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure, + status: result.status, + sameProviderNext, + skipProviderBreaker: fallbackResult.skipProviderBreaker, + }) ) { recordProviderFailure(provider, log, target.connectionId, profile); } @@ -4608,7 +4637,11 @@ async function handleRoundRobinCombo({ ? resolveResilienceSettings(settings) : resolveResilienceSettings(null); - const orderedTargets = resolveComboTargets(combo, allCombos, clampComboDepth(config.maxComboDepth)); + const orderedTargets = resolveComboTargets( + combo, + allCombos, + clampComboDepth(config.maxComboDepth) + ); const tagFilteredTargets = await applyRequestTagRouting(orderedTargets, body, log); const evalRankedTargets = orderTargetsByEvalScores(tagFilteredTargets, config.evalRouting, log); const filteredTargets = filterTargetsByRequestCompatibility( diff --git a/tests/unit/skip-provider-breaker-consumer-2743.test.ts b/tests/unit/skip-provider-breaker-consumer-2743.test.ts new file mode 100644 index 0000000000..b5a405f457 --- /dev/null +++ b/tests/unit/skip-provider-breaker-consumer-2743.test.ts @@ -0,0 +1,249 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +/** + * #2743 — gap d (deferred test debt): CONSUMER-side coverage for `skipProviderBreaker`. + * + * The PRODUCER is already covered (tests/unit/account-fallback-service.test.ts, the + * "G-02" block): `checkFallbackError` returns `skipProviderBreaker: true` for a 503 + + * `X-Omni-Fallback-Hint: connection_cooldown` (an embedded-service supervisor outage, + * NOT an upstream AI-provider failure). + * + * What was UNVERIFIED is the CONSUMER gate in `open-sse/services/combo.ts` (~line 4350): + * the decision that, when `skipProviderBreaker` is set, the whole-provider circuit + * breaker must NOT be tripped (`recordProviderFailure` is skipped) — it is a + * connection-level cooldown, not a provider-level failure. The producer test's final + * "breaker stays CLOSED" assertion is trivially true there because nothing ever calls + * `recordProviderFailure`, so it does not prove the consumer gate works. + * + * This test drives the consumer end of the flow: + * 1. the pure decision predicate (`shouldRecordProviderBreakerFailure`), and + * 2. its real effect — wiring the producer result THROUGH the predicate into + * `recordProviderFailure` against a real circuit breaker, asserting the breaker + * stays CLOSED for the skip-hint path and OPENS for the negative-control path. + */ + +const combo = await import("../../open-sse/services/combo.ts"); +const accountFallback = await import("../../open-sse/services/accountFallback.ts"); +const circuitBreaker = await import("../../src/shared/utils/circuitBreaker.ts"); + +const { shouldRecordProviderBreakerFailure } = combo; +const { + checkFallbackError, + recordProviderFailure, + isProviderInCooldown, + getProviderBreakerState, + clearProviderFailure, +} = accountFallback; +const { resetAllCircuitBreakers } = circuitBreaker; + +// Profile override that makes the provider breaker open on the FIRST recorded +// failure, so the negative control is deterministic regardless of env-tuned +// default thresholds (apikey defaults to 12). +const OPEN_ON_FIRST = { failureThreshold: 1, resetTimeoutMs: 60_000 } as const; + +test.beforeEach(() => { + resetAllCircuitBreakers(); +}); + +test.after(() => { + resetAllCircuitBreakers(); +}); + +// ─── 1. Pure decision predicate ────────────────────────────────────────────── +// Mirrors the four-way gate in combo.ts: only a provider-level failure code, on a +// target NOT followed by a same-provider target, that is NOT a stream-readiness +// failure and NOT flagged skipProviderBreaker, records a provider-breaker failure. + +test("predicate: plain 503 (no skip hint) DOES record a provider-breaker failure", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 503, + sameProviderNext: false, + skipProviderBreaker: false, + }), + true + ); +}); + +test("predicate: skipProviderBreaker:true suppresses the provider-breaker failure", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 503, + sameProviderNext: false, + skipProviderBreaker: true, + }), + false + ); +}); + +test("predicate: skipProviderBreaker undefined behaves like false (records)", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 503, + sameProviderNext: false, + // skipProviderBreaker omitted on purpose + }), + true + ); +}); + +test("predicate: stream-readiness failure never records (even on a 5xx)", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: true, + status: 503, + sameProviderNext: false, + skipProviderBreaker: false, + }), + false + ); +}); + +test("predicate: same-provider-next suppresses (a different model may still succeed)", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 503, + sameProviderNext: true, + skipProviderBreaker: false, + }), + false + ); +}); + +test("predicate: a non-provider-failure code (e.g. 404) never records", () => { + assert.equal( + shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 404, + sameProviderNext: false, + skipProviderBreaker: false, + }), + false + ); +}); + +// ─── 2. End-to-end consumer effect (producer → predicate → breaker) ─────────── +// Replays the exact combo.ts consumer wiring against a REAL circuit breaker. + +/** + * Mirror of the combo.ts consumer site: feed a real `checkFallbackError` result + * through the extracted predicate and into `recordProviderFailure` exactly as the + * combo router does. Returns whether the breaker failure was recorded. + */ +function runConsumerGate(opts: { + provider: string; + status: number; + errorText: string; + headers: Headers | Record | null; + sameProviderNext?: boolean; + isStreamReadinessFailure?: boolean; +}): boolean { + const fallbackResult = checkFallbackError( + opts.status, + opts.errorText, + 0, + null, + opts.provider, + opts.headers ?? null + ); + const shouldRecord = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: opts.isStreamReadinessFailure ?? false, + status: opts.status, + sameProviderNext: opts.sameProviderNext ?? false, + skipProviderBreaker: fallbackResult.skipProviderBreaker, + }); + if (shouldRecord) { + // Force the breaker to open on the first failure so the assertion is deterministic. + recordProviderFailure(opts.provider, undefined, null, OPEN_ON_FIRST); + } + return shouldRecord; +} + +test("consumer (positive skip): 503 + connection_cooldown hint → breaker stays CLOSED", () => { + const provider = "test-9router-skip-2743"; + // Sanity: clean start. + assert.equal(isProviderInCooldown(provider), false); + + const recorded = runConsumerGate({ + provider, + status: 503, + errorText: "9router is not running (state: stopped)", + headers: new Headers({ "X-Omni-Fallback-Hint": "connection_cooldown" }), + }); + + // The producer set skipProviderBreaker:true, so the consumer must NOT record a failure. + assert.equal(recorded, false, "recordProviderFailure must be skipped for the cooldown hint"); + assert.equal( + isProviderInCooldown(provider), + false, + "provider circuit breaker must remain CLOSED for a supervisor cooldown signal" + ); + const state = getProviderBreakerState(provider); + assert.equal(state?.failureCount ?? 0, 0, "breaker failure count must not be incremented"); +}); + +test("consumer (positive skip): five consecutive cooldown-hint 503s keep the breaker CLOSED", () => { + const provider = "test-9router-skip-loop-2743"; + for (let i = 0; i < 5; i++) { + const recorded = runConsumerGate({ + provider, + status: 503, + errorText: "9router is not running (state: stopped)", + headers: new Headers({ "X-Omni-Fallback-Hint": "connection_cooldown" }), + }); + assert.equal(recorded, false, `call ${i + 1} must skip recordProviderFailure`); + } + assert.equal( + isProviderInCooldown(provider), + false, + "repeated supervisor cooldown signals must never trip the whole-provider breaker" + ); +}); + +test("consumer (negative control): plain 503 WITHOUT the hint trips the breaker", () => { + const provider = "test-openai-noskip-2743"; + assert.equal(isProviderInCooldown(provider), false); + + const recorded = runConsumerGate({ + provider, + status: 503, + errorText: "service unavailable", + headers: null, + }); + + // No skip flag → the consumer records the failure → breaker opens (threshold = 1). + assert.equal(recorded, true, "a real upstream 503 must record a provider-breaker failure"); + assert.equal( + isProviderInCooldown(provider), + true, + "provider circuit breaker must OPEN for a real upstream outage" + ); + const state = getProviderBreakerState(provider); + assert.ok((state?.failureCount ?? 0) >= 1, "breaker failure count must be incremented"); + + clearProviderFailure(provider); +}); + +test("consumer (negative control): same-provider-next still suppresses recording", () => { + // A real upstream 503 (no skip hint) but the next combo target is the same provider: + // the gate must NOT trip the breaker so a different model can still be tried. + const provider = "test-openai-sameprovider-2743"; + const recorded = runConsumerGate({ + provider, + status: 503, + errorText: "service unavailable", + headers: null, + sameProviderNext: true, + }); + assert.equal(recorded, false, "same-provider-next must suppress the provider-breaker failure"); + assert.equal( + isProviderInCooldown(provider), + false, + "breaker must stay CLOSED for same-provider-next" + ); +});