[WRONG BRANCH] feat(responses): give the durable spend ledger a production caller (#4707) - #4756
Conversation
…4707) [skip ci] The #4546 work added a durable spend-reservation ledger so token, identity and pool ceilings survive a restart, and nothing in production reached it. The only call to admitWorkflowTurn() omitted its spend argument, so sharedSpendLedger() was never constructed, spend-ledger.jsonl was never created by ordinary traffic, and markDispatched, settle and abandon had no production caller at all. The ceilings the feature advertised stayed process-local and count-only. request-spend.ts is that caller. It books by observing the request's own send counter rather than by being called from each dispatch site: that counter moves exactly once per physical send -- a reservation increments it, a refund decrements it, and an externally reported send settles against a booking already counted -- so one entry per increment is one entry per send. A dispatch path added later cannot forget to book, which is the failure mode that produced an uncalled feature the first time. The observer may refuse. A ledger limit that could only describe a send after the fact would be no ceiling at all, so reserveDispatch consults it last, after every cheaper bound has passed, and a refusal denies the dispatch as spend-exhausted. Consulting it last matters because it is the only bound here that writes: an entry booked for a dispatch some other check would have refused is spend the request never makes, held against the scope until retention expires. A booking is confirmed dispatched only once a later send exists, because that later send proves the earlier one left. The newest stays open, so a reservation the budget hands back is still released for free. The cost is bounded and stated: a hard crash between reserving and sending replays as abandoned rather than unresolved, for at most one send per request. Settlement follows what the request learned. addFinalRequestLog is the one seam every request passes exactly once, whatever transport served it and however it ended, and the terminal usage is already known there. That usage belongs to the last send that left, so it settles with the real figure; every earlier send failed without reporting usage of its own and may still have been billed, so it becomes unresolved spend rather than free. A request that reports no usage at all -- a cancel, a lost stream -- leaves all of them unresolved. The ledger also now resolves what replay leaves behind. A reservation that survives restart has no owner: nothing in the new process can settle it, and leaving it live holds its tokens against the scope forever, which is a ceiling that only ever tightens. Deleting the entry is not the alternative, because that would hand the same send id a second reservation. An undispatched reservation never reached the wire and is abandoned; a dispatched one may already have been billed and becomes unresolved. Both are journaled, so a second restart has nothing left to redo. The reservation uses the caller's max_output_tokens as its output ceiling, captured in request-prepare before any body exists. A caller that omits it leaves the provider/model default in charge and reserves only the input estimate; settlement then books the real figure, so the gap is a looser bound up front rather than a wrong one after. The identity scope is the privacy-safe account label the request log already uses, and the ledger aliases it again on the way to disk, so no raw credential reaches either. The default policy still sets no token ceiling on any scope, so an unconfigured install accounts and reports without refusing anything. The operator configuration path for those limits is deliberately not in this change. Closes #4707
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. |
|
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 |
리뷰 · 우선순위 76 / 80이 PR은 이슈 #4707이 지적한 구멍을 메운다. #4546에서 durable spend-reservation ledger(reserve / markDispatched / settle / abandon / markLost)는 이미 들어갔는데, 생산 경로가 한 번도 닿지 않았다. 현재 dev HEAD 이번 배선은 HTTP 진입의 정착 지점은 재시작 구멍도 막았다. 베이스는 라인 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
) [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
![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: a6b9eca585
ℹ️ 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".
| const confirmOlderSends = (): void => { | ||
| for (let index = 0; index < live.length - 1; index += 1) ledger.markDispatched(live[index] as string); |
There was a problem hiding this comment.
Because only older bookings are marked dispatched, the newest—and therefore every ordinary single-send request—remains open for its entire upstream lifetime. If the process exits after that send reaches the provider but before final logging settles it, replay treats it as undispatched and abandons the reservation, returning potentially billed tokens and resetting the durable ceiling. This also breaks the existing focused check: bun test tests/lib/spend-reservation-ledger.test.ts fails at the torn-tail restart assertion because the replayed reservation is no longer exhausted. Mark the booking at the actual dispatch boundary while retaining explicit release() for reservations that never send.
AGENTS.md reference: src/AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
| sendBudget: options.sendBudget | ||
| ?? createRequestExecutionBudget(undefined, undefined, attachRequestSpendTracker(req, logCtx)), |
There was a problem hiding this comment.
For /v1/responses/compact, compact.ts creates its own sendBudget before making native compact attempts and passes that same holder into the routed handleResponses fallback. This conditional therefore never attaches a tracker: the native sends charged at compact.ts:892/942 and any routed fallback sends at compact.ts:1281 produce no durable reservations and cannot be constrained by the ledger. Attach the observer when the compact request creates its ingress budget, or allow an existing budget to acquire the tracker without resetting its counters.
Useful? React with 👍 / 👎.
| sendBudget: options.sendBudget | ||
| ?? createRequestExecutionBudget(undefined, undefined, attachRequestSpendTracker(req, logCtx)), |
There was a problem hiding this comment.
For a combo request, the tracker closes over the outer logCtx, which executeComboResponses changes to provider "combo"; child calls inherit options.sendBudget, so this conditional never installs a tracker over their target-specific childLog. Consequently combo reservations omit the selected account identity, use combo instead of the actual provider pool, and can reserve zero tokens because input/output estimates are populated on the child context. This corrupts ledger attribution and lets identity or provider-pool ceilings be bypassed by routing through a combo; the observer needs per-send target context rather than the fixed outer log context.
Useful? React with 👍 / 👎.
…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>
…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)
…ntics fix(responses): report a spent send budget as this proxy refusing (#4708)
|
Cascading downward. The durable spend ledger gains a production caller by observing the request budget's send counter, so a dispatch path added later cannot skip its booking. 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 |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
2 participants
Summary
The #4546 work added a durable spend-reservation ledger so token, identity and pool ceilings survive a restart, and nothing in production reached it. The only call to
admitWorkflowTurn()omitted itsspendargument, sosharedSpendLedger()was never constructed,spend-ledger.jsonlwas never created by ordinary traffic, andmarkDispatched,settleandabandonhad no production caller at all.src/server/responses/request-spend.tsis that caller. It books by observing the request's own send counter rather than by being called from each dispatch site: that counter moves exactly once per physical send — a reservation increments it, a refund decrements it, and an externally reported send settles against a booking already counted — so one entry per increment is one entry per send. A dispatch path added later cannot forget to book, which is the failure mode that produced an uncalled feature the first time.The observer may refuse. A ledger limit that could only describe a send after the fact would be no ceiling at all, so
reserveDispatchconsults it last, after every cheaper bound has passed, and a refusal denies the dispatch asspend-exhausted. Consulting it last matters because it is the only bound in that function that writes: an entry booked for a dispatch some other check would have refused is spend the request never makes, held against the scope until retention expires.A booking is confirmed dispatched only once a later send exists, because that later send proves the earlier one left. The newest stays open, so a reservation the budget hands back is still released for free. The cost is bounded and stated: a hard crash between reserving and sending replays as abandoned rather than unresolved, for at most one send per request.
Settlement follows what the request learned.
addFinalRequestLogis the one seam every request passes exactly once, whatever transport served it and however it ended, and the terminal usage is already known there. That usage belongs to the last send that left, so it settles with the real figure; every earlier send failed without reporting usage of its own and may still have been billed, so it becomes unresolved spend rather than free.The ledger also now resolves what replay leaves behind. A reservation that survives restart has no owner: nothing in the new process can settle it, and leaving it live holds its tokens against the scope forever — a ceiling that only ever tightens. Deleting the entry is not the alternative, because that would hand the same send id a second reservation. An undispatched reservation never reached the wire and is abandoned; a dispatched one may already have been billed and becomes unresolved. Both are journaled, so a second restart has nothing left to redo.
The identity scope is the privacy-safe account label the request log already uses, and the ledger aliases it again on the way to disk, so no raw credential reaches either.
Not in this change, deliberately: the default policy still sets no token ceiling on any scope, so an unconfigured install accounts and reports without refusing anything. The operator configuration path for those limits is separate work. The reservation's output ceiling is the caller's
max_output_tokens; when the caller omits it the provider/model default decides and only the input estimate is reserved up front, so the bound is looser rather than wrong, and settlement books the real figure.Closes #4707
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:
src/server/index.tsis still the onlyadmitWorkflowTurncaller and still passes four arguments, and that call site cannot carry a per-request reservation — it runs before the body is parsed or routed, and releases its workflow lease as soon aswork()returns aResponse, while streaming consumption continues afterwards throughtrackStreamLifetime. Booking there would mark a streaming reservation lost before terminal usage arrived.spentthroughcreateRequestExecutionBudgetto confirm one increment per physical send across all three shapes: a direct reservation, acountedExternallyreservation settled later byonSendsConsumed, and a surplus report. The surplus path books without being able to refuse, which is correct — those sends have already left.applyReserve/applyDispatchrestore live reservations and no pass resolved them, so the reconciliation loop is placed immediately after replay and inside theif (journal)block.core.tsand therefore inRESPONSES_CORE_MODULES;tests/responses/responses-core-modules.test.tswalks that graph and would fail on an owner missing from the inventory.scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json, and updatedstructure/transports/responses.md(owner table plus a new contract section), which owns this source area.request-log.tsandrequest-spend.ts, so no runtime import cycle is introduced.Regression coverage added —
tests/responses/responses-spend-ledger-wiring.test.ts:spend-exhaustedand the send is not charged;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
Ledger refusal scope, and why an observe-only ledger may not refuse
Two corrections landed on this branch after the first CI run, both on the bl2 contract. A reviewer will reasonably ask why a ledger that enforces nothing by default is allowed to refuse anything at all, so the answer belongs here.
It refuses only an operator's configured ceiling.
reserve()can deny for four different reasons, and only one of them is a spend decision:spend-limit-exceeded. The others — tracking capacity, an undurable reservation, a journal replay could not prove complete — all mean the ledger cannot account for this send, which is not the same statement as this send must not happen. The first version refused on any denial, so an install that had never configured a ceiling could stop sending because a journal got corrupted or a capacity bound was reached. That turns a feature added for observation into an outage, and it is a worse regression than the one #4707 describes. The default policy sets nomaxTokenson any scope, so an unconfigured install now keeps exactly the count caps it already had and is never newly refused.The shared ledger is resolved on the first charge, not when the request is built.
sharedSpendLedger()opens a journal under the OpenCodex home and caches it for the life of the process. Resolving it during request construction meant two things: a request that never dispatches — refused at admission, answered locally, cancelled before its first send — created a journal it had no business creating, and the home captured was whichever one happened to be current for the first request the process ever built, not the home in effect when a send actually goes out. Deferring to the first charge fixes both.Assertions corrected
Three assertions in the new tests claimed states the code never reaches.
tsconfigincludes onlysrc, so test code is never typechecked and a test that asserts the opposite of what it claims passes silently; these were found by reading.final-recovery-spentwhere the total ceiling refuses first. It assertstotal-exhausted, and checksreserveSpentseparately for the point it was actually making.File-size ratchet
src/server/responses/core.tsreached 214 lines against a 210-line cap intests/fixtures/file-size-baseline.json— the spend-observer wiring added four lines. The ratchet only ever lowers caps, so exceeding one is a hard failure. The comment and the expression are each one line again and the file is back at its cap.Why one test caught a real defect and three others caught nothing
The replay reconciliation shipped in this lane with a bug: it treated a replayed undispatched reservation as abandoned and handed its tokens back.
tests/lib/spend-reservation-ledger.test.tscaught it, because that test asserts an invariant — an exhausted scope is still exhausted after a restart — rather than the shape of the code that produces it. The reasoning behind the bug was wrong in a way no shape assertion would have noticed:opendoes not prove nothing was sent, because the torn-tail rule directly above says the journal may be missing its last record, so a send that dispatched and died before its dispatch record landed is exactly what survives replay asopen. Returning those tokens resets a ceiling that had already fired, which defeats the reason the ledger is on disk at all. Both live states now resolve to unresolved spend.Three assertions added by this lane did the opposite and had to be corrected. One claimed a limiter refusal on a path that short-circuits before reaching the limiter; one named a denial reason the total ceiling pre-empts; one pinned an exact status where the property that mattered was the loss of an error identity. All three passed while asserting something other than what they claimed to cover, and none would have been caught by typechecking —
tsconfigincludes onlysrc, so test code is never typechecked.The source oracle in
tests/lib/transient-budget-scope-source.test.tswas changed in the same spirit: instead of pinning the literal text of the line that mints the send budget, it now asserts that the spend observer is installed at that same place, which is the property that matters — a combo child inherits the parent's holder and must not open a second set of ledger entries for the same physical sends.