Skip to content

fix(keeper): rate-limit per-market event-driven liquidation scans - #366

Open
Morenikeoa wants to merge 1 commit into
dcccrypto:mainfrom
Morenikeoa:fix/liquidation-event-scan-rate-limit
Open

fix(keeper): rate-limit per-market event-driven liquidation scans#366
Morenikeoa wants to merge 1 commit into
dcccrypto:mainfrom
Morenikeoa:fix/liquidation-event-scan-rate-limit

Conversation

@Morenikeoa

Copy link
Copy Markdown
Contributor

Problem

The LaserStream event path in LiquidationService.start() debounces account updates per market (_debounceTimers, keyed by slabKey, 1s window) before firing scanMarket(). 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 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: 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 in budget.ts never sees it. Only RPC quota and CPU are burned, and nothing in account-loader.ts or liquidation.ts bounds 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:

  • If no prior scan has run for the market, proceed immediately (first event always reacts fast).
  • If a scan for that market ran within the last 5s, defer (re-arm the timer for the remaining wait) rather than executing immediately or dropping the trigger.

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):

  • Confirms the existing within-window debounce-coalescing behavior is unaffected
  • A second event-scan arriving within the 5s window is deferred, not dropped — and runs once the window elapses
  • Continuous churn over 11s of simulated time produces far fewer scans than updates, but at least one (no permanent starvation)

Test Output

 Test Files  1 passed (1)
      Tests  35 passed (35)

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.

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.
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Morenikeoa, we couldn't start this review because you've reached your PR review rate limit.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c5c54281-11ff-4f42-9071-6336860c5b8f

📥 Commits

Reviewing files that changed from the base of the PR and between 8ee810d and 83f31ee.

📒 Files selected for processing (2)
  • src/services/liquidation.ts
  • tests/services/liquidation.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dcccrypto

Copy link
Copy Markdown
Owner

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:

  • defers (does not drop) a second event-scan that arrives within MIN_EVENT_SCAN_INTERVAL_MS
  • continuous churn produces no more than one scan per rate-limit window, never zero forever

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

  • Orphaned timers: the entry path at :1664 does clearTimeout(existing) before scheduling. The self-scheduling call at :1554 overwrites the map entry without clearing, but it only ever runs from an already-fired timer, so there's nothing live to clear. No accumulation.
  • Shutdown leak: stop() clears every pending timer and empties the map (:1682-1683), so a SIGTERM mid-debounce doesn't leave the handle open.

Merge-order finding: this PR conflicts with #365

Both this and #365 add tests to tests/services/liquidation.test.ts. Verified by actually merging rather than guessing:

main + #365            → OK
main + #365 + #366     → CONFLICT in tests/services/liquidation.test.ts
                         (src/services/liquidation.ts merges CLEAN)
main + #366 + my #390  → OK, 39 passed
main + #365 + my #390  → OK, 39 passed

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 liquidation.ts itself is at risk. My #390 is compatible with either, in any order.

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 pr365 branch after reviewing it, so git merge pr365 errored rather than conflicted. The tell was an empty conflicted-files list. The numbers above come from a run that verifies each branch exists and distinguishes a real conflict (non-empty --diff-filter=U) from a merge that simply failed to start.

No changes requested on the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants