From 73a9bd5bdaa8fffc8f90bc9ba88bb8872f69c940 Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 04:16:45 +0900 Subject: [PATCH 1/2] docs(devlog): record what the post-merge audit found --- .../090_outcome.md | 22 +++++-- .../091_post_merge_audit.md | 57 +++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 devlog/_fin/260905_always_on_429_failover/091_post_merge_audit.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 index c0d40b496f..c942b6b908 100644 --- a/devlog/_fin/260905_always_on_429_failover/090_outcome.md +++ b/devlog/_fin/260905_always_on_429_failover/090_outcome.md @@ -1,6 +1,15 @@ # 090 — Outcome -Shipped as `fix(oauth): always fail over to another credential on 429`. +Shipped in three pull requests: + +| PR | Merge | What | +|---|---|---| +| [#3495](https://github.com/lidge-jun/opencodex/pull/3495) | `56a084aa9` | the failover fix itself | +| [#3499](https://github.com/lidge-jun/opencodex/pull/3499) | `26a2e512a` | GUI copy the fix invalidated | +| [#3503](https://github.com/lidge-jun/opencodex/pull/3503) | `6edc56328` | a per-request store read #3495 introduced | + +The second and third were not planned. Both were found by auditing the merged result against +the tree rather than against the plan, and both are recorded in `091`. ## What changed @@ -33,9 +42,12 @@ with one: the proposed sidecar Anthropic arm sat behind an early `return null` a been dead code that a naive string test still passed. Round 3 passed. Every finding was folded in; none was rebutted. -## Known follow-up +## Follow-ups — both closed `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. +went stale the moment #3495 landed: with the pool off a 429 now does move. Deferring it out of +the routing PR was right — an `AGENTS.md` screenshot gate plus a ten-locale copy pass does not +belong there — but leaving it deferred was not, because the toggle would have sent an operator +to the EXPERIMENTAL pool to buy failover they already had unconditionally. Closed by #3499. + +The per-request auth-store read is the more serious of the two and is written up in `091`. diff --git a/devlog/_fin/260905_always_on_429_failover/091_post_merge_audit.md b/devlog/_fin/260905_always_on_429_failover/091_post_merge_audit.md new file mode 100644 index 0000000000..8c1e360862 --- /dev/null +++ b/devlog/_fin/260905_always_on_429_failover/091_post_merge_audit.md @@ -0,0 +1,57 @@ +# 091 — What the post-merge audit found + +Both findings below came from re-reading the MERGED result against the tree, after the plan's +own criteria were satisfied. Neither was reachable from the plan, because both were created by +the fix itself. + +## F1 — #3495 put a file read in front of every Anthropic request + +`hasAnthropicFailoverQuorum` decides whether a request records the account that served it, so it +runs on the INITIAL resolution of ordinary traffic — not only after a 429. It calls +`getAccountSet`, which goes through `loadAuthStore`, and that has no cache: every call chmods the +config dir, chmods the secret, reads the whole credential file and normalizes it. + +The generic twin had already hit this exact wall and documented it in +`src/oauth/generic-account-failover.ts`: + +> Since presence now decides activation, this predicate runs on paths that have not seen a 429 at +> all […] so an uncached check would put a synchronous file read in front of every request for +> every OAuth provider. + +I read that module closely enough to copy its activation semantics and not closely enough to copy +the cache that makes those semantics affordable. Fixed in #3503 by mirroring it: same 2 s window, +same "the cache holds a COUNT, never a credential" rule (here a boolean derived from one). + +## F2 — the cache's invalidation was incomplete + +Found while auditing F1's own fix. The cache was cleared on rotation and on pool-state reset, but +not on the two roster mutations that reach it from the management API. Deleting the second +Anthropic account left quorum `true` for up to 2 s — long enough for a request to record an id +whose credential was already gone. + +`clearAnthropicSessionAffinityForAccount` (the DELETE route) and +`resetAnthropicRoutingForManualSelection` now invalidate too, so all four roster-mutating paths +are covered. + +The regression test observes `atime` on the credential file rather than stubbing the module. A +mock would pass against a read reintroduced through a different call path; the syscall +observation would not. + +## A CI lesson worth keeping + +The macOS job failed on `npm launcher restarts the stopped runtime after a staged update`. I +called it a flake and reran — it had genuinely passed on rerun for #3499. It then failed a +**second** time, and the workflow log says plainly: + +``` +macOS suite failed on attempt N (exit …); assertion failures are not retried. +``` + +So the second rerun was never going to help, and the flake call should not have been repeated +without new evidence. The actual cause was not the diff — the test passes 15/15 locally and +imports nothing this unit touched — but that `dev` had moved to a 2-way macOS shard +(`4cacdfbb6`, #3501) after this branch point, which is the maintainer's own fix for the +resource pressure that was timing the job out. Rebasing onto it turned macOS green. + +**Rule:** when a rerun fails the same way twice, stop rerunning and check whether the base +branch already carries the fix. A stale branch point is a cause, not a flake. From 4c67e3e498ef6cc045bc32a8cd4c31f0c87548be Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 04:17:53 +0900 Subject: [PATCH 2/2] test(oauth): pin the rotator set of every 429 recovery loop This unit found the same class of defect twice: the streaming loop grew generic OAuth rotation and the continuation loop did not, and the sidecar hook grew generic rotation while Anthropic stayed excluded. Both times a loop shipped with a subset of the rotators and nothing failed, because the gap is invisible unless you diff the loops against each other. Pin the per-rotator site count with the reason each number differs, so a fifth recovery site has to decide which rotators it needs rather than inheriting a subset by accident. --- tests/generic-oauth-failover.test.ts | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/generic-oauth-failover.test.ts b/tests/generic-oauth-failover.test.ts index 7bbfc57951..f4028247d1 100644 --- a/tests/generic-oauth-failover.test.ts +++ b/tests/generic-oauth-failover.test.ts @@ -303,6 +303,40 @@ describe("sidecar on429 wiring", () => { expect(coreSource.indexOf("apiKey: snapshot.accessToken")).toBeGreaterThan(helperStart); }); + test("every 429 recovery loop carries all three rotators (#3495 follow-up)", () => { + // This unit found the same defect twice: the streaming loop grew generic OAuth rotation and + // the continuation loop did not, and the sidecar hook grew generic rotation while Anthropic + // stayed excluded. Both times a loop shipped with a SUBSET of the rotators, and both times + // nothing failed -- the gap is invisible unless you diff the loops against each other. + // + // A rotator set is the contract: any site that recovers a 429 by swapping a credential must + // be able to swap ALL of them, or some provider's rate limit is terminal there while the + // identical limit recovers one loop over. + const rotators = { + key: /hasKeyPoolFailover\(/g, + anthropic: /rotateAnthropicAccountOn429\(/g, + generic: /rotateGenericOAuthAccountOn429\(/g, + }; + const counts = Object.fromEntries( + Object.entries(rotators).map(([name, re]) => [name, (coreSource.match(re) ?? []).length]), + ); + + // The counts differ by rotator because the recovery sites differ, and each number is a + // statement about which providers can recover where: + // + // generic = 4: streaming loop, continuation loop, sidecar hook, runTurn preflight. + // anthropic = 3: the same, MINUS runTurn -- that path is Cursor-only (cursor.ts is the + // sole adapter implementing runTurn), so Anthropic cannot reach it. + // key = 2: hasKeyPoolFailover guards only the two response loops; the sidecar + // reaches the key pool through rotateProviderTransportOn429 instead. + // + // Adding a fifth recovery site means deciding, deliberately, which rotators it needs and + // updating the matching number. That decision is the thing this test exists to force. + expect(counts.generic).toBe(4); + expect(counts.anthropic).toBe(3); + expect(counts.key).toBe(2); + }); + test("the helper fails closed rather than pairing a new bearer with an old identity", () => { const start = coreSource.indexOf("const applyFailoverSnapshot ="); expect(start).toBeGreaterThan(-1);