From b7a9e90f7ed51e0432c34d5f1443b1f41ea87db6 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 20:19:46 +0900 Subject: [PATCH 1/5] fix(codex): bound stored pool 401 recovery budget --- src/server/responses/compact.ts | 9 +- src/server/responses/core.ts | 21 +- src/server/responses/policy-fallback.ts | 17 +- tests/responses-native-main-refresh.test.ts | 57 ++++- tests/responses-pool-401-refresh.test.ts | 256 +++++++++++++++++++- tests/routing-policy-fallback.test.ts | 27 +++ 6 files changed, 364 insertions(+), 23 deletions(-) diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 6eb6b7754e..b14800fc7b 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -163,6 +163,10 @@ interface CompactHandoffRoute { */ const compactHandoffRoutes = new Map(); +export function clearCompactHandoffRoutesForTests(): void { + compactHandoffRoutes.clear(); +} + function pruneCompactHandoffRoutes(now: number): void { for (const [key, entry] of compactHandoffRoutes) { if (now - entry.lastUsedAt > COMPACT_HANDOFF_ROUTE_TTL_MS) compactHandoffRoutes.delete(key); @@ -740,6 +744,7 @@ export async function handleResponsesCompact( // actually happens, so every recorder call names the context that produced it. let outcomeCtx = authCtx; let upstream: Response; + let storedPool401ReplayAttempted = false; try { // Same connect timeout + keep-alive reset + transient-5xx recovery as /v1/responses — // compact hits the same ChatGPT host and must soft-avoid / clear affinity (#186). @@ -777,6 +782,7 @@ export async function handleResponsesCompact( ) { await upstream.body?.cancel().catch(() => undefined); const poolAuthCtx = authCtx.kind === "pool" ? authCtx : undefined; + storedPool401ReplayAttempted = poolAuthCtx !== undefined; const poolReplay = poolAuthCtx ? await refreshPoolCompactContext({ req, @@ -837,6 +843,7 @@ export async function handleResponsesCompact( // — reporting exhausted retries while another pool account sat idle (#913). if ( (upstream.status === 429 || upstream.status === 402) + && !storedPool401ReplayAttempted && usesCodexForwardPoolAuth(authCtx, route.provider) && !authCtx.fixedAccount && route.codexAccountMode @@ -947,7 +954,7 @@ export async function handleResponsesCompact( if (buffered.ok) { inspectResponseLogJson(logCtx, await buffered.clone().text()); forgetCompactHandoffRoute(req); - } else if (quotaFailure) { + } else if (quotaFailure && !storedPool401ReplayAttempted) { const fallbackModel = compactHandoffRoute(req, raw.model); if (fallbackModel && !req.signal.aborted) { const fallbackReq = new Request(req.url, { diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index f0c1291c2a..1c0fcd87f9 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -1433,6 +1433,8 @@ export interface HandleResponsesOptions { deferCodexResetDerivedCooldown?: boolean; /** 030-owned handoff when a child consumed the original failure under bounds. */ onConsumedComboFailure?: (failure: ConsumedComboFailure) => void; + /** A stored Pool credential was refreshed and its one allowed same-account replay was sent. */ + onStoredPool401ReplayDispatched?: () => void; /** Caller-owned for Chat/Claude replay; omitted only at genuine Responses ingress. */ translatorBudget?: TranslatorBudget; /** @@ -2289,6 +2291,7 @@ export async function handleComboResponses( attemptRetained = true; }; let consumedChildFailure: ConsumedComboFailure | undefined; + let storedPool401ReplayDispatched = false; const callbackGate = createChildPassthroughCallbackGate(options); let response: Response; try { @@ -2315,6 +2318,7 @@ export async function handleComboResponses( onCodexAuthContextResolved: value => { resolvedAuth = value; }, setTerminalOutcomeRecorder: value => { terminalRecorder = value; }, onConsumedComboFailure: value => { consumedChildFailure = value; }, + onStoredPool401ReplayDispatched: () => { storedPool401ReplayDispatched = true; }, onNativePassthroughTerminal: callbackGate.onTerminal, onNativePassthroughCancel: callbackGate.onCancel, }); @@ -2420,6 +2424,10 @@ export async function handleComboResponses( (logCtx.attempts ??= []).push(attempt); attemptRetained = true; lastFailure = failure.response; + if (storedPool401ReplayDispatched) { + adoptFailedChildLog(childLog); + return lastFailure; + } if (comboFailureDecision(failure.response.status, failure.classificationText, { code: failure.upstreamCode, }) === "stop") { @@ -3839,7 +3847,7 @@ async function handleResponsesInner( const opaqueBlobRecoveryGuard: OpaqueBlobRecoveryGuard = { attempted: false }; let oauth401ReplayAttempted = false; - let codexMain401ReplayAttempted = false; + let codex401ReplayKind: "main" | "stored" | null = null; const rateLimitPolicy = rateLimitRetryPolicyFor(route.provider); let rateLimitRetries = 0; const rebuildAndRefetch = async ( @@ -3914,9 +3922,9 @@ async function handleResponsesInner( upstreamResponse.status === 401 && (authCtx.kind === "main-pool" || authCtx.kind === "pool") && usesCodexForwardPoolAuth(authCtx, route.provider) - && !codexMain401ReplayAttempted + && codex401ReplayKind === null ) { - codexMain401ReplayAttempted = true; + codex401ReplayKind = authCtx.kind === "pool" ? "stored" : "main"; try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed */ } const poolAuthCtx = authCtx.kind === "pool" ? authCtx : undefined; const poolReplay = poolAuthCtx @@ -3972,6 +3980,7 @@ async function handleResponsesInner( recordAdapterTier(logCtx, request); refreshUndeclaredToolGuard(request); noteAttemptSend(logCtx.activeAttempt, passthroughEstimate, "oauth-401"); + if (codex401ReplayKind === "stored") options.onStoredPool401ReplayDispatched?.(); upstreamResponse = await fetchWithHeaderTimeout( request.url, { method: request.method, headers: request.headers, body: request.body }, @@ -3995,7 +4004,11 @@ async function handleResponsesInner( continue passthroughRecovery; } - if (codexMain401ReplayAttempted && upstreamResponse.status === 401) break; + if (codex401ReplayKind !== null && upstreamResponse.status === 401) break; + // A stored Pool 401 owns one refresh and one same-account replay. The replay + // result is authoritative for this logical request and cannot enter a later + // account, model, or combo recovery ladder. + if (codex401ReplayKind === "stored" && upstreamResponse.status >= 400) break; // Native Responses providers return before the generic adapter recovery loop below. Keep // their OAuth contract identical: one pre-stream 401 forces a credential refresh and one diff --git a/src/server/responses/policy-fallback.ts b/src/server/responses/policy-fallback.ts index eabb70cf25..a4f06d0fa6 100644 --- a/src/server/responses/policy-fallback.ts +++ b/src/server/responses/policy-fallback.ts @@ -116,16 +116,21 @@ export async function handleResponsesWithPolicyFallback( ): Promise { const runCore = deps.runCore ?? handleResponsesCore; let requestBodyReadNotified = false; - const coreOptions: CoreOptions = options.onRequestBodyRead - ? { - ...options, + let storedPool401ReplayDispatched = false; + const coreOptions: CoreOptions = { + ...options, + ...(options.onRequestBodyRead ? { onRequestBodyRead: () => { if (requestBodyReadNotified) return; requestBodyReadNotified = true; options.onRequestBodyRead?.(); }, - } - : options; + } : {}), + onStoredPool401ReplayDispatched: () => { + storedPool401ReplayDispatched = true; + options.onStoredPool401ReplayDispatched?.(); + }, + }; let rawBody: Record | null = null; try { const parsed = await readJsonRequestBody(req.clone()); @@ -150,7 +155,7 @@ export async function handleResponsesWithPolicyFallback( candidateKey({ provider: initialTrace.selected.provider, model: initialTrace.selected.model }), ]); - while (await shouldHopPolicyCandidate(response, req.signal)) { + while (!storedPool401ReplayDispatched && await shouldHopPolicyCandidate(response, req.signal)) { if (req.signal.aborted) return response; const next = rankPolicyFallbackCandidates(initialTrace, tried)[0]; if (!next) return response; diff --git a/tests/responses-native-main-refresh.test.ts b/tests/responses-native-main-refresh.test.ts index f700d7cae6..ab59bebe1f 100644 --- a/tests/responses-native-main-refresh.test.ts +++ b/tests/responses-native-main-refresh.test.ts @@ -3,6 +3,7 @@ import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { clearAccountNeedsReauth } from "../src/codex/auth-api"; +import { saveCodexAccountCredential } from "../src/codex/account-store"; import { MAIN_CODEX_ACCOUNT_ID } from "../src/codex/main-account"; import { clearCodexUpstreamHealth, clearThreadAccountMap } from "../src/codex/routing"; import { handleResponses, handleResponsesCompact } from "../src/server/responses"; @@ -13,8 +14,9 @@ const originalFetch = globalThis.fetch; let home = ""; let previousOcxHome: string | undefined; let previousCodexHome: string | undefined; +const OTHER_ACCOUNT_ID = "other"; -function config(): OcxConfig { +function config(options: { secondAccount?: boolean } = {}): OcxConfig { return { defaultProvider: "openai", activeCodexAccountId: MAIN_CODEX_ACCOUNT_ID, @@ -27,7 +29,8 @@ function config(): OcxConfig { codexAccountMode: "pool", }, }, - codexAccounts: [], + codexAccounts: options.secondAccount ? [{ id: OTHER_ACCOUNT_ID, label: "other" }] : [], + ...(options.secondAccount ? { accountPoolStrategy: "fill-first" } : {}), } as OcxConfig; } @@ -48,6 +51,7 @@ beforeEach(() => { process.env.OPENCODEX_HOME = home; process.env.CODEX_HOME = home; clearAccountNeedsReauth(MAIN_CODEX_ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); writeFileSync(join(home, "auth.json"), JSON.stringify({ @@ -62,6 +66,7 @@ beforeEach(() => { afterEach(() => { globalThis.fetch = originalFetch; clearAccountNeedsReauth(MAIN_CODEX_ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); if (previousOcxHome === undefined) delete process.env.OPENCODEX_HOME; @@ -159,4 +164,52 @@ describe("native main 401 refresh and replay", () => { expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); expect(harness.refreshes).toEqual(["refresh-grant"]); }); + + for (const path of ["/v1/responses", "/v1/responses/compact"] as const) { + test(`${path} keeps main-pool recovery eligible for a later Pool account`, async () => { + saveCodexAccountCredential(OTHER_ACCOUNT_ID, { + accessToken: "other-access", + refreshToken: "other-refresh", + expiresAt: Date.now() + 3_600_000, + chatgptAccountId: "account-other", + }); + const sends: string[] = []; + const refreshes: string[] = []; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = new URL(input instanceof Request ? input.url : String(input)); + if (url.hostname === "auth.openai.com") { + refreshes.push(new URLSearchParams(String(init?.body)).get("refresh_token") ?? ""); + return Response.json({ + access_token: "refreshed-access", + refresh_token: "rotated-refresh", + expires_in: 3600, + }); + } + if (!url.pathname.endsWith("/responses") && !url.pathname.endsWith("/responses/compact")) { + return Response.json({ rate_limit: { primary_window: { used_percent: 10 } } }); + } + const authorization = new Headers(init?.headers).get("authorization") ?? ""; + sends.push(authorization); + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "expired bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "main quota exhausted" } }, { status: 429 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "resp_other", object: "response", status: "completed", output: [] }); + } + return Response.json({ error: { message: "unexpected bearer" } }, { status: 500 }); + }) as typeof fetch; + + const cfg = config({ secondAccount: true }); + const response = path.endsWith("compact") + ? await handleResponsesCompact(request(path), cfg, { model: "", provider: "" } as RequestLogContext) + : await handleResponses(request(path), cfg, { model: "", provider: "" } as RequestLogContext); + + expect(response.status).toBe(200); + expect(sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access", "Bearer other-access"]); + expect(refreshes).toEqual(["refresh-grant"]); + }); + } }); diff --git a/tests/responses-pool-401-refresh.test.ts b/tests/responses-pool-401-refresh.test.ts index 73c78a8d13..a733953064 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -10,6 +10,7 @@ import { resolveCodexAccountForThreadDetailed, } from "../src/codex/routing"; import { handleResponses, handleResponsesCompact } from "../src/server/responses"; +import { clearCompactHandoffRoutesForTests } from "../src/server/responses/compact"; import type { RequestLogContext } from "../src/server/request-log"; import type { OcxConfig } from "../src/types"; @@ -57,19 +58,17 @@ const THREAD_ID = "thread-2887"; function request( path: "/v1/responses" | "/v1/responses/compact", - options: { affined?: boolean } = {}, + options: { affined?: boolean; model?: string; headers?: HeadersInit; stream?: boolean } = {}, ): Request { + const headers = new Headers(options.headers); + headers.set("content-type", "application/json"); + if (options.affined) headers.set("x-codex-parent-thread-id", THREAD_ID); return new Request(`http://localhost${path}`, { method: "POST", - headers: { - "content-type": "application/json", - // A bound thread is what makes affinity exist at all; without it there is no - // entry to carry across the refresh and the handoff cannot be observed. - ...(options.affined ? { "x-codex-parent-thread-id": THREAD_ID } : {}), - }, + headers, body: JSON.stringify(path.endsWith("compact") - ? { model: "gpt-5.5", input: [] } - : { model: "gpt-5.5", input: "hello", stream: false }), + ? { model: options.model ?? "gpt-5.5", input: [] } + : { model: options.model ?? "gpt-5.5", input: "hello", stream: options.stream ?? false }), }); } @@ -118,7 +117,10 @@ type Harness = { sends: string[]; refreshes: string[] }; * Upstream rejects the old bearer once, the token endpoint rotates, and the replay with the * new bearer succeeds — the reporter's deterministic harness. */ -function installHarness(options: { refresh?: () => Response } = {}): Harness { +function installHarness(options: { + refresh?: () => Response; + responseForSend?: (authorization: string, sendNumber: number, url: URL) => Response | undefined; +} = {}): Harness { const sends: string[] = []; const refreshes: string[] = []; globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { @@ -137,6 +139,8 @@ function installHarness(options: { refresh?: () => Response } = {}): Harness { } const authorization = new Headers(init?.headers).get("authorization") ?? ""; sends.push(authorization); + const customResponse = options.responseForSend?.(authorization, sends.length, url); + if (customResponse) return customResponse; if (authorization === "Bearer rejected-access") { return Response.json({ error: { message: "expired bearer" } }, { status: 401 }); } @@ -145,6 +149,26 @@ function installHarness(options: { refresh?: () => Response } = {}): Harness { return { sends, refreshes }; } +function recoveryComboConfig(): OcxConfig { + const cfg = config(); + cfg.providers.backup = { + adapter: "openai-responses", + baseUrl: "https://backup.example/v1", + authMode: "key", + apiKey: "backup-test-key", + }; + cfg.combos = { + recovery: { + strategy: "failover", + targets: [ + { provider: "openai", model: "gpt-5.5" }, + { provider: "backup", model: "m2" }, + ], + }, + }; + return cfg; +} + beforeEach(() => { home = mkdtempSync(join(tmpdir(), "ocx-responses-pool-401-")); previousOcxHome = process.env.OPENCODEX_HOME; @@ -152,6 +176,7 @@ beforeEach(() => { process.env.OPENCODEX_HOME = home; process.env.CODEX_HOME = home; clearAccountNeedsReauth(ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); writeStoredAccount(); @@ -159,7 +184,9 @@ beforeEach(() => { afterEach(() => { globalThis.fetch = originalFetch; + clearCompactHandoffRoutesForTests(); clearAccountNeedsReauth(ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); if (previousOcxHome === undefined) delete process.env.OPENCODEX_HOME; @@ -201,6 +228,215 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(false); }); + test("Responses does not compose a stored-account replay 429 with another Pool account", async () => { + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const harness = installHarness({ + responseForSend: authorization => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "pool exhausted" } }, { status: 429 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const cfg = config({ secondAccount: true }); + // This test needs one eligible alternate but must not advance the process-wide + // round-robin cursor used by the existing next-request affinity regression. + cfg.accountPoolStrategy = "fill-first"; + const response = await handleResponses( + request("/v1/responses"), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(429); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a combo stops after a stored-account replay consumes the recovery budget", async () => { + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "pool exhausted" } }, { status: 429 }); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery" }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(429); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a combo stops when the stored-account replay hits a transport error", async () => { + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + throw new TypeError("stored replay transport failure"); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery" }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(502); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a combo stops on a zero-output failure from the stored-account replay stream", async () => { + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + const events = [ + { type: "response.created", response: { id: "replay", status: "in_progress" } }, + { + type: "response.failed", + response: { + id: "replay", + status: "failed", + error: { type: "server_error", code: "upstream_server_error", message: "busy" }, + }, + }, + ]; + return new Response( + events.map(event => `data: ${JSON.stringify(event)}\n\n`).join(""), + { headers: { "content-type": "text/event-stream" } }, + ); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery", stream: true }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(502); + const failure = await response.clone().json() as { + error?: { code?: string; message?: string }; + }; + expect(failure.error?.code).toBe("upstream_server_error"); + expect(failure.error?.message).toContain("busy"); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("compact does not compose a stored-account replay 429 with a remembered model", async () => { + const headers = { "x-codex-parent-thread-id": "compact-refresh-budget" }; + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const cfg = config({ secondAccount: true }); + cfg.accountPoolStrategy = "fill-first"; + cfg.providers.seed = { + adapter: "openai-responses", + baseUrl: "https://seed.example/v1", + authMode: "key", + apiKey: "seed-test-key", + }; + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "seed.example") { + return Response.json({ + id: "seed", + object: "response", + status: "completed", + output: [{ + type: "message", + role: "assistant", + content: [{ type: "output_text", text: "seed summary", annotations: [] }], + }], + }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "pool exhausted" } }, { status: 429 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const seed = await handleResponsesCompact( + request("/v1/responses/compact", { model: "seed/seed-model", headers }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + expect(seed.status).toBe(200); + + const response = await handleResponsesCompact( + request("/v1/responses/compact", { headers }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(429); + expect(harness.sends).toEqual([ + "Bearer seed-test-key", + "Bearer rejected-access", + "Bearer refreshed-access", + ]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + test("the replayed account is still selectable on the NEXT request, not just this one", async () => { // The affinity entry is bound under generation G; the forced refresh CAS-writes G+1 and // isThreadAffinityGenerationLive demands exact equality. Without the same-lineage handoff diff --git a/tests/routing-policy-fallback.test.ts b/tests/routing-policy-fallback.test.ts index ac2fc34f28..acfadb3744 100644 --- a/tests/routing-policy-fallback.test.ts +++ b/tests/routing-policy-fallback.test.ts @@ -179,6 +179,33 @@ describe("policy candidate fallback", () => { expect(logCtx.activeAttempt).toBe(logCtx.attempts?.[1]); }); + test("a stored Pool 401 replay dispatch stops policy candidate fallback", async () => { + const trace = policyTrace(); + const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; + const seenModels: string[] = []; + let replaySignals = 0; + + const response = await handleResponsesWithPolicyFallback(request(), {} as OcxConfig, logCtx, { + onStoredPool401ReplayDispatched: () => { replaySignals += 1; }, + }, { + runCore: async (req, _config, childLog, options) => { + const body = await req.json() as { model: string }; + seenModels.push(body.model); + childLog.routeDecision = trace; + seedAttempt(childLog, "provider-a", "model-a"); + options.onStoredPool401ReplayDispatched?.(); + return Response.json( + { error: { message: "stored replay exhausted", type: "rate_limit_error" } }, + { status: 429 }, + ); + }, + }); + + expect(response.status).toBe(429); + expect(seenModels).toEqual(["policy/daily"]); + expect(replaySignals).toBe(1); + }); + test("returns local pacing overload without switching policy candidates", async () => { const trace = policyTrace(); const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; From d54ca13d6744a5fa665501783478fec418142107 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 21:00:24 +0900 Subject: [PATCH 2/5] fix(codex): bound the stored Pool 401 budget by account, not by any further send Follow-up to luvs01's a838b071c (#2895), which is preserved as the parent commit. That change enforced the recovery budget with a single break in the passthrough loop: a stored replay returning any status >= 400 ended the request. It sits above two ladders that send to the account already paying, so it stopped more than it meant to: - shouldRetryCodexPoolAccountModel400 (core.ts:4200), which retries the SAME account when the refreshed roster still grants an allow-listed gated model; - attemptOpaqueBlobRecovery (core.ts:4249), whose one-shot rebuild strips a rejected reasoning/compaction blob and resends to the same refreshed account. So 401 -> refresh -> invalid_encrypted_content became a user-visible 400 where the rebuild would have succeeded. Restoring the broad break turns the new opaque-blob regression red. What is scarce is another account's quota, not further sends, so the bound is stated that way. A quota failure after a stored replay has no same-account move left and stays terminal at the pool-retry site. A gated-model 400 keeps its ladder, and sameAccountOnly refuses alternate resolution inside retryCodexPoolOnAlternateAccount -- next to the existing fixedAccount guard, which already means the same thing for a different reason. Opaque-blob recovery is untouched. The dispatch signal also fired too early. fetchWithHeaderTimeout awaits pacing admission before invoking the executor, so signalling at the call site spent the budget even when a rejected pacing wait meant nothing reached the network. storedPoolReplayDispatchNotifier moves it to the last moment before the send, re-exposing waitForPacing and unpacedFetch because the helper reads both off the executor -- dropping the first loses pacing, dropping the second paces twice. 196 pass / 0 fail across the pool-401, native-main, policy-fallback, fetch-helper, opaque-blob, pool-rotation, compaction-routing, combo-recovery, stream-preflight and request-pacing suites; tsc clean; privacy:scan green. Five named mutations each turn their own test red. One of the new tests was vacuous on the first attempt and is recorded as such in the devlog: the injected resolver reported only the alternate account as entitled, so selection picked it and the stored 401 never happened. Co-authored-by: luvs01 --- .../170_pr2895_pool_401_recovery_budget.md | 94 +++++++++++ src/server/responses/core.ts | 45 ++++-- src/server/responses/fetch-helpers.ts | 42 +++++ .../responses-fetch-helpers-boundary.test.ts | 78 +++++++++ tests/responses-pool-401-refresh.test.ts | 150 ++++++++++++++++++ 5 files changed, 398 insertions(+), 11 deletions(-) create mode 100644 devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md new file mode 100644 index 0000000000..43cfa49e33 --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md @@ -0,0 +1,94 @@ +# Lane P — #2895 / #2892 gap 5: one recovery budget for a stored Pool 401 + +Carries contributor PR #2895 (`luvs01`, `a838b071c`) onto current `dev` and corrects the one +blocker in it. The contributor's commit is preserved with its authorship; this unit is the +follow-up commit on top. + +## What the contributor got right + +#2889 gave an ordinary stored Codex Pool account one generation-fenced forced refresh plus one +same-account replay after a pre-stream 401. Gap 5 of #2892 is that the replay's *result* was not +treated as final: a replay 429/402 could still be composed with another Pool account, a remembered +compact model, a combo target, or a policy-fallback candidate — so a single logical request could +spend several accounts' quota after the budget was already used. + +The contributor's structure is sound and is kept as-is: the boolean `codexMain401ReplayAttempted` +becomes a tri-state `codex401ReplayKind` (`"main" | "stored" | null`), an +`onStoredPool401ReplayDispatched` signal is threaded through combo and policy fallback, and compact +guards both its pool-rotation and remembered-model paths. `main-pool` keeps its full recovery +breadth, which is correct — a native main 401 is not a stored-account budget. + +## The blocker: the budget bounds accounts, not rescue + +The original patch enforced the bound with one line in `src/server/responses/core.ts`: + +```ts +if (codex401ReplayKind === "stored" && upstreamResponse.status >= 400) break; +``` + +That break sits *above* two recovery ladders that send to the account already paying: + +- `shouldRetryCodexPoolAccountModel400` (`:4200`) — an allow-listed gated-model 400, retried on + the **same** account when the refreshed roster still grants the model + (`retryCodexPoolOnAlternateAccount` sets `retryAuthCtx = firstAuthCtx` for exactly that case). +- `attemptOpaqueBlobRecovery` (`:4249`) — a rejected opaque reasoning/compaction blob, where the + one-shot rebuild strips the blob and resends to the same refreshed account. + +Neither charges a different account, so neither is inside the budget #2892 asked to bound. With the +broad break, `401 → refresh → invalid_encrypted_content` became a user-visible 400 where the +rebuild would have succeeded. A regression proves it: restoring that one line turns +*a stored-account replay may still rebuild a rejected opaque blob on the same account* red. + +The corrected boundary is stated in terms of what is actually scarce — **another account's quota**, +not further sends: + +- a quota failure (429/402) after a stored replay has no same-account move left, so it stays + terminal at the pool-retry site; +- a gated-model 400 keeps its ladder, with `sameAccountOnly` refusing alternate-account resolution + inside `retryCodexPoolOnAlternateAccount`; +- opaque-blob recovery is untouched. + +`sameAccountOnly` is a new field on the retry args rather than a check at the call site, because +the decision belongs where the alternate is resolved — the existing `fixedAccount` guard already +lives on that line and means the same thing for a different reason. + +## The timing defect in the dispatch signal + +The signal fired immediately before `fetchWithHeaderTimeout`, but that helper awaits +`pacing.waitForPacing()` (`src/server/responses/fetch-helpers.ts:121`) and only then invokes the +executor. A rejected pacing admission therefore marked the budget spent for a send that never +reached the network, and the request lost its fallback for nothing. + +`storedPoolReplayDispatchNotifier` wraps the executor so the signal fires at the last moment before +the send. It deliberately re-exposes `waitForPacing` and `unpacedFetch`: `fetchWithHeaderTimeout` +reads both off the executor, so a plain function wrapper would drop provider pacing — and a wrapper +that kept `waitForPacing` but dropped `unpacedFetch` would pace twice. Both are covered by named +mutations. + +## Verification + +196 pass / 0 fail across the pool-401, native-main, policy-fallback, fetch-helper, opaque-blob, +pool-rotation, compaction-routing, combo-recovery, stream-preflight, and request-pacing suites. +`bun x tsc --noEmit` clean; `privacy:scan` green. + +Named mutations, each turning its own test red: + +| Mutation | Test that fails | +| --- | --- | +| restore the broad `status >= 400` break | opaque blob rebuilt on the same account | +| remove the quota-terminal bound | both stored-replay-429 cases reach another account | +| `sameAccountOnly: false` | gated-model 400 after a stored replay reaches an alternate | +| notify eagerly instead of at send time | pacing-rejected case signals a phantom dispatch | +| drop `unpacedFetch` from the wrapper | pacing applied twice | + +One process note worth keeping: the gated-model test was **vacuous on the first attempt**. The +injected entitlement resolver reported only the other account as entitled, so initial selection +picked that account and the stored 401 never happened — the assertion passed with one send and no +refresh. It now returns both accounts on the first resolution and only the other account from the +retry resolution onward, which is what actually reaches the alternate-account branch. + +## Not in this unit + +Gaps 1–4 of #2892 (refresh-flight abort ownership, superseding-generation freshness, rotated-grant +fan-out to inactive aliases, atomic generation validation) remain open and are the other PR that +issue asks for. diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 1c0fcd87f9..d0db675e8e 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -321,7 +321,7 @@ import type { EffectiveSubagentRoster, SpawnAgentSurface } from "../../codex/cat import { buildToolBridgeMaps, collabSurface, injectDeveloperMessage, multiAgentGuidanceText } from "./collaboration"; import { mapCodexAuthContextErrorToResponse, nativeMainRefreshFailureResponse } from "./codex-auth-error"; import { hasUnreadableEncryptedAgentTask, looksLikeBackendCiphertext, sanitizeEncryptedContentInPlace } from "./encrypted-payload"; -import { fetchWithHeaderTimeout, providerFetch, safeHostLabel, safeOriginLabel } from "./fetch-helpers"; +import { fetchWithHeaderTimeout, providerFetch, safeHostLabel, safeOriginLabel, storedPoolReplayDispatchNotifier } from "./fetch-helpers"; import { classifyTransportFailureKind, transportErrorCode } from "../../lib/upstream-reachability"; import { acquireUpstreamHostAdmission, @@ -922,6 +922,15 @@ interface CodexPoolAccountRetryArgs { firstAuthCtx: Extract; firstResponse: Response; outcomeStatus: number; + /** + * Forbid resolving a DIFFERENT account for this retry. + * + * Set when a stored Pool 401 already spent this logical request's account budget on its own + * refresh and replay. The same-account gated-model retry above stays available, because it + * sends to the account that was already paying; only the alternate-account resolution below is + * out of budget. + */ + sameAccountOnly?: boolean; upstream: AbortController; connectMs: number; passthroughEstimate?: number; @@ -1084,7 +1093,9 @@ async function retryCodexPoolOnAlternateAccount( } // Exact account selectors may retry the same confirmed account above, but must never resolve // an alternate. Quota failures and a refreshed entitlement miss remain terminal. - if (!retryAuthCtx && firstAuthCtx.fixedAccount) return { kind: "no-alternate" }; + if (!retryAuthCtx && (firstAuthCtx.fixedAccount || args.sameAccountOnly === true)) { + return { kind: "no-alternate" }; + } try { retryAuthCtx ??= await resolveCodexAuthContext( req.headers, @@ -3980,17 +3991,25 @@ async function handleResponsesInner( recordAdapterTier(logCtx, request); refreshUndeclaredToolGuard(request); noteAttemptSend(logCtx.activeAttempt, passthroughEstimate, "oauth-401"); - if (codex401ReplayKind === "stored") options.onStoredPool401ReplayDispatched?.(); upstreamResponse = await fetchWithHeaderTimeout( request.url, { method: request.method, headers: request.headers, body: request.body }, upstream.signal, connectMs, parsed.stream, - providerFetch(route.provider, options.codexWsRuntimeIdentity, { - providerName: route.providerName, - modelId: route.modelId, - }), + // The replay-dispatched signal is what bounds the rest of this logical request, so it + // has to describe a send that actually happened. fetchWithHeaderTimeout awaits pacing + // admission BEFORE calling the executor, so signalling at the call site would spend the + // budget even when a rejected pacing wait means nothing reaches the network. Wrapping + // the executor moves the signal to the last moment before the send, where a throw from + // here on is a genuine transport attempt. + storedPoolReplayDispatchNotifier( + providerFetch(route.provider, options.codexWsRuntimeIdentity, { + providerName: route.providerName, + modelId: route.modelId, + }), + codex401ReplayKind === "stored" ? options.onStoredPool401ReplayDispatched : undefined, + ), route.provider.authMode === "forward", ).then(response => { settleObservedHostResponse(); @@ -4005,10 +4024,6 @@ async function handleResponsesInner( } if (codex401ReplayKind !== null && upstreamResponse.status === 401) break; - // A stored Pool 401 owns one refresh and one same-account replay. The replay - // result is authoritative for this logical request and cannot enter a later - // account, model, or combo recovery ladder. - if (codex401ReplayKind === "stored" && upstreamResponse.status >= 400) break; // Native Responses providers return before the generic adapter recovery loop below. Keep // their OAuth contract identical: one pre-stream 401 forces a credential refresh and one @@ -4209,6 +4224,13 @@ async function handleResponsesInner( } if (poolRetryOutcome !== undefined) { + // A stored Pool 401 spent this request's account budget on its own refresh and replay, so + // nothing afterwards may be paid for out of a DIFFERENT account. A quota failure has no + // same-account move left, so it is terminal here; the gated-model 400 ladder does have + // one — retrying the account the refreshed roster still grants — and keeps it, with + // alternate resolution refused by `sameAccountOnly`. + const storedReplaySpent = codex401ReplayKind === "stored"; + if (storedReplaySpent && poolRetryOutcome !== 400) break; const retry = await retryCodexPoolOnAlternateAccount({ req, config, @@ -4219,6 +4241,7 @@ async function handleResponsesInner( firstAuthCtx: authCtx, firstResponse: upstreamResponse, outcomeStatus: poolRetryOutcome, + sameAccountOnly: storedReplaySpent, upstream, connectMs, passthroughEstimate, diff --git a/src/server/responses/fetch-helpers.ts b/src/server/responses/fetch-helpers.ts index 898275e6fc..48308ac679 100644 --- a/src/server/responses/fetch-helpers.ts +++ b/src/server/responses/fetch-helpers.ts @@ -108,6 +108,48 @@ export function providerFetch( +/** + * Wrap a provider fetch so `onDispatch` fires immediately before the send, not before pacing. + * + * `fetchWithHeaderTimeout` awaits `waitForPacing` and only then calls the executor, so a caller + * that signals at the call site records a dispatch even when a rejected pacing wait means nothing + * reached the network. That matters when the signal bounds later recovery: the request would lose + * its fallback on the strength of a send that never happened. + * + * The pacing surface is preserved deliberately. `waitForPacing` and `unpacedFetch` are read off + * the executor by `fetchWithHeaderTimeout`, so a plain function wrapper would silently drop + * provider pacing and double-send the slot. + */ +export function storedPoolReplayDispatchNotifier( + executor: ProviderFetch, + onDispatch: (() => void) | undefined, +): ProviderFetch { + if (!onDispatch) return executor; + let notified = false; + const notifyOnce = (): void => { + if (notified) return; + notified = true; + onDispatch(); + }; + const unpacedSource = executor.unpacedFetch ?? executor; + const unpaced = Object.assign( + (input: Parameters[0], init?: RequestInit) => { + notifyOnce(); + return unpacedSource(input, init); + }, + { preconnect: unpacedSource.preconnect }, + ) as ProviderFetch["unpacedFetch"]; + const wrapped = async (input: Parameters[0], init?: RequestInit) => { + await executor.waitForPacing?.(init?.signal ?? undefined); + return unpaced!(input, init); + }; + return Object.assign(wrapped, { + preconnect: executor.preconnect, + waitForPacing: executor.waitForPacing, + unpacedFetch: unpaced, + }) as ProviderFetch; +} + export async function fetchWithHeaderTimeout( url: string, init: Omit, diff --git a/tests/responses-fetch-helpers-boundary.test.ts b/tests/responses-fetch-helpers-boundary.test.ts index b4bc7de6bf..dde92a5a69 100644 --- a/tests/responses-fetch-helpers-boundary.test.ts +++ b/tests/responses-fetch-helpers-boundary.test.ts @@ -3,6 +3,7 @@ import { readFileSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { createScanner, LanguageVariant, SyntaxKind } from "typescript/unstable/ast"; +import { fetchWithHeaderTimeout, storedPoolReplayDispatchNotifier } from "../src/server/responses/fetch-helpers"; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const helperPath = resolve(repoRoot, "src/server/responses/fetch-helpers.ts"); @@ -76,3 +77,80 @@ describe("Responses fetch-helper import boundary", () => { ]); }); }); + +describe("storedPoolReplayDispatchNotifier", () => { + function pacedExecutor(options: { pacing: () => Promise }) { + const sends: string[] = []; + const unpaced = Object.assign( + async (input: Parameters[0]) => { + sends.push(String(input)); + return new Response("ok"); + }, + { preconnect: () => {} }, + ); + const wrapped = Object.assign( + async (input: Parameters[0], init?: RequestInit) => { + await options.pacing(); + return unpaced(input, init); + }, + { preconnect: () => {}, waitForPacing: options.pacing, unpacedFetch: unpaced }, + ); + return { wrapped, sends }; + } + + test("does not signal a dispatch when pacing admission rejects", async () => { + // The signal bounds later account/model/combo recovery, so it has to describe a send that + // actually happened. fetchWithHeaderTimeout awaits pacing BEFORE calling the executor, so a + // caller signalling at its own call site would spend the budget for a request that never + // reached the network. + let dispatched = 0; + const executor = pacedExecutor({ pacing: () => Promise.reject(new Error("pacing closed")) }); + const notifier = storedPoolReplayDispatchNotifier(executor.wrapped, () => { dispatched += 1; }); + + await expect(fetchWithHeaderTimeout( + "https://example.test/v1/responses", + { method: "POST" }, + new AbortController().signal, + 1_000, + false, + notifier, + )).rejects.toThrow("pacing closed"); + + expect(executor.sends).toEqual([]); + expect(dispatched).toBe(0); + }); + + test("signals exactly once, after pacing admission, and preserves pacing", async () => { + let dispatched = 0; + let paced = 0; + const order: string[] = []; + const executor = pacedExecutor({ + pacing: async () => { paced += 1; order.push("pacing"); }, + }); + const notifier = storedPoolReplayDispatchNotifier(executor.wrapped, () => { + dispatched += 1; + order.push("dispatch"); + }); + + const response = await fetchWithHeaderTimeout( + "https://example.test/v1/responses", + { method: "POST" }, + new AbortController().signal, + 1_000, + false, + notifier, + ); + + expect(response.status).toBe(200); + expect(dispatched).toBe(1); + // Pacing is still applied exactly once — a plain function wrapper would drop waitForPacing + // and unpacedFetch, which fetchWithHeaderTimeout reads off the executor. + expect(paced).toBe(1); + expect(order).toEqual(["pacing", "dispatch"]); + }); + + test("returns the executor untouched when there is nothing to notify", () => { + const executor = pacedExecutor({ pacing: async () => {} }); + expect(storedPoolReplayDispatchNotifier(executor.wrapped, undefined)).toBe(executor.wrapped); + }); +}); diff --git a/tests/responses-pool-401-refresh.test.ts b/tests/responses-pool-401-refresh.test.ts index a733953064..b17a4ebd4a 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -588,4 +588,154 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(harness.sends).toEqual(["Bearer externally-replaced"]); expect(isAccountNeedsReauth(ACCOUNT_ID)).toBe(false); }); + + // The recovery budget bounds which ACCOUNT may be charged, not whether the request may be + // rescued at all. Two ladders send to the account that was already paying, so a stored replay + // must not cut them: the one-shot opaque-blob rebuild, and the allow-listed gated-model 400 + // retry against a still-entitled account. A blanket "no sends after the replay" rule passes + // every test above and silently converts both into a user-visible 400. + test("a stored-account replay may still rebuild a rejected opaque blob on the same account", async () => { + const harness = installHarness({ + responseForSend: (authorization, sendNumber) => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization !== "Bearer refreshed-access") return undefined; + // First refreshed send still carries the stale blob; upstream names the exact code. + if (sendNumber === 2) { + return Response.json({ + error: { type: "invalid_request_error", code: "invalid_encrypted_content" }, + }, { status: 400 }); + } + // The rebuild stripped it, so the same refreshed account now succeeds. + return Response.json({ id: "resp_rebuilt", object: "response", status: "completed", output: [] }); + }, + }); + + const req = new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "gpt-5.5", + input: [{ type: "reasoning", encrypted_content: "stale-blob", summary: [] }], + }), + }); + const response = await handleResponses( + req, + config(), + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(200); + // Three sends, all on the SAME account: rejected bearer, refreshed replay, rebuilt resend. + expect(harness.sends).toEqual([ + "Bearer rejected-access", + "Bearer refreshed-access", + "Bearer refreshed-access", + ]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a stored-account replay 429 cannot reach another account even when one is eligible", async () => { + // The mirror of the case above: a quota failure has no same-account move left, so it is + // terminal. Asserted with a healthy alternate present, so passing means the budget stopped + // it rather than there being nowhere to go. + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const harness = installHarness({ + responseForSend: authorization => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "quota exhausted" } }, { status: 402 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const cfg = config({ secondAccount: true }); + cfg.accountPoolStrategy = "fill-first"; + const response = await handleResponses( + request("/v1/responses"), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(402); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); + + test("a gated-model 400 after a stored replay retries the same account, never an alternate", async () => { + // The narrow seam the budget leaves open, tested on the branch where it could leak. When the + // refreshed roster no longer grants the model, retryCodexPoolOnAlternateAccount would + // ordinarily resolve a DIFFERENT account; after a stored replay it must decline instead, or + // the 400 ladder becomes a way to spend the account budget twice. + writeStoredAccount({ + [OTHER_ACCOUNT_ID]: storedRecord({ + accessToken: "other-access", + refreshToken: "other-grant", + generation: 1, + chatgptAccountId: "acc-other", + }), + }); + const gatedModel = "gpt-5.6-sol"; + const harness = installHarness({ + responseForSend: authorization => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + // Exactly the allow-listed unsupported-model detail the 400 ladder recognises. + return Response.json({ + detail: `The '${gatedModel}' model is not supported when using Codex with a ChatGPT account.`, + }, { status: 400 }); + } + if (authorization === "Bearer other-access") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + return undefined; + }, + }); + + const cfg = config({ secondAccount: true }); + cfg.accountPoolStrategy = "fill-first"; + const response = await handleResponses( + request("/v1/responses", { model: gatedModel }), + cfg, + { model: "", provider: "" } as RequestLogContext, + { + // Both accounts are entitled on the FIRST resolution, so ordinary selection still picks + // the affined work account and the stored 401 happens. From the retry resolution onward + // only the other account is entitled, which declines the same-account retry and leaves + // the alternate-account branch as the one under test. + resolveCodexModelEntitlements: (() => { + let call = 0; + return async () => { + call += 1; + const accounts = call === 1 ? [ACCOUNT_ID, OTHER_ACCOUNT_ID] : [OTHER_ACCOUNT_ID]; + return { + modelsByAccount: new Map(accounts.map(id => [id, new Set([gatedModel])])), + confirmedAccountIds: new Set(accounts), + credentialIdentities: new Map(), + }; + }; + })(), + }, + ); + + // The 400 is surfaced rather than paid for out of the other account. + expect(response.status).toBe(400); + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + }); }); From 34cebb175265af5af2ba5b76476358f9b37f9c8c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 21:02:35 +0900 Subject: [PATCH 3/5] test(codex): pin the combo bound for a stored replay 4xx that is neither quota nor model-400 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contributor's original bound was a single `status >= 400` break in the passthrough loop, so it stopped combo fallback for every stored replay 4xx. Removing that break to keep same-account rescue alive left the outer layers relying on the dispatch signal, and no test covered the case the break used to cover but the pool-retry site does not: a plain 403 is not a quota status, so it never reaches the quota bound at all. It behaves correctly — the signal stops the combo — but that was unverified. Disabling the combo gate now turns this case red along with the three existing ones. --- tests/responses-pool-401-refresh.test.ts | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/responses-pool-401-refresh.test.ts b/tests/responses-pool-401-refresh.test.ts index b17a4ebd4a..aff6b4effd 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -738,4 +738,38 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(response.status).toBe(400); expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); }); + + test("a combo stops after a stored replay 4xx that is neither quota nor a gated-model 400", async () => { + // The contributor's original bound was a single `status >= 400` break in the passthrough loop, + // which stopped combo fallback for EVERY stored replay 4xx. Removing it to keep same-account + // rescue alive means the outer layers now rely on the dispatch signal instead. This pins that + // substitution on the case the break used to cover and the pool-retry site does not: a plain + // 403 is not a quota status, so it never reaches the quota bound at all. + const cfg = recoveryComboConfig(); + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (url.hostname === "backup.example") { + return Response.json({ id: "must-not-run", object: "response", status: "completed", output: [] }); + } + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization === "Bearer refreshed-access") { + return Response.json({ error: { message: "forbidden" } }, { status: 403 }); + } + return undefined; + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: "combo/recovery" }), + cfg, + { model: "", provider: "" } as RequestLogContext, + ); + + expect(response.status).toBe(403); + // The backup target is never sent to, and the account is charged exactly twice. + expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); }); From 8150388faa58ddba8ec44104888e45b9400d1a13 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 21:19:50 +0900 Subject: [PATCH 4/5] fix(codex): drop the redundant quota break and prove the dispatch timing end to end Second review round on my own correction. Three of my coverage claims were wrong. The quota-terminal break at the pool-retry site was redundant: removing it left all 17 pool tests green, because sameAccountOnly already returns no-alternate for exactly that case. Two mechanisms were doing one job and no test could tell them apart, so the break is gone and the comment now explains why one flag is sufficient. The pacing test was a helper unit test, and review showed it could not catch the defect it was written for -- restoring eager notification at the core call site left it green. It is now an integration test through handleResponses. Two details had to be right before it would bite: route.provider is a snapshot taken at routing time, so enabling pacing mid-flight does nothing and the module-level queue depth is what can change under a live request; and the request must not be a combo, because handleComboResponses installs its own dispatch callback for the child and swallows the caller's. It also asserts the signal directly rather than a fallback outcome, since a pacing overload is deliberately terminal above the combo layer. The gated-model test covers the refusal of an alternate, not the same-account retry its name claimed, and now says so. The notifier's exactly-once guard was unprotected while the test sent once; it now sends twice through one notifier. 198 pass / 0 fail across the ten affected suites; tsc clean; privacy:scan green. The mutation table in the devlog is corrected to the five that actually turn a test red. --- .../170_pr2895_pool_401_recovery_budget.md | 39 +++++++---- src/server/responses/core.ts | 11 ++-- .../responses-fetch-helpers-boundary.test.ts | 15 ++++- tests/responses-pool-401-refresh.test.ts | 65 ++++++++++++++++++- 4 files changed, 112 insertions(+), 18 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md index 43cfa49e33..d4e267d9ae 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md @@ -42,12 +42,17 @@ rebuild would have succeeded. A regression proves it: restoring that one line tu The corrected boundary is stated in terms of what is actually scarce — **another account's quota**, not further sends: -- a quota failure (429/402) after a stored replay has no same-account move left, so it stays - terminal at the pool-retry site; -- a gated-model 400 keeps its ladder, with `sameAccountOnly` refusing alternate-account resolution - inside `retryCodexPoolOnAlternateAccount`; +- a quota failure (429/402) after a stored replay has no same-account move left, so `sameAccountOnly` + makes it terminal by refusing the alternate; +- a gated-model 400 keeps its ladder, because it can retry the account the refreshed roster still + grants, and `sameAccountOnly` refuses only the alternate resolution; - opaque-blob recovery is untouched. +That is **one** mechanism, not two. An earlier revision of this fix also broke at the pool-retry +site on a non-400 outcome, and review showed no test could tell the difference: `sameAccountOnly` +already produced the identical result by returning `no-alternate`. The redundant break is gone +rather than kept as unjustifiable control flow. + `sameAccountOnly` is a new field on the retry args rather than a check at the call site, because the decision belongs where the alternate is resolved — the existing `fixedAccount` guard already lives on that line and means the same thing for a different reason. @@ -76,16 +81,28 @@ Named mutations, each turning its own test red: | Mutation | Test that fails | | --- | --- | | restore the broad `status >= 400` break | opaque blob rebuilt on the same account | -| remove the quota-terminal bound | both stored-replay-429 cases reach another account | | `sameAccountOnly: false` | gated-model 400 after a stored replay reaches an alternate | -| notify eagerly instead of at send time | pacing-rejected case signals a phantom dispatch | +| disable the combo dispatch gate | all four combo cases reach the backup target | +| notify eagerly at the core call site | replay stuck in the pacing queue signals a dispatch | +| drop the `notified` guard | one notifier signals twice across two sends | | drop `unpacedFetch` from the wrapper | pacing applied twice | -One process note worth keeping: the gated-model test was **vacuous on the first attempt**. The -injected entitlement resolver reported only the other account as entitled, so initial selection -picked that account and the stored 401 never happened — the assertion passed with one send and no -refresh. It now returns both accounts on the first resolution and only the other account from the -retry resolution onward, which is what actually reaches the alternate-account branch. +Three process notes, all from tests that looked fine and were not: + +1. The gated-model test was **vacuous on the first attempt**. The injected entitlement resolver + reported only the other account as entitled, so initial selection picked that account and the + stored 401 never happened — it passed with one send and no refresh. It now returns both accounts + on the first resolution and only the alternate from the retry resolution onward. Its name was + also wrong: it asserts the *refusal* of an alternate, not a same-account retry, and now says so. +2. The pacing test began as a **helper unit test only**, which review showed could not catch the + defect it was written for: restoring eager notification at the core call site left it green. It + is now an integration test through `handleResponses`, and two details had to be right for it to + bite at all — `route.provider` is a snapshot taken at routing time, so enabling pacing + mid-flight does nothing (the module-level queue depth is what changes under a live request), and + the request must not be a combo, because `handleComboResponses` installs its own dispatch + callback for the child and would swallow the caller's. +3. "Signals exactly once" was **not mutation-protected** while the test invoked the notifier once. + It now sends twice through one notifier. ## Not in this unit diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index d0db675e8e..ec88c2eb22 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -4225,12 +4225,13 @@ async function handleResponsesInner( if (poolRetryOutcome !== undefined) { // A stored Pool 401 spent this request's account budget on its own refresh and replay, so - // nothing afterwards may be paid for out of a DIFFERENT account. A quota failure has no - // same-account move left, so it is terminal here; the gated-model 400 ladder does have - // one — retrying the account the refreshed roster still grants — and keeps it, with - // alternate resolution refused by `sameAccountOnly`. + // nothing afterwards may be paid for out of a DIFFERENT account. One flag carries that, + // rather than a status check here as well: a quota failure has no same-account move, so + // `sameAccountOnly` makes it terminal by refusing the alternate; the gated-model 400 + // ladder does have one — retrying the account the refreshed roster still grants — and + // keeps it. An earlier revision also broke here on a non-400 outcome, which no test could + // justify because this flag already produced the identical result. const storedReplaySpent = codex401ReplayKind === "stored"; - if (storedReplaySpent && poolRetryOutcome !== 400) break; const retry = await retryCodexPoolOnAlternateAccount({ req, config, diff --git a/tests/responses-fetch-helpers-boundary.test.ts b/tests/responses-fetch-helpers-boundary.test.ts index dde92a5a69..fc4c2e2fb3 100644 --- a/tests/responses-fetch-helpers-boundary.test.ts +++ b/tests/responses-fetch-helpers-boundary.test.ts @@ -120,7 +120,7 @@ describe("storedPoolReplayDispatchNotifier", () => { expect(dispatched).toBe(0); }); - test("signals exactly once, after pacing admission, and preserves pacing", async () => { + test("signals after pacing admission, once per notifier, and preserves pacing", async () => { let dispatched = 0; let paced = 0; const order: string[] = []; @@ -147,6 +147,19 @@ describe("storedPoolReplayDispatchNotifier", () => { // and unpacedFetch, which fetchWithHeaderTimeout reads off the executor. expect(paced).toBe(1); expect(order).toEqual(["pacing", "dispatch"]); + + // A second send through the SAME notifier must not signal again. One replay is one dispatch, + // and without the internal guard a retry inside the helper would report two. + await fetchWithHeaderTimeout( + "https://example.test/v1/responses", + { method: "POST" }, + new AbortController().signal, + 1_000, + false, + notifier, + ); + expect(dispatched).toBe(1); + expect(paced).toBe(2); }); test("returns the executor untouched when there is nothing to notify", () => { diff --git a/tests/responses-pool-401-refresh.test.ts b/tests/responses-pool-401-refresh.test.ts index aff6b4effd..867daf413d 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -11,6 +11,11 @@ import { } from "../src/codex/routing"; import { handleResponses, handleResponsesCompact } from "../src/server/responses"; import { clearCompactHandoffRoutesForTests } from "../src/server/responses/compact"; +import { + REQUEST_PACING_MAX_QUEUE_DEPTH, + resetProviderRequestPacingForTest, + setProviderRequestPacingLimitsForTest, +} from "../src/providers/request-pacing"; import type { RequestLogContext } from "../src/server/request-log"; import type { OcxConfig } from "../src/types"; @@ -676,11 +681,16 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(harness.refreshes).toEqual(["refresh-grant"]); }); - test("a gated-model 400 after a stored replay retries the same account, never an alternate", async () => { + test("a gated-model 400 after a stored replay refuses an alternate account", async () => { // The narrow seam the budget leaves open, tested on the branch where it could leak. When the // refreshed roster no longer grants the model, retryCodexPoolOnAlternateAccount would // ordinarily resolve a DIFFERENT account; after a stored replay it must decline instead, or // the 400 ladder becomes a way to spend the account budget twice. + // + // This covers the REFUSAL only. The same-account rescue that the ladder still allows is a + // different branch (retryAuthCtx = firstAuthCtx, taken when the refreshed roster still grants + // the model) and is covered by the opaque-blob case above, which is the ladder this fix was + // actually reported to have broken. writeStoredAccount({ [OTHER_ACCOUNT_ID]: storedRecord({ accessToken: "other-access", @@ -772,4 +782,57 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(harness.sends).toEqual(["Bearer rejected-access", "Bearer refreshed-access"]); expect(harness.refreshes).toEqual(["refresh-grant"]); }); + + test("a replay that never leaves the pacing queue does not spend the recovery budget", async () => { + // The dispatch signal is what bounds combo and policy fallback, so it has to describe a send + // that actually happened. fetchWithHeaderTimeout awaits pacing admission BEFORE calling the + // executor, so signalling at the call site would spend the budget for a replay that never + // reached the network — the request would lose its fallback for nothing. This drives the real + // path: pacing is enabled with a zero-depth queue, so the replay's admission is rejected. + // + // A plain request, deliberately NOT a combo: handleComboResponses installs its own + // onStoredPool401ReplayDispatched for the child, which replaces the caller's and would make + // the signal unobservable from here. + const cfg = config(); + // Pacing must be enabled BEFORE routing: `route.provider` is a snapshot taken at routing + // time, so enabling it mid-flight cannot affect the replay. The queue DEPTH limit, by + // contrast, is a module-level value read on every admission, which is what lets the first + // send through and rejects only the replay. + cfg.providers.openai!.requestPacing = { enabled: true, minIntervalMs: 60_000 }; + const harness = installHarness({ + responseForSend: (authorization, _sendNumber, url) => { + if (authorization === "Bearer rejected-access") { + // Close the admission queue only once the original send is through, so the rejection + // lands on the replay rather than on the request that produces the 401. + setProviderRequestPacingLimitsForTest({ maxQueueDepth: 0 }); + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + return undefined; + }, + }); + + let dispatchSignals = 0; + let response: Response; + try { + response = await handleResponses( + request("/v1/responses"), + cfg, + { model: "", provider: "" } as RequestLogContext, + { onStoredPool401ReplayDispatched: () => { dispatchSignals += 1; } }, + ); + } finally { + setProviderRequestPacingLimitsForTest({ maxQueueDepth: REQUEST_PACING_MAX_QUEUE_DEPTH }); + resetProviderRequestPacingForTest(); + } + + // The replay never reached the network, so the budget must not be reported as spent. Asserted + // on the signal itself rather than on a fallback outcome: a pacing overload is deliberately + // terminal (it propagates as an error and becomes a 429 above the combo layer), so the + // downstream fallback is unreachable here for a reason that has nothing to do with this fix. + expect(dispatchSignals).toBe(0); + expect(harness.sends.filter(send => send === "Bearer refreshed-access")).toEqual([]); + // The refresh did happen — this is the post-refresh replay being rejected, not an earlier stop. + expect(harness.refreshes).toEqual(["refresh-grant"]); + expect(response.status).toBe(429); + }); }); From a7fc45113c972193625b677aa34a1f9d02ddac2f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 21:33:41 +0900 Subject: [PATCH 5/5] test(codex): cover the permitted same-account gated-model retry directly CodeRabbit's remaining point: the ladder the budget deliberately leaves open was only covered by implication, through the opaque-blob case. The branch where refreshed entitlements still grant the original account -- retryAuthCtx = firstAuthCtx, a third send on the account already paying -- had no test of its own. Restoring the broad status >= 400 break now turns two tests red instead of one, which is the pair of same-account ladders that break was cutting. --- tests/responses-pool-401-refresh.test.ts | 47 ++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/responses-pool-401-refresh.test.ts b/tests/responses-pool-401-refresh.test.ts index 867daf413d..dbac95851a 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -835,4 +835,51 @@ describe("ordinary pool 401 refresh and replay (#2887)", () => { expect(harness.refreshes).toEqual(["refresh-grant"]); expect(response.status).toBe(429); }); + + test("a gated-model 400 after a stored replay still retries the SAME account when entitled", async () => { + // The branch the budget deliberately leaves open, asserted directly rather than by implication + // from the opaque-blob case. When the refreshed roster still grants the model, + // retryCodexPoolOnAlternateAccount sets retryAuthCtx = firstAuthCtx and sends again to the + // account already paying — no other account is charged, so it is outside the budget. + const gatedModel = "gpt-5.6-sol"; + const harness = installHarness({ + responseForSend: (authorization, sendNumber) => { + if (authorization === "Bearer rejected-access") { + return Response.json({ error: { message: "rejected bearer" } }, { status: 401 }); + } + if (authorization !== "Bearer refreshed-access") return undefined; + if (sendNumber === 2) { + return Response.json({ + detail: `The '${gatedModel}' model is not supported when using Codex with a ChatGPT account.`, + }, { status: 400 }); + } + // Upstream shards can briefly disagree during a gated-model rollout, so the same account + // succeeds on the retry. + return Response.json({ id: "resp_same_account", object: "response", status: "completed", output: [] }); + }, + }); + + const response = await handleResponses( + request("/v1/responses", { model: gatedModel }), + config(), + { model: "", provider: "" } as RequestLogContext, + { + // The single configured account stays entitled across both resolutions. + resolveCodexModelEntitlements: async () => ({ + modelsByAccount: new Map([[ACCOUNT_ID, new Set([gatedModel])]]), + confirmedAccountIds: new Set([ACCOUNT_ID]), + credentialIdentities: new Map(), + }), + }, + ); + + expect(response.status).toBe(200); + // Three sends, every one of them on the same account. + expect(harness.sends).toEqual([ + "Bearer rejected-access", + "Bearer refreshed-access", + "Bearer refreshed-access", + ]); + expect(harness.refreshes).toEqual(["refresh-grant"]); + }); });