From a13869364e65ab2a6897ef10e46ae70003a0b81e Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Fri, 14 Aug 2026 14:01:48 -0300 Subject: [PATCH] fix(sse): guard connection-scope cooldown against permanent agentrouter states Adversarial review of the #10334 Task 2 connection-scope branch found the "never terminal" invariant relied only on ruleScope === "connection", which is not structurally guaranteed against a future agentrouter rule pairing that scope with a permanent/credits-exhausted reason. Extract the guard into an exported, independently-testable predicate (isAgentrouterConnectionQuotaScope) that also requires reason === QUOTA_EXHAUSTED and !permanent/!creditsExhausted. Also: document the disableCooling(#2997) interaction and the providerErrorRules.ts 6h-vs-30min-cap discrepancy the review flagged, and add position-guard + exclusivity-positive tests so a future refactor that reorders the branch or stops locking non-agentrouter providers cannot pass silently. --- open-sse/config/providerErrorRules.ts | 11 +- src/sse/services/auth.ts | 58 +++++- .../unit/agentrouter-lock-scope-10334.test.ts | 167 +++++++++++++++++- 3 files changed, 231 insertions(+), 5 deletions(-) diff --git a/open-sse/config/providerErrorRules.ts b/open-sse/config/providerErrorRules.ts index 6d1573d2ae..17d72be598 100644 --- a/open-sse/config/providerErrorRules.ts +++ b/open-sse/config/providerErrorRules.ts @@ -231,8 +231,15 @@ function buildAgentrouterRules(): ProviderErrorRule[] { if (status !== 403) return null; const text = JSON.stringify(body ?? "").toLowerCase(); if (!text.includes("无权访问模型")) return null; - // 6h: effectively "until the operator fixes the key's model grants", - // without being an unrecoverable terminal state. + // Declares a 6h cooldown, but the effective cooldown is NOT 6h: the + // model-lockout persistence layer (recordModelLockoutFailure, called from + // markAccountUnavailable) clamps every base cooldown — this one included — + // to the configured model-lockout maxCooldownMs, which defaults to + // 1_800_000ms / 30min (src/lib/resilience/modelLockoutSettings.ts, + // DEFAULT_MODEL_LOCKOUT_SETTINGS.maxCooldownMs). So in practice this is + // "locked for ~30min by default (up to 6h if an operator raises the model- + // lockout cap in settings)", not "until the operator fixes the key's model + // grants" — it is a recoverable window, not a real fix-driven unlock. return { reason: "auth_error", scope: "model", cooldownMs: 6 * 60 * 60 * 1000 }; }, }, diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index c971f8073a..5a012cb5f5 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -1982,6 +1982,46 @@ export async function getProviderCredentialsWithQuotaPreflight( } } +/** + * #10334 — Guard for the agentrouter-exclusive "connection scope" quota + * cooldown branch in markAccountUnavailable. The "never terminal" invariant of + * that branch is NOT structurally guaranteed by `ruleScope === "connection"` + * alone — it also depends on the provider rule table only ever pairing scope + * "connection" with a genuinely transient reason. Today + * (`buildAgentrouterRules()` in providerErrorRules.ts) that is true: the only + * rule declaring scope "connection" is the quota-exhausted one. But a FUTURE + * agentrouter rule for a permanent account state (e.g. "账号已封禁") — or a 402 + * added to `AGENTROUTER_ERROR_STATUSES` with scope "connection", a natural- + * looking choice for an account ban — would otherwise be silently downgraded + * to a transient cooldown here instead of going through + * resolveTerminalConnectionStatus()/auto-disable below. Require the + * reason/permanent/creditsExhausted signals checkFallbackError already + * computes to explicitly confirm "this is quota, not a permanent state" + * before taking the early return. + * + * Exported (not just inlined) so a synthetic permanent/credits-exhausted + * `fallbackResult` can be tested directly — no rule in the table produces + * that combination today, so this predicate is the only way to pin the guard + * without editing the (production) rule table just for a test. + */ +export function isAgentrouterConnectionQuotaScope( + provider: string | null | undefined, + fallbackResult: { + ruleScope?: "model" | "provider" | "connection"; + reason?: string; + permanent?: boolean; + creditsExhausted?: boolean; + } +): boolean { + return ( + honorsRuleLockScope(provider) && + fallbackResult.ruleScope === "connection" && + fallbackResult.reason === RateLimitReason.QUOTA_EXHAUSTED && + !fallbackResult.permanent && + !fallbackResult.creditsExhausted + ); +} + /** Persist exponential-backoff state for an unavailable provider connection. */ export async function markAccountUnavailable( connectionId: string, @@ -2114,8 +2154,22 @@ export async function markAccountUnavailable( // below would then lock per MODEL instead of cooling the connection — exactly // what this scope must override. NEVER sets a terminal status: this is a // renewing quota window, not "credits_exhausted"/"banned"/"expired". - const ruleScopeIsConnection = - honorsRuleLockScope(provider) && fallbackResult.ruleScope === "connection"; + // + // The "never terminal" invariant above is NOT structurally guaranteed by + // ruleScope === "connection" alone — see isAgentrouterConnectionQuotaScope's + // doc comment for why (a future permanent-state rule could pair scope + // "connection" with a non-quota reason). That predicate is the actual guard. + const ruleScopeIsConnection = isAgentrouterConnectionQuotaScope(provider, fallbackResult); + // #2997's disableCooling opt-out is respected here (`!disableCooling` below): + // a connection with disableCooling=true skips this branch entirely and falls + // into the per-model-quota block further down, which locks the model for up + // to ~30min (mlSettings.maxCooldownMs) instead of cooling the connection for + // the rule's shorter transient window. That is a deliberate, if counter- + // intuitive, consequence of #2997's scope (opt-out was designed only for the + // CONNECTION-level cooldown, never extended to model lockout) — "opting out + // of cooldown" ends up producing a LONGER effective block for this one rule. + // Not addressed here; flagged for a future #2997 follow-up if it proves to be + // a real operator complaint. if (ruleScopeIsConnection && provider && !disableCooling) { const connectionCooldownMs = fallbackResult.cooldownMs > 0 ? fallbackResult.cooldownMs : COOLDOWN_MS.rateLimit; diff --git a/tests/unit/agentrouter-lock-scope-10334.test.ts b/tests/unit/agentrouter-lock-scope-10334.test.ts index 40d629b86f..fd538b762e 100644 --- a/tests/unit/agentrouter-lock-scope-10334.test.ts +++ b/tests/unit/agentrouter-lock-scope-10334.test.ts @@ -31,13 +31,17 @@ async function resetStorage() { fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); } -async function seedConnection(provider: string): Promise { +async function seedConnection( + provider: string, + overrides: Record = {} +): Promise { const conn = await providersDb.createProviderConnection({ provider, authType: "apikey", apiKey: `${provider}-key`, isActive: true, testStatus: "active", + ...overrides, }); return (conn as Record).id as string; } @@ -169,6 +173,12 @@ test("exclusivity: ollama-cloud with an equivalent account-wide-looking 429 keep // active, no rateLimitedUntil. assert.equal(after.testStatus, "active"); assert.ok(!after.rateLimitedUntil, "non-agentrouter providers must not gain connection cooldown"); + + // Positive assertion, not just the negative: the model lockout must have + // actually been recorded. Without this, a future refactor that stops + // locking anything for these providers would pass this test silently. + const lockout = accountFallback.getModelLockoutInfo("ollama-cloud", connId, "claude-opus-5"); + assert.ok(lockout, "expected the pre-existing per-model lockout to be recorded"); }); test("exclusivity: vertex with an equivalent account-wide-looking 429 keeps today's per-model lockout, no connection cooldown", async () => { @@ -188,4 +198,159 @@ test("exclusivity: vertex with an equivalent account-wide-looking 429 keeps toda const after = await providersDb.getProviderConnectionById(connId); assert.equal(after.testStatus, "active"); assert.ok(!after.rateLimitedUntil, "non-agentrouter providers must not gain connection cooldown"); + + // Positive assertion, not just the negative — see the ollama-cloud case above. + const lockout = accountFallback.getModelLockoutInfo("vertex", connId, "claude-opus-5"); + assert.ok(lockout, "expected the pre-existing per-model lockout to be recorded"); +}); + +// ─── Fix round 1 (#10334 review) ─────────────────────────────────────────── + +// Important finding: the "never terminal" invariant is not structurally +// guaranteed by `ruleScope === "connection"` alone — it depends on the +// provider rule table only ever pairing scope "connection" with a genuinely +// transient reason. isAgentrouterConnectionQuotaScope() is the actual guard; +// pin its predicate directly with synthetic fallbackResult shapes, since no +// rule in the current table produces a permanent/credits-exhausted result +// with scope "connection" (exercising it end-to-end would require editing +// the production rule table just for a test). +test("isAgentrouterConnectionQuotaScope: rejects a permanent rule result even with scope connection", () => { + const permanentConnectionScopeResult = { + ruleScope: "connection" as const, + reason: "auth_error", + permanent: true, + }; + assert.equal( + auth.isAgentrouterConnectionQuotaScope("agentrouter", permanentConnectionScopeResult), + false, + "a future permanent-state rule with scope connection must NOT take the transient-cooldown branch" + ); +}); + +test("isAgentrouterConnectionQuotaScope: rejects a credits-exhausted rule result even with scope connection", () => { + const creditsExhaustedConnectionScopeResult = { + ruleScope: "connection" as const, + reason: "quota_exhausted", + creditsExhausted: true, + }; + assert.equal( + auth.isAgentrouterConnectionQuotaScope("agentrouter", creditsExhaustedConnectionScopeResult), + false, + "a future credits-exhausted rule with scope connection must NOT take the transient-cooldown branch" + ); +}); + +test("isAgentrouterConnectionQuotaScope: accepts the real quota-exhausted/connection shape", () => { + const quotaConnectionScopeResult = { + ruleScope: "connection" as const, + reason: "quota_exhausted", + }; + assert.equal( + auth.isAgentrouterConnectionQuotaScope("agentrouter", quotaConnectionScopeResult), + true, + "today's only connection-scope rule result (quota_exhausted, no permanent/creditsExhausted) must pass" + ); +}); + +test("isAgentrouterConnectionQuotaScope: rejects non-agentrouter providers regardless of shape", () => { + const quotaConnectionScopeResult = { + ruleScope: "connection" as const, + reason: "quota_exhausted", + }; + assert.equal( + auth.isAgentrouterConnectionQuotaScope("ollama-cloud", quotaConnectionScopeResult), + false, + "honorsRuleLockScope must still gate every provider outside the agentrouter allowlist" + ); +}); + +// Minor finding: guard the branch's POSITION in markAccountUnavailable. If a +// future refactor moved the branch above the terminal-status guard (~line +// 2023) or the anti-thundering-herd guard (~line 2038), a credits_exhausted +// connection would be silently overwritten, or a live cooldown would be +// shortened — and the 6 tests above would stay green because none of them +// seed a connection with pre-existing terminal/cooldown state. +test("position guard: a connection already credits_exhausted stays terminal through an agentrouter quota 429", async () => { + await resetStorage(); + const connId = await seedConnection("agentrouter", { testStatus: "credits_exhausted" }); + + const result = await auth.markAccountUnavailable( + connId, + 429, + QUOTA_EXHAUSTED_429, + "agentrouter", + "claude-opus-5" + ); + + assert.equal(result.shouldFallback, true); + assert.equal(result.cooldownMs, 0, "terminal-status short-circuit returns cooldownMs 0"); + + const after = await providersDb.getProviderConnectionById(connId); + assert.equal( + after.testStatus, + "credits_exhausted", + "the connection-scope branch must never overwrite a pre-existing terminal status" + ); +}); + +test("position guard: an existing live cooldown is not shortened by the connection-scope branch", async () => { + await resetStorage(); + const futureCooldown = new Date(Date.now() + 10 * 60 * 1000).toISOString(); + const connId = await seedConnection("agentrouter", { + testStatus: "unavailable", + rateLimitedUntil: futureCooldown, + }); + + const result = await auth.markAccountUnavailable( + connId, + 429, + QUOTA_EXHAUSTED_429, + "agentrouter", + "claude-opus-5" + ); + + assert.equal(result.shouldFallback, true); + + const after = await providersDb.getProviderConnectionById(connId); + assert.equal( + after.rateLimitedUntil, + futureCooldown, + "the anti-thundering-herd guard must win: an existing live cooldown must not be reset/shortened" + ); +}); + +// Minor finding: disableCooling=true skips the connection-scope branch (the +// `!disableCooling` condition), so the #10334 bug survives for connections +// with that opt-out — they fall into the ~30min per-model lockout instead of +// the shorter connection cooldown. Documented in the block comment above the +// branch; pin the behavior so a future change to the guard is deliberate. +test("disableCooling=true skips the connection-scope branch and falls back to per-model lockout", async () => { + await resetStorage(); + const connId = await seedConnection("agentrouter", { + providerSpecificData: { disableCooling: true }, + }); + + const result = await auth.markAccountUnavailable( + connId, + 429, + QUOTA_EXHAUSTED_429, + "agentrouter", + "claude-opus-5" + ); + + assert.equal(result.shouldFallback, true); + + const after = await providersDb.getProviderConnectionById(connId); + // Connection is NOT cooled down — disableCooling's documented CONNECTION- + // level opt-out (#2997) is honored. + assert.equal(after.testStatus, "active"); + assert.ok(!after.rateLimitedUntil, "disableCooling must keep the connection selectable"); + + // But the model IS locked out instead (the #10334 bug's exact symptom for + // disableCooling connections — a deliberate, documented trade-off). + const lockout = accountFallback.getModelLockoutInfo("agentrouter", connId, "claude-opus-5"); + assert.ok( + lockout, + "expected a per-model lockout when disableCooling bypasses the connection branch" + ); });