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..d4e267d9ae --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/170_pr2895_pool_401_recovery_budget.md @@ -0,0 +1,111 @@ +# 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 `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. + +## 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 | +| `sameAccountOnly: false` | gated-model 400 after a stored replay reaches an alternate | +| 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 | + +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 + +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/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..ec88c2eb22 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, @@ -1433,6 +1444,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 +2302,7 @@ export async function handleComboResponses( attemptRetained = true; }; let consumedChildFailure: ConsumedComboFailure | undefined; + let storedPool401ReplayDispatched = false; const callbackGate = createChildPassthroughCallbackGate(options); let response: Response; try { @@ -2315,6 +2329,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 +2435,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 +3858,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 +3933,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 @@ -3978,10 +3997,19 @@ async function handleResponsesInner( 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(); @@ -3995,7 +4023,7 @@ async function handleResponsesInner( continue passthroughRecovery; } - if (codexMain401ReplayAttempted && upstreamResponse.status === 401) break; + if (codex401ReplayKind !== null && upstreamResponse.status === 401) 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 @@ -4196,6 +4224,14 @@ 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. 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"; const retry = await retryCodexPoolOnAlternateAccount({ req, config, @@ -4206,6 +4242,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/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-fetch-helpers-boundary.test.ts b/tests/responses-fetch-helpers-boundary.test.ts index b4bc7de6bf..fc4c2e2fb3 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,93 @@ 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 after pacing admission, once per notifier, 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"]); + + // 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", () => { + const executor = pacedExecutor({ pacing: async () => {} }); + expect(storedPoolReplayDispatchNotifier(executor.wrapped, undefined)).toBe(executor.wrapped); + }); +}); 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..dbac95851a 100644 --- a/tests/responses-pool-401-refresh.test.ts +++ b/tests/responses-pool-401-refresh.test.ts @@ -10,6 +10,12 @@ import { resolveCodexAccountForThreadDetailed, } 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"; @@ -57,19 +63,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 +122,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 +144,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 +154,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 +181,7 @@ beforeEach(() => { process.env.OPENCODEX_HOME = home; process.env.CODEX_HOME = home; clearAccountNeedsReauth(ACCOUNT_ID); + clearAccountNeedsReauth(OTHER_ACCOUNT_ID); clearCodexUpstreamHealth(); clearThreadAccountMap(); writeStoredAccount(); @@ -159,7 +189,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 +233,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 @@ -352,4 +593,293 @@ 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 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", + 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"]); + }); + + 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"]); + }); + + 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); + }); + + 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"]); + }); }); 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;