fix(spend-control): count in-flight reservations against hourly/daily caps under a live clock - #286
Conversation
… caps under a live clock check() captures `now` once, then getSpendingInWindow re-read the clock via a second this.now() to decide whether in-flight reservations (pendingTotal) count. On a real advancing clock (Date.now()) the second read can land a millisecond after the captured now, flipping `to >= this.now()` false and silently dropping the pending total from the hourly/daily window — so two concurrent payments could both clear the same remaining budget and overspend the operator's cap. The session window was unaffected (it reads sessionSpent + pendingTotal directly). Thread the caller's single `now` into getSpendingInWindow(from, to, now) and gate on `to >= now`. This removes the racing second read while keeping the guard meaningful: a genuinely historical window (to < now) still excludes live holds. Every existing spend-control test injects a frozen clock (now: () => clock), so its two reads always matched and the sub-ms window was masked — which is why the suite was green. Add a live-clock (advancing) test covering the hourly and daily windows; it fails on the pre-fix code and passes after.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughSpending-window calculations now reuse the timestamp captured by the caller. New tests verify that pending reservations remain counted when the clock advances during a single ChangesSpending-window timestamp consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR makes hourly and daily spend checks use one captured timestamp so active reservations are not intermittently omitted. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…lear the same cap Three fixes, all in the same family: a value read twice, or read without a guard, changing behaviour behind the caller's back. - spend-control: getSpendingInWindow re-read the clock and compared it against a bound the caller had already computed from an earlier read. A millisecond tick between the two flipped the guard false and silently dropped in-flight reservations from the hourly/daily figure, letting two concurrent payments both clear the same remaining budget. The caller's single `now` is threaded through. - /stats?days=: non-numeric became NaN and reported zero usage; negative dropped the newest day and mislabelled the response. One resolveStatsDays() guard. - logs --days: the same input class on the local path, where `parseInt || 1` let a negative through to a slice that trims from the end. Guarded at the sink. 809 tests green, typecheck and prettier clean, dist rebuilt (smoke check passed). Thanks to @erhnysr for #286 and #285.
What
SpendControl.getSpendingInWindow()decided whether in-flight reservations (pendingTotal()) count toward the hourly/daily spend by reading the clock a second time, and comparing it against the window bound the caller had already computed from a first read.check()capturesnowonce at its top and passes it as the window'sto:On the production clock (
() => Date.now()), when a millisecond ticks between read #1 and read #2,this.now() > to, the guard goes false, and the in-flight reservation total is silently dropped from the hourly/daily figure.Why that's a money-path bug
Reservations are the concurrency-safety mechanism. The x402 pre-sign hook does check-then-
reserve()synchronously, on purpose:When payment B's hourly/daily check drops payment A's live reservation, both clear the same remaining budget and the operator's cap is exceeded. The
sessionwindow was unaffected — it readssessionSpent + pendingTotal()directly, never through this guard — which is why the leak was narrow and silent.Severity is bounded: the racing gap is sub-millisecond per check, so this is intermittent under concurrent paid load, not every request. No security impact — it's an accounting/limit-enforcement correctness bug in the payment path.
Why the existing suite didn't catch it
Every test in
spend-control.test.tsbuilds itsSpendControlthrough a helper that injects a frozen clock:With a frozen clock, read #1 and read #2 return the identical value,
to >= this.now()is always true, the reservation is always counted, and the race cannot surface. The bug only appears when the clock actually advances between the two reads — which the frozen clock structurally prevents.Fix (threaded
now)Thread the caller's single clock reading into the helper and gate on it, instead of taking a second racing read:
All six call sites (2 in
check(), 2 ingetSpending(), 2 ingetStatus()) already hadnowin scope and now pass it through. This keeps the guard meaningful — a genuinely historical window (to < now) still correctly excludes live "now" holds — while removing the second, racing read. (I deliberately avoided the terserto >= from, which is always true for these callers and would leave the guard vacuous in a file reviewers scrutinize closely.)Tests (failing-first)
New
describe("in-flight reservations under a live (non-frozen) clock")with an advancing clock (each read 1ms later), covering the hourly and daily windows. A comment on the block states explicitly why the frozen-clock suite missed this.expected true to be false).spend-control.test.tssuite 71 passed — every existing frozen-clock test unchanged and green.tsc --noEmit,prettier --check,eslint: clean.792 passed. The 2 unrelated failures on the tree (router/brand-numbers,router/free-model-liveness) are pre-existing catalog/brand-snapshot drift — confirmed by reproducing them with this change stashed. Zero new failures.Scope
Touches only
src/spend-control.tsandsrc/spend-control.test.ts. Independent of the unrelated/stats?daysfix.Summary by CodeRabbit