test(oauth): pin the rotator set of every 429 recovery loop

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`.

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.

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);