From 8d0620d80cc4c35727d67ef4153ce59fca6c931f Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 01:22:18 +0900 Subject: [PATCH 1/5] docs(devlog): plan always-on 429 credential failover --- .../000_research_inventory.md | 78 +++++++++++++++ .../010_anthropic_reactive_split.md | 99 +++++++++++++++++++ .../020_generic_oauth_non_disableable.md | 86 ++++++++++++++++ .../030_types_docs_surface.md | 56 +++++++++++ 4 files changed, 319 insertions(+) create mode 100644 devlog/_plan/260905_always_on_429_failover/000_research_inventory.md create mode 100644 devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md create mode 100644 devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md create mode 100644 devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md diff --git a/devlog/_plan/260905_always_on_429_failover/000_research_inventory.md b/devlog/_plan/260905_always_on_429_failover/000_research_inventory.md new file mode 100644 index 0000000000..3a4d9934b4 --- /dev/null +++ b/devlog/_plan/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/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md b/devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md new file mode 100644 index 0000000000..72252567a3 --- /dev/null +++ b/devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md @@ -0,0 +1,99 @@ +# 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` (:3395-3420) + +The account identity must be captured even when the pool is off, or the rotation loops have +nothing to cool. Restructure the branch: + +```ts +if (route.providerName === "anthropic" && isAnthropicAccountPoolEnabled(config)) { + ... existing proactive selection, unchanged ... +} else if (route.providerName === "anthropic" && route.provider.authMode === "oauth" + && hasAnthropicFailoverQuorum()) { + // Pool off: keep the ordinary active-account resolution, but REMEMBER which account + // served the request so a later 429 cools that one. No affinity bind, no promotion, + // no quota-ranked pick -- those are proactive and remain opt-in. + const snapshot = await getValidAccessTokenSnapshot("anthropic"); + anthropicPoolAccountId = snapshot.accountId; + route.provider = { ...route.provider, apiKey: snapshot.accessToken }; + logCtx.provider = formatAnthropicProviderForLog("anthropic", snapshot.accountId, config); +} +``` + +Note the ordering constraint: the `else` arm of the outer `if (route.provider.authMode === "oauth")` +block currently handles every non-Anthropic-pool OAuth provider through the generic path. +Anthropic is excluded from `isGenericFailoverProvider`, so it reaches that arm and resolves +the active account normally. The minimal edit is therefore to capture `anthropicPoolAccountId` +from `resolved.accountId` in that shared arm when the provider is `anthropic` and the quorum +holds, rather than duplicating a resolution. Prefer that: one resolution, one stamp. + +## 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/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md b/devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md new file mode 100644 index 0000000000..b109fe6c2f --- /dev/null +++ b/devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md @@ -0,0 +1,86 @@ +# 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 (`tests/generic-oauth-failover.test.ts`) + +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. +4. `enabled: false` -> `preferredInitialAccount` returns `null` even with headroom evidence + (the proactive refusal is preserved). +5. Existing presence-default-on tests continue to pass unchanged. diff --git a/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md b/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md new file mode 100644 index 0000000000..d343d977d7 --- /dev/null +++ b/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md @@ -0,0 +1,56 @@ +# 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 + +`genericPoolSettingsDto` reports `inert: true` and the GUI's Anthropic pool settings panel +describes the opt-in pool. Neither lies after this change — the pool flag still means what the +panel says it means for proactive routing. A copy pass explaining "429 failover always on" is +desirable but is GUI-surface work; per `AGENTS.md` a PR touching `gui` requires a screenshot +in the description. Keeping `gui/` out of this PR keeps the change reviewable as a routing +fix. Recorded here as a deliberate deferral, not an oversight. + +## 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/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 +``` + +Repository-wide validation is delegated to GitHub Actions on the exact PR head SHA, which must +be observed green before the admin merge. From a21bacb099b51344f048486df00eb298c4ae4ecc Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 01:48:00 +0900 Subject: [PATCH 2/5] docs(devlog): fold audit round 1 into the 429 failover plan --- .../001_audit_round_1.md | 63 ++++++++++++ .../010_anthropic_reactive_split.md | 41 ++++---- .../020_generic_oauth_non_disableable.md | 32 +++++- .../030_types_docs_surface.md | 29 ++++-- .../040_missed_surfaces.md | 99 +++++++++++++++++++ 5 files changed, 232 insertions(+), 32 deletions(-) create mode 100644 devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md create mode 100644 devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md diff --git a/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md b/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md new file mode 100644 index 0000000000..050555c21c --- /dev/null +++ b/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md @@ -0,0 +1,63 @@ +# 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. + +## 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/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md b/devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md index 72252567a3..5183731af7 100644 --- a/devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md +++ b/devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md @@ -42,32 +42,31 @@ eligible non-excluded account — a deterministic, evidence-optional pick. No ne `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` (:3395-3420) +## Change 2 — `src/server/responses/core.ts` (:3475-3480) -The account identity must be captured even when the pool is off, or the rotation loops have -nothing to cool. Restructure the branch: +**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 (route.providerName === "anthropic" && isAnthropicAccountPoolEnabled(config)) { - ... existing proactive selection, unchanged ... -} else if (route.providerName === "anthropic" && route.provider.authMode === "oauth" - && hasAnthropicFailoverQuorum()) { - // Pool off: keep the ordinary active-account resolution, but REMEMBER which account - // served the request so a later 429 cools that one. No affinity bind, no promotion, - // no quota-ranked pick -- those are proactive and remain opt-in. - const snapshot = await getValidAccessTokenSnapshot("anthropic"); - anthropicPoolAccountId = snapshot.accountId; - route.provider = { ...route.provider, apiKey: snapshot.accessToken }; - logCtx.provider = formatAnthropicProviderForLog("anthropic", snapshot.accountId, config); -} + 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; ++} ``` -Note the ordering constraint: the `else` arm of the outer `if (route.provider.authMode === "oauth")` -block currently handles every non-Anthropic-pool OAuth provider through the generic path. -Anthropic is excluded from `isGenericFailoverProvider`, so it reaches that arm and resolves -the active account normally. The minimal edit is therefore to capture `anthropicPoolAccountId` -from `resolved.accountId` in that shared arm when the provider is `anthropic` and the quorum -holds, rather than duplicating a resolution. Prefer that: one resolution, one stamp. +One resolution, one stamp, no new credential read. ## Change 3 — the two rotation loops (:6173, :6584) diff --git a/devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md b/devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md index b109fe6c2f..715956155c 100644 --- a/devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md +++ b/devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md @@ -76,11 +76,33 @@ monotone with the old behaviour for every operator who never wrote the key. no edit — the predicate they call simply became presence-only. `:3422` guards `preferredInitialAccount`, which now self-gates on the proactive predicate. -## Tests (`tests/generic-oauth-failover.test.ts`) +## 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. -4. `enabled: false` -> `preferredInitialAccount` returns `null` even with headroom evidence - (the proactive refusal is preserved). -5. Existing presence-default-on tests continue to pass unchanged. +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/_plan/260905_always_on_429_failover/030_types_docs_surface.md b/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md index d343d977d7..63b8d39632 100644 --- a/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md +++ b/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md @@ -31,12 +31,24 @@ its own translation obligation across ten locales. ## Management API / GUI -`genericPoolSettingsDto` reports `inert: true` and the GUI's Anthropic pool settings panel -describes the opt-in pool. Neither lies after this change — the pool flag still means what the -panel says it means for proactive routing. A copy pass explaining "429 failover always on" is -desirable but is GUI-surface work; per `AGENTS.md` a PR touching `gui` requires a screenshot -in the description. Keeping `gui/` out of this PR keeps the change reviewable as a routing -fix. Recorded here as a deliberate deferral, not an oversight. +**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 @@ -46,11 +58,16 @@ Per the user's standing instruction, **no repository-wide local suite**. Focused 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/_plan/260905_always_on_429_failover/040_missed_surfaces.md b/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md new file mode 100644 index 0000000000..07163f5579 --- /dev/null +++ b/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md @@ -0,0 +1,99 @@ +# 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. + +Add a third branch, after the generic one, using the Anthropic rotator and its own +`anthropicPoolFailovers` bound: + +```ts +} 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; } +} +``` + +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. +- Update the `failoverAccountSnapshot(` occurrence count from 3 to 4. From 892c716626502de2d6261629b1f59dd780ad63b3 Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 02:03:49 +0900 Subject: [PATCH 3/5] docs(devlog): fix the unreachable sidecar Anthropic arm found in audit round 2 --- .../001_audit_round_1.md | 11 ++++ .../040_missed_surfaces.md | 59 +++++++++++++++++-- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md b/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md index 050555c21c..402042c5f0 100644 --- a/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md +++ b/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md @@ -55,6 +55,17 @@ move. That string becomes stale. `gui/` stays out of scope (the AGENTS.md screen 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 diff --git a/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md b/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md index 07163f5579..04bf1f656b 100644 --- a/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md +++ b/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md @@ -59,10 +59,40 @@ bridge. It tries the key pool, then generic OAuth. Anthropic is excluded from ge by design, so an Anthropic 429 inside a web-search or image turn is terminal even with the pool enabled. -Add a third branch, after the generic one, using the Anthropic rotator and its own -`anthropicPoolFailovers` bound: +**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 @@ -78,10 +108,26 @@ Add a third branch, after the generic one, using the Anthropic rotator and its o route.provider = { ...route.provider, apiKey: accessToken }; promoteAnthropicActiveAccount(nextAccountId); logCtx.provider = formatAnthropicProviderForLog("anthropic", nextAccountId, config); - } catch { return null; } + } 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` @@ -95,5 +141,8 @@ same way. - 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. -- Update the `failoverAccountSnapshot(` occurrence count from 3 to 4. +- 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. From 72eeb00257dd784465ad9c679040738228fbf1c0 Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 02:09:43 +0900 Subject: [PATCH 4/5] fix(oauth): always fail over to another credential on 429 Rotation on a 429 was gated three different ways. An apiKeyPool of two keys 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 while the second account sat idle. Separate reactive failover from proactive routing. Reactive rotation runs only after upstream refused, so it activates on account presence and is no longer disableable. Proactive routing -- affinity, quota-ranked selection, strategy, autoSwitchThreshold, and the pre-dispatch preference -- still moves a healthy request, so it stays opt-in and oauthAccountFailover.enabled still refuses it. Also closes two surfaces that never rotated at all: the continuation loop had no generic-OAuth arm, so an xAI or Cursor continuation 429 was terminal even with failover active, and the sidecar hook had no Anthropic arm, so a 429 in a web-search or image turn was terminal even with the pool on. The sidecar gate is now a positive else-if; as an early return it made the new arm unreachable. --- src/oauth/anthropic-routing.ts | 38 +++++- src/oauth/generic-account-failover.ts | 46 +++++-- src/server/responses/core.ts | 101 +++++++++++++-- src/types/config.ts | 27 ++-- src/types/provider.ts | 10 +- tests/adapter-event-oauth-failover.test.ts | 17 ++- tests/always-on-429-failover.test.ts | 142 +++++++++++++++++++++ tests/generic-oauth-failover.test.ts | 68 ++++++++-- 8 files changed, 394 insertions(+), 55 deletions(-) create mode 100644 tests/always-on-429-failover.test.ts 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. From 9e7c27c76d7bdc3c39d4da9d9f24fd5706445925 Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 02:11:04 +0900 Subject: [PATCH 5/5] docs(devlog): close the always-on 429 failover unit --- .../000_research_inventory.md | 0 .../001_audit_round_1.md | 0 .../010_anthropic_reactive_split.md | 0 .../020_generic_oauth_non_disableable.md | 0 .../030_types_docs_surface.md | 0 .../040_missed_surfaces.md | 0 .../090_outcome.md | 41 +++++++++++++++++++ 7 files changed, 41 insertions(+) rename devlog/{_plan => _fin}/260905_always_on_429_failover/000_research_inventory.md (100%) rename devlog/{_plan => _fin}/260905_always_on_429_failover/001_audit_round_1.md (100%) rename devlog/{_plan => _fin}/260905_always_on_429_failover/010_anthropic_reactive_split.md (100%) rename devlog/{_plan => _fin}/260905_always_on_429_failover/020_generic_oauth_non_disableable.md (100%) rename devlog/{_plan => _fin}/260905_always_on_429_failover/030_types_docs_surface.md (100%) rename devlog/{_plan => _fin}/260905_always_on_429_failover/040_missed_surfaces.md (100%) create mode 100644 devlog/_fin/260905_always_on_429_failover/090_outcome.md diff --git a/devlog/_plan/260905_always_on_429_failover/000_research_inventory.md b/devlog/_fin/260905_always_on_429_failover/000_research_inventory.md similarity index 100% rename from devlog/_plan/260905_always_on_429_failover/000_research_inventory.md rename to devlog/_fin/260905_always_on_429_failover/000_research_inventory.md diff --git a/devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md b/devlog/_fin/260905_always_on_429_failover/001_audit_round_1.md similarity index 100% rename from devlog/_plan/260905_always_on_429_failover/001_audit_round_1.md rename to devlog/_fin/260905_always_on_429_failover/001_audit_round_1.md diff --git a/devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md b/devlog/_fin/260905_always_on_429_failover/010_anthropic_reactive_split.md similarity index 100% rename from devlog/_plan/260905_always_on_429_failover/010_anthropic_reactive_split.md rename to devlog/_fin/260905_always_on_429_failover/010_anthropic_reactive_split.md diff --git a/devlog/_plan/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 similarity index 100% rename from devlog/_plan/260905_always_on_429_failover/020_generic_oauth_non_disableable.md rename to devlog/_fin/260905_always_on_429_failover/020_generic_oauth_non_disableable.md diff --git a/devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md b/devlog/_fin/260905_always_on_429_failover/030_types_docs_surface.md similarity index 100% rename from devlog/_plan/260905_always_on_429_failover/030_types_docs_surface.md rename to devlog/_fin/260905_always_on_429_failover/030_types_docs_surface.md diff --git a/devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md b/devlog/_fin/260905_always_on_429_failover/040_missed_surfaces.md similarity index 100% rename from devlog/_plan/260905_always_on_429_failover/040_missed_surfaces.md rename to devlog/_fin/260905_always_on_429_failover/040_missed_surfaces.md 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.