From a8eff6e139b5d3f62589bcfa631033f7da110862 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 19 Sep 2026 23:59:03 +0900 Subject: [PATCH 1/4] fix(codex): preserve pool reauthentication failure causes PoolQuotaResult now carries the failure source observed while obtaining WHAM/quota or refreshing tokens, so poolAccountDto reports the actual cause instead of inferring it from overlapping booleans. Terminal WHAM 401s surface as quota_unauthorized and terminal refresh failures as refresh_failed. Carried from #5191. The two regression cases are registered from tests/helpers/pool-reauth-cause.ts rather than written inline: the original placement grew tests/codex-integration/codex-auth-api.test.ts from 6522 to 6560 lines against a 6549-line file-size-baseline cap that only ever moves downward. Registering them through the same registerXCases seam the file already uses for two other helpers leaves it at 6525 with the assertions byte-identical. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --- src/codex/auth-api/account-list.ts | 24 +++++--- src/codex/auth-api/pool-quota-probe.ts | 12 ++-- src/oauth/health.ts | 3 +- .../codex-integration/codex-auth-api.test.ts | 3 + tests/helpers/pool-reauth-cause.ts | 57 +++++++++++++++++++ 5 files changed, 85 insertions(+), 14 deletions(-) create mode 100644 tests/helpers/pool-reauth-cause.ts diff --git a/src/codex/auth-api/account-list.ts b/src/codex/auth-api/account-list.ts index faa8eeefc08..23563a01a73 100644 --- a/src/codex/auth-api/account-list.ts +++ b/src/codex/auth-api/account-list.ts @@ -105,18 +105,26 @@ export function poolAccountDto( const plan = codexPlanValue(account.plan); const quota = quotaForPlan(quotaResult.quota, plan); const runtimeReauth = isAccountNeedsReauth(account.id); + const rawReauthReason: CodexAccountReauthReason | undefined = !hasCredential + ? "missing_credential" + : quotaResult.reauthReason + ? quotaResult.reauthReason + : runtimeReauth + ? "refresh_failed" + : undefined; const needsReauth = !hasCredential || quotaResult.needsReauth || runtimeReauth; - const health = projectCodexAccountHealth({ accountId: account.id, needsReauth }); + const healthReason = rawReauthReason === "quota_unauthorized" || rawReauthReason === "missing_credential" + ? "unauthorized" + : "refresh_failed"; + const health = projectCodexAccountHealth({ + accountId: account.id, + needsReauth, + reauthReason: needsReauth ? healthReason : undefined, + }); // `needsReauth` is an OR of three independent causes plus a persisted verdict resolved inside the // health projection. Emitting only the boolean is what left #4212's reporter guessing which // account took their model away and why, so name the cause they actually have to act on. - const reauthReason: CodexAccountReauthReason | undefined = !hasCredential - ? "missing_credential" - : runtimeReauth - ? "refresh_failed" - : quotaResult.needsReauth - ? "quota_unauthorized" - : health.status === "reauth_required" ? health.reason : undefined; + const reauthReason: CodexAccountReauthReason | undefined = rawReauthReason ?? (health.status === "reauth_required" ? health.reason : undefined); return { id: account.id, email: projectEmail(account.email, maskEmails) ?? account.email, diff --git a/src/codex/auth-api/pool-quota-probe.ts b/src/codex/auth-api/pool-quota-probe.ts index 84f8519af1e..4b27ad875a4 100644 --- a/src/codex/auth-api/pool-quota-probe.ts +++ b/src/codex/auth-api/pool-quota-probe.ts @@ -46,6 +46,8 @@ export interface PoolQuotaResult { resetRefreshLineage?: ManualResetRefreshLineage; quota: StoredAccountQuota | null; needsReauth: boolean; + /** Failure source observed while obtaining the quota, when reauthentication is required. */ + reauthReason?: "refresh_failed" | "quota_unauthorized"; /** Credential generation whose cache or network result this DTO state belongs to. */ credentialGeneration?: number; /** Present only when this call freshly parsed a WHAM usage response. */ @@ -178,7 +180,7 @@ export async function recoverPoolQuotaFrom401(ctx: { // the credential it condemned, so a late terminal response arriving after the operator // re-authenticated would quarantine the replacement. markAccountNeedsReauth(accountId, captureConfigGeneration(), rejectedGeneration); - return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; + return { quota: existing ?? null, needsReauth: true, reauthReason: "quota_unauthorized", credentialGeneration: rejectedGeneration }; } const claim = claimQuotaRecovery(accountId, rejectedGeneration); @@ -186,7 +188,7 @@ export async function recoverPoolQuotaFrom401(ctx: { // A lineage fenced by a TERMINAL refresh failure stays terminal. Without this, the // budget being used would make the next bare 401 report a dead credential as healthy. if (quotaRecoveryTerminalFor(accountId, rejectedGeneration)) { - return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; + return { quota: existing ?? null, needsReauth: true, reauthReason: "refresh_failed", credentialGeneration: rejectedGeneration }; } // Otherwise: this lineage spent its attempt, another caller is mid-refresh, or a // transient failure is backing off. Report transient and let the next poll try — @@ -226,7 +228,7 @@ export async function recoverPoolQuotaFrom401(ctx: { // Everything else is unknown, and unknown is not proof. if (e instanceof TokenRefreshError && isTerminalRefreshError(e)) { markAccountNeedsReauth(accountId, captureConfigGeneration(), rejectedGeneration); - return { quota: existing ?? null, needsReauth: true, credentialGeneration: rejectedGeneration }; + return { quota: existing ?? null, needsReauth: true, reauthReason: "refresh_failed", credentialGeneration: rejectedGeneration }; } return { quota: existing ?? null, needsReauth: false, credentialGeneration: rejectedGeneration }; } @@ -258,7 +260,7 @@ export async function recoverPoolQuotaFrom401(ctx: { // let the next poll call a dead credential healthy. The evidence is about the // REFRESHED credential, which is what the replay used. markAccountNeedsReauth(accountId, writerGeneration, refreshed.generation); - return { quota: existing ?? null, needsReauth: true, credentialGeneration: refreshed.generation }; + return { quota: existing ?? null, needsReauth: true, reauthReason: "quota_unauthorized", credentialGeneration: refreshed.generation }; } return { quota: existing ?? null, needsReauth: false, credentialGeneration: refreshed.generation }; } @@ -401,7 +403,7 @@ export async function fetchFreshPoolAccountQuota( } if (e instanceof TokenRefreshError) { return withQuotaProbeEvidence( - { quota: existing ?? null, needsReauth: true, credentialGeneration: requestCredentialGeneration }, + { quota: existing ?? null, needsReauth: true, reauthReason: "refresh_failed", credentialGeneration: requestCredentialGeneration }, quotaProbeEvidence, ); } diff --git a/src/oauth/health.ts b/src/oauth/health.ts index 8c4332af850..3379f700aeb 100644 --- a/src/oauth/health.ts +++ b/src/oauth/health.ts @@ -202,6 +202,7 @@ export function projectStoredOAuthAccountHealth( export function projectCodexAccountHealth(input: { accountId: string; needsReauth: boolean; + reauthReason?: "unauthorized" | "forbidden" | "refresh_failed"; now?: number; }): OAuthAccountHealth { // One read serves every verdict below. Each lookup re-reads and re-hardens the whole store @@ -239,7 +240,7 @@ export function projectCodexAccountHealth(input: { const snap = getCodexAccountHealthSnapshot(input.accountId, now); return projectOAuthAccountHealth({ needsReauth, - reauthReason: needsReauth ? "refresh_failed" : undefined, + reauthReason: needsReauth ? (input.reauthReason ?? "refresh_failed") : undefined, cooldownUntilMs: snap?.cooldownUntil, cooldownReason: cooldownReasonFromSource(snap?.cooldownSource), now, diff --git a/tests/codex-integration/codex-auth-api.test.ts b/tests/codex-integration/codex-auth-api.test.ts index d02474985e4..663eab92359 100644 --- a/tests/codex-integration/codex-auth-api.test.ts +++ b/tests/codex-integration/codex-auth-api.test.ts @@ -1,5 +1,6 @@ import { registerWarmupRateLimitCases } from "../helpers/codex-warmup-rate-limit"; import { registerResetCreditConsumeValidationTests } from "../helpers/reset-credit-consume-validation"; +import { registerPoolReauthCauseCases } from "../helpers/pool-reauth-cause"; import * as usageHistoryModule from "../../src/usage/log"; import { getAccountQuotaHistory } from "../../src/codex/quota"; import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; @@ -2090,6 +2091,8 @@ describe("codex-auth API", () => { expect(existsSync(join(TEST_DIR, "config.json"))).toBe(false); }); + registerPoolReauthCauseCases(makeConfig, seedPoolAccount); + test("pool plan refresh batches multiple authoritative changes into one config save", async () => { const config = makeConfig(); seedPoolAccount(config, { id: "pool-plan-a", email: "pool-plan-a@example.com", plan: "plus" }); diff --git a/tests/helpers/pool-reauth-cause.ts b/tests/helpers/pool-reauth-cause.ts new file mode 100644 index 00000000000..f34fbc8eb6c --- /dev/null +++ b/tests/helpers/pool-reauth-cause.ts @@ -0,0 +1,57 @@ +import { expect, test } from "bun:test"; +import { handleCodexAuthAPI } from "../../src/codex/auth-api"; +import type { OcxConfig } from "../../src/types"; + +/** + * #4212 follow-up: `poolAccountDto` used to infer the reauthentication cause from overlapping + * booleans, so a WHAM 401 and a dead refresh grant both reported `refresh_failed`. These cases + * pin the observed cause through `handleCodexAuthAPI`, one per failure source. + * + * They live here rather than in `codex-auth-api.test.ts` because that file sits against its + * `file-size-baseline.json` cap, which only ever moves downward. + */ +export function registerPoolReauthCauseCases( + makeConfig: () => OcxConfig, + seedPoolAccount: ( + config: OcxConfig, + account: { id: string; email: string; expiresAt?: number }, + ) => void, +): void { + test("pool quota rejection reports quota authorization as the reauthentication cause", async () => { + const config = makeConfig(); + seedPoolAccount(config, { id: "pool-quota-rejected", email: "pool-quota-rejected@example.com" }); + globalThis.fetch = (async () => Response.json( + { detail: { code: "invalid_refresh_token" } }, + { status: 401 }, + )) as typeof fetch; + + 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 }> }; + + expect(data.accounts.find(account => account.id === "pool-quota-rejected")) + .toMatchObject({ reauthReason: "quota_unauthorized" }); + }); + + test("pool token refresh rejection reports refresh failure as the reauthentication cause", async () => { + const config = makeConfig(); + seedPoolAccount(config, { + id: "pool-refresh-rejected", + email: "pool-refresh-rejected@example.com", + expiresAt: Date.now() - 1, + }); + const urls: string[] = []; + globalThis.fetch = (async input => { + urls.push(String(input)); + return Response.json({ error: "invalid_grant" }, { status: 400 }); + }) 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; 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" }); + }); +} From c4c4ae950c87714c5aed44867de137e6e894e84d Mon Sep 17 00:00:00 2001 From: lidge-jun Date: Sun, 20 Sep 2026 02:43:52 +0900 Subject: [PATCH 2/4] fix(server): stop a local catalog-sync warning from un-readying the proxy /readyz went to a terminal failed state on any nonempty warning from the post-startup Codex sync. A warning there describes artifacts written into the LOCAL Codex home, not the proxy's ability to serve: no catalog source so Codex keeps its native catalog, combos omitted from the catalog, a conversation-history relabel left to Codex's own writer, or a caught catalog-refresh exception after which config injection still runs and still reports its own ok. The sync continues past every one of them. #5181 is what that cost. A single-replica Kubernetes deployment lost its only Service endpoint while every non-Codex route stayed healthy, because the readiness probe read a degradation of a local file as a dead process. Switching the probe to /healthz restored service, which is the reporter's own evidence that the process was viable. ok and warning are separate fields in the sync result because they answer separate questions. ok is the sync's verdict on the essential work - the write admission and the config injection. The gate now follows ok alone. The narrowing is only to warning. A throw, a null outcome, and ok !== true stay terminal, so the service-home write refusal, the external-provider injection failure, the config/integrity preflight refusal and the final injection failure all still fail readiness. A throw and a null stay terminal for a different reason: neither is a classified outcome, so the gate cannot claim one. Readiness also still stays pending until the sync SETTLES, which is the race the gate was built for and which this does not touch. This is the boundary the Claude Code roster reconciliation already has - it delays the ready transition without being allowed to fail it - applied to the catalog sync. Scope note: the brief for this work held that the Codex pool's "needs reauthentication" warning was itself the terminal warning. It is not. That line comes from warnGatedNativeSuppressedOnce (src/codex/catalog/gated-native-warn.ts) and is a bare console.warn that never enters the structured result, and per-account pool refresh failures are caught and returned as null by model-entitlements.ts rather than propagating. The account-level fault that does reach the gate is a native MAIN credential refresh failing outside that per-account catch: it rejects the entitlement gather, surfaces as the catch-all "catalog sync skipped" warning, and permanently un-readies the process. That is the same class of blast radius the report describes, and it is closed here. Closes #5181 --- src/codex/desired-state.ts | 5 ++- src/server/readiness.ts | 39 ++++++++++++++----- structure/catalog.md | 15 +++++-- .../codex-desired-state.test.ts | 28 +++++++++++++ tests/server/proxy-liveness.test.ts | 27 ++++++++++++- tests/server/server-live.test.ts | 5 ++- 6 files changed, 101 insertions(+), 18 deletions(-) diff --git a/src/codex/desired-state.ts b/src/codex/desired-state.ts index 8f34c65585a..a2cc3a96b7d 100644 --- a/src/codex/desired-state.ts +++ b/src/codex/desired-state.ts @@ -260,7 +260,10 @@ export async function syncCodexOnStartIfEnabled( // The `.catch` is deliberate and stays: a failure to APPLY must not stop the // proxy from coming up. A failed sync simply reports no writes. The readiness // gate observes the real outcome so /readyz reflects the sync state exactly as - // the PR contract defines (ready only on ok=true with no warning). + // the contract defines: ready on ok=true once the sync has settled. A nonempty + // `warning` is a local-Codex-artifact degradation the sync chose to continue + // past, and #5181 is what it cost to treat it as terminal — a healthy + // multi-provider proxy reported itself permanently unready. const outcome = readinessGate ? await runStartupReadinessSync(readinessGate, async () => (await sync(port)) ?? null) : await sync(port).catch(() => undefined); diff --git a/src/server/readiness.ts b/src/server/readiness.ts index 5dc3c2021f4..f48cfb5591a 100644 --- a/src/server/readiness.ts +++ b/src/server/readiness.ts @@ -3,10 +3,30 @@ * * `GET /healthz` answers "is the process alive and serving HTTP?" the instant the * listener binds. Readiness is stricter: the proxy is "ready" only after the - * post-startup Codex catalog/config sync (`syncModelsToCodex`) has settled with - * `ok=true` and no catalog-sync warning. Until then the process is live (Codex can - * open a socket) but not ready (a request would race the sync or hit a stale - * catalog), so clients should back off. + * post-startup Codex catalog/config sync (`syncModelsToCodex`) has SETTLED and + * reported `ok=true`. Until it settles the process is live (Codex can open a + * socket) but not ready — a request would race the sync — so clients back off. + * + * What readiness is NOT (#5181): it is not a verdict on the local Codex client's + * artifacts. A nonempty `warning` used to be terminal here, which made a + * degradation of files written into the local Codex home permanently un-ready a + * proxy that was serving every other provider correctly. In a single-replica + * Kubernetes deployment that removed the only Service endpoint. + * + * `ok` and `warning` are separate fields in the sync result because they answer + * separate questions, and the gate must not conflate them. `ok` is the sync's own + * verdict on whether the essential work — config injection, and the write + * admission that precedes it — succeeded. `warning` names a degradation the sync + * itself decided to continue past: no catalog source so Codex keeps its native + * catalog, combos omitted from the catalog, a conversation-history relabel left + * to Codex's own writer, or a caught catalog-refresh exception after which + * injection still runs and still reports its own `ok`. None of those stops this + * process from accepting HTTP or routing to a provider, so none of them may close + * the gate. + * + * That boundary is not new, only extended: the Claude Code roster reconciliation + * in `src/cli/claude-agent-startup-sync.ts` already delays the ready transition + * without being allowed to fail it, on the same reasoning. * * Design contract (per P1 review): * - NO module-global mutable state. Each `startServer` invocation gets its own @@ -65,8 +85,11 @@ export interface SyncOutcomeLike { /** * Drive the gate from the post-startup sync. Awaits `syncFn`; the gate goes to - * `ready` ONLY on `ok=true` with no nonempty warning. A throw, `null`, `ok=false`, - * or a nonempty warning transitions to `failed`. Used directly by `handleStart` + * `ready` on `ok=true`. A throw, `null`, or `ok !== true` transitions to + * `failed`; a nonempty `warning` does not, because the sync that produced it + * still reported the essential work done (see the file header, #5181). A throw + * and `null` stay terminal because neither is a classified outcome: the startup + * path did not reach a verdict, so the gate cannot claim one. Used by `handleStart` * so the startup transition is unit-testable without spawning the proxy. Returns * the raw sync outcome so a caller that also needs the #1046 write flags (did the * sync actually write the catalog/cache?) can keep them without a second call. @@ -90,10 +113,6 @@ export async function runStartupReadinessSync( gate.markFailed(); return result; } - if (result.warning !== undefined && result.warning !== "") { - gate.markFailed(); - return result; - } gate.markReady(); return result; } diff --git a/structure/catalog.md b/structure/catalog.md index 73d05b1662a..bcd3956dfd2 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -202,9 +202,18 @@ Each `startServer` invocation owns a private, one-shot readiness gate created be binds. `handleStart` supplies its gate and transitions it only after the shared catalog sync and best-effort Claude Code roster reconciliation have both settled. The catalog sync remains the authority for ready versus failed; a roster warning does not make an otherwise healthy proxy fail. -Calls without a supplied gate receive a fresh private gate that intentionally remains pending. Only -`ok: true` with no nonempty warning becomes ready; `null`, a throw, `ok !== true`, or a nonempty -warning becomes failed. State is isolated per server instance. +Calls without a supplied gate receive a fresh private gate that intentionally remains pending. +`ok: true` becomes ready; `null`, a throw, or `ok !== true` becomes failed. State is isolated per +server instance. + +A nonempty catalog-sync `warning` does not become failed. `ok` is the sync's verdict on the +essential work (write admission and config injection); `warning` names a degradation of artifacts +in the local Codex home that the sync deliberately continued past — no catalog source, omitted +combos, a conversation-history relabel left to Codex's own writer, or a caught catalog-refresh +exception after which injection still runs. None of those stops the process from serving HTTP or +routing to a provider. Treating them as terminal is what #5181 reported: a single-replica +Kubernetes deployment lost its only Service endpoint while every non-Codex route stayed healthy. +This is the same boundary the Claude roster reconciliation already has, applied to the catalog sync. Exact unauthenticated `GET /readyz` returns sanitized identity fields plus pending, ready, or failed: `200` for ready, or `503` with `Retry-After: 1` for pending and terminal failed. The full CLI syntax diff --git a/tests/codex-integration/codex-desired-state.test.ts b/tests/codex-integration/codex-desired-state.test.ts index 92651f2293b..91991670298 100644 --- a/tests/codex-integration/codex-desired-state.test.ts +++ b/tests/codex-integration/codex-desired-state.test.ts @@ -23,6 +23,7 @@ import { shouldSyncGrokOnStart, syncCodexOnStartIfEnabled, } from "../../src/codex/desired-state"; +import { createReadinessGate } from "../../src/server/readiness"; import type { OcxConfig } from "../../src/types"; import { removeTreeWithRetry } from "../helpers/remove-tree"; @@ -268,6 +269,33 @@ describe("the startup gate", () => { expect(ran.catalogWritten).toBe(false); expect(ran.cacheSynced).toBe(false); }); + + /** + * #5181. The gate is driven from here, so the boundary is worth pinning at the caller and not + * only in `runStartupReadinessSync`: a catalog-sync warning describes artifacts in the local + * Codex home, and the proxy that failed to write them is still serving every other provider. + * Treating it as terminal is what removed the only Service endpoint in a single-replica + * Kubernetes deployment. The sync's own `ok` remains the verdict. + */ + test("a catalog-sync warning leaves the gate ready; only ok=false fails it", async () => { + const degraded = createReadinessGate(); + await syncCodexOnStartIfEnabled( + 10100, + {}, + async () => ({ ok: true, warning: "catalog sync skipped: no Codex catalog source found." }), + degraded, + ); + expect(degraded.getStatus()).toBe("ready"); + + const refused = createReadinessGate(); + await syncCodexOnStartIfEnabled( + 10100, + {}, + async () => ({ ok: false }), + refused, + ); + expect(refused.getStatus()).toBe("failed"); + }); }); describe("Grok has the same durability, because it shipped without it", () => { diff --git a/tests/server/proxy-liveness.test.ts b/tests/server/proxy-liveness.test.ts index 3d83e113ff8..d675f04c8b8 100644 --- a/tests/server/proxy-liveness.test.ts +++ b/tests/server/proxy-liveness.test.ts @@ -594,9 +594,32 @@ describe("runStartupReadinessSync", () => { expect(gate.getStatus()).toBe("failed"); }); - test("ok=true with nonempty warning → failed", async () => { + // #5181: a nonempty warning names a degradation of the LOCAL Codex home's artifacts that the + // sync itself continued past. Treating it as terminal permanently un-readied a proxy that was + // still serving every other provider, and in a single-replica Kubernetes deployment that + // removed the only Service endpoint. `ok` is the sync's verdict; `warning` is not. + test.each([ + "catalog sync skipped: no Codex catalog source found; keeping Codex's native catalog.", + "1 combo omitted from the catalog because member capabilities are incomplete.", + "catalog sync skipped: refresh failed", + "Codex conversation-history relabel left to Codex's native writer: preflight refused.", + ])("ok=true with a local-artifact warning → ready (%s)", async warning => { const gate = createReadinessGate(); - await runStartupReadinessSync(gate, async () => ({ ok: true, warning: "catalog sync skipped: no source" })); + await runStartupReadinessSync(gate, async () => ({ ok: true, warning })); + expect(gate.getStatus()).toBe("ready"); + }); + + // The narrowing is to `warning` alone. A sync that reports the essential work unfinished is + // still terminal, warning or not, so a genuine startup failure cannot ride in as a degradation. + test("ok=false with a warning → still failed", async () => { + const gate = createReadinessGate(); + await runStartupReadinessSync(gate, async () => ({ ok: false, warning: "catalog sync skipped: no source" })); + expect(gate.getStatus()).toBe("failed"); + }); + + test("a result with no ok field at all → failed", async () => { + const gate = createReadinessGate(); + await runStartupReadinessSync(gate, async () => ({ warning: "catalog sync skipped: no source" })); expect(gate.getStatus()).toBe("failed"); }); diff --git a/tests/server/server-live.test.ts b/tests/server/server-live.test.ts index 96ef76625ff..7834021604d 100644 --- a/tests/server/server-live.test.ts +++ b/tests/server/server-live.test.ts @@ -1398,11 +1398,12 @@ test("frame diagnostics retain only metadata for text, binary, and bounded views // PRIVATE gate via createReadinessGate(); starting/failing a second server in the // same process can never reset or mutate the first server's gate. describe("GET /readyz", () => { - test("controlled startup sync drives the server gate to ready or failed", async () => { + test("startup sync drives the gate: ok=true is ready even with a warning, ok=false fails (#5181)", async () => { saveConfig(forwardConfig()); const cases = [ { outcome: { ok: true }, expectedStatus: "ready", expectedHttp: 200 }, - { outcome: { ok: true, warning: "catalog sync blocked" }, expectedStatus: "failed", expectedHttp: 503 }, + { outcome: { ok: true, warning: "catalog sync blocked" }, expectedStatus: "ready", expectedHttp: 200 }, + { outcome: { ok: false }, expectedStatus: "failed", expectedHttp: 503 }, ] as const; for (const { outcome, expectedStatus, expectedHttp } of cases) { From 4b345b37a207afba9cfae44853761e2a79e9e162 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 18 Sep 2026 15:32:31 -0700 Subject: [PATCH 3/4] fix(codex): scope model denial evidence to the credential generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An authenticated unsupported-model refusal is remembered for six hours under (account id, model id) with no credential generation. Re-auth of the same pool account keeps the internal id and increments the generation, and can swap the subscription underneath it — so the previous credential's refusal keeps steering routing away from an account that now has access. The account-wide forget that would have covered this (`model-entitlements.ts`) sits behind a condition requiring a previously cached roster with changed identity, so with no roster it never runs — which is exactly the state the flagship request path is usually in, since nothing on it refills the five-minute roster cache. Entries now carry the generation they were observed under, and: - the reader drops evidence whose generation is no longer live, so superseded evidence stops influencing the replacement without depending on the conditional forget; - a write from an older generation cannot overwrite newer evidence, so an in-flight generation-G refusal landing after G+1 is saved neither resurrects nor rewrites it; - a clear from an older generation cannot delete newer evidence, so a late generation-G success cannot re-admit an account the current credential has just been refused by. An equal generation still clears: that is the ordinary "this account just served this model" case. Evidence that cannot name a generation is dropped rather than attributed to whatever is current. Main-account contexts carry no pool generation, and already carry no account id, so nothing changes there. Cache-only routing, the six-hour TTL, the entry bound, positive-roster precedence and empty-filter restoration are all untouched. The liveness predicate is injected rather than imported so the store stays a leaf module; production wires `isCodexAccountGenerationLive`. Closes #4952 --- src/codex/model-entitlements.ts | 24 ++++- src/codex/observed-model-denials.ts | 73 ++++++++++++-- src/server/responses/core-codex-account.ts | 6 +- src/server/responses/passthrough-dispatch.ts | 18 +++- .../codex-model-denial-evidence.test.ts | 96 +++++++++++++++++-- 5 files changed, 193 insertions(+), 24 deletions(-) diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 39ed12a7b3a..51f69331ff4 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -2,7 +2,11 @@ import { createHash } from "node:crypto"; import { readBoundedResponseBody } from "../lib/bounded-body"; import type { CodexAccountCredentialRecord, OcxConfig } from "../types"; import { isSelectableCodexPoolAccount } from "./account-id"; -import { getValidCodexToken, loadCodexAccountRecordSnapshot } from "./account-store"; +import { + getValidCodexToken, + isCodexAccountGenerationLive, + loadCodexAccountRecordSnapshot, +} from "./account-store"; import { getMainAccountToken, getValidMainAccountToken, @@ -22,6 +26,7 @@ import { forgetObservedCodexModelDenialsForAccount, observedDeniedCodexAccountIdsForModel, recordObservedCodexModelDenial, + setObservedDenialGenerationCheck, resetObservedCodexModelDenialsForTests, } from "./observed-model-denials"; @@ -1330,23 +1335,36 @@ export function cachedDeniedCodexAccountIdsForModel( * admissible here: 400 covers every malformed request too, and remembering one of those as an * entitlement fact would steer routing away from a perfectly capable account. */ +// Denial evidence is credential-scoped (#4952). The store stays a leaf module, so the +// liveness predicate is injected here, where the account store is already a dependency. +setObservedDenialGenerationCheck(isCodexAccountGenerationLive); + export function recordCodexModelDenialEvidence( accountId: string | null | undefined, modelId: string | undefined, + generation: number | null | undefined, now = Date.now(), ): void { if (!accountId || !modelId) return; if (!ENTITLEMENT_PREFERRED_NATIVE_OPENAI_MODELS.has(modelId)) return; - recordObservedCodexModelDenial(accountId, modelId, now); + // A refusal with no credential generation cannot be attributed to the credential that + // earned it, so it is dropped rather than recorded against whatever is current now + // (#4952). Every production caller has the dispatched auth context in hand. + if (typeof generation !== "number") return; + recordObservedCodexModelDenial(accountId, modelId, generation, now); } /** Drop the refusal evidence for a pair the account has just served successfully. */ export function clearCodexModelDenialEvidence( accountId: string | null | undefined, modelId: string | undefined, + generation: number | null | undefined, ): void { if (!accountId || !modelId) return; - clearObservedCodexModelDenial(accountId, modelId); + // Same reasoning as the write: a success that cannot name its generation must not clear + // evidence that may belong to a newer credential (#4952). + if (typeof generation !== "number") return; + clearObservedCodexModelDenial(accountId, modelId, generation); } /** Synchronous projection for management/catalog readers after a discovery pass. */ diff --git a/src/codex/observed-model-denials.ts b/src/codex/observed-model-denials.ts index 97b0297a88b..d9ce702ac1d 100644 --- a/src/codex/observed-model-denials.ts +++ b/src/codex/observed-model-denials.ts @@ -48,8 +48,38 @@ const OBSERVED_DENIAL_TTL_MS = 6 * 60 * 60_000; /** Bounded like the roster cache: pool size times flagship count, with room to spare. */ const OBSERVED_DENIAL_MAX_ENTRIES = 512; -/** `accountId\u0000modelId` -> expiry. Insertion order is the eviction order. */ -const observedDenials = new Map(); +/** + * One entry of refusal evidence. + * + * `generation` is the credential generation the refusal was observed under (#4952). Evidence + * is about a CREDENTIAL, not an account id: reauthenticating the same internal account can + * swap the subscription underneath it, so a refusal from the previous credential says nothing + * about the replacement. Carrying the generation lets a reader ignore superseded evidence and + * lets a late reply from an older generation be refused rather than applied. + */ +interface ObservedDenial { + expiresAt: number; + generation: number; +} + +/** `accountId\u0000modelId` -> entry. Insertion order is the eviction order. */ +const observedDenials = new Map(); + +/** + * Whether `generation` is still the account's live credential. + * + * Injected rather than imported at module scope so the store stays a leaf: `account-store` + * reads the credential file, and a unit test of this map should not have to stand one up. + * Production wiring passes {@link isCodexAccountGenerationLive}. + */ +type GenerationLiveCheck = (accountId: string, generation: number) => boolean; + +let generationIsLive: GenerationLiveCheck = () => true; + +/** Wire the liveness predicate. Called once at startup; tests substitute their own. */ +export function setObservedDenialGenerationCheck(check: GenerationLiveCheck): void { + generationIsLive = check; +} function denialKey(accountId: string, modelId: string): string { return `${accountId}\u0000${modelId}`; @@ -72,12 +102,18 @@ function modelIdOfDenialKey(key: string): string { export function recordObservedCodexModelDenial( accountId: string, modelId: string, + generation: number, now = Date.now(), ): void { const key = denialKey(accountId, modelId); + const existing = observedDenials.get(key); + // A refusal dispatched under generation G can arrive after G+1 has been saved. It is + // evidence about a credential that no longer exists, so it must not overwrite — or + // resurrect — evidence belonging to the replacement (#4952). + if (existing && existing.generation > generation) return; // Delete before set so the refreshed entry moves to the back of the eviction order. observedDenials.delete(key); - observedDenials.set(key, now + OBSERVED_DENIAL_TTL_MS); + observedDenials.set(key, { expiresAt: now + OBSERVED_DENIAL_TTL_MS, generation }); while (observedDenials.size > OBSERVED_DENIAL_MAX_ENTRIES) { const oldest = observedDenials.keys().next(); if (oldest.done) break; @@ -91,8 +127,19 @@ export function recordObservedCodexModelDenial( * A success is newer and stronger evidence than the refusal that preceded it: whatever the * entitlement was when upstream refused, it is not that now. */ -export function clearObservedCodexModelDenial(accountId: string, modelId: string): void { - observedDenials.delete(denialKey(accountId, modelId)); +export function clearObservedCodexModelDenial( + accountId: string, + modelId: string, + generation: number, +): void { + const key = denialKey(accountId, modelId); + const existing = observedDenials.get(key); + if (!existing) return; + // The mirror of the write fence: a success dispatched under G arriving after G+1 was + // refused must not clear the replacement's evidence (#4952). Equal generations clear, + // because that is the ordinary "this account just served this model" case. + if (existing.generation > generation) return; + observedDenials.delete(key); } /** @@ -122,16 +169,26 @@ export function observedDeniedCodexAccountIdsForModel( ): ReadonlySet | undefined { if (!modelId) return undefined; const denied = new Set(); - for (const [key, expiresAt] of [...observedDenials]) { - if (expiresAt <= now) { + for (const [key, entry] of [...observedDenials]) { + if (entry.expiresAt <= now) { + observedDenials.delete(key); + continue; + } + const accountId = accountIdOfDenialKey(key); + // Evidence from a superseded credential is dropped rather than merely ignored: the + // account has reauthenticated, so this row can never become relevant again (#4952). + // This is what makes the fix self-healing without depending on the conditional + // account-wide forget, which cannot run when no roster was ever cached. + if (!generationIsLive(accountId, entry.generation)) { observedDenials.delete(key); continue; } - if (modelIdOfDenialKey(key) === modelId) denied.add(accountIdOfDenialKey(key)); + if (modelIdOfDenialKey(key) === modelId) denied.add(accountId); } return denied.size > 0 ? denied : undefined; } export function resetObservedCodexModelDenialsForTests(): void { observedDenials.clear(); + generationIsLive = () => true; } diff --git a/src/server/responses/core-codex-account.ts b/src/server/responses/core-codex-account.ts index c201e7acf36..9d65fed1c35 100644 --- a/src/server/responses/core-codex-account.ts +++ b/src/server/responses/core-codex-account.ts @@ -841,7 +841,11 @@ export async function retryCodexPoolOnAlternateAccount( parsed.modelId, ); if (retryModelDenial !== undefined) { - recordCodexModelDenialEvidence(retryAuthCtx.accountId, retryModelDenial); + recordCodexModelDenialEvidence( + retryAuthCtx.accountId, + retryModelDenial, + retryAuthCtx.kind === "pool" ? retryAuthCtx.generation : undefined, + ); } if (!retrySameConfirmedAccount || retrySendCount >= maxRetrySends) break; // Caller-owned main is an alternate-account replay and can never enter the bounded diff --git a/src/server/responses/passthrough-dispatch.ts b/src/server/responses/passthrough-dispatch.ts index ed8288a9140..501bfda138d 100644 --- a/src/server/responses/passthrough-dispatch.ts +++ b/src/server/responses/passthrough-dispatch.ts @@ -1340,8 +1340,16 @@ export async function preparePassthroughExchange( // refusal: whatever the entitlement was when upstream declined, it is not that now. Both // ids are cleared because the wire model can differ from the routed one. if (upstreamResponse.ok) { - clearCodexModelDenialEvidence(admissionState.authCtx.accountId, route.modelId); - clearCodexModelDenialEvidence(admissionState.authCtx.accountId, parsed.modelId); + clearCodexModelDenialEvidence( + admissionState.authCtx.accountId, + route.modelId, + admissionState.authCtx.kind === "pool" ? admissionState.authCtx.generation : undefined, + ); + clearCodexModelDenialEvidence( + admissionState.authCtx.accountId, + parsed.modelId, + admissionState.authCtx.kind === "pool" ? admissionState.authCtx.generation : undefined, + ); } const model400Denial = await codexPoolAccountModel400Denial( upstreamResponse, @@ -1354,7 +1362,11 @@ export async function preparePassthroughExchange( // answer about this model, and the roster cache that selection otherwise reads expires // five minutes after a catalog sync fills it -- so without remembering this, the next // request selects the same account on quota alone and takes the same 400 (#4906). - recordCodexModelDenialEvidence(admissionState.authCtx.accountId, model400Denial); + recordCodexModelDenialEvidence( + admissionState.authCtx.accountId, + model400Denial, + admissionState.authCtx.kind === "pool" ? admissionState.authCtx.generation : undefined, + ); poolRetryOutcome = 400; } else if (!admissionState.authCtx.fixedAccount && await shouldRetryCodexPoolAccountQuota( upstreamResponse, diff --git a/tests/codex-integration/codex-model-denial-evidence.test.ts b/tests/codex-integration/codex-model-denial-evidence.test.ts index bc1f7067de4..9eac0808d06 100644 --- a/tests/codex-integration/codex-model-denial-evidence.test.ts +++ b/tests/codex-integration/codex-model-denial-evidence.test.ts @@ -6,12 +6,16 @@ import { resetCodexModelEntitlementCacheForTests, seedCodexModelEntitlementsForTests, } from "../../src/codex/model-entitlements"; +import { setObservedDenialGenerationCheck } from "../../src/codex/observed-model-denials"; import { codexUnsupportedModelFromDetail, isAllowListedCodexAccountModel400, shouldRetryCodexPoolAccountModel400, } from "../../src/server/responses/core-codex-account"; +/** Credential generation these fixtures record under (#4952). */ +const GEN = 1; + const TEST_CLIENT_VERSION = "0.146.0"; const DAYBREAK = "gpt-daybreak-blue-latest"; const SOL = "gpt-5.6-sol"; @@ -55,7 +59,7 @@ describe("upstream refusal as per-account model denial evidence", () => { // selection sees nothing and picks the Free account on quota. expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); - recordCodexModelDenialEvidence("free", SOL, now); + recordCodexModelDenialEvidence("free", SOL, GEN, now); expect([...(cachedDeniedCodexAccountIdsForModel(SOL, now) ?? [])]).toEqual(["free"]); // Model-scoped: refusing Sol says nothing about Astra on the same account. @@ -64,7 +68,7 @@ describe("upstream refusal as per-account model denial evidence", () => { test("a confirmed roster grant outranks an earlier refusal", () => { const now = 1_800_000_000_000; - recordCodexModelDenialEvidence("plus", ASTRA, now); + recordCodexModelDenialEvidence("plus", ASTRA, GEN, now); expect([...(cachedDeniedCodexAccountIdsForModel(ASTRA, now) ?? [])]).toEqual(["plus"]); // A rollout reached the account. The newer answer wins, so a refusal cannot strand an @@ -75,10 +79,10 @@ describe("upstream refusal as per-account model denial evidence", () => { test("a success clears the refusal for that pair only", () => { const now = 1_800_000_000_000; - recordCodexModelDenialEvidence("free", SOL, now); - recordCodexModelDenialEvidence("free", ASTRA, now); + recordCodexModelDenialEvidence("free", SOL, GEN, now); + recordCodexModelDenialEvidence("free", ASTRA, GEN, now); - clearCodexModelDenialEvidence("free", SOL); + clearCodexModelDenialEvidence("free", SOL, GEN); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); expect([...(cachedDeniedCodexAccountIdsForModel(ASTRA, now) ?? [])]).toEqual(["free"]); @@ -86,7 +90,7 @@ describe("upstream refusal as per-account model denial evidence", () => { test("refusal evidence expires, and outlives the five-minute roster window", () => { const now = 1_800_000_000_000; - recordCodexModelDenialEvidence("free", SOL, now); + recordCodexModelDenialEvidence("free", SOL, GEN, now); // The roster TTL is where #4797's evidence disappeared. This must still be answering there. expect([...(cachedDeniedCodexAccountIdsForModel(SOL, now + 5 * 60_000 + 1) ?? [])]) @@ -99,8 +103,8 @@ describe("upstream refusal as per-account model denial evidence", () => { test("only always-visible natives are recorded, so a 400 elsewhere cannot steer routing", () => { const now = 1_800_000_000_000; - recordCodexModelDenialEvidence("free", "gpt-5.5", now); - recordCodexModelDenialEvidence("free", DAYBREAK, now); + recordCodexModelDenialEvidence("free", "gpt-5.5", GEN, now); + recordCodexModelDenialEvidence("free", DAYBREAK, GEN, now); expect(cachedDeniedCodexAccountIdsForModel("gpt-5.5", now)).toBeUndefined(); // Daybreak is account-gated and fails closed through the eligibility path instead. @@ -109,7 +113,7 @@ describe("upstream refusal as per-account model denial evidence", () => { test("an excluded account stays unknown rather than denied", () => { const now = 1_800_000_000_000; - recordCodexModelDenialEvidence("free", SOL, now); + recordCodexModelDenialEvidence("free", SOL, GEN, now); // The native-main read fence: an excluded account must produce the selection it does today. expect(cachedDeniedCodexAccountIdsForModel(SOL, now, { @@ -179,3 +183,77 @@ describe("unsupported-model refusal detection", () => { )).toBe(false); }); }); + +// ─── Credential generation (#4952) ─────────────────────────────────────────── +// +// Denial evidence is about a CREDENTIAL, not an account id. Reauthenticating the +// same internal account keeps the id and increments the generation, and can swap +// the subscription underneath it — so a refusal earned by the old credential must +// not steer routing away from the replacement. The account-wide forget that used +// to be relied on sits behind a condition requiring a previously cached roster, +// so with no roster it never runs; these pin the store's own behaviour instead. + +describe("denial evidence is scoped to the credential generation (#4952)", () => { + beforeEach(() => { + resetCodexModelEntitlementCacheForTests(); + }); + + test("evidence from a superseded credential stops denying the replacement", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("pooled", SOL, 1, now); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pooled"])); + + // The account reauthenticates: same id, generation 1 is no longer live. + setObservedDenialGenerationCheck((_id, generation) => generation === 2); + + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); + }); + + test("a late refusal from the old generation cannot deny the replacement", () => { + const now = Date.now(); + // The replacement has already been refused and re-granted, so nothing is recorded + // for generation 2 — then generation 1's in-flight 400 finally lands. + setObservedDenialGenerationCheck((_id, generation) => generation === 2); + recordCodexModelDenialEvidence("pooled", SOL, 1, now); + + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); + }); + + test("a late refusal cannot overwrite newer evidence", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("pooled", SOL, 2, now); + // Generation 1's refusal arrives afterwards; it must not take the entry back a + // generation, which would make it vanish the moment the reader checks liveness. + recordCodexModelDenialEvidence("pooled", SOL, 1, now); + + setObservedDenialGenerationCheck((_id, generation) => generation === 2); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pooled"])); + }); + + test("a late success from the old generation cannot clear newer evidence", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("pooled", SOL, 2, now); + // Generation 1's 200 lands after generation 2 was refused. Clearing here would + // re-admit an account that the current credential has just been refused by. + clearCodexModelDenialEvidence("pooled", SOL, 1); + + setObservedDenialGenerationCheck((_id, generation) => generation === 2); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pooled"])); + }); + + test("a success from the same generation still clears, which is the ordinary case", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("pooled", SOL, 2, now); + clearCodexModelDenialEvidence("pooled", SOL, 2); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); + }); + + test("evidence with no generation is dropped rather than attributed to the current one", () => { + const now = Date.now(); + // A main-account context carries no pool generation. It also carries no account + // id, so this is belt-and-braces — but recording against whatever is current now + // would be attributing a refusal to a credential that never earned it. + recordCodexModelDenialEvidence("pooled", SOL, undefined, now); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); + }); +}); From 3252e665bb2c40211702a5f794f38ace99f080df Mon Sep 17 00:00:00 2001 From: lidge-jun Date: Sun, 20 Sep 2026 02:50:40 +0900 Subject: [PATCH 4/4] fix(codex): keep main-pool denial evidence and fence stale writes before they evict Three defects in the carried #5092, found by adversarial review of the generation fences rather than by CI, which was green on all three. 1. main-pool evidence was silently discarded. Both production call sites pass `kind === "pool" ? generation : undefined`, and the carried code dropped any refusal that could not name a generation. A `main-pool` context - the stored main login taking part in rotation - has a real accountId and no POOL credential generation, because its credential lives in auth.json. So for that account #4906 reverted: the pool would re-send the model the login had just refused, on every request. A refusal with no generation is now ACCOUNT-scoped and the generation fences do not apply to it. This is the rule core-codex-account.ts already uses for the quota writer, where an absent credential generation skips the liveness check instead of discarding the write. Request-owned `main` is unaffected: its accountId is null and the existing guard returns before any of this. 2. The stale-write fence only rejected a late refusal when the same key already held newer evidence. With no entry for its own key the stale row was inserted, and at the 512-entry bound that insert evicts the oldest valid row - which no later read fence can restore, because the evidence is gone. The write now rejects a generation that is no longer live before touching the map at all, which closes the race the issue names rather than only its common case. 3. The read fence validated liveness for every non-expired entry before checking the model or the caller's exclusion set, and DELETED any row that came back not-live. Two problems. The issue asks for validation after the exclusion read fence, and an excluded account - a draining profile switch, or a request-owned credential - was causing a credential-store read on its behalf. And the predicate cannot tell "this account reauthenticated" from "the store could not be read": loadCodexAccountRecordStore catches a read failure and returns {}, so an unreadable store deleted valid evidence permanently. The reader now matches the model and applies the exclusion set first, and SKIPS a non-live row instead of deleting it. Skipping already produces the routing outcome on every read; deletion only saved memory in a bounded map, and it was the part that was unsafe. Superseded rows still leave by TTL, by eviction, and by the account-wide forget. Each liveness check also reloaded and reparsed the whole credential store per row, on the request path. The seam is now a factory that opens once per lookup over one loadCodexAccountRecordSnapshot, which is the shape that snapshot was added for, and it is not opened at all when no matching row carries a generation. Every constraint the issue sets is unchanged: cache-only routing with no added upstream fetch, the six-hour TTL, the 512-entry bound, positive-roster precedence, and empty-filter restoration. This remains an ordering preference, not an eligibility filter, and a live-generation refusal still denies. Co-authored-by: Abhishek Sharma --- src/codex/account-store.ts | 17 ++++ src/codex/model-entitlements.ts | 37 ++++---- src/codex/observed-model-denials.ts | 84 ++++++++++++----- .../codex-model-denial-evidence.test.ts | 92 +++++++++++++++++-- 4 files changed, 179 insertions(+), 51 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 4d681ecf2f4..cd8b83e4d26 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -370,6 +370,23 @@ export function isCodexAccountGenerationLive(id: string, generation: number): bo return !!record?.credential && record.deletedAt == null && record.generation === generation; } +/** + * The same verdict as {@link isCodexAccountGenerationLive}, over ONE store load. + * + * `readCodexAccountRecord` is `loadCodexAccountRecordStore()[id]`, so asking it per row + * reloads, reparses and renormalizes the whole file per row — the exact shape + * {@link loadCodexAccountRecordSnapshot} was added to avoid. The denial reader resolves + * several accounts in one synchronous pass on the request path, so it opens one checker and + * closes over the snapshot instead. + */ +export function beginCodexAccountGenerationLiveCheck(): (id: string, generation: number) => boolean { + const snapshot = loadCodexAccountRecordSnapshot(); + return (id, generation) => { + const record = snapshot[id]; + return !!record?.credential && record.deletedAt == null && record.generation === generation; + }; +} + export function saveCodexAccountCredentialIfGeneration( id: string, generation: number, diff --git a/src/codex/model-entitlements.ts b/src/codex/model-entitlements.ts index 51f69331ff4..c9e1c8c8e0e 100644 --- a/src/codex/model-entitlements.ts +++ b/src/codex/model-entitlements.ts @@ -3,8 +3,8 @@ import { readBoundedResponseBody } from "../lib/bounded-body"; import type { CodexAccountCredentialRecord, OcxConfig } from "../types"; import { isSelectableCodexPoolAccount } from "./account-id"; import { + beginCodexAccountGenerationLiveCheck, getValidCodexToken, - isCodexAccountGenerationLive, loadCodexAccountRecordSnapshot, } from "./account-store"; import { @@ -1302,13 +1302,14 @@ export function cachedDeniedCodexAccountIdsForModel( // absent an ongoing catalog sync the loop above contributes nothing at all. An upstream // refusal does not expire on that schedule and is not a snapshot of a pending answer: it is // the account's own Codex surface naming this model and declining it (#4906). - for (const accountId of observedDeniedCodexAccountIdsForModel(modelId, now) ?? []) { - // Under the caller's read fence, like the roster loop above. Nothing here reads account - // storage, but an excluded account must stay UNKNOWN rather than denied so a profile switch - // or a request-owned credential produces the same selection it does today. - if (options.excludeAccountIds?.has(accountId)) continue; - denied.add(accountId); - } + // The caller's read fence is passed IN rather than applied to the result, so an excluded + // account is skipped before the credential-generation validation reads account storage + // (#4952). An excluded account must stay UNKNOWN rather than denied, so a profile switch or + // a request-owned credential produces the same selection it does today. + const observedDenied = observedDeniedCodexAccountIdsForModel(modelId, now, { + ...(options.excludeAccountIds ? { excludeAccountIds: options.excludeAccountIds } : {}), + }); + for (const accountId of observedDenied ?? []) denied.add(accountId); // One account holds one entry per client version, and upstream filters the roster by that // version. So the same account can legitimately carry a granted entry under a current client // and a denied one under an older client that predates the model. Positive evidence is @@ -1337,7 +1338,7 @@ export function cachedDeniedCodexAccountIdsForModel( */ // Denial evidence is credential-scoped (#4952). The store stays a leaf module, so the // liveness predicate is injected here, where the account store is already a dependency. -setObservedDenialGenerationCheck(isCodexAccountGenerationLive); +setObservedDenialGenerationCheck(beginCodexAccountGenerationLiveCheck); export function recordCodexModelDenialEvidence( accountId: string | null | undefined, @@ -1347,11 +1348,12 @@ export function recordCodexModelDenialEvidence( ): void { if (!accountId || !modelId) return; if (!ENTITLEMENT_PREFERRED_NATIVE_OPENAI_MODELS.has(modelId)) return; - // A refusal with no credential generation cannot be attributed to the credential that - // earned it, so it is dropped rather than recorded against whatever is current now - // (#4952). Every production caller has the dispatched auth context in hand. - if (typeof generation !== "number") return; - recordObservedCodexModelDenial(accountId, modelId, generation, now); + // A caller that cannot name a credential generation records ACCOUNT-scoped evidence rather + // than none. The one production context in that position is `main-pool`, whose credential + // lives in `auth.json` and has no pool generation; discarding its refusals would revert + // #4906 for the stored main login (#4952). Request-owned `main` never reaches here — its + // `accountId` is null and the guard above returns. + recordObservedCodexModelDenial(accountId, modelId, typeof generation === "number" ? generation : undefined, now); } /** Drop the refusal evidence for a pair the account has just served successfully. */ @@ -1361,10 +1363,9 @@ export function clearCodexModelDenialEvidence( generation: number | null | undefined, ): void { if (!accountId || !modelId) return; - // Same reasoning as the write: a success that cannot name its generation must not clear - // evidence that may belong to a newer credential (#4952). - if (typeof generation !== "number") return; - clearObservedCodexModelDenial(accountId, modelId, generation); + // Mirror of the write: an account-scoped success clears account-scoped evidence. It cannot + // clear a credential-scoped entry that names a newer generation, and vice versa (#4952). + clearObservedCodexModelDenial(accountId, modelId, typeof generation === "number" ? generation : undefined); } /** Synchronous projection for management/catalog readers after a discovery pass. */ diff --git a/src/codex/observed-model-denials.ts b/src/codex/observed-model-denials.ts index d9ce702ac1d..e8a0c9ec32f 100644 --- a/src/codex/observed-model-denials.ts +++ b/src/codex/observed-model-denials.ts @@ -56,29 +56,46 @@ const OBSERVED_DENIAL_MAX_ENTRIES = 512; * swap the subscription underneath it, so a refusal from the previous credential says nothing * about the replacement. Carrying the generation lets a reader ignore superseded evidence and * lets a late reply from an older generation be refused rather than applied. + * + * It is OPTIONAL because not every account that can be refused has one. A `main-pool` context + * — the stored main login taking part in rotation — carries a real `accountId` and a + * `writerGeneration`, but no pool credential generation, because its credential lives in + * `auth.json` rather than the pool store. Dropping its evidence would have silently reverted + * #4906 for that account: the pool would re-send the same model to the login that just refused + * it, on every request, forever. An entry without a generation is account-scoped, and the + * generation fences below simply do not apply to it. This is the same rule the quota writer + * already uses in `core-codex-account.ts`, where an absent credential generation skips the + * liveness check instead of discarding the write. */ interface ObservedDenial { expiresAt: number; - generation: number; + generation?: number; } /** `accountId\u0000modelId` -> entry. Insertion order is the eviction order. */ const observedDenials = new Map(); +/** Whether `generation` is still the account's live credential. */ +type GenerationLiveCheck = (accountId: string, generation: number) => boolean; + /** - * Whether `generation` is still the account's live credential. + * Opens one liveness check. + * + * A FACTORY rather than a bare predicate because the production implementation reads the + * credential store, and a lookup can ask about several accounts. Loading once per lookup and + * closing over that snapshot is the shape `loadCodexAccountRecordSnapshot` exists for; a bare + * predicate would reload and reparse the whole store per row, on the request path. * - * Injected rather than imported at module scope so the store stays a leaf: `account-store` + * Injected rather than imported at module scope so this stays a leaf module: `account-store` * reads the credential file, and a unit test of this map should not have to stand one up. - * Production wiring passes {@link isCodexAccountGenerationLive}. */ -type GenerationLiveCheck = (accountId: string, generation: number) => boolean; +type GenerationLiveCheckFactory = () => GenerationLiveCheck; -let generationIsLive: GenerationLiveCheck = () => true; +let beginGenerationLiveCheck: GenerationLiveCheckFactory = () => () => true; /** Wire the liveness predicate. Called once at startup; tests substitute their own. */ -export function setObservedDenialGenerationCheck(check: GenerationLiveCheck): void { - generationIsLive = check; +export function setObservedDenialGenerationCheck(begin: GenerationLiveCheckFactory): void { + beginGenerationLiveCheck = begin; } function denialKey(accountId: string, modelId: string): string { @@ -102,15 +119,21 @@ function modelIdOfDenialKey(key: string): string { export function recordObservedCodexModelDenial( accountId: string, modelId: string, - generation: number, + generation: number | undefined, now = Date.now(), ): void { + // A refusal dispatched under generation G can arrive after G+1 has been saved. Reject it + // BEFORE touching the map at all, not merely when this key already holds newer evidence: + // with no entry for this key the stale row would otherwise be inserted, and at the entry + // bound it would evict a valid row that nothing can restore (#4952). + if (generation !== undefined && !beginGenerationLiveCheck()(accountId, generation)) return; const key = denialKey(accountId, modelId); const existing = observedDenials.get(key); - // A refusal dispatched under generation G can arrive after G+1 has been saved. It is - // evidence about a credential that no longer exists, so it must not overwrite — or - // resurrect — evidence belonging to the replacement (#4952). - if (existing && existing.generation > generation) return; + // Second fence, for the window where the replacement credential has been dispatched but the + // store read above still answers live. Both sides must name a generation to be comparable; + // an account-scoped entry is not older or newer than a credential-scoped one. + if (existing !== undefined && existing.generation !== undefined && generation !== undefined + && existing.generation > generation) return; // Delete before set so the refreshed entry moves to the back of the eviction order. observedDenials.delete(key); observedDenials.set(key, { expiresAt: now + OBSERVED_DENIAL_TTL_MS, generation }); @@ -130,7 +153,7 @@ export function recordObservedCodexModelDenial( export function clearObservedCodexModelDenial( accountId: string, modelId: string, - generation: number, + generation: number | undefined, ): void { const key = denialKey(accountId, modelId); const existing = observedDenials.get(key); @@ -138,7 +161,8 @@ export function clearObservedCodexModelDenial( // The mirror of the write fence: a success dispatched under G arriving after G+1 was // refused must not clear the replacement's evidence (#4952). Equal generations clear, // because that is the ordinary "this account just served this model" case. - if (existing.generation > generation) return; + if (existing.generation !== undefined && generation !== undefined + && existing.generation > generation) return; observedDenials.delete(key); } @@ -166,29 +190,41 @@ export function forgetObservedCodexModelDenialsForAccount(accountId: string | nu export function observedDeniedCodexAccountIdsForModel( modelId: string | undefined, now = Date.now(), + options: { excludeAccountIds?: ReadonlySet } = {}, ): ReadonlySet | undefined { if (!modelId) return undefined; const denied = new Set(); + // Opened lazily and at most once: a lookup that matches no credential-scoped row must not + // read the credential store at all. + let isLive: GenerationLiveCheck | undefined; for (const [key, entry] of [...observedDenials]) { if (entry.expiresAt <= now) { observedDenials.delete(key); continue; } + // Model and the caller's exclusion fence FIRST. The issue asks for identity validation + // after the exclusion read, and an excluded account — a draining profile switch, or a + // request-owned credential — must produce no credential-store read on its behalf. + if (modelIdOfDenialKey(key) !== modelId) continue; const accountId = accountIdOfDenialKey(key); - // Evidence from a superseded credential is dropped rather than merely ignored: the - // account has reauthenticated, so this row can never become relevant again (#4952). - // This is what makes the fix self-healing without depending on the conditional - // account-wide forget, which cannot run when no roster was ever cached. - if (!generationIsLive(accountId, entry.generation)) { - observedDenials.delete(key); - continue; + if (options.excludeAccountIds?.has(accountId)) continue; + if (entry.generation !== undefined) { + isLive ??= beginGenerationLiveCheck(); + // Superseded evidence stops denying the replacement. It is SKIPPED, not deleted: the + // predicate cannot tell "this account reauthenticated" from "the credential store could + // not be read", and deleting on the second would throw away valid evidence that a + // transient read failure was never entitled to touch. Skipping already delivers the + // routing outcome the issue asks for, on every read, without depending on the + // conditional account-wide forget that cannot run when no roster was ever cached. + // Superseded rows still leave by TTL, by eviction, and by the account-wide forget. + if (!isLive(accountId, entry.generation)) continue; } - if (modelIdOfDenialKey(key) === modelId) denied.add(accountId); + denied.add(accountId); } return denied.size > 0 ? denied : undefined; } export function resetObservedCodexModelDenialsForTests(): void { observedDenials.clear(); - generationIsLive = () => true; + beginGenerationLiveCheck = () => () => true; } diff --git a/tests/codex-integration/codex-model-denial-evidence.test.ts b/tests/codex-integration/codex-model-denial-evidence.test.ts index 9eac0808d06..18e12f7d44b 100644 --- a/tests/codex-integration/codex-model-denial-evidence.test.ts +++ b/tests/codex-integration/codex-model-denial-evidence.test.ts @@ -198,13 +198,18 @@ describe("denial evidence is scoped to the credential generation (#4952)", () => resetCodexModelEntitlementCacheForTests(); }); + /** The liveness seam is a factory so one lookup loads the credential store once. */ + function onlyGenerationIsLive(live: number): void { + setObservedDenialGenerationCheck(() => (_id, generation) => generation === live); + } + test("evidence from a superseded credential stops denying the replacement", () => { const now = Date.now(); recordCodexModelDenialEvidence("pooled", SOL, 1, now); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pooled"])); // The account reauthenticates: same id, generation 1 is no longer live. - setObservedDenialGenerationCheck((_id, generation) => generation === 2); + onlyGenerationIsLive(2); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); }); @@ -213,7 +218,7 @@ describe("denial evidence is scoped to the credential generation (#4952)", () => const now = Date.now(); // The replacement has already been refused and re-granted, so nothing is recorded // for generation 2 — then generation 1's in-flight 400 finally lands. - setObservedDenialGenerationCheck((_id, generation) => generation === 2); + onlyGenerationIsLive(2); recordCodexModelDenialEvidence("pooled", SOL, 1, now); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); @@ -226,7 +231,7 @@ describe("denial evidence is scoped to the credential generation (#4952)", () => // generation, which would make it vanish the moment the reader checks liveness. recordCodexModelDenialEvidence("pooled", SOL, 1, now); - setObservedDenialGenerationCheck((_id, generation) => generation === 2); + onlyGenerationIsLive(2); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pooled"])); }); @@ -237,7 +242,7 @@ describe("denial evidence is scoped to the credential generation (#4952)", () => // re-admit an account that the current credential has just been refused by. clearCodexModelDenialEvidence("pooled", SOL, 1); - setObservedDenialGenerationCheck((_id, generation) => generation === 2); + onlyGenerationIsLive(2); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pooled"])); }); @@ -248,12 +253,81 @@ describe("denial evidence is scoped to the credential generation (#4952)", () => expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); }); - test("evidence with no generation is dropped rather than attributed to the current one", () => { + // A `main-pool` context — the stored main login taking part in rotation — has a real account + // id and NO pool credential generation, because its credential lives in auth.json. Dropping + // its evidence would silently revert #4906 for that account: the pool would re-send the model + // the login just refused, on every request. Its evidence is account-scoped instead. + test("evidence with no generation is account-scoped, not discarded", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("main-pool-account", SOL, undefined, now); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["main-pool-account"])); + }); + + test("account-scoped evidence is not expired by a pool generation rolling over", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("main-pool-account", SOL, undefined, now); + // No generation was ever claimed, so there is nothing for the liveness fence to supersede. + onlyGenerationIsLive(7); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["main-pool-account"])); + }); + + test("an account-scoped success clears account-scoped evidence", () => { const now = Date.now(); - // A main-account context carries no pool generation. It also carries no account - // id, so this is belt-and-braces — but recording against whatever is current now - // would be attributing a refusal to a credential that never earned it. - recordCodexModelDenialEvidence("pooled", SOL, undefined, now); + recordCodexModelDenialEvidence("main-pool-account", SOL, undefined, now); + clearCodexModelDenialEvidence("main-pool-account", SOL, undefined); expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toBeUndefined(); }); + + // The write fence has to reject a stale refusal BEFORE it mutates the map, not only when the + // same key already holds newer evidence. With no entry for its own key the stale row would be + // inserted, and at the entry bound the insert evicts the oldest valid row — which no later + // read fence can restore, because the evidence is simply gone. + test("a stale refusal for an unseen key cannot evict valid evidence at the entry bound", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("first-pooled", SOL, 2, now); + for (let i = 0; i < 511; i++) recordCodexModelDenialEvidence(`filler-${i}`, SOL, 2, now); + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)?.has("first-pooled")).toBe(true); + + // Generation 1 is dead and this account has no entry of its own. The insert would take the + // map to 513 and evict the oldest row, which is the valid one recorded first. + onlyGenerationIsLive(2); + recordCodexModelDenialEvidence("late-stale", SOL, 1, now); + + const denied = cachedDeniedCodexAccountIdsForModel(SOL, now); + expect(denied?.has("first-pooled")).toBe(true); + expect(denied?.has("late-stale")).toBe(false); + }); + + // The issue asks for identity validation AFTER the exclusion read fence. An excluded account + // — a draining profile switch, or a request-owned credential — must not cause a credential + // store read on its behalf, and must stay unknown rather than denied. + test("an excluded account is skipped before the liveness check reads anything", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("excluded", SOL, 1, now); + let lookups = 0; + setObservedDenialGenerationCheck(() => { + lookups += 1; + return () => true; + }); + + expect(cachedDeniedCodexAccountIdsForModel(SOL, now, { + excludeAccountIds: new Set(["excluded"]), + })).toBeUndefined(); + expect(lookups).toBe(0); + }); + + test("the credential store is opened at most once per lookup", () => { + const now = Date.now(); + recordCodexModelDenialEvidence("pool-a", SOL, 1, now); + recordCodexModelDenialEvidence("pool-b", SOL, 1, now); + recordCodexModelDenialEvidence("pool-c", SOL, 1, now); + let opens = 0; + setObservedDenialGenerationCheck(() => { + opens += 1; + return () => true; + }); + + expect(cachedDeniedCodexAccountIdsForModel(SOL, now)).toEqual(new Set(["pool-a", "pool-b", "pool-c"])); + expect(opens).toBe(1); + }); });