From c5c27b813a37887540c4c94e1518ff09ea6b9b3b Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Fri, 24 Jul 2026 09:57:01 -0300 Subject: [PATCH] =?UTF-8?q?fix(sse):=20cap=20exact=20cooldowns=20only=20wh?= =?UTF-8?q?en=20synthetic=20=E2=80=94=20verified=20upstream=20resets=20pas?= =?UTF-8?q?s=20uncapped=20(#8393)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract vs cap: #6863 requires a model lockout to honor a VERIFIED upstream quota reset exactly (e.g. Antigravity "Resets in 92h27m28s", shipped in v3.8.47). #7940 requires SYNTHETIC exact-cooldown estimates (the quota_exhausted until-midnight heuristic) to respect the operator's maxCooldownMs so they cannot balloon unbounded. Both are legitimate, non-conflicting contracts — they apply to different kinds of values. Root cause: #7980 (fixing #7940) changed recordModelLockoutFailure() in open-sse/services/accountFallback.ts to unconditionally clamp every exactCooldownMs against maxCooldownMs, with no way to distinguish a verified upstream reset from a synthetic estimate. A real ~92h reset got clamped to the operator's ~30min cap, and the router went on hammering 429 against quota that was known not to recover for days — regressing #6863's contract by omission, not by new policy (the "honor it exactly" docstrings on selectLockoutCooldownMs() and its call sites were left untouched and now describe dead code). Fix: add an opt-in `exactCooldownVerified` flag to recordModelLockoutFailure()'s options. When true, exactCooldownMs bypasses the maxCooldownMs clamp entirely; when false/omitted (the default), behavior is byte-identical to before this change. Set the flag only at the 4 call sites that already carry upstream provenance for the value they pass — usedUpstreamRetryHint / quotaResetHintMs from checkFallbackError(): - open-sse/services/combo.ts (2 sites): exactCooldownVerified mirrors lockoutHintMs > 0, which is only ever nonzero when it traces back to a genuine upstream signal. - src/sse/services/auth.ts (2 sites): exactCooldownVerified mirrors the same usedUpstreamRetryHint / quotaResetHintMs check already used to derive exactCooldownMs at each site. The quota_exhausted → until-midnight synthetic default and plain exponential backoff are untouched and stay capped, per #7940. The two other recordModelLockoutFailure call sites (combo.ts quality failure, auth.ts local-404/grok-web-403) never carry a verified hint and were left unmodified. Validation (TDD): tests/unit/combo-lockout-quota-reset-6863.test.ts red→green with its assertions unchanged (was clamping ~332,848,000ms to ~1,799,995ms; now honors the parsed reset). Added a boundary pair to tests/unit/model-lockout-exact-cooldown-cap.test.ts proving the same magnitude resolves differently by provenance: synthetic stays capped, verified passes through whole. Full existing suite in that file plus combo-model-lockout-honors-reset-1308.test.ts stay green unmodified. Swept 45 lockout/cooldown-adjacent test files (502/505 passing); the 3 failures reproduce byte-identical on a pristine origin/release/v3.8.49 checkout (PROVIDER_BREAKER_FAILURE_STATUSES ReferenceError in untouched chat.ts, and a documented timing-sensitive serial test) — confirmed pre-existing, out of this fix's scope. npm run typecheck:core and npm run lint are clean. Refs #6863 Refs #7940 Refs #7980 --- open-sse/services/accountFallback.ts | 27 +++++++-- open-sse/services/combo.ts | 14 ++++- src/sse/services/auth.ts | 11 ++++ .../model-lockout-exact-cooldown-cap.test.ts | 56 +++++++++++++++++++ 4 files changed, 103 insertions(+), 5 deletions(-) diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index 8f0ca7d8f3..28f871719f 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -587,7 +587,22 @@ export function recordModelLockoutFailure( status: number, fallbackCooldownMs: number, profile: ProviderProfile | null = null, - options: { exactCooldownMs?: number | null; maxCooldownMs?: number } = {} + options: { + exactCooldownMs?: number | null; + maxCooldownMs?: number; + /** + * #6863 vs #7940: set true only when `exactCooldownMs` was parsed/verified from + * an actual upstream signal (Retry-After header, X-RateLimit-Reset, or a reset + * parsed from the error body — i.e. `usedUpstreamRetryHint`/`quotaResetHintMs` + * from `checkFallbackError`). A verified reset is honored exactly, even past + * `maxCooldownMs` — a real "Resets in 92h" must not be clamped down to minutes, + * or the router hammers 429 against quota that is known not to come back. + * Leave false/omitted for SYNTHETIC estimates (e.g. the quota_exhausted + * until-midnight default below, or plain exponential backoff) — those stay + * capped, per #7940. + */ + exactCooldownVerified?: boolean; + } = {} ) { ensureCleanupTimer(); const key = getModelLockKey(provider, connectionId, model, reason, status); @@ -610,15 +625,19 @@ export function recordModelLockoutFailure( const failureCount = withinWindow ? previous.failureCount + 1 : 1; const baseCooldownMs = getModelLockBaseCooldown(status, fallbackCooldownMs, profile); - // Cap both exponential backoff and exact cooldowns (e.g. daily-quota - // until-midnight) against maxCooldownMs so user-configured caps are honored. + // Cap exponential backoff and SYNTHETIC exact cooldowns (e.g. the daily-quota + // until-midnight heuristic below) against maxCooldownMs so user-configured caps + // are honored (#7940). A caller-VERIFIED exact cooldown (#6863 — parsed from an + // actual upstream Retry-After/reset signal, see `exactCooldownVerified` above) + // bypasses the cap instead of being clamped to a window the upstream already + // told us is wrong. const maxCooldownMs = typeof options.maxCooldownMs === "number" && options.maxCooldownMs > 0 ? options.maxCooldownMs : null; const cooldownMs = typeof options.exactCooldownMs === "number" && options.exactCooldownMs > 0 - ? maxCooldownMs !== null + ? maxCooldownMs !== null && !options.exactCooldownVerified ? Math.min(options.exactCooldownMs, maxCooldownMs) : options.exactCooldownMs : Math.min( diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 34a6fb4210..935bbcfd81 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -2295,6 +2295,12 @@ export async function handleComboChat({ fallbackResult.usedUpstreamRetryHint === true ? cooldownMs : (fallbackResult.quotaResetHintMs ?? 0); + // #6863 vs #7940: lockoutHintMs is only ever nonzero when it traces back to + // a genuine upstream signal (usedUpstreamRetryHint or a parsed quotaResetHintMs) + // — never a synthetic estimate. Tell recordModelLockoutFailure to honor it + // exactly instead of clamping it to maxCooldownMs (#7940's cap still applies + // to the exponential-backoff / synthetic-default paths). + const lockoutHintVerified = lockoutHintMs > 0; const selectedConnectionId = result.headers?.get("X-OmniRoute-Selected-Connection-Id") || result.headers?.get("x-omniroute-selected-connection-id") || @@ -2441,9 +2447,12 @@ export async function handleComboChat({ profile, { // #1308/#6863: honor a long upstream reset (e.g. "Resets in 160h") over - // the short base cooldown / exponential backoff when present. + // the short base cooldown / exponential backoff when present. #7940's + // maxCooldownMs cap only applies to synthetic values — a verified + // upstream reset (lockoutHintVerified) bypasses it. exactCooldownMs: selectLockoutCooldownMs(lockoutHintMs, mlSettings), maxCooldownMs: mlSettings.maxCooldownMs, + exactCooldownVerified: lockoutHintVerified, } ); lockoutRecorded = true; @@ -2493,8 +2502,11 @@ export async function handleComboChat({ profile, { // #1308/#6863: honor a long upstream reset over base/exponential cooldown. + // #7940's maxCooldownMs cap only applies to synthetic values — a verified + // upstream reset (lockoutHintVerified) bypasses it. exactCooldownMs: selectLockoutCooldownMs(lockoutHintMs, mlSettings), maxCooldownMs: mlSettings.maxCooldownMs, + exactCooldownVerified: lockoutHintVerified, } ); } diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index 0d3906acc8..3eaeb7a737 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -2064,6 +2064,13 @@ export async function markAccountUnavailable( ? fallbackResult.cooldownMs : (fallbackResult.quotaResetHintMs ?? null), maxCooldownMs: mlSettings.maxCooldownMs, + // #6863 vs #7940: exactCooldownMs above is only ever set from a genuine + // upstream signal (Retry-After/reset header or a parsed quotaResetHintMs) — + // never a synthetic estimate — so it must bypass maxCooldownMs instead of + // being clamped down to a window the upstream already told us is wrong. + exactCooldownVerified: + fallbackResult.usedUpstreamRetryHint === true || + typeof fallbackResult.quotaResetHintMs === "number", } ); // Update last error for observability (without changing terminal status) @@ -2133,6 +2140,10 @@ export async function markAccountUnavailable( exactCooldownMs: fallbackResult.usedUpstreamRetryHint === true ? fallbackResult.cooldownMs : null, maxCooldownMs: mlSettings.maxCooldownMs, + // #6863 vs #7940: only a genuine upstream retry hint bypasses maxCooldownMs; + // absent a hint, exactCooldownMs above is null and this falls through to the + // (still-capped) exponential-backoff branch. + exactCooldownVerified: fallbackResult.usedUpstreamRetryHint === true, } ); updateProviderConnection(connectionId, { diff --git a/tests/unit/model-lockout-exact-cooldown-cap.test.ts b/tests/unit/model-lockout-exact-cooldown-cap.test.ts index bd167afa5c..ec2dbbf69d 100644 --- a/tests/unit/model-lockout-exact-cooldown-cap.test.ts +++ b/tests/unit/model-lockout-exact-cooldown-cap.test.ts @@ -81,4 +81,60 @@ describe("recordModelLockoutFailure — exactCooldownMs cap against maxCooldownM // so exactCooldownMs=300000 should be preserved as-is assert.strictEqual(result.cooldownMs, 300_000); }); + + // #6863 vs #7940 boundary: the same magnitude (~92.5h, the Antigravity reset + // from #6863) against the same maxCooldownMs (~30min) must resolve two + // different ways depending on provenance — a SYNTHETIC estimate stays capped + // (#7940's contract), a caller-VERIFIED upstream reset passes through exactly + // (#6863's contract). #7980 regressed this by capping both indiscriminately. + const RESET_6863_MS = 332_848_000; // "Resets in 92h27m28s" + const CAP_7940_MS = 1_800_000; // 30min operator-configured max + + it("still caps a SYNTHETIC exactCooldownMs even when it dwarfs maxCooldownMs (#7940)", () => { + accountFallback.clearAllModelLockouts(); + + const result = accountFallback.recordModelLockoutFailure( + "antigravity", + "conn-5", + "claude-sonnet-4-6", + "rate_limit", + 429, + 120_000, + null, + // No exactCooldownVerified flag — mirrors an un-provenanced/estimated exact + // cooldown, which must still respect the operator's maxCooldownMs cap. + { exactCooldownMs: RESET_6863_MS, maxCooldownMs: CAP_7940_MS } + ); + + assert.strictEqual( + result.cooldownMs, + CAP_7940_MS, + `synthetic exactCooldownMs must stay capped at maxCooldownMs; got ${result.cooldownMs}` + ); + }); + + it("passes a VERIFIED exactCooldownMs through uncapped past maxCooldownMs (#6863)", () => { + accountFallback.clearAllModelLockouts(); + + const result = accountFallback.recordModelLockoutFailure( + "antigravity", + "conn-6", + "claude-sonnet-4-6", + "rate_limit", + 429, + 120_000, + null, + { + exactCooldownMs: RESET_6863_MS, + maxCooldownMs: CAP_7940_MS, + exactCooldownVerified: true, + } + ); + + assert.strictEqual( + result.cooldownMs, + RESET_6863_MS, + `verified upstream exactCooldownMs must bypass maxCooldownMs entirely; got ${result.cooldownMs}` + ); + }); });