mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-07-26 09:52:11 +03:00
fix(sse): cap exact cooldowns only when synthetic — verified upstream resets pass uncapped (#8393)
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
This commit is contained in:
committed by
GitHub
parent
f0096f0224
commit
c5c27b813a
@@ -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(
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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, {
|
||||
|
||||
@@ -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}`
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user