From c344464d9a4e557c94edde096e57a7ea0c966679 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sun, 20 Sep 2026 04:11:25 +0900 Subject: [PATCH 1/2] fix(codex): keep transient pool refresh failures transient and surface stored auth causes --- src/codex/auth-api/account-list.ts | 6 +++--- src/codex/auth-api/pool-quota-probe.ts | 10 ++++++++-- src/oauth/health.ts | 12 +++++++++++- tests/oauth/oauth-health.test.ts | 19 +++++++++++++++++++ 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/codex/auth-api/account-list.ts b/src/codex/auth-api/account-list.ts index 23563a01a73..3ffd01dddda 100644 --- a/src/codex/auth-api/account-list.ts +++ b/src/codex/auth-api/account-list.ts @@ -82,9 +82,9 @@ export function mainQuotaWithCarriedResetCredits( /** * Why an account needs the operator. `missing_credential`, `refresh_failed`, and * `quota_unauthorized` are the three causes this surface tells apart on its own. `unauthorized` - * and `forbidden` exist because the shared health projection may return them; today - * `projectCodexAccountHealth` only ever produces `refresh_failed`, so accepting the full union - * keeps this field correct if that projection widens rather than silently dropping a reason. + * and `forbidden` come from the shared health projection: `projectCodexAccountHealth` maps a + * stored verification failure's `http_status:401`/`http_status:403` to them, so the union has + * to accept every reason the projection can emit rather than silently dropping one. */ export type CodexAccountReauthReason = | "missing_credential" diff --git a/src/codex/auth-api/pool-quota-probe.ts b/src/codex/auth-api/pool-quota-probe.ts index 4b27ad875a4..496142ecff7 100644 --- a/src/codex/auth-api/pool-quota-probe.ts +++ b/src/codex/auth-api/pool-quota-probe.ts @@ -1,4 +1,4 @@ -import { capturePoolQuotaWriter, getValidCodexToken, isCodexAccountGenerationLive, forceRefreshCodexPoolToken, markCodexAccountValidated, markCodexAccountValidationFailed, readCodexAccountRecord, CodexCredentialGenerationConflictError, CodexCredentialRefreshLockTimeoutError, CodexCredentialRefreshBusyError, CodexCredentialRefreshStaleError, TokenRefreshError } from "../account-store"; +import { capturePoolQuotaWriter, getValidCodexToken, isCodexAccountGenerationLive, forceRefreshCodexPoolToken, markCodexAccountValidated, markCodexAccountValidationFailed, readCodexAccountRecord, isTerminalCodexPoolRefreshFailure, CodexCredentialGenerationConflictError, CodexCredentialRefreshLockTimeoutError, CodexCredentialRefreshBusyError, CodexCredentialRefreshStaleError, TokenRefreshError } from "../account-store"; import type { PoolQuotaWriter } from "../quota-types"; import { isValidWhamHistoryObservation, getAccountQuota, isCompleteCodexQuotaRecoverySnapshot, parseUsageQuota, setAccountQuotaFromParsed } from "../quota"; import type { StoredAccountQuota, WhamUsageResponse } from "../quota"; @@ -401,7 +401,13 @@ export async function fetchFreshPoolAccountQuota( quotaProbeSkipped: true, }, quotaProbeEvidence); } - if (e instanceof TokenRefreshError) { + // Terminal means the grant itself is dead or missing; an `unknown` refresh failure (a + // token-endpoint 5xx, a transport blip) may clear, so it reports transient like the + // 401-recovery path instead of quarantining a healthy account (#2887). The dead grant is + // marked so the next listing still names the cause rather than flipping back to healthy + // once this response's quota snapshot is cached. + if (isTerminalCodexPoolRefreshFailure(e)) { + markAccountNeedsReauth(accountId, captureConfigGeneration(), requestCredentialGeneration); return withQuotaProbeEvidence( { quota: existing ?? null, needsReauth: true, reauthReason: "refresh_failed", credentialGeneration: requestCredentialGeneration }, quotaProbeEvidence, diff --git a/src/oauth/health.ts b/src/oauth/health.ts index 3379f700aeb..7f596f13464 100644 --- a/src/oauth/health.ts +++ b/src/oauth/health.ts @@ -231,6 +231,16 @@ export function projectCodexAccountHealth(input: { const needsReauth = input.needsReauth || validationAuthFailed || terminalGrantFailure; + // A stored verification failure carries its own cause. When the caller did not name a + // reason, surface the persisted one instead of flattening every verdict into + // `refresh_failed`. + const storedVerificationFailed = validationAuthFailed || terminalGrantFailure; + const storedFailureReason: "unauthorized" | "forbidden" | undefined = !storedVerificationFailed + ? undefined + : record?.lastCodexValidationError === "http_status:403" ? "forbidden" + : record?.lastCodexValidationError === "http_status:401" ? "unauthorized" + : undefined; + // Deferred validation is only worth reporting while the credential itself is still viable. A // revoked grant needs a re-login, not a "Refresh quotas" click, so reauth is resolved first. if (!needsReauth && record?.codexValidationPending) { @@ -240,7 +250,7 @@ export function projectCodexAccountHealth(input: { const snap = getCodexAccountHealthSnapshot(input.accountId, now); return projectOAuthAccountHealth({ needsReauth, - reauthReason: needsReauth ? (input.reauthReason ?? "refresh_failed") : undefined, + reauthReason: needsReauth ? (input.reauthReason ?? storedFailureReason ?? "refresh_failed") : undefined, cooldownUntilMs: snap?.cooldownUntil, cooldownReason: cooldownReasonFromSource(snap?.cooldownSource), now, diff --git a/tests/oauth/oauth-health.test.ts b/tests/oauth/oauth-health.test.ts index 8b3aa068b6d..2f42bac60b8 100644 --- a/tests/oauth/oauth-health.test.ts +++ b/tests/oauth/oauth-health.test.ts @@ -166,6 +166,25 @@ describe("projectCodexAccountHealth", () => { .toEqual({ status: "healthy" }); }); + test.each([ + ["http_status:401", "unauthorized"], + ["http_status:403", "forbidden"], + ] as const)("a stored verification failure surfaces %s as %s", async (validationError, reason) => { + withPoolStoreDir(); + const { markCodexAccountValidationFailed, readCodexAccountRecord, saveCodexAccountCredential } = + await import("../../src/codex/account-store"); + const accountId = "pool-stored-" + reason; + saveCodexAccountCredential(accountId, { + accessToken: "a", refreshToken: "r", expiresAt: Date.now() + 3_600_000, chatgptAccountId: "cg", + }, { validationPending: true }); + markCodexAccountValidationFailed(accountId, validationError, { + expectedGeneration: readCodexAccountRecord(accountId)!.generation, + }); + + expect(projectCodexAccountHealth({ accountId, needsReauth: false })) + .toEqual({ status: "reauth_required", reason }); + }); + test("the CLI collector reports the terminal verdict too", async () => { withPoolStoreDir(); const { markCodexAccountValidationFailed, readCodexAccountRecord, saveCodexAccountCredential } = From 4d4bfd78634506f1972b5cbdb1b287956465e989 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sun, 20 Sep 2026 07:01:30 +0900 Subject: [PATCH 2/2] fix(codex): persist terminal pool reauth verdicts and let stored causes outrank bare marks A memory-only reauth mark carries no cause of its own, so poolAccountDto no longer names refresh_failed on its behalf: projectCodexAccountHealth then falls back to the persisted verdict, letting a stored http_status:401/403 surface as unauthorized/forbidden while the mark is still present. Terminal refresh failures found during quota probes (revoked/expired grants) are now persisted through markCodexAccountValidationFailed with terminal: true, the same verdict the token guardian writes - the in-memory mark dies with the process, and only the stored verdict keeps a cached listing from calling the dead grant healthy after a restart. Applied in both recoverPoolQuotaFrom401 and fetchFreshPoolAccountQuota. Tests: the deferred-validation matrix now expects the stored http status cause whether or not the in-memory mark survives; the pool-reauth-cause helper gains persisted-verdict and restart-simulation assertions for the dead-grant path. --- src/codex/auth-api/account-list.ts | 12 +-- src/codex/auth-api/pool-quota-probe.ts | 21 ++++- .../codex-integration/codex-auth-api.test.ts | 7 +- tests/helpers/pool-reauth-cause.ts | 87 ++++++++++++++++++- 4 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/codex/auth-api/account-list.ts b/src/codex/auth-api/account-list.ts index 3ffd01dddda..b19e487264a 100644 --- a/src/codex/auth-api/account-list.ts +++ b/src/codex/auth-api/account-list.ts @@ -107,15 +107,15 @@ export function poolAccountDto( const runtimeReauth = isAccountNeedsReauth(account.id); const rawReauthReason: CodexAccountReauthReason | undefined = !hasCredential ? "missing_credential" - : quotaResult.reauthReason - ? quotaResult.reauthReason - : runtimeReauth - ? "refresh_failed" - : undefined; + : quotaResult.reauthReason; const needsReauth = !hasCredential || quotaResult.needsReauth || runtimeReauth; + // An in-memory reauth mark carries no cause of its own, so it must not name one: passing + // a caller reason here would outrank the stored verdict's http_status inside the + // projection and hide unauthorized/forbidden until the mark is gone. With no caller + // reason the projection falls back to the persisted cause, then to refresh_failed. const healthReason = rawReauthReason === "quota_unauthorized" || rawReauthReason === "missing_credential" ? "unauthorized" - : "refresh_failed"; + : rawReauthReason; const health = projectCodexAccountHealth({ accountId: account.id, needsReauth, diff --git a/src/codex/auth-api/pool-quota-probe.ts b/src/codex/auth-api/pool-quota-probe.ts index 496142ecff7..538f6023240 100644 --- a/src/codex/auth-api/pool-quota-probe.ts +++ b/src/codex/auth-api/pool-quota-probe.ts @@ -227,6 +227,13 @@ export async function recoverPoolQuotaFrom401(ctx: { // A refresh that failed terminally is the one case where the credential really is gone. // Everything else is unknown, and unknown is not proof. if (e instanceof TokenRefreshError && isTerminalRefreshError(e)) { + // Persist the same verdict the token guardian writes: the in-memory mark dies with + // this process, and only the stored terminal failure keeps a cached listing from + // calling the dead grant healthy after a restart. + markCodexAccountValidationFailed(accountId, `refresh_${e.reason}`, { + expectedGeneration: rejectedGeneration, + terminal: true, + }); markAccountNeedsReauth(accountId, captureConfigGeneration(), rejectedGeneration); return { quota: existing ?? null, needsReauth: true, reauthReason: "refresh_failed", credentialGeneration: rejectedGeneration }; } @@ -403,10 +410,18 @@ export async function fetchFreshPoolAccountQuota( } // Terminal means the grant itself is dead or missing; an `unknown` refresh failure (a // token-endpoint 5xx, a transport blip) may clear, so it reports transient like the - // 401-recovery path instead of quarantining a healthy account (#2887). The dead grant is - // marked so the next listing still names the cause rather than flipping back to healthy - // once this response's quota snapshot is cached. + // 401-recovery path instead of quarantining a healthy account (#2887). A revoked or + // expired grant is also written to the record as a terminal validation failure, the same + // verdict the token guardian persists: the in-memory mark dies with this process, and + // the stored verdict is what keeps a cached listing from calling the dead grant healthy + // after a restart. if (isTerminalCodexPoolRefreshFailure(e)) { + if (e instanceof TokenRefreshError && isTerminalRefreshError(e)) { + markCodexAccountValidationFailed(accountId, `refresh_${e.reason}`, { + expectedGeneration: requestCredentialGeneration, + terminal: true, + }); + } markAccountNeedsReauth(accountId, captureConfigGeneration(), requestCredentialGeneration); return withQuotaProbeEvidence( { quota: existing ?? null, needsReauth: true, reauthReason: "refresh_failed", credentialGeneration: requestCredentialGeneration }, diff --git a/tests/codex-integration/codex-auth-api.test.ts b/tests/codex-integration/codex-auth-api.test.ts index 663eab92359..ab750aa06d0 100644 --- a/tests/codex-integration/codex-auth-api.test.ts +++ b/tests/codex-integration/codex-auth-api.test.ts @@ -5340,14 +5340,17 @@ describe("codex-auth API", () => { if (restart) clearAccountNeedsReauth(accountId); const rows = await listCodexAuthAccounts(config, false); const authFailed = !replace && (status === 401 || status === 403); + // The stored verdict's own http status names the cause whether or not the in-memory + // mark still exists; a bare mark no longer flattens the projection to refresh_failed. + const expectedReason = status === 403 ? "forbidden" : "unauthorized"; const row = rows.find(entry => entry.id === accountId); expect(row).toMatchObject({ needsReauth: authFailed, - health: { status: authFailed ? "reauth_required" : "warning", reason: authFailed ? "refresh_failed" : "validation_pending" }, + health: { status: authFailed ? "reauth_required" : "warning", reason: authFailed ? expectedReason : "validation_pending" }, }); // The reason travels with the state, so an operator reading the account surface can tell a // failed refresh from a pending validation without inferring it from `health` (#4212). - if (authFailed) expect(row).toMatchObject({ reauthReason: "refresh_failed" }); + if (authFailed) expect(row).toMatchObject({ reauthReason: expectedReason }); else expect(row).not.toHaveProperty("reauthReason"); fail = false; await refresh(); diff --git a/tests/helpers/pool-reauth-cause.ts b/tests/helpers/pool-reauth-cause.ts index f34fbc8eb6c..a1e8c05bc34 100644 --- a/tests/helpers/pool-reauth-cause.ts +++ b/tests/helpers/pool-reauth-cause.ts @@ -1,5 +1,11 @@ import { expect, test } from "bun:test"; -import { handleCodexAuthAPI } from "../../src/codex/auth-api"; +import { + clearAccountNeedsReauth, + handleCodexAuthAPI, + setAccountQuotaFromParsed, +} from "../../src/codex/auth-api"; +import { readCodexAccountRecord } from "../../src/codex/account-store"; +import { captureConfigGeneration } from "../../src/lib/state-store-sweeper"; import type { OcxConfig } from "../../src/types"; /** @@ -27,10 +33,15 @@ export function registerPoolReauthCauseCases( const req = new Request("http://localhost/api/codex-auth/accounts?refresh=1"); const resp = await handleCodexAuthAPI(req, new URL(req.url), config); - const data = await resp!.json() as { accounts: Array<{ id: string; reauthReason?: string }> }; + const data = await resp!.json() as { + accounts: Array<{ id: string; reauthReason?: string; health?: { status: string; reason?: string } }>; + }; expect(data.accounts.find(account => account.id === "pool-quota-rejected")) - .toMatchObject({ reauthReason: "quota_unauthorized" }); + .toMatchObject({ + reauthReason: "quota_unauthorized", + health: { status: "reauth_required", reason: "unauthorized" }, + }); }); test("pool token refresh rejection reports refresh failure as the reauthentication cause", async () => { @@ -40,6 +51,7 @@ export function registerPoolReauthCauseCases( email: "pool-refresh-rejected@example.com", expiresAt: Date.now() - 1, }); + setAccountQuotaFromParsed("pool-refresh-rejected", { weeklyPercent: 12 }, captureConfigGeneration()); const urls: string[] = []; globalThis.fetch = (async input => { urls.push(String(input)); @@ -48,10 +60,77 @@ export function registerPoolReauthCauseCases( const req = new Request("http://localhost/api/codex-auth/accounts/refresh", { method: "POST" }); const resp = await handleCodexAuthAPI(req, new URL(req.url), config); - const data = await resp!.json() as { accounts: Array<{ id: string; reauthReason?: string }> }; + const data = await resp!.json() as { + accounts: Array<{ id: string; needsReauth?: boolean; reauthReason?: string }>; + }; expect(urls).toEqual(["https://auth.openai.com/oauth/token"]); expect(data.accounts.find(account => account.id === "pool-refresh-rejected")) .toMatchObject({ reauthReason: "refresh_failed" }); + + // A dead grant stays dead: the cached-quota listing performs no refresh at all, so only + // the recorded marks keep the cause visible instead of flipping back to healthy. + const cachedReq = new Request("http://localhost/api/codex-auth/accounts"); + const cachedResp = await handleCodexAuthAPI(cachedReq, new URL(cachedReq.url), config); + const cachedData = await cachedResp!.json() as { + accounts: Array<{ id: string; needsReauth?: boolean; reauthReason?: string }>; + }; + expect(urls).toEqual(["https://auth.openai.com/oauth/token"]); + expect(cachedData.accounts.find(account => account.id === "pool-refresh-rejected")) + .toMatchObject({ needsReauth: true, reauthReason: "refresh_failed" }); + + // The terminal verdict is also persisted like the token guardian's, so losing the + // in-memory mark on restart cannot flip the dead grant back to healthy. + expect(readCodexAccountRecord("pool-refresh-rejected")).toMatchObject({ + lastCodexValidationStatus: "failed", + lastCodexValidationError: "refresh_revoked", + lastCodexValidationTerminal: true, + }); + clearAccountNeedsReauth("pool-refresh-rejected"); + const restartedReq = new Request("http://localhost/api/codex-auth/accounts"); + const restartedResp = await handleCodexAuthAPI(restartedReq, new URL(restartedReq.url), config); + const restartedData = await restartedResp!.json() as { + accounts: Array<{ + id: string; + needsReauth?: boolean; + reauthReason?: string; + health?: { status: string; reason?: string }; + }>; + }; + expect(urls).toEqual(["https://auth.openai.com/oauth/token"]); + expect(restartedData.accounts.find(account => account.id === "pool-refresh-rejected")) + .toMatchObject({ + needsReauth: true, + reauthReason: "refresh_failed", + health: { status: "reauth_required", reason: "refresh_failed" }, + }); + }); + + test("a transient pool token refresh failure does not raise reauthentication", async () => { + const config = makeConfig(); + seedPoolAccount(config, { + id: "pool-refresh-transient", + email: "pool-refresh-transient@example.com", + expiresAt: Date.now() - 1, + }); + // A token-endpoint 5xx classifies as `unknown`, which the account store treats as + // transient: the credential may still be fine, so no reauth cause may surface (#2887). + globalThis.fetch = (async () => Response.json({ error: "server_error" }, { status: 500 })) as typeof fetch; + + const req = new Request("http://localhost/api/codex-auth/accounts/refresh", { method: "POST" }); + const resp = await handleCodexAuthAPI(req, new URL(req.url), config); + const data = await resp!.json() as { + accounts: Array<{ + id: string; + needsReauth?: boolean; + reauthReason?: string; + health?: { status: string; reason?: string }; + }>; + }; + + const account = data.accounts.find(row => row.id === "pool-refresh-transient"); + expect(account?.needsReauth).toBe(false); + expect(account?.reauthReason).toBeUndefined(); + expect(account?.health?.status).not.toBe("reauth_required"); }); }