fix(oauth): always fail over to another credential on 429

devlog/_fin/260905_always_on_429_failover/000_research_inventory.md

@@ -0,0 +1,78 @@
+# 000 — Inventory: where a 429 does and does not move to another credential
+
+## The report
+
+"멀티계정이나 멀티 api 일때 pool 모드가 안 켜져있더라도 429 나면 다른걸로 옮기는 기능이
+다 꺼져있어" — with several accounts or several API keys configured, a 429 does not move the
+request to another credential unless the operator turned a pool mode on.
+
+The follow-up constraint is what makes this a design change rather than a default flip:
+**429 failover must be on by default and must not be switchable off.**
+
+## What actually exists today
+
+Three independent rotators, three different activation rules.
+
+| Surface | Module | Activation | Verdict |
+|---|---|---|---|
+| API-key pool | `src/providers/key-failover.ts` | `hasKeyPoolFailover`: key auth + `apiKeyPool.length >= 2` | Already unconditional. This is the model to copy. |
+| Generic OAuth | `src/oauth/generic-account-failover.ts` | `isGenericOAuthFailoverEnabled`: per-provider bool > global bool > presence (2+ accounts) | On by default, **but an explicit `false` still disables it.** |
+| Anthropic OAuth | `src/oauth/anthropic-routing.ts` | `rotateAnthropicAccountOn429` returns `null` unless `isAnthropicAccountPoolEnabled(config)` | **Off by default. This is the reported bug.** |
+| Codex (openai) | `src/codex/routing.ts` | `recordCodexUpstreamOutcome` cools + `pickAlternateCodexAccount` promotes, no pool-enable flag | Already unconditional. Leave alone. |
+
+### The Anthropic hole, precisely
+
+`src/oauth/anthropic-routing.ts:456`:
+
+```ts
+export function rotateAnthropicAccountOn429(...): string | null {
+  if (!isAnthropicAccountPoolEnabled(config)) return null;
+```
+
+`anthropicAccountPool.enabled` defaults to absent, so `isAnthropicAccountPoolEnabled` is
+`false` on a stock install. An operator who logs into two Anthropic accounts and hits a 429
+gets the upstream 429 relayed to the client with no attempt at the second account.
+
+The call sites in `src/server/responses/core.ts` compound it. Both the streaming loop
+(`:6173`) and the continuation loop (`:6584`) guard on `anthropicPoolAccountId` being set —
+and that variable is only assigned at `:3412`, inside
+`if (route.providerName === "anthropic" && isAnthropicAccountPoolEnabled(config))`. So with
+the pool off there is not even an account id recorded to cool. The rotation is doubly dead:
+no identity captured, and the rotator would refuse anyway.
+
+### The generic OAuth hole
+
+`isGenericOAuthFailoverEnabled` reads presence as consent (#2568d), which is right. But the
+precedence chain lets `oauthAccountFailover.enabled: false` — global or per provider — turn
+reactive rotation off entirely. The user's instruction removes that possibility.
+
+## The distinction this unit introduces
+
+The reason Anthropic gated rotation behind the pool flag is that its pool bundles two very
+different behaviours under one switch:
+
+- **Proactive routing** — session affinity, quota-ranked new-session selection,
+  `autoSwitchThreshold`, `strategy`. This changes which account serves a *healthy* request.
+  It is experimental, it has provider-terms implications, and it stays opt-in.
+- **Reactive failover** — the account that just returned 429 is cooled and the request is
+  retried on another usable account. This only ever runs *after* upstream refused. It cannot
+  spread load, cannot cross-contaminate a session, and cannot fire at all unless the operator
+  deliberately logged in twice.
+
+Reactive failover is a safety net, not a routing policy. That is why it can be non-disableable
+without breaking the caution the pool flag was written to express: with the pool off, the
+operator still gets exactly one account per session — they just stop getting a hard 429 when
+that account is spent and a second one is sitting idle.
+
+## Non-goals
+
+- No change to Codex quota scopes or probe leases.
+- No change to combo failover.
+- No weakening of `isPoolCredentialUsable` (the fail-closed `local-cli` rule).
+- No new proactive behaviour for anyone who has not opted in.
+
+## Implementation phases
+
+- `010` — Anthropic reactive/proactive split.
+- `020` — Generic OAuth: make reactive rotation non-disableable.
+- `030` — Types, docs and surface alignment.

devlog/_fin/260905_always_on_429_failover/001_audit_round_1.md

@@ -0,0 +1,74 @@
+# 001 — Audit round 1 (grok-4.6, read-only plan audit)
+
+Verdict: **fail**, four blockers. All four independently reconfirmed against source before
+amendment; none were rebutted.
+
+## B1 — Two rotation surfaces were missed entirely
+
+The plan's call-site inventory was incomplete, and both omissions violate the binding
+requirement on their own.
+
+**B1a. The continuation loop has no generic-OAuth arm.** `src/server/responses/core.ts`
+~6549-6628 rotates API keys (`hasKeyPoolFailover` + `rotateProviderTransportOn429`) and
+Anthropic (`rotateAnthropicAccountOn429`) — and nothing else. Confirmed by scanning the
+window: the only rotators present are those two. So an xAI or Cursor continuation 429 never
+moves to a second account *even today, with failover fully enabled*. This is a pre-existing
+defect in #2568's coverage, not something this unit introduces, but it sits exactly inside
+the user's requirement.
+
+**B1b. The sidecar hook has no Anthropic arm.** `rotateSidecarProviderOn429` (~5201-5245),
+injected into both the web-search and image-bridge loops, tries the key pool and then
+*generic* OAuth. Anthropic is excluded from generic failover by design, and the hook never
+reads `anthropicPoolAccountId`. Confirmed: no occurrence of `anthropic` in the hook body.
+So an Anthropic 429 inside a web-search or image turn does not rotate — with the pool ON
+either. Also pre-existing, also in scope.
+
+## B2 — Three existing tests assert the behaviour this unit reverses
+
+Doc 020 claimed existing tests keep passing. False:
+
+- `tests/generic-oauth-failover.test.ts:80` — "an explicit knob still wins over presence"
+  expects `rotateGenericOAuthAccountOn429(config(false), ...)` to be `null`.
+- `tests/generic-oauth-failover.test.ts:107-114` — "a per-provider override beats the global
+  switch" expects `isGenericOAuthFailoverEnabled(config(true, false), "xai") === false`.
+- `tests/adapter-event-oauth-failover.test.ts:129` — "an explicit opt-out keeps single-account
+  behaviour with two accounts stored" asserts the 429 is relayed on `config(false)`.
+
+These are not incidental: they are the encoded intent of #2568d, which the user is now
+explicitly overriding. They must be **rewritten to assert the new contract**, with the reason
+recorded in the test body, not deleted and not left to fail. `tests/adapter-event-oauth-failover.test.ts`
+joins the focused verification list in 030.
+
+## B3 — 010 Change 2 proposed a duplicate credential resolution
+
+Rejected in favour of the note that followed it in the same doc. Anthropic *does* reach the
+shared else-arm when the pool is off (the inner `if` requires the pool flag), `resolved.accountId`
+there is the account that actually served the request, and a second
+`getValidAccessTokenSnapshot("anthropic")` would mint a redundant credential read. Capture is
+one line beside the existing `genericFailoverAccountId` stamp.
+
+## B4 — 030's "the GUI does not lie" claim is false
+
+`gui/src/i18n/en.ts:1818`: `"anthropicPool.disabledDesc": "Uses only the active Claude account."`
+After this change, disabled still means no affinity and no proactive pick — but a 429 *does*
+move. That string becomes stale. `gui/` stays out of scope (the AGENTS.md screenshot gate is a
+real cost for a routing fix), so 030 must record it as **known-stale copy with a follow-up**,
+not as truth-preserving.
+
+## Round 2
+
+Re-audited by the same reviewer after the amendments above. B1a, B2, B3 and B4 confirmed
+closed. B1b's *reasoning* was confirmed sound but its *code* was not implementable: the
+proposed `else if` sat behind an early `return null` and would have been dead code, with a
+naive string test still passing. Fixed in 040b by inverting the generic gate into a positive
+`else if` and deferring `return null` to a trailing `else`. Round 2 also confirmed both
+occurrence-count guards (`failoverAccountSnapshot(` and `applyFailoverSnapshot(snapshot)`)
+must move 3 -> 4, and that `oauth-account-429` is already a valid `AttemptRecoveryKind`
+(`src/usage/log.ts:52`).
+
+## Accepted without change
+
+Audit items 3 and 6 confirmed the plan: dropping the loop flag-clause introduces no regression
+(the all-cooled synthetic 429 at ~3399 correctly stays proactive-gated), and the credential
+pairing rules hold — Anthropic has no per-account origin or project, so its token-only swap is
+safe, and `applyFailoverSnapshot` must not start being used for it.

devlog/_fin/260905_always_on_429_failover/010_anthropic_reactive_split.md

@@ -0,0 +1,98 @@
+# 010 — Anthropic: reactive 429 rotation independent of the pool flag
+
+## Goal
+
+`rotateAnthropicAccountOn429` must work when `anthropicAccountPool.enabled` is absent or
+`false`, provided two or more usable Anthropic OAuth accounts are stored. Affinity, strategy
+and `autoSwitchThreshold` stay behind the flag.
+
+## Change 1 — `src/oauth/anthropic-routing.ts`
+
+Add a presence predicate beside the existing flag predicate:
+
+```ts
+/**
+ * Reactive 429 failover quorum: two or more accounts that could serve traffic if asked.
+ * Cooldowns are deliberately ignored -- this answers "did the operator log in twice",
+ * not "who is free right now", and a cooled account must not switch the feature off
+ * exactly when it is needed.
+ */
+export function hasAnthropicFailoverQuorum(now = Date.now()): boolean {
+  const set = getAccountSet(PROVIDER);
+  if (!set) return false;
+  return set.accounts.filter(a => a.needsReauth !== true && isPoolCredentialUsable(a.id, now)).length >= 2;
+}
+```
+
+Replace the hard gate in `rotateAnthropicAccountOn429`:
+
+```ts
+-  if (!isAnthropicAccountPoolEnabled(config)) return null;
++  // Reactive 429 failover is a safety net, not a routing policy: it only ever runs after
++  // upstream refused, and only when the operator deliberately stored a second account.
++  // The pool flag still gates PROACTIVE routing (affinity, strategy, autoSwitchThreshold).
++  if (!isAnthropicAccountPoolEnabled(config) && !hasAnthropicFailoverQuorum(now)) return null;
+```
+
+With the flag off, `pickAlternateAnthropicAccount` falls to the `quota` branch
+(`anthropicPoolStrategy` normalizes an absent strategy to `quota`), which calls
+`pickLowestUsage`. That reads whatever usage evidence exists and otherwise returns the first
+eligible non-excluded account — a deterministic, evidence-optional pick. No new code path.
+
+`clearAnthropicSessionAffinityForAccount` still runs. Harmless with the flag off: the
+affinity map is empty because nothing binds into it.
+
+## Change 2 — `src/server/responses/core.ts` (:3475-3480)
+
+**Amended after audit round 1 (B3).** An earlier draft of this doc proposed a dedicated
+`else if` arm that called `getValidAccessTokenSnapshot("anthropic")` itself. That is rejected:
+it mints a second credential read for an account the shared arm has already resolved.
+
+Anthropic reaches the shared OAuth else-arm whenever the pool is off, because the inner `if`
+requires `isAnthropicAccountPoolEnabled`. That arm resolves the active account into
+`resolved`, and `resolved.accountId` is precisely the account that will serve the request.
+So the capture is one stamp beside the existing generic one:
+
+```ts
+ if (isGenericFailoverProvider(route.providerName, route.provider)) {
+   genericFailoverAccountId = resolved.accountId;
+ }
++// Anthropic is excluded from isGenericFailoverProvider (its pool owns affinity and a
++// fail-closed local-cli rule), so without this its identity is dropped and a later 429 has
++// nothing to cool. Reactive failover needs only the id -- no affinity bind, no promotion,
++// no quota-ranked pick. Those are proactive and stay behind the pool flag.
++if (route.providerName === "anthropic" && hasAnthropicFailoverQuorum()) {
++  anthropicPoolAccountId = resolved.accountId;
++}
+```
+
+One resolution, one stamp, no new credential read.
+
+## Change 3 — the two rotation loops (:6173, :6584)
+
+Both read:
+
+```ts
+&& isAnthropicAccountPoolEnabled(config)
+```
+
+Drop that clause. `rotateAnthropicAccountOn429` now owns the activation decision, and
+`anthropicPoolAccountId` is only non-null when there was something to rotate. Keeping the
+clause here would re-impose the gate the module just stopped applying.
+
+`promoteAnthropicActiveAccount(nextAccountId)` inside the loop: with the pool off this
+persists the store's active account after a successful failover. That is correct and desirable
+— the old account is rate-limited, so the next request should start on the one that worked.
+It is also exactly what the API-key rotator does (`provider.apiKey = candidate.key` then
+`saveConfigPreservingClaudeCode`). Keep it.
+
+## Tests (`tests/anthropic-account-pool.test.ts` + new file)
+
+1. Pool flag absent, two usable accounts, 429 on A -> `rotateAnthropicAccountOn429` returns B
+   and A is cooled.
+2. Pool flag `false`, same -> same result (an explicit false is not a reactive kill switch).
+3. Pool flag absent, ONE account -> returns `null` (strict no-op, nowhere to go).
+4. Pool flag absent -> `resolveAnthropicAccountForSession` still returns
+   `{ reason: "pool-disabled" }` with the store active account, and binds no affinity.
+5. Pool flag absent, second account is a `local-cli` credential with expired access ->
+   no quorum, returns `null` (fail-closed rule preserved).

devlog/_fin/260905_always_on_429_failover/020_generic_oauth_non_disableable.md

@@ -0,0 +1,108 @@
+# 020 — Generic OAuth: reactive rotation stops being switchable off
+
+## Goal
+
+`oauthAccountFailover.enabled: false` — global or per provider — must no longer suppress
+reactive 429 rotation. Presence (2+ eligible accounts) becomes the sole activation rule, which
+makes generic OAuth behave exactly like the API-key pool.
+
+## Change — `src/oauth/generic-account-failover.ts`
+
+`isGenericOAuthFailoverEnabled` currently reads:
+
+```ts
+const perProvider = provider.oauthAccountFailover?.enabled;
+if (typeof perProvider === "boolean") return perProvider;
+const global = config.oauthAccountFailover?.enabled;
+if (typeof global === "boolean") return global;
+return hasFailoverAccountQuorum(providerName, now);
+```
+
+Becomes:
+
+```ts
+/**
+ * Whether reactive 429 rotation is active for this provider.
+ *
+ * Presence is the ONLY rule: two or more eligible stored accounts. The former
+ * `oauthAccountFailover.enabled` booleans no longer suppress it -- a stranded 429 with an
+ * idle second account logged in is a defect, not a configuration choice, and the operator
+ * who does not want rotation expresses that by not storing a second account.
+ *
+ * The knob survives for PROACTIVE preference (`preferredInitialAccount`), which does change
+ * which account serves a healthy request and therefore remains refusable.
+ */
+export function isGenericOAuthFailoverEnabled(config, providerName, now = Date.now()): boolean {
+  const provider = config.providers?.[providerName];
+  if (!provider || !isGenericFailoverProvider(providerName, provider)) return false;
+  return hasFailoverAccountQuorum(providerName, now);
+}
+```
+
+## The knob is not deleted — it is re-scoped
+
+Deleting `oauthAccountFailover` would be a config-compat break: existing files carry it,
+`src/config.ts` validates it, `src/oauth/index.ts:1367` preserves it across preset overwrite,
+`provider-routes.ts:952` preserves it across management writes, and
+`pool-settings-capability.ts` serves it in a DTO. Removing the field would make those paths
+drop operator data and would fail `tests/oauth-upsert-preserves-api-key.test.ts`.
+
+So the field stays and keeps its `strategy` / `autoSwitchThreshold` meaning. Only
+`enabled` changes meaning: it now governs the proactive preference, not the reactive net.
+
+`preferredInitialAccount` currently opens with `if (!isGenericOAuthFailoverEnabled(...)) return null;`.
+That call must be replaced with a proactive-specific predicate, or the re-scoped `enabled: false`
+would stop refusing the thing it is supposed to refuse:
+
+```ts
+/** Proactive pre-dispatch preference: refusable, because it moves a HEALTHY request. */
+function isProactivePreferenceEnabled(config, providerName, now): boolean {
+  const provider = config.providers?.[providerName];
+  if (!provider || !isGenericFailoverProvider(providerName, provider)) return false;
+  const perProvider = provider.oauthAccountFailover?.enabled;
+  if (typeof perProvider === "boolean" && !perProvider) return false;
+  const global = config.oauthAccountFailover?.enabled;
+  if (typeof global === "boolean" && !global) return false;
+  return hasFailoverAccountQuorum(providerName, now);
+}
+```
+
+Only `false` is honoured here; `true` adds nothing over presence. That keeps the predicate
+monotone with the old behaviour for every operator who never wrote the key.
+
+## Call sites in `src/server/responses/core.ts`
+
+`:5222`, `:5528`, `:6216` all guard rotation with `isGenericOAuthFailoverEnabled`. They need
+no edit — the predicate they call simply became presence-only. `:3422` guards
+`preferredInitialAccount`, which now self-gates on the proactive predicate.
+
+## Tests
+
+**Amended after audit round 1 (B2).** Three existing tests encode the OLD contract and will go
+red. They are rewritten to assert the new one, each carrying the reason in the test body — a
+reversed assertion with no explanation is indistinguishable from a test someone broke.
+
+Rewritten:
+
+- `tests/generic-oauth-failover.test.ts:80` "an explicit knob still wins over presence" ->
+  becomes "an explicit knob no longer disables reactive rotation": `config(false)` still
+  rotates.
+- `tests/generic-oauth-failover.test.ts:107-114` "a per-provider override beats the global
+  switch" -> the override now governs the PROACTIVE preference only; reactive rotation ignores
+  both booleans.
+- `tests/adapter-event-oauth-failover.test.ts:129` "an explicit opt-out keeps single-account
+  behaviour with two accounts stored" -> with two accounts stored, the opt-out no longer keeps
+  the 429; the second account serves the retry.
+
+New:
+
+1. `oauthAccountFailover.enabled: false` globally, two accounts, 429 -> still rotates.
+2. Per-provider `enabled: false`, two accounts, 429 -> still rotates.
+3. One account -> `null` regardless of any flag (strict no-op preserved).
+4. `enabled: false` -> `preferredInitialAccount` returns `null` even with headroom evidence,
+   proving the proactive refusal survived the re-scope.
+
+Unaffected (verified, not assumed): `tests/account-pool-management-api.test.ts` (Anthropic pool
+DTO round-trip), `tests/management-provider-validation.test.ts:996-1031` and
+`tests/oauth-upsert-preserves-api-key.test.ts` (field preservation only — the knob is kept, so
+preservation still holds).