From 01b915f178b441c1acdfdcf8df2c009eb3a9150b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 02:39:09 +0900 Subject: [PATCH 1/6] fix(codex): heal a dormant same-grant record and drop stale 401 evidence Two remaining gaps from #2892. Gaps 1-2 shipped as 8f199fcb6, gap 5 as 84049830e. Gap 3. A refresh rotates the refresh token, but only the flight owner and live joiners ever learn the new one. A non-deleted record holding the same grant that is not participating in the flight keeps a token upstream has just invalidated; its next refresh sends a dead grant, and `invalid_grant` classifies as `revoked`, retiring a healthy account because we rotated its grant and never told it. Owner and eligible aliases now commit in one lock acquisition and one `persist`, so no window exists where some records hold the dead grant. Eligibility is deliberately narrow: same pre-refresh fingerprint, access token, expiry, and the same `chatgptAccountId` as the owner. The rotated access token, refresh token, and expiry move together. An adversarial design audit rejected a wider version of this and it is worth recording why, because the wider version looks obviously better and is not. Repairing only the grant of an alias whose access token had moved on would advance its generation while keeping the old access token. `plan-from-token` reads a higher generation as proof of a newer JWT -- that is how JWT plan claims supersede a WHAM observation -- so a stale JWT could then overwrite an authoritative plan. Worse, flights are keyed by grant and do not record participants, so such a scan cannot distinguish a dormant record from a live joiner; rotating a joiner's grant while preserving its 401-rejected bearer makes the provenance CAS inapplicable and the recursion's freshness shortcut hands the rejected token straight back. This therefore closes gap 3 only for genuinely untouched duplicates. A mixed alias still needs durable grant lineage and verified identity binding, which the current fingerprint model cannot express safely, and #2892 says so rather than claiming otherwise. Gap 4. The reporter described an async interleaving between the generation check and the side effects. That specific race is not reachable: `recordCodexUpstreamOutcome` is synchronous and no `await` separates them. The cross-process race is real regardless, since the check is an unlocked read while writers hold the mutation lock, and OS preemption needs no `await`. Health and reauth carry no credential identity, so a stale 401 landing in that window quarantines the replacement. Taking the credential lock per outcome is not available -- it runs with `busy_timeout=0`, which would turn contention into request errors -- so the sequence stays synchronous and re-validates afterwards, restoring the prior health and reauth state when the generation stopped being live. The window remains; its effects no longer survive it. Affinity sweeping is not rolled back: entries already carry a generation and self-invalidate, and re-adding swept entries would be the worse bug. Verification. 420 pass / 0 fail across codex-routing, codex-account-store, responses-pool-401-refresh, codex-plan, and codex-auth-api. Typecheck and privacy:scan clean. Six named mutations each drove a test red: owner-only CAS, dropping the identity guard, dropping the untouched-access guard, removing the rollback, and making the rollback unconditional -- the last two failing opposite tests, which is what proves the rollback is conditional rather than a blanket "never quarantine". The gap-4 regression needs a seam. A single process cannot schedule another process's write, so both reads would observe one store and any assertion would be vacuous. `setCodexCredentialRevalidationSeamForTests` stands in for that writer, is undefined in production, and costs one null check. --- .../260830_lane_q_2892_gaps_3_4/000_units.md | 113 ++++++++++++++++++ src/codex/account-store.ts | 90 +++++++++++++- src/codex/routing.ts | 49 +++++++- tests/codex-account-store.test.ts | 95 +++++++++++++++ tests/codex-routing.test.ts | 48 +++++++- 5 files changed, 392 insertions(+), 3 deletions(-) create mode 100644 devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md diff --git a/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md b/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md new file mode 100644 index 0000000000..a8c7b27c2d --- /dev/null +++ b/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md @@ -0,0 +1,113 @@ +# Lane Q — issue #2892 gaps 3 and 4 + +The last two of the five gaps #2892 raised against the merged stored-Pool 401 +recovery path. Gaps 1–2 shipped as `8f199fcb6` (#2920), gap 5 as `84049830e` +(#2922). An independent recon audit re-derived both remaining gaps from current +`dev` and confirmed each is still reachable — and corrected the reporter on one +point, recorded below. + +## Gap 3 — a rotated grant never reaches an inactive same-grant alias + +A successful refresh persists the rotated credential to the flight owner only, via +the generation CAS at `src/codex/account-store.ts:748`. A live joiner can CAS the +result onto its own record. Nothing writes to a third category: a non-deleted +record carrying the same `refreshGrantFingerprint` that is not participating in +the flight. + +`findFreshCredentialForGrant` (`src/codex/account-store.ts:393`) is a pre-fetch +lookup and propagates nothing. So the alias keeps a refresh token that upstream +has just rotated away. The next refresh on that alias sends a dead grant, and +`invalid_grant` is classified `revoked` — which retires a healthy account. That +classification is correct behavior for a genuinely dead grant; the defect is that +the grant died because we rotated it and never told the alias. + +### The design an adversarial audit rejected + +My first plan had two branches: an untouched alias adopts the rotated credential +whole, and an alias whose access token had changed concurrently keeps its own +access token but takes **only** the rotated refresh token. An independent audit +refused that second branch, with two findings I could not rebut: + +- The generation fence in `src/codex/plan-from-token.ts:32` treats a higher + generation as proof of a **newer access-token JWT**, which is what lets JWT plan + claims supersede an older WHAM observation. Bumping a generation while + deliberately keeping the old access token lets a stale JWT overwrite an + authoritative plan. `tests/codex-plan.test.ts:129` already pins that meaning. +- A flight is keyed by grant and does not record participant account ids + (`src/codex/account-store.ts:308`), so a scan cannot distinguish a dormant alias + from a live joiner. Rotating a joiner's grant while preserving its 401-rejected + access token makes the provenance CAS inapplicable, and the recursion's + freshness shortcut then returns the rejected bearer — defeating 401 recovery in + exactly the case the branch existed to serve. + +### What ships instead + +One batch compare-and-swap, one `persist`, and a deliberately narrow eligibility +test. An alias is repaired only when it is provably an untouched duplicate of the +pre-refresh credential: same old grant fingerprint, same access token, same +expiry, and the same `chatgptAccountId` as the owner. Such an alias receives the +rotated access token, refresh token, and expiry **together**, so the generation +bump keeps meaning what every fence already assumes. `replacedAt` and the +validation metadata are preserved, because the probe-lease lineage check accepts +only an intact `G → G+1`. + +Anything else is left alone: a differing access token, a differing account id, or +a tombstone. The `chatgptAccountId` equality requirement is not decoration — a +fingerprint is `sha256` of the refresh token and carries no identity claim +(`src/codex/account-store.ts:62`), and no repository invariant guarantees one +grant cannot span two account ids. + +This is a **partial** close of gap 3, and the issue comment says so. Dormant +duplicates stop being retired for a grant we rotated away; a mixed alias still is. +Healing that case needs durable grant lineage and verified identity binding, which +the current fingerprint-and-generation model cannot express safely. + +The flight's returned `resolvedGrantFingerprint` stays the **old** fingerprint: +joiners wait on that key, and retagging it would make every legitimate joiner look +foreign. + +## Gap 4 — stale credential evidence writes unscoped state + +The reporter described an async interleaving between validation and mutation. That +part is wrong and worth stating: `recordCodexUpstreamOutcome` is synchronous +(`src/codex/routing.ts:2095`) and there is **no `await`** between the generation +check at `src/codex/routing.ts:2210` and the mutations at 2216–2223. The +same-process race the issue describes is not reachable. + +The cross-process race is real regardless. The check is an unlocked synchronous +store read (`src/codex/account-store.ts:186`) while writers coordinate under the +mutation lock, and OS preemption needs no `await`. The side effects then carry no +credential identity: health entries have no generation field, reauth state is a +bare `Set` fenced only by config generation, and affinity clearing removes +every entry for the account. + +Affinity is already self-invalidating on the next generation check. Health and +reauth are not. Rather than thread a generation through every health consumer, the +fix keeps the whole sequence synchronous and makes the *observable end state* +free of stale evidence: snapshot the health entry and the reauth flag, apply the +mutations, then re-validate the generation and roll back if it stopped being live. + +A cross-process replacement landing inside the window is therefore caught after +the fact instead of being prevented, which is the strongest guarantee available +without taking the config lock on the request path — the lock runs with +`busy_timeout=0`, so acquiring it per outcome would convert ordinary contention +into thrown request errors. Affinity clearing is deliberately not rolled back: it +is self-invalidating, and re-adding swept entries would be a worse bug than the +sweep. `recordCodexUpstreamOutcome` stays synchronous — many callers consume it as +`void` (`src/server/responses/core.ts:391`), so making it async would silently +leave mutations unawaited. + +## Constraints the audit flagged + +The refresh-flight map is keyed by the old grant (`src/codex/account-store.ts:315`) +and joiner provenance deliberately carries that old fingerprint, so alias +propagation must not disturb that ordering. The config lock runs with +`busy_timeout=0` and must stay synchronous, so the routing path must not acquire +it per outcome. Affinity requires exact credential-generation equality, so any +alias generation bump has to be reasoned about rather than assumed harmless. + +## Evidence standard + +Each regression is driven red by a named mutation, using the existing blocked-fetch +seam rather than a timing sleep. Any assertion that survives its mutation is +deleted rather than kept. diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index d919171364..d5570de478 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -218,6 +218,86 @@ export function saveCodexAccountCredentialIfGeneration( }); } +/** + * Commit a refreshed credential to its owner AND to any record that is provably an untouched + * duplicate of the pre-refresh credential (#2892 gap 3). + * + * A refresh normally rotates the refresh token, and the owner CAS above changes only the owner's + * record. A second non-deleted record holding the same grant that is not participating in the + * flight therefore keeps a refresh token upstream has just rotated away. Its next refresh sends a + * dead grant, and `invalid_grant` classifies as `revoked` — retiring a healthy account because we + * rotated its grant and never told it. + * + * Eligibility is deliberately narrow, and each condition earns its place: + * + * - Same pre-refresh grant fingerprint, access token, AND expiry. Anything else means the alias was + * updated concurrently, and repairing only its grant while keeping its own access token would + * advance a generation without advancing the access-token JWT. `plan-from-token` reads a higher + * generation as proof of a newer JWT (that is how JWT plan claims supersede a WHAM observation), + * so that combination lets a stale JWT overwrite an authoritative plan. It would also hand a live + * forced-refresh joiner back its own 401-rejected bearer: flights are keyed by grant and do not + * record participants, so a scan cannot tell a dormant alias from a joiner, and the recursion's + * freshness shortcut does not re-compare against the rejected token. + * - Same `chatgptAccountId` as the owner. A fingerprint is `sha256` of the refresh token and + * carries no identity claim; no invariant here guarantees one grant cannot span two account ids, + * so identity is compared rather than assumed. + * + * The rotated access token, refresh token, and expiry move together, keeping a generation bump + * meaning what every fence already assumes. `replacedAt` and the validation metadata survive + * because the probe-lease settlement check accepts only an intact `G → G+1` lineage. + * + * One lock acquisition and one `persist` for the owner and every alias: `persist` writes the whole + * store, so a second pass would open a window in which some records hold the dead grant. + */ +export function commitRefreshedCodexCredentialWithAliases( + id: string, + generation: number, + cred: CodexAccountCredentials, +): { committed: boolean; propagatedAliasIds: string[] } { + return withCredentialMutationLockSync(() => { + const store = loadCodexAccountRecordStore(); + const current = store[id]; + if (!current || current.generation !== generation || current.deletedAt != null || !current.credential) { + return { committed: false, propagatedAliasIds: [] }; + } + const priorCredential = current.credential; + const priorFingerprint = recordGrantFingerprint(current); + const refreshGrantFingerprint = priorCredential.refreshToken === cred.refreshToken + ? current.refreshGrantFingerprint ?? refreshGrantFingerprintForToken(cred.refreshToken) + : refreshGrantFingerprintForToken(cred.refreshToken); + store[id] = { + credential: cred, + generation: generation + 1, + refreshGrantFingerprint, + replacedAt: current.replacedAt, + ...preservedValidationMetadata(current), + }; + + const propagatedAliasIds: string[] = []; + // Nothing to propagate when the grant did not actually rotate: the aliases already hold it. + if (priorFingerprint !== undefined && priorCredential.refreshToken !== cred.refreshToken) { + for (const [aliasId, alias] of Object.entries(store)) { + if (aliasId === id || alias.deletedAt != null || !alias.credential) continue; + if (recordGrantFingerprint(alias) !== priorFingerprint) continue; + if (alias.credential.accessToken !== priorCredential.accessToken) continue; + if (alias.credential.expiresAt !== priorCredential.expiresAt) continue; + if (alias.credential.chatgptAccountId !== priorCredential.chatgptAccountId) continue; + store[aliasId] = { + // The alias keeps its OWN chatgptAccountId value, which the guard above proved equal. + credential: { ...cred, chatgptAccountId: alias.credential.chatgptAccountId }, + generation: alias.generation + 1, + refreshGrantFingerprint, + replacedAt: alias.replacedAt, + ...preservedValidationMetadata(alias), + }; + propagatedAliasIds.push(aliasId); + } + } + persist(store); + return { committed: true, propagatedAliasIds }; + }); +} + export function tombstoneCodexAccount(id: string): number { return withCredentialMutationLockSync(() => { const store = loadCodexAccountRecordStore(); @@ -745,9 +825,17 @@ async function resolveCodexToken( expiresAt: safeExpiresAt, chatgptAccountId: lockedCred.chatgptAccountId, }; - if (!saveCodexAccountCredentialIfGeneration(id, startGeneration, updated)) { + // Commit to the owner and, in the same write, to any record that is still an untouched + // duplicate of the credential this flight started from (#2892 gap 3). Without this the rotated + // grant reaches only the owner and live joiners, and a dormant same-grant record is left + // holding a refresh token upstream has invalidated. + const commit = commitRefreshedCodexCredentialWithAliases(id, startGeneration, updated); + if (!commit.committed) { throw new CodexCredentialGenerationConflictError(); } + if (commit.propagatedAliasIds.length > 0) { + console.warn(`[codex-auth] rotated refresh grant propagated to ${commit.propagatedAliasIds.length} dormant same-grant account record(s)`); + } return { accessToken: updated.accessToken, chatgptAccountId: updated.chatgptAccountId, diff --git a/src/codex/routing.ts b/src/codex/routing.ts index 24fce08cfa..041bb9b52b 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -5,7 +5,7 @@ import { codexAccountLogLabel } from "./account-label"; import { isCodexAccountPaused } from "./account-pause"; import { clearCodexAccountPin, codexAccountPriorityLookup, pinnedCodexAccountId } from "./account-priority"; import { isCodexAccountUsable, type CodexAccountUsabilityOptions } from "./account-usability"; -import { isAccountNeedsReauth, markAccountNeedsReauth } from "./account-runtime-state"; +import { clearAccountNeedsReauth, isAccountNeedsReauth, markAccountNeedsReauth } from "./account-runtime-state"; import { POOL_KEY_CODEX, normalizeAccountPoolStickyLimit, @@ -127,6 +127,18 @@ const upstreamHealth = new Map(); * from account-wide Retry-After/default throttles and transient health. */ const quotaScopedHealth = new Map>(); +/** + * Test-only stand-in for a concurrent credential write from another process, invoked between the + * 401 side effects and their re-validation (#2892 gap 4). Production leaves this undefined. + */ +let credentialRevalidationSeamForTests: ((accountId: string) => void) | undefined; + +/** Install (or clear with `null`) the concurrent-write seam used by the gap-4 regression. */ +export function setCodexCredentialRevalidationSeamForTests( + seam: ((accountId: string) => void) | null, +): void { + credentialRevalidationSeamForTests = seam ?? undefined; +} let lastReconciledGeneration = 0; let liveHealthAccountIds = new Set(); @@ -2213,6 +2225,27 @@ export function recordCodexUpstreamOutcome( ) { return; } + /* + * The pre-check above closes the same-process race, but not a cross-process one (#2892 gap 4). + * `isCodexAccountGenerationLive` is an unlocked read while credential writers coordinate under + * the mutation lock, and OS preemption needs no `await` — so another process can replace the + * credential between the check and these writes. Health and reauth carry no credential identity + * of their own (a health entry has no generation field, reauth is a bare id set), so a stale 401 + * that lands in that window would quarantine the REPLACEMENT credential. + * + * Taking the credential lock here is not an option: it runs with `busy_timeout=0`, so acquiring + * it per outcome would turn ordinary contention into thrown request-path errors. Instead the + * mutations stay synchronous and are re-validated afterwards, restoring the previous health and + * reauth state if the generation stopped being live. The window still exists; what changes is + * that its effects do not survive it. + * + * Affinity sweeping is deliberately NOT rolled back: an affinity entry already carries a + * credential generation and self-invalidates on the next check, and re-adding swept entries + * would be a worse bug than the sweep. + */ + const priorHealth = upstreamHealth.get(accountId); + const priorScopedHealth = quotaScopedHealth.get(accountId); + const priorNeedsReauth = isAccountNeedsReauth(accountId); upstreamHealth.set(accountId, { consecutiveFailures: 1, lastFailureStatus, @@ -2221,6 +2254,20 @@ export function recordCodexUpstreamOutcome( quotaScopedHealth.delete(accountId); markAccountNeedsReauth(accountId, writerGeneration); clearThreadAccountMapForAccount(accountId); + // The interleaving this guards against is a WRITE FROM ANOTHER PROCESS, which no single-process + // test can schedule: both reads below would observe the same store. Without a seam the rollback + // is unreachable in a test and any regression asserting it would be vacuous, so the seam exists + // to stand in for the other process. It is undefined in production and costs one null check. + credentialRevalidationSeamForTests?.(accountId); + if ( + meta.credentialGeneration !== undefined + && !isCodexAccountGenerationLive(accountId, meta.credentialGeneration) + ) { + if (priorHealth === undefined) upstreamHealth.delete(accountId); + else upstreamHealth.set(accountId, priorHealth); + if (priorScopedHealth !== undefined) quotaScopedHealth.set(accountId, priorScopedHealth); + if (!priorNeedsReauth) clearAccountNeedsReauth(accountId); + } return; } diff --git a/tests/codex-account-store.test.ts b/tests/codex-account-store.test.ts index 6dcebd4d1e..7e89469b65 100644 --- a/tests/codex-account-store.test.ts +++ b/tests/codex-account-store.test.ts @@ -1005,6 +1005,101 @@ describe("codex-account-store CRUD", () => { globalThis.fetch = originalFetch; } }); + + test("a successful refresh advances an untouched dormant same-grant alias in the same write (#2892 gap 3)", async () => { + const { getValidCodexToken, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../src/codex/account-store"); + const expiresAt = 0; + const shared = { refreshToken: "dormant-grant", expiresAt, chatgptAccountId: "acc" }; + // Owner drives the refresh. `dormant` is an untouched duplicate that never calls in — the + // record the rotated grant used to skip, leaving it to send a dead grant on its next refresh. + saveCodexAccountCredential("dormant-owner", { accessToken: "shared-old", ...shared }); + saveCodexAccountCredential("dormant-alias", { accessToken: "shared-old", ...shared }); + // Negative cases: each must be left strictly alone. + saveCodexAccountCredential("alias-other-account", { + accessToken: "shared-old", + refreshToken: "dormant-grant", + expiresAt, + chatgptAccountId: "different-acc", + }); + saveCodexAccountCredential("alias-moved-on", { accessToken: "already-newer", ...shared }); + saveCodexAccountCredential("alias-other-grant", { + accessToken: "shared-old", + refreshToken: "unrelated-grant", + expiresAt, + chatgptAccountId: "acc", + }); + const aliasGeneration = readCodexAccountRecord("dormant-alias")!.generation; + const otherAccountGeneration = readCodexAccountRecord("alias-other-account")!.generation; + const movedOnGeneration = readCodexAccountRecord("alias-moved-on")!.generation; + + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => Response.json({ + access_token: "rotated-access", + refresh_token: "rotated-grant", + expires_in: 3600, + })) as typeof fetch; + + try { + await getValidCodexToken("dormant-owner"); + + const owner = readCodexAccountRecord("dormant-owner")!; + expect(owner.credential).toMatchObject({ accessToken: "rotated-access", refreshToken: "rotated-grant" }); + + // The dormant alias adopts the rotated credential WHOLE — access token, refresh token, and + // expiry together — so its bumped generation still means "newer JWT", which is what the + // plan-from-token fence reads it as. + const alias = readCodexAccountRecord("dormant-alias")!; + expect(alias.credential?.refreshToken).toBe("rotated-grant"); + expect(alias.credential?.accessToken).toBe("rotated-access"); + expect(alias.credential?.expiresAt).toBe(owner.credential!.expiresAt); + expect(alias.credential?.chatgptAccountId).toBe("acc"); + expect(alias.generation).toBe(aliasGeneration + 1); + expect(alias.refreshGrantFingerprint).toBe(owner.refreshGrantFingerprint); + + // A same-grant record on a DIFFERENT chatgpt account is not provably the same identity: a + // fingerprint is sha256 of the refresh token and carries no identity claim. + const otherAccount = readCodexAccountRecord("alias-other-account")!; + expect(otherAccount.credential?.refreshToken).toBe("dormant-grant"); + expect(otherAccount.generation).toBe(otherAccountGeneration); + + // An alias whose access token already moved on must NOT be given a generation bump with a + // stale JWT, and must not be handed back a possibly-rejected bearer. + const movedOn = readCodexAccountRecord("alias-moved-on")!; + expect(movedOn.credential?.accessToken).toBe("already-newer"); + expect(movedOn.credential?.refreshToken).toBe("dormant-grant"); + expect(movedOn.generation).toBe(movedOnGeneration); + + expect(readCodexAccountRecord("alias-other-grant")!.credential?.refreshToken).toBe("unrelated-grant"); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("a tombstoned same-grant record is not resurrected by grant propagation (#2892 gap 3)", async () => { + const { getValidCodexToken, readCodexAccountRecord, saveCodexAccountCredential, tombstoneCodexAccount } = + await import("../src/codex/account-store"); + const shared = { accessToken: "tomb-old", refreshToken: "tomb-grant", expiresAt: 0, chatgptAccountId: "acc" }; + saveCodexAccountCredential("tomb-owner", { ...shared }); + saveCodexAccountCredential("tomb-deleted", { ...shared }); + tombstoneCodexAccount("tomb-deleted"); + + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => Response.json({ + access_token: "tomb-new", + refresh_token: "tomb-rotated", + expires_in: 3600, + })) as typeof fetch; + try { + await getValidCodexToken("tomb-owner"); + const deleted = readCodexAccountRecord("tomb-deleted")!; + expect(deleted.deletedAt).toBeGreaterThan(0); + expect(deleted.credential).toBeUndefined(); + } finally { + globalThis.fetch = originalFetch; + } + }); + }); describe("shared refresh flight plan reconciliation (#2892 gap 2 follow-up)", () => { diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index eb93bc14c2..dc58667004 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -31,10 +31,11 @@ import { resetCodexRoutingForManualSelection, resolveCodexAccountForThread, resolveCodexAccountForThreadDetailed, + setCodexCredentialRevalidationSeamForTests, tryAcquireCodexQuotaProbeLease, } from "../src/codex/routing"; import { clearPoolRotationState } from "../src/codex/pool-rotation"; -import { removeCodexAccountCredential, saveCodexAccountCredential } from "../src/codex/account-store"; +import { readCodexAccountRecord, removeCodexAccountCredential, saveCodexAccountCredential } from "../src/codex/account-store"; import { clearAccountNeedsReauth, clearAccountQuota, @@ -484,6 +485,51 @@ describe("codex routing", () => { expect(resolveCodexAccountForThread("credential-403-next", config)).toBe("b"); }); + test("a 401 racing a cross-process credential replacement leaves no health or reauth evidence (#2892 gap 4)", () => { + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + saveTestCredential("a"); + const generation = readCodexAccountRecord("a")!.generation; + // Establish a benign prior health entry so the rollback has to RESTORE it, not merely delete. + recordCodexUpstreamOutcome(config, "a", 503); + const priorHealth = getCodexUpstreamHealth("a"); + expect(priorHealth).toMatchObject({ lastFailureStatus: 503 }); + expect(isAccountNeedsReauth("a")).toBe(false); + + // Stand in for another process replacing the credential inside the window between the + // generation check and the side effects. A single process cannot schedule that write, which is + // why the seam exists: without it both reads see one store and the rollback is unreachable. + setCodexCredentialRevalidationSeamForTests(() => { saveTestCredential("a"); }); + try { + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); + } finally { + setCodexCredentialRevalidationSeamForTests(null); + } + + // The rejected credential is gone, so its 401 must not quarantine the replacement. + expect(isAccountNeedsReauth("a")).toBe(false); + expect(getCodexUpstreamHealth("a")).toMatchObject({ lastFailureStatus: 503 }); + expect(readCodexAccountRecord("a")!.generation).toBe(generation + 1); + }); + + test("a 401 on the live credential still quarantines the account (#2892 gap 4 does not over-roll-back)", () => { + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + saveTestCredential("a"); + const generation = readCodexAccountRecord("a")!.generation; + + // No concurrent replacement: the evidence is about the credential still in the store, so every + // side effect must survive. This is the assertion that stops the rollback from being a blanket + // "never quarantine" regression. + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); + + expect(isAccountNeedsReauth("a")).toBe(true); + expect(getCodexUpstreamHealth("a")).toMatchObject({ consecutiveFailures: 1, lastFailureStatus: 401 }); + }); + + test("connect failures contribute to transient failover", () => { const config = makeConfig(); updateAccountQuota("a", 10); From 33aa92e682f2f603559c89d763ff4614e06e4b3e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 02:57:49 +0900 Subject: [PATCH 2/6] fix(codex): scope 401 evidence by generation and reconcile alias plans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for the two generation-boundary defects @Ingwannu and the Codex reviewer found on a798e2dbf. Both were reproduced on that exact head; both are real, and my original approach to each was wrong. Gap 4 was not closed by a post-write re-read. A replacement can land at any point after `recordCodexUpstreamOutcome` returns, so re-reading the store inside it narrows the window and nothing more. The reproduction is simply: record a 401 at G, let the call return, then persist G+1 — the quarantine still applied to G+1. Evidence is now tagged with the credential it describes and checked when READ. `credentialFailureGeneration` records the generation a 401/403 came from, and the health readers drop a failure whose credential no longer exists. The reauth set became a map from account id to the justifying generation; `undefined` is preserved as an account-wide mark so a login flow holding no specific credential still quarantines unconditionally, and an account-wide mark outranks a scoped one. A healthy terminal retires the tag. Alias propagation installed the rotated JWT but not its plan. A plus -> pro rotation left the alias holding a Pro credential while its configured plan stayed plus, and its cached-token fast path never repairs that, so quota scoring and the 30-day projection stayed wrong until a restart or a WHAM refresh. Each propagated alias is now reconciled at its OWN committed generation, so the commit returns `{ id, generation }` pairs rather than ids: aliases need not share a generation and the plan note is generation-fenced. The test seam is gone. The regression now reproduces the real surviving ordering with no seam at all, which is strictly better than the seam it replaces. One assertion in the new plan test was vacuous when written: with one save per record, owner and alias generations coincided, so asserting the per-alias fence passed even with the owner's generation substituted. The fixture now advances the alias so they diverge, and that mutation turns red. 421 pass / 0 fail across codex-routing, codex-account-store, codex-plan, responses-pool-401-refresh, and codex-auth-api. Typecheck and privacy:scan clean. Mutations: unscoping the evidence turns the gap-4 test red while the live-credential test stays green; owner-only plan reconciliation turns the alias plan test red; the owner-generation fence turns it red only once generations diverge. --- .../260830_lane_q_2892_gaps_3_4/000_units.md | 52 ++++++++++++---- src/codex/account-runtime-state.ts | 44 +++++++++++-- src/codex/account-store.ts | 27 +++++--- src/codex/routing.ts | 61 +++++++++++-------- tests/codex-plan.test.ts | 49 +++++++++++++++ tests/codex-routing.test.ts | 33 +++++----- 6 files changed, 195 insertions(+), 71 deletions(-) diff --git a/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md b/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md index a8c7b27c2d..d1515809ed 100644 --- a/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md +++ b/devlog/_plan/260830_lane_q_2892_gaps_3_4/000_units.md @@ -82,20 +82,46 @@ bare `Set` fenced only by config generation, and affinity clearing remov every entry for the account. Affinity is already self-invalidating on the next generation check. Health and -reauth are not. Rather than thread a generation through every health consumer, the -fix keeps the whole sequence synchronous and makes the *observable end state* -free of stale evidence: snapshot the health entry and the reauth flag, apply the -mutations, then re-validate the generation and roll back if it stopped being live. - -A cross-process replacement landing inside the window is therefore caught after -the fact instead of being prevented, which is the strongest guarantee available -without taking the config lock on the request path — the lock runs with -`busy_timeout=0`, so acquiring it per outcome would convert ordinary contention -into thrown request errors. Affinity clearing is deliberately not rolled back: it -is self-invalidating, and re-adding swept entries would be a worse bug than the -sweep. `recordCodexUpstreamOutcome` stays synchronous — many callers consume it as +reauth were not. + +My first attempt snapshotted the state, mutated, then re-read the generation and +rolled back. Two reviewers independently rejected it, correctly: a replacement can +land at any point *after* `recordCodexUpstreamOutcome` returns, so a post-write +read narrows the window without closing it. @Ingwannu reproduced the surviving +ordering on the exact head — record a 401 at G, return, then persist G+1, and the +quarantine still applied to G+1. + +The evidence is now tagged with the credential it came from and checked when it is +*read*, which is what actually settles it. `credentialFailureGeneration` holds the +generation a 401/403 was derived from, and the health readers (`shouldFailover`, +`getCodexUpstreamHealth`) drop a failure whose credential is gone. The reauth set +became a map from account id to the generation that justified the flag, with +`undefined` preserved as an account-wide mark so a login flow with no specific +credential still quarantines unconditionally. + +Affinity clearing stays un-reverted: entries already carry a generation and +self-invalidate, so re-adding swept entries would be the worse bug. +`recordCodexUpstreamOutcome` stays synchronous — many callers consume it as `void` (`src/server/responses/core.ts:391`), so making it async would silently -leave mutations unawaited. +leave mutations unawaited. The config lock is still never taken on the request +path; it runs with `busy_timeout=0`, and per-outcome acquisition would turn +contention into request errors. + +## The alias plan note + +Review also caught that propagation installs the rotated JWT on an alias but left +its configured plan alone: a `plus → pro` rotation gave the alias a Pro credential +while its plan stayed `plus`, and the cached-token fast path never repairs that, so +quota scoring and the 30-day projection stayed wrong until a restart or a WHAM +refresh. Each propagated alias is now reconciled at its **own** committed +generation, which is why the commit returns `{ id, generation }` rather than ids — +aliases need not share a generation, and the plan note is generation-fenced. + +That last point produced the one genuinely vacuous assertion of this unit: with a +single `saveCodexAccountCredential` per record, owner and alias generations +coincided, so an assertion about the per-alias fence passed even when the code used +the owner's generation. The fixture now advances the alias twice so the generations +diverge, and the mutation turns red. ## Constraints the audit flagged diff --git a/src/codex/account-runtime-state.ts b/src/codex/account-runtime-state.ts index a2a6495e6a..ff036429d7 100644 --- a/src/codex/account-runtime-state.ts +++ b/src/codex/account-runtime-state.ts @@ -1,18 +1,43 @@ import { captureConfigGeneration, type GenerationContext } from "../lib/state-store-sweeper"; +import { isCodexAccountGenerationLive } from "./account-store"; -const reauthAccounts = new Set(); +/** + * Accounts quarantined for reauthentication, each remembering WHICH credential produced the + * evidence (#2892 gap 4). + * + * A 401 describes one credential, not an account. Recording only the id let a 401 raced by a + * cross-process credential replacement quarantine the replacement: the flag outlived the credential + * it was evidence about, and routing then refused a perfectly good credential until a restart. A + * post-write re-read cannot fix that — the replacement may land at any point after the write — so + * the generation travels WITH the flag and is checked when the flag is read. + * + * `undefined` means "no credential generation was supplied", which stays account-wide: callers such + * as a login flow have no specific credential in hand, and their quarantine must not silently expire. + */ +const reauthAccounts = new Map(); let lastReconciledGeneration = 0; let liveAccountIds = new Set(); -export function markAccountNeedsReauth(id: string, writerGeneration = captureConfigGeneration()): void { +export function markAccountNeedsReauth( + id: string, + writerGeneration = captureConfigGeneration(), + credentialGeneration?: number, +): void { if (writerGeneration < lastReconciledGeneration && !liveAccountIds.has(id)) return; - reauthAccounts.add(id); + // An account-wide mark supersedes a generation-scoped one: it is the stronger claim. + if (credentialGeneration === undefined || !reauthAccounts.has(id)) { + reauthAccounts.set(id, credentialGeneration); + return; + } + const existing = reauthAccounts.get(id); + if (existing === undefined) return; + reauthAccounts.set(id, Math.max(existing, credentialGeneration)); } export function reconcileCodexReauthState(context: GenerationContext): number { if (context.generation <= lastReconciledGeneration) return 0; let removed = 0; - for (const id of reauthAccounts) { + for (const id of [...reauthAccounts.keys()]) { if (context.codexAccountIds.has(id)) continue; reauthAccounts.delete(id); removed += 1; @@ -23,7 +48,16 @@ export function reconcileCodexReauthState(context: GenerationContext): number { } export function isAccountNeedsReauth(id: string): boolean { - return reauthAccounts.has(id); + if (!reauthAccounts.has(id)) return false; + const credentialGeneration = reauthAccounts.get(id); + if (credentialGeneration === undefined) return true; + // The credential this evidence describes is gone, so the evidence is spent. Drop it rather than + // re-deriving the same answer on every read. + if (!isCodexAccountGenerationLive(id, credentialGeneration)) { + reauthAccounts.delete(id); + return false; + } + return true; } export function clearAccountNeedsReauth(id: string): void { diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index d5570de478..15a9f83d78 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -253,12 +253,12 @@ export function commitRefreshedCodexCredentialWithAliases( id: string, generation: number, cred: CodexAccountCredentials, -): { committed: boolean; propagatedAliasIds: string[] } { +): { committed: boolean; propagatedAliases: { id: string; generation: number }[] } { return withCredentialMutationLockSync(() => { const store = loadCodexAccountRecordStore(); const current = store[id]; if (!current || current.generation !== generation || current.deletedAt != null || !current.credential) { - return { committed: false, propagatedAliasIds: [] }; + return { committed: false, propagatedAliases: [] }; } const priorCredential = current.credential; const priorFingerprint = recordGrantFingerprint(current); @@ -273,7 +273,9 @@ export function commitRefreshedCodexCredentialWithAliases( ...preservedValidationMetadata(current), }; - const propagatedAliasIds: string[] = []; + // Each alias carries its OWN committed generation: aliases need not share one, and the plan + // reconciliation below is generation-fenced, so an id alone would be reconciled at the wrong fence. + const propagatedAliases: { id: string; generation: number }[] = []; // Nothing to propagate when the grant did not actually rotate: the aliases already hold it. if (priorFingerprint !== undefined && priorCredential.refreshToken !== cred.refreshToken) { for (const [aliasId, alias] of Object.entries(store)) { @@ -282,19 +284,20 @@ export function commitRefreshedCodexCredentialWithAliases( if (alias.credential.accessToken !== priorCredential.accessToken) continue; if (alias.credential.expiresAt !== priorCredential.expiresAt) continue; if (alias.credential.chatgptAccountId !== priorCredential.chatgptAccountId) continue; + const aliasGeneration = alias.generation + 1; store[aliasId] = { // The alias keeps its OWN chatgptAccountId value, which the guard above proved equal. credential: { ...cred, chatgptAccountId: alias.credential.chatgptAccountId }, - generation: alias.generation + 1, + generation: aliasGeneration, refreshGrantFingerprint, replacedAt: alias.replacedAt, ...preservedValidationMetadata(alias), }; - propagatedAliasIds.push(aliasId); + propagatedAliases.push({ id: aliasId, generation: aliasGeneration }); } } persist(store); - return { committed: true, propagatedAliasIds }; + return { committed: true, propagatedAliases }; }); } @@ -833,8 +836,16 @@ async function resolveCodexToken( if (!commit.committed) { throw new CodexCredentialGenerationConflictError(); } - if (commit.propagatedAliasIds.length > 0) { - console.warn(`[codex-auth] rotated refresh grant propagated to ${commit.propagatedAliasIds.length} dormant same-grant account record(s)`); + if (commit.propagatedAliases.length > 0) { + console.warn(`[codex-auth] rotated refresh grant propagated to ${commit.propagatedAliases.length} dormant same-grant account record(s)`); + // The rotated JWT can carry a different `chatgpt_plan_type`, and each alias now holds that + // JWT. Without this the alias keeps its old plan — its cached-token fast path never + // reconciles, and repository-wide reconciliation only runs at startup — so quota scoring and + // the 30-day projection stay wrong until a restart or a WHAM refresh. Reconcile at each + // alias's OWN committed generation, since the plan note is generation-fenced. + for (const alias of commit.propagatedAliases) { + await notePlanFromRefreshedAccessToken(alias.id, updated.accessToken, alias.generation); + } } return { accessToken: updated.accessToken, diff --git a/src/codex/routing.ts b/src/codex/routing.ts index 041bb9b52b..cc9a3a1694 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -128,16 +128,27 @@ const upstreamHealth = new Map(); */ const quotaScopedHealth = new Map>(); /** - * Test-only stand-in for a concurrent credential write from another process, invoked between the - * 401 side effects and their re-validation (#2892 gap 4). Production leaves this undefined. + * Credential generation each 401/403 quarantine was derived from (#2892 gap 4). + * + * Health is keyed by account id, but credential evidence describes one CREDENTIAL. A replacement + * from another process can land at any point after the outcome is recorded, so re-reading the store + * right after the write narrows the window without closing it. Recording the generation alongside + * the failure lets the READER decide, which is what actually settles the race: once the credential + * is gone its failure is spent, and routing stops holding the replacement down. */ -let credentialRevalidationSeamForTests: ((accountId: string) => void) | undefined; +const credentialFailureGeneration = new Map(); -/** Install (or clear with `null`) the concurrent-write seam used by the gap-4 regression. */ -export function setCodexCredentialRevalidationSeamForTests( - seam: ((accountId: string) => void) | null, -): void { - credentialRevalidationSeamForTests = seam ?? undefined; +/** + * Drop a credential-failure health entry whose credential no longer exists, and report whether it + * was spent. Called from the health readers so a stale 401 cannot outlive its credential. + */ +function dropSpentCredentialFailure(accountId: string): boolean { + const generation = credentialFailureGeneration.get(accountId); + if (generation === undefined) return false; + if (isCodexAccountGenerationLive(accountId, generation)) return false; + credentialFailureGeneration.delete(accountId); + upstreamHealth.delete(accountId); + return true; } let lastReconciledGeneration = 0; let liveHealthAccountIds = new Set(); @@ -294,12 +305,14 @@ export function clearThreadAccountMapForAccount(accountId: string): void { export function clearCodexUpstreamHealth(): void { upstreamHealth.clear(); quotaScopedHealth.clear(); + credentialFailureGeneration.clear(); runtimeActiveCodexAccountId = undefined; } export function clearCodexUpstreamHealthForAccount(accountId: string): void { upstreamHealth.delete(accountId); quotaScopedHealth.delete(accountId); + credentialFailureGeneration.delete(accountId); } export function reconcileCodexRoutingHealth(context: GenerationContext): number { @@ -308,6 +321,7 @@ export function reconcileCodexRoutingHealth(context: GenerationContext): number for (const accountId of upstreamHealth.keys()) { if (context.codexAccountIds.has(accountId)) continue; upstreamHealth.delete(accountId); + credentialFailureGeneration.delete(accountId); removed += 1; } for (const accountId of quotaScopedHealth.keys()) { @@ -323,6 +337,7 @@ export function reconcileCodexRoutingHealth(context: GenerationContext): number export function getCodexUpstreamHealth( accountId: string, ): CodexUpstreamHealth | null { + dropSpentCredentialFailure(accountId); return upstreamHealth.get(accountId) ?? null; } @@ -1599,6 +1614,7 @@ function applyQuotaAutoSwitch( function shouldFailover(config: OcxConfig, accountId: string, now: number): boolean { const threshold = config.upstreamFailoverThreshold ?? 3; if (threshold <= 0) return false; + dropSpentCredentialFailure(accountId); const health = upstreamHealth.get(accountId); if (health?.lastFailureAt && now - health.lastFailureAt > CODEX_FAILURE_WINDOW_MS) return false; return !!health && health.consecutiveFailures >= threshold; @@ -2124,6 +2140,9 @@ export function recordCodexUpstreamOutcome( const outcomeClass = classifyCodexUpstreamOutcome(outcome, meta.denial); const quotaScope = codexQuotaScopeForModel(meta.modelId); if (outcomeClass === "success") { + // A healthy terminal retires any credential-failure tag: whatever the 401 described, this + // account is answering now. + credentialFailureGeneration.delete(accountId); const scopedProbe = meta.probeQuotaScope ? scopedHealthFor(accountId, meta.probeQuotaScope) : undefined; @@ -2243,31 +2262,21 @@ export function recordCodexUpstreamOutcome( * credential generation and self-invalidates on the next check, and re-adding swept entries * would be a worse bug than the sweep. */ - const priorHealth = upstreamHealth.get(accountId); - const priorScopedHealth = quotaScopedHealth.get(accountId); - const priorNeedsReauth = isAccountNeedsReauth(accountId); upstreamHealth.set(accountId, { consecutiveFailures: 1, lastFailureStatus, lastFailureAt: now, }); quotaScopedHealth.delete(accountId); - markAccountNeedsReauth(accountId, writerGeneration); - clearThreadAccountMapForAccount(accountId); - // The interleaving this guards against is a WRITE FROM ANOTHER PROCESS, which no single-process - // test can schedule: both reads below would observe the same store. Without a seam the rollback - // is unreachable in a test and any regression asserting it would be vacuous, so the seam exists - // to stand in for the other process. It is undefined in production and costs one null check. - credentialRevalidationSeamForTests?.(accountId); - if ( - meta.credentialGeneration !== undefined - && !isCodexAccountGenerationLive(accountId, meta.credentialGeneration) - ) { - if (priorHealth === undefined) upstreamHealth.delete(accountId); - else upstreamHealth.set(accountId, priorHealth); - if (priorScopedHealth !== undefined) quotaScopedHealth.set(accountId, priorScopedHealth); - if (!priorNeedsReauth) clearAccountNeedsReauth(accountId); + // Tag both the health entry and the reauth flag with the credential this evidence came from, so + // a replacement landing after this call cannot inherit a quarantine that was never about it. + if (meta.credentialGeneration !== undefined) { + credentialFailureGeneration.set(accountId, meta.credentialGeneration); + } else { + credentialFailureGeneration.delete(accountId); } + markAccountNeedsReauth(accountId, writerGeneration, meta.credentialGeneration); + clearThreadAccountMapForAccount(accountId); return; } diff --git a/tests/codex-plan.test.ts b/tests/codex-plan.test.ts index 0c4b9c0936..788264e1da 100644 --- a/tests/codex-plan.test.ts +++ b/tests/codex-plan.test.ts @@ -198,3 +198,52 @@ describe("WHAM-wins plan provenance gate (release-audit fix)", () => { }); }); + +describe("rotated-JWT plan reconciliation across propagated aliases (#2892 gap 3)", () => { + test("a plan-changing rotated JWT reconciles the alias, not just the refresh owner", async () => { + const { getValidCodexToken, readCodexAccountRecord } = await import("../src/codex/account-store"); + const oldJwt = chatgptPlanJwt("plus"); + const shared = { refreshToken: "plan-grant", expiresAt: 0, chatgptAccountId: "acct" }; + saveCodexAccountCredential("plan-owner", { accessToken: oldJwt, ...shared }); + saveCodexAccountCredential("plan-alias", { accessToken: oldJwt, ...shared }); + // Advance the alias so its generation DIVERGES from the owner's. Without this both land on the + // same number and an assertion about the per-alias fence would pass even if the code used the + // owner's generation — a vacuous test. Re-saving the identical credential keeps the record an + // eligible untouched duplicate while bumping only its generation. + saveCodexAccountCredential("plan-alias", { accessToken: oldJwt, ...shared }); + saveCodexAccountCredential("plan-alias", { accessToken: oldJwt, ...shared }); + saveConfig({ + ...loadConfig(), + codexAccounts: [ + { id: "plan-owner", email: "owner@test", plan: "plus" }, + { id: "plan-alias", email: "alias@test", plan: "plus" }, + ], + } as OcxConfig); + + const rotatedJwt = chatgptPlanJwt("pro"); + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => Response.json({ + access_token: rotatedJwt, + refresh_token: "plan-grant-rotated", + expires_in: 3600, + })) as typeof fetch; + try { + await getValidCodexToken("plan-owner"); + + // The alias holds the rotated Pro JWT after propagation... + const alias = readCodexAccountRecord("plan-alias")!; + expect(alias.credential?.accessToken).toBe(rotatedJwt); + + // ...so its configured plan must be reconciled too. Reconciling only the owner left the alias + // on "plus" while carrying a Pro credential, and its cached-token fast path never repairs + // that, so quota scoring stayed wrong until a restart or a WHAM refresh. + const accounts = loadConfig().codexAccounts ?? []; + expect(accounts.find(a => a.id === "plan-owner")?.plan).toBe("pro"); + expect(accounts.find(a => a.id === "plan-alias")?.plan).toBe("pro"); + // The alias is fenced at its OWN committed generation, not the owner's. + expect(accounts.find(a => a.id === "plan-alias")?.planCredentialGeneration).toBe(alias.generation); + } finally { + globalThis.fetch = originalFetch; + } + }); +}); diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index dc58667004..5e3c7fa307 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -31,7 +31,6 @@ import { resetCodexRoutingForManualSelection, resolveCodexAccountForThread, resolveCodexAccountForThreadDetailed, - setCodexCredentialRevalidationSeamForTests, tryAcquireCodexQuotaProbeLease, } from "../src/codex/routing"; import { clearPoolRotationState } from "../src/codex/pool-rotation"; @@ -485,32 +484,28 @@ describe("codex routing", () => { expect(resolveCodexAccountForThread("credential-403-next", config)).toBe("b"); }); - test("a 401 racing a cross-process credential replacement leaves no health or reauth evidence (#2892 gap 4)", () => { + test("a 401 does not quarantine a credential that replaced the rejected one AFTER the outcome (#2892 gap 4)", () => { const config = makeConfig(); updateAccountQuota("a", 10); updateAccountQuota("b", 20); saveTestCredential("a"); const generation = readCodexAccountRecord("a")!.generation; - // Establish a benign prior health entry so the rollback has to RESTORE it, not merely delete. - recordCodexUpstreamOutcome(config, "a", 503); - const priorHealth = getCodexUpstreamHealth("a"); - expect(priorHealth).toMatchObject({ lastFailureStatus: 503 }); - expect(isAccountNeedsReauth("a")).toBe(false); - // Stand in for another process replacing the credential inside the window between the - // generation check and the side effects. A single process cannot schedule that write, which is - // why the seam exists: without it both reads see one store and the rollback is unreachable. - setCodexCredentialRevalidationSeamForTests(() => { saveTestCredential("a"); }); - try { - recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); - } finally { - setCodexCredentialRevalidationSeamForTests(null); - } + // Record the 401 while the rejected credential is still the live one, so every side effect is + // legitimately applied. This is the ordering @Ingwannu reproduced: the replacement lands AFTER + // recordCodexUpstreamOutcome returns, which no post-write re-read inside it can ever observe. + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); + expect(isAccountNeedsReauth("a")).toBe(true); + expect(getCodexUpstreamHealth("a")).toMatchObject({ consecutiveFailures: 1, lastFailureStatus: 401 }); - // The rejected credential is gone, so its 401 must not quarantine the replacement. - expect(isAccountNeedsReauth("a")).toBe(false); - expect(getCodexUpstreamHealth("a")).toMatchObject({ lastFailureStatus: 503 }); + // Another process replaces the credential. The 401 was evidence about a credential that no + // longer exists, so it must not hold the replacement out of rotation. + saveTestCredential("a"); expect(readCodexAccountRecord("a")!.generation).toBe(generation + 1); + + expect(isAccountNeedsReauth("a")).toBe(false); + expect(getCodexUpstreamHealth("a")).toBeNull(); + expect(resolveCodexAccountForThread("gap4-replacement-selectable", config)).toBe("a"); }); test("a 401 on the live credential still quarantines the account (#2892 gap 4 does not over-roll-back)", () => { From fe99acf13ad063cda23ad0304bde0e467dee7642 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 03:31:24 +0900 Subject: [PATCH 3/6] fix(codex): own credential provenance per health entry and fence sidecar 401s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on f8e7bbdc4. All three boundaries @Ingwannu reproduced were real; the first was a defect my own previous commit introduced. 1. Provenance now lives ON the health entry, not in a side map keyed by account id. The side map meant "spend whatever health is current when the old credential is found dead", so a G1 401 followed by a G2 save and a genuine G2 503 deleted the 503. `credentialFailureGeneration` is a field on CodexUpstreamHealth, only an entry carrying it can be spent, and any later write replaces the entry along with the tag. `preservedCooldownFields` explicitly drops it, or a cooldown write would inherit provenance belonging to a different failure — which is exactly the ownership bug one level down. 2. Stored-pool sidecar outcomes carry the generation. `sidecarOutcomeRecorder` and both `recordOutcome` closures in openai-sidecar could take a vision or web-search 401/403 and record it account-wide, so the replacement inherited the quarantine. All three now pass `credentialGeneration` for `kind === "pool"`. `main-pool` keeps unfenced semantics deliberately: it has no stored-record generation. 3. `findFreshCredentialForGrant` requires identity equality. Adoption copies both the access and refresh tokens, so a shared grant fingerprint was never sufficient proof that two records are the same upstream account. Both `chatgptAccountId` values must be non-empty and exactly equal; without an expected identity the function adopts nothing. Regressions, each mutation-proven: G1 401 -> G2 save -> G2 503 keeps the 503, and the same for a workspace-denial overwrite; a sidecar 401 does not quarantine the replacement; a same-grant sibling on a different identity is never adopted and is itself left untouched. Removing each guard turns its own test red. 494 pass / 0 fail across codex-routing, codex-account-store, codex-plan, responses-pool-401-refresh, codex-auth-api, vision-sidecar-e2e, and web-search. Typecheck and privacy:scan clean. Still open and acknowledged: if #2933 lands first, the alias plan settlement should be unified with its shared-flight path rather than kept as two loops. --- src/codex/account-store.ts | 9 +++++ src/codex/routing.ts | 65 ++++++++++++++++--------------- src/providers/openai-sidecar.ts | 5 +++ src/server/responses/core.ts | 5 +++ tests/codex-account-store.test.ts | 43 ++++++++++++++++++++ tests/codex-routing.test.ts | 63 ++++++++++++++++++++++++++++++ 6 files changed, 158 insertions(+), 32 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 15a9f83d78..59e6bfd92b 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -477,12 +477,20 @@ function findFreshCredentialForGrant( refreshGrantFingerprint: string, excludeId: string, rejectedAccessToken?: string, + expectedChatgptAccountId?: string, ): CodexAccountCredentials | null { const now = Date.now(); const records = loadCodexAccountRecordStore(); + // Adoption copies another record's access AND refresh tokens onto the caller, so the two records + // must be the same upstream identity. A grant fingerprint is `sha256` of the refresh token and + // carries no identity claim, and nothing here guarantees one grant cannot span two accounts, so + // require both ids to be present and exactly equal rather than inferring identity from the grant. + if (!expectedChatgptAccountId) return null; for (const [candidateId, candidate] of Object.entries(records)) { if (candidateId === excludeId || candidate.deletedAt != null || !candidate.credential) continue; if (recordGrantFingerprint(candidate) !== refreshGrantFingerprint) continue; + if (!candidate.credential.chatgptAccountId) continue; + if (candidate.credential.chatgptAccountId !== expectedChatgptAccountId) continue; // A sibling alias can hold a still-unexpired copy of the exact token upstream // just rejected. Reusing it would bump the generation and replay the identical // bearer — a second 401 dressed up as recovery. @@ -762,6 +770,7 @@ async function resolveCodexToken( refreshGrantFingerprint, id, forced?.rejectedAccessToken, + lockedCred.chatgptAccountId, ); if (sameGrantFreshCredential) { if (!saveCodexAccountCredentialIfGeneration(id, startGeneration, sameGrantFreshCredential)) { diff --git a/src/codex/routing.ts b/src/codex/routing.ts index cc9a3a1694..483d57c2f2 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -91,6 +91,15 @@ type CodexUpstreamHealth = { * flaky account without throwing CodexAccountCooldownError (hard-only). */ softAvoidUntil?: number; + /** + * Credential generation a 401/403 quarantine was derived from (#2892 gap 4). + * + * Provenance lives ON the entry rather than in a side map keyed by account id. A side map spends + * "whatever health is current when the old credential is found dead", which deletes a later + * unrelated entry: a G1 401, then a G2 save, then a genuine G2 503 would lose the 503. Only the + * entry that carries this field can be spent, and any later write simply replaces it. + */ + credentialFailureGeneration?: number; }; const CODEX_DEFAULT_QUOTA_COOLDOWN_MS = 60_000; @@ -128,27 +137,20 @@ const upstreamHealth = new Map(); */ const quotaScopedHealth = new Map>(); /** - * Credential generation each 401/403 quarantine was derived from (#2892 gap 4). + * Spend a credential-failure health entry whose credential no longer exists (#2892 gap 4). * - * Health is keyed by account id, but credential evidence describes one CREDENTIAL. A replacement - * from another process can land at any point after the outcome is recorded, so re-reading the store - * right after the write narrows the window without closing it. Recording the generation alongside - * the failure lets the READER decide, which is what actually settles the race: once the credential - * is gone its failure is spent, and routing stops holding the replacement down. - */ -const credentialFailureGeneration = new Map(); - -/** - * Drop a credential-failure health entry whose credential no longer exists, and report whether it - * was spent. Called from the health readers so a stale 401 cannot outlive its credential. + * A 401/403 describes one CREDENTIAL, not an account, and a replacement can land at any point after + * the outcome is recorded — so re-reading the store inside `recordCodexUpstreamOutcome` narrows the + * window without closing it. The reader decides instead, and it may only spend an entry that + * actually carries credential provenance: a later transient or quota write replaces the entry and + * with it the tag, so this can never delete evidence that belongs to a different failure. */ -function dropSpentCredentialFailure(accountId: string): boolean { - const generation = credentialFailureGeneration.get(accountId); - if (generation === undefined) return false; - if (isCodexAccountGenerationLive(accountId, generation)) return false; - credentialFailureGeneration.delete(accountId); +function dropSpentCredentialFailure(accountId: string): void { + const health = upstreamHealth.get(accountId); + const generation = health?.credentialFailureGeneration; + if (health === undefined || generation === undefined) return; + if (isCodexAccountGenerationLive(accountId, generation)) return; upstreamHealth.delete(accountId); - return true; } let lastReconciledGeneration = 0; let liveHealthAccountIds = new Set(); @@ -305,14 +307,12 @@ export function clearThreadAccountMapForAccount(accountId: string): void { export function clearCodexUpstreamHealth(): void { upstreamHealth.clear(); quotaScopedHealth.clear(); - credentialFailureGeneration.clear(); runtimeActiveCodexAccountId = undefined; } export function clearCodexUpstreamHealthForAccount(accountId: string): void { upstreamHealth.delete(accountId); quotaScopedHealth.delete(accountId); - credentialFailureGeneration.delete(accountId); } export function reconcileCodexRoutingHealth(context: GenerationContext): number { @@ -321,7 +321,6 @@ export function reconcileCodexRoutingHealth(context: GenerationContext): number for (const accountId of upstreamHealth.keys()) { if (context.codexAccountIds.has(accountId)) continue; upstreamHealth.delete(accountId); - credentialFailureGeneration.delete(accountId); removed += 1; } for (const accountId of quotaScopedHealth.keys()) { @@ -717,7 +716,13 @@ function withProbeLeaseReleased(health: CodexUpstreamHealth, now: number): Codex */ function preservedCooldownFields(health: CodexUpstreamHealth | undefined): Partial { if (!health) return {}; - const { consecutiveFailures: _f, consecutiveSuccesses: _s, lastFailureStatus: _st, lastFailureAt: _at, softAvoidUntil: _sa, ...cooldownFields } = health; + // `credentialFailureGeneration` is provenance for ONE credential failure, so it must not survive + // into a later transient or quota entry — otherwise that entry inherits the tag and gets spent + // when the old credential dies, deleting evidence that was never about it (#2892 gap 4 review). + const { + consecutiveFailures: _f, consecutiveSuccesses: _s, lastFailureStatus: _st, lastFailureAt: _at, + softAvoidUntil: _sa, credentialFailureGeneration: _cg, ...cooldownFields + } = health; return cooldownFields; } @@ -2140,9 +2145,6 @@ export function recordCodexUpstreamOutcome( const outcomeClass = classifyCodexUpstreamOutcome(outcome, meta.denial); const quotaScope = codexQuotaScopeForModel(meta.modelId); if (outcomeClass === "success") { - // A healthy terminal retires any credential-failure tag: whatever the 401 described, this - // account is answering now. - credentialFailureGeneration.delete(accountId); const scopedProbe = meta.probeQuotaScope ? scopedHealthFor(accountId, meta.probeQuotaScope) : undefined; @@ -2266,15 +2268,14 @@ export function recordCodexUpstreamOutcome( consecutiveFailures: 1, lastFailureStatus, lastFailureAt: now, + // Provenance rides on the entry: only this failure can be spent when its credential dies. + ...(meta.credentialGeneration !== undefined + ? { credentialFailureGeneration: meta.credentialGeneration } + : {}), }); quotaScopedHealth.delete(accountId); - // Tag both the health entry and the reauth flag with the credential this evidence came from, so - // a replacement landing after this call cannot inherit a quarantine that was never about it. - if (meta.credentialGeneration !== undefined) { - credentialFailureGeneration.set(accountId, meta.credentialGeneration); - } else { - credentialFailureGeneration.delete(accountId); - } + // The reauth flag carries the same provenance, so a replacement landing after this call cannot + // inherit a quarantine that was never about it. markAccountNeedsReauth(accountId, writerGeneration, meta.credentialGeneration); clearThreadAccountMapForAccount(accountId); return; diff --git a/src/providers/openai-sidecar.ts b/src/providers/openai-sidecar.ts index 892b08462b..70e788882b 100644 --- a/src/providers/openai-sidecar.ts +++ b/src/providers/openai-sidecar.ts @@ -140,6 +140,9 @@ export async function resolveFirstUsableOpenAiSidecar( probeLeaseId: authContext.probeLeaseId, probeQuotaScope: authContext.probeQuotaScope, writerGeneration: authContext.writerGeneration, + // 401/403 here is evidence about this exact stored credential; without the generation a + // replacement inherits the quarantine (#2892 gap 4). + ...(authContext.kind === "pool" ? { credentialGeneration: authContext.generation } : {}), }, ), }; @@ -172,6 +175,8 @@ export async function resolveFirstUsableOpenAiSidecar( threadId: authContext.affinityKey, probeLeaseId: authContext.probeLeaseId, writerGeneration: authContext.writerGeneration, + // Same fence as the exact-account recorder above (#2892 gap 4). + ...(authContext.kind === "pool" ? { credentialGeneration: authContext.generation } : {}), }, ), } diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 4557af6c03..b06237fac9 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -399,6 +399,11 @@ export function sidecarOutcomeRecorder( probeLeaseId: authCtx.probeLeaseId, probeQuotaScope: authCtx.probeQuotaScope, writerGeneration: authCtx.writerGeneration, + // A vision or web-search sidecar can return 401/403, and that is evidence about the exact + // stored credential it used. Without the generation it becomes an account-wide quarantine + // that a replacement inherits (#2892 gap 4). `main-pool` has no stored-record generation, so + // it keeps the unfenced account-wide semantics. + ...(authCtx.kind === "pool" ? { credentialGeneration: authCtx.generation } : {}), }) : undefined; } diff --git a/tests/codex-account-store.test.ts b/tests/codex-account-store.test.ts index 7e89469b65..99978bd4a0 100644 --- a/tests/codex-account-store.test.ts +++ b/tests/codex-account-store.test.ts @@ -1100,6 +1100,49 @@ describe("codex-account-store CRUD", () => { } }); + + test("a same-grant sibling on a DIFFERENT upstream identity is never adopted (#2892 review)", async () => { + const { forceRefreshCodexPoolToken, getCodexAccountCredential, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../src/codex/account-store"); + // Both records share one stored grant, but they claim different upstream accounts. Adoption + // copies BOTH tokens, so treating a shared fingerprint as proof of identity would hand this + // caller another account's credential. + saveCodexAccountCredential("foreign-caller", { + accessToken: "rejected-token", + refreshToken: "foreign-grant", + expiresAt: Date.now() + 3600_000, + chatgptAccountId: "acct-one", + }); + saveCodexAccountCredential("foreign-sibling", { + accessToken: "sibling-fresh", + refreshToken: "foreign-grant", + expiresAt: Date.now() + 3600_000, + chatgptAccountId: "acct-two", + }); + const generation = readCodexAccountRecord("foreign-caller")!.generation; + + const originalFetch = globalThis.fetch; + let tokenCalls = 0; + globalThis.fetch = (async () => { + tokenCalls += 1; + return Response.json({ access_token: "own-new", refresh_token: "own-rotated", expires_in: 3600 }); + }) as typeof fetch; + try { + const result = await forceRefreshCodexPoolToken("foreign-caller", { + rejectedGeneration: generation, + rejectedAccessToken: "rejected-token", + }); + // A real refresh must have run instead of adopting the foreign sibling. + expect(tokenCalls).toBe(1); + expect(result.accessToken).toBe("own-new"); + expect(getCodexAccountCredential("foreign-caller")?.accessToken).not.toBe("sibling-fresh"); + // The sibling is untouched: this path must not write to another identity's record. + expect(getCodexAccountCredential("foreign-sibling")?.accessToken).toBe("sibling-fresh"); + } finally { + globalThis.fetch = originalFetch; + } + }); + }); describe("shared refresh flight plan reconciliation (#2892 gap 2 follow-up)", () => { diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index 5e3c7fa307..277b00bbd1 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -525,6 +525,69 @@ describe("codex routing", () => { }); + test("a later transient failure is not deleted by a spent credential-failure tag (#2892 gap 4 review)", () => { + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + saveTestCredential("a"); + const generation = readCodexAccountRecord("a")!.generation; + + // G1 401, then the credential is replaced, then a GENUINE 503 against G2 — all before any + // health read. Provenance keyed only by account id would spend "whatever health is current" + // and delete this 503; provenance on the entry cannot, because the 503 write replaced the tag. + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); + saveTestCredential("a"); + recordCodexUpstreamOutcome(config, "a", 503); + + expect(getCodexUpstreamHealth("a")).toMatchObject({ lastFailureStatus: 503 }); + expect(isAccountNeedsReauth("a")).toBe(false); + }); + + test("a workspace denial overwriting a spent credential failure survives the read (#2892 gap 4 review)", () => { + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + saveTestCredential("a"); + const generation = readCodexAccountRecord("a")!.generation; + + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); + saveTestCredential("a"); + // A workspace denial is a different ownership class and must not be collateral damage. + recordCodexUpstreamOutcome(config, "a", 403, { denial: "workspace" }); + + expect(getCodexUpstreamHealth("a")).toMatchObject({ lastFailureStatus: 403 }); + }); + + + test("a sidecar 401 does not quarantine the credential that replaced it (#2892 gap 4 review)", async () => { + const { sidecarOutcomeRecorder } = await import("../src/server/responses/core"); + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + saveTestCredential("a"); + const generation = readCodexAccountRecord("a")!.generation; + + // A vision or web-search sidecar returns 401 for a stored Pool credential. Recording that + // without the credential generation produced an account-wide quarantine, so the replacement + // inherited it and the account stayed unroutable. + const record = sidecarOutcomeRecorder(config, { + kind: "pool", + accountId: "a", + writerGeneration: 0, + generation, + accessToken: "access-a", + chatgptAccountId: "acct-a", + }); + expect(record).toBeDefined(); + record!(401); + expect(isAccountNeedsReauth("a")).toBe(true); + + saveTestCredential("a"); + expect(isAccountNeedsReauth("a")).toBe(false); + expect(getCodexUpstreamHealth("a")).toBeNull(); + }); + + test("connect failures contribute to transient failover", () => { const config = makeConfig(); updateAccountQuota("a", 10); From 7d95449244f82396557ffb5ff6a125fff4b24940 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 03:45:50 +0900 Subject: [PATCH 4/6] refactor(codex): settle alias plans on the shared flight, not the commit Rebased onto dev after #2933 landed and unified the two plan-settlement paths, as agreed on that PR rather than leaving two competing loops. #2933 moved the owner's plan reconciliation onto the flight, because a flight outlives the caller that opened it and an aborted owner would otherwise drop the note entirely. My alias propagation had its own loop inside the credential commit, which would have made two places responsible for the same concern. The committed aliases now travel on CodexRefreshResult, and the flight settles the owner and every alias in one place. That inherits #2933's guarantee for free: an aborted owner still reconciles alias plans, which the commit-site loop could only have done for whoever happened to be waiting. Each alias keeps its own committed generation, since the plan note is generation-fenced. The joiner-CAS branch is deliberately untouched, matching #2933's boundary. 426 pass / 0 fail across codex-routing, codex-account-store, codex-plan, responses-pool-401-refresh, and codex-auth-api. Typecheck and privacy:scan clean. Dropping the alias carry from the unified path turns the plan-propagation test red, so the single path is genuinely covered. --- src/codex/account-store.ts | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 59e6bfd92b..ca8235ae4e 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -371,6 +371,12 @@ function withCredentialMutationLockSync(fn: () => T): T { type CodexTokenResult = { accessToken: string; chatgptAccountId: string; generation: number }; type CodexRefreshResult = CodexTokenResult & { credential?: CodexAccountCredentials; + /** + * Records that adopted this refresh's rotated credential through same-grant propagation, each + * with its own committed generation (#2892 gap 3). Carried on the result so the flight settles + * every plan in one place rather than the commit doing its own (#2933). + */ + propagatedAliases?: { id: string; generation: number }[]; /** * Grant the returned credential actually belongs to. * @@ -847,20 +853,16 @@ async function resolveCodexToken( } if (commit.propagatedAliases.length > 0) { console.warn(`[codex-auth] rotated refresh grant propagated to ${commit.propagatedAliases.length} dormant same-grant account record(s)`); - // The rotated JWT can carry a different `chatgpt_plan_type`, and each alias now holds that - // JWT. Without this the alias keeps its old plan — its cached-token fast path never - // reconciles, and repository-wide reconciliation only runs at startup — so quota scoring and - // the 30-day projection stay wrong until a restart or a WHAM refresh. Reconcile at each - // alias's OWN committed generation, since the plan note is generation-fenced. - for (const alias of commit.propagatedAliases) { - await notePlanFromRefreshedAccessToken(alias.id, updated.accessToken, alias.generation); - } } return { accessToken: updated.accessToken, chatgptAccountId: updated.chatgptAccountId, generation: startGeneration + 1, credential: updated, + // Aliases that adopted this rotated credential travel on the result so the FLIGHT settles + // their plans in the same single place as the owner's (#2933). Each carries its own committed + // generation because the plan note is generation-fenced. + ...(commit.propagatedAliases.length > 0 ? { propagatedAliases: commit.propagatedAliases } : {}), // The grant this flight was OPENED for, not the rotated one it produced. Joiners // are waiting on that key, and a successful refresh normally rotates the refresh // token — tagging the new grant would make every legitimate joiner look foreign. @@ -882,6 +884,12 @@ async function resolveCodexToken( */ const refreshPromise = fetchPromise.then(async (result): Promise => { await notePlanFromRefreshedAccessToken(id, result.accessToken, result.generation); + // One settlement path for the whole flight: the refreshing account, then any dormant alias that + // adopted the same rotated JWT. An alias holds the identical access token, so a changed + // `chatgpt_plan_type` applies to it too, and its cached-token fast path would never reconcile it. + for (const alias of result.propagatedAliases ?? []) { + await notePlanFromRefreshedAccessToken(alias.id, result.accessToken, alias.generation); + } return result; }).finally(() => { if (refreshLocks.get(refreshGrantFingerprint) === flight) refreshLocks.delete(refreshGrantFingerprint); From 5a37957691e7dc9025f1456f862ac59627fbf44c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 04:00:09 +0900 Subject: [PATCH 5/6] fix(codex): fail closed on absent identity and stop testing past the guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round on 7d9544924. All three blockers were correct. Empty account ids are not an identity. `""` equals `""`, but that proves nothing about which upstream account either record was meant to use, and a matching bearer snapshot only shows the two records copied the same token once. Propagation now requires the owner identity to be non-empty and each alias identity to be non-empty before exact equality, and leaves an unidentified dormant record untouched. The sidecar test was passing for the wrong reason. It hardcoded `writerGeneration: 0`, which sits below whatever reconciliation state earlier tests advanced to, so `recordCodexUpstreamOutcome` could reject the outcome at its writer-generation guard before reaching the credential-generation logic under test. It now captures the current generation the way a production pool auth context does, and asserts the quarantine actually applied before asserting it is later released — so the test cannot silently stop exercising its own subject. The gap-4 comment described the rollback design that no longer exists. It claimed health has no generation field, reauth is a bare id set, and mutations are re-validated and restored afterwards; the shipped code does the opposite. Rewritten to describe tagging and read-time judgement, including why no re-read can close the race and why `preservedCooldownFields` must drop the tag. 496 pass / 0 fail across the seven-file set run AS A SET, not filtered: codex-routing, codex-account-store, codex-plan, responses-pool-401-refresh, codex-auth-api, vision-sidecar-e2e, web-search. Typecheck and privacy:scan clean. Accepting empty ids turns the new regression red. Noting one difference in evidence: the order-dependent failure reported at codex-routing.test.ts:583 did not reproduce here on Bun 1.4.0, either filtered or as a set. The fragility was real regardless — a stale hardcoded writer generation can short-circuit the guard — so the fixture is fixed rather than left resting on a runtime difference. --- src/codex/account-store.ts | 10 +++++++++- src/codex/routing.ts | 24 +++++++++++----------- tests/codex-account-store.test.ts | 33 +++++++++++++++++++++++++++++++ tests/codex-routing.test.ts | 9 ++++++++- 4 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index ca8235ae4e..011ac0692e 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -277,12 +277,20 @@ export function commitRefreshedCodexCredentialWithAliases( // reconciliation below is generation-fenced, so an id alone would be reconciled at the wrong fence. const propagatedAliases: { id: string; generation: number }[] = []; // Nothing to propagate when the grant did not actually rotate: the aliases already hold it. - if (priorFingerprint !== undefined && priorCredential.refreshToken !== cred.refreshToken) { + // An absent owner identity fails closed: two empty strings compare equal but prove nothing about + // which upstream account either record was meant to use, and a matching bearer snapshot only + // shows they copied the same token once. Leave those dormant records alone. + if ( + priorFingerprint !== undefined + && priorCredential.refreshToken !== cred.refreshToken + && !!priorCredential.chatgptAccountId + ) { for (const [aliasId, alias] of Object.entries(store)) { if (aliasId === id || alias.deletedAt != null || !alias.credential) continue; if (recordGrantFingerprint(alias) !== priorFingerprint) continue; if (alias.credential.accessToken !== priorCredential.accessToken) continue; if (alias.credential.expiresAt !== priorCredential.expiresAt) continue; + if (!alias.credential.chatgptAccountId) continue; if (alias.credential.chatgptAccountId !== priorCredential.chatgptAccountId) continue; const aliasGeneration = alias.generation + 1; store[aliasId] = { diff --git a/src/codex/routing.ts b/src/codex/routing.ts index 483d57c2f2..51ea7179f2 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -2250,19 +2250,21 @@ export function recordCodexUpstreamOutcome( * The pre-check above closes the same-process race, but not a cross-process one (#2892 gap 4). * `isCodexAccountGenerationLive` is an unlocked read while credential writers coordinate under * the mutation lock, and OS preemption needs no `await` — so another process can replace the - * credential between the check and these writes. Health and reauth carry no credential identity - * of their own (a health entry has no generation field, reauth is a bare id set), so a stale 401 - * that lands in that window would quarantine the REPLACEMENT credential. + * credential after this check, or at any point after this whole function returns. No re-read here + * can close that: a replacement is always free to land one instruction later. * - * Taking the credential lock here is not an option: it runs with `busy_timeout=0`, so acquiring - * it per outcome would turn ordinary contention into thrown request-path errors. Instead the - * mutations stay synchronous and are re-validated afterwards, restoring the previous health and - * reauth state if the generation stopped being live. The window still exists; what changes is - * that its effects do not survive it. + * Taking the credential lock is not an option either: it runs with `busy_timeout=0`, so acquiring + * it per outcome would turn ordinary contention into thrown request-path errors. * - * Affinity sweeping is deliberately NOT rolled back: an affinity entry already carries a - * credential generation and self-invalidates on the next check, and re-adding swept entries - * would be a worse bug than the sweep. + * So the evidence is TAGGED with the credential it describes and judged when it is READ. The + * health entry carries `credentialFailureGeneration` and the reauth map carries the same + * generation; `dropSpentCredentialFailure` and `isAccountNeedsReauth` discard an entry whose + * credential is gone. A later transient or quota write replaces the entry along with its tag, and + * `preservedCooldownFields` drops the tag explicitly, so this provenance can never be spent + * against a failure it did not describe. + * + * Affinity sweeping needs no tag: an affinity entry already carries a credential generation and + * self-invalidates on the next check, and re-adding swept entries would be a worse bug. */ upstreamHealth.set(accountId, { consecutiveFailures: 1, diff --git a/tests/codex-account-store.test.ts b/tests/codex-account-store.test.ts index 99978bd4a0..fe7274496d 100644 --- a/tests/codex-account-store.test.ts +++ b/tests/codex-account-store.test.ts @@ -1229,4 +1229,37 @@ describe("shared refresh flight plan reconciliation (#2892 gap 2 follow-up)", () resetJwtPlanNotesForTests(); } }); + + test("records with EMPTY account ids are never treated as the same identity (#2892 review)", async () => { + const { getValidCodexToken, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../src/codex/account-store"); + // Grant fingerprint, access token and expiry all match, and both ids are "". Two empty strings + // compare equal but prove nothing about which upstream account either record was meant to use, + // so propagation must fail closed rather than write a credential into an unidentified record. + const shared = { accessToken: "anon-old", refreshToken: "anon-grant", expiresAt: 0, chatgptAccountId: "" }; + saveCodexAccountCredential("anon-owner", { ...shared }); + saveCodexAccountCredential("anon-alias", { ...shared }); + const aliasGeneration = readCodexAccountRecord("anon-alias")!.generation; + + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => Response.json({ + access_token: "anon-new", + refresh_token: "anon-rotated", + expires_in: 3600, + })) as typeof fetch; + try { + await getValidCodexToken("anon-owner"); + + // The owner still rotates normally. + expect(readCodexAccountRecord("anon-owner")!.credential?.refreshToken).toBe("anon-rotated"); + // The unidentified record is untouched, generation included. + const alias = readCodexAccountRecord("anon-alias")!; + expect(alias.credential?.accessToken).toBe("anon-old"); + expect(alias.credential?.refreshToken).toBe("anon-grant"); + expect(alias.generation).toBe(aliasGeneration); + } finally { + globalThis.fetch = originalFetch; + } + }); + }); diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index 277b00bbd1..b8df8b4a98 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -34,6 +34,7 @@ import { tryAcquireCodexQuotaProbeLease, } from "../src/codex/routing"; import { clearPoolRotationState } from "../src/codex/pool-rotation"; +import { captureConfigGeneration } from "../src/lib/state-store-sweeper"; import { readCodexAccountRecord, removeCodexAccountCredential, saveCodexAccountCredential } from "../src/codex/account-store"; import { clearAccountNeedsReauth, @@ -573,13 +574,19 @@ describe("codex routing", () => { const record = sidecarOutcomeRecorder(config, { kind: "pool", accountId: "a", - writerGeneration: 0, + // Use the CURRENT captured generation, as a production pool auth context does. A hardcoded 0 + // is below whatever reconciliation state earlier tests advanced to, so + // recordCodexUpstreamOutcome could reject the outcome at its writer-generation guard and the + // assertion would pass without ever reaching the credential-generation logic under test. + writerGeneration: captureConfigGeneration(), generation, accessToken: "access-a", chatgptAccountId: "acct-a", }); expect(record).toBeDefined(); record!(401); + // Guard the guard: if this is false the outcome never applied, so the assertions below would be + // vacuous rather than proving the replacement is not quarantined. expect(isAccountNeedsReauth("a")).toBe(true); saveTestCredential("a"); From bd26bcd393d07f6a25f40b3a104c54a5f646cc82 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 04:14:48 +0900 Subject: [PATCH 6/6] fix(codex): spend a stale credential failure before any branch reads health MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit found a third ownership path on the current head and it reproduces: after a G1 401, a G2 replacement, and a genuine G2 503, consecutiveFailures was 2 instead of 1. Reader-side spending was not sufficient. The transient and workspace branches derive their new entry from the current one, so a spent G1 401 donated its failure count to G2's first real failure and dropped the provenance tag while writing — after which no read could detect the inheritance. The account then reached the failover threshold one failure early. recordCodexUpstreamOutcome now spends a stale credential failure once, before any branch inspects health, so transient, workspace and quota all start from evidence that still describes a live credential. The regression covers both inheritance paths, transient and workspace denial. Removing the entry-point spend turns it red with the exact 2-versus-1 count. Worth recording: my first attempt to mutation-prove this patched the wrong call site — the reader inside getCodexUpstreamHealth rather than the entry point — and the test stayed green, which would have looked like vacuous coverage. Mutating the intended site fails it correctly. 497 pass / 0 fail across the seven-file set as a set. Typecheck and privacy:scan clean. --- src/codex/routing.ts | 10 ++++++++++ tests/codex-routing.test.ts | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/codex/routing.ts b/src/codex/routing.ts index 51ea7179f2..250aac9636 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -2144,6 +2144,16 @@ export function recordCodexUpstreamOutcome( const now = meta.now ?? Date.now(); const outcomeClass = classifyCodexUpstreamOutcome(outcome, meta.denial); const quotaScope = codexQuotaScopeForModel(meta.modelId); + /* + * Spend a stale credential failure BEFORE any branch reads health (#2892 gap 4 review). + * + * Reader-side spending alone is not enough: the transient and workspace branches derive their new + * entry from the current one, so a spent G1 401 would donate its `consecutiveFailures` to G2's + * first genuine 503 and drop the tag while doing it. The account then reaches the failover + * threshold one failure early, and no later read can tell. Clearing it here means every branch + * starts from evidence that still describes a live credential. + */ + dropSpentCredentialFailure(accountId); if (outcomeClass === "success") { const scopedProbe = meta.probeQuotaScope ? scopedHealthFor(accountId, meta.probeQuotaScope) diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index b8df8b4a98..1ec9f8cfda 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -595,6 +595,28 @@ describe("codex routing", () => { }); + test("a spent credential failure does not donate its failure count to a later transient (#2892 gap 4 review)", () => { + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + saveTestCredential("a"); + const generation = readCodexAccountRecord("a")!.generation; + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: generation }); + saveTestCredential("a"); + // G2's first genuine transient must start the count at 1. Inheriting the spent 401's count + // pushes the account over the failover threshold a failure early, and because the transient + // write drops the provenance tag, no later read can detect that it happened. + recordCodexUpstreamOutcome(config, "a", 503); + expect(getCodexUpstreamHealth("a")).toMatchObject({ consecutiveFailures: 1, lastFailureStatus: 503 }); + // The same inheritance path exists for a workspace denial. + clearCodexUpstreamHealthForAccount("a"); + recordCodexUpstreamOutcome(config, "a", 401, { credentialGeneration: readCodexAccountRecord("a")!.generation }); + saveTestCredential("a"); + recordCodexUpstreamOutcome(config, "a", 403, { denial: "workspace" }); + expect(getCodexUpstreamHealth("a")).toMatchObject({ consecutiveFailures: 1, lastFailureStatus: 403 }); + }); + + test("connect failures contribute to transient failover", () => { const config = makeConfig(); updateAccountQuota("a", 10);