test(responses): pin the #4546 incident as one system, not five fixes (#4546) - #4772
Conversation
…#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.
|
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. |
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.
리뷰 · 우선순위 77 / 80설명이 PR은 이슈 #4546에서 터진 비용 폭주를 지금 CURRENT 테스트가 묶는 생산 모듈은 가짜 mock이 아니라 실제 구현입니다. 같은 #4546 에픽의 다른 열린 줄기와는 겹치지 않습니다. #4764/#4765는 점수 77/80입니다. CURRENT 라인 1 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
리뷰 · 우선순위 77 / 80이 PR(#4772)은 #4546 증폭 사고를 “다섯 개 개별 수정”이 아니라 한 시스템으로 고정하는 tip 회귀 테스트다. 요청 실행 예산( 시나리오는 사고 축을 그대로 옮겼다. (1) 같은 요청이 여러 층에서 복구를 시도해도 천장에서 멈추고, repair가 공유 reserve를 쓰면 account-failover는 스택 tip이다. base는 파일 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
…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.
|
Landing the send-budget and durable-accounting lane, top layer first. This layer pins the #4546 incident scenarios as an integration regression. 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 |
ba16750
into
codex/bl5-recovery-limiter-dispatch
1 participant
Summary
Each layer of this lane closes one seam of the #4546 amplification: the credential hop 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 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 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.
This is the lane tip: its CI run is the gate for every layer beneath it.
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.
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.Hosted CI runs on this branch and gates the stack. The run URL and conclusion are reported to the integrating maintainer.
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.