fix(keeper): back off retries for a position that keeps failing liquidation

[Morenikeoa]

Problem

LiquidationService's dedup state is purely about concurrency, not history:

• _cycleSeenPositions / _cycleOwnerCounts are cleared every polling cycle (scanAndLiquidateAll).
• _inFlightPositions only guards true concurrent execution.

Nothing remembers that a liquidate() call for a given position failed. A position whose liquidation keeps failing — for example, an owner racing a cheap top-up timed to land between the keeper's scan and its pre-submit recheck, repeatedly flipping stillLiquidatable to false right before send — gets re-attempted at full transaction-fee cost on every single polling cycle and LaserStream event, forever, with zero increasing cost to the owner.

This is an asymmetric-cost DoS surface: the attacker's action (a tiny, cheap on-chain state change) costs far less than the keeper's resulting paid transaction attempt, and no existing breaker catches it — KeeperBudget's circuit breaker is global (cycle/hour/day spend + success-rate), not keyed per-position, so a slow background drain across several flapping accounts can sit under the global threshold indefinitely.

Production Impact

An attacker can keep N marginal accounts permanently flapping at the liquidation boundary, each costing the keeper a real priority-fee-bearing transaction every ~60s cycle (or faster via LaserStream-triggered re-evaluation), draining the keeper's hot wallet at a steady rate with no alert and no escalating cost to the attacker.

Fix

Added a per-position failure-backoff map (_positionBackoff) in LiquidationService, checked in gatedLiquidate() (the single entry point for both the polling and LaserStream paths) before the existing in-flight/cycle-dedup checks:

• The first failure is free — immediate retry permitted. A single recheck-abort (oracle moved, owner topped up once) is routine and must not delay a position that's still genuinely liquidatable. This preserves the existing H-1 test's documented intent ("a null (aborted) resolution must release the in-flight guard... otherwise every legitimate recheck-abort would permanently wedge the position").
• From the second consecutive failure onward, the retry cooldown escalates exponentially (5s, 10s, 20s, ... capped at 5 minutes — mirroring the existing cycle-level maxBackoffMs constant already used for whole-cycle failures).
• A successful liquidation clears the position's failure history.
• The position is never permanently skipped — the cooldown always expires, preserving the protocol guarantee that a genuinely undercollateralized position will eventually be liquidated.

This also saves the RPC cost of the expensive pre-submit recheck (oracle drift guard, stillLiquidatable, fresh slab fetch) for backed-off positions, not just the final send.

Proof of Fix

New tests in tests/services/liquidation.test.ts (BUG-103: per-position failure backoff):

• Allows an immediate retry after exactly one failure (free first failure)
• Throttles a position after repeated consecutive failures — a third immediate call does not re-invoke liquidate()
• Clears the backoff once a liquidation lands successfully

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.

[coderabbitai]

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 23 minutes and 38 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: 1705a237-7835-4dd6-9374-374d319701b8

📥 Commits

Reviewing files that changed from the base of the PR and between 8ee810d and 3cc6fa2.

📒 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[dcccrypto]

Independent verification — not an approval (QA/Security own that), just evidence for whoever reviews. Verdict: genuine. Plus a merge-compatibility check with a sibling PR, since both touch liquidation.ts.

Method: ran the PR's tests first (35 passed), then removed only the backoff skip:

if (backoff && Date.now() < backoff.retryAfter) {   →   if (false) {

Result:

1 FAILED
  × throttles a position after repeated consecutive failures instead of retrying every call

That's the behaviour the PR exists for, and it's the only test that moves — the other 34 cover paths that legitimately shouldn't change.

Merge compatibility with #390

liquidation.ts currently has six open PRs against it, and both this one and #390 also edit tests/services/liquidation.test.ts. I checked whether they can coexist rather than guessing:

#365 merged onto main        → OK
#390 merged on top of #365   → OK, no conflicts
combined liquidation.test.ts → 39 passed

So they're independent in practice — #365 sits around :650 and :963-1006 (scan/dispatch), #390 in the v17 pre-submit block at :1107+. Either order works.

A correction to my own first attempt, since it produced a scary answer: I initially reported a conflict to myself because I ran the second git merge while the first was still --no-commit — the failure was my test harness, not the branches. Committing the first merge before testing the second shows them clean. Flagging that because "these two PRs conflict" would have been a wrong and costly thing to tell you.

One design note

The backoff key is per-position, and _positionBackoff.delete(positionKey) on success means a position that recovers immediately regains full retry cadence — good, no lingering penalty after a transient failure.

Worth being aware of (not a change request): the map has no eviction. A keeper running for a long time across many markets accumulates one entry per position that has ever failed. Entries are tiny and only added on failure, so it's unlikely to matter — but if positions churn heavily it grows unboundedly, and the natural fix is to drop entries once retryAfter is well in the past during the same sweep that reads them.

No changes requested from me.