fix(deployment): close managed deployments once every lease on them has closed - #3939
Conversation
…as closed The stale-deployment sweep only ever selected deployments with no lease row at all. A deployment whose provider closed its lease kept an open, lease-less deployment on chain for good: no other closer applies, since the unreachable-provider sweeps and the runtime-limit job both key off an active lease. Three deployments from June are still open this way for one user, each with a lease the provider closed within minutes to hours of creation. The sweep now treats a deployment as stale when it has no active lease and its last lease closed before the same ten-minute cutoff that already applies to its creation height. Bids expire well inside that window, so a group left without a lease past the cutoff cannot get one through Console anyway, and closing returns whatever escrow remains to the wallet.
|
Warning Review limit reached
On-demand reviews are free for the next 7 days. After that, they cost $0.25 per reviewed file. Or wait 48 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 50 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3939 +/- ##
==========================================
- Coverage 82.35% 81.78% -0.58%
==========================================
Files 1280 1180 -100
Lines 35404 32799 -2605
Branches 8566 8046 -520
==========================================
- Hits 29158 26824 -2334
+ Misses 5515 5262 -253
+ Partials 731 713 -18
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the reported (zero) findings, I also traced the widened staleness definition into its other call site: cleanUpForWallet(wallet, 0) is invoked from reclaimTrialOrphanedDeployments before every trial-wallet deployment create, so this fix now also closes trial deployments whose lease closed very recently (not just never-leased ones) — this matches the PR's stated intent rather than being a side effect, but it's worth a human confirming that broadened blast radius is acceptable.
Extended reasoning...
The diff renames createdHeight to staleBeforeHeight and rewrites the HAVING clause in findStaleDeployments so a deployment counts as stale only when it has zero currently-open leases (COUNT(...) FILTER (WHERE closedHeight IS NULL) = 0) and its last closed lease closed before the cutoff (COALESCE(MAX(closedHeight), 0) < staleBeforeHeight), replacing the old buggy COUNT(leases) = 0 check that ignored closed leases entirely. A new asHeight() guard validates the height is a safe integer before it is string-interpolated into the raw SQL literal, mitigating injection risk from that one non-parameterized value (the other height usage stays parameterized via Op.lt). New integration tests cover never-leased, all-leases-closed-before-cutoff, last-lease-closed-at/after-cutoff, still-active-lease, created-at/after-cutoff, already-closed, and other-owner scenarios, and correctly use the repo's setup() convention.
I independently re-derived the SQL logic and it matches the stated semantics, including the "never leased" case falling out of COALESCE(..., 0) < staleBeforeHeight. I also checked the one behavior-widening consequence flagged by the bug hunt: cleanUpForWallet(wallet, 0) is called synchronously from deployment-writer.service.ts's reclaimTrialOrphanedDeployments before every trial-wallet deployment create, so the broadened staleness definition (any deployment with no active lease and last-closed-lease height below the near-current cutoff) now applies there too, not just in the twice-daily managed-wallet cron. This looks like the intended fix rather than an unintended side effect, but it does mean closes are triggered synchronously on a hot path (deployment creation) for a larger population than before.
Given this touches on-chain fund-closing logic (escrow/deposit release) and a subtle SQL correctness fix that is easy to get subtly wrong, plus a real (if likely intended) widening of behavior on a synchronous hot path, I'm choosing to defer with a brief note on what else was checked, rather than stay fully silent or approve outright.
1 participant
Why
A managed deployment whose leases have all been closed by the provider stays open on chain with no lease, no spend, and whatever deposit is left locked in escrow, and nothing in Console ever closes it. One user has three such deployments from June 22: each got a lease that the provider closed within minutes to hours (one with
lease_closed_reason_unstable), and all three are stillactivetoday.The
cleanup-stale-deploymentscron is the only closer that does not need a lease, butfindStaleDeploymentsselected deployments withCOUNT(leases) = 0, counting closed lease rows too. Every other closer (unreachable-provider sweeps, runtime-limit job, trial closers) starts from an active lease, so a deployment whose leases all closed is out of reach for all of them.What
findStaleDeploymentsnow counts only active leases, and also requires the last lease close height to be below the cutoff, so a lease closed seconds ago is not acted on until the same ten-minute window that already applies to the creation height has passed. Bids expire well inside that window, so a group without a lease past the cutoff cannot get one through Console anyway.createdHeighttostaleBeforeHeightsince it now bounds both heights. The cleaner's private method is renamed to match what it does.findStaleDeploymentscovering never-leased, all-leases-closed, lease closed after the cutoff, active lease, created after the cutoff, already closed, and other owners.Before merging it is worth sizing the first run: count open deployments on managed wallets that have lease rows but no active lease. The twice-daily cron closes each wallet's batch in one derived tx and already handles the unsettleable-escrow and already-closed cases.