-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(responses): share one transient send budget with the Codex passthrough (#4546) #4605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1583be7
d50276f
e4b7110
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Comment on lines
4972
to
+4983
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes the shared Responses transport contract by making Codex passthrough recovery legs consume one request-scoped transient-send allowance, but the commit updates only the devlog and leaves the applicable AGENTS.md reference: src/AGENTS.md:L10-L11 Useful? React with 👍 / 👎. |
||
|
|
||
| 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 }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Running AGENTS.md reference: src/AGENTS.md:L24-L26 Useful? React with 👍 / 👎. |
||
| ); | ||
| } 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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||||||
|
Comment on lines
+35
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Correct the temporal-dead-zone explanation. A read of a Proposed correction- // 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.
+ // for `#4546`: the passthrough branch did not use the request-scoped budget, so each
+ // of those legs used the helper's fresh default of 3.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| 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 }"); | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the incomplete verification sentence before the heading.
Line 120 ends with
The regression that, then Line 121 starts## Step 0 statuswithout a blank line. This produces malformed prose and triggers MD022. Remove the duplicate fragment or complete it, then leave a blank line before the heading.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 121-121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Source: Linters/SAST tools