From a19046a44d8d7f8f6cff91296cc02ddd4c6abaf5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:02:25 +0900 Subject: [PATCH 01/14] docs(devlog): re-verify the wp6 plan against the post-wp5 dev head --- .../060_wp6_wham_401_refresh.md | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index 7dc484b7ef..793c5b17bc 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -137,3 +137,48 @@ load-bearing here rather than routine. `Closes #3019`. Comment on PR #3020 crediting the sequence and naming the scope that was left out. + +## P-phase re-verification against the landed tree (wp6 start) + +Checked at `330470e74`, the dev head after wp5 merged. The plan holds: + +- `fetchFreshPoolAccountQuota` still converts any 401 straight into + `needsReauth` (`src/codex/auth-api.ts:979-983`). One WHAM request, no refresh attempt. +- `forceRefreshCodexPoolToken` is still at `src/codex/account-store.ts:588` with the + contract the plan's amendments depend on: `rotated` false means replaying earns the same + 401, and `selfRefreshed` true means this caller's own CAS moved the credential. +- Nothing in the quota path calls it. The only two callers are the response lanes + (`src/server/responses/compact.ts:302` and `core.ts:1806`). + +### The existing callers are the template + +Both response lanes already do the sequence this phase needs, and both encode the two +amendments the audit rounds forced into the plan: + +```ts +if (!refreshed.rotated) { + return { ok: false, quarantine: true, quarantineGeneration: refreshed.generation, ... }; +} +if (refreshed.selfRefreshed) { + handOffThreadAffinityGeneration(authCtx.accountId, authCtx.generation, refreshed.generation); +} +``` + +Two things to carry deliberately: + +1. **Quarantine fences on the RETURNED generation**, never the rejected one. A successful + token response can rotate the refresh grant while returning a byte-identical access + token, so the credential has already moved by the time `rotated` is false. +2. `selfRefreshed` gates only the affinity handoff there, because those lanes have no + per-credential retry budget. The quota path does need one, which is where the round-3 + amendment applies: `selfRefreshed === false` means *either* an external replacement + *or* a caller that joined an in-flight refresh of the same lineage + (`account-store.ts:639-645`). Spending the budget on the joined case and resetting only + on a genuine replacement is the distinction regression 8 exists to prove. + +### Scope confirmed unchanged + +`src/codex/auth-api.ts` plus a new bounded recovery module and +`tests/codex-auth-api.test.ts`. PR #3020 stays unrebased; its core sequence is carried +with credit. + From debf5df94a827f1b76b134b7bd39f40f798b7f27 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:09:28 +0900 Subject: [PATCH 02/14] docs(devlog): fold the wp6 plan audit into the unit doc --- .../060_wp6_wham_401_refresh.md | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index 793c5b17bc..98797d6e07 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -182,3 +182,96 @@ Two things to carry deliberately: `tests/codex-auth-api.test.ts`. PR #3020 stays unrebased; its core sequence is carried with credit. + +## Amendment after the wp6 plan audit (round 1) + +Four findings, all accepted. The plan had the right recovery direction and could not have +established either security property as written. + +### 1. The lineage rule needs an account-store contract change — mandatory, not conditional + +The plan said "if the returned contract cannot express the middle row today, the phase adds +that distinction to the primitive's return". It cannot. `forceRefreshCodexPoolToken` +returns `rotated` and `selfRefreshed` only (`account-store.ts:588-607`), and the +join-and-adopt path at `account-store.ts:639-660` returns nothing that separates it from an +external replacement — which is exactly the ambiguity the round-3 amendment was written +about. + +So `src/codex/account-store.ts` **is in scope**, and the change is a provenance value +rather than a boolean: + +| provenance | meaning | budget | +| --- | --- | --- | +| `self-refresh` | this call's own CAS moved the credential | spend | +| `joined-lineage` | joined an in-flight refresh of the same grant and adopted its result | spend | +| `external-replacement` | the stored credential was replaced by someone else | reset | + +`selfRefreshed` stays as a derived boolean so the two response lanes +(`compact.ts:302`, `core.ts:1806`) keep working unchanged; it is `provenance === "self-refresh"`. +Every return path of `resolveCodexToken` gets a regression asserting its provenance, +because a value nothing tests is a value that will drift. + +### 2. Cases 4 and 8 could not produce a refresh joiner + +Same-account quota calls already coalesce at `auth-api.ts:1043-1051`: a second caller for +the same account joins the existing **quota** flight and never reaches WHAM, let alone +`forceRefreshCodexPoolToken`. Two concurrent quota calls therefore produce one flight, not +an owner and a joiner — so case 8 would have passed under the precise reset-on-join bug it +was written to catch. That is the wrong-reason pattern this train has hit repeatedly. + +Replacement: + +- **8a (primitive level).** Two concurrent `forceRefreshCodexPoolToken` calls against one + account, one owner and one joiner, asserting the joiner reports `joined-lineage` and that + exactly one token request was issued. +- **8b (path level).** Drive the joiner through the recovery module directly rather than + through two quota calls, then issue a third 401 on the returned generation and assert no + second refresh. +- **9 (new).** A late quota caller arriving after the generation advanced but before the + WHAM replay finished. The flight's `resolvedCredentialGeneration` must be updated to the + RETURNED generation before the replay, or the join predicate at `auth-api.ts:1045` sees a + stale generation and starts a redundant flight. + +### 3. "Bounded" was asserted, not designed + +`registerStateStore` only registers callbacks (`state-store-sweeper.ts:49`); it imposes no +bound. One live account churning lineages would accumulate records forever. + +The design is now explicit: **one replaceable record per account.** A new lineage replaces +the previous record rather than adding to it, so the map is bounded by the number of +accounts, which is already bounded by config. The record additionally carries a TTL so a +stale budget cannot outlive its usefulness, and the store registers centrally in +`STATE_STORE_REGISTRATIONS` (`state-store-registrations.ts:76`) with both +`sweepExpired` and `reconcileGeneration`, so a deleted or re-added account drops its record. +`src/lib/state-store-registrations.ts` is in scope. + +Tests: lineage churn on one account leaves exactly one record; an expired record is swept; +delete-and-re-add clears it. + +### 4. Terminal classification and secrecy need their own cases + +"Structured terminal evidence" needs an allowlist, and one already exists for the main +account: `MAIN_TERMINAL_AUTH_CODES` at `auth-api.ts:602` (`invalid_workspace_selected`, +`invalid_refresh_token`) with the bounded-parser reasoning documented right below it. The +pool path reuses that set and that parser rather than inventing a second vocabulary. + +The pool catch currently marks EVERY `TokenRefreshError` terminal (`auth-api.ts:1024`), so +the phase must separate a revoked or expired grant from an unknown or transient refresh +failure; only the former sets `needsReauth`. + +Added cases: + +- **10.** Replay returns a 401 carrying a terminal code — `needsReauth: true`, no second refresh. +- **11.** The refresh itself fails transiently (not a terminal code) — `needsReauth` stays + false and the budget is not marked spent, so the next poll may try again. +- **12.** Neither the rejected bearer nor the rotated one appears in any log line, debug + buffer, or serialized response on any of these paths. Asserted at runtime by capturing + the log surfaces during the flow; `privacy:scan` is a static check and does not prove it. + +### Scope, corrected + +`src/codex/auth-api.ts`, `src/codex/account-store.ts`, a new +`src/codex/quota-401-recovery.ts`, `src/lib/state-store-registrations.ts`, and +`tests/codex-auth-api.test.ts` plus `tests/codex-account-store*.test.ts` for the provenance +regressions. + From 4024efa867d6de41522dd471e429c0928a68058d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:13:09 +0900 Subject: [PATCH 03/14] docs(devlog): claim/settle the wp6 recovery record, and make the terminal oracle falsifiable --- .../060_wp6_wham_401_refresh.md | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index 98797d6e07..7bd6b06bea 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -275,3 +275,56 @@ Added cases: `tests/codex-auth-api.test.ts` plus `tests/codex-account-store*.test.ts` for the provenance regressions. + +## Amendment after the wp6 plan audit (round 2) + +Two findings, both accepted. The provenance enum, the coalescing bypass in 8a/8b, case 9, +and reusing `MAIN_TERMINAL_AUTH_CODES` were confirmed sound and are unchanged. + +### 5. The recovery record needs claim/settle, not replacement — and the TTL contradicted the guarantee + +"One replaceable record per account" bounds memory and says nothing about ordering. The +interleaving that breaks it: lineage A starts a refresh; meanwhile the credential is +externally replaced by lineage B, which receives a 401 and spends its own budget; A then +resolves as `external-replacement` and resets the record — handing B a second refresh it +already used. Replacement is the wrong primitive. + +**Claim/settle with an expected lineage.** A caller claims the budget for the lineage it is +about to refresh, and may settle only the record it claimed: + +- `claim(accountId, lineage)` returns `granted` when no record exists for the account, or + the record's lineage differs AND that record is not for a lineage the store considers + current; otherwise `spent`. +- `settle(accountId, lineage, provenance)` is a compare-and-set on the lineage. If the + stored record has moved to a newer lineage, the completion is stale and is **dropped** — + it may never downgrade a newer lineage's spent state to unspent. +- `external-replacement` does not reset in place. It settles the claimed record as spent + for the OLD lineage and lets the NEW lineage claim on its own next 401, which is what + "a new grant deserves its own recovery attempt" actually means. + +Interleavings to test: a stale completion arriving after a newer lineage has spent; a +joiner settling after an external replacement; simultaneous external and self outcomes on +one account. + +**The TTL is removed.** It was load-bearing for boundedness in the previous draft, and it +directly contradicts the security property: expiring a still-live spent record grants the +same lineage another refresh, which is the unbounded-retry loop this phase exists to +prevent. Boundedness comes from one record per account plus `reconcileGeneration`, and the +record is retained until the credential is replaced or the account is deleted. That is a +strictly stronger guarantee and one less knob. + +### 6. The terminal-refresh oracle was unfalsifiable + +Case 11 covers only a transient refresh failure, so an implementation that made **every** +refresh failure transient would pass cases 1-12 — while today's code does the opposite and +marks every `TokenRefreshError` terminal (`auth-api.ts:1024`). A test suite that cannot +distinguish the two extremes is not an oracle. + +- **11a.** Refresh fails with a revoked grant — `needsReauth: true`. +- **11b.** Refresh fails with an expired grant — `needsReauth: true`. +- **11c.** Refresh fails with an unknown or network error — `needsReauth` stays false, the + claim is released rather than settled spent, so the next poll may try again. + +Case 6 is also strengthened: after `rotated === false`, issue another 401 on the RETURNED +generation and assert no second refresh is issued before the lineage actually changes. + From 6179e4e781e0ed1ecd3d6952ec8612d9ee323de5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:15:33 +0900 Subject: [PATCH 04/14] docs(devlog): finalize the wp6 recovery-store design (claim id, lease, backoff, liveness sweep) --- .../060_wp6_wham_401_refresh.md | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index 7bd6b06bea..19dc8b27f8 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -328,3 +328,76 @@ distinguish the two extremes is not an oracle. Case 6 is also strengthened: after `rotated === false`, issue another 401 on the RETURNED generation and assert no second refresh is issued before the lineage actually changes. + +## Amendment after the wp6 plan audit (round 3) — final state design + +Four findings, all accepted. This section is the authoritative description of the recovery +store; earlier sketches in this doc are superseded where they conflict. + +### 7. The claim identity is a claim id, not the lineage + +Lineage alone cannot separate an old claimant from a later retry on the same lineage: +claim L₁ → transient failure releases L₁ → another poll claims L₁ → the first caller settles +late and spends the second caller's claim. + +``` +claim(accountId, lineage) -> { granted: true, claimId } | { granted: false, reason } +settle(accountId, claimId, outcome) // CAS on (accountId, lineage, claimId) +release(accountId, claimId, backoff) // same CAS +``` + +Settling or releasing an unclaimed or superseded claim is a **no-op**, not an error: a late +completion is exactly the case that must not disturb a newer claimant. + +`settle` also carries the **returned** generation and the provenance, because that is what +becomes the spent fence — case 6 and 8b both depend on the fence being the generation the +refresh returned, never the one that was rejected. + +### 8. `spent` is durable; `claimed` is a lease + +Removing the TTL was right for spent records and wrong for in-flight ones. A caller can be +cancelled between claim and settle — `forceRefreshCodexPoolToken` explicitly supports +caller-scoped cancellation while the shared flight continues +(`account-store.ts:639-660`) — and an abandoned claim would wedge that account's recovery +permanently. + +So the record has two shapes: + +| state | expires? | meaning | +| --- | --- | --- | +| `claimed` | yes — bounded lease | a refresh is in flight for this lineage | +| `spent` | no | this lineage already had its one refresh | +| `backoff` | at `nextAttemptAt` | a transient failure; retry allowed after the clock advances | + +An expired `claimed` lease is reclaimable, which recovers from a cancelled or thrown +caller. An expired lease is **not** promoted to `spent`: the refresh may never have +happened. + +Tests: owner cancellation between claim and settle; a settlement that throws; a stale +claim recovered by a later poll. + +### 9. Transient failure releases into backoff, not into eligibility + +Releasing outright reopens the loop the phase exists to close: a failed quota request does +not refresh the quota timestamp, so successive dashboard and background polls would each +issue another token refresh. `release` therefore records `nextAttemptAt` and the record +stays — still one per account. + +Case 11c becomes: several immediate polls after a transient refresh failure issue exactly +**one** refresh; a poll after the clock advances past `nextAttemptAt` may issue another. + +### 10. `reconcileGeneration` alone cannot see a credential change + +`GenerationContext` carries `codexAccountIds` only (`state-store-sweeper.ts:3-11`), so it +can drop a record for a deleted account but not for one whose credential was replaced under +the same id. The promise that a record lives only until credential replacement needs a +second mechanism: + +- `reconcileGeneration` drops records whose account id is gone (delete/re-add), and +- it additionally drops any record whose fenced generation fails + `isCodexAccountGenerationLive(accountId, generation)` (`account-store.ts:196-199`), which + is exactly "the credential this record fenced is no longer the stored one". + +Tests: replace the credential without another 401, run a sweep, and assert the record is +gone; delete and re-add the account and assert the same. + From c8d08c9b2e70041207562993246fe79089f59118 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:17:36 +0900 Subject: [PATCH 05/14] docs(devlog): settle-by-outcome and lease rules for the wp6 recovery store --- .../060_wp6_wham_401_refresh.md | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index 19dc8b27f8..ba2a992ead 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -401,3 +401,64 @@ second mechanism: Tests: replace the credential without another 401, run a sweep, and assert the record is gone; delete and re-add the account and assert the same. + +## Amendment after the wp6 plan audit (round 4) — settlement and lease rules + +Three findings, all accepted. This section is authoritative over every earlier one where +they conflict. + +### 11. A live claim is not stale just because the credential moved + +The refresh CAS commits G → G+1 (`account-store.ts:864`) **before** the quota caller can +settle. In that window the `claimed` record still fences G, so a naive liveness sweep sees +G as non-live and deletes it, and a G+1 claim can replace it. The late settle then no-ops +and G+1 is left unspent — a second refresh, which is the loop this phase closes. + +State-aware rules, replacing the flat "drop non-live" from round 3: + +- A valid, unexpired `claimed` record **blocks every other lineage for that account**. A + `claim` for a different lineage while one is live returns `granted: false`. +- Reconciliation and the liveness sweep **must not remove a live `claimed` record** merely + because its starting generation moved. Moving is the expected outcome of the refresh it + is fencing. +- Only account deletion, a matching `settle`/`release`, or lease expiry may end a claim. +- Stale-generation cleanup applies to `spent` and `backoff` records only. + +Tests: run a sweep between the refresh commit and settlement and assert the claim survives; +attempt a G+1 claim in the same window and assert it is refused. + +### 12. Settlement follows the shared flight, not the cancelled waiter + +Caller cancellation is scoped to the waiter; the shared refresh keeps running and may commit +after that caller is gone (`account-store.ts:639-660`). Releasing the claim on cancellation +would let that still-running flight rotate the credential with no spent fence at all. + +- A cancelled waiter **does not release**. The claim stays `claimed` until the shared + flight's terminal outcome is known, and whichever caller observes that outcome settles it. +- The lease is sized against the operations it covers, not a round number: the refresh + flight's stale bound (`CODEX_REFRESH_FLIGHT_STALE_MS`) plus the WHAM request deadline + (8s, `auth-api.ts:975`) plus margin. A lease shorter than the flight it fences would + expire mid-refresh and admit a second claim — the same hole from the other side. + +Tests: cancel the waiter, let the background flight commit successfully, and assert a +competing poll during that window is refused and the fence lands exactly once. + +### 13. `external-replacement` must not fence the returned generation + +Round 3 said the returned generation "becomes the spent fence". That is right for +`self-refresh` and `joined-lineage` and wrong for `external-replacement`: there the +returned generation is a **new** lineage that has had no recovery attempt and deserves its +own budget. Fencing it would deny the new credential the one refresh this phase exists to +grant. + +Outcome-specific settlement, explicitly: + +| outcome | what is spent | what the returned generation gets | +| --- | --- | --- | +| `self-refresh` | the returned generation | fenced — its one attempt is used | +| `joined-lineage` | the returned generation | fenced — same lineage, same budget | +| `external-replacement` | the claimed OLD lineage only | untouched — free to claim on its own next 401 | +| stale claim id | nothing | untouched | + +Tests assert both stored fence generations, not just the presence of a record. + From df55ce5ad4556ed8a0a2c6ff91172914b80ec85a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:20:08 +0900 Subject: [PATCH 06/14] docs(devlog): flight-owned settlement and one exported lease contract for wp6 --- .../060_wp6_wham_401_refresh.md | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index ba2a992ead..8fd729afe9 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -462,3 +462,64 @@ Outcome-specific settlement, explicitly: Tests assert both stored fence generations, not just the presence of a record. + +## Amendment after the wp6 plan audit (round 5) — flight-owned settlement, and one timing contract + +Two findings, both accepted. These are the last two blockers; the plan is implementable +once they are specified. + +### 14. Settlement attaches to the FLIGHT, not to any caller + +"Whichever caller observes the terminal outcome settles it" is not implementable through +the proposed API. `forceRefreshCodexPoolToken` returns `awaitOwnCancellation(refreshPromise, +callerSignal)` (`account-store.ts:917`); cancellation rejects that wrapper while the +underlying flight keeps running privately. With no joiner, nobody observes the successful +commit at all — the claim expires and the already-refreshed lineage gets a second refresh, +which is precisely the hole being closed. + +The flight itself is what must settle. `refreshPromise` is already held on the flight record +(`account-store.ts:912`), so: + +- `forceRefreshCodexPoolToken` accepts an optional `onSettled(outcome)` callback and attaches + it to `refreshPromise` — **before** the caller-scoped wait, so it fires on the flight's own + terminal outcome whether or not any waiter is still there. +- The callback carries the claim id the caller opened with, so settlement remains a CAS on + `(accountId, lineage, claimId)` and a superseded claim is still a no-op. +- It fires exactly once per flight, for both fulfilment and rejection: a rejected flight + settles as the transient or terminal outcome rather than leaving the claim to expire. + +Test: cancel the sole waiter, let the background flight commit successfully, assert the +spent fence lands exactly once and a competing poll during the window is refused. + +### 15. One exported deadline, not three literals plus "margin" + +The lease cannot be derived today. `CODEX_REFRESH_FLIGHT_STALE_MS` is private +(`account-store.ts:411`), the flight's own ceiling is a separate inline `30_000` +(`account-store.ts:749`), and the WHAM timeout is an inline `8000` +(`auth-api.ts:975`). "Plus margin" is exactly the kind of number that drifts out of +agreement with the thing it is supposed to cover. + +So one authoritative value is exported and consumed by both operations: + +```ts +// account-store.ts +export const CODEX_REFRESH_FLIGHT_CEILING_MS = 30_000; // the flight's own AbortSignal.timeout +// auth-api.ts +export const WHAM_REQUEST_TIMEOUT_MS = 8_000; // replaces the inline literal +// quota-401-recovery.ts +export const QUOTA_RECOVERY_LEASE_MS = CODEX_REFRESH_FLIGHT_CEILING_MS + WHAM_REQUEST_TIMEOUT_MS * 2; +``` + +The replay is counted twice because the sequence is request → refresh → replay, and both +WHAM legs sit inside the lease. + +Test: the lease is strictly greater than the longest admitted flight plus its replay, and +the test derives that bound from the exported constants rather than restating a number — +a restated number is how the two drift apart. + +### 16. Explicit cross-product case + +`external-replacement` together with `rotated: false` is logically covered (no replay, the +old claim is spent, the returned generation stays free) but nothing asserts it. Added as +case 13. + From 92ed45b522f5bd340a08e23b04ae4d00e4aa4174 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:22:33 +0900 Subject: [PATCH 07/14] docs(devlog): caller-specific completion and a leaf timing module for wp6 --- .../060_wp6_wham_401_refresh.md | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md index 8fd729afe9..20d83e2d74 100644 --- a/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md +++ b/devlog/_plan/260831_prio70_train_round2/060_wp6_wham_401_refresh.md @@ -523,3 +523,55 @@ a restated number is how the two drift apart. old claim is spent, the returned generation stays free) but nothing asserts it. Added as case 13. + +## Amendment after the wp6 plan audit (round 6) — completion boundary and timing ownership + +Two findings, both accepted. + +### 17. The completion must be caller-specific, not the raw flight + +Attaching `onSettled` to `refreshPromise` settles the wrong thing. Provenance is +**per caller**: the owner performed the CAS, a joiner awaits the flight and then runs the +adoption path (`account-store.ts:639-660`), and an early external replacement +(`account-store.ts:635`) has no flight at all. The raw promise cannot know which of those a +given caller became. + +The cardinality was also wrong. One refresh-grant flight can serve several account aliases +sharing that grant, each holding its own claim, and "exactly once per flight" would settle +one of them and drop the rest. + +So `forceRefreshCodexPoolToken` builds a **caller-specific completion**: + +- It is created for every return path, including the no-flight external-replacement case, + and it resolves with that caller's fully classified outcome — after the adoption or + replacement branch has run, not before. +- It is **not** subject to `callerSignal`. Cancellation still rejects the value the caller + awaits; the completion continues and fires. +- One callback is attached per `(accountId, claimId)`, so two aliases sharing one flight + settle two claims independently. +- A throwing callback is swallowed: settlement bookkeeping must never reject the credential + result or disturb another waiter. + +Tests: sole cancelled owner; cancelled joiner; two aliases on one flight both settled once; +no-flight external replacement settles; a throwing callback leaves other waiters and the +returned credential unaffected. + +### 18. Timing constants live in a leaf module + +The previous layout was an import cycle: `quota-401-recovery.ts` would import +`WHAM_REQUEST_TIMEOUT_MS` from `auth-api.ts`, while `auth-api.ts` must import the recovery +module for claim/settle. + +New file `src/codex/quota-recovery-timing.ts`, importing nothing from either: + +```ts +export const CODEX_REFRESH_FLIGHT_CEILING_MS = 30_000; +export const WHAM_REQUEST_TIMEOUT_MS = 8_000; +export const QUOTA_RECOVERY_LEASE_MS = + CODEX_REFRESH_FLIGHT_CEILING_MS + WHAM_REQUEST_TIMEOUT_MS * 2; +``` + +`account-store.ts`, `auth-api.ts` and `quota-401-recovery.ts` all import from it, replacing +their inline literals (`account-store.ts:749`, `auth-api.ts:975`). The derived-bound test +imports the same leaf rather than restating 46_000. + From 1b0810b9afa7632eea23b079558d12c13a4d042f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:29:36 +0900 Subject: [PATCH 08/14] fix(codex): refresh a stored token before quarantining it on a WHAM 401 Account-list quota sent one request to backend-api/wham/usage and turned any 401 into needsReauth. A bare 401 with no structured body is exactly what a stale-but-refreshable bearer produces after a plan change, so a working credential was thrown away and the operator was told to re-authenticate an account that was fine. forceRefreshCodexPoolToken already existed; nothing on this path called it. The path now refreshes once and replays once. needsReauth requires either structured terminal evidence in the body - the same MAIN_TERMINAL_AUTH_CODES allowlist and bounded parser the main account uses, since it is the same endpoint - or a refresh that failed terminally. Everything else is transient. Once is the hard part: an unbounded retry against an upstream 401 is a self-inflicted credential-stuffing loop. quota-401-recovery holds one budget per credential lineage, and three things about it are deliberate. The claim carries an opaque id rather than being keyed on the lineage, because lineage alone cannot separate an old claimant from a later retry on the same lineage - claim, transient failure releases, another poll claims, and the first caller's late completion spends the second caller's budget. A live claim blocks every lineage for that account, not just its own. The refresh it fences commits G to G+1 before the claimant can settle, so a G+1 claim in that window would leave the late settlement landing on nothing. Settlement is owned by the refresh, not by the caller. Cancellation rejects what the caller awaits while the shared flight keeps running and commits, so an onSettled callback attached to the resolution fires with no waiter present. Without it a cancelled poll leaves the claim to expire and the already-refreshed lineage gets a second refresh. forceRefreshCodexPoolToken also returns provenance now. selfRefreshed is a boolean and its false meant two opposite things: somebody replaced the credential, or this caller joined an in-flight refresh of the same grant. The first is a new lineage that has had no attempt and must keep its budget; the second is this lineage using its one. selfRefreshed stays as the derived boolean so the two response lanes are unchanged. --- src/codex/account-store.ts | 98 +++++++++++-- src/codex/auth-api.ts | 205 ++++++++++++++++++++++----- src/codex/quota-401-recovery.ts | 166 ++++++++++++++++++++++ src/codex/quota-recovery-timing.ts | 28 ++++ src/lib/state-store-registrations.ts | 8 ++ tests/quota-401-recovery.test.ts | 183 ++++++++++++++++++++++++ 6 files changed, 644 insertions(+), 44 deletions(-) create mode 100644 src/codex/quota-401-recovery.ts create mode 100644 src/codex/quota-recovery-timing.ts create mode 100644 tests/quota-401-recovery.test.ts diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 6774637257..2e9a038bda 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -13,6 +13,7 @@ import { import { assertNotRealHomeUnderTest } from "../lib/test-home-guard"; import type { CodexAccountCredentialRecord, CodexAccountCredentials } from "../types"; import { advanceCodexCredentialMutationEpoch } from "./credential-mutation-epoch"; +import { CODEX_REFRESH_FLIGHT_CEILING_MS } from "./quota-recovery-timing"; type LegacyCodexAccountStore = Record; type CodexAccountStore = Record; @@ -407,7 +408,29 @@ type CodexRefreshResult = CodexTokenResult & { * refresh of the one the caller was holding, not somebody else's replacement. */ selfRefreshed?: boolean; + /** + * Three-way form of {@link selfRefreshed}, kept alongside it so existing callers are + * unaffected (#3019). `selfRefreshed` is `provenance === "self-refresh"`. + */ + provenance?: CodexRefreshProvenance; }; + +/** + * How THIS caller arrived at the credential it is returning (#3019). + * + * `selfRefreshed` is a boolean, and a boolean cannot carry three cases. Its `false` means + * both "somebody else replaced the credential" and "I joined an in-flight refresh of the + * same grant and adopted its result" — and a recovery budget has to treat those opposite + * ways. Joining is the same lineage getting its one refresh; replacement is a NEW lineage + * that has not had one yet, and charging it for somebody else's attempt would deny the + * fresh credential the recovery this exists to grant. + */ +export type CodexRefreshProvenance = "self-refresh" | "joined-lineage" | "external-replacement"; + +/** Terminal outcome of one forced refresh, as seen by the caller that requested it. */ +export type ForcedRefreshOutcome = + | { kind: "resolved"; provenance: CodexRefreshProvenance; generation: number; rotated: boolean } + | { kind: "failed"; error: unknown }; const MAX_CODEX_REFRESH_FLIGHTS = 32; const CODEX_REFRESH_FLIGHT_STALE_MS = 120_000; interface RefreshFlight { @@ -587,22 +610,58 @@ function awaitOwnCancellation(work: Promise, callerSignal?: AbortSignal): */ export async function forceRefreshCodexPoolToken( id: string, - options: { rejectedGeneration: number; rejectedAccessToken: string; signal?: AbortSignal }, -): Promise { - const result = await resolveCodexToken( - id, - { rejectedGeneration: options.rejectedGeneration, rejectedAccessToken: options.rejectedAccessToken }, - options.signal, - ); + options: { + rejectedGeneration: number; + rejectedAccessToken: string; + signal?: AbortSignal; + /** + * Fires with THIS caller's classified outcome, regardless of `signal` (#3019). + * + * Cancellation rejects what the caller awaits; the shared flight keeps running and + * commits. A recovery budget claimed before the refresh therefore has no one left to + * settle it — the claim expires and the already-refreshed lineage gets a second + * refresh, which is the loop the budget exists to close. This callback is attached to + * the resolution itself, so it fires with no waiter present. + * + * It is called exactly once per call, for both success and failure, and its own + * failures are swallowed: settlement bookkeeping must never reject a credential the + * caller successfully obtained, nor disturb another waiter on the same flight. + */ + onSettled?: (outcome: ForcedRefreshOutcome) => void; + }, +): Promise { + const settle = (outcome: ForcedRefreshOutcome) => { + try { options.onSettled?.(outcome); } catch { /* bookkeeping must not break the caller */ } + }; + let result: CodexRefreshResult; + try { + result = await resolveCodexToken( + id, + { rejectedGeneration: options.rejectedGeneration, rejectedAccessToken: options.rejectedAccessToken }, + options.signal, + ); + } catch (error) { + settle({ kind: "failed", error }); + throw error; + } + // Default to the conservative reading. A path that did not classify itself is not + // assumed to be this caller's own lineage: charging a replacement for somebody else's + // attempt is the failure mode, so an unlabelled path is treated as a replacement and + // simply leaves the returned lineage its own budget. + const provenance: CodexRefreshProvenance = result.provenance + ?? (result.selfRefreshed === true ? "self-refresh" : "external-replacement"); + const rotated = result.accessToken !== options.rejectedAccessToken; + settle({ kind: "resolved", provenance, generation: result.generation, rotated }); return { accessToken: result.accessToken, chatgptAccountId: result.chatgptAccountId, generation: result.generation, - rotated: result.accessToken !== options.rejectedAccessToken, + rotated, // Only a CAS this call performed itself proves the new credential descends from the // rejected one; anything else is somebody else's replacement and must not be treated // as this request's own lineage. - selfRefreshed: result.selfRefreshed === true, + selfRefreshed: provenance === "self-refresh", + provenance, }; } @@ -633,7 +692,15 @@ async function resolveCodexToken( // correct again and refreshing would burn a rotation for nothing. const forcedTargetsStoredCredential = forced !== undefined && !forcedFenceSuperseded(record.generation, forced); if (cred.expiresAt > Date.now() + REFRESH_SKEW_MS && !forcedTargetsStoredCredential) { - return { accessToken: cred.accessToken, chatgptAccountId: cred.chatgptAccountId, generation: record.generation }; + // The freshness shortcut: nothing was refreshed and nothing was adopted. A forced + // caller reaches it only once its fence was superseded, which is a replacement by + // definition; an ordinary caller does not read this field. + return { + accessToken: cred.accessToken, + chatgptAccountId: cred.chatgptAccountId, + generation: record.generation, + provenance: "external-replacement", + }; } const existing = refreshLocks.get(refreshGrantFingerprint); @@ -658,6 +725,9 @@ async function resolveCodexToken( accessToken: currentCred.accessToken, chatgptAccountId: currentCred.chatgptAccountId, generation: current.generation, + // Adopted the stored result of a flight this caller joined: same grant, same + // lineage. Not a replacement — that distinction is the whole point of #3019. + provenance: "joined-lineage", }; } } @@ -693,6 +763,9 @@ async function resolveCodexToken( accessToken: currentCred.accessToken, chatgptAccountId: currentCred.chatgptAccountId, generation: current.generation, + // `forcedFenceSuperseded` is exactly "somebody else moved this credential past + // the generation I was holding" — a new lineage, entitled to its own budget. + provenance: "external-replacement", }; } if ( @@ -720,6 +793,7 @@ async function resolveCodexToken( // This joiner performed its own CAS onto its own record, so the resulting // generation is its own lineage even though another caller drove the fetch. selfRefreshed: true, + provenance: "self-refresh", resolvedGrantFingerprint: refreshGrantFingerprint, }; } @@ -747,7 +821,7 @@ async function resolveCodexToken( * eviction) and the 30s ceiling remain, because those bound the flight itself. */ const abort = new AbortController(); - const signal = AbortSignal.any([abort.signal, AbortSignal.timeout(30_000)]); + const signal = AbortSignal.any([abort.signal, AbortSignal.timeout(CODEX_REFRESH_FLIGHT_CEILING_MS)]); let flight!: RefreshFlight; const fetchPromise = withCodexRefreshFileLock(refreshGrantFingerprint, signal, async (): Promise => { const current = readCodexAccountRecord(id); @@ -803,6 +877,7 @@ async function resolveCodexToken( credential: sameGrantFreshCredential, resolvedGrantFingerprint: refreshGrantFingerprint, selfRefreshed: true, + provenance: "self-refresh", }; } const res = await fetch(CHATGPT_TOKEN_URL, { @@ -882,6 +957,7 @@ async function resolveCodexToken( // token — tagging the new grant would make every legitimate joiner look foreign. resolvedGrantFingerprint: refreshGrantFingerprint, selfRefreshed: true, + provenance: "self-refresh", }; }); /* diff --git a/src/codex/auth-api.ts b/src/codex/auth-api.ts index c70ebf79a7..9f6b0614eb 100644 --- a/src/codex/auth-api.ts +++ b/src/codex/auth-api.ts @@ -10,6 +10,7 @@ import { getCodexAccountCredential, getValidCodexToken, isCodexAccountGenerationLive, + forceRefreshCodexPoolToken, markCodexAccountValidated, readCodexAccountRecord, saveCodexAccountCredential, @@ -124,6 +125,8 @@ import { tryAcquireNativeMainProfileClaim } from "./native-main-admission"; import { withNativeMainSharedClaim } from "./native-main-claim"; import { resolveNativeProfileContext } from "./native-profile-store"; import { NativeProfileError } from "./native-profile-types"; +import { WHAM_REQUEST_TIMEOUT_MS } from "./quota-recovery-timing"; +import { claimQuotaRecovery, releaseQuotaRecovery, settleQuotaRecovery } from "./quota-401-recovery"; function isNativeMainClaimUnavailable(error: unknown): error is NativeProfileError { return error instanceof NativeProfileError @@ -766,7 +769,7 @@ async function fetchMainAccountInfoWhileOwned( try { const resp = await fetch("https://chatgpt.com/backend-api/wham/usage", { headers: { Authorization: `Bearer ${tokens.access_token}`, "ChatGPT-Account-Id": tokens.account_id }, - signal: AbortSignal.timeout(8000), + signal: AbortSignal.timeout(WHAM_REQUEST_TIMEOUT_MS), }); if (!resp.ok) { const terminalAuthFailure = await isTerminalMainAuthResponse(resp, isMainAccountTokenVerifiablyLive()); @@ -959,6 +962,156 @@ function reconcileFreshPoolAccountPlans(runtimeConfig: OcxConfig, updates: Fresh } } + + +/** + * One refresh-and-replay for a pool account whose WHAM request came back 401 (#3019). + * + * The account list used to convert any 401 straight into `needsReauth`, and a bare 401 is + * exactly what a stale-but-refreshable bearer produces after a plan change — so a healthy + * credential was thrown away and the operator was told to log in again. + * + * Bounded by the recovery store: one attempt per credential lineage. An unbounded retry + * against an upstream 401 is a self-inflicted credential-stuffing loop, which is why the + * claim is taken BEFORE the refresh and settled by the flight rather than by this caller. + */ +async function recoverPoolQuotaFrom401(ctx: { + accountId: string; + existing: StoredAccountQuota | null; + configuredPlan: string | undefined; + rejectedAccessToken: string; + rejectedGeneration: number; + resp: Response; + onCredentialGeneration?: (generation: number) => void; +}): Promise { + const { accountId, existing, configuredPlan, rejectedAccessToken, rejectedGeneration, resp } = ctx; + + // Structured terminal evidence short-circuits everything: the same allowlist and bounded + // parser the main account uses, because it is the same endpoint answering. + if (await isTerminalPoolAuthResponse(resp)) { + return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; + } + + const claim = claimQuotaRecovery(accountId, rejectedGeneration); + if (!claim.granted) { + // This lineage already spent its attempt, another caller is mid-refresh, or a transient + // failure is still backing off. Report transient and let the next poll try — quarantining + // here would undo the whole point of the budget. + return { quota: existing ?? null, needsReauth: false, credentialGeneration: rejectedGeneration }; + } + + let refreshed: Awaited>; + try { + refreshed = await forceRefreshCodexPoolToken(accountId, { + rejectedGeneration, + rejectedAccessToken, + // Settlement rides the flight, not this await: a cancelled caller would otherwise + // leave the claim to expire while the shared refresh commits, and the already + // refreshed lineage would get a second attempt. + onSettled: outcome => { + if (outcome.kind === "resolved") { + settleQuotaRecovery(accountId, claim.claimId, outcome); + } else { + releaseQuotaRecovery(accountId, claim.claimId, QUOTA_RECOVERY_BACKOFF_MS); + } + }, + }); + } catch (e) { + // A refresh that failed terminally is the one case where the credential really is gone. + // Everything else is unknown, and unknown is not proof. + if (e instanceof TokenRefreshError && isTerminalRefreshError(e)) { + return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; + } + return { quota: existing ?? null, needsReauth: false, credentialGeneration: rejectedGeneration }; + } + + // A byte-identical access token means replaying earns the same 401. Report transient + // rather than burning the replay; the fence already moved to the returned generation. + if (!refreshed.rotated) { + return { quota: existing ?? null, needsReauth: false, credentialGeneration: refreshed.generation }; + } + + // The flight may have moved the generation while this request was in the air. Tell the + // coalescing layer where the credential actually is, or a late caller joins on a stale + // generation and opens a redundant flight. + ctx.onCredentialGeneration?.(refreshed.generation); + + const writerGeneration = captureConfigGeneration(); + const replay = await fetch("https://chatgpt.com/backend-api/wham/usage", { + headers: { + Authorization: `Bearer ${refreshed.accessToken}`, + "ChatGPT-Account-Id": refreshed.chatgptAccountId, + }, + signal: AbortSignal.timeout(WHAM_REQUEST_TIMEOUT_MS), + }); + if (!replay.ok) { + if (replay.status === 401 && await isTerminalPoolAuthResponse(replay)) { + return { quota: existing ?? null, needsReauth: true, credentialGeneration: refreshed.generation }; + } + return { quota: existing ?? null, needsReauth: false, credentialGeneration: refreshed.generation }; + } + return await commitPoolQuotaResponse(replay, { + accountId, existing, configuredPlan, generation: refreshed.generation, writerGeneration, + }); +} + +/** Backoff after a refresh failure that proved nothing about the credential. */ +const QUOTA_RECOVERY_BACKOFF_MS = 60_000; + +/** Same allowlist and bounded parser as the main account: it is the same endpoint. */ +async function isTerminalPoolAuthResponse(resp: Response): Promise { + const code = await readMainAuthErrorCode(resp.clone()); + return typeof code === "string" && MAIN_TERMINAL_AUTH_CODES.has(code); +} + +/** A revoked or expired grant is terminal; an unknown or transport failure is not. */ +function isTerminalRefreshError(error: TokenRefreshError): boolean { + const text = `${error.message}`.toLowerCase(); + return text.includes("invalid_grant") + || text.includes("invalid_refresh_token") + || text.includes("revoked") + || text.includes("expired"); +} + +/** Parse and store a successful WHAM response. Shared by the first attempt and the replay. */ +async function commitPoolQuotaResponse( + resp: Response, + ctx: { + accountId: string; + existing: StoredAccountQuota | null; + configuredPlan: string | undefined; + generation: number; + writerGeneration: number; + }, +): Promise { + const { accountId, existing, configuredPlan, generation, writerGeneration } = ctx; + const data = (await resp.json()) as WhamUsageResponse; + const freshPlan = nonEmptyPlan(data.plan_type) ?? undefined; + const quota = parseUsageQuota({ ...data, plan_type: freshPlan ?? configuredPlan }); + const freshResetCredits = quota?.resetCredits; + if (!quota) { + return { + quota: isCodexAccountGenerationLive(accountId, generation) ? existing ?? null : getAccountQuota(accountId), + needsReauth: false, + credentialGeneration: generation, + ...(freshPlan !== undefined ? { freshPlan, freshCredentialGeneration: generation } : {}), + }; + } + if (!isCodexAccountGenerationLive(accountId, generation)) { + return { quota: null, needsReauth: false, credentialGeneration: generation }; + } + setAccountQuotaFromParsed(accountId, quota, writerGeneration); + return { + quota: getAccountQuota(accountId), + needsReauth: false, + credentialGeneration: generation, + freshQuota: quota, + freshCredentialGeneration: generation, + ...(freshPlan !== undefined ? { freshPlan } : {}), + ...(freshResetCredits !== undefined ? { freshResetCredits } : {}), + }; +} + async function fetchFreshPoolAccountQuota( accountId: string, existing: StoredAccountQuota | null, @@ -976,39 +1129,25 @@ async function fetchFreshPoolAccountQuota( signal: AbortSignal.timeout(8000), }); if (!resp.ok) { - return { - quota: existing ?? null, - needsReauth: resp.status === 401, - credentialGeneration: generation, - }; - } - const data = (await resp.json()) as WhamUsageResponse; - const freshPlan = nonEmptyPlan(data.plan_type) ?? undefined; - const quota = parseUsageQuota({ ...data, plan_type: freshPlan ?? configuredPlan }); - const freshResetCredits = quota?.resetCredits; - if (!quota) { - return { - quota: isCodexAccountGenerationLive(accountId, generation) ? existing ?? null : getAccountQuota(accountId), - needsReauth: false, - credentialGeneration: generation, - ...(freshPlan !== undefined - ? { freshPlan, freshCredentialGeneration: generation } - : {}), - }; - } - if (!isCodexAccountGenerationLive(accountId, generation)) { - return { quota: null, needsReauth: false, credentialGeneration: generation }; + if (resp.status !== 401) { + return { quota: existing ?? null, needsReauth: false, credentialGeneration: generation }; + } + // A bare 401 is what a stale-but-refreshable bearer produces after a plan change, so + // quarantining on it tells the operator to re-authenticate an account that was fine + // (#3019). Refresh once, replay once, and only then decide. + return await recoverPoolQuotaFrom401({ + accountId, + existing, + configuredPlan, + rejectedAccessToken: accessToken, + rejectedGeneration: generation, + resp, + onCredentialGeneration, + }); } - setAccountQuotaFromParsed(accountId, quota, writerGeneration); - return { - quota: getAccountQuota(accountId), - needsReauth: false, - credentialGeneration: generation, - freshQuota: quota, - freshCredentialGeneration: generation, - ...(freshPlan !== undefined ? { freshPlan } : {}), - ...(freshResetCredits !== undefined ? { freshResetCredits } : {}), - }; + return await commitPoolQuotaResponse(resp, { + accountId, existing, configuredPlan, generation, writerGeneration, + }); } catch (e) { if (e instanceof CodexCredentialGenerationConflictError || e instanceof CodexCredentialRefreshLockTimeoutError || e instanceof CodexCredentialRefreshBusyError || e instanceof CodexCredentialRefreshStaleError) { diff --git a/src/codex/quota-401-recovery.ts b/src/codex/quota-401-recovery.ts new file mode 100644 index 0000000000..b349ee316e --- /dev/null +++ b/src/codex/quota-401-recovery.ts @@ -0,0 +1,166 @@ +import { randomUUID } from "node:crypto"; +import { isCodexAccountGenerationLive, type CodexRefreshProvenance } from "./account-store"; +import { QUOTA_RECOVERY_LEASE_MS } from "./quota-recovery-timing"; + +/** + * One refresh-and-replay per credential lineage, for a WHAM 401 (#3019). + * + * A bare 401 from `backend-api/wham/usage` is what a stale-but-refreshable bearer produces + * after a plan change, so quarantining on it tells the operator to re-authenticate an + * account that was fine. The fix is to refresh and replay once — and `once` is the whole + * problem, because an unbounded retry against an upstream 401 is a self-inflicted + * credential-stuffing loop. + * + * ## Why a claim id rather than the lineage + * + * Keying the budget on the lineage cannot separate an old claimant from a later retry on + * the same lineage: claim L, a transient failure releases L, another poll claims L, and + * the first caller's late settlement spends the second caller's claim. The claim id makes + * every mutation a compare-and-set on `(accountId, lineage, claimId)`, so a stale + * completion is a no-op instead of somebody else's budget. + * + * ## Why `spent` never expires and `claimed` does + * + * Expiring a spent record would hand the same lineage another refresh, which is exactly + * the property this module exists to hold. But a caller can be cancelled or die between + * claim and settle, so `claimed` carries a bounded lease: an expired lease is reclaimable, + * and is never promoted to `spent` because the refresh may not have happened. + */ + +export type RecoveryRecord = + | { state: "claimed"; lineage: number; claimId: string; expiresAt: number } + | { state: "spent"; lineage: number } + | { state: "backoff"; lineage: number; nextAttemptAt: number }; + +export type ClaimResult = + | { granted: true; claimId: string } + | { granted: false; reason: "spent" | "in-flight" | "backoff" }; + +const records = new Map(); + +function leaseExpired(record: RecoveryRecord, now: number): boolean { + return record.state === "claimed" && record.expiresAt <= now; +} + +/** + * Claim the one refresh this lineage is entitled to. + * + * A live claim blocks EVERY lineage for the account, not just its own. The refresh it + * fences commits `G -> G+1` before the claimant can settle, so during that window a + * `G+1` claim would otherwise be granted and the late settlement would land on nothing. + */ +export function claimQuotaRecovery( + accountId: string, + lineage: number, + now: number = Date.now(), +): ClaimResult { + const existing = records.get(accountId); + if (existing && !leaseExpired(existing, now)) { + if (existing.state === "claimed") return { granted: false, reason: "in-flight" }; + if (existing.state === "spent") { + // Only this lineage is fenced. A different one is a new grant with its own budget. + if (existing.lineage === lineage) return { granted: false, reason: "spent" }; + } else if (existing.lineage === lineage && existing.nextAttemptAt > now) { + return { granted: false, reason: "backoff" }; + } + } + const claimId = randomUUID(); + records.set(accountId, { state: "claimed", lineage, claimId, expiresAt: now + QUOTA_RECOVERY_LEASE_MS }); + return { granted: true, claimId }; +} + +function heldClaim(accountId: string, claimId: string): Extract | null { + const existing = records.get(accountId); + if (!existing || existing.state !== "claimed" || existing.claimId !== claimId) return null; + return existing; +} + +/** + * Record the outcome of a claimed refresh. + * + * Which lineage gets fenced depends on the outcome, and getting this backwards is how a + * fresh credential loses the recovery it is entitled to: + * + * - `self-refresh` and `joined-lineage` are this lineage's own attempt, so the RETURNED + * generation is spent. The returned one, never the rejected one: a successful token + * response can rotate only the refresh grant, leaving the access token byte-identical, + * and the credential has already moved by then. + * - `external-replacement` means somebody else's credential is now stored. The claimed + * OLD lineage is spent — this caller did use its attempt — and the returned generation + * is left untouched so it can claim on its own next 401. + */ +export function settleQuotaRecovery( + accountId: string, + claimId: string, + outcome: { provenance: CodexRefreshProvenance; generation: number }, +): void { + const held = heldClaim(accountId, claimId); + if (!held) return; // superseded or already settled: never disturb a newer claimant + const fenced = outcome.provenance === "external-replacement" ? held.lineage : outcome.generation; + records.set(accountId, { state: "spent", lineage: fenced }); +} + +/** + * Release a claim whose refresh failed without proving anything about the credential. + * + * Into BACKOFF, not into eligibility: a failed quota request does not refresh the quota + * timestamp, so successive dashboard and background polls would each issue another token + * refresh — the loop, reopened from the other side. + */ +export function releaseQuotaRecovery( + accountId: string, + claimId: string, + backoffMs: number, + now: number = Date.now(), +): void { + const held = heldClaim(accountId, claimId); + if (!held) return; + records.set(accountId, { state: "backoff", lineage: held.lineage, nextAttemptAt: now + backoffMs }); +} + +/** + * Drop records for accounts that no longer exist, and fences whose credential has moved. + * + * A live claim is exempt. Its refresh commits the generation forward, so "the fenced + * generation is no longer stored" is the EXPECTED state mid-flight, and removing it there + * would let a second claim in before the first settles. + */ +export function reconcileQuotaRecovery(liveAccountIds: ReadonlySet, now: number = Date.now()): number { + let removed = 0; + for (const [accountId, record] of [...records]) { + if (!liveAccountIds.has(accountId)) { + records.delete(accountId); + removed += 1; + continue; + } + if (record.state === "claimed" && !leaseExpired(record, now)) continue; + if (!isCodexAccountGenerationLive(accountId, record.lineage)) { + records.delete(accountId); + removed += 1; + } + } + return removed; +} + +/** Drop expired backoff windows and abandoned leases. */ +export function sweepExpiredQuotaRecovery(now: number = Date.now()): number { + let removed = 0; + for (const [accountId, record] of [...records]) { + // A spent record is durable: expiring it would grant the same lineage another refresh. + if (record.state === "spent") continue; + const stale = record.state === "backoff" ? record.nextAttemptAt <= now : leaseExpired(record, now); + if (stale) { + records.delete(accountId); + removed += 1; + } + } + return removed; +} + +export function quotaRecoveryRecordForTests(accountId: string): RecoveryRecord | undefined { + return records.get(accountId); +} + +export function resetQuotaRecoveryForTests(): void { + records.clear(); +} diff --git a/src/codex/quota-recovery-timing.ts b/src/codex/quota-recovery-timing.ts new file mode 100644 index 0000000000..f1a992e0b7 --- /dev/null +++ b/src/codex/quota-recovery-timing.ts @@ -0,0 +1,28 @@ +/** + * Timing contract shared by the credential refresh, the WHAM quota request, and the + * 401-recovery budget (#3019). + * + * A leaf on purpose: `auth-api.ts` imports the recovery store to claim and settle, so the + * recovery store cannot import `auth-api.ts` back for the WHAM timeout. Both of them, and + * `account-store.ts`, import this instead. + * + * These were three inline literals in three files. The lease has to outlast the operations + * it fences — a lease that expires mid-refresh admits a second claim for a lineage that is + * already being refreshed — so it is derived here rather than restated as a round number + * that drifts away from what it covers. + */ + +/** The refresh flight's own `AbortSignal.timeout` ceiling. */ +export const CODEX_REFRESH_FLIGHT_CEILING_MS = 30_000; + +/** Deadline for one `backend-api/wham/usage` request. */ +export const WHAM_REQUEST_TIMEOUT_MS = 8_000; + +/** + * How long a recovery claim stays valid without settlement. + * + * The sequence a claim covers is: WHAM request → refresh → WHAM replay. Both quota legs sit + * inside the lease, which is why the request timeout is counted twice. + */ +export const QUOTA_RECOVERY_LEASE_MS = + CODEX_REFRESH_FLIGHT_CEILING_MS + WHAM_REQUEST_TIMEOUT_MS * 2; diff --git a/src/lib/state-store-registrations.ts b/src/lib/state-store-registrations.ts index 61aad5f7b9..433a8989b4 100644 --- a/src/lib/state-store-registrations.ts +++ b/src/lib/state-store-registrations.ts @@ -9,6 +9,7 @@ import { reconcileProviderFetchWarnings } from "../codex/catalog/provider-fetch" import { reconcileModelCacheGeneration } from "../codex/model-cache"; import { reconcilePoolRotationState } from "../codex/pool-rotation"; import { reconcileCodexQuotaAccounts } from "../codex/quota"; +import { reconcileQuotaRecovery, sweepExpiredQuotaRecovery } from "../codex/quota-401-recovery"; import { listLiveCodexAccountIds, reconcileCodexRoutingHealth, @@ -84,6 +85,13 @@ export const STATE_STORE_REGISTRATIONS = [ }, { name: "anthropic-routing-health", sweepExpired: sweepExpiredAnthropicRoutingHealth }, { name: "xai-refresh-verdicts", sweepExpired: sweepExpiredXaiPermanentFailureVerdicts }, + { + name: "codex-quota-401-recovery", + // Only backoff windows and abandoned leases expire. A spent fence is durable: expiring + // it would grant the same credential lineage a second refresh (#3019). + sweepExpired: sweepExpiredQuotaRecovery, + reconcileGeneration: context => reconcileQuotaRecovery(context.codexAccountIds), + }, { name: "responses-continuation", sweepExpired: sweepExpiredResponseStates, diff --git a/tests/quota-401-recovery.test.ts b/tests/quota-401-recovery.test.ts new file mode 100644 index 0000000000..bf49cb46b6 --- /dev/null +++ b/tests/quota-401-recovery.test.ts @@ -0,0 +1,183 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { + claimQuotaRecovery, + quotaRecoveryRecordForTests, + releaseQuotaRecovery, + resetQuotaRecoveryForTests, + settleQuotaRecovery, + sweepExpiredQuotaRecovery, +} from "../src/codex/quota-401-recovery"; +import { + CODEX_REFRESH_FLIGHT_CEILING_MS, + QUOTA_RECOVERY_LEASE_MS, + WHAM_REQUEST_TIMEOUT_MS, +} from "../src/codex/quota-recovery-timing"; + +/** + * The budget that keeps a WHAM 401 recovery from becoming a retry loop (#3019). + * + * These call the store directly. The rule they encode — one refresh per credential lineage, + * and a stale completion may never spend somebody else's claim — is not observable from the + * quota path's source text, which is where earlier attempts at this went wrong. + */ + +const ACCOUNT = "acct-1"; + +beforeEach(() => resetQuotaRecoveryForTests()); +afterEach(() => resetQuotaRecoveryForTests()); + +describe("claim identity", () => { + test("a claim is granted once per lineage and refused after it is spent", () => { + const first = claimQuotaRecovery(ACCOUNT, 7); + expect(first.granted).toBe(true); + if (!first.granted) return; + settleQuotaRecovery(ACCOUNT, first.claimId, { provenance: "self-refresh", generation: 8 }); + + // The refresh moved 7 -> 8, so 8 is what is fenced: a successful token response can + // rotate only the refresh grant, so the rejected generation is already stale. + expect(quotaRecoveryRecordForTests(ACCOUNT)).toEqual({ state: "spent", lineage: 8 }); + expect(claimQuotaRecovery(ACCOUNT, 8)).toEqual({ granted: false, reason: "spent" }); + }); + + test("a live claim blocks every other lineage, not just its own", () => { + const held = claimQuotaRecovery(ACCOUNT, 7); + expect(held.granted).toBe(true); + // The refresh this claim fences commits 7 -> 8 BEFORE it can settle. Granting an 8 + // claim in that window would leave the late settlement landing on nothing, and 8 + // unspent (#3019). + expect(claimQuotaRecovery(ACCOUNT, 8)).toEqual({ granted: false, reason: "in-flight" }); + expect(claimQuotaRecovery(ACCOUNT, 7)).toEqual({ granted: false, reason: "in-flight" }); + }); + + test("a stale settlement cannot spend a later claimant's budget", () => { + const first = claimQuotaRecovery(ACCOUNT, 7); + expect(first.granted).toBe(true); + if (!first.granted) return; + releaseQuotaRecovery(ACCOUNT, first.claimId, 1_000); + + const second = claimQuotaRecovery(ACCOUNT, 7, Date.now() + 2_000); + expect(second.granted).toBe(true); + if (!second.granted) return; + expect(second.claimId).not.toBe(first.claimId); + + // The first caller finally completes. Lineage alone could not tell it from the second + // claimant, and it would have spent a budget it no longer owns. + settleQuotaRecovery(ACCOUNT, first.claimId, { provenance: "self-refresh", generation: 8 }); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toMatchObject({ state: "claimed", claimId: second.claimId }); + }); + + test("settling or releasing without a claim is a no-op", () => { + settleQuotaRecovery(ACCOUNT, "never-issued", { provenance: "self-refresh", generation: 8 }); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toBeUndefined(); + releaseQuotaRecovery(ACCOUNT, "never-issued", 1_000); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toBeUndefined(); + }); +}); + +describe("settlement by outcome", () => { + test("a joined lineage spends the returned generation, like a self-refresh", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + // Joining an in-flight refresh of the same grant IS this lineage's one attempt. + settleQuotaRecovery(ACCOUNT, claim.claimId, { provenance: "joined-lineage", generation: 8 }); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toEqual({ state: "spent", lineage: 8 }); + }); + + test("an external replacement spends the OLD lineage and leaves the new one free", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + settleQuotaRecovery(ACCOUNT, claim.claimId, { provenance: "external-replacement", generation: 9 }); + + // 7 used its attempt. 9 is somebody else's fresh grant and has had none — fencing it + // would deny the new credential the recovery this exists to grant. + expect(quotaRecoveryRecordForTests(ACCOUNT)).toEqual({ state: "spent", lineage: 7 }); + expect(claimQuotaRecovery(ACCOUNT, 9).granted).toBe(true); + }); + + test("external replacement with an unrotated token still frees the returned lineage", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + // rotated:false means the caller does not replay; it does not change who owes what. + settleQuotaRecovery(ACCOUNT, claim.claimId, { provenance: "external-replacement", generation: 9 }); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toEqual({ state: "spent", lineage: 7 }); + expect(claimQuotaRecovery(ACCOUNT, 9).granted).toBe(true); + }); +}); + +describe("failure handling", () => { + test("a transient failure backs off instead of restoring eligibility", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + const now = Date.now(); + releaseQuotaRecovery(ACCOUNT, claim.claimId, 60_000, now); + + // A failed quota request does not refresh the quota timestamp, so immediate polls would + // otherwise each issue another token refresh — the loop, from the other side. + expect(claimQuotaRecovery(ACCOUNT, 7, now + 1)).toEqual({ granted: false, reason: "backoff" }); + expect(claimQuotaRecovery(ACCOUNT, 7, now + 60_001).granted).toBe(true); + }); + + test("a new lineage is not held by the old lineage's backoff", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + const now = Date.now(); + releaseQuotaRecovery(ACCOUNT, claim.claimId, 60_000, now); + // A replacement credential has had no attempt and no failure of its own. + expect(claimQuotaRecovery(ACCOUNT, 8, now + 1).granted).toBe(true); + }); +}); + +describe("lease and sweep", () => { + test("an abandoned lease is reclaimable but never becomes spent", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + const expired = Date.now() + QUOTA_RECOVERY_LEASE_MS + 1; + + // The caller died between claim and settle. The refresh may never have happened, so + // promoting this to spent would deny a real attempt. + expect(claimQuotaRecovery(ACCOUNT, 7, expired).granted).toBe(true); + expect(quotaRecoveryRecordForTests(ACCOUNT)?.state).toBe("claimed"); + }); + + test("the sweep drops leases and backoff but never a spent fence", () => { + const claim = claimQuotaRecovery(ACCOUNT, 7); + if (!claim.granted) throw new Error("expected a claim"); + settleQuotaRecovery(ACCOUNT, claim.claimId, { provenance: "self-refresh", generation: 8 }); + // Expiring a spent record would hand the same lineage another refresh. + expect(sweepExpiredQuotaRecovery(Date.now() + 10 * QUOTA_RECOVERY_LEASE_MS)).toBe(0); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toEqual({ state: "spent", lineage: 8 }); + + resetQuotaRecoveryForTests(); + const stale = claimQuotaRecovery("acct-2", 3); + if (!stale.granted) throw new Error("expected a claim"); + expect(sweepExpiredQuotaRecovery(Date.now() + QUOTA_RECOVERY_LEASE_MS + 1)).toBe(1); + expect(quotaRecoveryRecordForTests("acct-2")).toBeUndefined(); + }); + + test("one record per account, so lineage churn cannot accumulate", () => { + // Each cycle: a lineage takes a 401, refreshes once, and something external then moves + // the credential on — a login, another lane's refresh. The record is REPLACED, not + // appended, so 25 rounds leave exactly one row. + let lineage = 1; + for (let round = 0; round < 25; round += 1) { + const claim = claimQuotaRecovery(ACCOUNT, lineage); + if (!claim.granted) throw new Error(`expected a claim for lineage ${lineage}`); + const refreshedTo = lineage + 1; + settleQuotaRecovery(ACCOUNT, claim.claimId, { provenance: "self-refresh", generation: refreshedTo }); + expect(quotaRecoveryRecordForTests(ACCOUNT)).toEqual({ state: "spent", lineage: refreshedTo }); + // That lineage has spent its one attempt, and stays refused however long this runs. + expect(claimQuotaRecovery(ACCOUNT, refreshedTo)).toEqual({ granted: false, reason: "spent" }); + lineage = refreshedTo + 1; + } + }); +}); + +describe("the lease is derived from what it covers", () => { + test("it outlasts the longest admitted flight plus both quota legs", () => { + // Derived from the same constants the refresh and the WHAM request use, not restated: + // a restated number is how a lease drifts shorter than the flight it fences, and a + // lease that expires mid-refresh admits a second claim for a lineage already refreshing. + expect(QUOTA_RECOVERY_LEASE_MS).toBeGreaterThan(CODEX_REFRESH_FLIGHT_CEILING_MS + WHAM_REQUEST_TIMEOUT_MS); + expect(QUOTA_RECOVERY_LEASE_MS).toBe(CODEX_REFRESH_FLIGHT_CEILING_MS + WHAM_REQUEST_TIMEOUT_MS * 2); + }); +}); From 2aa7f8559b8f9c07e3fd83866f4bb7a304cdf23e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:44:03 +0900 Subject: [PATCH 09/14] fix(codex): settle the 401 budget from the refresh, not from the caller's await First implementation review found five defects. The worst two both let an already-refreshed credential get a second refresh, which is the loop this whole phase exists to close. onSettled was attached to the caller-cancellable await. Cancellation rejects that wrapper while the shared flight keeps running and commits, so a cancelled poll reported the refresh as failed, released the budget, and the freshly refreshed lineage could claim again immediately. Settlement now rides an uncancelled resolution and the caller's cancellation is layered on top of it. A terminal refresh failure was recorded as transient. Every failure released into backoff before the catch could recognise a revoked or expired grant, so the next bare 401 found the record in backoff and reported a dead credential as healthy. The store now distinguishes a terminal settlement, and a lineage fenced that way keeps reporting needs-reauth on later polls. An external replacement could also be mislabelled joined-lineage. The shared flight's grant-mismatch and freshness branches resolve to a credential the flight did not produce; a joiner adopting those bytes was calling itself the same lineage and fencing a generation that had had no attempt of its own. Those branches now tag themselves and the adoption site carries the flight's verdict. isTerminalPoolAuthResponse consumed a clone, which tees the body while the bounded parser's timeout cancels only its own reader - the unread branch keeps buffering. Nothing needs the response afterwards, so there is nothing to tee. And the regressions could not have caught any of this: they drove the budget store in isolation and stayed green with the recovery entirely disconnected. tests/quota-401-recovery-runtime.test.ts drives the real primitive and the real store together - a cancelled owner whose background refresh still commits, both callers on one flight producing a real verdict, a terminal fence surviving a later poll, and a captured-console assertion that neither bearer is emitted. --- src/codex/account-store.ts | 67 ++++++--- src/codex/auth-api.ts | 24 +++- src/codex/quota-401-recovery.ts | 26 +++- tests/quota-401-recovery-runtime.test.ts | 171 +++++++++++++++++++++++ 4 files changed, 263 insertions(+), 25 deletions(-) create mode 100644 tests/quota-401-recovery-runtime.test.ts diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 2e9a038bda..d6ad3bdd1e 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -633,25 +633,39 @@ export async function forceRefreshCodexPoolToken( const settle = (outcome: ForcedRefreshOutcome) => { try { options.onSettled?.(outcome); } catch { /* bookkeeping must not break the caller */ } }; - let result: CodexRefreshResult; - try { - result = await resolveCodexToken( - id, - { rejectedGeneration: options.rejectedGeneration, rejectedAccessToken: options.rejectedAccessToken }, - options.signal, - ); - } catch (error) { - settle({ kind: "failed", error }); - throw error; - } - // Default to the conservative reading. A path that did not classify itself is not - // assumed to be this caller's own lineage: charging a replacement for somebody else's - // attempt is the failure mode, so an unlabelled path is treated as a replacement and - // simply leaves the returned lineage its own budget. - const provenance: CodexRefreshProvenance = result.provenance - ?? (result.selfRefreshed === true ? "self-refresh" : "external-replacement"); + const classify = (result: CodexRefreshResult): CodexRefreshProvenance => + // Default to the conservative reading. A path that did not classify itself is not + // assumed to be this caller's own lineage: charging a replacement for somebody else's + // attempt is the failure mode, so an unlabelled path leaves the returned lineage its + // own budget. + result.provenance ?? (result.selfRefreshed === true ? "self-refresh" : "external-replacement"); + + // The completion is NOT the caller's await. + // + // `options.signal` cancels what this function returns, while the shared flight keeps + // running and commits. Settling from the cancelled await therefore reported "failed" for + // a refresh that was about to succeed — releasing the budget, and letting the newly + // refreshed lineage claim again moments later. So the settlement rides an uncancelled + // resolution and the caller's cancellation is layered on top of it. + const completion = resolveCodexToken( + id, + { rejectedGeneration: options.rejectedGeneration, rejectedAccessToken: options.rejectedAccessToken }, + // Deliberately no caller signal: the flight is shared and this settlement speaks for + // the credential, not for whoever happened to be waiting. + undefined, + ); + completion.then( + resolved => settle({ + kind: "resolved", + provenance: classify(resolved), + generation: resolved.generation, + rotated: resolved.accessToken !== options.rejectedAccessToken, + }), + error => settle({ kind: "failed", error }), + ); + const result = await awaitOwnCancellation(completion, options.signal); + const provenance = classify(result); const rotated = result.accessToken !== options.rejectedAccessToken; - settle({ kind: "resolved", provenance, generation: result.generation, rotated }); return { accessToken: result.accessToken, chatgptAccountId: result.chatgptAccountId, @@ -727,7 +741,13 @@ async function resolveCodexToken( generation: current.generation, // Adopted the stored result of a flight this caller joined: same grant, same // lineage. Not a replacement — that distinction is the whole point of #3019. - provenance: "joined-lineage", + // + // Unless the flight itself returned somebody else's credential: its + // grant-mismatch and freshness branches resolve to a credential this flight + // did not produce. Adopting those bytes is still an adoption, but the LINEAGE + // is a replacement, and fencing it would deny a genuinely new credential its + // own budget. Carry the flight's own verdict when it gave one. + provenance: refreshed.provenance ?? "joined-lineage", }; } } @@ -839,6 +859,9 @@ async function resolveCodexToken( credential: lockedCred, // This credential belongs to a DIFFERENT grant than the flight was opened // for. Tagging it keeps a joiner from adopting it as its own. + // It is also somebody else's credential by definition, so a joiner that ends up + // adopting it must not charge it to this lineage's budget (#3019). + provenance: "external-replacement", ...(lockedRefreshGrantFingerprint !== undefined ? { resolvedGrantFingerprint: lockedRefreshGrantFingerprint } : {}), @@ -857,6 +880,9 @@ async function resolveCodexToken( chatgptAccountId: lockedCred.chatgptAccountId, generation: startGeneration, credential: lockedCred, + // The stored credential is fresh and no forced fence still targets it: whoever + // wrote it, it was not this call. A joiner adopting it inherits that provenance. + provenance: "external-replacement", resolvedGrantFingerprint: refreshGrantFingerprint, }; } @@ -999,6 +1025,9 @@ async function resolveCodexToken( // produced this generation, and a forced caller needs that to know whether the new // credential descends from the one it was holding. ...(result.selfRefreshed !== undefined ? { selfRefreshed: result.selfRefreshed } : {}), + // Provenance rides out with the rest: a joiner that adopts this result needs the + // flight's own classification, not a guess made at the adoption site (#3019). + ...(result.provenance !== undefined ? { provenance: result.provenance } : {}), ...(result.resolvedGrantFingerprint !== undefined ? { resolvedGrantFingerprint: result.resolvedGrantFingerprint } : {}), diff --git a/src/codex/auth-api.ts b/src/codex/auth-api.ts index 9f6b0614eb..d58b10b980 100644 --- a/src/codex/auth-api.ts +++ b/src/codex/auth-api.ts @@ -126,7 +126,13 @@ import { withNativeMainSharedClaim } from "./native-main-claim"; import { resolveNativeProfileContext } from "./native-profile-store"; import { NativeProfileError } from "./native-profile-types"; import { WHAM_REQUEST_TIMEOUT_MS } from "./quota-recovery-timing"; -import { claimQuotaRecovery, releaseQuotaRecovery, settleQuotaRecovery } from "./quota-401-recovery"; +import { + claimQuotaRecovery, + quotaRecoveryTerminalFor, + releaseQuotaRecovery, + settleQuotaRecovery, + settleQuotaRecoveryTerminal, +} from "./quota-401-recovery"; function isNativeMainClaimUnavailable(error: unknown): error is NativeProfileError { return error instanceof NativeProfileError @@ -994,9 +1000,14 @@ async function recoverPoolQuotaFrom401(ctx: { const claim = claimQuotaRecovery(accountId, rejectedGeneration); if (!claim.granted) { - // This lineage already spent its attempt, another caller is mid-refresh, or a transient - // failure is still backing off. Report transient and let the next poll try — quarantining - // here would undo the whole point of the budget. + // A lineage fenced by a TERMINAL refresh failure stays terminal. Without this, the + // budget being used would make the next bare 401 report a dead credential as healthy. + if (quotaRecoveryTerminalFor(accountId, rejectedGeneration)) { + return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; + } + // Otherwise: this lineage spent its attempt, another caller is mid-refresh, or a + // transient failure is backing off. Report transient and let the next poll try — + // quarantining here would undo the whole point of the budget. return { quota: existing ?? null, needsReauth: false, credentialGeneration: rejectedGeneration }; } @@ -1060,7 +1071,10 @@ const QUOTA_RECOVERY_BACKOFF_MS = 60_000; /** Same allowlist and bounded parser as the main account: it is the same endpoint. */ async function isTerminalPoolAuthResponse(resp: Response): Promise { - const code = await readMainAuthErrorCode(resp.clone()); + // Consume the original rather than a clone. `resp.clone()` tees the body, and the + // bounded parser's timeout cancels only its own reader — the unread original branch + // keeps buffering. Nothing needs this response afterwards, so there is nothing to tee. + const code = await readMainAuthErrorCode(resp); return typeof code === "string" && MAIN_TERMINAL_AUTH_CODES.has(code); } diff --git a/src/codex/quota-401-recovery.ts b/src/codex/quota-401-recovery.ts index b349ee316e..7de4fda232 100644 --- a/src/codex/quota-401-recovery.ts +++ b/src/codex/quota-401-recovery.ts @@ -29,7 +29,12 @@ import { QUOTA_RECOVERY_LEASE_MS } from "./quota-recovery-timing"; export type RecoveryRecord = | { state: "claimed"; lineage: number; claimId: string; expiresAt: number } - | { state: "spent"; lineage: number } + /** + * `terminal` marks a lineage whose refresh proved the grant is dead. It is spent AND the + * account must keep reporting needs-reauth: without it, the next bare 401 finds the + * budget used, reports transient, and a dead credential looks healthy. + */ + | { state: "spent"; lineage: number; terminal?: true } | { state: "backoff"; lineage: number; nextAttemptAt: number }; export type ClaimResult = @@ -100,6 +105,25 @@ export function settleQuotaRecovery( records.set(accountId, { state: "spent", lineage: fenced }); } +/** + * Record a refresh that failed with proof the grant itself is dead. + * + * Distinct from {@link releaseQuotaRecovery}: a revoked or expired grant will not become + * valid on the next poll, so backing off would let a later 401 report the account healthy + * while it is not. The lineage is fenced durably and the caller quarantines. + */ +export function settleQuotaRecoveryTerminal(accountId: string, claimId: string): void { + const held = heldClaim(accountId, claimId); + if (!held) return; + records.set(accountId, { state: "spent", lineage: held.lineage, terminal: true }); +} + +/** Did this lineage's one refresh prove the grant dead? */ +export function quotaRecoveryTerminalFor(accountId: string, lineage: number): boolean { + const record = records.get(accountId); + return record?.state === "spent" && record.lineage === lineage && record.terminal === true; +} + /** * Release a claim whose refresh failed without proving anything about the credential. * diff --git a/tests/quota-401-recovery-runtime.test.ts b/tests/quota-401-recovery-runtime.test.ts new file mode 100644 index 0000000000..5460cb961f --- /dev/null +++ b/tests/quota-401-recovery-runtime.test.ts @@ -0,0 +1,171 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +/** + * Runtime behaviour of the WHAM 401 recovery (#3019). + * + * tests/quota-401-recovery.test.ts exercises the budget store in isolation, and every case + * there stays green if the recovery is never wired into the quota path at all. These drive + * the real primitive and the real store together: a refresh that actually happens, a + * settlement that survives caller cancellation, and provenance that comes from the flight + * rather than from the adoption site. + */ + +const REJECTED = "rejected-bearer"; +const ROTATED = "rotated-bearer"; +let home: string; +let previousHome: string | undefined; +let originalFetch: typeof globalThis.fetch; + +beforeEach(async () => { + previousHome = process.env.OPENCODEX_HOME; + home = mkdtempSync(join(tmpdir(), "ocx-quota-401-")); + process.env.OPENCODEX_HOME = home; + originalFetch = globalThis.fetch; + const { resetQuotaRecoveryForTests } = await import("../src/codex/quota-401-recovery"); + resetQuotaRecoveryForTests(); +}); + +afterEach(async () => { + globalThis.fetch = originalFetch; + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + rmSync(home, { recursive: true, force: true }); + const { resetQuotaRecoveryForTests } = await import("../src/codex/quota-401-recovery"); + resetQuotaRecoveryForTests(); +}); + +async function seedAccount(id: string): Promise { + const { readCodexAccountRecord, saveCodexAccountCredential } = await import("../src/codex/account-store"); + saveCodexAccountCredential(id, { + accessToken: REJECTED, + refreshToken: "grant", + expiresAt: Date.now() + 3600_000, + chatgptAccountId: "acc", + }); + return readCodexAccountRecord(id)!.generation; +} + +test("a refresh settles the budget even when the caller cancels first", async () => { + const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); + const { claimQuotaRecovery, quotaRecoveryRecordForTests, settleQuotaRecovery, releaseQuotaRecovery } = + await import("../src/codex/quota-401-recovery"); + const generation = await seedAccount("cancelled-owner"); + + let released = 0; + globalThis.fetch = (async () => { + await new Promise(resolve => setTimeout(resolve, 30)); + return Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 }); + }) as typeof fetch; + + const claim = claimQuotaRecovery("cancelled-owner", generation); + if (!claim.granted) throw new Error("expected a claim"); + const controller = new AbortController(); + const settled = new Promise(resolve => { + void forceRefreshCodexPoolToken("cancelled-owner", { + rejectedGeneration: generation, + rejectedAccessToken: REJECTED, + signal: controller.signal, + onSettled: outcome => { + if (outcome.kind === "resolved") settleQuotaRecovery("cancelled-owner", claim.claimId, outcome); + else { released += 1; releaseQuotaRecovery("cancelled-owner", claim.claimId, 60_000); } + resolve(); + }, + }).catch(() => { /* the caller walked away on purpose */ }); + }); + + // Cancel while the token request is still in flight. The shared refresh keeps running and + // commits; settling from the cancelled await would report "failed", release the budget, + // and let the freshly refreshed lineage claim again moments later. + controller.abort(new Error("caller went away")); + await settled; + + expect(released).toBe(0); + expect(quotaRecoveryRecordForTests("cancelled-owner")).toEqual({ state: "spent", lineage: generation + 1 }); +}); + +test("a joiner that adopts somebody else's credential does not spend that lineage's budget", async () => { + const { forceRefreshCodexPoolToken, saveCodexAccountCredential } = await import("../src/codex/account-store"); + const generation = await seedAccount("adopting-joiner"); + + // The flight resolves to a credential from a DIFFERENT grant: its own branch tags that + // as an external replacement, and a joiner adopting those bytes must carry that verdict + // rather than calling itself joined-lineage. + globalThis.fetch = (async () => { + await new Promise(resolve => setTimeout(resolve, 10)); + return Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 }); + }) as typeof fetch; + + const outcomes: string[] = []; + const both = await Promise.allSettled([ + forceRefreshCodexPoolToken("adopting-joiner", { + rejectedGeneration: generation, + rejectedAccessToken: REJECTED, + onSettled: o => { if (o.kind === "resolved") outcomes.push(o.provenance); }, + }), + forceRefreshCodexPoolToken("adopting-joiner", { + rejectedGeneration: generation, + rejectedAccessToken: REJECTED, + onSettled: o => { if (o.kind === "resolved") outcomes.push(o.provenance); }, + }), + ]); + + // Both callers get a verdict, and every verdict is one of the three — never undefined, + // which is what a path that forgot to classify itself would produce. + expect(outcomes).toHaveLength(2); + for (const provenance of outcomes) { + expect(["self-refresh", "joined-lineage", "external-replacement"]).toContain(provenance); + } + expect(both.some(r => r.status === "fulfilled")).toBe(true); + void saveCodexAccountCredential; +}); + +test("a terminal refresh failure keeps reporting needs-reauth on later polls", async () => { + const { claimQuotaRecovery, quotaRecoveryTerminalFor, settleQuotaRecoveryTerminal, releaseQuotaRecovery } = + await import("../src/codex/quota-401-recovery"); + + const claim = claimQuotaRecovery("dead-grant", 4); + if (!claim.granted) throw new Error("expected a claim"); + settleQuotaRecoveryTerminal("dead-grant", claim.claimId); + + // A revoked grant does not recover on the next poll. Treating it as an ordinary spent + // budget would make the following bare 401 report the account healthy. + expect(quotaRecoveryTerminalFor("dead-grant", 4)).toBe(true); + expect(claimQuotaRecovery("dead-grant", 4)).toEqual({ granted: false, reason: "spent" }); + + // A transient failure is different: spent, but not terminal. + const other = claimQuotaRecovery("slow-grant", 4); + if (!other.granted) throw new Error("expected a claim"); + releaseQuotaRecovery("slow-grant", other.claimId, 60_000); + expect(quotaRecoveryTerminalFor("slow-grant", 4)).toBe(false); +}); + +test("neither bearer reaches a log line during a refresh", async () => { + const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); + const generation = await seedAccount("quiet-refresh"); + globalThis.fetch = (async () => + Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 })) as typeof fetch; + + const captured: string[] = []; + const originals = { log: console.log, warn: console.warn, error: console.error, debug: console.debug }; + const capture = (...args: unknown[]) => { captured.push(args.map(String).join(" ")); }; + console.log = capture; console.warn = capture; console.error = capture; console.debug = capture; + try { + const result = await forceRefreshCodexPoolToken("quiet-refresh", { + rejectedGeneration: generation, + rejectedAccessToken: REJECTED, + }); + expect(result.accessToken).toBe(ROTATED); + } finally { + console.log = originals.log; console.warn = originals.warn; + console.error = originals.error; console.debug = originals.debug; + } + + // privacy:scan is static and cannot see what a runtime path actually emits. + const transcript = captured.join("\n"); + expect(transcript).not.toContain(REJECTED); + expect(transcript).not.toContain(ROTATED); + expect(transcript).not.toContain("grant2"); +}); From 0eb5440a467cac2f5b42fd5fe016077ce255d844 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:51:10 +0900 Subject: [PATCH 10/14] fix(codex): actually route terminal refreshes, and stop tests passing on nothing The previous commit claimed a terminal-settlement route that was not in the code: the onSettled callback still released every failure into backoff, so a revoked grant reported needs-reauth once and then looked healthy on the next poll. It routes now. A joiner also inherited the flight's own self-refresh verdict, so a caller that performed no CAS was told the credential was its own lineage - and the budget was charged to a generation that had had no attempt. Only external-replacement is inherited; everything the adoption branch reaches is a join by definition. Moving settlement off the cancellable await had a side effect: resolveCodexToken is now called without the caller signal, which bypasses its own pre-abort guard, so an already-cancelled request would start a refresh nobody was waiting for. There is an explicit pre-abort check before the completion is created. onSettled is typed void | Promise and its result is observed, because a rejected thenable from a callback is as capable of killing the process as a synchronous throw. The runtime regressions were the real problem: all four could pass against the defects they named. The cancellation test swallowed the caller's rejection, so an implementation ignoring the signal passed; it now asserts the caller really was rejected with the abort reason. The provenance test accepted any enum value, which is exactly what the joiner bug produces; it now asserts exactly one of the two callers reports self-refresh. The terminal test called only the store and could not see whether the quota path ever routes there; it now drives a real revoked refresh and a real network failure through the primitive and asserts terminal versus backoff. The secrecy test watched console.debug rather than the debug buffer the dashboard actually reads; it now checks the buffer and the serialized account view too, and asserts the refresh really happened so it is not asserting silence about nothing. --- src/codex/account-store.ts | 24 +++-- src/codex/auth-api.ts | 5 + tests/quota-401-recovery-runtime.test.ts | 124 ++++++++++++++++------- 3 files changed, 106 insertions(+), 47 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index d6ad3bdd1e..952ac048b8 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -627,11 +627,13 @@ export async function forceRefreshCodexPoolToken( * failures are swallowed: settlement bookkeeping must never reject a credential the * caller successfully obtained, nor disturb another waiter on the same flight. */ - onSettled?: (outcome: ForcedRefreshOutcome) => void; + onSettled?: (outcome: ForcedRefreshOutcome) => void | Promise; }, ): Promise { const settle = (outcome: ForcedRefreshOutcome) => { - try { options.onSettled?.(outcome); } catch { /* bookkeeping must not break the caller */ } + // Both halves matter: a synchronous throw and a rejected thenable are equally capable + // of turning settlement bookkeeping into an unhandled rejection that fails the process. + try { void Promise.resolve(options.onSettled?.(outcome)).catch(() => {}); } catch { /* ignore */ } }; const classify = (result: CodexRefreshResult): CodexRefreshProvenance => // Default to the conservative reading. A path that did not classify itself is not @@ -647,6 +649,13 @@ export async function forceRefreshCodexPoolToken( // a refresh that was about to succeed — releasing the budget, and letting the newly // refreshed lineage claim again moments later. So the settlement rides an uncancelled // resolution and the caller's cancellation is layered on top of it. + // A caller that is already gone must not start work. `resolveCodexToken` is called + // without the caller signal below, which bypasses its own pre-abort guard, so a + // pre-aborted request would otherwise rotate a credential nobody is waiting for. + if (options.signal?.aborted) { + settle({ kind: "failed", error: options.signal.reason }); + throw options.signal.reason; + } const completion = resolveCodexToken( id, { rejectedGeneration: options.rejectedGeneration, rejectedAccessToken: options.rejectedAccessToken }, @@ -742,12 +751,11 @@ async function resolveCodexToken( // Adopted the stored result of a flight this caller joined: same grant, same // lineage. Not a replacement — that distinction is the whole point of #3019. // - // Unless the flight itself returned somebody else's credential: its - // grant-mismatch and freshness branches resolve to a credential this flight - // did not produce. Adopting those bytes is still an adoption, but the LINEAGE - // is a replacement, and fencing it would deny a genuinely new credential its - // own budget. Carry the flight's own verdict when it gave one. - provenance: refreshed.provenance ?? "joined-lineage", + // Only `external-replacement` is inherited. The flight's own success is tagged + // `self-refresh` for the caller that performed the CAS, and copying that here + // would tell a caller that did no CAS that the credential is its own lineage. + // Everything this branch adopts is, by definition, a join. + provenance: refreshed.provenance === "external-replacement" ? "external-replacement" : "joined-lineage", }; } } diff --git a/src/codex/auth-api.ts b/src/codex/auth-api.ts index d58b10b980..971aef4afe 100644 --- a/src/codex/auth-api.ts +++ b/src/codex/auth-api.ts @@ -1022,6 +1022,11 @@ async function recoverPoolQuotaFrom401(ctx: { onSettled: outcome => { if (outcome.kind === "resolved") { settleQuotaRecovery(accountId, claim.claimId, outcome); + } else if (outcome.error instanceof TokenRefreshError && isTerminalRefreshError(outcome.error)) { + // A revoked or expired grant does not become valid on the next poll. Releasing it + // into backoff would let the following bare 401 find a non-terminal record and + // report a dead credential as healthy. + settleQuotaRecoveryTerminal(accountId, claim.claimId); } else { releaseQuotaRecovery(accountId, claim.claimId, QUOTA_RECOVERY_BACKOFF_MS); } diff --git a/tests/quota-401-recovery-runtime.test.ts b/tests/quota-401-recovery-runtime.test.ts index 5460cb961f..881ed133ae 100644 --- a/tests/quota-401-recovery-runtime.test.ts +++ b/tests/quota-401-recovery-runtime.test.ts @@ -63,6 +63,7 @@ test("a refresh settles the budget even when the caller cancels first", async () const claim = claimQuotaRecovery("cancelled-owner", generation); if (!claim.granted) throw new Error("expected a claim"); const controller = new AbortController(); + let callerRejected: unknown; const settled = new Promise(resolve => { void forceRefreshCodexPoolToken("cancelled-owner", { rejectedGeneration: generation, @@ -73,33 +74,37 @@ test("a refresh settles the budget even when the caller cancels first", async () else { released += 1; releaseQuotaRecovery("cancelled-owner", claim.claimId, 60_000); } resolve(); }, - }).catch(() => { /* the caller walked away on purpose */ }); + }).then( + () => { callerRejected = "resolved"; }, + error => { callerRejected = error; }, + ); }); // Cancel while the token request is still in flight. The shared refresh keeps running and // commits; settling from the cancelled await would report "failed", release the budget, // and let the freshly refreshed lineage claim again moments later. - controller.abort(new Error("caller went away")); + const abortReason = new Error("caller went away"); + controller.abort(abortReason); await settled; + // The caller really was cancelled — otherwise this test would also pass against an + // implementation that simply ignores the signal. + expect(callerRejected).toBe(abortReason); expect(released).toBe(0); expect(quotaRecoveryRecordForTests("cancelled-owner")).toEqual({ state: "spent", lineage: generation + 1 }); }); -test("a joiner that adopts somebody else's credential does not spend that lineage's budget", async () => { - const { forceRefreshCodexPoolToken, saveCodexAccountCredential } = await import("../src/codex/account-store"); +test("exactly one of two callers on a shared flight performed the CAS", async () => { + const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); const generation = await seedAccount("adopting-joiner"); - // The flight resolves to a credential from a DIFFERENT grant: its own branch tags that - // as an external replacement, and a joiner adopting those bytes must carry that verdict - // rather than calling itself joined-lineage. globalThis.fetch = (async () => { - await new Promise(resolve => setTimeout(resolve, 10)); + await new Promise(resolve => setTimeout(resolve, 20)); return Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 }); }) as typeof fetch; const outcomes: string[] = []; - const both = await Promise.allSettled([ + await Promise.allSettled([ forceRefreshCodexPoolToken("adopting-joiner", { rejectedGeneration: generation, rejectedAccessToken: REJECTED, @@ -112,60 +117,101 @@ test("a joiner that adopts somebody else's credential does not spend that lineag }), ]); - // Both callers get a verdict, and every verdict is one of the three — never undefined, - // which is what a path that forgot to classify itself would produce. + // Only one caller can have moved the credential. Accepting "any of the three enum + // values" would pass against the bug where a joiner copies the flight's own + // `self-refresh` and reports a CAS it never performed. expect(outcomes).toHaveLength(2); - for (const provenance of outcomes) { - expect(["self-refresh", "joined-lineage", "external-replacement"]).toContain(provenance); - } - expect(both.some(r => r.status === "fulfilled")).toBe(true); - void saveCodexAccountCredential; + expect(outcomes.filter(p => p === "self-refresh")).toHaveLength(1); + expect(outcomes.filter(p => p !== "self-refresh")).toHaveLength(1); }); -test("a terminal refresh failure keeps reporting needs-reauth on later polls", async () => { - const { claimQuotaRecovery, quotaRecoveryTerminalFor, settleQuotaRecoveryTerminal, releaseQuotaRecovery } = - await import("../src/codex/quota-401-recovery"); +test("a revoked grant routes to terminal settlement, a slow one does not", async () => { + const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); + const { claimQuotaRecovery, quotaRecoveryRecordForTests } = await import("../src/codex/quota-401-recovery"); - const claim = claimQuotaRecovery("dead-grant", 4); - if (!claim.granted) throw new Error("expected a claim"); - settleQuotaRecoveryTerminal("dead-grant", claim.claimId); - - // A revoked grant does not recover on the next poll. Treating it as an ordinary spent - // budget would make the following bare 401 report the account healthy. - expect(quotaRecoveryTerminalFor("dead-grant", 4)).toBe(true); - expect(claimQuotaRecovery("dead-grant", 4)).toEqual({ granted: false, reason: "spent" }); - - // A transient failure is different: spent, but not terminal. - const other = claimQuotaRecovery("slow-grant", 4); - if (!other.granted) throw new Error("expected a claim"); - releaseQuotaRecovery("slow-grant", other.claimId, 60_000); - expect(quotaRecoveryTerminalFor("slow-grant", 4)).toBe(false); + // Drive the real primitive to a real refresh failure and route it exactly the way + // recoverPoolQuotaFrom401 does, so a wiring that never calls terminal settlement fails. + const { TokenRefreshError } = await import("../src/codex/account-store"); + const { releaseQuotaRecovery, settleQuotaRecoveryTerminal, quotaRecoveryTerminalFor } = + await import("../src/codex/quota-401-recovery"); + const route = (accountId: string, claimId: string, error: unknown) => { + if (error instanceof TokenRefreshError && /invalid_grant|revoked|expired|invalid_refresh_token/i.test(String(error.message))) { + settleQuotaRecoveryTerminal(accountId, claimId); + } else { + releaseQuotaRecovery(accountId, claimId, 60_000); + } + }; + + const revokedGeneration = await seedAccount("dead-grant"); + globalThis.fetch = (async () => new Response("{\"error\":\"invalid_grant\"}", { status: 400 })) as typeof fetch; + const revokedClaim = claimQuotaRecovery("dead-grant", revokedGeneration); + if (!revokedClaim.granted) throw new Error("expected a claim"); + await forceRefreshCodexPoolToken("dead-grant", { + rejectedGeneration: revokedGeneration, + rejectedAccessToken: REJECTED, + onSettled: o => { if (o.kind === "failed") route("dead-grant", revokedClaim.claimId, o.error); }, + }).catch(() => { /* the refresh is supposed to fail */ }); + + expect(quotaRecoveryTerminalFor("dead-grant", revokedGeneration)).toBe(true); + expect(quotaRecoveryRecordForTests("dead-grant")).toMatchObject({ state: "spent", terminal: true }); + + const slowGeneration = await seedAccount("slow-grant"); + globalThis.fetch = (async () => { throw new Error("socket hang up"); }) as typeof fetch; + const slowClaim = claimQuotaRecovery("slow-grant", slowGeneration); + if (!slowClaim.granted) throw new Error("expected a claim"); + await forceRefreshCodexPoolToken("slow-grant", { + rejectedGeneration: slowGeneration, + rejectedAccessToken: REJECTED, + onSettled: o => { if (o.kind === "failed") route("slow-grant", slowClaim.claimId, o.error); }, + }).catch(() => { /* transient by design */ }); + + // A network failure proves nothing about the grant, so it must not fence the account. + expect(quotaRecoveryTerminalFor("slow-grant", slowGeneration)).toBe(false); + expect(quotaRecoveryRecordForTests("slow-grant")).toMatchObject({ state: "backoff" }); }); -test("neither bearer reaches a log line during a refresh", async () => { +test("no bearer reaches the console, the debug buffer, or a serialized account row", async () => { const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); + const { getDebugLogEntries } = await import("../src/lib/debug-log-buffer"); const generation = await seedAccount("quiet-refresh"); globalThis.fetch = (async () => Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 })) as typeof fetch; + const before = getDebugLogEntries({ limit: 500 }).length; const captured: string[] = []; const originals = { log: console.log, warn: console.warn, error: console.error, debug: console.debug }; const capture = (...args: unknown[]) => { captured.push(args.map(String).join(" ")); }; console.log = capture; console.warn = capture; console.error = capture; console.debug = capture; + let result: Awaited>; try { - const result = await forceRefreshCodexPoolToken("quiet-refresh", { + result = await forceRefreshCodexPoolToken("quiet-refresh", { rejectedGeneration: generation, rejectedAccessToken: REJECTED, }); - expect(result.accessToken).toBe(ROTATED); } finally { console.log = originals.log; console.warn = originals.warn; console.error = originals.error; console.debug = originals.debug; } + // The refresh really happened — otherwise this asserts silence about nothing. + expect(result.accessToken).toBe(ROTATED); + expect(result.rotated).toBe(true); + const secrets = [REJECTED, ROTATED, "grant2", "grant"]; // privacy:scan is static and cannot see what a runtime path actually emits. const transcript = captured.join("\n"); - expect(transcript).not.toContain(REJECTED); - expect(transcript).not.toContain(ROTATED); - expect(transcript).not.toContain("grant2"); + for (const secret of secrets) expect(transcript).not.toContain(secret); + + // console is not the only sink: the dashboard reads this buffer over the management API. + const debugText = JSON.stringify(getDebugLogEntries({ after: before, limit: 500 })); + for (const secret of secrets) expect(debugText).not.toContain(secret); + + // And nothing the store hands out for an account may carry a bearer: the account list + // serializes from these records, so a leak here becomes a leak over the management API. + const { listCodexAccountIds, readCodexAccountRecord } = await import("../src/codex/account-store"); + const publicView = JSON.stringify(listCodexAccountIds().map(id => { + const record = readCodexAccountRecord(id); + // Mirror what the account list exposes: identity and freshness, never the credential. + return record && { id, generation: record.generation, deletedAt: record.deletedAt }; + })); + for (const secret of secrets) expect(publicView).not.toContain(secret); }); From 49045f0c3dd4885abb7ebad4720cdc5e38ea7404 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 08:57:31 +0900 Subject: [PATCH 11/14] fix(codex): make terminal 401 evidence durable, and read the reason not the text Structured terminal evidence from WHAM was reported and then forgotten. The first-response branch and the replay branch each returned needsReauth: true, but the claim had already been settled non-terminally by the successful refresh - so the next poll found a spent budget, reported transient, and a dead credential looked healthy again. All three terminal branches now mark the account, which is the durable, generation-independent signal the account list already reads. isTerminalRefreshError matched substrings of the error message. TokenRefreshError carries a reason discriminator, and a durable quarantine decision should not be one reworded string away from changing. It reads reason now. The terminal regression also could not have caught either problem: it reimplemented the production settlement callback locally, so deleting the real wiring left it green. It now drives listCodexAuthAccounts through bare WHAM 401 to token invalid_grant to a second poll, and asserts the account-level mark directly - because asserting only the second response is satisfied by a cached quota and proves nothing about durability. Two smaller test fixes from the same review: the provenance assertion accepted any non-self value, which an incorrect external-replacement joiner also satisfies, so it now names joined-lineage; and the secrecy check built its own account row by hand, which cannot catch a leak in the real serializer, so it now asserts against listCodexAuthAccounts. --- src/codex/auth-api.ts | 16 +++-- tests/quota-401-recovery-runtime.test.ts | 82 +++++++++++++++++++++--- 2 files changed, 83 insertions(+), 15 deletions(-) diff --git a/src/codex/auth-api.ts b/src/codex/auth-api.ts index 971aef4afe..09ad18dc02 100644 --- a/src/codex/auth-api.ts +++ b/src/codex/auth-api.ts @@ -995,6 +995,9 @@ async function recoverPoolQuotaFrom401(ctx: { // Structured terminal evidence short-circuits everything: the same allowlist and bounded // parser the main account uses, because it is the same endpoint answering. if (await isTerminalPoolAuthResponse(resp)) { + // Durable, not just this response: the account list re-polls, and without a recorded + // mark the next bare 401 finds nothing terminal and reports the account healthy. + markAccountNeedsReauth(accountId); return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; } @@ -1036,6 +1039,7 @@ async function recoverPoolQuotaFrom401(ctx: { // A refresh that failed terminally is the one case where the credential really is gone. // Everything else is unknown, and unknown is not proof. if (e instanceof TokenRefreshError && isTerminalRefreshError(e)) { + markAccountNeedsReauth(accountId); return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; } return { quota: existing ?? null, needsReauth: false, credentialGeneration: rejectedGeneration }; @@ -1062,6 +1066,9 @@ async function recoverPoolQuotaFrom401(ctx: { }); if (!replay.ok) { if (replay.status === 401 && await isTerminalPoolAuthResponse(replay)) { + // The refresh already settled this claim non-terminally, so the record alone would + // let the next poll call a dead credential healthy. + markAccountNeedsReauth(accountId); return { quota: existing ?? null, needsReauth: true, credentialGeneration: refreshed.generation }; } return { quota: existing ?? null, needsReauth: false, credentialGeneration: refreshed.generation }; @@ -1085,11 +1092,10 @@ async function isTerminalPoolAuthResponse(resp: Response): Promise { /** A revoked or expired grant is terminal; an unknown or transport failure is not. */ function isTerminalRefreshError(error: TokenRefreshError): boolean { - const text = `${error.message}`.toLowerCase(); - return text.includes("invalid_grant") - || text.includes("invalid_refresh_token") - || text.includes("revoked") - || text.includes("expired"); + // Read the discriminator, not the message. TokenRefreshError carries `reason`, and + // matching on human text would let a durable quarantine decision change the next time + // somebody rewords an error string. + return error.reason === "revoked" || error.reason === "expired"; } /** Parse and store a successful WHAM response. Shared by the first attempt and the replay. */ diff --git a/tests/quota-401-recovery-runtime.test.ts b/tests/quota-401-recovery-runtime.test.ts index 881ed133ae..07e0c6b65e 100644 --- a/tests/quota-401-recovery-runtime.test.ts +++ b/tests/quota-401-recovery-runtime.test.ts @@ -48,6 +48,16 @@ async function seedAccount(id: string): Promise { return readCodexAccountRecord(id)!.generation; } +/** Register the account in config too, so the account-list API actually returns a row. */ +async function seedListedAccount(id: string): Promise { + const generation = await seedAccount(id); + const { loadConfig, saveConfig } = await import("../src/config"); + const config = loadConfig(); + const accounts = [...(config.codexAccounts ?? []).filter(a => a.id !== id), { id, label: id }]; + saveConfig({ ...config, codexAccounts: accounts }); + return generation; +} + test("a refresh settles the budget even when the caller cancels first", async () => { const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); const { claimQuotaRecovery, quotaRecoveryRecordForTests, settleQuotaRecovery, releaseQuotaRecovery } = @@ -122,7 +132,9 @@ test("exactly one of two callers on a shared flight performed the CAS", async () // `self-refresh` and reports a CAS it never performed. expect(outcomes).toHaveLength(2); expect(outcomes.filter(p => p === "self-refresh")).toHaveLength(1); - expect(outcomes.filter(p => p !== "self-refresh")).toHaveLength(1); + // Specifically joined-lineage: an "external-replacement" joiner is also non-self, and + // would wrongly leave this lineage's budget unspent. + expect(outcomes.filter(p => p === "joined-lineage")).toHaveLength(1); }); test("a revoked grant routes to terminal settlement, a slow one does not", async () => { @@ -170,6 +182,59 @@ test("a revoked grant routes to terminal settlement, a slow one does not", async expect(quotaRecoveryRecordForTests("slow-grant")).toMatchObject({ state: "backoff" }); }); +test("a revoked grant stays needs-reauth across polls, through the real quota path", async () => { + const { listCodexAuthAccounts } = await import("../src/codex/auth-api"); + const { loadConfig } = await import("../src/config"); + await seedListedAccount("dead-grant"); + + // Bare WHAM 401 -> token endpoint says invalid_grant -> the account list re-polls. + // Routing this through listCodexAuthAccounts is the point: a local reimplementation of + // the settlement callback stays green with the production wiring deleted. + let tokenCalls = 0; + globalThis.fetch = (async (input: string | URL | Request) => { + const url = String(input); + if (url.includes("/wham/usage")) return new Response("{}", { status: 401 }); + tokenCalls += 1; + return new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }); + }) as typeof fetch; + + const config = loadConfig(); + const first = await listCodexAuthAccounts(config, true); + expect(first.find(row => row.id === "dead-grant")?.needsReauth).toBe(true); + + // The evidence has to be DURABLE, not just present in the response that discovered it. + // The recovery record alone cannot carry it: by the time the next poll runs, the claim is + // spent and a spent budget reports transient. Assert the account-level mark directly, so + // dropping markAccountNeedsReauth fails here rather than being masked by a cached quota. + const { isAccountNeedsReauth } = await import("../src/codex/auth-api"); + expect(isAccountNeedsReauth("dead-grant")).toBe(true); + + const second = await listCodexAuthAccounts(config, true); + expect(second.find(row => row.id === "dead-grant")?.needsReauth).toBe(true); + // And the dead grant is not retried on every poll. + expect(tokenCalls).toBe(1); +}); + +test("a transient refresh failure does not quarantine the account", async () => { + const { listCodexAuthAccounts } = await import("../src/codex/auth-api"); + const { loadConfig } = await import("../src/config"); + const { quotaRecoveryRecordForTests } = await import("../src/codex/quota-401-recovery"); + await seedListedAccount("slow-grant"); + + globalThis.fetch = (async (input: string | URL | Request) => { + if (String(input).includes("/wham/usage")) return new Response("{}", { status: 401 }); + throw new Error("socket hang up"); + }) as typeof fetch; + + const rows = await listCodexAuthAccounts(loadConfig(), true); + // A network failure proves nothing about the grant. + expect(rows.find(row => row.id === "slow-grant")?.needsReauth).toBe(false); + // Nor may it leave a durable quarantine behind. + const { isAccountNeedsReauth } = await import("../src/codex/auth-api"); + expect(isAccountNeedsReauth("slow-grant")).toBe(false); + expect(quotaRecoveryRecordForTests("slow-grant")).toMatchObject({ state: "backoff" }); +}); + test("no bearer reaches the console, the debug buffer, or a serialized account row", async () => { const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); const { getDebugLogEntries } = await import("../src/lib/debug-log-buffer"); @@ -205,13 +270,10 @@ test("no bearer reaches the console, the debug buffer, or a serialized account r const debugText = JSON.stringify(getDebugLogEntries({ after: before, limit: 500 })); for (const secret of secrets) expect(debugText).not.toContain(secret); - // And nothing the store hands out for an account may carry a bearer: the account list - // serializes from these records, so a leak here becomes a leak over the management API. - const { listCodexAccountIds, readCodexAccountRecord } = await import("../src/codex/account-store"); - const publicView = JSON.stringify(listCodexAccountIds().map(id => { - const record = readCodexAccountRecord(id); - // Mirror what the account list exposes: identity and freshness, never the credential. - return record && { id, generation: record.generation, deletedAt: record.deletedAt }; - })); - for (const secret of secrets) expect(publicView).not.toContain(secret); + // And the REAL serializer, not a hand-built stand-in: a leak has to be caught in what + // the management API actually returns. + const { listCodexAuthAccounts } = await import("../src/codex/auth-api"); + const { loadConfig } = await import("../src/config"); + const rows = JSON.stringify(await listCodexAuthAccounts(loadConfig(), false)); + for (const secret of secrets) expect(rows).not.toContain(secret); }); From 4830a992a382b5f08334b884f91154d26633aa83 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 09:01:12 +0900 Subject: [PATCH 12/14] fix(codex): scope terminal evidence to the credential it condemns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The durable reauth mark was account-wide, which outlives the credential it was about. Ordinary reauthentication clears it, but a terminal response still in flight when the operator logs back in would land afterwards and quarantine the replacement. All three marks now carry the generation the evidence concerns: the rejected one for the first response and the refresh failure, the refreshed one for the replay. The durability regression only ever reached the refresh-failure branch — bare 401 followed by invalid_grant — so deleting the other two marks left it green. There are now separate cases for structured terminal evidence on the first 401 (which needs no refresh at all) and on the replay, each asserting the mark is present and then that a replacement credential does not inherit it. The pre-abort guard also had no test of its own. It does now: an already-aborted signal must issue zero fetches, reject with the exact reason, settle failed once, and leave no claim held. And the secrecy test seeded a credential without registering the account, so the account-list assertion was scanning a list that never contained the account. It registers now and asserts the row is actually there before checking it. --- src/codex/auth-api.ts | 13 ++- tests/quota-401-recovery-runtime.test.ts | 109 ++++++++++++++++++++++- 2 files changed, 117 insertions(+), 5 deletions(-) diff --git a/src/codex/auth-api.ts b/src/codex/auth-api.ts index 09ad18dc02..91734a3229 100644 --- a/src/codex/auth-api.ts +++ b/src/codex/auth-api.ts @@ -997,7 +997,11 @@ async function recoverPoolQuotaFrom401(ctx: { if (await isTerminalPoolAuthResponse(resp)) { // Durable, not just this response: the account list re-polls, and without a recorded // mark the next bare 401 finds nothing terminal and reports the account healthy. - markAccountNeedsReauth(accountId); + // + // Scoped to the generation this evidence is ABOUT. An account-wide mark would outlive + // the credential it condemned, so a late terminal response arriving after the operator + // re-authenticated would quarantine the replacement. + markAccountNeedsReauth(accountId, captureConfigGeneration(), rejectedGeneration); return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; } @@ -1039,7 +1043,7 @@ async function recoverPoolQuotaFrom401(ctx: { // A refresh that failed terminally is the one case where the credential really is gone. // Everything else is unknown, and unknown is not proof. if (e instanceof TokenRefreshError && isTerminalRefreshError(e)) { - markAccountNeedsReauth(accountId); + markAccountNeedsReauth(accountId, captureConfigGeneration(), rejectedGeneration); return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; } return { quota: existing ?? null, needsReauth: false, credentialGeneration: rejectedGeneration }; @@ -1067,8 +1071,9 @@ async function recoverPoolQuotaFrom401(ctx: { if (!replay.ok) { if (replay.status === 401 && await isTerminalPoolAuthResponse(replay)) { // The refresh already settled this claim non-terminally, so the record alone would - // let the next poll call a dead credential healthy. - markAccountNeedsReauth(accountId); + // let the next poll call a dead credential healthy. The evidence is about the + // REFRESHED credential, which is what the replay used. + markAccountNeedsReauth(accountId, writerGeneration, refreshed.generation); return { quota: existing ?? null, needsReauth: true, credentialGeneration: refreshed.generation }; } return { quota: existing ?? null, needsReauth: false, credentialGeneration: refreshed.generation }; diff --git a/tests/quota-401-recovery-runtime.test.ts b/tests/quota-401-recovery-runtime.test.ts index 07e0c6b65e..82cd83b251 100644 --- a/tests/quota-401-recovery-runtime.test.ts +++ b/tests/quota-401-recovery-runtime.test.ts @@ -235,10 +235,116 @@ test("a transient refresh failure does not quarantine the account", async () => expect(quotaRecoveryRecordForTests("slow-grant")).toMatchObject({ state: "backoff" }); }); +test("an already-aborted caller starts no refresh at all", async () => { + const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); + const { claimQuotaRecovery, quotaRecoveryRecordForTests, releaseQuotaRecovery, settleQuotaRecovery } = + await import("../src/codex/quota-401-recovery"); + const generation = await seedAccount("pre-aborted"); + + let fetches = 0; + globalThis.fetch = (async () => { + fetches += 1; + return Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 }); + }) as typeof fetch; + + const claim = claimQuotaRecovery("pre-aborted", generation); + if (!claim.granted) throw new Error("expected a claim"); + const reason = new Error("caller already gone"); + const controller = new AbortController(); + controller.abort(reason); + + let failures = 0; + let rejected: unknown; + await forceRefreshCodexPoolToken("pre-aborted", { + rejectedGeneration: generation, + rejectedAccessToken: REJECTED, + signal: controller.signal, + onSettled: outcome => { + if (outcome.kind === "failed") { failures += 1; releaseQuotaRecovery("pre-aborted", claim.claimId, 60_000); } + else settleQuotaRecovery("pre-aborted", claim.claimId, outcome); + }, + }).catch(error => { rejected = error; }); + + // Settlement runs on an uncancelled completion, which bypasses resolveCodexToken's own + // pre-abort guard — so without an explicit check here a caller that is already gone + // would rotate a credential nobody is waiting for. + expect(fetches).toBe(0); + expect(rejected).toBe(reason); + expect(failures).toBe(1); + // And the claim is not left held: it was released, not abandoned to its lease. + expect(quotaRecoveryRecordForTests("pre-aborted")).toMatchObject({ state: "backoff" }); +}); + +test("structured terminal evidence on the FIRST 401 needs no refresh and is generation-scoped", async () => { + const { listCodexAuthAccounts, isAccountNeedsReauth } = await import("../src/codex/auth-api"); + const { loadConfig } = await import("../src/config"); + const generation = await seedListedAccount("structured-first"); + + let tokenCalls = 0; + globalThis.fetch = (async (input: string | URL | Request) => { + if (String(input).includes("/wham/usage")) { + return new Response(JSON.stringify({ detail: { code: "invalid_refresh_token" } }), { status: 401 }); + } + tokenCalls += 1; + return Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 }); + }) as typeof fetch; + + const rows = await listCodexAuthAccounts(loadConfig(), true); + expect(rows.find(row => row.id === "structured-first")?.needsReauth).toBe(true); + // A body that says the grant is dead needs no refresh to prove it. + expect(tokenCalls).toBe(0); + expect(isAccountNeedsReauth("structured-first")).toBe(true); + + // The evidence is about THAT credential. A replacement must not inherit the quarantine. + const { saveCodexAccountCredential } = await import("../src/codex/account-store"); + saveCodexAccountCredential("structured-first", { + accessToken: "fresh-login", + refreshToken: "fresh-grant", + expiresAt: Date.now() + 3600_000, + chatgptAccountId: "acc", + }); + expect(isAccountNeedsReauth("structured-first")).toBe(false); + void generation; +}); + +test("structured terminal evidence on the REPLAY is scoped to the refreshed credential", async () => { + const { listCodexAuthAccounts, isAccountNeedsReauth } = await import("../src/codex/auth-api"); + const { loadConfig } = await import("../src/config"); + await seedListedAccount("structured-replay"); + + let whamCalls = 0; + globalThis.fetch = (async (input: string | URL | Request) => { + if (String(input).includes("/wham/usage")) { + whamCalls += 1; + // Bare first, structured-terminal on the replay: only the second answer proves death. + return whamCalls === 1 + ? new Response("{}", { status: 401 }) + : new Response(JSON.stringify({ detail: { code: "invalid_refresh_token" } }), { status: 401 }); + } + return Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 }); + }) as typeof fetch; + + const rows = await listCodexAuthAccounts(loadConfig(), true); + expect(whamCalls).toBe(2); + expect(rows.find(row => row.id === "structured-replay")?.needsReauth).toBe(true); + expect(isAccountNeedsReauth("structured-replay")).toBe(true); + + const { saveCodexAccountCredential } = await import("../src/codex/account-store"); + saveCodexAccountCredential("structured-replay", { + accessToken: "fresh-login", + refreshToken: "fresh-grant", + expiresAt: Date.now() + 3600_000, + chatgptAccountId: "acc", + }); + expect(isAccountNeedsReauth("structured-replay")).toBe(false); +}); + test("no bearer reaches the console, the debug buffer, or a serialized account row", async () => { const { forceRefreshCodexPoolToken } = await import("../src/codex/account-store"); const { getDebugLogEntries } = await import("../src/lib/debug-log-buffer"); - const generation = await seedAccount("quiet-refresh"); + // Registered, not just credentialed: an unlisted account produces no row at all, and the + // serializer assertion below would then be checking an empty list. + const generation = await seedListedAccount("quiet-refresh"); globalThis.fetch = (async () => Response.json({ access_token: ROTATED, refresh_token: "grant2", expires_in: 3600 })) as typeof fetch; @@ -275,5 +381,6 @@ test("no bearer reaches the console, the debug buffer, or a serialized account r const { listCodexAuthAccounts } = await import("../src/codex/auth-api"); const { loadConfig } = await import("../src/config"); const rows = JSON.stringify(await listCodexAuthAccounts(loadConfig(), false)); + expect(rows).toContain("quiet-refresh"); for (const secret of secrets) expect(rows).not.toContain(secret); }); From a449e00862f82180ed11db1880981b88f391b315 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 09:05:22 +0900 Subject: [PATCH 13/14] docs(devlog): record what 12 review rounds changed in the #3019 unit --- .../061_wp6_outcome.md | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 devlog/_plan/260831_prio70_train_round2/061_wp6_outcome.md diff --git a/devlog/_plan/260831_prio70_train_round2/061_wp6_outcome.md b/devlog/_plan/260831_prio70_train_round2/061_wp6_outcome.md new file mode 100644 index 0000000000..847d4f9f0c --- /dev/null +++ b/devlog/_plan/260831_prio70_train_round2/061_wp6_outcome.md @@ -0,0 +1,70 @@ +# 061 outcome — wp6 (#3019): what the reviews changed + +Seven plan-audit rounds before any code, then five implementation rounds. The plan that +was implemented is materially different from the one written at wp0, and the difference is +the point of this record. + +## What the plan audits changed, before a line was written + +| round | what the plan got wrong | +| --- | --- | +| 1 | "Add provenance to the primitive **if** the contract cannot express it" — it cannot; the change is mandatory, and `account-store.ts` belongs in scope | +| 1 | The concurrency cases could not produce a refresh joiner at all: same-account quota calls coalesce at `auth-api.ts:1043` **before** reaching the primitive, so case 8 would have passed under the exact bug it targeted | +| 2 | The TTL contradicted the security property — expiring a spent record hands the same lineage another refresh | +| 3 | Keying the claim on the lineage cannot separate an old claimant from a later retry on the same lineage | +| 4 | The refresh commits `G → G+1` before the claimant settles, so a liveness sweep would delete a live claim and a `G+1` claim would replace it | +| 4 | Two sections contradicted each other on `external-replacement`: fencing the returned generation there denies a fresh credential its own budget | +| 5 | "Whichever caller observes the outcome settles" is not implementable — `awaitOwnCancellation` rejects the wrapper while the flight continues privately, so with no joiner nobody observes the commit | +| 6 | Attaching settlement to the raw flight settles the wrong thing: provenance is per caller, and one grant-flight can serve several aliases each holding their own claim | +| 6 | The timing layout was an import cycle — `auth-api` must import the recovery store, so the store cannot import `auth-api` for the WHAM timeout | + +## What the implementation reviews changed + +The same defect kept reappearing: **something is unknown, and the code treats it as +settled.** Every instance either opened the retry loop this phase exists to close, or made +a dead credential look healthy. + +| round | defect | +| --- | --- | +| 1 | `onSettled` on the caller-cancellable await: a cancelled poll reported "failed" for a refresh that was committing, released the budget, and the refreshed lineage could claim again | +| 1 | A joiner inherited the flight's `self-refresh`, so a caller that performed no CAS was told the credential was its own lineage | +| 1 | An external replacement from the flight's grant-mismatch and freshness branches was labelled `joined-lineage` | +| 1 | `resp.clone()` tees the body while the bounded parser cancels only its own reader | +| 2 | The terminal route was in the commit message and not in the code — every failure still released into backoff | +| 2 | Moving settlement off the caller signal bypassed `resolveCodexToken`'s own pre-abort guard, so an already-cancelled request started a refresh | +| 3 | Structured terminal evidence was reported and forgotten: the claim was already settled non-terminally, so the next poll called a dead credential healthy | +| 3 | `isTerminalRefreshError` matched message substrings while `TokenRefreshError` carries a `reason` discriminator | +| 4 | The reauth mark was account-wide, so a terminal response still in flight when the operator re-authenticated would quarantine the replacement | + +## Tests that proved nothing + +Five separate times a regression passed against the defect it named: + +- The first suite drove the budget store in isolation and stayed green with the recovery + entirely disconnected from the quota path. +- The cancellation test swallowed the caller's rejection, so an implementation ignoring + the signal passed. +- The provenance test accepted any enum value — including the wrong one. +- The terminal test reimplemented the production callback locally, so deleting the real + wiring left it green. +- The secrecy test watched `console.debug` rather than the debug buffer the dashboard + reads, built its own account row by hand, and scanned a list that never contained the + account under test. + +Each is now driven through the real path: `listCodexAuthAccounts` for the terminal and +serialization cases, `forceRefreshCodexPoolToken` for cancellation and provenance, and the +actual `getDebugLogEntries` buffer for secrecy. Every fix was driven RED against its own +defect and restored. + +## The shape that survived + +- **Claim id, not lineage.** A clear or settle is a compare-and-set on + `(accountId, lineage, claimId)`, so a late completion is a no-op rather than somebody + else's budget. +- **`spent` is durable, `claimed` is a lease.** Expiring a spent record would grant a + second refresh; an abandoned lease is reclaimable but never promoted to spent. +- **Settlement rides the refresh, not the caller.** Cancellation rejects what the caller + awaits; the flight still settles. +- **Evidence is scoped to what it condemns.** A terminal mark names the generation it is + about, so it cannot outlive the credential and quarantine its replacement. + From 6074b68590024b1c42b9790b72fcf794ca31284c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 09:12:36 +0900 Subject: [PATCH 14/14] test(state-store): register the 401 recovery budget in the owner inventory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI shard 4 caught what my focused runs could not: STATE_STORE_REGISTRATIONS is mirrored by a hand-maintained list in tests/state-store-sweeper.test.ts, and adding codex-quota-401-recovery to the production registrations without adding it there fails that assertion. The duplication is deliberate — it forces someone to decide a new store has an owner and a sweep policy rather than inheriting one silently — so the fix is to record it, not to loosen the check. My focused file list did not include this test, which is exactly the indirect-dependency case AGENTS.md warns about: the registration is data read by another module's test, so the import graph does not connect them. --- tests/state-store-sweeper.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/state-store-sweeper.test.ts b/tests/state-store-sweeper.test.ts index ea2d473b27..3bcfb882b7 100644 --- a/tests/state-store-sweeper.test.ts +++ b/tests/state-store-sweeper.test.ts @@ -101,6 +101,10 @@ describe("state-store sweeper", () => { "combo-target-cooldowns", "anthropic-routing-health", "xai-refresh-verdicts", + // #3019: the WHAM 401 recovery budget. Registered here deliberately — the inventory + // is hand-maintained so a new store cannot be added without someone deciding it has + // an owner and a sweep policy. + "codex-quota-401-recovery", "responses-continuation", "antigravity-replay", "config-warning-memos",