[WRONG BRANCH] fix(usage): attribute each attempt to the account that dispatched it (#4717) - #4760
Conversation
…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>
|
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. |
리뷰 · 우선순위 76 / 80이 PR은 API 키로 보내는 요청에서 실제로 나간 키가 누구인지를 usage 로그에 남기게 고칩니다. 지금 스택은 핵심 설계는 네 겹입니다. (1) 추가로 현재 라인 tip 커밋 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
…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)
|
Cascading downward. Retry usage is attributed to the account that actually dispatched it. 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 |
b0cceec
into
codex/bl3-budget-refusal-semantics
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
2 participants
Summary
A request that started on API key A and retried on key B attributed the whole request's usage to A, so budget diagnostics blamed a key that never served the successful attempt. Key selection is captured before secret resolution and dispatch can rebuild against a newer selection, but attempts still went through the generic send path and the account-label validator rejected key identities outright.
This carries #4717 by @thisisjun786, whose commits are preserved with their original authorship and credited with a
Co-authored-bytrailer on the final commit.What it does:
k+ 32-hex account label derived from the provider and the captured key-selection reference. The raw reference is hash input and never label output; the persisted-label validator accepts only that shape.Dropped from the original PR: the Command Code reasoning-effort retry commit. That retry stays on the same selected key, so it is not needed to tell one account's usage from another's, and leaving it out keeps this layer reviewable. The small injected-executor fix it depended on is retained.
Added here: a 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.
usageAttributionstakes 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 same arithmetic is why hidden attempts must not be folded into the response the client sees: the Codex client treats
response.completed.usageas 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
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:
git cherry-pickof the fork's own commits rather than by reimplementation, so authorship and the reviewed diff are preserved. Three of four commits applied; the Command Code commit was skipped deliberately.core-combo.ts(the attribution initialisation now sits beside the existing requested-effort initialisation),structure/runtime.md(the upstream-key section added after the combo-effort section), and one import line inserver-combo-failover-e2e.test.ts.adapterDispatchBudgetis still what reaches every adapter-owned transport inadapter-dispatch.ts,adapter-continuation.tsandrun-turn-execution.ts, andassumeChargeis still the settlement seam inrequest-send-budget.ts. The skipped commit would have reverted those to the bare holder.key-attribution.test.tsis registered in bothscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json, andtests/providers/command-code-retry.test.tsis absent with no entry pointing at it.kfollowed by 32 lowercase hex characters. This is pseudonymisation, not secrecy against offline guessing by a holder of candidate configuration — the property that matters here is that no raw credential is written.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