[WRONG BRANCH] fix(responses): report a spent send budget as this proxy refusing (#4708) - #4758
Conversation
) [skip ci] When a request exhausts its shared send budget the three dispatch paths disagreed about what the client was told. Passthrough returned 429 and explicitly declined to blame the provider. The adapter paths did not special case SendBudgetExhaustedError: they fell through describeUpstreamConnectFailure and answered 502 "Provider unreachable" for a refusal this process made itself. The runTurn path pushed an unstructured error event, which is inferred back to 502 and delivered under HTTP 200. The status is the load-bearing half, and it is worse than a mislabel. The Codex client retries 5xx and does not retry a direct 429, so telling it the provider broke makes it send the whole turn again -- the amplification this budget exists to stop. Reporting the refusal as a quota code would stop the client for a reason that is not true, and the retryable streaming rate-limit codes would restart the stream, so neither is available. Both adapter catch sites now answer 429 before describeUpstreamConnectFailure can launder the refusal, and runTurn emits it as a structured terminal event with its status, type and code on the event itself, because an unstructured message is inferred back to 502. classifyError keeps the distinct code by matching the supplied type rather than the status. An upstream 429 still classifies as rate_limit_exceeded; only this proxy's own refusal carries request_send_budget_exhausted. Before this the passthrough path asked for that code and the classifier overwrote it, so even the one path that got the status right could not be told apart afterwards. A local 429 must also not look like a provider one to our own routing. rotateRunTurnAdapterOnPreflight429 returns early on the code, before it reads the status, so a refusal cannot rotate a credential or write a cooldown against an account that rate-limited nothing -- a fake quota signal that outlives the request and misroutes later ones. The terminal-guard continuation loop never consulted sendBudgetExhausted() while the main recovery loop did, so a spent budget could still same-key 429-replay on a live stream. It is checked before the wait cancels the upstream body, so a refusal keeps the real 429 with its Retry-After and quota evidence intact. Upstream classification of a provider 429 as org or project spend exhaustion is a separate contract and is not touched here. Closes #4708
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 76 / 80이 PR은 이슈 #4708을 고친다. 요청의 공유 send budget이 바닥났을 때, 세 갈래 디스패치가 클라이언트에게 서로 다른 답을 주고 있었다. 현재 고치는 축은 네 곳이다. (1) 검증 쪽은 로컬 full suite/typecheck/build를 돌리지 말라는 레인 규칙을 지켰다. 대신 베이스는 라인 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…4546) [skip ci] Carried from PR #4717. The Command Code reasoning-effort retry commit is left behind: that retry stays on the same selected key, so it is not needed to tell account A's usage from account B's, and keeping it out keeps this layer reviewable. The injected-executor fix it depended on is already here. Adds the consumer-side assertion the attribution depends on. A row carries BOTH the per-attempt records and the request total, and a reader that added them would report 600 input tokens for 300 that were actually spent. usageAttributions takes the attempts when a row has them and the entry row only when it has none, so the parent total is a fallback for rows written before attempts existed rather than another column to sum. That arithmetic is also why hidden attempts must not be folded into the response the client sees: the Codex client treats response.completed.usage as the exact usage for that response and adds it to its durable turn and thread totals, so a proxy-side sum would corrupt accounting it owns. Closes #4717 Co-authored-by: thisisjun786 <259586770+thisisjun786@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01a2b1f0cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| status: 429, | ||
| errorType: "rate_limit_error", | ||
| code: SEND_BUDGET_EXHAUSTED_CODE, |
There was a problem hiding this comment.
When a combo target uses a runTurn adapter and its send budget throws here, both comboAttempt branches later in this function discard the new status, errorType, and code: lines 337–343 and 433–439 still return formatErrorResponse(502, "upstream_error", ...). Since core-combo.ts invokes every child with comboAttempt: true, a spent budget can therefore still be reported as the retryable provider failure this change is intended to eliminate. Convert the preflight event through adapterFailureFromEvent or otherwise preserve its structured 429 fields, and cover an actual combo/runTurn refusal rather than only testing the event helper.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| // is inferred back to 502, which the Codex client retries. | ||
| ? { | ||
| type: "error", | ||
| status: 429, |
There was a problem hiding this comment.
For a non-combo stream: false request handled by a runTurn adapter, this 429 is only stored on the queued event. The buffered path passes the event through buildResponseJSON, but then constructs the HTTP response with new Response(...) and no status override, so the client still receives HTTP 200 rather than the documented direct 429. Derive the buffered response status from the terminal error event, using the same adapterFailureFromEvent mapping, so the newly added status affects the wire response.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
…later (#4546) [skip ci] The transient-hold resolver and the pool-wide recovery limiter added in #4626 still have no production caller, so #4701 is not closed here. Wiring them turned up a defect in the thing being wired, and that has to be fixed first. A withheld dispatch promises the caller a retry time. It was computed from the probe pacing alone. When the RATIO limiter is what refused, the account usually has no probe state at all -- nothing was ever granted for it -- so nextProbeAt returned now, and the refusal told the caller to try again immediately. A withheld dispatch that busy-loops puts the same load on an already-failing pool as the dispatch it refused, which is the opposite of what the limiter is for. It also violates the Retry-After half of #4701's completion criteria directly. The limiter is the only thing that knows when its own window moves, so it now says: nextRecoveryAt returns now while the allowance is unspent, and otherwise the moment the oldest bucket still inside the window falls out. Every such bucket started after now - windowMs, so the answer is always strictly in the future, and it is a real change point rather than a guessed delay. The withheld result takes the later of that and the probe pacing. The existing zero-allowance test asserted only that the result was withheld, which is why the defect survived the unit suite that was written to cover this module. It now asserts the time as well. Refs #4701
…#4546) Each layer of this lane closes one seam of the #4546 amplification: the credential hop that was charged twice, the spend ledger with no caller, the refusal reported as a provider fault, the usage attributed to the wrong key, the withheld recovery that said "retry now". What none of them checks is whether the seams agree with each other. This composes the real primitives -- the request execution budget, the durable spend ledger with its request-scoped caller, the pool recovery limiter -- and asserts that the numbers describe the same events: physical sends, budget consumption, ledger reservation and settlement, and the refusal the caller is given. The scenarios are the incident's own: a request whose every layer tries to recover, concurrent requests contending for one process-wide recovery allowance, a caller that keeps its detour instead of adding a second trial to a failing account, a fan-out child spending the parent's allowance rather than a fresh one, and a restart that must neither reset a ceiling nor settle the same send twice. A fixture that only counted sends would have passed throughout the incident, which is why every case ties a send count to the spend the ledger recorded for it.
The account-change scenario the incident needs, written against current behaviour because the #4710 refusal is owned by another lane and is not in this stack yet. What it pins now: continuation state is dropped and the turn continues, an uploaded file reference is classified non-portable and is NOT removed by the scrub, and the carriers must be read directly because the portability verdict reports only the first reason it finds -- a body carrying both a response id and a file reports the response id. What it documents: once the refusal lands, that body must be declined before dispatch and the refusal must win over the response id. The two properties above are what the change has to preserve, so they are asserted now. Also pins the accounting invariant that refusal owes: a decision made before dispatch spends no send and books no ledger entry. A refusal counted as a send would appear as provider load that never happened and would push a healthy account toward a cooldown.
…fusing sends (#4546) Four fixes, batched into one push so the queue only pays once. 1. src/server/responses/core.ts was 214 lines against a 210-line cap in tests/fixtures/file-size-baseline.json. The spend-observer wiring added four lines of comment and continuation. The comment is now one line and the expression one line, and the file is back at its cap. The ratchet only ever lowers caps, so growing past one is a hard failure rather than a nudge. 2. The spend tracker refused a dispatch on ANY ledger denial. Only an operator's configured ceiling should: capacity, durability and a journal this process could not prove complete all mean the ledger cannot ACCOUNT for the send, which is not a reason to refuse one. An unconfigured install keeps the count caps it already had and is not newly refused, and a degraded ledger must not become an outage. 3. The shared ledger is now resolved on the first charge rather than when the request is built. It opens a journal under the OpenCodex home, and a request that never dispatches has no business creating one; this also means the home in effect at dispatch is the one written to, instead of whichever home was current when the first request of the process happened to be constructed. 4. Three assertions in the new tests claimed states the code never reaches. The concurrent-probe case asserted a limiter refusal, but the second caller short-circuits on the lease before it reaches the limiter and costs no allowance; the shared bound is now proved by asking the limiter directly. The exhausted-ceiling case asserted final-recovery-spent where the total ceiling refuses first, so it asserts total-exhausted and checks reserveSpent separately for the point it was making. The unstructured-error control asserted an exact 502 where the property that matters is that the identity is gone, so it asserts that instead. Tests are not typechecked -- tsconfig includes only src -- so a test that asserts the opposite of what it claims passes silently. These were found by reading, not by running.
Two source-of-truth failures from the previous tip run, both mine. tests/lib/transient-budget-scope-source.test.ts pinned the exact core.ts line that mints the request's send budget, and bl2 changed it to install the spend observer. The oracle now matches the new shape and additionally asserts the observer is attached at the same place, which is the property that actually matters: a combo child inherits the parent's holder and must not open a second set of ledger entries for the same physical sends. tests/lib/spend-reservation-ledger.test.ts caught a real defect in the replay reconciliation, not a stale expectation. An exhausted scope must still be exhausted after a restart -- that is the whole reason the ledger is on disk -- and abandoning a replayed undispatched reservation handed its tokens back and reset the ceiling. The distinction I drew was wrong. "Open" does not prove nothing was sent: the torn-tail rule immediately above says the journal may be missing its last record, so a send can dispatch and die before its dispatch record lands. Both live states now resolve to unresolved spend, which is the conservative answer and the one that preserves the ceiling. The bl2 wiring test asserted the old split and is updated to the new figures, along with the structure contract and the tracker's own comment.
…spatch fix(routing): give a withheld recovery a retry time that is actually later (#4546)
fix(usage): attribute each attempt to the account that dispatched it (#4717)
|
Cascading downward. Internal budget exhaustion no longer leaves as Evidence at the verified tip d9e5b28 (tree
Chained-child stacks merge top-down, so this lands in the parent branch and cascades to Maintainer integration decision under MAINTAINERS.md / AGENTS.md: a maintainer with maintain or admin access may integrate into |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
2 participants
Summary
When a request exhausts its shared send budget, the three dispatch paths disagreed about what the client was told. Passthrough returned 429 and explicitly declined to blame the provider. The adapter paths did not special-case
SendBudgetExhaustedError: they fell throughdescribeUpstreamConnectFailureand answered502 upstream_error— "Provider unreachable" — for a refusal this process made itself. The runTurn path pushed an unstructured error event, which is inferred back to 502 and delivered under HTTP 200.The status is the load-bearing half, and the 502 is worse than a mislabel. The Codex client retries 5xx and does not retry a direct 429 (
retry_429is false in its provider policy), so telling it the provider broke makes it send the whole turn again — the amplification this budget exists to stop. Reporting the refusal as a quota code would stop the client for a reason that is not true, and the retryable streaming rate-limit codes (rate_limit_exceeded,slow_down) would restart the stream, so neither is available. 429 with a distinct code is the only option that both halts the client and stays honest.Both adapter catch sites now answer 429 before
describeUpstreamConnectFailurecan launder the refusal, and runTurn emits it as a structured terminal event carrying its status, type and code on the event itself, because an unstructured message is inferred back to 502.classifyErrorkeeps the distinct code by matching the supplied type rather than the status, so an upstream 429 still classifies asrate_limit_exceededand only this proxy's own refusal carriesrequest_send_budget_exhausted. Before this, the passthrough path asked for that code and the classifier overwrote it, so even the one path that got the status right could not be told apart afterwards.A local 429 must also not look like a provider one to our own routing.
rotateRunTurnAdapterOnPreflight429returns early on the code, before it reads the status, so a refusal cannot rotate a credential or write a cooldown against an account that rate-limited nothing — a fake quota signal that outlives the request and misroutes later ones.The terminal-guard continuation loop never consulted
sendBudgetExhausted()while the main recovery loop did, so a spent budget could still same-key 429-replay on a live stream. It is now checked before the wait cancels the upstream body, so a refusal keeps the real 429 with itsRetry-Afterand quota evidence intact.Out of scope, deliberately: classifying an upstream 429 as org or project spend exhaustion is a separate contract with a separate owner; nothing in
src/codex/quota-rejection.tsis touched here.Closes #4708
Verification
No local suite, focused test, typecheck, install, or build step was run. The repository owner prohibits local suite execution in this lane after a past local run deleted real user home data. Verification is static reading plus hosted CI.
Static checks performed:
retry_429: falsewhile retrying 5xx and transport failures, and a non-quota HTTP 429 maps to a terminalRetryLimitthat is explicitly non-retryable. That is why 502 caused a client resend and 429 does not.classifyError's ordering: the new branch is keyed on the suppliedtypeand placed before the status-429 branch, so an upstream 429 arriving with any other type is unaffected. Verified no test or source assertedrate_limit_exceededfor the passthrough budget refusal, so nothing depended on the old collapsed code.adapterFailureFromEvent:event.codeoverrides the classified code andevent.errorTypeoverrides the type, so the structured runTurn event produces exactly 429 /rate_limit_error/request_send_budget_exhausted.prepareSameTarget429Wait, which is what cancels the retained upstream body.scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json, and updatedstructure/transports/responses.md.Regression coverage added —
tests/responses/responses-send-budget-errors.test.ts:rate_limit_exceeded;describeUpstreamConnectFailurecall;Hosted CI: non-tip layer of a stacked lane, carrying
[skip ci]under the maintainer-approved DEV-STACK-08 tip-only CI policy. The lane's CI gate runs on the tip branch.Checklist