fix(keeper): rate-limit per-market event-driven liquidation scans - #366
fix(keeper): rate-limit per-market event-driven liquidation scans#366Morenikeoa wants to merge 1 commit into
Conversation
The LaserStream event path's 1s debounce (start()'s onAccount handler) only coalesces account updates *within* one window -- it does not bound the sustained rate of distinct windows. Each new update clears and restarts a fresh 1s timer for that market's slabKey, so an owner toggling their own account state just over 1s apart (well under the 60s polling interval) can force a full scanMarket() RPC fan-out (getProgramAccounts-style scan + fetchSlabWithRetry) on every settled window, repeatedly, for as long as the churn continues. This is invisible to every existing breaker: no liquidation transaction is ever sent (most of these resolve as "not liquidatable" and return early), so the SOL-spend circuit breaker in budget.ts never sees it -- only RPC quota and CPU are burned, with no rate limiter anywhere in account-loader.ts or liquidation.ts bounding event-trigger frequency. Adds a per-market minimum event-scan interval (5s) via a new _maybeRunEventScan() gate, checked when a debounce window settles. If a scan for that market already ran within the last 5s, the trigger is deferred (re- armed for the remaining wait) rather than executed immediately or dropped -- so sustained churn produces at most one scan per 5s window, never zero forever once churn pauses. BUG-104 from a clean-room Phase 4 audit pass.
|
Warning Review limit reached
More reviews will be available in 59 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
Independent verification — not an approval (QA/Security own that). Verdict: genuine. Plus a merge-order finding that affects this PR and #365. Method: ran the PR's tests first (35 passed), then removed only the rate-limit branch: if (elapsed < LiquidationService.MIN_EVENT_SCAN_INTERVAL_MS) { → if (false) {Result: 2 FAILED, and both are well-aimed:
The second is the one I'd have gone looking for. A rate limiter that drops rather than defers is the classic way this goes wrong — under sustained churn you'd starve the market of scans entirely and never liquidate, which is worse than the flood it was meant to fix. It's pinned by name. The debounce is correct — I checked the two usual bugs
Merge-order finding: this PR conflicts with #365Both this and #365 add tests to So the conflict is test-file only — the source changes are genuinely independent (#365 in the retry/backoff path, #366 in the event-scan path). Whichever of #365/#366 lands second needs a small manual resolve in the test file; nothing in A caveat on my own method, since I got this wrong twice before reporting it: my first run said "#365 conflicts" — that was my harness, because I'd deleted the local No changes requested on the code. |
Problem
The LaserStream event path in
LiquidationService.start()debounces account updates per market (_debounceTimers, keyed byslabKey, 1s window) before firingscanMarket(). That debounce only coalesces updates within one window — it does not bound the sustained rate of distinct windows. Each new update clears and restarts a fresh 1s timer for that market, so an owner toggling their own account state just over 1s apart (well under the 60s polling interval) can force a fullscanMarket()RPC fan-out (getProgramAccounts-style scan +fetchSlabWithRetry) on every settled window, repeatedly, for as long as the churn continues.This is invisible to every existing breaker: most of these resolve as "not liquidatable" inside
gatedLiquidate's pre-submit recheck and return early, so no liquidation transaction is ever sent — the SOL-spend circuit breaker inbudget.tsnever sees it. Only RPC quota and CPU are burned, and nothing inaccount-loader.tsorliquidation.tsbounds event-trigger frequency.Production Impact
A single misbehaving (or merely active) account can force the keeper into a sustained, much-higher-than-intended RPC call rate for its market, well beyond what the 60s polling cycle would generate — degrading RPC quota availability for all markets and burning CPU, with no operator-visible signal since no spend/success-rate metric is affected.
Fix
Added a per-market minimum event-scan interval (
MIN_EVENT_SCAN_INTERVAL_MS = 5_000) via a new_maybeRunEventScan()method, checked when a debounce window settles:Sustained churn now produces at most one scan per 5s window per market — a bounded ~12x reduction from the unbounded worst case — while still reacting roughly 12x faster than the 60s polling fallback once churn settles, and never starving a market permanently (the deferred scan always eventually runs).
Proof of Fix
New tests in
tests/services/liquidation.test.ts(BUG-104: per-market event-scan rate limit):Test Output
Full suite: 979 passed, 33 skipped, 1 pre-existing unrelated failure (
tests/v17-risk-params.poc.test.ts— stale assertion from #345, out of scope here).pnpm build— clean, zero errors.