Repository navigation
feat(budget): park lost settlements durably and fold them at pre-check - #162
hasitpbhatt wants to merge 28 commits into
Conversation
![orcacode-review[bot]](https://avatars.githubusercontent.com/in/4254738?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1154): 🟠 P1 Settle the AdapterError branch like the sibling provider-error branch so a budgeted key isn't charged its entire remaining allowance for an internal adapter fault
The new budget settlement keys the fail-closed "charge the full remaining allowance" rule on usage_seen being False (see _settlement_amount, lines 935-953). The sibling except Exception branch (lines 1155-1205) explicitly sets agg_usage = _settle_unmeasured_stream(...) and usage_seen = True before the finally runs _finalize(), so a mid-stream provider failure is charged only the delivery estimate — its own comment says the alternative "would charge (and exhaust) the key's entire remaining budget" for every transient failure. The except AdapterError: branch (an internal protocol-adapter bug — "our own fault, not the caller's") does NOT do this: it just closes the stream and returns, leaving usage_seen False. The finally then runs _finalize(), whose _settlement_amount() computes max(recorded_cost≈0, cap - spent_snapshot) = the entire remaining allowance, and charge_budget clamps spent_microcents to the cap. So a single AdapterError on a budgeted key — even one that delivered nothing — permanently exhausts the key and bills the customer's whole remaining budget for our own bug. This contradicts the explicit design intent expressed in the sibling branch and in test_budgeted_stream_midstream_error_charges_actual_only. Fix: before return in the AdapterError branch, add agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body) and usage_seen = True, mirroring the except Exception branch.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 269 calls · 15M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
47cdc8b to
ddacb57
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1164): 🟠 P1 AdapterError mid-stream leaves usage_seen False and exhausts the key's entire remaining budget
The new fail-closed settlement (_settlement_amount, lines 935-966) charges the full remaining allowance (cap - kc._budget_spent) whenever a stream ends with usage_seen == False. The cancel branch (line 1132) and the provider-error branch (line 1215) both mark the delivery as known — agg_usage = _settle_unmeasured_stream(...) + usage_seen = True — before _finalize() runs. The except AdapterError branch (lines 1146-1164) does neither: it closes the upstream and returns, so _finalize() runs with usage_seen == False and agg_usage == {}. _settlement_amount() then raises row_values["cost_microcents"] to the key's whole remaining allowance and charge_budget moves the counter by that amount, permanently exhausting a budgeted key on our own adapter bug. This is exactly the defect the commit fixed for the provider-error branch (its test comment: "Before the fix, usage_seen stayed False in that branch and every transient provider failure permanently exhausted the key (charged cap - spent)") and for client disconnects; an AdapterError is the same class of mid-stream failure whose error response IS delivered to the client (the adapter emits a native error event), so per the commit's own policy it should be priced from _settle_unmeasured_stream(agg_usage, agg_output_chars, body) and marked known. Fix: in the AdapterError branch, before return, add agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body); usage_seen = True (mirroring the except Exception branch).
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 276 calls · 15.6M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
ddacb57 to
eac6f2a
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1168): 🟠 P1 Settle the AdapterError branch before returning so a budgeted key is not charged its full remaining allowance
The except AdapterError: branch (a mid-stream failure of the proxy's own SSE adapter, after content has already been forwarded) returns without calling _settle_unmeasured_stream and without setting usage_seen = True, so _finalize → _settlement_amount() runs with usage_seen still False and charges max(actual, cap - spent) — the key's entire remaining lifetime allowance — and records that amount on the row. The two sibling failure branches in this same commit were explicitly refined to avoid exactly this: the except Exception provider-error branch (line 1197) and the cancel branch (line 1129) both settle with _settle_unmeasured_stream and mark usage_seen = True first, and the test test_budgeted_stream_midstream_error_charges_actual_only documents that leaving usage_seen False "permanently exhausted the key (charged cap - spent)" and calls that a bug. An adapter fault (our own bug, e.g. the malformed-chunk case in tests/integration/test_adapter_failure_attribution.py) hits the identical pattern: a budgeted key's whole remaining allowance is consumed for a few delivered tokens the caller cannot steer — the exact over-bill _settle_unmeasured_stream's docstring says must not happen. The adapter-fault integration tests use uncapped keys, so this path is untested. Fix: before return, mirror the generic branch — agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body) and usage_seen = True — so _finalize prices the delivered content instead of the full remaining cap.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 272 calls · 15.2M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
app/routes/chat.py (line 1164): 🟠 P1 Mark the settlement known in the AdapterError branch, or a budgeted key is charged its full remaining allowance on an adapter fault
The streaming generator's failure branches all settle the delivery before _finalize runs: the cancel branch and the provider-error branch both call _settle_unmeasured_stream(...) and set usage_seen = True (lines 1132, 1198), precisely so _finalize's _settlement_amount() (line 952: if not usage_seen: actual = max(actual, cap - _budget_spent)) charges only the delivered estimate instead of the whole remaining allowance. The except AdapterError: branch (an adapter bug mid-stream — the deployment's own fault, not the client's and not the provider's) sets neither: it just sets status/error_type, closes the stream and returns. The finally (line 1210) then runs _finalize() → _commit_row() → _settlement_amount() with usage_seen still False, so the row records cap - kc._budget_spent (the key's FULL remaining allowance, e.g. 100_000 microcents for a fresh 10-cent key) and charge_budget moves the counter to the cap — the key is maxed out by a single adapter fault that delivered nothing or a few chunks, where the sibling provider-error branch would charge ~0 (empty delivery) or the character estimate. Fix: mirror the sibling branches before the return — agg_usage = _settle_unmeasured_stream(agg_usage, agg_output_chars, body) then usage_seen = True.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 237 calls · 13.3M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 289 calls · 18M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 237 calls · 13.8M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 268 calls · 16M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 249 calls · 14.8M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 283 calls · 16.5M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 252 calls · 14.1M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from c387d54 to 6f8a2a4 during the review; these findings describe a diff the PR no longer has.
These findings describec387d54654d6. Re-run the review to check the PR as it stands now.
The gate evaluated to blocked, but it is not enforced here: branch protection will not see it.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 265 calls · 15.8M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 285 calls · 17.3M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
|
@orcacode-review |
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 287 calls · 17.7M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
|
I spent a while trying to settle the last open finding on this one end-to-end, and I want to report what I found — including that I could not confirm it. Three things are blocking this PR, and they are not the same kind of thing1. It conflicts with 2. Ten review threads are still unresolved (hard blocker). The repo ruleset has 3. One finding I could not settle either way — On that last one: I could not reproduce itThe premise is that two concurrent requests both precheck at That requires the fail-closed arm in
In every case the row recorded the real cost, For contrast, the same harness does trigger it on #161: an injected adapter fault there produces So: I can't call this finding real, and I can't call it dead either. I could not construct the state it describes; that is not the same as proving it impossible. The concurrency window it needs is exactly the kind my end-to-end harness can't force. You know this code far better than my probes do — could you say whether the fail-closed arm is still reachable at all on the current head, and if so whether clamping the seed at Why the bot says "0 findings" — and why that is not evidence hereThis is worth flagging, because the green checkmark is misleading on this PR:
So the flip from "1 P1" to "0 findings" cannot be explained by a fix. I re-triggered the review today to check, and it came back clean again — but that is the same non-deterministic judge disagreeing with its own earlier pass, not independent confirmation. Two clean runs out of three, on identical code, is weak evidence. Worth knowing generally: re-running the bot does not resolve existing threads either. The count stayed at 10 unresolved through today's re-review. Only a human closing them lifts the ruleset gate. To summarise what I think unblocks this: rebase onto |
|
Merge-order carve-out (see #165): this PR's "do not merge before #163" banner applies to #161/#162, not to #160. #160 (schema + spend library + startup migration, inert) must land first and independently of #163: any build of this PR requires the Also: #163 currently lacks this stack's newer commits pushed after its last update (e.g. #162's |
6f8a2a4 to
ead5e8f
Compare
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
ead5e8f to
6aebfc5
Compare
🐳 OrcaCode ReviewThis pull request was not reviewed. A maintainer of this repository can comment No comments were posted for this commit. Details
OrcaCode Review — Route Smarter. Ship Safer. Spend Less. |
|
@orcacode-review |
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 298 calls · 19.8M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
|
Status after #160 landed ( Conflicts now. Four files: #160's push 4 rewrote The bot is no longer clean on the current head. On
Worth noting because the "0 findings" state this PR had on 09-27/09-30 is what I checked last time, and it has changed since. 12 of 12 threads are unresolved, which keeps the ruleset gate shut regardless of the above. One correction to carry over from my 09-30 note: I could not reproduce the clamp finding on this branch. I instrumented That is not proof the arm is dead — the two new findings above are about exactly that arm, so it is clearly still on the reviewer's mind. It does mean I can't settle it from the outside, and the two new P1s look like the more productive place to start. Rebase and work those two and I'll re-verify end-to-end before anything merges. Stack-wide status is in #165. |
…normalize Anthropic usage keys Two holes in the enforcement layer Continuum-AI-Corp#161 added: - The blocking fail-closed arm fired whenever response had no truthy usage, so an EMPTY 200 (usage {}, or usage missing with empty content) permanently exhausted a budgeted key — inconsistent with the streaming twin that treats a {0,0} usage as known-zero, and with a client simply not waiting. Gate on delivered content: fail closed only when real content was returned and the cost is unaccountable. - _build_log_row read only prompt_tokens/completion_tokens, so a usage frame keyed input_tokens/output_tokens (what /v1/messages forwards) normalized to zero tokens and defeated every token-keyed gate. Read both shapes.
The three findings from the review of the previous push shared one root
cause: the charge was decided by four independent signals, none of which
owned the decision. usage_seen was set by whichever handler ran, presence
of a usage dict was treated as measurement, and nothing anywhere looked
at whether content had actually been delivered.
Handlers now record facts and nothing else. The provider-error, adapter
and client-disconnect arms set stream_ending; the loop sets
stream_completed the moment the upstream stream ends, before our own
[DONE] framing, so where a client disconnects stops being able to change
the bill. One function (_unmeasured_charge) turns those facts plus the
delivery characters into the charge, and the blocking path calls the same
function, so the two can no longer drift apart.
Consequences, all of them previously reachable holes:
- An empty unmeasured stream settles at zero instead of consuming the
whole remaining allowance, matching the blocking gate.
- A client that hangs up at the trailing [DONE] frame is charged what a
client that stayed connected is charged. Reading the full answer and
leaving was the cheapest way to use a capped key.
- A usage frame with no countable token key ({"total_tokens": 123}) is
unmeasured, on both paths. It used to normalize to zero tokens and bill
a delivered completion nothing.
- A blocking completion with content and no usage is priced from what it
returned, not charged the lifetime allowance. The old rule billed a
customer their entire budget for one request whose upstream omitted a
field.
- include_usage is no longer forced on a client that explicitly declined
it. Declining is priced from the delivery; a provider that ignores the
usage frame we requested still fails closed.
- The 1-microcent floor: a short delivery estimates below the smallest
representable amount, and a delivery that costs nothing is exactly what
the cap must not allow.
Rows now record the estimated tokens behind an estimated charge, so a
non-zero cost never sits on a zero-token row.
Tests: the empty unmeasured stream, the disconnect at [DONE], tokenless
usage on both paths, and the re-split of the fail-closed cases (requested
and dropped usage vs a client that declined it).
A debug print guarded by an env var was left inside _unmeasured_charge while working through the settlement rework: it reads an environment variable on every unmeasured settlement and writes to stdout inside a request handler.
The streaming retry loop detaches each commit so the in-flight stream cannot abort it, then waits for the task on the cancellation path. When the task is cancelled itself, that await raises and the arm passed over it with `pass`. Before enforcement that cost one log row; now it costs a budget charge, because the row and the settlement share a transaction: nothing is written, spent_microcents never moves, and nothing is logged. The cap reopens for exactly the key whose settlement failed, with no trace. The arm now re-runs the write inside anyio.CancelScope(shield=True) — the same primitive the rest of the teardown uses, and the only way an await survives a cancellation already unwinding us. `_commit_row` is idempotent on trace_id, so a commit that landed with a lost ack is recognised rather than charged twice. If even the shielded retry cannot land, the loss is logged with the amount instead of passing over it. This belongs here rather than in the park PR: enforcement is what makes the arm cost a charge, and this PR is where enforcement becomes live. A review finding it on this diff is right.
… live helpers _settle_unmeasured_stream died when settlement became one decision function: no runtime path calls it anymore, and its logic lives in _estimate_usage plus _unmeasured_charge. Carrying it (and a test file for it) would ship dead code with a passing suite. The unit tests are re-pointed at the live helpers so the character math and the countable-usage gate stay pinned, and _blocking_delivery_has_content says which rung consumes it.
…on it A park larger than the remaining allowance moved nowhere and stopped the scan, so the counter could sit below the cap on a key refused by a row nothing would ever shrink. Apply the allowance oldest-debt-first and rewrite the oversized row to its remainder, which keeps the invariant the fold exists to hold: either the queue is empty or the counter is exactly on the cap. The over-claim stays a row, so it still blocks and folds for free if the cap is raised. created_at alone is not a total order (second resolution on SQLite, ties on Postgres), and two workers computing the same fold have to agree on which row is the partial one, so the scan breaks ties on trace_id.
`checkfirst` asks the catalog and then creates, and the ask cannot see another worker's uncommitted CREATE, so two boots racing an upgrade both issue it and one is rejected. That was the only startup DDL outside `_apply_ddl`, and on Postgres the error aborts the transaction the rest of startup runs in: the worker never comes up, and never reaches the fold this table exists to feed. Generalise `_apply_ddl` to a callable so dialect-generated DDL goes through the same savepoint, and teach `_already_applied` the shape this collision actually takes in Postgres — a unique violation on the catalog, not an "already exists".
The retry handler rolls the failed attempt back before deciding, and that is an await — so it is where a cancellation aimed at the request can land. An exception raised inside a handler is not caught by the same try's other arms, so the cancellation escaped past every give-up below it: the row was already lost, the park never happened, and the key's cap reopened for the cost it had just been served. Treat it like the other arm's rollback failure and carry on to the give-up. The next attempt then still finds the session poisoned, so what lands is one park; what matters is that the settlement is accounted for rather than dropped.
`_insert_park` answered "not durable" to every failure except a unique violation, including a COMMIT that applied and lost its ack. The caller then stored the amount in `_unsettled` beside the row it had just written, another worker folded and deleted the row, and the stale memory copy re-filed as a new park under a `trace_id` that no longer collided — billing one delivery twice and leaving the key pinned on its cap with no debt left to fold. Ask the database what landed instead of guessing, and drop the memory hold for any row a fold actually billed. An unreadable park ledger folded into the total as zero, which dispatched a key whose cap was held shut only by parks this worker could not see. The two reads are not the same connection either: the counter rides the request's, the ledger opens a new one, so a checkout timeout hid every park while the request worked fine. `pending_parked_spend` now answers `None` for unknown and the pre-check maps unknown onto the cap. Oldest-debt-first was decided by `trace_id`, because `created_at` came from `CURRENT_TIMESTAMP` — one second wide on SQLite, and a recovered outage re-files a whole batch inside a single pre-check. The row stamps itself Python-side at sub-second resolution so the tiebreak is the rare path it is documented as.
A commit whose acknowledgement was lost, combined with a durability probe that also failed, parks an obligation whose charge is already in spent_microcents. Folding it re-charges the same delivery, and because the fold deletes the park row there is nothing left to correct it. The request-log row and the charge share one transaction, so the log row proves the charge landed: clear those parks without billing them, and drop the matching in-memory hold so a later pre-check cannot re-file them.
`test_parked_queue_drains_oldest_first_as_the_cap_opens` failed on
Windows: the column default stamps `created_at` from the wall clock, and
two back-to-back `datetime.now()` calls return the SAME value there
(the clock ticks every ~15.6ms), so two parks written moments apart tied
and fell through to the `trace_id` tiebreak. "t-new" sorted before
"t-old", the younger debt was billed first, and the fold trimmed the
older one instead. On ubuntu CI the stamps differ, the test passes, and
the failure mode stays invisible.
Pin the stamps in both tests so the assertions are about fold ordering
rather than the host, and verified the FIFO test still bites by reversing
the offsets: it then fails with exactly the original `{'t-old': 1500}`.
The ordering guarantee is real but weaker than the docstring claimed.
`trace_id` is a uuid4, so parks sharing a stamp break the tie arbitrarily
— no schema change here because nothing in the accounting depends on it:
the parked total is conserved either way and the counter still reaches
min(cap, spent + debt). Only which row is trimmed differs, so the
docstring now says so and a new test pins the invariant that holds either
way (total conserved, remainder stays visible, queue drains to empty).
…ssing-usage fail-closed on delivered content
The streaming commit retry loop detached each commit task and only
awaited it on the give-up path; when the task itself was cancelled
(loop teardown, direct cancel), the inner `except BaseException: pass`
swallowed it, so the delivered stream recorded no row, charged nothing,
and filed no park -- the cap reopened for exactly the key whose
settlement failed. Run the same `_give_up_settlement` probe-and-park as
every sibling arm before propagating.
The blocking fail-closed arm's fail-closed raise fired whenever `usage`
was absent -- including an empty 200 -- while the same delivery with
`usage: {0, 0}` correctly settled at 0. Gate the missing-usage
disjunct on delivered content so an empty response can never exhaust
the key.
Continuum-AI-Corp#161 simplified the pre-check to a plain counter read, which is correct there because nothing parks yet. This rung reintroduces budget_precheck precisely so a parked obligation is folded before the 429 decision, and routing the gate through read_spent would silently skip that: a key whose settlement failed would keep dispatching as if it had never spent anything, which is the entire state durable recovery exists to close. Also reconciles the blocking settlement tests with the unified decision function now in the tree: a usage-less blocking completion is priced from what it returned (floored at one microcent), never charged the remaining allowance, and the parked obligation a write outage leaves behind is that same amount.
Ported from Continuum-AI-Corp#161, where the arm that swallowed a cancelled detached commit task cost only a log row; enforcement is what makes it cost a charge, so the fix belongs on the rung where enforcement lands. This rung already had the stronger form: _give_up_settlement probes durability and parks the obligation, so the cost keeps counting even when the write can no longer land at all — which is why #161s shielded re-run of the write is not reproduced here, and why #161s regression test (which asserts the retry lands a row) is not either. test_streaming_commit_task_ cancellation_gives_up pins this path on this rung.
Rebasing onto Continuum-AI-Corp#161 brought its shielded-retry test along. It asserts a retried write lands a row, which is what Continuum-AI-Corp#161 does; here the cancelled commit task is probed and parked instead, which test_streaming_commit_task_cancellation_gives_up already pins. Carrying both would assert two different mechanisms for one arm.
db9b0b7 to
e5cadb3
Compare
There was a problem hiding this comment.
🐳 OrcaCode Review
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 306 calls · 21M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
|
/orcacode-review |
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 273 calls · 16.5M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from becff5f to 3cec25f during the review; these findings describe a diff the PR no longer has.
These findings describebecff5f40b08. Re-run the review to check the PR as it stands now.
The gate evaluated to blocked, but it is not enforced here: branch protection will not see it.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 276 calls · 17.3M tokens · 95% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 298 calls · 19.8M tokens · 96% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
3cec25f to
e5cadb3
Compare
2 participants
Orca-Code-Review — push 18
❌ 1 finding blocks merge
PR 3 of a 4-PR stack replacing #91. Builds on #160 and #161 — review commit
ddacb57only (the diff below is cumulative until those merge).Merge order: do not merge this before #163. The
except AdapterError:branchbelow returns without settling, so
_finalizeseesusage_seen=Falseand_settlement_amountcharges a budgeted key its entire remaining lifetime budgetfor a server-side adapter fault. This is known and owned by #163 (
4cdf36a),which settles an adapter fault like the provider-error branch instead — it is
listed here so it does not read as an unfixed gap in this PR's own scope, and it
needs no separate fix.
Review scope: durable recovery — a lost settlement keeps counting against the cap across restarts and workers.
budget_parkstable (one row per settlement, keyed bytrace_id, created by the startup migration): park writes are idempotent, so an ack-lost commit retries into the same key instead of double-billing.spent_microcentsin one CAS-guarded transaction (charge + row deletes); concurrent folders cannot double-bill, and over-remainder parks stay visible while keeping the key exhausted.OrcaCode Review: PASSED, no findings. Tests: budget unit + integration green; ruff clean.
Known gap, pre-existing and deliberately out of scope here:
app/routes/chat.py:1062— theexcept BaseException: passthat followsawait commit_taskon the streaming teardown. A secondCancelledErroraimed atthat await rather than at the commit leaves the settlement neither charged nor
parked, and the arm swallows it. Blamed to
mainat9021c8f; this stack neitherintroduces nor touches it.