diff --git a/devlog/_fin/260905_always_on_429_failover/000_research_inventory.md b/devlog/_fin/260905_always_on_429_failover/000_research_inventory.md new file mode 100644 index 0000000000..3a4d9934b4 --- /dev/null +++ b/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. diff --git a/devlog/_fin/260905_always_on_429_failover/001_audit_round_1.md b/devlog/_fin/260905_always_on_429_failover/001_audit_round_1.md new file mode 100644 index 0000000000..402042c5f0 --- /dev/null +++ b/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. diff --git a/devlog/_fin/260905_always_on_429_failover/010_anthropic_reactive_split.md b/devlog/_fin/260905_always_on_429_failover/010_anthropic_reactive_split.md new file mode 100644 index 0000000000..5183731af7 --- /dev/null +++ b/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). diff --git a/devlog/_fin/260905_always_on_429_failover/020_generic_oauth_non_disableable.md b/devlog/_fin/260905_always_on_429_failover/020_generic_oauth_non_disableable.md new file mode 100644 index 0000000000..715956155c --- /dev/null +++ b/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). diff --git a/devlog/_fin/260905_always_on_429_failover/030_types_docs_surface.md b/devlog/_fin/260905_always_on_429_failover/030_types_docs_surface.md new file mode 100644 index 0000000000..63b8d39632 --- /dev/null +++ b/devlog/_fin/260905_always_on_429_failover/030_types_docs_surface.md @@ -0,0 +1,73 @@ +# 030 — Types, docs and management surface + +## `src/types/config.ts` + +`anthropicAccountPool` doc comment currently says "Failover on 429 + sticky affinity". After +010 the flag no longer owns failover, so the comment must stop claiming it: + +``` + * Opt-in Anthropic OAuth PROACTIVE routing (#294). Default OFF. + * Sticky session affinity and quota-ranked new-session selection. + * Reactive 429 failover is NOT gated here -- it activates on account presence like every + * other multi-credential provider, and cannot be switched off. +``` + +`oauthAccountFailover` doc comment must stop advertising `false` as a way to keep strict +single-account behaviour on 429, and say what it does govern now. + +## `src/types/provider.ts` + +Same correction on the per-provider override: an explicit boolean no longer "beats presence" +for reactive rotation; it governs the proactive pre-dispatch preference. + +## `docs-site/` + +No page currently documents `anthropicAccountPool` or `oauthAccountFailover` (an `rg` over +`docs-site/src/content/docs/en/` for those identifiers returns nothing), so there is no stale +English page to correct and no translated locale that can contradict it. Scope here is +therefore the in-repo type comments plus this devlog unit, and a docs page is out of scope +rather than skipped: adding a first-ever provider-pooling page would be a separate unit with +its own translation obligation across ten locales. + +## Management API / GUI + +**Corrected after audit round 1 (B4).** An earlier draft claimed the GUI copy stays truthful. +It does not. `gui/src/i18n/en.ts:1818`: + +``` +"anthropicPool.disabledDesc": "Uses only the active Claude account. Enable only if you accept experimental routing." +``` + +After 010 that is **stale**: disabled still means no affinity and no proactive pick, but a 429 +now does move to another account. The string overstates what the off position buys. + +`gui/` nevertheless stays out of this PR. Per `AGENTS.md` a PR whose title or description +mentions `gui` must carry a screenshot of the UI change, and this is a routing fix whose +reviewability suffers from a ten-locale copy pass bolted on. The honest record is therefore: +**known-stale copy, follow-up owed**, across `en` and the nine translated locales that mirror +it. Not "the panel does not lie". + +`genericPoolSettingsDto` reporting `inert: true` is unaffected — it describes the `strategy` +and `autoSwitchThreshold` fields the selector still does not consume. + +## Verification plan + +Per the user's standing instruction, **no repository-wide local suite**. Focused only: + +``` +bun run typecheck +bun test tests/anthropic-account-pool.test.ts +bun test tests/generic-oauth-failover.test.ts +bun test tests/adapter-event-oauth-failover.test.ts +bun test tests/key-failover.test.ts +bun test tests/always-on-429-failover.test.ts +bun test tests/account-pool-management-api.test.ts +bun test tests/oauth-upsert-preserves-api-key.test.ts +``` + +`tests/adapter-event-oauth-failover.test.ts` was added to this list after audit round 1 (B2): +it drives a real Cursor 429 through `handleResponses` and asserts the opt-out behaviour this +unit reverses, so omitting it would have moved the failure to CI. + +Repository-wide validation is delegated to GitHub Actions on the exact PR head SHA, which must +be observed green before the admin merge. diff --git a/devlog/_fin/260905_always_on_429_failover/040_missed_surfaces.md b/devlog/_fin/260905_always_on_429_failover/040_missed_surfaces.md new file mode 100644 index 0000000000..04bf1f656b --- /dev/null +++ b/devlog/_fin/260905_always_on_429_failover/040_missed_surfaces.md @@ -0,0 +1,148 @@ +# 040 — The two surfaces audit round 1 found missing + +Both are pre-existing coverage gaps rather than regressions this unit introduces, and both +strand a 429 that the user's requirement says must move. They become work-phases of their own. + +## 040a — Generic OAuth arm for the continuation loop + +`src/server/responses/core.ts` ~6549-6628 (the continuation/turn-retry loop) rotates API keys +and Anthropic accounts but has no generic-OAuth arm, so xAI / Cursor / Kimi / Copilot / +Antigravity / Nous continuation 429s never move. + +The fix mirrors the arm already present in the main streaming loop at ~6216, using the same +request-local state (`genericFailoverAccountId`, `genericFailovers`, +`GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST`) so the per-request bound is shared rather than +re-armed: + +```ts +if ( + response.status === 429 + && genericFailoverAccountId + && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST + && isGenericOAuthFailoverEnabled(config, route.providerName) +) { + const nextAccountId = rotateGenericOAuthAccountOn429( + config, route.providerName, genericFailoverAccountId, response.headers.get("retry-after"), + ); + if (nextAccountId) { + try { void response.body?.cancel().catch(() => {}); } catch { /* already closed */ } + try { + const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); + genericFailoverAccountId = nextAccountId; + genericFailovers += 1; + if (applyFailoverSnapshot(snapshot)) { + invalidateSameTargetRequest(); + activeAdapter = resolveAdapter( + resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire), + config.cacheRetention, + ); + sealRequestAttemptIdentity(logCtx.activeAttempt, logCtx.provider, activeAdapter.name, logCtx.accountLogLabel); + nextContinuationRecoveryKind = "oauth-account-429"; + continue; + } + } catch { /* fall through to emit continuation error below */ } + } +} +``` + +Placement: after the Anthropic arm, matching the streaming loop's order (keys, then Anthropic, +then generic). `applyFailoverSnapshot` is mandatory — it carries the Copilot origin, +Antigravity project and Kiro metadata pairing. A hand-rolled `apiKey` swap here would +reintroduce the #2841 mixed-identity bug, and `tests/generic-oauth-failover.test.ts:248` +asserts `failoverAccountSnapshot(` appears exactly 3 times — adding a 4th call site means that +count must be updated to 4 deliberately, which is the guard working as designed. + +## 040b — Anthropic arm for the sidecar hook + +`rotateSidecarProviderOn429` (~5201-5245) is shared by the web-search loop and the image +bridge. It tries the key pool, then generic OAuth. Anthropic is excluded from generic failover +by design, so an Anthropic 429 inside a web-search or image turn is terminal even with the pool +enabled. + +**Amended after audit round 2.** A first draft appended an `else if` after the generic branch. +That would have been **dead code**. The current `else` block opens with + +```ts +if (!genericFailoverAccountId || genericFailovers >= MAX || !isGenericOAuthFailoverEnabled(...)) + return null; +``` + +and Anthropic never has a `genericFailoverAccountId` — `isGenericFailoverProvider` excludes it +(`src/oauth/generic-account-failover.ts:53`). So the guard returns `null` before any later +branch is reached, and a naive "the hook mentions Anthropic" string test would still pass while +the feature stayed dead. The generic gate must therefore be **inverted into a positive +condition**, with `return null` deferred until both OAuth arms have missed: + +```ts +if (rotated) { + route.provider = rotated; +} else if ( + genericFailoverAccountId + && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST + && isGenericOAuthFailoverEnabled(config, route.providerName) +) { + const nextAccountId = rotateGenericOAuthAccountOn429( + config, route.providerName, genericFailoverAccountId, retryAfter, + ); + if (!nextAccountId) return null; + try { + const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); + genericFailoverAccountId = nextAccountId; + genericFailovers += 1; + if (!applyFailoverSnapshot(snapshot)) return null; + } catch { + return null; + } +} else if ( + anthropicPoolAccountId + && anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST +) { + const nextAccountId = rotateAnthropicAccountOn429( + config, anthropicPoolAccountId, retryAfter, anthropicSessionKey, + ); + if (!nextAccountId) return null; + try { + const accessToken = await getAnthropicPoolAccessToken(nextAccountId); + anthropicPoolAccountId = nextAccountId; + anthropicPoolFailovers += 1; + route.provider = { ...route.provider, apiKey: accessToken }; + promoteAnthropicActiveAccount(nextAccountId); + logCtx.provider = formatAnthropicProviderForLog("anthropic", nextAccountId, config); + } catch { + return null; + } +} else { + // Neither a key pool, nor a generic OAuth roster, nor an Anthropic pool could serve a + // replacement credential. The 429 is terminal for this sidecar turn. + return null; +} +``` + +The inversion is behaviour-preserving for every provider that reaches the hook today: an +API-key provider still takes the first arm, a generic OAuth provider still takes the second +with the identical three conditions, and everything else still returns `null` — just from the +trailing `else` instead of the leading guard. + +The structural test must assert **reachability**, not mention: that the Anthropic branch is not +preceded by an unconditional `return null` in the same `else` chain. A test that only greps for +the word `anthropic` in the hook body is exactly the test that would have passed against the +dead first draft. + +Deliberately NOT routed through `applyFailoverSnapshot`: that helper's contract is +snapshot-pairing for providers that carry per-account routing metadata. Anthropic carries none, +its pool has a fail-closed `local-cli` credential rule that `getAnthropicPoolAccessToken` +enforces, and the structural test at `tests/generic-oauth-failover.test.ts:243` asserts the hook +body does **not** contain `apiKey: snapshot.accessToken` — this branch never builds a snapshot, +so it does not trip that guard. The two existing Anthropic rotation sites apply the token the +same way. + +## Test additions + +- Structural: the continuation loop contains all three rotators (keys, Anthropic, generic), so a + fourth surface cannot silently ship with two of them. Same spirit as the existing sidecar + divergence test that caught this class of bug once already. +- Structural: the sidecar hook contains an Anthropic arm AND the generic gate is a positive + `else if` rather than an early-return guard, so the Anthropic arm is reachable. +- Update BOTH occurrence counts from 3 to 4: `failoverAccountSnapshot(` and + `applyFailoverSnapshot(snapshot)`. Audit round 2 confirmed + `tests/generic-oauth-failover.test.ts:248` ties the two together, so bumping only one fails. diff --git a/devlog/_fin/260905_always_on_429_failover/090_outcome.md b/devlog/_fin/260905_always_on_429_failover/090_outcome.md new file mode 100644 index 0000000000..c0d40b496f --- /dev/null +++ b/devlog/_fin/260905_always_on_429_failover/090_outcome.md @@ -0,0 +1,41 @@ +# 090 — Outcome + +Shipped as `fix(oauth): always fail over to another credential on 429`. + +## What changed + +| Surface | Before | After | +|---|---|---| +| `apiKeyPool` | presence-activated | unchanged (this was the model) | +| Generic OAuth reactive | presence, but `enabled: false` disabled it | presence only, not disableable | +| Generic OAuth proactive | shared the same predicate | own predicate, `enabled: false` still refuses | +| Anthropic reactive | dead unless `anthropicAccountPool.enabled` | presence-activated, flag-independent | +| Anthropic proactive | behind the flag | unchanged, still behind the flag | +| Continuation loop | keys + Anthropic only | keys + Anthropic + generic OAuth | +| Sidecar `on429` hook | keys + generic OAuth only | keys + generic OAuth + Anthropic | + +## Verification + +No repository-wide local suite (standing instruction). `bun run typecheck` clean; eight focused +files green, 232 pass / 0 fail. Receipt: +`.codexclaw/evidence/01a06d31-a387-7320-a093-dfe3ece724fe/test-receipt.json` (97 pass across the +five failover-critical files). Repository-wide validation is delegated to CI on the exact PR head. + +One failure appears when `management-provider-validation.test.ts` runs in the same invocation as +the pool tests. It is pre-existing cross-file interference, proven by stashing `src` and `tests` +and reproducing it identically on the unmodified tree; the file passes 97/97 alone. + +## Review history + +Three audit rounds, same reviewer (`xai/grok-4.6`), recorded in `001_audit_round_1.md`. Round 1 +failed with four blockers — two of them surfaces the plan had missed entirely. Round 2 failed +with one: the proposed sidecar Anthropic arm sat behind an early `return null` and would have +been dead code that a naive string test still passed. Round 3 passed. Every finding was folded +in; none was rebutted. + +## Known follow-up + +`gui/src/i18n/en.ts:1818` `anthropicPool.disabledDesc` ("Uses only the active Claude account") +is now stale — with the pool off a 429 does move. `gui/` was kept out deliberately: an +`AGENTS.md` screenshot gate plus a ten-locale copy pass does not belong in a routing fix. Owed +as its own change. diff --git a/src/oauth/anthropic-routing.ts b/src/oauth/anthropic-routing.ts index 7930a5b185..f2c1edc342 100644 --- a/src/oauth/anthropic-routing.ts +++ b/src/oauth/anthropic-routing.ts @@ -236,6 +236,36 @@ export function getEligibleAnthropicAccounts(now = Date.now()): string[] { .map(account => account.id); } +/** + * Whether a 429 has somewhere to go: two or more accounts that could serve traffic if asked. + * + * Reactive failover is a safety net, not a routing policy. It runs only AFTER upstream refused, + * it cannot spread load across a healthy session, and it cannot fire at all unless the operator + * deliberately logged in twice. So it activates on presence, exactly like an `apiKeyPool` of two + * keys does in `providers/key-failover.ts` -- and unlike the PROACTIVE pool (affinity, + * quota-ranked new-session picks, `autoSwitchThreshold`, `strategy`), which changes which + * account serves a healthy request and therefore stays behind `anthropicAccountPool.enabled`. + * + * Cooldowns are deliberately ignored here. They are transient and per-request, while this + * answers the durable question "did the operator store a second account". Counting a cooled + * account as absent would switch the feature off for the length of the cooldown -- precisely + * when it is needed. + * + * `isPoolCredentialUsable` is still applied, so the fail-closed background `local-cli` rule + * holds: an expired background slot is not a quorum and cannot be adopted. + */ +export function hasAnthropicFailoverQuorum(now = Date.now()): boolean { + const set = getAccountSet(PROVIDER); + if (!set) return false; + let usable = 0; + for (const account of set.accounts) { + if (account.needsReauth === true) continue; + if (!isPoolCredentialUsable(account.id, now)) continue; + if (++usable >= 2) return true; + } + return false; +} + /** Earliest remaining cooldown among cooled Anthropic accounts, for client Retry-After. */ export function getAnthropicPoolRetryAfterSeconds(now = Date.now()): number | null { const set = getAccountSet(PROVIDER); @@ -573,7 +603,13 @@ export function rotateAnthropicAccountOn429( sessionKey?: string | null, now = Date.now(), ): string | null { - if (!isAnthropicAccountPoolEnabled(config)) return null; + // Reactive 429 failover is NOT gated on the pool flag. That flag buys PROACTIVE routing -- + // session affinity, quota-ranked new-session selection, autoSwitchThreshold, strategy -- all + // of which move a HEALTHY request and stay opt-in. Rotating away from an account upstream has + // just rate-limited is a different thing: it only ever runs after a refusal, and stranding a + // 429 while a second logged-in account sits idle is a defect, not a configuration choice. + // Presence is the activation rule, the same one an apiKeyPool of two keys already uses. + if (!isAnthropicAccountPoolEnabled(config) && !hasAnthropicFailoverQuorum(now)) return null; const parsedRetry = parseRetryAfterMs(retryAfterHeader, now); const cooldownMs = parsedRetry ?? DEFAULT_COOLDOWN_MS; diff --git a/src/oauth/generic-account-failover.ts b/src/oauth/generic-account-failover.ts index 785b9f7bfa..9a7a2566f9 100644 --- a/src/oauth/generic-account-failover.ts +++ b/src/oauth/generic-account-failover.ts @@ -148,18 +148,21 @@ export function hasFailoverAccountQuorum(providerName: string, now = Date.now()) } /** - * Whether generic rotation is active for this provider. + * Whether REACTIVE 429 rotation is active for this provider. * - * Precedence, most specific first: + * Presence is the only rule: two or more eligible stored accounts. The + * `oauthAccountFailover.enabled` booleans no longer suppress it. * - * 1. `providers..oauthAccountFailover.enabled` — an operator may accept rotation on one - * provider and refuse it on another, because provider terms differ. - * 2. `oauthAccountFailover.enabled` — the global switch. Anyone who already wrote `false` keeps - * strict single-account behaviour across this change. - * 3. Presence: 2 or more eligible stored accounts (#2568d, owner decision). + * That is a deliberate narrowing of #2568d. Rotation here runs only after upstream has already + * refused the request, so the choice the old knob offered was between "retry on the second + * account you deliberately logged in" and "return a 429 while that account sits idle". The + * second is a defect, not a preference — and an operator who does not want rotation expresses + * that by not storing a second account, exactly as they do for `apiKeyPool`. * - * Only an explicit boolean overrides presence. A malformed value falls through instead of - * throwing, because a typo in a knob must not take a provider out of service. + * The knob is not gone. It still governs {@link isProactivePreferenceEnabled}, which decides + * whether a HEALTHY request may be steered to a different account before dispatch — a real + * behavioural choice that remains refusable — and it still carries `strategy` and + * `autoSwitchThreshold`. */ export function isGenericOAuthFailoverEnabled( config: OcxConfig, @@ -168,10 +171,23 @@ export function isGenericOAuthFailoverEnabled( ): boolean { const provider = config.providers?.[providerName]; if (!provider || !isGenericFailoverProvider(providerName, provider)) return false; - 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); +} + +/** + * Whether the pre-dispatch account PREFERENCE may run for this provider. + * + * Unlike reactive rotation, this moves a request that upstream has not refused, so it stays + * refusable: an explicit `false` — per provider first, then global — turns it off. `true` adds + * nothing over presence, so only `false` is honoured; that keeps the predicate identical to the + * old behaviour for every operator who never wrote the key, and a malformed value falls through + * rather than taking a provider out of service. + */ +function isProactivePreferenceEnabled(config: OcxConfig, providerName: string, now: number): boolean { + const provider = config.providers?.[providerName]; + if (!provider || !isGenericFailoverProvider(providerName, provider)) return false; + if (provider.oauthAccountFailover?.enabled === false) return false; + if (config.oauthAccountFailover?.enabled === false) return false; return hasFailoverAccountQuorum(providerName, now); } @@ -265,7 +281,9 @@ export function preferredInitialAccount( providerName: string, now = Date.now(), ): string | null { - if (!isGenericOAuthFailoverEnabled(config, providerName)) return null; + // The PROACTIVE predicate, not the reactive one: this steers a request upstream has not + // refused, so `oauthAccountFailover.enabled: false` must still be able to refuse it. + if (!isProactivePreferenceEnabled(config, providerName, now)) return null; // This runs on the initial resolution of EVERY request, and `loadAuthStore` has no // cache: each call chmods the config dir, chmods the secret, reads the whole file and // normalizes it (store.ts:136-151). So the store is consulted at most ONCE here, behind diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index d0b3f94572..cb1e00678e 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -122,6 +122,7 @@ import { getAnthropicPoolAccessToken, getAnthropicPoolRetryAfterSeconds, isAnthropicAccountPoolEnabled, + hasAnthropicFailoverQuorum, promoteAnthropicActiveAccount, resolveAnthropicAccountForSession, rotateAnthropicAccountOn429, @@ -3477,6 +3478,14 @@ async function handleResponsesInner( if (isGenericFailoverProvider(route.providerName, route.provider)) { genericFailoverAccountId = resolved.accountId; } + // Anthropic is excluded from isGenericFailoverProvider -- its own pool owns affinity and + // a fail-closed local-cli credential rule -- so without this stamp its identity is + // dropped whenever the pool flag is off, and a later 429 has no account to cool. Reactive + // failover needs only the id: no affinity bind, no promotion, no quota-ranked pick. Those + // are proactive and stay behind anthropicAccountPool.enabled. + if (route.providerName === "anthropic" && hasAnthropicFailoverQuorum()) { + anthropicPoolAccountId = resolved.accountId; + } // Captured beside the account it fences, so the two can never disagree. if (hasPassiveAccountQuota(route.providerName)) { passiveQuotaWriterGeneration = captureConfigGeneration(); @@ -5207,12 +5216,15 @@ async function handleResponsesInner( }); if (rotated) { route.provider = rotated; - } else { - if ( - !genericFailoverAccountId - || genericFailovers >= GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST - || !isGenericOAuthFailoverEnabled(config, route.providerName) - ) return null; + } else if ( + // A POSITIVE gate, not an early return. An early `return null` here made every later arm + // unreachable: Anthropic never has a genericFailoverAccountId (isGenericFailoverProvider + // excludes it), so its sidecar 429s died on this guard before the Anthropic arm below + // could ever be considered. + genericFailoverAccountId + && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST + && isGenericOAuthFailoverEnabled(config, route.providerName) + ) { const nextAccountId = rotateGenericOAuthAccountOn429( config, route.providerName, @@ -5228,6 +5240,39 @@ async function handleResponsesInner( } catch { return null; } + } else if ( + // Anthropic's pool is excluded from generic failover, so without this arm a 429 inside a + // web-search or image-bridge turn was terminal even with the pool fully enabled -- while + // the very same 429 on the main response path rotated. + anthropicPoolAccountId + && anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST + ) { + const nextAccountId = rotateAnthropicAccountOn429( + config, + anthropicPoolAccountId, + retryAfter, + anthropicSessionKey, + ); + if (!nextAccountId) return null; + try { + // Deliberately NOT applyFailoverSnapshot: that helper exists to pair per-account routing + // metadata (Copilot origin, Antigravity project, Kiro context) with its bearer. Anthropic + // carries none, and getAnthropicPoolAccessToken is what enforces its fail-closed + // local-cli credential rule. Both existing Anthropic rotation sites apply the token the + // same way. + const accessToken = await getAnthropicPoolAccessToken(nextAccountId); + anthropicPoolAccountId = nextAccountId; + anthropicPoolFailovers += 1; + route.provider = { ...route.provider, apiKey: accessToken }; + promoteAnthropicActiveAccount(nextAccountId); + logCtx.provider = formatAnthropicProviderForLog("anthropic", nextAccountId, config); + } catch { + return null; + } + } else { + // No key pool, no generic OAuth roster, no Anthropic pool could produce a replacement + // credential. The 429 is terminal for this sidecar turn. + return null; } const rotatedAdapter = resolveAdapter( resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire), @@ -6171,7 +6216,6 @@ async function handleResponsesInner( while ( upstreamResponse.status === 429 && anthropicPoolAccountId - && isAnthropicAccountPoolEnabled(config) && anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST ) { const nextAccountId = rotateAnthropicAccountOn429( @@ -6582,7 +6626,6 @@ async function handleResponsesInner( if ( response.status === 429 && anthropicPoolAccountId - && isAnthropicAccountPoolEnabled(config) && anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST ) { const nextAccountId = rotateAnthropicAccountOn429( @@ -6613,6 +6656,48 @@ async function handleResponsesInner( } } } + // Generic OAuth rotation for the continuation loop. The streaming loop grew this arm with + // #2568 and this one did not, so an xAI/Cursor/Kimi/Copilot/Antigravity/Nous continuation + // 429 stayed terminal even with failover fully active -- the same class of divergence the + // two sidecars already produced once. Request-local state is shared with the other arms so + // the per-request bound cannot be silently re-armed by reaching a different loop. + if ( + response.status === 429 + && genericFailoverAccountId + && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST + && isGenericOAuthFailoverEnabled(config, route.providerName) + ) { + const nextAccountId = rotateGenericOAuthAccountOn429( + config, + route.providerName, + genericFailoverAccountId, + response.headers.get("retry-after"), + ); + if (nextAccountId) { + try { void response.body?.cancel().catch(() => {}); } catch { /* already closed */ } + try { + // The FULL snapshot through the shared helper, never 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 credential with another's + // routing data. + const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); + genericFailoverAccountId = nextAccountId; + genericFailovers += 1; + if (applyFailoverSnapshot(snapshot)) { + invalidateSameTargetRequest(); + activeAdapter = resolveAdapter( + resolveWireProtocolOverride(route.providerName, route.modelId, route.provider, inboundWire), + config.cacheRetention, + ); + sealRequestAttemptIdentity(logCtx.activeAttempt, logCtx.provider, activeAdapter.name, logCtx.accountLogLabel); + nextContinuationRecoveryKind = "oauth-account-429"; + continue; + } + } catch { + // fall through to emit continuation error below + } + } + } if (shouldAttemptImageTierRetry({ status: response.status, adapterName: activeAdapter.name, diff --git a/src/types/config.ts b/src/types/config.ts index a3a3c33932..fdbec5ba25 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -760,9 +760,14 @@ export interface OcxConfig { */ maxUpstreamBodyBytes?: number; /** - * Opt-in Anthropic OAuth account pool (#294). Default OFF. - * Failover on 429 + sticky affinity; new sessions may pick lowest known 5h usage. + * Opt-in Anthropic OAuth PROACTIVE routing (#294). Default OFF. + * Sticky session affinity; new sessions may pick lowest known 5h usage. * Experimental — see docs and GUI warning before enabling. + * + * Reactive 429 failover is NOT gated here. It activates on account presence, like every + * other multi-credential provider, and cannot be switched off: rotating away from an account + * upstream has just rate-limited only ever runs after a refusal, so stranding it while a + * second logged-in account sits idle is a defect rather than a configuration choice. */ anthropicAccountPool?: { enabled?: boolean; @@ -776,17 +781,17 @@ export interface OcxConfig { quotaWindow?: OcxAccountPoolQuotaWindow; }; /** - * Generic OAuth multi-account 429 failover (#2568). Presence-driven by default. + * Generic OAuth multi-account PROACTIVE account preference (#2568, #695). * - * Rotates to another logged-in account of the SAME provider when one is rate-limited, for - * OAuth providers that have no pool of their own — xAI, Cursor, Kimi, GitHub Copilot, - * Antigravity, Nous. The Codex pool and the Anthropic pool own their own rotation and are - * excluded; this setting changes neither. + * Reactive 429 rotation — moving to another logged-in account of the SAME provider when one + * is rate-limited — is presence-driven and NOT configurable here. It activates whenever a + * provider has 2 or more eligible stored accounts, the same consent rule an `apiKeyPool` of + * two keys already applies, and a single account remains a strict no-op. * - * With the key absent, rotation activates when a provider has 2 or more eligible stored - * accounts — the same consent rule API-key pools already apply to a 2+ key pool (#2568d). A - * single account is a strict no-op. Set `false` to keep strict single-account behaviour; - * `providers..oauthAccountFailover` overrides this per provider. + * What `enabled: false` still refuses is the PRE-DISPATCH preference: steering a request + * upstream has not refused toward the account with more known headroom. That moves a healthy + * request, so it stays a real choice. `providers..oauthAccountFailover` overrides this + * per provider, and only `false` is meaningful — `true` adds nothing over presence. */ oauthAccountFailover?: { enabled?: boolean; diff --git a/src/types/provider.ts b/src/types/provider.ts index a3a4dd4c10..459634fd16 100644 --- a/src/types/provider.ts +++ b/src/types/provider.ts @@ -424,11 +424,13 @@ export interface OcxProviderConfig { */ authMode?: "key" | "forward" | "oauth" | "local"; /** - * Per-provider override for generic OAuth multi-account 429 failover (#2568). + * Per-provider override for the generic OAuth PROACTIVE account preference (#2568, #695). * - * Rotation is presence-driven by default — 2+ logged-in accounts activate it — so this exists - * for the operator who accepts rotation on one provider and refuses it on another. An explicit - * boolean here beats the global `oauthAccountFailover` and beats presence. + * Reactive 429 rotation is presence-driven and cannot be refused here — 2+ logged-in accounts + * activate it, and a 429 with an idle second account is a defect rather than a preference. + * What an explicit `false` still refuses is the pre-dispatch preference that steers a HEALTHY + * request toward the account with more known headroom. It beats the global + * `oauthAccountFailover`; only `false` is meaningful, since `true` adds nothing over presence. */ oauthAccountFailover?: { enabled?: boolean; diff --git a/tests/adapter-event-oauth-failover.test.ts b/tests/adapter-event-oauth-failover.test.ts index 37221304c0..bb3b4c37ea 100644 --- a/tests/adapter-event-oauth-failover.test.ts +++ b/tests/adapter-event-oauth-failover.test.ts @@ -126,16 +126,21 @@ describe("#2568 adapter-event OAuth failover", () => { expect(body).toContain("rate_limit_exceeded"); }); - test("an explicit opt-out keeps single-account behaviour with two accounts stored", async () => { - // Presence is consent, but only when the operator has not already said no. Someone who wrote - // `enabled: false` gets the pre-#2568d behaviour unchanged. + test("an explicit opt-out no longer strands a 429 when a second account is stored", async () => { + // Reversed deliberately. `enabled: false` used to keep pre-#2568d single-account behaviour + // on a 429; it now governs only the proactive pre-dispatch preference. Stranding a rate + // limit while a second logged-in account sits idle is a defect rather than a preference, + // and the operator who wants one account expresses that by storing one account. await seedAccounts(2); - attempts = [[{ type: "error", message: "Cursor rate limit exceeded: resource_exhausted" }]]; + attempts = [ + [{ type: "error", message: "Cursor rate limit exceeded: resource_exhausted" }], + [{ type: "text", text: "ok" }], + ]; const body = await (await handleResponses(request(true), config(false), { model: "", provider: "" })).text(); - expect(attemptKeys).toEqual(["cursor-access-1"]); - expect(body).toContain("rate_limit_exceeded"); + expect(attemptKeys).toEqual(["cursor-access-1", "cursor-access-0"]); + expect(body).not.toContain("rate_limit_exceeded"); }); test("the first delta reaches the client before the turn completes", async () => { diff --git a/tests/always-on-429-failover.test.ts b/tests/always-on-429-failover.test.ts new file mode 100644 index 0000000000..1ab40da0ad --- /dev/null +++ b/tests/always-on-429-failover.test.ts @@ -0,0 +1,142 @@ +/** + * 429 credential failover is a safety net, not a routing policy. + * + * Three rotators existed with three different activation rules: `apiKeyPool` rotated on presence, + * generic OAuth rotated on presence but could be switched off, and Anthropic rotated only behind + * `anthropicAccountPool.enabled` -- which defaults absent. So an operator with two Claude accounts + * logged in and a stock config got a hard 429 with the second account sitting idle. + * + * These tests pin the separation that resolves it: REACTIVE rotation (after upstream refused) + * activates on presence and cannot be disabled, while PROACTIVE routing (affinity, quota-ranked + * new-session selection, strategy, autoSwitchThreshold) stays behind the opt-in flag. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + clearAnthropicAccountPoolState, + getEligibleAnthropicAccounts, + hasAnthropicFailoverQuorum, + isAnthropicAccountPoolEnabled, + resolveAnthropicAccountForSession, + rotateAnthropicAccountOn429, +} from "../src/oauth/anthropic-routing"; +import { clearPoolRotationState } from "../src/codex/pool-rotation"; +import { getAccountSet, saveCredential, setActiveAccount } from "../src/oauth/store"; +import { clearAccountQuotaCache } from "../src/providers/quota"; +import type { OcxConfig } from "../src/types"; +import { removeTreeWithRetry } from "./helpers/remove-tree"; + +const originalHome = process.env.OPENCODEX_HOME; +let home: string; + +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), "ocx-always-on-429-")); + process.env.OPENCODEX_HOME = home; + clearAnthropicAccountPoolState(); + clearPoolRotationState(); + clearAccountQuotaCache("anthropic"); +}); + +afterEach(() => { + clearAnthropicAccountPoolState(); + clearPoolRotationState(); + clearAccountQuotaCache("anthropic"); + if (originalHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = originalHome; + removeTreeWithRetry(home); +}); + +/** No `anthropicAccountPool` key at all: what a stock install that never opted in looks like. */ +function poolAbsent(): OcxConfig { + return { + port: 0, + defaultProvider: "anthropic", + providers: { + anthropic: { adapter: "anthropic", baseUrl: "https://api.anthropic.com", authMode: "oauth" }, + }, + } as OcxConfig; +} + +/** An operator who explicitly wrote `false` -- the strongest form of "I did not opt in". */ +function poolDisabled(): OcxConfig { + return { ...poolAbsent(), anthropicAccountPool: { enabled: false } } as OcxConfig; +} + +async function seedAccounts(count: number): Promise { + for (let i = 0; i < count; i++) { + await saveCredential("anthropic", { + access: `access-${i}`, + refresh: `refresh-${i}`, + expires: Date.now() + 3_600_000, + accountId: `uuid-${i}`, + email: `user${i}@example.test`, + } as never); + } + const set = getAccountSet("anthropic")!; + const ids = set.accounts.map(account => account.id); + // saveCredential activates the last account appended; pin the first for a predictable active. + if (ids[0]) await setActiveAccount("anthropic", ids[0]); + return ids; +} + +describe("Anthropic reactive 429 failover without the pool flag", () => { + test("a 429 rotates to the second account with the pool key absent", async () => { + const ids = await seedAccounts(2); + expect(isAnthropicAccountPoolEnabled(poolAbsent())).toBe(false); + expect(hasAnthropicFailoverQuorum()).toBe(true); + + expect(rotateAnthropicAccountOn429(poolAbsent(), ids[0]!, null)).toBe(ids[1]); + // The account that actually 429'd is the one cooled -- not whichever is active later. + expect(getEligibleAnthropicAccounts()).toEqual([ids[1]!]); + }); + + test("an explicit enabled:false does not strand the 429 either", async () => { + // The flag buys proactive routing. Refusing that is a real choice; refusing to retry a + // rate-limited request on an account the operator deliberately logged in is not. + const ids = await seedAccounts(2); + expect(rotateAnthropicAccountOn429(poolDisabled(), ids[0]!, null)).toBe(ids[1]); + }); + + test("a single account is still a strict no-op", async () => { + // Rotating to itself would replay the same 429 on the same credential, and cooling the only + // account would take the provider out of service for nothing. + const ids = await seedAccounts(1); + expect(hasAnthropicFailoverQuorum()).toBe(false); + expect(rotateAnthropicAccountOn429(poolAbsent(), ids[0]!, null)).toBeNull(); + }); + + test("Retry-After from upstream still drives the cooldown", async () => { + const ids = await seedAccounts(2); + expect(rotateAnthropicAccountOn429(poolAbsent(), ids[0]!, "600")).toBe(ids[1]); + expect(getEligibleAnthropicAccounts()).not.toContain(ids[0]!); + }); + + test("when every account is cooled the 429 is surfaced rather than looped", async () => { + const ids = await seedAccounts(2); + expect(rotateAnthropicAccountOn429(poolAbsent(), ids[0]!, null)).toBe(ids[1]); + expect(rotateAnthropicAccountOn429(poolAbsent(), ids[1]!, null)).toBeNull(); + }); +}); + +describe("proactive Anthropic routing stays opt-in", () => { + test("with the pool off, selection still returns the active account and reports pool-disabled", async () => { + // The whole point of the split: reactive rotation turning on must not drag session affinity + // or quota-ranked selection on with it. An operator who never opted in still gets exactly + // one account per session -- they just stop getting a hard 429 when it is spent. + const ids = await seedAccounts(2); + const selection = resolveAnthropicAccountForSession("session-1", poolAbsent()); + expect(selection.accountId).toBe(ids[0]!); + expect(selection.reason).toBe("pool-disabled"); + }); + + test("repeated resolves never drift to the second account", async () => { + const ids = await seedAccounts(2); + const picks = Array.from( + { length: 5 }, + () => resolveAnthropicAccountForSession(null, poolDisabled()).accountId, + ); + expect(picks.every(id => id === ids[0]!)).toBe(true); + }); +}); diff --git a/tests/generic-oauth-failover.test.ts b/tests/generic-oauth-failover.test.ts index 64edf30754..7bbfc57951 100644 --- a/tests/generic-oauth-failover.test.ts +++ b/tests/generic-oauth-failover.test.ts @@ -9,9 +9,11 @@ import { hasFailoverAccountQuorum, isGenericFailoverProvider, isGenericOAuthFailoverEnabled, + preferredInitialAccount, rotateGenericOAuthAccountOn429, } from "../src/oauth/generic-account-failover"; import { getAccountSet, markAccountNeedsReauth, saveCredential } from "../src/oauth/store"; +import { clearAccountQuotaCache, setCachedProviderAccountQuotaForTests } from "../src/providers/quota"; import { resolveCopilotApiBaseUrl } from "../src/oauth/github-copilot"; import { resolveProviderTransport } from "../src/providers/xai-transport"; import type { OcxConfig, OcxProviderConfig } from "../src/types"; @@ -28,6 +30,7 @@ beforeEach(() => { afterEach(() => { clearGenericFailoverHealth(); + clearAccountQuotaCache("xai"); if (originalHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = originalHome; removeTreeWithRetry(home); @@ -77,9 +80,15 @@ describe("#2568 generic OAuth account failover", () => { expect(eligibleFailoverAccounts("xai")).toEqual([second!]); }); - test("an explicit knob still wins over presence, in both directions", async () => { + test("an explicit knob no longer disables REACTIVE rotation", async () => { + // This assertion is deliberately the reverse of what it was under #2568d. The knob used to + // suppress rotation entirely; it now governs only the proactive pre-dispatch preference. + // The choice it offered here was between retrying on the second account the operator + // deliberately logged in and returning a 429 while that account sat idle -- and the second + // is a defect, not a preference. Refusing rotation is expressed by not storing a second + // account, exactly as it is for an apiKeyPool. const ids = await seed(2); - expect(rotateGenericOAuthAccountOn429(config(false), "xai", ids[0]!, null)).toBeNull(); + expect(rotateGenericOAuthAccountOn429(config(false), "xai", ids[0]!, null)).toBe(ids[1]); clearGenericFailoverHealth(); expect(rotateGenericOAuthAccountOn429(config(true), "xai", ids[0]!, null)).toBe(ids[1]); }); @@ -104,13 +113,27 @@ describe("#2568 generic OAuth account failover", () => { expect(rotateGenericOAuthAccountOn429(config(), "xai", "not-a-real-account", null)).toBe(ids[0]); }); - test("a per-provider override beats the global switch", async () => { - // Provider terms differ, so an operator may accept rotation on one provider and refuse it on - // another. The narrower setting is the one that means something. + test("neither switch can turn REACTIVE rotation off, in either direction", async () => { + // Also reversed from #2568d. Reactive rotation is presence-only now, so a per-provider + // false, a global false, and any combination of the two all still rotate. What the override + // still buys is the PROACTIVE preference, covered by its own test below. const ids = await seed(2); - expect(isGenericOAuthFailoverEnabled(config(true, false), "xai")).toBe(false); + expect(isGenericOAuthFailoverEnabled(config(true, false), "xai")).toBe(true); expect(isGenericOAuthFailoverEnabled(config(false, true), "xai")).toBe(true); - expect(rotateGenericOAuthAccountOn429(config(true, false), "xai", ids[0]!, null)).toBeNull(); + expect(isGenericOAuthFailoverEnabled(config(false, false), "xai")).toBe(true); + expect(rotateGenericOAuthAccountOn429(config(true, false), "xai", ids[0]!, null)).toBe(ids[1]); + }); + + test("the knob still refuses the PROACTIVE pre-dispatch preference", async () => { + // The half of the old contract that survives: steering a request upstream has NOT refused + // is a real behavioural choice, so an explicit false must still be able to decline it. + // Without headroom evidence the preference is a no-op anyway, so this pins the refusal + // rather than the ranking. + const ids = await seed(2); + setCachedProviderAccountQuotaForTests("xai", ids[0]!, { fiveHourPercent: 99 }); + setCachedProviderAccountQuotaForTests("xai", ids[1]!, { fiveHourPercent: 1 }); + expect(preferredInitialAccount(config(false), "xai")).toBeNull(); + expect(preferredInitialAccount(config(true, false), "xai")).toBeNull(); }); test("a second account flagged for reauth is not a quorum", async () => { @@ -229,9 +252,28 @@ describe("sidecar on429 wiring", () => { // The OAuth branch is gated on all three of: an account this request actually used, the // per-request bound, and the activation predicate. Dropping the bound lets a short // Retry-After spin; dropping the account binding lets a rotation cool an innocent account. - expect(body).toContain("!genericFailoverAccountId"); - expect(body).toContain("genericFailovers >= GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST"); - expect(body).toContain("!isGenericOAuthFailoverEnabled(config, route.providerName)"); + // The gate is a POSITIVE else-if, not an early return: an early bare return here made the + // Anthropic arm below unreachable, because Anthropic never has a genericFailoverAccountId. + expect(body).toContain("genericFailoverAccountId"); + expect(body).toContain("genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST"); + expect(body).toContain("isGenericOAuthFailoverEnabled(config, route.providerName)"); + + // Anthropic's pool is excluded from generic failover, so it needs its own arm here or a 429 + // inside a web-search/image turn is terminal while the same 429 on the main path rotates. + const anthropic = body.indexOf("rotateAnthropicAccountOn429("); + expect(anthropic).toBeGreaterThan(oauth); + + // REACHABILITY, not mention. The first draft of this arm sat behind an unconditional early + // return and was dead code that a grep for "anthropic" would have happily passed. Every gate + // ahead of it must therefore be a positive `else if`; a plain `else` block would swallow the + // request and never fall through, which is precisely how the dead version was shaped. + // (A `return null` INSIDE an arm is fine — that is a rotation that genuinely found no + // candidate. What must not exist is a gate that returns before the arm is considered.) + const chainStart = body.indexOf("if (rotated) {"); + expect(chainStart).toBeGreaterThan(-1); + expect(body.slice(chainStart, anthropic)).not.toContain("} else {"); + // ...and the chain still ends in a terminal else, so an unrotatable 429 is not swallowed. + expect(body.slice(anthropic)).toContain("} else {"); // The FULL snapshot, not a bare bearer, and applied through the shared helper rather than // inline. Inlining is what produced the original defect: three sites each swapped `apiKey` @@ -247,7 +289,11 @@ describe("sidecar on429 wiring", () => { // bearer by hand would reintroduce the mixed-identity bug this helper exists to prevent. const snapshotUses = coreSource.match(/failoverAccountSnapshot\(/g) ?? []; const helperUses = coreSource.match(/applyFailoverSnapshot\(snapshot\)/g) ?? []; - expect(snapshotUses.length).toBe(3); + // Four since the continuation loop gained its own generic-OAuth arm: the streaming loop grew + // one with #2568 and the continuation loop did not, so an xAI/Cursor continuation 429 stayed + // terminal. Bumping this count is the deliberate act of admitting a fourth rotation site -- + // which is exactly why the guard is a count and not a floor. + expect(snapshotUses.length).toBe(4); expect(helperUses.length).toBe(snapshotUses.length); // The bearer is written in exactly one place — inside the helper. Any other occurrence is a // rotation site that skipped the pairing rules.