diff --git a/devlog/_plan/260914_cost_guard_stabilization/030_move_reason_evidence.md b/devlog/_plan/260914_cost_guard_stabilization/030_move_reason_evidence.md index 291ed25817..6604022d95 100644 --- a/devlog/_plan/260914_cost_guard_stabilization/030_move_reason_evidence.md +++ b/devlog/_plan/260914_cost_guard_stabilization/030_move_reason_evidence.md @@ -34,3 +34,26 @@ wp3 lands the reason at the decision point and the record, because that is what makes the wp2 and wp3 rules auditable in the field rather than only in tests. The dashboard rendering and the amplification metric (sends per logical request) belong with wp4, where the send budget gives them a denominator that means something. + +## Outcome + +Closed. `resolveCodexAccountForThreadDetailed` now returns a `CodexAffinityDecision` on every +selection path, the pool auth context carries it, and `logCtx.affinity` / `logCtx.affinityReason` +are assigned in `core.ts` (`849f3c9ccf`). A release recorded by the outcome path -- a 429 +clearing the pin -- is held per thread, bounded at 4096 entries, and consumed by that thread's +next resolve. + +Two audit rounds changed the shape, and both corrections are worth keeping: + +The reason was being synthesized at the call site instead of read from the guard that actually +refused the account. It now comes from `codexAccountBlockReason`, and a release survives a +resolve that finds no account at all (`b8d90ba3a8`, closing #4598). + +`appendUsageEntry` builds the persisted entry from an explicit field whitelist, so the affinity +fields the writer set were dropped silently by the normalizer and the whole feature was a no-op +end to end. `ab6fd697c1` adds them to the whitelist and surfaces the decision in the route +explanation. The general lesson for anything downstream of the usage log: a field the writer +sets but the normalizer does not name does not exist. + +What wp3 deliberately did not do: render the reason in the dashboard, and count sends per +logical request. Both wait for wp4's budget to give them a denominator. diff --git a/devlog/_plan/260914_cost_guard_stabilization/040_send_budget.md b/devlog/_plan/260914_cost_guard_stabilization/040_send_budget.md index c62836c01c..eab453e1c2 100644 --- a/devlog/_plan/260914_cost_guard_stabilization/040_send_budget.md +++ b/devlog/_plan/260914_cost_guard_stabilization/040_send_budget.md @@ -143,3 +143,113 @@ That is observable today on the Codex, passthrough and combo paths -- `logCtx.attempts[].sendCount` across combo children. It is **not** observable for the Kiro and Cursor inner retries, which call `noteAttemptSend` once before dispatching, so those need instrumentation before their counts can be pinned. + +## Step 1 status, and six corrections the next audit round produced + +Step 1 landed (`7f9284ab1e`): `sendBudget` rides `HandleResponsesOptions`, is minted once at +ingress (`core.ts:3461`) and inherited by a combo child through the existing options spread +(`core.ts:3113`). Six findings from the follow-up audit change what comes next, so they are +recorded rather than quietly folded in. + +**The combo fix is half a fix.** A child inherits the *counter* but the adapter initial send +never reads it as a *limit*: `core.ts:7656` passes `attempts: transientPolicy.attempts` raw. +The oracle's own comment justifies that with "nothing has been spent yet", which is true for a +first turn and false for combo target 2. So target 1 can spend the budget and target 2 still +draws a fresh full policy allowance. Until `:7656` draws the remainder like every other leg, +the measured 12 does not come down. + +**The cross-account move is not merely unbudgeted, it is unbounded per request.** +`retryCodexPoolOnAlternateAccount` is at `core.ts:1434` (not `:1645`), and it sends directly +with `fetchWithHeaderTimeout` at `:1626` inside a loop whose `maxRetrySends` is 1 for a real +alternate but **7** for the same-account gated-400 ladder. The important part is the caller: +it sits inside `passthroughRecovery: for (;;)` (`:5628`), `excludeAccountId` excludes only the +account that just failed (`:1492`), and no per-request flag records that a move already +happened. Sequential account moves are bounded today by pool exhaustion and cooldowns, by +nothing else. A flat `used` counter does not close that; a separate move counter does. + +**`fetchWithResetRetry` has no counting seam at all.** `onSendsConsumed` lives only on +`TransientRetryOptions` (`upstream-retry.ts:304`) and fires only from `fetchWithTransientRetry` +(`:479`). Every leg that falls back to reset-only retry -- the non-policy adapter initial send +and every `rebuildAndRefetch` recovery kind when `refetchTransientPolicy` is null -- is +*uncountable*, not just uncounted. Step 2 therefore starts by giving `ResetRetryOptions` the +same callback, not by adding call-site wiring. + +**There is a fourth floor.** Besides `core.ts:4995` and `upstream-retry.ts:374, 420`, the +inner `remaining = () => Math.max(1, budget - sent)` at `upstream-retry.ts:439` re-floors the +reset call. Removing the three named sites still lets a spent budget send once. + +**The exhaustion contract is already decided by the codebase, twice.** `fetchWithTransientRetry` +returns the last response with its body intact when the budget runs out (`:476`), and the +reachable native-Chat path preserves the terminal 429 (pinned at +`tests/responses/chat-completions-endpoint.test.ts:1553, 1597`). The synthetic throw at +`chat-native.ts:308` is an unreachable backstop, not the policy. Return-the-last-answer is the +contract; a throw would hide the status, the `Retry-After` header and any quota body -- exactly +the evidence #3294/#3606 said to preserve. The throw stays only as a typed backstop for a +caller that forgot to check. + +**`sendCount` already reaches the wire.** The claim above that it "never reaches /api/usage or +the GUI" is wrong. It is a required persisted field (`src/usage/log.ts:100`), it survives the +whitelist normalizer (`:465`), `/api/logs` spreads it (`src/server/management/shared.ts:222`) +and the GUI already types it (`gui/src/pages/Logs.tsx:126`). What is missing is rendering (the +attempts table has no column) and aggregation (`summarizeUsage` counts attempts, never sends). + +## Delivery slices + +Steps 2-5 are not one diff. Verification here is hosted CI only, so a slice that breaks forty +pinned counts at once is undiagnosable. They ship in this order, one PR each: + +- **Slice A (this cycle).** Split the budget and close the two holes that need no new plumbing: + `TransientSendBudget` gains `accountMoves` with `CROSS_ACCOUNT_MAX_SENDS = 1`; + `retryCodexPoolOnAlternateAccount` charges a move and refuses a second one with the existing + `{ kind: "no-alternate" }` path after `recordUnmovedTransientOutcome()`; the adapter initial + send at `:7656` draws `remainingTransientSendBudget(transientPolicy.attempts)`. The split has + to come first because step 2 without it collapses the working 3 same-account + 1 alternate + shape that `tests/responses/responses-compaction-routing.test.ts:1346` pins. +- **Slice B.** `onSendsConsumed` on `ResetRetryOptions`, unconditional wiring at `:7652` and + `:7775`, `sendBudget` on `HandleResponsesCompactOptions`, and the empty-completion / + `runTurnAttempt` charge at `core.ts:7346`. +- **Slice C.** All four floors to `Math.max(0, ...)` plus the refusal contract above, with the + pinned counts in `responses-opaque-blob-recovery.test.ts` rewritten to the refusal shape. +- **Slice D.** The pool-wide retry ratio cap and `sendCount` aggregation. + +Kiro (up to ~18 sends per call, ~36 with the text fallback) and Cursor ride +`AdapterFetchContext`; that field must be optional and unlimited by default or every adapter +unit test that calls the transport context-free breaks. + +## Slice A landed, and the four counterexamples that shaped it + +PR #4609 carries the guarded profile from the PRD: four model sends per logical request, a base +allowance of three, and one final-recovery reserve that an account move and a validated rebuild +share. An adversarial audit round found four things that would have shipped as defects. + +**Charging the same send twice.** `permit.use()` increments `used`, and `onSendsConsumed` +increments it again for anything routed through the retry helper. A four-send cap would have +behaved as a two-send cap and every acceptance row would have been off by a factor of two. The +intent now carries `countedExternally`, so a helper-routed permit books the reserve and the +alternate-target ledgers but leaves `used` to the reporter. + +**Removing the floor kills a recovery the PRD wants kept.** The pinned sanitized-rebuild case +at `responses-opaque-blob-recovery.test.ts:600` is three 502s plus one rebuild, and its own +comment says the rebuild "draws on what is LEFT of that same budget" -- which is the floor. With +the floor gone the rebuild gets zero and the request dies at three. `recoverySendAllowance` +spends the base allowance first and only then draws the reserve, which is what keeps that fourth +send alive for the right reason instead of by accident. + +**The exhaustion contract is a call-site problem.** A typed throw inside the helper cannot +restore a body the caller already cancelled, and every catch on these paths launders a rejection +into 502 `upstream_error`. So the OAuth 401 replay and the same-target 429 wait check the +remainder in their own conditions, before the cancel, and an exhausted request returns the real +401 or 429 with its `Retry-After`. The typed error stays only as the backstop for a leg that +never had a prior response. + +**Reserving too early burns the slot on a request that never moved.** The same-account +gated-model 400 ladder runs through the same function and is bounded at eight sends by +`maxRetrySends`. Reserving before `retrySameConfirmedAccount` is known would have spent the +single failover slot on it. The reservation is guarded on `!retryAuthCtx`, which the ladder has +already set. + +Residual, accepted rather than hidden: `maxTargetTransitions` and `maxAlternateTargetSends` +would refuse the pinned three-target combo hop, so combo hops are not wired to +`reserveDispatch` in this slice and those fields are exercised only by the account-failover +path. Wiring combo needs a per-target policy, not a per-request transition cap. Compact, Kiro, +Cursor and the generic OAuth hops still hold their own allowances. diff --git a/src/lib/request-execution-budget.ts b/src/lib/request-execution-budget.ts new file mode 100644 index 0000000000..e7581c4605 --- /dev/null +++ b/src/lib/request-execution-budget.ts @@ -0,0 +1,202 @@ +/** + * One logical request, one execution budget (#4546). + * + * The amplification behind #4546 was never a single missing limit. Every layer that can + * re-send a request -- transport retry, adapter retry, auth recovery, account failover, combo + * failover, repair -- counted its own allowance, so a per-layer 3 composed into a per-request + * 12. #4605 and #4608 gave the transient layers one shared counter; this module is the policy + * that counter answers to. + * + * The policy is an INTERSECTION of constraints, not four independent counters. A request that + * still has total allowance left is not thereby entitled to a second account move, and a + * request that changed credentials does not get its target-transition allowance back. The + * default profile keeps the recovery shape that actually works today -- three same-account + * sends plus one alternate -- by funding the alternate from a reserve that a validated + * sanitized repair can spend instead, but never both. + */ +import type { TransientSendBudget } from "./upstream-retry"; + +export type SendClass = + | "initial" + | "transient" + | "auth-recovery" + | "repair" + | "account-failover" + | "combo-failover" + | "prewarm"; + +export interface RequestExecutionBudgetPolicy { + /** Every model send of one logical request, including the reserve. */ + readonly maxTotalModelSends: number; + /** Shared by the initial send, same-target transient retries, and refresh/repair legs. */ + readonly baseSendAllowance: number; + /** ONE final recovery, shared by an account move and a validated rebuild. Not one each. */ + readonly finalRecoveryAllowance: number; + readonly maxAlternateTargetSends: number; + readonly maxTargetTransitions: number; +} + +/** + * Text Codex guarded profile. Three same-account sends plus one alternate is the recovery + * shape that live traffic depends on, so a flat ceiling of 3 would break a working path. + */ +export const CODEX_TEXT_GUARDED_BUDGET_POLICY: RequestExecutionBudgetPolicy = { + maxTotalModelSends: 4, + baseSendAllowance: 3, + finalRecoveryAllowance: 1, + maxAlternateTargetSends: 1, + maxTargetTransitions: 1, +}; + +export const REQUEST_BUDGET_POLICY_VERSION = "guarded-v1"; + +export type BudgetDenial = + | "total-exhausted" + | "base-allowance-exhausted" + | "final-recovery-spent" + | "alternate-target-exhausted" + | "target-transition-exhausted" + | "not-replay-safe"; + +export interface DispatchIntent { + readonly sendClass: SendClass; + /** + * (provider route, endpoint, model lane, upstream credential identity). A quota domain is a + * different thing and must not be folded in here. + */ + readonly targetKey: string; + /** + * False refuses the dispatch outright. A request whose execution state upstream is unknown + * is not replayable just because budget remains (RFC 9110 9.2.2). + */ + 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. + */ + readonly countedExternally?: boolean; +} + +export interface SingleUseDispatchPermit { + readonly sendClass: SendClass; + /** Consume exactly once. A second call returns false and charges nothing. */ + use(): boolean; +} + +export type DispatchDecision = + | { allowed: true; permit: SingleUseDispatchPermit } + | { allowed: false; reason: BudgetDenial }; + +/** + * Carried on HandleResponsesOptions so a combo child, a rebuild and an alternate-account leg + * all decrement the same holder. `used` is the existing #4605 counter and still counts every + * model send; the reserve is what the fourth send draws on once the base allowance is gone. + */ +export interface RequestExecutionBudget extends TransientSendBudget { + readonly logicalRequestId: string; + readonly policyVersion: string; + readonly policy: RequestExecutionBudgetPolicy; + reserveDispatch(intent: DispatchIntent): DispatchDecision; + /** + * 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. + */ + remainingBaseSends(cap: number): number; + readonly reserveSpent: boolean; + readonly alternateTargetSends: number; + readonly targetTransitions: number; + readonly lastTargetKey: string | undefined; +} + +const RESERVE_FUNDED_CLASSES: ReadonlySet = new Set([ + "account-failover", + "combo-failover", + "repair", + "auth-recovery", +]); + +let logicalRequestSeq = 0; + +export function createRequestExecutionBudget( + policy: RequestExecutionBudgetPolicy = CODEX_TEXT_GUARDED_BUDGET_POLICY, + logicalRequestId?: string, +): RequestExecutionBudget { + let reserveSpent = false; + let alternateTargetSends = 0; + let targetTransitions = 0; + let lastTargetKey: string | undefined; + + const budget: RequestExecutionBudget = { + used: 0, + logicalRequestId: logicalRequestId ?? `lr-${Date.now().toString(36)}-${(logicalRequestSeq += 1).toString(36)}`, + policyVersion: REQUEST_BUDGET_POLICY_VERSION, + policy, + get reserveSpent() { return reserveSpent; }, + get alternateTargetSends() { return alternateTargetSends; }, + get targetTransitions() { return targetTransitions; }, + 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)); + }, + 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" }; + + const changesTarget = lastTargetKey !== undefined && lastTargetKey !== intent.targetKey; + const isAlternateTarget = changesTarget || intent.sendClass === "account-failover" + || intent.sendClass === "combo-failover"; + if (isAlternateTarget && changesTarget && targetTransitions >= policy.maxTargetTransitions) { + return { allowed: false, reason: "target-transition-exhausted" }; + } + if (isAlternateTarget && alternateTargetSends >= policy.maxAlternateTargetSends) { + return { allowed: false, reason: "alternate-target-exhausted" }; + } + + // 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; + if (drawsReserve) { + if (!RESERVE_FUNDED_CLASSES.has(intent.sendClass)) { + return { allowed: false, reason: "base-allowance-exhausted" }; + } + if (reserveSpent || policy.finalRecoveryAllowance <= 0) { + return { allowed: false, reason: "final-recovery-spent" }; + } + } + + let consumed = false; + 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; + return true; + }, + }, + }; + }, + }; + if (lastTargetKey === undefined) lastTargetKey = undefined; + return budget; +} + +export function isRequestExecutionBudget( + value: TransientSendBudget | undefined, +): value is RequestExecutionBudget { + return typeof (value as RequestExecutionBudget | undefined)?.reserveDispatch === "function"; +} diff --git a/src/lib/upstream-retry.ts b/src/lib/upstream-retry.ts index 6fd57eb406..805a1e347a 100644 --- a/src/lib/upstream-retry.ts +++ b/src/lib/upstream-retry.ts @@ -72,6 +72,46 @@ export interface TransientSendBudget { export function createTransientSendBudget(): TransientSendBudget { return { used: 0 }; } + +/** + * Refusal raised when a logical request has no send left (#4546, REQ-B04/B05). + * + * It is deliberately a distinct type rather than a generic `Error`: every call site that + * catches a helper rejection today launders it into HTTP 502 `upstream_error`, which would + * report a proxy-side budget decision as an upstream fault and hide the real 401/429 the + * request already had. Callers must recognise this and return the structured local error + * instead. It is a backstop, not the policy -- a call site that still holds a reusable + * upstream response is supposed to check the remainder BEFORE it cancels that body. + */ +export class SendBudgetExhaustedError extends Error { + readonly code = "request_send_budget_exhausted"; + constructor(label?: string) { + super(label + ? `request send budget exhausted before dispatch (${label})` + : "request send budget exhausted before dispatch"); + this.name = "SendBudgetExhaustedError"; + } +} + +/** + * Configuration refusal for an attempts value that is not a send count. + * + * `undefined` means "use the policy default" and `0` means "refuse". A negative, fractional, + * NaN or infinite value is a programming or configuration error, and silently substituting the + * default for it is how a broken budget turns back into three free sends. + */ +export class InvalidSendBudgetError extends Error { + constructor(value: unknown) { + super(`invalid upstream send budget: ${String(value)}`); + this.name = "InvalidSendBudgetError"; + } +} + +function normalizeSendAttempts(value: number | undefined, fallback: number): number { + if (value === undefined) return fallback; + if (!Number.isInteger(value) || value < 0) throw new InvalidSendBudgetError(value); + return value; +} const TRANSIENT_RETRY_BASE_DELAY_MS = 400; const TRANSIENT_RETRY_MAX_DELAY_MS = 5_000; // A failed attempt slower than this is the "slow 502" incident shape (191s observed on @@ -371,7 +411,11 @@ export async function fetchWithResetRetry( opts: ResetRetryOptions = {}, firstRecovery?: UpstreamSendRecovery, ): Promise { - const attempts = Math.max(1, opts.attempts ?? RESET_RETRY_MAX_ATTEMPTS); + const attempts = normalizeSendAttempts(opts.attempts, RESET_RETRY_MAX_ATTEMPTS); + // Zero is zero. The old Math.max(1, ...) floor meant an exhausted budget still bought one + // more send on every recovery leg, which is most of what made a bounded per-layer retry + // compose into an unbounded per-request count. + if (attempts === 0) throw new SendBudgetExhaustedError(opts.label); let lastError: unknown; let sawReset = false; for (let attempt = 0; attempt < attempts; attempt++) { @@ -417,7 +461,7 @@ export async function fetchWithTransientRetry( doFetch: ReplayableFetch, opts: TransientRetryOptions = {}, ): Promise { - const budget = Math.max(1, opts.attempts ?? TRANSIENT_RETRY_MAX_ATTEMPTS); + const budget = normalizeSendAttempts(opts.attempts, TRANSIENT_RETRY_MAX_ATTEMPTS); const slowAttemptMs = opts.slowAttemptMs ?? TRANSIENT_RETRY_SLOW_ATTEMPT_MS; const transientStatuses: number[] = []; // `attempts` is ONE total-send budget shared with the inner reset layer, not a per-layer @@ -434,13 +478,15 @@ export async function fetchWithTransientRetry( sent += 1; return doFetch(recovery); }; - // Floor of 1 keeps the inner call legal once the budget is spent; the loop condition, not a - // zero-attempt inner call, is what actually stops the retries. - const remaining = () => Math.max(1, budget - sent); + // No floor. A spent budget hands the inner helper 0, which refuses rather than buying one + // 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); // 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() }); for (let attempt = 0; sent < budget; attempt++) { @@ -470,6 +516,8 @@ export async function fetchWithTransientRetry( } 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. + // A budget refusal is not upstream evidence of anything and must stay recognisable. + if (err instanceof SendBudgetExhaustedError) throw err; throw new UpstreamRetryEvidenceError(transientStatuses, err); } } diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 40e7f5bd50..06d1937362 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -224,9 +224,15 @@ import { prepareSameTarget429Wait, sleepWithAbort, TRANSIENT_RETRY_MAX_ATTEMPTS, - createTransientSendBudget, + SendBudgetExhaustedError, type TransientSendBudget, } from "../../lib/upstream-retry"; +import { + createRequestExecutionBudget, + isRequestExecutionBudget, + type SendClass, + type SingleUseDispatchPermit, +} from "../../lib/request-execution-budget"; import { ForwardAdmissionCredentialError, hasForwardableCodexBearer, @@ -1289,6 +1295,8 @@ interface CodexPoolAccountRetryArgs { translatorBudget: TranslatorBudget; turnAdmissionLease?: AdmissionLease; resolveCodexModelEntitlements?: typeof resolveCodexModelEntitlements; + /** The logical request's execution budget: the account move is its fourth send. */ + sendBudget?: TransientSendBudget; }; firstAuthCtx: Extract; firstResponse: Response; @@ -1483,6 +1491,27 @@ async function retryCodexPoolOnAlternateAccount( recordUnmovedTransientOutcome(); return { kind: "no-alternate" }; } + // An account move is the guarded profile's fourth send and draws the single shared + // final-recovery reserve. Nothing bounded it per request before: `excludeAccountId` excludes + // only the account that just failed, and the caller's recovery loop can return here after the + // alternate fails too, so one request could walk the pool an account at a time. The permit is + // consumed immediately before the physical send, so a resolution that finds no alternate + // costs nothing. + const executionBudget = isRequestExecutionBudget(args.options.sendBudget) + ? args.options.sendBudget + : undefined; + let accountMovePermit: SingleUseDispatchPermit | undefined; + if (!retryAuthCtx && executionBudget) { + const decision = executionBudget.reserveDispatch({ + sendClass: "account-failover", + targetKey: `${route.providerName}|${route.modelId}|alternate-account`, + }); + if (!decision.allowed) { + recordUnmovedTransientOutcome(); + return { kind: "no-alternate" }; + } + accountMovePermit = decision.permit; + } try { retryAuthCtx ??= await resolveCodexAuthContext( callerAuthHeaders, @@ -1621,6 +1650,16 @@ async function retryCodexPoolOnAlternateAccount( let upstreamResponse: Response; try { while (true) { + // The same-account gated-model 400 ladder below keeps its own `maxRetrySends` bound and + // does not take the reserve again; only the move itself does. + if (accountMovePermit) { + const charged = accountMovePermit.use(); + accountMovePermit = undefined; + if (!charged) { + recordUnmovedTransientOutcome(); + return { kind: "no-alternate" }; + } + } noteAttemptSend(logCtx.activeAttempt, passthroughEstimate); try { upstreamResponse = await fetchWithHeaderTimeout( @@ -3458,7 +3497,7 @@ export async function handleResponses( translatorBudget, // Created once at genuine ingress; a combo child arrives with the parent's holder already // in options and must not start a fresh allowance. - sendBudget: options.sendBudget ?? createTransientSendBudget(), + sendBudget: options.sendBudget ?? createRequestExecutionBudget(), }); return ownsBudget ? finalizeOwnedTranslatorBudget(response, translatorBudget) : response; } catch (error) { @@ -4989,10 +5028,43 @@ async function handleResponsesInner( // fresh default of 3. It is now a holder carried on options, so a combo child inherits the // parent's spend instead of starting over per target -- both halves of the measured // amplification in #4546. - const sendBudget = options.sendBudget ?? createTransientSendBudget(); + const sendBudget = options.sendBudget ?? createRequestExecutionBudget(); const noteTransientSends = (used: number): void => { sendBudget.used += Math.max(0, used); }; + // No floor. Math.max(1, ...) meant an exhausted request still funded one send on every + // recovery leg, so a bounded per-leg allowance never became a bounded per-request one. const remainingTransientSendBudget = (budget: number): number => - Math.max(1, budget - sendBudget.used); + isRequestExecutionBudget(sendBudget) + ? sendBudget.remainingBaseSends(budget) + : Math.max(0, budget - sendBudget.used); + const sendBudgetExhausted = (): boolean => + remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS) === 0; + /** + * How many sends a recovery leg may make, and the permit that authorises the last one. + * + * The base allowance is spent first. Once it is gone a recovery class may still draw the + * single shared final-recovery reserve -- which is what keeps the validated sanitized rebuild + * after a 5xx streak alive at four total sends -- but an account move and a rebuild cannot + * each take one. `countedExternally` is set because these legs run through the retry helper, + * which reports the same send again through `onSendsConsumed`. + */ + const recoverySendAllowance = ( + cap: number, + sendClass: SendClass, + targetKey: string, + ): { attempts: number; permit?: SingleUseDispatchPermit } => { + const base = remainingTransientSendBudget(cap); + if (base > 0) return { attempts: base }; + if (!isRequestExecutionBudget(sendBudget)) return { attempts: 0 }; + const decision = sendBudget.reserveDispatch({ sendClass, targetKey, countedExternally: true }); + return decision.allowed ? { attempts: 1, permit: decision.permit } : { attempts: 0 }; + }; + /** + * 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 + * mislabelled evidence #4592 existed to stop. + */ + const recoveryClassFor = (recovery: AttemptRecoveryKind): SendClass => + /401|429|oauth|rate-limit|key/.test(recovery) ? "auth-recovery" : "repair"; if ("passthrough" in adapter && adapter.passthrough && !routedCompaction) { let hostAdmissionLease = pendingHostAdmissionLease; @@ -5466,6 +5538,15 @@ async function handleResponsesInner( releaseCodexAuthContextProbeLease(authCtx); return clientCancelledResponse(); } + // A budget refusal is a proxy decision, not an upstream fault. Reporting it as + // 502 upstream_error would blame the provider for a limit this process applied, and + // would record a fake reachability failure against the account's health. + if (err instanceof SendBudgetExhaustedError) { + releaseUpstreamHostAdmission(hostAdmissionLease); + hostAdmissionLease = null; + releaseCodexAuthContextProbeLease(authCtx); + return formatErrorResponse(429, "request_send_budget_exhausted", err.message); + } const localRefusal = mapCodexAuthContextErrorToResponse(unwrapUpstreamRetryEvidenceError(err), { now: Date.now(), accountSelector: route.codexAccountNamespace, }); @@ -5597,8 +5678,21 @@ async function handleResponsesInner( const rebuiltBodyRefusal = refuseOversizedOutboundBody(request); if (rebuiltBodyRefusal) return { failed: rebuiltBodyRefusal }; 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 + // already sent once, and letting the second call through would be a free send. + if (allowance.permit && !allowance.permit.use()) { + throw new SendBudgetExhaustedError(safeHostLabel(request.url)); + } noteAttemptSend(logCtx.activeAttempt, passthroughEstimate, innerRecovery ?? recovery); return fetchWithHeaderTimeout(request.url, applyUpstreamRecoveryInit({ method: request.method, @@ -5616,7 +5710,7 @@ async function handleResponsesInner( route.provider.authMode === "forward") .then(adoptObservedResponse); }, - { abortSignal: upstream.signal, label: safeHostLabel(request.url), attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS), onSendsConsumed: noteTransientSends }, + { abortSignal: upstream.signal, label: safeHostLabel(request.url), attempts: allowance.attempts, onSendsConsumed: noteTransientSends }, ); } catch (err) { return { failed: transportFailureResponse(err) }; @@ -5739,6 +5833,10 @@ async function handleResponsesInner( && isOAuth401ReplayProvider && sentOAuthSnapshot && !oauth401ReplayAttempted + // Refused here, before the 401 body is cancelled: once it is gone the request can only + // answer with a synthetic 502, which would report a proxy budget decision as an upstream + // fault and throw away the credential evidence the client needs. + && !sendBudgetExhausted() ) { oauth401ReplayAttempted = true; try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed/closed */ } @@ -5889,6 +5987,10 @@ async function handleResponsesInner( upstreamResponse.status === 429 && rateLimitPolicy !== null && rateLimitRetries < rateLimitPolicy.attempts + // Checked here rather than inside the helper: prepareSameTarget429Wait releases the 429 + // body, so a refusal discovered after the wait can no longer return the real rate-limit + // answer and would surface a synthetic 502 instead. + && !sendBudgetExhausted() ) { rateLimitRetries += 1; // Release unread body + deliberate wait via the shared same-target helper. @@ -7653,7 +7755,13 @@ async function handleResponsesInner( abortSignal: upstream.signal, label: safeHostLabel(builtInitialRequest.url), ...(transientPolicy - ? { attempts: transientPolicy.attempts, onSendsConsumed: noteTransientSends } + // Draws the remainder, not the raw policy. A combo child inherits the parent's + // holder but used to take a fresh full allowance on its own first send, so the + // shared counter was inherited without ever being read as a limit. + ? { + attempts: remainingTransientSendBudget(transientPolicy.attempts), + onSendsConsumed: noteTransientSends, + } : {}), }, ); @@ -7762,8 +7870,22 @@ async function handleResponsesInner( const refetchWithPolicy = (route.provider.adapter === "google" || refetchTransientPolicy) ? fetchWithTransientRetry : fetchWithResetRetry; + // Same rule as the passthrough rebuild: spend the base allowance first, then the one + // shared final-recovery reserve, so a recovery that follows a spent streak still gets + // its single send instead of dying at three. + const refetchAllowance = refetchTransientPolicy + ? recoverySendAllowance( + refetchTransientPolicy.attempts, + recoveryClassFor(recovery), + `${route.providerName}|${route.modelId}|${recovery}`, + ) + : undefined; return await refetchWithPolicy( - recoveryKind => fetchWithHeaderTimeout(retryRequest.url, + 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, @@ -7771,13 +7893,14 @@ async function handleResponsesInner( dispatchOverride: oauthDispatch(retryRequest), providerName: route.providerName, modelId: route.modelId, - })), + })); + }, { abortSignal: upstream.signal, label: safeHostLabel(retryRequest.url), - ...(refetchTransientPolicy + ...(refetchAllowance ? { - attempts: remainingTransientSendBudget(refetchTransientPolicy.attempts), + attempts: refetchAllowance.attempts, onSendsConsumed: noteTransientSends, } : {}), @@ -7803,6 +7926,7 @@ async function handleResponsesInner( && isOAuth401ReplayProvider && sentOAuthSnapshot && !oauth401ReplayAttempted + && !sendBudgetExhausted() ) { oauth401ReplayAttempted = true; try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed/closed */ } @@ -7897,6 +8021,7 @@ async function handleResponsesInner( upstreamResponse.status === 429 && rateLimitPolicy !== null && rateLimitRetries < rateLimitPolicy.attempts + && !sendBudgetExhausted() ) { rateLimitRetries += 1; // Release unread body + deliberate wait via the shared same-target helper. diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index 9fb26bac23..1e7f2a1f10 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -27,13 +27,17 @@ describe("transient send budget stays request-scoped", () => { // One holder per LOGICAL request, read before any leg can send and inherited by combo // children through the options spread rather than recreated per child turn. - expect(core.match(/const sendBudget = options\.sendBudget \?\? createTransientSendBudget\(\);/g)) + expect(core.match(/const sendBudget = options\.sendBudget \?\? createRequestExecutionBudget\(\);/g)) .toHaveLength(1); // Genuine ingress mints it; a child arrives with the parent's and must not replace it. - expect(core).toContain("sendBudget: options.sendBudget ?? createTransientSendBudget(),"); + expect(core).toContain("sendBudget: options.sendBudget ?? createRequestExecutionBudget(),"); // The regressed shape: a counter local to one call frame, which a combo child restarts. expect(core).not.toContain("let transientSendsUsed = 0;"); expect(core.match(/const remainingTransientSendBudget = \(budget: number\): number =>/g)).toHaveLength(1); + // Zero has to mean zero. The Math.max(1, ...) floor funded one more send on every recovery + // leg, which is most of how a bounded per-leg allowance composed into an unbounded + // per-request count (#4546 REQ-B04). + expect(core).not.toContain("Math.max(1, budget - sendBudget.used)"); // Seven legs report into the same counter: the adapter initial send, the 429/rotation // refetch, the terminal-guard continuation, and the four Codex passthrough sends (initial, @@ -42,11 +46,19 @@ describe("transient send budget stays request-scoped", () => { // dead zone there, so each of those legs silently took the helper's fresh default of 3. expect(core.match(/onSendsConsumed: noteTransientSends/g)).toHaveLength(7); - // Every leg except the adapter initial send must ask for the REMAINDER. Only that one may - // pass a policy value directly, because nothing has been spent yet. - expect(core.match(/attempts: remainingTransientSendBudget\(/g)).toHaveLength(6); - expect(core).toContain("attempts: remainingTransientSendBudget(refetchTransientPolicy.attempts)"); + // EVERY leg asks for the remainder now, including the adapter initial send. That one used + // to pass the raw policy on the argument that nothing had been spent yet -- true for a first + // turn, false for a combo child, which inherits the parent's holder and then took a fresh + // full allowance on its own first send. Five sites spell it directly; the two rebuild legs + // go through recoverySendAllowance, which spends the base allowance first and only then + // draws the single shared final-recovery reserve. + expect(core.match(/attempts: remainingTransientSendBudget\(/g)).toHaveLength(5); + expect(core).toContain("attempts: remainingTransientSendBudget(transientPolicy.attempts)"); expect(core).toContain("attempts: remainingTransientSendBudget(continuationTransientPolicy.attempts)"); + // The reserve path: an account move and a validated rebuild share ONE final send, so a + // request cannot take both and reach five. + expect(core.match(/recoverySendAllowance\(/g)).toHaveLength(2); + expect(core).toContain("countedExternally: true"); // The passthrough legs have no adapter policy to draw from, so they name the helper's own // ceiling rather than re-spelling the number. expect(core).toContain("attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS)"); @@ -57,6 +69,7 @@ describe("transient send budget stays request-scoped", () => { // The regressed shape: a leg handing itself a fresh full budget. expect(core).not.toContain("attempts: continuationTransientPolicy.attempts }"); expect(core).not.toContain("attempts: refetchTransientPolicy.attempts }"); + expect(core).not.toContain("attempts: transientPolicy.attempts,"); }); test("the helper still exposes the seam those call sites depend on", () => { @@ -64,5 +77,10 @@ describe("transient send budget stays request-scoped", () => { expect(retry).toContain("onSendsConsumed?: (sends: number) => void;"); // Reported in `finally` so every exit path — return, throw, abort — feeds the counter. expect(retry).toMatch(/} finally \{\n\s*opts\.onSendsConsumed\?\.\(sent\);/); + // A spent budget must refuse rather than round itself up to one more send. + expect(retry).not.toContain("Math.max(1, opts.attempts ?? RESET_RETRY_MAX_ATTEMPTS)"); + expect(retry).not.toContain("Math.max(1, opts.attempts ?? TRANSIENT_RETRY_MAX_ATTEMPTS)"); + expect(retry).not.toContain("Math.max(1, budget - sent)"); + expect(retry).toContain("class SendBudgetExhaustedError extends Error"); }); });