From d18e68436c222646a40e43c0ae1a17d2db258430 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 22:43:54 +0900 Subject: [PATCH 1/3] fix(lib): make the dispatch permit the charge, and close the uncounted send paths (#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify. --- scripts/test-layout/layout.json | 1 + src/lib/request-execution-budget.ts | 89 ++++-- src/lib/upstream-retry.ts | 32 ++- src/server/responses/compact.ts | 15 +- src/server/responses/core.ts | 265 ++++++++++++++---- tests/fixtures/test-layout-expected.json | 1 + tests/lib/request-execution-budget.test.ts | 167 +++++++++++ .../lib/transient-budget-scope-source.test.ts | 72 +++++ 8 files changed, 550 insertions(+), 92 deletions(-) create mode 100644 tests/lib/request-execution-budget.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index a731a90c41..00f10bc494 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1132,6 +1132,7 @@ "repo-hygiene.test.ts": "ci-workflows", "request-decompress.test.ts": "usage", "request-evidence.test.ts": "usage", + "request-execution-budget.test.ts": "lib", "request-history-index.test.ts": "usage", "request-log-conversation.test.ts": "usage", "request-log-estimate-cap.test.ts": "usage", diff --git a/src/lib/request-execution-budget.ts b/src/lib/request-execution-budget.ts index e7581c4605..80654b0a94 100644 --- a/src/lib/request-execution-budget.ts +++ b/src/lib/request-execution-budget.ts @@ -72,17 +72,28 @@ export interface DispatchIntent { readonly replaySafe?: boolean; /** * True when the physical send is already reported through another counter -- the retry - * helpers' `onSendsConsumed` hook. The permit then books the reserve, alternate-target and - * transition ledgers but leaves `used` to that reporter, because charging both is how a - * four-send cap silently becomes a two-send cap. + * helpers' `onSendsConsumed` hook. The send is still booked at reservation time, because an + * advisory reservation cannot stop a concurrent leg; what changes is that the booking is + * PENDING, and the first send the external reporter names settles it instead of adding a + * second charge. Charging both is how a four-send cap silently becomes a two-send cap. */ readonly countedExternally?: boolean; } export interface SingleUseDispatchPermit { readonly sendClass: SendClass; - /** Consume exactly once. A second call returns false and charges nothing. */ + /** + * Confirm the dispatch this permit already paid for. The reservation is the charge, so this + * charges nothing; it is how a leg proves it is the one that sent. A second call returns + * false, which is what keeps a retry thunk from sending twice on one permit. + */ use(): boolean; + /** + * Hand back a reservation that never dispatched -- a credential move that found no alternate, + * a rebuild abandoned before the send. Idempotent, and a no-op once the permit was used or + * once an external send reporter already settled it. + */ + release(): void; } export type DispatchDecision = @@ -103,6 +114,9 @@ export interface RequestExecutionBudget extends TransientSendBudget { * Sends still available from the base allowance, capped by a layer's own maximum. * Returns 0 when the allowance is gone -- it never floors to 1, because a floor of 1 is * what let every recovery leg send one more time forever. + * + * A reserved-but-unconfirmed send is spent for this purpose. The alternative -- counting only + * confirmed sends -- is what let two legs read the same remainder and both dispatch. */ remainingBaseSends(cap: number): number; readonly reserveSpent: boolean; @@ -124,13 +138,30 @@ export function createRequestExecutionBudget( policy: RequestExecutionBudgetPolicy = CODEX_TEXT_GUARDED_BUDGET_POLICY, logicalRequestId?: string, ): RequestExecutionBudget { + let spent = 0; + // Reservations whose physical send is reported by a retry helper rather than by the permit. + // They are already charged; the reporter's first send settles one instead of charging again. + let pendingExternalSends = 0; let reserveSpent = false; let alternateTargetSends = 0; let targetTransitions = 0; let lastTargetKey: string | undefined; const budget: RequestExecutionBudget = { - used: 0, + get used(): number { return spent; }, + set used(next: number) { + // The retry helpers report their real send count by assigning through this field. A + // reservation taken with `countedExternally` has already booked one of those sends, so + // the report settles the pending booking first and only the surplus is charged. + const delta = next - spent; + if (delta <= 0) { + spent = Math.max(0, next); + return; + } + const settled = Math.min(delta, pendingExternalSends); + pendingExternalSends -= settled; + spent += delta - settled; + }, logicalRequestId: logicalRequestId ?? `lr-${Date.now().toString(36)}-${(logicalRequestSeq += 1).toString(36)}`, policyVersion: REQUEST_BUDGET_POLICY_VERSION, policy, @@ -140,11 +171,11 @@ export function createRequestExecutionBudget( get lastTargetKey() { return lastTargetKey; }, remainingBaseSends(cap: number): number { const capped = Number.isFinite(cap) ? Math.trunc(cap) : 0; - return Math.max(0, Math.min(capped, policy.baseSendAllowance - budget.used)); + return Math.max(0, Math.min(capped, policy.baseSendAllowance - spent)); }, reserveDispatch(intent: DispatchIntent): DispatchDecision { if (intent.replaySafe === false) return { allowed: false, reason: "not-replay-safe" }; - if (budget.used >= policy.maxTotalModelSends) return { allowed: false, reason: "total-exhausted" }; + if (spent >= policy.maxTotalModelSends) return { allowed: false, reason: "total-exhausted" }; const changesTarget = lastTargetKey !== undefined && lastTargetKey !== intent.targetKey; const isAlternateTarget = changesTarget || intent.sendClass === "account-failover" @@ -159,7 +190,7 @@ export function createRequestExecutionBudget( // The base allowance is spent first. Only once it is gone does a recovery class reach // for the single shared reserve -- an account move and a validated rebuild cannot each // take one. - const drawsReserve = budget.remainingBaseSends(policy.baseSendAllowance) === 0; + const drawsReserve = policy.baseSendAllowance - spent <= 0; if (drawsReserve) { if (!RESERVE_FUNDED_CLASSES.has(intent.sendClass)) { return { allowed: false, reason: "base-allowance-exhausted" }; @@ -169,29 +200,47 @@ export function createRequestExecutionBudget( } } - let consumed = false; + // THE RESERVATION IS THE CHARGE. Deciding here and charging in `use()` left a window in + // which two legs read the same remainder, both received a permit, and both dispatched: + // one remaining send admitted two physical sends, which is the per-request multiplication + // this budget exists to stop. Everything is booked now; `release()` is the way back. + const previousTargetKey = lastTargetKey; + spent += 1; + if (intent.countedExternally === true) pendingExternalSends += 1; + if (drawsReserve) reserveSpent = true; + if (isAlternateTarget) alternateTargetSends += 1; + if (changesTarget) targetTransitions += 1; + lastTargetKey = intent.targetKey; + + let settled: "open" | "used" | "released" = "open"; return { allowed: true, permit: { sendClass: intent.sendClass, use(): boolean { - if (consumed) return false; - consumed = true; - // Charged here, immediately before the physical send, rather than reported after - // the helper returns: a counter that is only reconciled afterwards cannot stop two - // concurrent legs that both read the same remainder. - if (intent.countedExternally !== true) budget.used += 1; - if (drawsReserve) reserveSpent = true; - if (isAlternateTarget) alternateTargetSends += 1; - if (changesTarget) targetTransitions += 1; - lastTargetKey = intent.targetKey; + if (settled !== "open") return false; + settled = "used"; return true; }, + release(): void { + if (settled !== "open") return; + settled = "released"; + // An externally counted reservation the reporter already settled paid for a send + // that physically happened. Refunding it would hand the request a free send back. + if (intent.countedExternally === true) { + if (pendingExternalSends === 0) return; + pendingExternalSends -= 1; + } + spent -= 1; + if (drawsReserve) reserveSpent = false; + if (isAlternateTarget) alternateTargetSends -= 1; + if (changesTarget) targetTransitions -= 1; + lastTargetKey = previousTargetKey; + }, }, }; }, }; - if (lastTargetKey === undefined) lastTargetKey = undefined; return budget; } diff --git a/src/lib/upstream-retry.ts b/src/lib/upstream-retry.ts index aba90a96b2..3a4fb619a3 100644 --- a/src/lib/upstream-retry.ts +++ b/src/lib/upstream-retry.ts @@ -357,17 +357,22 @@ export interface ResetRetryOptions { label?: string; /** Total upstream sends allowed, including the first one. Not a per-layer retry count. */ attempts?: number; -} - -export interface TransientRetryOptions extends ResetRetryOptions { - /** Test seam: per-attempt slow budget override (defaults to TRANSIENT_RETRY_SLOW_ATTEMPT_MS). */ - slowAttemptMs?: number; /** * Reports how many upstream sends this call actually consumed, so a caller that spans * several legs of one request (initial send, then a 429/account-recovery refetch) can * keep them on ONE budget instead of handing each leg a fresh one. + * + * It lives on the RESET options, not on the transient ones, because every leg that falls + * back to reset-only retry -- the non-policy adapter initial send, and every + * `rebuildAndRefetch` recovery kind whose provider has no transient policy -- was not merely + * uncounted but UNCOUNTABLE: the callback existed on a type those call sites never reach. */ onSendsConsumed?: (sends: number) => void; +} + +export interface TransientRetryOptions extends ResetRetryOptions { + /** Test seam: per-attempt slow budget override (defaults to TRANSIENT_RETRY_SLOW_ATTEMPT_MS). */ + slowAttemptMs?: number; /** * How long this caller can wait on an honoured `Retry-After`, defaulting to * {@link RETRY_AFTER_CEILING_MS}. It is a deadline, never a clamp: an instruction inside it @@ -455,6 +460,10 @@ export async function fetchWithResetRetry( let sawReset = false; for (let attempt = 0; attempt < attempts; attempt++) { if (opts.abortSignal?.aborted) throw abortError(opts.abortSignal); + // Reported before the await, one physical send at a time: a send that rejects has still + // been made, and this helper leaves through four exits (return, reset give-up, non-reset + // rethrow, abort), so a per-send report is the only shape that is correct on all of them. + opts.onSendsConsumed?.(1); try { return await doFetch(attempt === 0 ? firstRecovery : "connection-reset"); } catch (err) { @@ -517,13 +526,22 @@ export async function fetchWithTransientRetry( // more send -- the loop condition alone was never enough, because every later recovery leg // called this helper again and the floor funded each of them. const remaining = () => Math.max(0, budget - sent); + // The inner reset layer now has its own `onSendsConsumed`, and these are the same physical + // sends `countedFetch` already counts. Forwarding the reporter down the `remaining()` path + // would report each of them twice, which is how a four-send cap becomes a two-send cap. One + // send is counted once, by the outermost layer that owns the budget. + const innerResetOptions = (): ResetRetryOptions => ({ + ...opts, + attempts: remaining(), + onSendsConsumed: undefined, + }); // Reported in `finally` rather than at each exit: this function returns from five places // and throws from one, and a caller sharing the budget across request legs must be told the // real count on every one of them. try { if (budget === 0) throw new SendBudgetExhaustedError(opts.label); let attemptStart = Date.now(); - let res = await fetchWithResetRetry(countedFetch, { ...opts, attempts: remaining() }); + let res = await fetchWithResetRetry(countedFetch, innerResetOptions()); for (let attempt = 0; sent < budget; attempt++) { // A non-replayable gateway status was settled after the request body had already left // for the origin; retrying it here is the automatic resend the marker exists to forbid. @@ -561,7 +579,7 @@ export async function fetchWithTransientRetry( attemptStart = Date.now(); transientStatuses.push(res.status); try { - res = await fetchWithResetRetry(countedFetch, { ...opts, attempts: remaining() }, "transient-5xx"); + res = await fetchWithResetRetry(countedFetch, innerResetOptions(), "transient-5xx"); } catch (err) { // Keep the prior 5xx evidence attached: the origin already responded, so // this rejection is not pre-connection and must not classify as neutral. diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 02060fb000..65bfd1c82b 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -682,6 +682,13 @@ export async function handleResponsesCompact( // Combo-resolved targets skip native compact so failover can advance through the // combo target list when the picked model returns 429/5xx — the routed path below // dispatches through handleResponses → handleComboResponses with full failover. + // + // One holder for the WHOLE logical compact, declared above the native branch because the + // routed fallback below is not a different request: a native attempt that 404s, or a quota + // failure that hands off, continues here. The routed turn used to call handleResponses with + // no budget at all, so `handleResponsesInner` minted a fresh four after the native attempt + // had already spent some of the first one. + const sendBudget: RequestExecutionBudget = options.sendBudget ?? createRequestExecutionBudget(); if (supportsNativeResponsesCompactEndpoint(route.providerName, route.provider) && !accountGatedCompactWireModel && !route.combo) { if (req.signal.aborted) { return formatErrorResponse(499, "client_cancelled", "Client cancelled compact request"); @@ -774,9 +781,6 @@ export async function handleResponsesCompact( // so routed-model reasoning items (reasoning_text content) don't 400 the ChatGPT backend. const compactBody = sanitizeReasoningInputContent(compactBodyRaw) as typeof compactBodyRaw; const compactUrl = `${base}/responses/compact`; - // One holder for this logical compact, inherited by the handoff child so a second model - // does not start over with a fresh four. - const sendBudget: RequestExecutionBudget = options.sendBudget ?? createRequestExecutionBudget(); const compactTargetKey = `${route.providerName}|${route.modelId}|compact`; const actualCompactHostKey = upstreamHostHealthKey( route.providerName, @@ -1223,7 +1227,10 @@ export async function handleResponsesCompact( body: JSON.stringify(internalBody), }); linkRequestSessionLane(req, internalReq); - const response = await handleResponses(internalReq, config, logCtx, { abortSignal: req.signal, turnAdmissionLease, ...(admission ? { admission } : {}) }); + // The routed compaction turn is a handoff inside the same logical request, so it draws the + // REMAINDER. Minting here is what let a native attempt spend three sends and the routed + // fallback spend four more. + const response = await handleResponses(internalReq, config, logCtx, { abortSignal: req.signal, turnAdmissionLease, sendBudget, ...(admission ? { admission } : {}) }); if (!response.ok) return response; let json: { output?: unknown[]; status?: unknown; error?: unknown }; if (response.headers.get("content-type")?.includes("text/event-stream")) { diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 57c808401f..a7c248ea69 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -1540,6 +1540,8 @@ async function retryCodexPoolOnAlternateAccount( && !(error instanceof CodexAccountCooldownError) && !(error instanceof CodexMainProfileDrainingError); if (unexpectedRetryError) { + // The reservation is the charge now, so an abandoned move has to hand its send back. + accountMovePermit?.release(); await firstResponse.body?.cancel().catch(() => undefined); releaseCodexAuthContextProbeLease(firstAuthCtx); throw error; @@ -1566,6 +1568,8 @@ async function retryCodexPoolOnAlternateAccount( writerGeneration: firstAuthCtx.writerGeneration, }); } + // No usable alternate was resolved, so the reserved move never becomes a send. + accountMovePermit?.release(); recordUnmovedTransientOutcome(); return { kind: "no-alternate" }; } @@ -1651,7 +1655,21 @@ async function retryCodexPoolOnAlternateAccount( // seven additional same-account sends (eight total including the original), re-checking the // exact allow-listed body and fresh entitlement before every later send. Alternate-account and // quota recovery retain their historical one-send bound. - const maxRetrySends = retrySameConfirmedAccount ? 7 : 1; + // + // Two different bounds, and the effective one is the smaller. `maxRetrySends` answers "how + // many times is it worth re-asking THIS account for a model its roster still grants"; the + // shared budget answers "how many times may this LOGICAL REQUEST reach upstream in total, + // across every layer that can re-send". A ladder of eight layered on sends the request had + // already made is exactly the per-request multiplication #4546 is about, so the ladder is + // capped at what the request has left. The floor of one keeps the single retry this function + // was called to make -- the move already paid for itself with its own permit -- and each rung + // past the first reserves its own send below, so a refusal stops the ladder with the last + // upstream answer intact. + const ladderTargetKey = `${route.providerName}|${route.modelId}|${retryAuthCtx.accountId}`; + const sharedSendsLeft = executionBudget + ? Math.max(1, executionBudget.policy.maxTotalModelSends - executionBudget.used) + : Number.POSITIVE_INFINITY; + const maxRetrySends = Math.min(retrySameConfirmedAccount ? 7 : 1, sharedSendsLeft); let retrySendCount = 0; let upstreamResponse: Response; try { @@ -1723,6 +1741,18 @@ async function retryCodexPoolOnAlternateAccount( throw error; } if (!entitledCodexAccountIdsForModel(refreshed, route.modelId)?.has(retryAuthCtx.accountId)) break; + // The next rung is another physical send of this logical request: a same-account, + // same-target replay, charged as an ordinary transient send rather than as a move. + // Reserved here, immediately before looping back, so a refusal stops the ladder with the + // last upstream 400 intact instead of spending a send it cannot make. + if (executionBudget) { + const rung = executionBudget.reserveDispatch({ + sendClass: "transient", + targetKey: ladderTargetKey, + }); + if (!rung.allowed || !rung.permit.use()) break; + chargeWorkflowSends(args.options.workflowRootId, 1); + } await upstreamResponse.body?.cancel().catch(() => undefined); } } finally { @@ -5069,6 +5099,14 @@ async function handleResponsesInner( const adapterSendBudget = isRequestExecutionBudget(sendBudget) ? sendBudget : undefined; const sendBudgetExhausted = (): boolean => remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS) === 0; + /** + * A credential hop reserves the send its own replay will make, and that replay is a recovery + * leg. The leg must SPEND the hop's reservation instead of taking a second one: the + * final-recovery reserve is single, so a rebuild that reserved on top of a hop would be + * refused and the request would answer with a synthetic 502 in place of the real 429 the hop + * was recovering from. + */ + let pendingHopPermit: SingleUseDispatchPermit | undefined; /** * How many sends a recovery leg may make, and the permit that authorises the last one. * @@ -5085,10 +5123,39 @@ async function handleResponsesInner( ): { attempts: number; permit?: SingleUseDispatchPermit } => { const base = remainingTransientSendBudget(cap); if (base > 0) return { attempts: base }; + if (pendingHopPermit) { + const hopPermit = pendingHopPermit; + pendingHopPermit = undefined; + return { attempts: 1, permit: hopPermit }; + } if (!isRequestExecutionBudget(sendBudget)) return { attempts: 0 }; const decision = sendBudget.reserveDispatch({ sendClass, targetKey, countedExternally: true }); return decision.allowed ? { attempts: 1, permit: decision.permit } : { attempts: 0 }; }; + /** + * One credential hop of this logical request, admitted by the INTERSECTION of two bounds. + * + * `GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST` and `ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST` + * stay exactly as they are: they bound rotation within one credential roster. What neither + * can see is everything else this request already sent, so three hops layered on a spent + * budget still reached upstream three more times. A hop now happens only when its own layer + * cap AND the shared budget both permit it, and the smaller of the two wins. + * + * `countedExternally` is for the hops whose replay goes out through the retry helper, which + * reports the same physical send through `onSendsConsumed`; the others are charged here and + * nowhere else. A refusal is not an error: the caller keeps the real upstream response -- + * status, `Retry-After`, quota body -- because return-the-last-answer is the exhaustion + * contract this unit settled on. + */ + const reserveCredentialHop = ( + sendClass: SendClass, + targetKey: string, + countedExternally = false, + ): { allowed: boolean; permit?: SingleUseDispatchPermit } => { + if (!isRequestExecutionBudget(sendBudget)) return { allowed: true }; + const decision = sendBudget.reserveDispatch({ sendClass, targetKey, countedExternally }); + return decision.allowed ? { allowed: true, permit: decision.permit } : { allowed: false }; + }; /** * Both classes share the one reserve, so this only changes what the decision is called -- * but a recovery event that says "repair" when a credential refresh drove it is the kind of @@ -5708,15 +5775,16 @@ async function handleResponsesInner( recordAttemptCredentialSource(logCtx.activeAttempt, route.providerName, route.provider, retryAdapter.name); const rebuiltBodyRefusal = refuseOversizedOutboundBody(request); if (rebuiltBodyRefusal) return { failed: rebuiltBodyRefusal }; + // The base allowance is spent first; once it is gone this leg may still draw the one + // shared final-recovery reserve, which is what keeps a validated sanitized rebuild + // after a 5xx streak alive at four total sends instead of dying at three. Reserved + // outside the try so the finally can hand it back if the leg never reached its send. + const allowance = recoverySendAllowance( + TRANSIENT_RETRY_MAX_ATTEMPTS, + recoveryClassFor(recovery), + `${route.providerName}|${route.modelId}|${recovery}`, + ); try { - // The base allowance is spent first; once it is gone this leg may still draw the one - // shared final-recovery reserve, which is what keeps a validated sanitized rebuild - // after a 5xx streak alive at four total sends instead of dying at three. - const allowance = recoverySendAllowance( - TRANSIENT_RETRY_MAX_ATTEMPTS, - recoveryClassFor(recovery), - `${route.providerName}|${route.modelId}|${recovery}`, - ); return await fetchWithTransientRetry( innerRecovery => { // Gated on the return, not fire-and-forget: a consumed permit means this leg @@ -5746,6 +5814,9 @@ async function handleResponsesInner( } catch (err) { return { failed: transportFailureResponse(err) }; } finally { + // A no-op once the permit was used or once onSendsConsumed settled it; it only refunds + // a reservation whose send never happened. + allowance.permit?.release(); request.releaseBodyObservation?.(); } }; @@ -5982,29 +6053,45 @@ async function handleResponsesInner( && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST && isGenericOAuthFailoverEnabled(config, route.providerName) ) { - const nextAccountId = rotateGenericOAuthAccountOn429( - config, route.providerName, genericFailoverAccountId, - upstreamResponse.headers.get("retry-after"), + // The roster cap above is one half of the bound; the request's shared budget is the + // other. A refused hop leaves the real 429 -- body, Retry-After and any quota evidence + // -- exactly as upstream sent it. + const hop = reserveCredentialHop( + "account-failover", + `${route.providerName}|${route.modelId}|oauth-account-429`, + true, ); - let snapshot: OAuthAccessSnapshot | undefined; - if (nextAccountId) { - try { snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); } - catch { /* Keep the original 429 body readable when the next credential is unavailable. */ } - } - if (snapshot && await applyFailoverSnapshot(snapshot)) { - genericFailovers += 1; - route.provider = resolveProviderTransport( - route.providerName, route.provider, parsed.options.promptCacheKey, sentOAuthSnapshot?.apiBaseUrl, + if (hop.allowed) { + const nextAccountId = rotateGenericOAuthAccountOn429( + config, route.providerName, genericFailoverAccountId, + upstreamResponse.headers.get("retry-after"), ); - bindRouteReasoningReplayScope({ - parsed, providerName: route.providerName, provider: route.provider, - adapterName: "openai-responses", oauthCredentialSnapshot: replayOAuthCredentialSnapshot, - }); - try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already closed */ } - const result = await rebuildAndRefetch("oauth-account-429"); - if ("failed" in result) return result.failed; - upstreamResponse = result; - continue passthroughRecovery; + let snapshot: OAuthAccessSnapshot | undefined; + if (nextAccountId) { + try { snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); } + catch { /* Keep the original 429 body readable when the next credential is unavailable. */ } + } + if (snapshot && await applyFailoverSnapshot(snapshot)) { + genericFailovers += 1; + route.provider = resolveProviderTransport( + route.providerName, route.provider, parsed.options.promptCacheKey, sentOAuthSnapshot?.apiBaseUrl, + ); + bindRouteReasoningReplayScope({ + parsed, providerName: route.providerName, provider: route.provider, + adapterName: "openai-responses", oauthCredentialSnapshot: replayOAuthCredentialSnapshot, + }); + try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already closed */ } + // The replay IS this hop's send, so the rebuild spends the reservation instead of + // asking for one of its own. + pendingHopPermit = hop.permit; + const result = await rebuildAndRefetch("oauth-account-429"); + pendingHopPermit = undefined; + if ("failed" in result) return result.failed; + upstreamResponse = result; + continue passthroughRecovery; + } + // No credential moved, so the reservation costs nothing. + hop.permit?.release(); } } @@ -7035,20 +7122,36 @@ async function handleResponsesInner( && genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST && isGenericOAuthFailoverEnabled(config, route.providerName) ) { + // Intersection with the request's shared budget. The sidecar replay is dispatched by the + // web-search/image loop and never reaches `onSendsConsumed`, so this reservation is the + // charge; a refusal returns null and the caller keeps the real 429 it already has. + const hop = reserveCredentialHop( + "account-failover", + `${route.providerName}|${route.modelId}|sidecar-oauth-429`, + ); + if (!hop.allowed) return null; const nextAccountId = rotateGenericOAuthAccountOn429( config, route.providerName, genericFailoverAccountId, retryAfter, ); - if (!nextAccountId) return null; + if (!nextAccountId) { + hop.permit?.release(); + return null; + } try { const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); genericFailovers += 1; - if (!await applyFailoverSnapshot(snapshot)) return null; + if (!await applyFailoverSnapshot(snapshot)) { + hop.permit?.release(); + return null; + } } catch { + hop.permit?.release(); return null; } + hop.permit?.use(); } else if ( // Anthropic's pool is excluded from generic failover, so without this arm a 429 inside a // web-search or image-bridge turn was terminal even with the pool fully enabled -- while @@ -7056,6 +7159,13 @@ async function handleResponsesInner( anthropicPoolAccountId && anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST ) { + // Same intersection for the Anthropic roster: its own per-request bound still applies, + // and the shared budget decides whether this request may spend another send at all. + const hop = reserveCredentialHop( + "account-failover", + `${route.providerName}|${route.modelId}|sidecar-anthropic-429`, + ); + if (!hop.allowed) return null; const nextAccountId = rotateAnthropicAccountOn429( config, anthropicPoolAccountId, @@ -7064,7 +7174,10 @@ async function handleResponsesInner( Date.now(), responseHeaders, ); - if (!nextAccountId) return null; + if (!nextAccountId) { + hop.permit?.release(); + return null; + } try { // Deliberately NOT applyFailoverSnapshot: that helper exists to pair per-account routing // metadata (Copilot origin, Antigravity project, Kiro context) with its bearer. Anthropic @@ -7078,8 +7191,10 @@ async function handleResponsesInner( route.provider = { ...route.provider, apiKey: admitted.accessToken }; logCtx.provider = formatAnthropicProviderForLog("anthropic", admitted.accountId, config); } catch { + hop.permit?.release(); return null; } + hop.permit?.use(); } else { // No key pool, no generic OAuth roster, no Anthropic pool could produce a replacement // credential. The 429 is terminal for this sidecar turn. @@ -7408,17 +7523,33 @@ async function handleResponsesInner( || genericFailovers >= GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST || !isGenericOAuthFailoverEnabled(config, route.providerName) ) return false; + // Intersection with the request's shared budget: the roster bound above answers "may this + // credential set rotate again", this answers "may this request send again at all". The + // replayed turn is dispatched by runTurnAttempt and never reaches `onSendsConsumed`, so + // this reservation is the charge. Refusing returns false, which leaves the preflight 429 + // to reach the client exactly as the adapter produced it. + const hop = reserveCredentialHop( + "account-failover", + `${route.providerName}|${route.modelId}|runturn-oauth-429`, + ); + if (!hop.allowed) return false; const nextAccountId = rotateGenericOAuthAccountOn429( config, route.providerName, genericFailoverAccountId, null, ); - if (!nextAccountId) return false; + if (!nextAccountId) { + hop.permit?.release(); + return false; + } try { const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); genericFailovers += 1; - if (!await applyFailoverSnapshot(snapshot)) return false; + if (!await applyFailoverSnapshot(snapshot)) { + hop.permit?.release(); + return false; + } // A Cursor conversation/checkpoint is credential-scoped. The failed attempt emitted no // client-visible bytes, so replay is safe, but carrying its account identity into the next // account would not be. Let the rotated adapter derive a fresh identity and conversation. @@ -7435,7 +7566,10 @@ async function handleResponsesInner( inboundWire, ); const rotatedAdapter = resolveSelectionAdapter(rotatedProvider, config.cacheRetention); - if (!rotatedAdapter.runTurn) return false; + if (!rotatedAdapter.runTurn) { + hop.permit?.release(); + return false; + } runTurnAdapter = rotatedAdapter; bindRouteReasoningReplayScope({ parsed, @@ -7448,8 +7582,11 @@ async function handleResponsesInner( }); sealRequestAttemptIdentity(logCtx.activeAttempt, logCtx.provider, rotatedAdapter.name, logCtx.accountLogLabel); recordAttemptCredentialSource(logCtx.activeAttempt, route.providerName, route.provider, rotatedAdapter.name); + // The caller replays the turn on this rotation, so the reservation is now confirmed. + hop.permit?.use(); return true; } catch { + hop.permit?.release(); return false; } }; @@ -7913,32 +8050,38 @@ async function handleResponsesInner( `${route.providerName}|${route.modelId}|${recovery}`, ) : undefined; - return await refetchWithPolicy( - recoveryKind => { - if (refetchAllowance?.permit && !refetchAllowance.permit.use()) { - throw new SendBudgetExhaustedError(safeHostLabel(retryRequest.url)); - } - return fetchWithHeaderTimeout(retryRequest.url, - applyUpstreamRecoveryInit({ - method: retryRequest.method, headers: retryRequest.headers, body: retryRequest.body, - }, recoveryKind), upstream.signal, connectMs, parsed.stream, - providerFetch(route.provider, options.codexWsRuntimeIdentity, { - dispatchOverride: oauthDispatch(retryRequest), - providerName: route.providerName, - modelId: route.modelId, - })); - }, - { - abortSignal: upstream.signal, - label: safeHostLabel(retryRequest.url), - ...(refetchAllowance - ? { - attempts: refetchAllowance.attempts, - onSendsConsumed: noteTransientSends, + try { + return await refetchWithPolicy( + recoveryKind => { + if (refetchAllowance?.permit && !refetchAllowance.permit.use()) { + throw new SendBudgetExhaustedError(safeHostLabel(retryRequest.url)); } - : {}), - }, - ); + return fetchWithHeaderTimeout(retryRequest.url, + applyUpstreamRecoveryInit({ + method: retryRequest.method, headers: retryRequest.headers, body: retryRequest.body, + }, recoveryKind), upstream.signal, connectMs, parsed.stream, + providerFetch(route.provider, options.codexWsRuntimeIdentity, { + dispatchOverride: oauthDispatch(retryRequest), + providerName: route.providerName, + modelId: route.modelId, + })); + }, + { + abortSignal: upstream.signal, + label: safeHostLabel(retryRequest.url), + ...(refetchAllowance + ? { + attempts: refetchAllowance.attempts, + onSendsConsumed: noteTransientSends, + } + : {}), + }, + ); + } finally { + // Refunds only a reservation whose send never happened -- an abort settled before + // the thunk ran. A used or externally settled permit ignores this. + refetchAllowance?.permit?.release(); + } } finally { retryRequest.releaseBodyObservation?.(); } diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 8500c4fc78..234362405b 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -960,6 +960,7 @@ "repo-hygiene.test.ts": "ci-workflows", "request-decompress.test.ts": "usage", "request-evidence.test.ts": "usage", + "request-execution-budget.test.ts": "lib", "request-history-index.test.ts": "usage", "request-log-conversation.test.ts": "usage", "request-log-estimate-cap.test.ts": "usage", diff --git a/tests/lib/request-execution-budget.test.ts b/tests/lib/request-execution-budget.test.ts new file mode 100644 index 0000000000..e3f43b570f --- /dev/null +++ b/tests/lib/request-execution-budget.test.ts @@ -0,0 +1,167 @@ +import { describe, expect, test } from "bun:test"; +import { + CODEX_TEXT_GUARDED_BUDGET_POLICY, + createRequestExecutionBudget, + type RequestExecutionBudgetPolicy, +} from "../../src/lib/request-execution-budget"; + +/** + * The permit is the charge (#4546). + * + * `reserveDispatch` used to decide and `permit.use()` used to charge, which made the decision + * advisory: two legs that read the same remainder in the same turn -- an account move and a + * rebuild, a combo child and its parent -- both received a permit and both dispatched. One + * remaining send admitted two physical sends, which is the per-request multiplication the whole + * budget exists to stop. These pin the three properties the fix depends on: the second racer is + * refused, an abandoned reservation is refunded exactly, and a send counted by a retry helper is + * charged once rather than twice. + */ +const ONE_SEND_LEFT: RequestExecutionBudgetPolicy = { + maxTotalModelSends: 1, + baseSendAllowance: 1, + finalRecoveryAllowance: 0, + maxAlternateTargetSends: 1, + maxTargetTransitions: 1, +}; + +describe("atomic dispatch permits", () => { + test("two interleaved reserves for one remaining send produce exactly one permit", () => { + const budget = createRequestExecutionBudget(ONE_SEND_LEFT); + // Both legs reserve before either dispatches. This is the ordering that used to pass twice. + const first = budget.reserveDispatch({ sendClass: "initial", targetKey: "t" }); + const second = budget.reserveDispatch({ sendClass: "transient", targetKey: "t" }); + + expect(first.allowed).toBe(true); + expect(second.allowed).toBe(false); + if (second.allowed) throw new Error("unreachable"); + expect(second.reason).toBe("total-exhausted"); + // The reservation itself spent the send, before anything confirmed it. + expect(budget.used).toBe(1); + expect(budget.remainingBaseSends(5)).toBe(0); + + if (!first.allowed) throw new Error("unreachable"); + expect(first.permit.use()).toBe(true); + // Confirmation charges nothing more, and a second confirmation is refused rather than + // buying the retry thunk another send. + expect(first.permit.use()).toBe(false); + expect(budget.used).toBe(1); + }); + + test("release restores the remainder exactly, including the single shared reserve", () => { + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + for (let i = 0; i < CODEX_TEXT_GUARDED_BUDGET_POLICY.baseSendAllowance; i++) { + const send = budget.reserveDispatch({ sendClass: "transient", targetKey: "a" }); + expect(send.allowed).toBe(true); + if (send.allowed) send.permit.use(); + } + expect(budget.used).toBe(3); + + // The fourth send: an account move funded by the final-recovery reserve. + const move = budget.reserveDispatch({ sendClass: "account-failover", targetKey: "b" }); + expect(move.allowed).toBe(true); + if (!move.allowed) throw new Error("unreachable"); + expect(budget.used).toBe(4); + expect(budget.reserveSpent).toBe(true); + expect(budget.alternateTargetSends).toBe(1); + expect(budget.targetTransitions).toBe(1); + expect(budget.lastTargetKey).toBe("b"); + + // The resolver found no alternate account, so the move never became a send. + move.permit.release(); + expect(budget.used).toBe(3); + expect(budget.reserveSpent).toBe(false); + expect(budget.alternateTargetSends).toBe(0); + expect(budget.targetTransitions).toBe(0); + expect(budget.lastTargetKey).toBe("a"); + + // Exactly restored: the request can still make its one final-recovery send elsewhere. + const rebuild = budget.reserveDispatch({ sendClass: "repair", targetKey: "a" }); + expect(rebuild.allowed).toBe(true); + expect(budget.used).toBe(4); + + // A released permit is inert afterwards, and releasing twice cannot refund twice. + move.permit.release(); + expect(move.permit.use()).toBe(false); + expect(budget.used).toBe(4); + }); + + test("a countedExternally permit plus its external report charges exactly one send", () => { + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + const leg = budget.reserveDispatch({ + sendClass: "auth-recovery", + targetKey: "t", + countedExternally: true, + }); + expect(leg.allowed).toBe(true); + if (!leg.allowed) throw new Error("unreachable"); + // Booked immediately -- a concurrent leg must see this send as spent even though the retry + // helper has not reported it yet. + expect(budget.used).toBe(1); + + expect(leg.permit.use()).toBe(true); + // `onSendsConsumed` reporting one physical send settles the pending booking instead of + // charging a second time. Charging both is how a four-send cap became a two-send cap. + budget.used += 1; + expect(budget.used).toBe(1); + + // Sends the helper made beyond the reserved one are still charged in full. + budget.used += 2; + expect(budget.used).toBe(3); + }); + + test("an external report settles the booking, so a late release refunds nothing", () => { + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + const leg = budget.reserveDispatch({ + sendClass: "auth-recovery", + targetKey: "t", + countedExternally: true, + }); + if (!leg.allowed) throw new Error("unreachable"); + budget.used += 1; + expect(budget.used).toBe(1); + // The send physically happened. A refund here would hand the request a free one back. + leg.permit.release(); + expect(budget.used).toBe(1); + }); +}); + +describe("layer caps intersect the shared budget", () => { + test("a second credential hop is refused while total allowance remains", () => { + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + const initial = budget.reserveDispatch({ sendClass: "initial", targetKey: "acct-1" }); + if (!initial.allowed) throw new Error("unreachable"); + initial.permit.use(); + + const firstHop = budget.reserveDispatch({ sendClass: "account-failover", targetKey: "acct-2" }); + expect(firstHop.allowed).toBe(true); + if (!firstHop.allowed) throw new Error("unreachable"); + firstHop.permit.use(); + + // GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST would allow a second and a third hop. The shared + // budget still has two of four sends left. The effective allowance is the intersection, and + // the cross-account bounds are what refuse here: one target transition and one alternate + // send per request, whichever is reached first. + const secondHop = budget.reserveDispatch({ sendClass: "account-failover", targetKey: "acct-3" }); + expect(secondHop.allowed).toBe(false); + if (secondHop.allowed) throw new Error("unreachable"); + expect(secondHop.reason).toBe("target-transition-exhausted"); + expect(budget.used).toBe(2); + expect(budget.used).toBeLessThan(CODEX_TEXT_GUARDED_BUDGET_POLICY.maxTotalModelSends); + }); + + test("a same-target replay stops at the base allowance instead of taking the reserve", () => { + const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + for (let i = 0; i < 3; i++) { + const rung = budget.reserveDispatch({ sendClass: "transient", targetKey: "same" }); + expect(rung.allowed).toBe(true); + if (rung.allowed) rung.permit.use(); + } + // The gated-model 400 ladder is same-account, same-target: it is an ordinary transient send + // and may not reach for the reserve an account move or a validated rebuild is funded from. + const fourth = budget.reserveDispatch({ sendClass: "transient", targetKey: "same" }); + expect(fourth.allowed).toBe(false); + if (fourth.allowed) throw new Error("unreachable"); + expect(fourth.reason).toBe("base-allowance-exhausted"); + expect(budget.reserveSpent).toBe(false); + }); +}); diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index 1e7f2a1f10..84188532b1 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -84,3 +84,75 @@ describe("transient send budget stays request-scoped", () => { expect(retry).toContain("class SendBudgetExhaustedError extends Error"); }); }); + +/** + * The dispatch paths that were not merely uncounted but UNCOUNTABLE (#4546). + * + * Three holes survived the earlier slices, and each is invisible at runtime until a real account + * pool is hot: `fetchWithResetRetry` had no reporting seam at all, so every leg without a + * transient policy sent off the books; the compact endpoint's routed fallback called + * `handleResponses` with no budget, so a native attempt's spend was forgotten the moment it fell + * through; and the credential hops enforced their own per-roster caps against a counter that knew + * nothing about the rest of the request. The wiring is what these assert -- the arithmetic is + * pinned in `request-execution-budget.test.ts`. + */ +describe("every dispatch path reports into the shared budget", () => { + test("the reset-only helper counts its own physical sends", () => { + const retry = source("lib/upstream-retry.ts"); + // The seam moved onto ResetRetryOptions. On TransientRetryOptions it could not be reached by + // the non-policy adapter send or by any rebuildAndRefetch leg with a null transient policy. + const resetOptions = retry.slice( + retry.indexOf("export interface ResetRetryOptions {"), + retry.indexOf("export interface TransientRetryOptions"), + ); + expect(resetOptions).toContain("onSendsConsumed?: (sends: number) => void;"); + // One report per physical send, before the await, so a rejected send still counts. + expect(retry).toContain("opts.onSendsConsumed?.(1);"); + // ...and the transient layer, which already counts the same sends through countedFetch, + // suppresses the inner reporter. Forwarding it would count every inner send twice. + expect(retry).toContain("onSendsConsumed: undefined,"); + expect(retry).not.toContain("fetchWithResetRetry(countedFetch, { ...opts, attempts: remaining() })"); + }); + + test("compact holds ONE budget for the native attempt, the handoff child and the routed turn", () => { + const compact = source("server/responses/compact.ts"); + // Declared once, at function scope. Inside the native branch it was out of reach of the + // routed fallback below, which is reached by a 404 native compact and by a quota failure. + expect(compact.match(/const sendBudget: RequestExecutionBudget = options.sendBudget ?? createRequestExecutionBudget();/g)) + .toHaveLength(1); + // The routed compaction turn inherits it instead of letting handleResponsesInner mint a + // fresh four. + expect(compact).toContain("turnAdmissionLease, sendBudget,"); + // The handoff child already inherited; both paths must keep doing so. + expect(compact).toContain("{ ...options, sendBudget }"); + }); + + test("credential hops keep their roster cap AND reserve from the shared budget", () => { + const core = source("server/responses/core.ts"); + // Four hop sites: the native passthrough 429, the shared sidecar hook's generic and + // Anthropic arms, and the runTurn preflight 429. + expect(core.match(/reserveCredentialHop(/g)).toHaveLength(4); + // The per-roster caps are NOT replaced. The effective allowance is the intersection, so + // removing either half is a behaviour change that has to be argued for. + expect(core).toContain("genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST"); + expect(core).toContain("genericFailovers >= GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST"); + expect(core).toContain("anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST"); + // A refused hop hands the reservation back rather than spending a send it never made. + expect(core.match(/hop.permit?.release();/g)?.length ?? 0).toBeGreaterThanOrEqual(6); + // The passthrough hop's replay spends the hop's own reservation; a second one would be + // refused as final-recovery-spent and would answer 502 instead of the real 429. + expect(core).toContain("pendingHopPermit = hop.permit;"); + }); + + test("the gated-model 400 ladder cannot outrun the request's total", () => { + const core = source("server/responses/core.ts"); + // The ladder keeps its own bound; the shared total is the other half of the minimum. + expect(core).toContain("Math.min(retrySameConfirmedAccount ? 7 : 1, sharedSendsLeft)"); + expect(core).toContain("executionBudget.policy.maxTotalModelSends - executionBudget.used"); + // Every rung past the first reserves its own send, so the ladder is visible to later legs + // instead of spending the request's allowance invisibly. + expect(core).toContain("targetKey: ladderTargetKey,"); + // The regressed shape: a flat seven-rung ladder that no request-level bound could see. + expect(core).not.toContain("const maxRetrySends = retrySameConfirmedAccount ? 7 : 1;"); + }); +}); From 00ff1cce005c439b3ed4a3c8c3804479f1604b41 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 23:59:16 +0900 Subject: [PATCH 2/3] fix(responses): repair the source-oracle regexes and classify a roster hop as auth-recovery (#4546) Three regex literals in the source oracle were unescaped; one was an unterminated group, which is an early SyntaxError that took the whole test file down at module load. And the four generic-OAuth/Anthropic credential hops reserved as account-failover, which sets isAlternateTarget unconditionally: under maxAlternateTargetSends 1 the first rotation refused every later one and consumed the slot a genuine cross-pool move needs, so a roster whose first two accounts were 429'd returned the 429 while a free third sat unused. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify. --- src/server/responses/core.ts | 18 ++++-- tests/lib/request-execution-budget.test.ts | 55 ++++++++++++++----- .../lib/transient-budget-scope-source.test.ts | 6 +- 3 files changed, 58 insertions(+), 21 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index a7c248ea69..133459dc99 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -5147,6 +5147,16 @@ async function handleResponsesInner( * status, `Retry-After`, quota body -- because return-the-last-answer is the exhaustion * contract this unit settled on. */ + /** + * A credential rotation inside ONE provider's roster is "auth-recovery", not + * "account-failover". The distinction is load-bearing: "account-failover" sets + * `isAlternateTarget` unconditionally, so under `maxAlternateTargetSends: 1` the first + * rotation would refuse every later one AND consume the single slot a genuine cross-pool + * move needs -- a roster whose first two accounts are both 429'd would return the 429 + * while a free third account sat unused. The roster cap bounds how far rotation walks; + * the shared total bounds how many sends the request makes. Reserve "account-failover" + * for a real move between pools. + */ const reserveCredentialHop = ( sendClass: SendClass, targetKey: string, @@ -6057,7 +6067,7 @@ async function handleResponsesInner( // other. A refused hop leaves the real 429 -- body, Retry-After and any quota evidence // -- exactly as upstream sent it. const hop = reserveCredentialHop( - "account-failover", + "auth-recovery", `${route.providerName}|${route.modelId}|oauth-account-429`, true, ); @@ -7126,7 +7136,7 @@ async function handleResponsesInner( // web-search/image loop and never reaches `onSendsConsumed`, so this reservation is the // charge; a refusal returns null and the caller keeps the real 429 it already has. const hop = reserveCredentialHop( - "account-failover", + "auth-recovery", `${route.providerName}|${route.modelId}|sidecar-oauth-429`, ); if (!hop.allowed) return null; @@ -7162,7 +7172,7 @@ async function handleResponsesInner( // Same intersection for the Anthropic roster: its own per-request bound still applies, // and the shared budget decides whether this request may spend another send at all. const hop = reserveCredentialHop( - "account-failover", + "auth-recovery", `${route.providerName}|${route.modelId}|sidecar-anthropic-429`, ); if (!hop.allowed) return null; @@ -7529,7 +7539,7 @@ async function handleResponsesInner( // this reservation is the charge. Refusing returns false, which leaves the preflight 429 // to reach the client exactly as the adapter produced it. const hop = reserveCredentialHop( - "account-failover", + "auth-recovery", `${route.providerName}|${route.modelId}|runturn-oauth-429`, ); if (!hop.allowed) return false; diff --git a/tests/lib/request-execution-budget.test.ts b/tests/lib/request-execution-budget.test.ts index e3f43b570f..18342f255c 100644 --- a/tests/lib/request-execution-budget.test.ts +++ b/tests/lib/request-execution-budget.test.ts @@ -126,27 +126,54 @@ describe("atomic dispatch permits", () => { }); describe("layer caps intersect the shared budget", () => { - test("a second credential hop is refused while total allowance remains", () => { - const budget = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); - const initial = budget.reserveDispatch({ sendClass: "initial", targetKey: "acct-1" }); + test("a roster credential hop walks within the shared total; a cross-pool move does not", () => { + // The two classes answer different questions and must not be conflated. A credential + // rotation inside ONE provider's roster is "auth-recovery": its own roster cap decides how + // far it walks, and the shared total decides how many sends the request may make. A move + // between pools is "account-failover", which is bounded to a single alternate target so a + // request cannot shop the whole estate. + const roster = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + const initial = roster.reserveDispatch({ sendClass: "initial", targetKey: "acct-1" }); if (!initial.allowed) throw new Error("unreachable"); initial.permit.use(); - const firstHop = budget.reserveDispatch({ sendClass: "account-failover", targetKey: "acct-2" }); + const firstHop = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-2" }); expect(firstHop.allowed).toBe(true); if (!firstHop.allowed) throw new Error("unreachable"); firstHop.permit.use(); - // GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST would allow a second and a third hop. The shared - // budget still has two of four sends left. The effective allowance is the intersection, and - // the cross-account bounds are what refuse here: one target transition and one alternate - // send per request, whichever is reached first. - const secondHop = budget.reserveDispatch({ sendClass: "account-failover", targetKey: "acct-3" }); - expect(secondHop.allowed).toBe(false); - if (secondHop.allowed) throw new Error("unreachable"); - expect(secondHop.reason).toBe("target-transition-exhausted"); - expect(budget.used).toBe(2); - expect(budget.used).toBeLessThan(CODEX_TEXT_GUARDED_BUDGET_POLICY.maxTotalModelSends); + // The second hop is what a roster of three 429'd accounts needs. Classifying it as a + // cross-account move would refuse it here and strand a free third account. + const secondHop = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-3" }); + expect(secondHop.allowed).toBe(true); + if (!secondHop.allowed) throw new Error("unreachable"); + secondHop.permit.use(); + expect(roster.used).toBe(3); + + // The shared total is the real bound: the fourth send is the reserve, and a fifth is gone. + const fourth = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-4" }); + expect(fourth.allowed).toBe(true); + if (!fourth.allowed) throw new Error("unreachable"); + fourth.permit.use(); + const fifth = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-5" }); + expect(fifth.allowed).toBe(false); + expect(roster.used).toBe(CODEX_TEXT_GUARDED_BUDGET_POLICY.maxTotalModelSends); + + // A genuine cross-pool move keeps its one-transition bound with total allowance to spare. + const pool = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); + const first = pool.reserveDispatch({ sendClass: "initial", targetKey: "pool-a" }); + if (!first.allowed) throw new Error("unreachable"); + first.permit.use(); + const move = pool.reserveDispatch({ sendClass: "account-failover", targetKey: "pool-b" }); + expect(move.allowed).toBe(true); + if (!move.allowed) throw new Error("unreachable"); + move.permit.use(); + const secondMove = pool.reserveDispatch({ sendClass: "account-failover", targetKey: "pool-c" }); + expect(secondMove.allowed).toBe(false); + if (secondMove.allowed) throw new Error("unreachable"); + expect(secondMove.reason).toBe("target-transition-exhausted"); + expect(pool.used).toBe(2); + expect(pool.used).toBeLessThan(CODEX_TEXT_GUARDED_BUDGET_POLICY.maxTotalModelSends); }); test("a same-target replay stops at the base allowance instead of taking the reserve", () => { diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index 84188532b1..afe489056d 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -118,7 +118,7 @@ describe("every dispatch path reports into the shared budget", () => { const compact = source("server/responses/compact.ts"); // Declared once, at function scope. Inside the native branch it was out of reach of the // routed fallback below, which is reached by a 404 native compact and by a quota failure. - expect(compact.match(/const sendBudget: RequestExecutionBudget = options.sendBudget ?? createRequestExecutionBudget();/g)) + expect(compact.match(/const sendBudget: RequestExecutionBudget = options\.sendBudget \?\? createRequestExecutionBudget\(\);/g)) .toHaveLength(1); // The routed compaction turn inherits it instead of letting handleResponsesInner mint a // fresh four. @@ -131,14 +131,14 @@ describe("every dispatch path reports into the shared budget", () => { const core = source("server/responses/core.ts"); // Four hop sites: the native passthrough 429, the shared sidecar hook's generic and // Anthropic arms, and the runTurn preflight 429. - expect(core.match(/reserveCredentialHop(/g)).toHaveLength(4); + expect(core.match(/reserveCredentialHop\(/g)).toHaveLength(4); // The per-roster caps are NOT replaced. The effective allowance is the intersection, so // removing either half is a behaviour change that has to be argued for. expect(core).toContain("genericFailovers < GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST"); expect(core).toContain("genericFailovers >= GENERIC_OAUTH_MAX_FAILOVERS_PER_REQUEST"); expect(core).toContain("anthropicPoolFailovers < ANTHROPIC_POOL_MAX_FAILOVERS_PER_REQUEST"); // A refused hop hands the reservation back rather than spending a send it never made. - expect(core.match(/hop.permit?.release();/g)?.length ?? 0).toBeGreaterThanOrEqual(6); + expect(core.match(/hop\.permit\?\.release\(\);/g)?.length ?? 0).toBeGreaterThanOrEqual(6); // The passthrough hop's replay spends the hop's own reservation; a second one would be // refused as final-recovery-spent and would answer 502 instead of the real 429. expect(core).toContain("pendingHopPermit = hop.permit;"); From 70a737c3fda0743b49f1c4b2557ccae59ac0825f Mon Sep 17 00:00:00 2001 From: JUN Date: Tue, 15 Sep 2026 00:31:18 +0900 Subject: [PATCH 3/3] fix(responses): let the gated-400 ladder keep its own bound, and stabilise its target key (#4546) Hosted CI at 00ff1cce00 failed three tests, all from this layer. #2097 pins the same-account gated-model 400 recovery at eight dispatches; clamping the ladder to what the request budget had left cut it to four, which is the flat-ceiling mistake 040_send_budget.md warns about. The rungs are still charged and still reserve, but a refusal no longer ends the ladder. The ladder target key no longer folds in the account id, which had made every same-account rung read as a target change and spend the one cross-account slot a genuine move needs. The new unit test used a changing target key that production never produces, and the new file name collided with the usage-domain regex seed. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify. --- scripts/test-layout/layout.json | 4 +- src/server/responses/core.ts | 24 +++++++++--- tests/fixtures/test-layout-expected.json | 4 +- ...st.ts => execution-budget-permits.test.ts} | 14 ++++--- .../lib/transient-budget-scope-source.test.ts | 39 ++++++++++++++----- 5 files changed, 61 insertions(+), 24 deletions(-) rename tests/lib/{request-execution-budget.test.ts => execution-budget-permits.test.ts} (94%) diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 00f10bc494..c9b5ad4c68 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1132,7 +1132,6 @@ "repo-hygiene.test.ts": "ci-workflows", "request-decompress.test.ts": "usage", "request-evidence.test.ts": "usage", - "request-execution-budget.test.ts": "lib", "request-history-index.test.ts": "usage", "request-log-conversation.test.ts": "usage", "request-log-estimate-cap.test.ts": "usage", @@ -1445,7 +1444,8 @@ "main-device-reauth-api.test.ts": "codex-integration", "main-device-reauth-ui.test.ts": "gui", "adapter-input-media-guard.test.ts": "adapters", - "chat-media-translation.test.ts": "responses" + "chat-media-translation.test.ts": "responses", + "execution-budget-permits.test.ts": "lib" }, "migrated": [ "adapters", diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 133459dc99..913fb13795 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -1665,11 +1665,17 @@ async function retryCodexPoolOnAlternateAccount( // was called to make -- the move already paid for itself with its own permit -- and each rung // past the first reserves its own send below, so a refusal stops the ladder with the last // upstream answer intact. - const ladderTargetKey = `${route.providerName}|${route.modelId}|${retryAuthCtx.accountId}`; - const sharedSendsLeft = executionBudget - ? Math.max(1, executionBudget.policy.maxTotalModelSends - executionBudget.used) - : Number.POSITIVE_INFINITY; - const maxRetrySends = Math.min(retrySameConfirmedAccount ? 7 : 1, sharedSendsLeft); + // The ladder replays to the SAME account, so it must reserve under the same target key the + // other legs use. Folding the account id in made every rung read as a target change, which + // spent the one cross-account slot a real move needs on a same-account replay. + const ladderTargetKey = `${route.providerName}|${route.modelId}`; + // The ladder keeps its OWN bound rather than drawing on what the request has left. Clamping it + // to the shared total looked right and broke a working, pinned path: #2097 fixes this recovery + // at eight same-account dispatches (tests/server/server-auth.test.ts), and a request that has + // already spent sends would silently stop short of it. Reconciling an eight-send same-account + // ladder with a four-send request total is a policy decision, not a clamp to add in passing. + // What this diff does fix is that the rungs are now CHARGED instead of free. + const maxRetrySends = retrySameConfirmedAccount ? 7 : 1; let retrySendCount = 0; let upstreamResponse: Response; try { @@ -1745,12 +1751,18 @@ async function retryCodexPoolOnAlternateAccount( // same-target replay, charged as an ordinary transient send rather than as a move. // Reserved here, immediately before looping back, so a refusal stops the ladder with the // last upstream 400 intact instead of spending a send it cannot make. + // Every rung is CHARGED, and a refusal does not end the ladder. That asymmetry is + // deliberate and it is the one place the shared cap yields. This is a same-account, + // same-target replay of a model-gating 400 whose own bound is eight dispatches, pinned by + // #2097; letting a spent request budget cut it to four would break a recovery that works + // today, which is precisely the mistake 040_send_budget.md warns a flat ceiling makes. + // The request total still governs everything that changes target or credential. if (executionBudget) { const rung = executionBudget.reserveDispatch({ sendClass: "transient", targetKey: ladderTargetKey, }); - if (!rung.allowed || !rung.permit.use()) break; + if (rung.allowed) rung.permit.use(); chargeWorkflowSends(args.options.workflowRootId, 1); } await upstreamResponse.body?.cancel().catch(() => undefined); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 234362405b..a327241231 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -960,7 +960,6 @@ "repo-hygiene.test.ts": "ci-workflows", "request-decompress.test.ts": "usage", "request-evidence.test.ts": "usage", - "request-execution-budget.test.ts": "lib", "request-history-index.test.ts": "usage", "request-log-conversation.test.ts": "usage", "request-log-estimate-cap.test.ts": "usage", @@ -1277,5 +1276,6 @@ "main-device-reauth-api.test.ts": "codex-integration", "main-device-reauth-ui.test.ts": "gui", "adapter-input-media-guard.test.ts": "adapters", - "chat-media-translation.test.ts": "responses" + "chat-media-translation.test.ts": "responses", + "execution-budget-permits.test.ts": "lib" } diff --git a/tests/lib/request-execution-budget.test.ts b/tests/lib/execution-budget-permits.test.ts similarity index 94% rename from tests/lib/request-execution-budget.test.ts rename to tests/lib/execution-budget-permits.test.ts index 18342f255c..2276c921ae 100644 --- a/tests/lib/request-execution-budget.test.ts +++ b/tests/lib/execution-budget-permits.test.ts @@ -132,30 +132,34 @@ describe("layer caps intersect the shared budget", () => { // far it walks, and the shared total decides how many sends the request may make. A move // between pools is "account-failover", which is bounded to a single alternate target so a // request cannot shop the whole estate. + // Production reserves every roster hop under ONE key per hop site -- provider|model|site -- + // because a CHANGED target key is an alternate target whatever the send class says. Using a + // per-account key here would have tested a shape the code never produces. + const ROSTER_KEY = "openai|gpt-5.6|sidecar-oauth-429"; const roster = createRequestExecutionBudget(CODEX_TEXT_GUARDED_BUDGET_POLICY); - const initial = roster.reserveDispatch({ sendClass: "initial", targetKey: "acct-1" }); + const initial = roster.reserveDispatch({ sendClass: "initial", targetKey: ROSTER_KEY }); if (!initial.allowed) throw new Error("unreachable"); initial.permit.use(); - const firstHop = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-2" }); + const firstHop = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: ROSTER_KEY }); expect(firstHop.allowed).toBe(true); if (!firstHop.allowed) throw new Error("unreachable"); firstHop.permit.use(); // The second hop is what a roster of three 429'd accounts needs. Classifying it as a // cross-account move would refuse it here and strand a free third account. - const secondHop = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-3" }); + const secondHop = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: ROSTER_KEY }); expect(secondHop.allowed).toBe(true); if (!secondHop.allowed) throw new Error("unreachable"); secondHop.permit.use(); expect(roster.used).toBe(3); // The shared total is the real bound: the fourth send is the reserve, and a fifth is gone. - const fourth = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-4" }); + const fourth = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: ROSTER_KEY }); expect(fourth.allowed).toBe(true); if (!fourth.allowed) throw new Error("unreachable"); fourth.permit.use(); - const fifth = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: "acct-5" }); + const fifth = roster.reserveDispatch({ sendClass: "auth-recovery", targetKey: ROSTER_KEY }); expect(fifth.allowed).toBe(false); expect(roster.used).toBe(CODEX_TEXT_GUARDED_BUDGET_POLICY.maxTotalModelSends); diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index afe489056d..0772484aba 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -1,4 +1,20 @@ -import { describe, expect, test } from "bun:test"; + test("the gated-model 400 ladder is charged, and keeps its own bound", () => { + const core = source("server/responses/core.ts"); + // Every rung reserves and charges, so the ladder is visible to later legs instead of + // spending the request's allowance invisibly -- that part was the real defect. + expect(core).toContain("targetKey: ladderTargetKey,"); + expect(core).toContain("if (rung.allowed) rung.permit.use();"); + // A same-account replay must reserve under the SAME target key the other legs use. Folding + // the account id in made every rung read as a target change and spent the one cross-account + // slot a genuine move needs. + expect(core).toContain("const ladderTargetKey = `${route.providerName}|${route.modelId}`;"); + expect(core).not.toContain("|${retryAuthCtx.accountId}`;"); + // The ladder keeps its own bound and a budget refusal does NOT end it. #2097 pins this + // recovery at eight same-account dispatches; clamping it to what the request has left would + // cut a working path to four, which is the flat-ceiling mistake 040 warns about. + expect(core).toContain("const maxRetrySends = retrySameConfirmedAccount ? 7 : 1;"); + expect(core).not.toContain("Math.min(retrySameConfirmedAccount ? 7 : 1, sharedSendsLeft)"); + });import { describe, expect, test } from "bun:test"; import { readFileSync } from "node:fs"; import { join } from "node:path"; import { repoPath } from "../helpers/repo-root"; @@ -144,15 +160,20 @@ describe("every dispatch path reports into the shared budget", () => { expect(core).toContain("pendingHopPermit = hop.permit;"); }); - test("the gated-model 400 ladder cannot outrun the request's total", () => { + test("the gated-model 400 ladder is charged, and keeps its own bound", () => { const core = source("server/responses/core.ts"); - // The ladder keeps its own bound; the shared total is the other half of the minimum. - expect(core).toContain("Math.min(retrySameConfirmedAccount ? 7 : 1, sharedSendsLeft)"); - expect(core).toContain("executionBudget.policy.maxTotalModelSends - executionBudget.used"); - // Every rung past the first reserves its own send, so the ladder is visible to later legs - // instead of spending the request's allowance invisibly. + // Every rung reserves and charges, so the ladder is visible to later legs instead of + // spending the request's allowance invisibly -- that was the real defect. expect(core).toContain("targetKey: ladderTargetKey,"); - // The regressed shape: a flat seven-rung ladder that no request-level bound could see. - expect(core).not.toContain("const maxRetrySends = retrySameConfirmedAccount ? 7 : 1;"); + expect(core).toContain("if (rung.allowed) rung.permit.use();"); + // A same-account replay reserves under the SAME target key the other legs use. Folding the + // account id in made every rung read as a target change and spent the one cross-account slot + // a genuine move needs. + expect(core).toContain("const ladderTargetKey = `${route.providerName}|${route.modelId}`;"); + // The ladder keeps its own bound and a budget refusal does NOT end it. #2097 pins this + // recovery at eight same-account dispatches; clamping it to what the request has left cut a + // working path to four, which is the flat-ceiling mistake 040_send_budget.md warns about. + expect(core).toContain("const maxRetrySends = retrySameConfirmedAccount ? 7 : 1;"); + expect(core).not.toContain("Math.min(retrySameConfirmedAccount ? 7 : 1, sharedSendsLeft)"); }); });