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 1502d6f453..c62836c01c 100644 --- a/devlog/_plan/260914_cost_guard_stabilization/040_send_budget.md +++ b/devlog/_plan/260914_cost_guard_stabilization/040_send_budget.md @@ -117,6 +117,23 @@ The shape to build, in order: Out of scope and worth stating: a client that re-sends on its own is not bounded by any of this. That needs a logical-request identity shared with the client. +Verification is hosted CI only, as for the rest of this unit. The regression that +## Step 0 status + +Landed. The owner turned out to live in `handleResponsesInner`, not the `handleResponses` +wrapper, and the four passthrough sends sit inside the same outer try -- so the declaration was +in the temporal dead zone for them and a reference-only change would have thrown at runtime. +The fix hoists the three bindings above the passthrough branch and wires all four sends with +`attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS)` and `onSendsConsumed`. + +The trap an audit round caught before it was written: do NOT copy the adapter's +`transientRetryPolicyFor(...) ? ... : {}` gate onto these sites. That function returns null for +Codex forward auth, so the copy would have made the whole change a silent no-op. + +Consequence to expect in the logs: an initial 401 now spends one of the three, so a later 5xx +streak on the refresh leg gets two rather than a fresh three. Combo stays at 12 until the budget +rides `HandleResponsesOptions`, because each child runs its own `handleResponsesInner`. + Verification is hosted CI only, as for the rest of this unit. The regression that matters is a table test: for each failure shape (5xx streak, 401-then-5xx, combo fan-out), assert the exact number of upstream sends, because the defect is a count. diff --git a/src/lib/upstream-retry.ts b/src/lib/upstream-retry.ts index afdbbcf3ae..dbeb846da3 100644 --- a/src/lib/upstream-retry.ts +++ b/src/lib/upstream-retry.ts @@ -55,7 +55,8 @@ const RESET_RETRY_BASE_DELAY_MS = 150; const RESET_RETRY_MAX_DELAY_MS = 1_000; // Transient-5xx status retry layer (pre-stream only; devlog/_plan/260716_claudecode_hardening/010). -const TRANSIENT_RETRY_MAX_ATTEMPTS = 3; // 1 initial + 2 retries +/** Total sends one transient-retry helper call may make: 1 initial + 2 retries. */ +export const TRANSIENT_RETRY_MAX_ATTEMPTS = 3; 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 diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index ec1b583ed3..aa441e6b7d 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -223,6 +223,7 @@ import { isTransientUpstreamStatus, prepareSameTarget429Wait, sleepWithAbort, + TRANSIENT_RETRY_MAX_ATTEMPTS, } from "../../lib/upstream-retry"; import { ForwardAdmissionCredentialError, @@ -4971,6 +4972,16 @@ async function handleResponsesInner( routedMuseToolNameAliases = builtRequest.convertedMuseToolNameAliases ?? new Map(); }; + // One request-scoped transient-retry budget owner, declared ABOVE the passthrough branch so + // that branch shares it too. It used to sit below, which put it in the temporal dead zone for + // the passthrough sends and left each recovery leg taking the helper's fresh default of 3 -- + // the source of the measured amplification in #4546. A per-leg budget lets a request that + // recovers several times multiply upstream load. + let transientSendsUsed = 0; + const noteTransientSends = (used: number): void => { transientSendsUsed += Math.max(0, used); }; + const remainingTransientSendBudget = (budget: number): number => + Math.max(1, budget - transientSendsUsed); + if ("passthrough" in adapter && adapter.passthrough && !routedCompaction) { let hostAdmissionLease = pendingHostAdmissionLease; pendingHostAdmissionLease = null; @@ -5513,7 +5524,7 @@ async function handleResponsesInner( // retry wrapper replaces — proves the host was reached (#914 review). .then(adoptObservedResponse); }, - { abortSignal: upstream.signal, label: safeHostLabel(request.url) }, + { abortSignal: upstream.signal, label: safeHostLabel(request.url), attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS), onSendsConsumed: noteTransientSends }, ); } catch (err) { return transportFailureResponse(err); @@ -5593,7 +5604,7 @@ async function handleResponsesInner( route.provider.authMode === "forward") .then(adoptObservedResponse); }, - { abortSignal: upstream.signal, label: safeHostLabel(request.url) }, + { abortSignal: upstream.signal, label: safeHostLabel(request.url), attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS), onSendsConsumed: noteTransientSends }, ); } catch (err) { return { failed: transportFailureResponse(err) }; @@ -5813,7 +5824,7 @@ async function handleResponsesInner( route.provider.authMode === "forward") .then(adoptObservedResponse); }, - { abortSignal: upstream.signal, label: safeHostLabel(request.url) }, + { abortSignal: upstream.signal, label: safeHostLabel(request.url), attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS), onSendsConsumed: noteTransientSends }, ); } catch (err) { return transportFailureResponse(err); @@ -5910,7 +5921,7 @@ async function handleResponsesInner( route.provider.authMode === "forward") .then(adoptObservedResponse); }, - { abortSignal: upstream.signal, label: safeHostLabel(request.url) }, + { abortSignal: upstream.signal, label: safeHostLabel(request.url), attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS), onSendsConsumed: noteTransientSends }, ); } catch (err) { return transportFailureResponse(err); @@ -7551,13 +7562,6 @@ async function handleResponsesInner( notifyResponseComplete(json); return new Response(JSON.stringify(json), { headers: { "Content-Type": "application/json" } }); } - // One request-scoped transient-retry budget owner, declared here so BOTH the initial send - // and the later recovery refetches (429, key/account rotation, OAuth replay) share it. A - // per-leg budget would let a request that recovers several times multiply upstream load. - let transientSendsUsed = 0; - const noteTransientSends = (used: number): void => { transientSendsUsed += Math.max(0, used); }; - const remainingTransientSendBudget = (budget: number): number => - Math.max(1, budget - transientSendsUsed); try { initialRequest = await activeAdapter.buildRequest(parsed, { headers: selectedForwardHeaders, translatorBudget }); refreshRequestToolAliases(initialRequest); diff --git a/tests/lib/transient-budget-scope-source.test.ts b/tests/lib/transient-budget-scope-source.test.ts index e693deb0ff..66ee919b66 100644 --- a/tests/lib/transient-budget-scope-source.test.ts +++ b/tests/lib/transient-budget-scope-source.test.ts @@ -29,15 +29,24 @@ describe("transient send budget stays request-scoped", () => { expect(core.match(/let transientSendsUsed = 0;/g)).toHaveLength(1); expect(core.match(/const remainingTransientSendBudget = \(budget: number\): number =>/g)).toHaveLength(1); - // Initial send, 429/rotation refetch, and terminal-guard continuation: three legs, three - // reports into the same counter. - expect(core.match(/onSendsConsumed: noteTransientSends/g)).toHaveLength(3); + // 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, + // rebuild refetch, OAuth 401 replay, rate-limit 429 replay). The passthrough four were added + // for #4546: the owner used to be declared BELOW that branch, which put it in the temporal + // 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); - // The refetch and continuation legs must ask for the REMAINDER. Only the initial send may + // 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(2); + expect(core.match(/attempts: remainingTransientSendBudget\(/g)).toHaveLength(6); expect(core).toContain("attempts: remainingTransientSendBudget(refetchTransientPolicy.attempts)"); expect(core).toContain("attempts: remainingTransientSendBudget(continuationTransientPolicy.attempts)"); + // 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)"); + // The trap that would make the passthrough wiring a silent no-op: transientRetryPolicyFor + // returns null for Codex forward auth, so gating these sites on it would restore a fresh 3. + expect(core).not.toContain("transientPolicy ? { attempts: remainingTransientSendBudget(TRANSIENT_RETRY_MAX_ATTEMPTS)"); // The regressed shape: a leg handing itself a fresh full budget. expect(core).not.toContain("attempts: continuationTransientPolicy.attempts }"); diff --git a/tests/responses/responses-opaque-blob-recovery.test.ts b/tests/responses/responses-opaque-blob-recovery.test.ts index c2a5c8e262..a47a155f38 100644 --- a/tests/responses/responses-opaque-blob-recovery.test.ts +++ b/tests/responses/responses-opaque-blob-recovery.test.ts @@ -617,7 +617,11 @@ describe("opaque blob recovery through /v1/responses", () => { const body = await response.json() as { error?: { message?: string } }; expect(body.error?.message).toBe(FUNCTION_OUTPUT_DECRYPT_MESSAGE); - expect(outbound).toHaveLength(6); + // Three sends spend the request's transient budget, then the sanitized rebuild draws on what + // is LEFT of that same budget rather than a fresh allowance, so it sends once and stops. + // This used to be 6 (3 + 3), which is the per-leg multiplication #4546 measured. + expect(outbound).toHaveLength(4); + expect(logCtx.activeAttempt?.sendCount).toBe(4); const initialInput = outbound.at(0)?.input as Array> | undefined; const finalInput = outbound.at(-1)?.input as Array> | undefined; expect(initialInput?.at(1)).toEqual(functionOutputReplayInput().at(1));