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

@@ -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 };
       },
     },

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;

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<string> {
+async function seedConnection(
+  provider: string,
+  overrides: Record<string, unknown> = {}
+): Promise<string> {
   const conn = await providersDb.createProviderConnection({
     provider,
     authType: "apikey",
     apiKey: `${provider}-key`,
     isActive: true,
     testStatus: "active",
+    ...overrides,
   });
   return (conn as Record<string, unknown>).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"
+  );
 });