feat(oauth): generic multi-account 429 failover for HTTP-status paths (#2568a)

devlog/_plan/260825_owner_backlog_and_bugpr_closeout/110_wp7_oauth_failover.md

@@ -0,0 +1,64 @@
+# 110 — wp7: generic OAuth multi-account 429 failover (#2568)
+
+## Call-site enumeration (audit hazard H4)
+
+Every `hasKeyPoolFailover` reference on dev, and what it actually does:
+
+| File | Status |
+|---|---|
+| `src/providers/key-failover.ts:86` | the definition |
+| `src/server/responses/core.ts:4908` | live 429 rotation loop (streaming) |
+| `src/server/responses/core.ts:5252` | live 429 rotation (non-streaming) |
+| `src/server/chat-native.ts:248` | live 429 rotation (native Chat) |
+| `src/server/responses/compact.ts:86` | **import only** — no call |
+| `src/server/responses/collaboration.ts:69` | **import only** — no call |
+| `src/server/responses/encrypted-payload.ts:68` | **import only** — no call |
+
+So there are exactly THREE live key-pool rotation sites, not five. The three
+import-only files are dead references; they are out of scope here but worth noting, since
+the audit's concern was that a rotator could be generalized while leaving live paths
+unfixed.
+
+OAuth rotation today exists at exactly two sites, both in `core.ts`
+(`:4941` streaming, `:5288` non-streaming), and both are Anthropic-only and gated on
+`isAnthropicAccountPoolEnabled`.
+
+## Design
+
+`anthropic-routing.ts` is already written against generic primitives. The provider-specific
+parts are three: the constant `PROVIDER = "anthropic"`, the config gate, and token minting
+via `getAnthropicPoolAccessToken`. The last one is NOT trivially generic — it enforces a
+fail-closed rule about background `local-cli` credential slots that exists because of how
+the Claude CLI stores its token. A generic rotator must not silently apply or drop that rule.
+
+Plan: parameterize the rotator by provider, keep the Anthropic credential rule attached to
+Anthropic, and let each provider declare whether it participates.
+
+## Consent decision (recorded assumption, HIGH severity — needs owner review)
+
+The issue proposes presence-driven activation: 2+ accounts means rotate, mirroring how a
+2-key API pool is treated as consent. `request_user_input` is denied under an active goal,
+so I could not ask, and this is a product decision rather than a code one.
+
+**Assumption taken: ship it OPT-IN, not presence-driven.** Reasoning:
+
+- Rotating across subscription accounts spends a second account's quota. An API key pool
+  spends the operator's own metered credit; a subscription account is a different kind of
+  resource, and the Anthropic pool shipped opt-in for exactly this reason.
+- Opt-in is reversible in one direction only that matters: turning it on later costs the
+  user nothing, whereas a default-on rotation that surprises someone has already spent the
+  quota.
+- The audit flagged this specific question as needing explicit review rather than being
+  settled by an opt-out knob.
+
+If the owner prefers presence-driven, the change is a one-line default in
+`oauthAccountFailoverEnabled` — the mechanism does not change.
+
+## Acceptance
+
+1. A provider with 2+ eligible accounts and the knob on rotates on 429 and replays once.
+2. A single-account provider is a strict no-op.
+3. The knob off is a strict no-op regardless of account count.
+4. Existing Anthropic configuration keeps its current meaning.
+5. The Codex pool is untouched.
+

devlog/_plan/260825_owner_backlog_and_bugpr_closeout/111_wp7_audit_response.md

@@ -0,0 +1,85 @@
+# 111 — wp7 audit response: the plan was wrong, and the scope is bigger than #2568 states
+
+Auditor verdict: **fail**, 7 blocking findings. I verified each against the tree. Six hold;
+one I partially rebut. The honest conclusion is that `110` understated the problem badly
+enough that it should not be implemented as written.
+
+## Verified against the tree
+
+**B1 — the call-site table was incomplete. ACCEPTED.** The three import-only files were
+right, and the three direct callers were right, but there are two more LIVE rotation sites
+reached through an injected `on429` hook rather than a direct call:
+`src/images/loop.ts:574` and `src/web-search/loop.ts:511`, wired from
+`core.ts:4267` and `core.ts:4355`. My `rg` for the symbol could not see them because the
+call site passes a closure. That is exactly the failure mode hazard H4 warned about.
+
+**B2 — the standalone Antigravity image endpoint bypasses core entirely. ACCEPTED.**
+`src/server/images.ts` resolves the active OAuth token, reads its project separately, and
+returns upstream 429 without any rotation.
+
+**B3 — Cursor 429s never reach an HTTP-status rotator. ACCEPTED, and this is the finding
+that breaks the plan.** Cursor converts transport failures into adapter EVENTS
+(`src/adapters/cursor.ts:308`), and both the streaming and buffered paths have already
+committed HTTP 200 by then (`core.ts:4556`, `:4619`). `cursor-errors.ts` classifies a bare
+`resource_exhausted` tail into a 429 class at the ADAPTER layer, not as a response status.
+So a `response.status === 429` loop — which is the entire mechanism #2568 proposes — cannot
+serve Cursor at all. The issue names Cursor as an affected provider; the proposed design
+cannot deliver it.
+
+**B4 — a token-only resolver is unsafe. ACCEPTED.** `getValidAccessTokenForAccount` returns
+a string, but Copilot credentials carry an account-specific `apiBaseUrl`
+(`src/oauth/github-copilot.ts:281`) and Antigravity needs an account-matched `projectId`
+that `core.ts:2780` deliberately keeps paired with the token snapshot precisely so "an
+account rotation cannot mix a fresh token with project metadata re-read from a different
+credential generation". The existing code already anticipated this hazard; my plan would
+have reintroduced it.
+
+**B5 — continuity/cache criteria absent. ACCEPTED.** Anthropic rebinds affinity on rotation
+(`anthropic-routing.ts:490`); xAI pins conversation/session ids
+(`providers/xai-transport.ts:105`); Cursor scopes checkpoints to credential identity
+(`cursor/request-builder.ts:422`). A rotator that ignores these silently corrupts
+continuation state rather than failing loudly.
+
+**B7 — the acceptance criteria permitted a partial ship. ACCEPTED.** "A provider" and "one
+replay" would have been satisfied by xAI-Responses-only, while Anthropic already allows
+three failovers.
+
+## Partial rebuttal
+
+**B6 — opt-in vs presence-driven.** The auditor is right that the issue asks for failover
+"without first discovering and enabling a toggle", and right that default-off lets the
+implementation pass while the reported xAI workflow stays broken. I accept that as the
+issue's contract.
+
+Where I do not fully agree: the auditor treats this as a plan defect. It is a product
+decision I was structurally unable to take — `request_user_input` is denied under an active
+goal, and spending a second subscription account's quota by default is not a call an agent
+should make silently. The correct resolution is not to pick a default under duress; it is to
+escalate. Recorded as such below.
+
+## Consequence: wp7 does not proceed as a single work-phase
+
+The plan claimed a parameterization. The tree says otherwise: three distinct failure
+surfaces (HTTP-status rotation, adapter-event classification, and a bypassing image
+endpoint), three distinct credential shapes (bare token, token+apiBaseUrl,
+token+projectId), and three distinct continuity contracts. That is a program, not a phase.
+
+wp7 is therefore split, and the first unit is the only one that can be built safely without
+an owner decision:
+
+- **wp7a** — generic rotator for HTTP-status OAuth paths (`core.ts` streaming and
+  non-streaming), with a per-provider credential resolver that returns the FULL snapshot
+  (token plus whatever routing metadata that provider pairs with it), not a bare string.
+  Gated behind an explicit knob so no default changes.
+- **wp7b** — adapter-event 429 rotation, which is what Cursor actually needs.
+- **wp7c** — `src/server/images.ts` and the injected `on429` sidecar loops.
+- **wp7d** — the activation-default decision. **ESCALATE: needs the owner.**
+
+## Escalation (NEEDS_HUMAN, recorded)
+
+The default-on question is not mine to settle. Presence-driven rotation spends another
+subscription account's quota without the operator asking for it in that moment; opt-in
+leaves the issue's stated workflow broken. Both readings are defensible and the issue text
+supports the auditor's. This phase reports **NEEDS_HUMAN** on that specific decision, and
+implements nothing that depends on it.
+

src/oauth/generic-account-failover.ts

@@ -0,0 +1,160 @@
+/**
+ * Generic OAuth multi-account 429 failover (#2568).
+ *
+ * The API-key twin (`providers/key-failover.ts`) rotates by default for any key provider with a
+ * 2+ pool, but it returns false for `authMode === "oauth"`, and the only OAuth rotator that
+ * exists is Anthropic's — behind its own opt-in. So xAI, Cursor, Kimi, GitHub Copilot,
+ * Antigravity and Nous have no recovery path on a 429 even with several accounts logged in.
+ *
+ * Deliberately narrower than the Anthropic pool: no session affinity, no quota-ranked selection,
+ * no probe leases. Those carry provider-specific meaning; this module only answers "the account
+ * that just 429'd is cooled, is there another one we may use".
+ *
+ * NOT a home for Codex (`codex/routing.ts` owns quota scopes and probe leases) or Anthropic
+ * (`oauth/anthropic-routing.ts` owns affinity and a fail-closed local-cli credential rule).
+ * Both are excluded by `isGenericFailoverProvider`.
+ */
+import { getAccountSet } from "./store";
+import { getValidAccessSnapshotForAccount, type OAuthAccessSnapshot } from "./index";
+import { parseRetryAfterMs } from "../combos/failover";
+import { sweepExpiredOnWrite } from "../lib/state-store-sweeper";
+import type { OcxConfig, OcxProviderConfig } from "../types";
+
+/** Cap same-request rotations so a short Retry-After cannot spin. Mirrors the Anthropic bound. */
+export const GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST = 3;
+
+const DEFAULT_COOLDOWN_MS = 60_000;
+const MAX_COOLDOWN_MS = 15 * 60_000;
+
+/**
+ * Providers whose rotation is owned elsewhere and must not be handled here.
+ *
+ * `openai` is the Codex pool: quota scopes, probe leases and affinity semantics that this
+ * module deliberately does not reimplement. `anthropic` has its own pool with a fail-closed
+ * rule about background local-cli credential slots.
+ */
+const EXCLUDED_PROVIDERS = new Set(["openai", "anthropic"]);
+
+interface AccountHealth {
+  cooldownUntil: number;
+  cooldownSource: "retry-after" | "default";
+}
+
+/** Process-local, like the Anthropic pool's: a restart is allowed to forget a cooldown. */
+const health = new Map<string, AccountHealth>();
+
+const healthKey = (provider: string, accountId: string) => `${provider}\u0000${accountId}`;
+
+function isCooled(provider: string, accountId: string, now: number): boolean {
+  const entry = health.get(healthKey(provider, accountId));
+  if (!entry) return false;
+  if (entry.cooldownUntil <= now) {
+    health.delete(healthKey(provider, accountId));
+    return false;
+  }
+  return true;
+}
+
+/** True when this provider participates in generic rotation at all. */
+export function isGenericFailoverProvider(providerName: string, provider: OcxProviderConfig): boolean {
+  return provider.authMode === "oauth" && !EXCLUDED_PROVIDERS.has(providerName);
+}
+
+/**
+ * Whether generic rotation is active for this provider.
+ *
+ * Default OFF pending an owner decision on presence-driven activation (#2568 asks for no
+ * toggle; rotating spends another subscription account's quota, so the default is escalated
+ * rather than chosen here). The mechanism does not change if the default flips.
+ */
+export function isGenericOAuthFailoverEnabled(config: OcxConfig, providerName: string): boolean {
+  const provider = config.providers?.[providerName];
+  if (!provider || !isGenericFailoverProvider(providerName, provider)) return false;
+  return config.oauthAccountFailover?.enabled === true;
+}
+
+/** Accounts that may serve traffic right now: not cooled, not flagged for reauth. */
+export function eligibleFailoverAccounts(providerName: string, now = Date.now()): string[] {
+  const set = getAccountSet(providerName);
+  if (!set) return [];
+  return set.accounts
+    .filter(account => account.needsReauth !== true && !isCooled(providerName, account.id, now))
+    .map(account => account.id);
+}
+
+/**
+ * Cool the account that actually 429'd and name the next eligible one, or null.
+ *
+ * Returns the id only; the caller mints the credential so a failed refresh does not leave the
+ * cooldown applied to an account we then could not use.
+ */
+export function rotateGenericOAuthAccountOn429(
+  config: OcxConfig,
+  providerName: string,
+  failedAccountId: string,
+  retryAfterHeader: string | null | undefined,
+  now = Date.now(),
+): string | null {
+  if (!isGenericOAuthFailoverEnabled(config, providerName)) return null;
+  const set = getAccountSet(providerName);
+  // A single stored account has nowhere to go; rotating to itself would just replay the 429.
+  if (!set || set.accounts.length < 2) return null;
+
+  const parsed = parseRetryAfterMs(retryAfterHeader, now);
+  const cooldownMs = Math.min(parsed ?? DEFAULT_COOLDOWN_MS, MAX_COOLDOWN_MS);
+  health.set(healthKey(providerName, failedAccountId), {
+    cooldownUntil: now + cooldownMs,
+    cooldownSource: parsed ? "retry-after" : "default",
+  });
+  sweepExpiredOnWrite(now);
+
+  const eligible = eligibleFailoverAccounts(providerName, now).filter(id => id !== failedAccountId);
+  if (eligible.length === 0) return null;
+  // Deterministic: start after the failed account so repeated 429s walk the roster instead of
+  // hammering whichever id happens to sort first.
+  const order = set.accounts.map(account => account.id);
+  const start = order.indexOf(failedAccountId);
+  for (let i = 1; i <= order.length; i++) {
+    const candidate = order[(start + i) % order.length]!;
+    if (candidate !== failedAccountId && eligible.includes(candidate)) return candidate;
+  }
+  return null;
+}
+
+/**
+ * Full credential snapshot for a rotated account.
+ *
+ * Returns the snapshot rather than a bare bearer: Antigravity pairs an account-matched
+ * `projectId` with its token and Kiro carries routing metadata, so a token-only swap would mix
+ * one account's bearer with another's routing data.
+ */
+export async function failoverAccountSnapshot(
+  providerName: string,
+  accountId: string,
+): Promise<OAuthAccessSnapshot> {
+  return getValidAccessSnapshotForAccount(providerName, accountId);
+}
+
+/** Earliest remaining cooldown, for a client-facing Retry-After when every account is cooled. */
+export function genericFailoverRetryAfterSeconds(providerName: string, now = Date.now()): number | null {
+  const set = getAccountSet(providerName);
+  if (!set) return null;
+  let earliest: number | null = null;
+  for (const account of set.accounts) {
+    const entry = health.get(healthKey(providerName, account.id));
+    if (!entry || entry.cooldownUntil <= now) continue;
+    if (earliest === null || entry.cooldownUntil < earliest) earliest = entry.cooldownUntil;
+  }
+  return earliest === null ? null : Math.max(1, Math.ceil((earliest - now) / 1000));
+}
+
+/** Test seam and manual-recovery hook. */
+export function clearGenericFailoverHealth(providerName?: string): void {
+  if (!providerName) {
+    health.clear();
+    return;
+  }
+  for (const key of [...health.keys()]) {
+    if (key.startsWith(`${providerName}\u0000`)) health.delete(key);
+  }
+}

src/oauth/index.ts

@@ -490,6 +490,22 @@ export async function getValidAccessTokenForAccount(provider: string, accountId:
   return (await resolveAccessSnapshotForAccount(provider, accountId)).accessToken;
 }
 
+/**
+ * Account-scoped resolver returning the FULL snapshot, not just the bearer.
+ *
+ * A rotator that swaps only the token silently mixes credential generations: Antigravity pairs
+ * an account-matched `projectId` with its token (see the pairing comment in
+ * server/responses/core.ts), Kiro carries routing metadata, and Copilot's observed snapshot
+ * carries an account-specific API origin. Reading those back from "whichever account is active"
+ * after a rotation is exactly the mixing this returns in one piece to prevent (#2568).
+ */
+export async function getValidAccessSnapshotForAccount(
+  provider: string,
+  accountId: string,
+): Promise<OAuthAccessSnapshot> {
+  return resolveAccessSnapshotForAccount(provider, accountId);
+}
+
 /** Terminal refresh failures (revoked/rotated-away grants) — retrying cannot succeed. */
 function isTerminalRefreshError(err: unknown): boolean {
   const msg = (err instanceof Error ? err.message : String(err)).toLowerCase();

src/routing/analytics.ts

@@ -119,6 +119,7 @@ const COOLDOWN_RECOVERY_KINDS = new Set([
   "key-429",
   "oauth-401",
   "anthropic-oauth-429",
+  "oauth-account-429",
 ]);
 
 function percentile(sorted: number[], p: number): number | undefined {