Fix/cancel orders expired holdinvoice continued - #884
Conversation
When the cancel-orders job expires an order, run the transition under the per-order mutex and re-read the order so it does not stomp an order that advanced concurrently (e.g. a release/payout in flight under the same lock). For ACTIVE orders the buyer never signalled fiat-sent, so cancel the hold invoice and refund the seller instead of orphaning the payment hash. Previously the order was marked EXPIRED without canceling the hold invoice, and since the expired-hold-invoice check ignores EXPIRED orders, the seller's funds stayed locked until the on-chain CLTV timeout. FIAT_SENT orders are not auto-refunded here (the buyer claims to have paid) and are left for the dispute/admin flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
package.json was bumped to 0.15.2 (commit 056284a) but package-lock.json still declared 0.15.1. The CI 'Run prettier' step runs 'npm install' followed by 'git diff --exit-code', and npm rewrites the lockfile version to match package.json, producing an uncommitted diff that fails the check. Sync the lockfile version so the working tree stays clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…canceled on expiry
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe expiration job now locks each order, reloads its state, cancels eligible hold invoices, notifies both parties, preserves ChangesOrder expiration handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jobs/cancel_orders.ts (1)
120-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the ACTIVE and FIAT_SENT handling into named helpers.
The mutex callback now covers reload, status guard, hold-invoice cancellation, dual notifications, dispute logging, and persistence in one function. Extracting the ACTIVE-branch logic (lines 140-167) and the FIAT_SENT-branch logic (lines 168-173) into two named helper functions would shorten the callback and make each branch independently testable.
♻️ Proposed extraction outline
+async function handleActiveOrderExpiration( + bot: HasTelegram, + updatedOrder: OrderDocument, +) { + if (!updatedOrder.hash) return; + await cancelHoldInvoice({ hash: updatedOrder.hash }); + const buyerUser = await User.findOne({ _id: updatedOrder.buyer_id }); + const sellerUser = await User.findOne({ _id: updatedOrder.seller_id }); + if (buyerUser === null || sellerUser === null) return; + const i18nCtxBuyer = await getUserI18nContext(buyerUser); + const i18nCtxSeller = await getUserI18nContext(sellerUser); + await messages.toBuyerHoldInvoiceExpiredMessage(bot, buyerUser, updatedOrder, i18nCtxBuyer); + await messages.toSellerHoldInvoiceExpiredMessage(bot, sellerUser, updatedOrder, i18nCtxSeller); +} + +function handleFiatSentOrderExpiration(updatedOrder: OrderDocument) { + if (!updatedOrder.hash) return; + logger.warn( + `Order Id ${updatedOrder.id} expired in FIAT_SENT with an open hold invoice; leaving it for dispute/admin handling`, + ); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobs/cancel_orders.ts` around lines 120 - 180, The mutex callback in the order-expiration flow is handling both status-specific branches inline; extract the ACTIVE handling and FIAT_SENT handling into two named helper functions. Move the existing hold-invoice cancellation and buyer/seller notification logic into the ACTIVE helper, and the warning log into the FIAT_SENT helper, then invoke them from the callback while preserving the current status and hash conditions and leaving persistence unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobs/cancel_orders.ts`:
- Around line 133-141: Ensure the ACTIVE-order expiration flow only sends the
ACTIVE notification and transitions the order to EXPIRED after cancelHoldInvoice
succeeds. In the surrounding cancel-order logic, inspect the result returned by
cancelHoldInvoice for updatedOrder.hash and stop or propagate the failure when
it returns undefined, preserving the existing retry behavior for later runs.
---
Nitpick comments:
In `@jobs/cancel_orders.ts`:
- Around line 120-180: The mutex callback in the order-expiration flow is
handling both status-specific branches inline; extract the ACTIVE handling and
FIAT_SENT handling into two named helper functions. Move the existing
hold-invoice cancellation and buyer/seller notification logic into the ACTIVE
helper, and the warning log into the FIAT_SENT helper, then invoke them from the
callback while preserving the current status and hash conditions and leaving
persistence unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a03f8ee-dbdd-4fbf-b23e-b546343ab2fd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
jobs/cancel_orders.ts
|
Hey @ToRyVand @knocte — this PR continues #837 (originally by @grunch, open since June with review comments from @Luquitasjeffrey that weren't addressed). I picked it up — added the buyer/seller notifications for the ACTIVE expiry case that Lucas flagged, and also fixed cancelHoldInvoice to re-throw LND errors so the cancellation failure is actually handled instead of swallowed. Would appreciate a review if you have a moment! Thanks! |
|
Nice work continuing this @Matobi98 🙏 The mutex + status re-check is the right way to avoid racing a release-in-flight, and leaving the order One thing worth a look: making Minor: FIAT_SENT still gets marked |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/jobs/cancel_orders.spec.ts (1)
32-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for an order that advances before the mutex callback.
Configure
expiredOrderswith an order ID, then makeOrder.findById()return an order in a non-expirable status such asCOMPLETED. Assert that the job does not cancel the invoice, save the order, or emitorderUpdated. This protects the status re-check that prevents a stale query result from overwriting a concurrent state transition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/jobs/cancel_orders.spec.ts` at line 32, Extend the test setup around orderFindByIdStub to configure expiredOrders with an order ID and return a COMPLETED order from Order.findById(). Assert that the job neither cancels the invoice, saves the order, nor emits orderUpdated when the order advances before the mutex callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/jobs/cancel_orders.spec.ts`:
- Around line 67-70: Apply the repository’s Prettier formatting to the proxy
object containing toBuyerExpiredOrderMessage and toSellerExpiredOrderMessage,
using npm run format, and retain the generated formatting changes without
disabling lint or formatting rules.
---
Nitpick comments:
In `@tests/jobs/cancel_orders.spec.ts`:
- Line 32: Extend the test setup around orderFindByIdStub to configure
expiredOrders with an order ID and return a COMPLETED order from
Order.findById(). Assert that the job neither cancels the invoice, saves the
order, nor emits orderUpdated when the order advances before the mutex callback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f975cab3-ad1e-4b50-88b1-71ac39dab711
📒 Files selected for processing (1)
tests/jobs/cancel_orders.spec.ts
|
Good catch @ToRyVand — I audited all eight call sites (commands.ts lines 322, 534, 711, 790; start.ts lines 445, 534; check_hold_invoice_expired.ts:45; and cancel_orders.ts:142). In every case they're wrapped in a try/catch that logs the error, so a LND failure now causes the operation to abort instead of proceeding with inconsistent state (e.g. marking an order CANCELED when the hold invoice is still open). That's a behavior change, but a safer one: previously the order could end up in a wrong terminal state with its hold invoice still live; now it stays in its current state and retries or lets the user try again. The tradeoff is that the user gets no feedback when LND is down — I've opened a follow-up issue for that. #899 I've also added explicit tests for the cancelHoldInvoice failure path in cancel_orders.ts and updated the PR description to document the behavior change across call sites. |
|
Thanks ToRyVand! Peer review approved — good to merge on my end. |
3 participants
Summary
Continues #837 (originally by @grunch).
Ensures that when the cancel-orders job expires an
ACTIVEorder, the associated Lightning hold invoice is canceled so the seller's locked funds are refunded promptly, instead of being orphaned until the on-chain CLTV timeout. The expiry transition is serialized under the per-order mutex with a state re-check to avoid racing concurrent flows.Previously, the job marked expired orders as
EXPIREDwithout canceling the hold invoice. Because the expired-hold-invoice check ignoresEXPIREDorders, the seller's funds remained locked until the on-chain timeout.Changes
jobs/cancel_orders.ts— Run the expiry transition insidePerOrderIdMutex.instance.runExclusive(orderId, ...)and re-read the order under the lock so the job does not stomp an order that advanced concurrently. OnlyACTIVEandFIAT_SENTorders are expired:ACTIVE— the buyer never signalled fiat-sent, so cancel the hold invoice (refund the seller) before marking the orderEXPIRED, and notify both buyer and seller that the sats are no longer in escrow. This addresses @Luquitasjeffrey's review comment on fix: cancel hold invoice when expiring ACTIVE orders #837: without the notification, a malicious seller could keep the fiat payment after the order expired silently, since the buyer would have no way of knowing the escrow was no longer backing the trade.FIAT_SENT— the buyer claims to have paid; the hold invoice is left open and the case is logged for the dispute/admin flow rather than auto-refunded.ln/hold_invoice.ts—cancelHoldInvoicenow re-throws LND errors instead of swallowing them, consistent withsettleHoldInvoicewhich already had this behavior with the comment "Callers must not mark an order as settled/frozen if the invoice settlement did not happen." The same reasoning applies here: callers must not mark an orderEXPIREDif the hold invoice cancellation did not happen. This is a prerequisite for the error handling incancel_orders.tsto work correctly — ifcancelHoldInvoicefails, theACTIVEorder is left untouched so the next job run retries the cancellation rather than silently leaving the seller's funds locked.Behavior change for other call sites:
cancelHoldInvoiceis also called frombot/commands.ts(cooperative cancel, seller cancel, admin cancel) andbot/start.ts. All of these are wrapped in outertry/catchblocks that log the error. With this change, a LND failure during those flows will now abort the operation silently instead of proceeding with inconsistent state (e.g. marking an orderCANCELEDwhile its hold invoice is still live). The order stays in its previous state and the user can retry — safer than before, but without user-facing feedback on failure. A follow-up issue will address surfacing an error message to the user in these cases.Testing
Manually tested end-to-end against a local regtest LND setup (temporarily lowering
ORDER_PUBLISHED_EXPIRATION_WINDOW/HOLD_INVOICE_EXPIRATION_WINDOWto speed up expiry):ACTIVEorder expiry: hold invoice canceled in LND (confirmed viasubscribeInvoiceterminal statecanceled=true), both parties notifiedFIAT_SENTorder expiry: hold invoice left open, warning logged, left for dispute/adminWAITING_PAYMENT/WAITING_BUYER_INVOICEexpirynpx tsc --noEmit,eslint,prettier --checkpass on changed filesSummary by CodeRabbit