From 1583be7e4af640efb6b01f31e364909198017577 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 16:46:34 +0900 Subject: [PATCH 1/3] fix(responses): share one transient send budget with the Codex passthrough (#4546) The budget owner was declared below the passthrough branch, so it was in the temporal dead zone for those four sends and each took the helper fresh default of 3. Hoisting it above the branch and wiring the sends makes one logical request share one transient budget across its recovery legs. The cross-account alternate is untouched because it does not go through the helper, so the 3+1 recovery shape is preserved. --- .../040_send_budget.md | 17 ++++++++++++ src/lib/upstream-retry.ts | 3 ++- src/server/responses/core.ts | 26 +++++++++++-------- 3 files changed, 34 insertions(+), 12 deletions(-) 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); From d50276fe79840da8eb05427c7abde05748f14649 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 16:49:15 +0900 Subject: [PATCH 2/3] test(responses): pin the shared transient budget across a sanitized rebuild The repeated function-output decrypt case sent 6 times (3 on the first leg, a fresh 3 on the rebuild). With the budget shared it sends 4: the rebuild draws on what is left rather than a new allowance. That count is the regression for #4546. --- tests/responses/responses-opaque-blob-recovery.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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)); From e4b7110568a8a3f63ea767be099bc1e98974a954 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 16:55:45 +0900 Subject: [PATCH 3/3] test(lib): pin the passthrough legs into the shared-budget source oracle The oracle asserted exactly three legs report into the counter. The four Codex passthrough sends now do too, and the oracle names them plus the transientRetryPolicyFor gate that would silently restore a fresh allowance. --- .../lib/transient-budget-scope-source.test.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) 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 }");