Repository navigation
feat(budget): spend accounting schema, migration and library [1/4] - #160
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.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 174 calls · 5.1M tokens · 92% 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
hasitpbhatt
changed the title
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: 153 calls · 5.7M tokens · 92% 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: 169 calls · 6.4M tokens · 92% 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 verified this end-to-end against current The PR body says "Scope: the budget schema and the accounting library, and nothing else. No request-path behavior changes — deliberately. This PR does not make So I tested whether that claim is actually true, rather than taking it on trust:
Plus The one thing I'd have wanted from an inert-schema PR is exactly what the last commit added: the On the substance of the finding — it's factually right that nothing calls this yet, and it would be a real P1 if this PR claimed to enforce the cap. It doesn't. As written, the finding would block the first slice of any stacked change, since slice 1 is inert by construction. Worth noting the stack has a separate constraint that does matter: #161 and #162 both say "do not merge this before #163", because their @hasitpbhatt — if you agree this one is scope-by-design, could you resolve it? That unblocks #160, which is the only one of the four that merges cleanly onto current |
|
Agreed — scope-by-design, and your verification is exactly the evidence this needed. Thank you for running it end-to-end rather than taking the PR body on trust. Acted on:
@xizhuomengcontin — one ask: since this repo requires a human approving review and your 09-30 write-up is the most thorough verification it's had, could you submit it as an APPROVE review if you're comfortable? (An issue comment can't clear the required-review gate.) If you lack permission to approve, flagging any maintainer to dismiss/resolve the remaining review state would work too. Remaining on this PR: CI on push 4, and the OrcaCode re-review of the new head. |
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: 210 calls · 9.6M tokens · 94% 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 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. |
The upgrade seed is a correlated SUM over requests_log, and the index that would serve it was created further down the same function — so the one boot that runs the seed was also the one that could not use the index, and later boots do neither. Build the index first and pin the order with a test. Also states the two contracts the schema leaves implicit: the seed counts soft-deleted request rows on purpose, because restoring an accrued lifetime total can only ever tighten a cap, and cap_microcents is budget_limit_cents scaled to microcents rather than the column itself.
The seed ran only inside the branch that added the column, and the two statements do not vouch for each other: on SQLite the ALTER is durable the instant it executes while the seed is DML in the transaction a kill — or the `database is locked` this very aggregate provokes on an upgrade that overlaps the old machine's writes — rolls back. Gating on the column's absence made that half-applied boot the only one that could ever have seeded, so every key predating the release kept a full fresh allowance forever, silently, which is exactly the outcome the seed exists to prevent. It runs on every boot now, restricted to keys that hold a cap, and the statement is idempotent because a log row and its charge are one commit — a key already holding spend has nothing to restore. Three more from the same review: - Gate the Postgres BIGINT widen on the reflected type rather than the column's name, which was present forever and so took ACCESS EXCLUSIVE on api_keys at every start. - Correct the model comment justifying that widen with a client-supplied budget no route accepts, in the wrong unit. - Make the upgrade tests able to fail: the legacy fixture had one key and one log row, so a seed that dropped its correlation predicate and stamped every key with the table total passed. It now has three keys with three histories, pins the half-applied boot above, and the concurrency test runs over a file instead of `:memory:`'s StaticPool, where two "independent" sessions shared one connection and the atomic guard never met a concurrent writer.
A `WHERE spent_microcents = 0` backfill can never see traffic logged after the boot that seeded: until Continuum-AI-Corp#161 wires the per-request charge the counter only moves at boot, so every later request under-reports lifetime spend permanently. Repair to max(counter, log total) on every boot instead, clamped to budget_limit_cents scaled to microcents and never lowered — the clamp keeps charge_budget's deliberate post-cap under-count a fixed point instead of an inflation target. Also tolerate Postgres surfacing a lost CREATE INDEX race as a pg_class verror, which the boot previously died on.
…ntity map Session-default synchronize_session evaluates the SET in Python against a loaded copy of the row and marks it dirty, so a caller that loaded the key before charging (the documented Continuum-AI-Corp#161 flow: validate_api_key loads it, the same session commits the charge with the log row) flushes that stale value over the DB's atomic UPDATE on commit — silently dropping a concurrent charge and under-counting lifetime spend. Disable sync on both UPDATEs and pin it with a two-session regression test that fails without the fix.
The boot repair clamped spent_microcents against `budget_limit_cents * 10_000` with a literal, while `packages.auth.spend` defines the same conversion as MICROCENTS_PER_CENT. Two unlinked copies of one unit factor across packages: re-typing either silently rescales every capped key's budget by 10,000x, which spend.py's own docstring warns about for callers and nothing warned the migration author about. Define the factor once in `packages/db.units`, below both layers, and import it from `spend` and `migrate` alike so the dependency direction stays auth -> db. spend re-exports it, keeping the name where the budget primitives are documented to live. Pinned by a test that perturbs the shared constant and reads the scale back out of the statement the boot emits, so a re-typed literal fails rather than drifting silently.
7e1f493 to
2c75b3a
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 |
🐳 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
✅ No findings — nothing to flag in this PR. Great work!
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 199 calls · 9.2M tokens · 94% 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
…um-AI-Corp#160) * feat(budget): add spent accounting, startup migration, and atomic charge * fix(budget): let the boot seed use the index it aggregates through The upgrade seed is a correlated SUM over requests_log, and the index that would serve it was created further down the same function — so the one boot that runs the seed was also the one that could not use the index, and later boots do neither. Build the index first and pin the order with a test. Also states the two contracts the schema leaves implicit: the seed counts soft-deleted request rows on purpose, because restoring an accrued lifetime total can only ever tighten a cap, and cap_microcents is budget_limit_cents scaled to microcents rather than the column itself. * fix(budget): make the lifetime-spend seed a repair, not a one-shot The seed ran only inside the branch that added the column, and the two statements do not vouch for each other: on SQLite the ALTER is durable the instant it executes while the seed is DML in the transaction a kill — or the `database is locked` this very aggregate provokes on an upgrade that overlaps the old machine's writes — rolls back. Gating on the column's absence made that half-applied boot the only one that could ever have seeded, so every key predating the release kept a full fresh allowance forever, silently, which is exactly the outcome the seed exists to prevent. It runs on every boot now, restricted to keys that hold a cap, and the statement is idempotent because a log row and its charge are one commit — a key already holding spend has nothing to restore. Three more from the same review: - Gate the Postgres BIGINT widen on the reflected type rather than the column's name, which was present forever and so took ACCESS EXCLUSIVE on api_keys at every start. - Correct the model comment justifying that widen with a client-supplied budget no route accepts, in the wrong unit. - Make the upgrade tests able to fail: the legacy fixture had one key and one log row, so a seed that dropped its correlation predicate and stamped every key with the table total passed. It now has three keys with three histories, pins the half-applied boot above, and the concurrency test runs over a file instead of `:memory:`'s StaticPool, where two "independent" sessions shared one connection and the atomic guard never met a concurrent writer. * fix(budget): cast migration seed to BIGINT and clarify schema library scope * fix(budget): make the boot seed a cap-clamped monotonic repair A `WHERE spent_microcents = 0` backfill can never see traffic logged after the boot that seeded: until Continuum-AI-Corp#161 wires the per-request charge the counter only moves at boot, so every later request under-reports lifetime spend permanently. Repair to max(counter, log total) on every boot instead, clamped to budget_limit_cents scaled to microcents and never lowered — the clamp keeps charge_budget's deliberate post-cap under-count a fixed point instead of an inflation target. Also tolerate Postgres surfacing a lost CREATE INDEX race as a pg_class verror, which the boot previously died on. * fix(budget): keep the charge UPDATE from writing back through the identity map Session-default synchronize_session evaluates the SET in Python against a loaded copy of the row and marks it dirty, so a caller that loaded the key before charging (the documented Continuum-AI-Corp#161 flow: validate_api_key loads it, the same session commits the charge with the log row) flushes that stale value over the DB's atomic UPDATE on commit — silently dropping a concurrent charge and under-counting lifetime spend. Disable sync on both UPDATEs and pin it with a two-session regression test that fails without the fix. * fix(budget): scale the boot cap from the one microcent constant The boot repair clamped spent_microcents against `budget_limit_cents * 10_000` with a literal, while `packages.auth.spend` defines the same conversion as MICROCENTS_PER_CENT. Two unlinked copies of one unit factor across packages: re-typing either silently rescales every capped key's budget by 10,000x, which spend.py's own docstring warns about for callers and nothing warned the migration author about. Define the factor once in `packages/db.units`, below both layers, and import it from `spend` and `migrate` alike so the dependency direction stays auth -> db. spend re-exports it, keeping the name where the budget primitives are documented to live. Pinned by a test that perturbs the shared constant and reads the scale back out of the statement the boot emits, so a re-typed literal fails rather than drifting silently.
2 participants
Orca-Code-Review — push 5
✅ no blocking findings
What this PR is
PR 1 of 4 replacing #91 (same feature, split for reviewability; #91 will be closed once this stack is up).
Scope: the budget schema and the accounting library, and nothing else. No request-path behavior changes — deliberately. This PR does not make
budget_limit_centsenforced, and it is not supposed to.spent_microcentscolumn, startup migration,packages.auth.spendlibrary + unit testsis_exhausted/charge_budgetintoexecute_chat— the enforcement itselfOn the open "dead code" P1
The review bot flagged
charge_budget/is_exhausted/read_spentas having zero callers inapp/and called that dead code. That is accurate about this diff and is the intended shape of the stack: a landable-but-unused library is how the schema and the atomic-charge semantics get reviewed on their own, before they change anyone's requests. The wiring exists and is up for review right now in #161, which adds the pre-dispatchis_exhaustedcheck and the single-transactioncharge_budget(commit=False)alongside theRequestLoginsert.Merging #160 first is safe: it only adds a column, a startup migration and a module nothing imports.
Included
spent_microcentscounter + BIGINT widening onApiKey; spend index onrequests_log.ensure_budget_columnsstartup migration (upgrade seeding from request history, concurrent-boot race guard), wired into lifespan.packages.auth.spend:charge_budget(single atomic UPDATE, never exceeds cap, clamps instead),read_spent,is_exhausted.Review state
OrcaCode Review posted CHANGES_REQUESTED with 1 P1 on push 1 — the stack-scope item discussed above. It is not currently passing, and this description previously claimed otherwise; corrected.
Tests: targeted unit suite green. Lint: ruff clean.