Repository navigation
fix(billing): give back the velocity increment when a charge loses its race - #323
Conversation
…s race The velocity monitor records a charge in its own transaction, before the billing transaction takes the wallet lock. When the ledger insert then violates the wallet/operation-key uniqueness constraint, the billing transaction rolls back -- but that already-committed increment does not roll back with it. The loser adopts the winner's durable debit and returns it as a successful idempotent replay, so one charge is counted twice against the caller's hourly and daily spend. Those counters are what the spend cap and the anomaly auto-freeze are measured against, so this is not a skewed metric: the wallet is eventually throttled and then frozen for money it never spent. The existing reverse_velocity_record already says exactly this in its own comment -- it just could never run here, because it is a closure bound to the transaction that rolled back. Three parts: - Lift the reversal to a module-level _reverse_velocity_counters, so a caller with a live session can issue it. - Hoist the period markers out of the transaction. The reversal needs to know which period the increment landed in; without them it could decrement a period the charge never touched, and under-counting is the worse direction -- it silently raises the cap and delays the freeze. - Reverse on both ways out after the increment lands: the IntegrityError path, whether it adopts the winner's debit or re-raises, and any other exception. No-ops when nothing was recorded. Reproduced deterministically by landing the winner's debit in the window between the loser's duplicate pre-check and its own insert: one 3-credit charge left 6.00 recorded against both counters before this change, 3.00 after. The test fails without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: d4a9040f41
ℹ️ 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".
| except Exception: | ||
| # Same debt on every other way out after the increment landed -- | ||
| # a conflict raised by the ledger writer, a database error mid | ||
| # flush. No-ops when the increment never happened. | ||
| await self._reverse_recorded_velocity( | ||
| wallet_id=wallet_id, | ||
| amount=charge_amount, |
There was a problem hiding this comment.
If the transaction commits but its acknowledgement or subsequent session cleanup raises—for example, during a database connection failure—the broad handler assumes the debit rolled back and subtracts its velocity in a new transaction. The ledger debit and balance change can therefore remain durable while hourly/daily spend is reduced, allowing later charges to bypass budget and anomaly limits. Before compensating, verify that no matching debit committed (using the operation key where available), or restrict compensation to failures with a known rollback outcome.
AGENTS.md reference: app/services/AGENTS.md:L7-L14
Useful? React with 👍 / 👎.
Summary
The velocity monitor records a charge in its own transaction, before the billing transaction takes the wallet lock. When the ledger insert then violates the wallet/operation-key uniqueness constraint, the billing transaction rolls back — but that already-committed increment does not roll back with it. The loser adopts the winner's durable debit and returns it as a successful idempotent replay, so one charge is counted twice against the caller's hourly and daily spend.
That is not a skewed metric. Those counters are what the spend cap and the anomaly auto-freeze are measured against, so the wallet is eventually throttled and then frozen for money it never spent.
reverse_velocity_recordalready says exactly this in its own comment — it simply could never run here, because it is a closure bound to the transaction that rolled back.Reproduced deterministically, by landing the winner's debit in the window between the loser's duplicate pre-check and its own insert:
6.000000003.00000000The fix, in three parts
_reverse_velocity_countersso any caller holding a live session can issue it. The in-transaction closure now delegates to it and keeps itssession.refresh(wallet).recorded_velocity. The reversal has to know which period the increment landed in; without them it could decrement a period the charge never touched, and under-counting is the worse direction — an over-count throttles a caller who did not spend, an under-count silently raises the cap and delays the auto-freeze.IntegrityErrorpath — whether it adopts the winner's debit or re-raises, its own increment was spurious either way, since the winner recorded its own — and any other exception escaping after that point (a conflict from the ledger writer, a database error mid-flush). Both no-op when the monitor never got as far as recording, which is the common case for early validation failures.Noticed, not fixed here
velocity_monitor.py:126-127carries the same falsy-zero pattern that #321 just fixed elsewhere:Since #321 made
daily_limit=0a meaningful value that now reaches the monitor, a zero-limit wallet gets the default limit for alerting instead of its own. This is alerting and freeze-threshold only — the hard cap inbilling_engineis already fixed — but it is the same bug class, and #321 sharpened the edge on it. Left out to keep this change to one behavior; happy to take it next.The three other findings from the same review remain open and unbundled: the nested connection checkout in
authorize_and_reserve,max_calls_per_tool/aggregate_value_capbeing read outside the atomic reservation, andcreate_permitpreferring the request-bodysubject_key_idagainst its own comment.Type of Change
Validation
pytest -q— newtest_losing_an_operation_key_race_does_not_leave_velocity_overcounted, verified to fail without the fix (Decimal('6.00000000') != Decimal('3.00000000')) and pass with it. Full-suite run in progress locally; CI covers the same on this head.ruff check— clean onapp/and the test filepython scripts/export_openapi.py --check— n/a, no API surface changepython scripts/generate_sim_inventory.py --check— n/a, no simulator changeAgentMoney.chargewith a sharedoperation_key, the same pathPOST /v1/billing/chargeusesSECURITY_LIMITATIONS.mdFor trust-plane changes,
make trust-release-gateoutput is not pasted here: this corrects a metering counter rather than a permit, receipt, or audit invariant, and no proof artifact changes. Say the word if you want the gate run anyway before merge.Risk
Notes on migration, rollout, or rollback concerns: no migration, no schema change, no API change. The only behavior difference is on paths that previously left a counter inflated — a wallet that lost an idempotency race now keeps the velocity it actually spent. Wallets already carrying an inflated counter from a past race are not retroactively corrected; the counters self-heal at the next hourly/daily period reset.
🤖 Generated with Claude Code
https://claude.ai/code/session_017iqdJ2aL7KsafeRz7BFi2m
Generated by Claude Code
Note
Cursor Bugbot is generating a summary for commit d4a9040. Configure here.