feat: add job to check for expired hold invoices#746
feat: add job to check for expired hold invoices#746ndungudedan wants to merge 2 commits intolnp2pBot:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a scheduled job to detect expired LN hold invoices, cancel them, mark orders as Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In `@jobs/check_hold_invoice_expired.ts`:
- Around line 21-29: The current loop fetches ordersWithHoldInvoice before
acquiring PerOrderIdMutex, causing a TOCTOU race where another process may
change order.status; inside the runExclusive callback (for the same
String(order._id)) re-fetch the latest Order by id (e.g.,
Order.findById(order._id)), verify its status still matches the expected
precondition (the status you intended to expire, e.g., 'HOLD_INVOICE' or
whatever predicate you used to build ordersWithHoldInvoice), and only then set
status = 'HOLD_INVOICE_EXPIRED', await order.save(), and call
OrderEvents.orderUpdated(order); if the re-fetched order no longer meets the
precondition, skip updating. Ensure you reference
PerOrderIdMutex.instance.runExclusive, order._id, Order.findById (or
equivalent), order.save, and OrderEvents.orderUpdated.
- Around line 24-27: The job currently sets order.status to
'HOLD_INVOICE_EXPIRED' and emits OrderEvents.orderUpdated but misses cancelling
the hold invoice and notifying users; modify the handler in
jobs/check_hold_invoice_expired.ts to first call and await cancelHoldInvoice({
hash: order.hash }) (wrapped in try/catch so failures are logged but do not
block cleanup), then update order.status and save, and after saving send the
standard notifications to both parties using
messages.toSellerDidntPayInvoiceMessage() and
messages.toBuyerSellerDidntPayInvoiceMessage() (or
messages.expiredOrderMessage() where appropriate) so both buyer and seller are
explicitly informed; ensure the sequence matches other flows (cancelHoldInvoice
→ set status/save → send messages → OrderEvents.orderUpdated) and that calls are
idempotent.
🧹 Nitpick comments (1)
bot/start.ts (1)
242-244: The job doesn't receive thebotinstance, limiting its ability to notify users.Most other scheduled jobs (e.g.,
cancelOrders,deleteOrders,nodeInfo) receive thebotinstance to send Telegram messages. If user notifications are added (as suggested in the job implementation review), thebotreference will be needed here.Future-proof by passing bot
schedule.scheduleJob(`0 * * * * *`, async () => { - await checkHoldInvoiceExpired(); + await checkHoldInvoiceExpired(bot); });
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/bot/bot.spec.ts (1)
307-315:⚠️ Potential issue | 🟡 MinorMissing
checkHoldInvoiceExpiredstub in the../jobsmock.The
../jobsproxy (lines 307–315) doesn't include a stub for the newly addedcheckHoldInvoiceExpiredjob. While thescheduleJobcall count was bumped to 8, the job function itself will beundefinedin the mock, so the test doesn't actually verify that the new job is wired correctly.Add the stub and assert it is scheduled with the expected cron expression (
* * * * *— every minute, per the PR description):Proposed fix
'../jobs': { attemptPendingPayments: attemptPendingPaymentsStub, cancelOrders: sinon.stub().resolves(), deleteOrders: sinon.stub().resolves(), calculateEarnings: sinon.stub().resolves(), attemptCommunitiesPendingPayments: sinon.stub().resolves(), // deleteCommunity: sinon.stub().resolves(), nodeInfo: sinon.stub().resolves(), + checkHoldInvoiceExpired: sinon.stub().resolves(), },And optionally verify the cron expression for the new job in the assertion block (after line 466):
expect(scheduleStub.scheduleJob.callCount).to.be.equal(8); + // Verify the new hold-invoice-expired job is scheduled every minute + const calls = scheduleStub.scheduleJob.getCalls(); + const holdInvoiceCall = calls.find((c: any) => c.args[0] === '* * * * *'); + expect(holdInvoiceCall).to.not.be.undefined;Based on learnings: "Add fixtures in
tests/bot/mocks/when altering localization or jobs."Also applies to: 466-466
- Implemented a new job `checkHoldInvoiceExpired` to update orders with expired hold invoices. - Scheduled the job to run every minute in the bot's initialization. - Added a new status 'HOLD_INVOICE_EXPIRED' to the order model for tracking expired invoices.
ndungudedan
force-pushed
the
change-order-status
branch
from
7aa4435 to
cf4bc73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
jobs/check_hold_invoice_expired.ts (1)
60-75: Notifications are silently skipped when buyer or seller is missing.If either
buyerUserorsellerUserisnull, both notifications are skipped. In practice, both should always exist for orders in these statuses, but a log message would help diagnose issues if this ever happens.Suggested improvement
if (buyerUser !== null && sellerUser !== null) { // ... send messages ... + } else { + logger.warning( + `checkHoldInvoiceExpired: buyer or seller not found for order ${updatedOrder._id}`, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobs/check_hold_invoice_expired.ts` around lines 60 - 75, The current conditional skips both notifications if either buyerUser or sellerUser is null; change the logic to handle each recipient independently: call getUserI18nContext and messages.toBuyerHoldInvoiceExpiredMessage when buyerUser is non-null, and do the same separately for sellerUser when non-null, and add a processLogger.warn (or similar) when either buyerUser or sellerUser is missing to surface the unexpected state; reference buyerUser, sellerUser, getUserI18nContext, messages.toBuyerHoldInvoiceExpiredMessage, messages.toSellerHoldInvoiceExpiredMessage, bot and updatedOrder when making the changes.tests/bot/bot.spec.ts (1)
466-466: Count bump is correct, but the'../jobs'mock is missingcheckHoldInvoiceExpired.The schedule count correctly increased to 8. However, the
'../jobs'mock (lines 307-315) doesn't include acheckHoldInvoiceExpiredstub. While this doesn't break the test (sincescheduleJobis stubbed and never executes callbacks), adding it would make the mock complete and allow future assertions on the job's invocation.Additionally, there's no dedicated unit test for the
checkHoldInvoiceExpiredjob itself. Given that this job modifies order state, cancels hold invoices, and sends user notifications, unit tests would be valuable. Based on learnings, fixtures should be added intests/bot/mocks/when altering jobs.Add missing stub
'../jobs': { attemptPendingPayments: attemptPendingPaymentsStub, cancelOrders: sinon.stub().resolves(), deleteOrders: sinon.stub().resolves(), calculateEarnings: sinon.stub().resolves(), attemptCommunitiesPendingPayments: sinon.stub().resolves(), // deleteCommunity: sinon.stub().resolves(), nodeInfo: sinon.stub().resolves(), + checkSolvers: sinon.stub().resolves(), + checkHoldInvoiceExpired: sinon.stub().resolves(), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bot/bot.spec.ts` at line 466, The '../jobs' mock used in tests is missing a stub for checkHoldInvoiceExpired; add a stub/export named checkHoldInvoiceExpired alongside the existing mocked jobs (the same style as the other job stubs around the mock block) so scheduleJob can be asserted against it later, and add a new unit test for the checkHoldInvoiceExpired job (exercise the checkHoldInvoiceExpired handler, assert it updates order state, cancels hold invoices, and sends notifications) with required fixtures placed into tests/bot/mocks/ per the existing fixture patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jobs/check_hold_invoice_expired.ts`:
- Line 17: The job only updates order status for orders in orderStatuses
(['ACTIVE','DISPUTE','FIAT_SENT']) but does not update the related Dispute
record when an order in DISPUTE is moved to 'HOLD_INVOICE_EXPIRED', leaving
stale disputes; import the Dispute model/class in
jobs/check_hold_invoice_expired.ts and when you set an order's status to
'HOLD_INVOICE_EXPIRED' for orders that were 'DISPUTE', also find the associated
Dispute (by orderId or dispute.orderId) and update its status to
'SELLER_REFUNDED' (same flow as cancelorder) and save the change so solvers'
queues are cleared.
---
Nitpick comments:
In `@jobs/check_hold_invoice_expired.ts`:
- Around line 60-75: The current conditional skips both notifications if either
buyerUser or sellerUser is null; change the logic to handle each recipient
independently: call getUserI18nContext and
messages.toBuyerHoldInvoiceExpiredMessage when buyerUser is non-null, and do the
same separately for sellerUser when non-null, and add a processLogger.warn (or
similar) when either buyerUser or sellerUser is missing to surface the
unexpected state; reference buyerUser, sellerUser, getUserI18nContext,
messages.toBuyerHoldInvoiceExpiredMessage,
messages.toSellerHoldInvoiceExpiredMessage, bot and updatedOrder when making the
changes.
In `@tests/bot/bot.spec.ts`:
- Line 466: The '../jobs' mock used in tests is missing a stub for
checkHoldInvoiceExpired; add a stub/export named checkHoldInvoiceExpired
alongside the existing mocked jobs (the same style as the other job stubs around
the mock block) so scheduleJob can be asserted against it later, and add a new
unit test for the checkHoldInvoiceExpired job (exercise the
checkHoldInvoiceExpired handler, assert it updates order state, cancels hold
invoices, and sends notifications) with required fixtures placed into
tests/bot/mocks/ per the existing fixture patterns.
- Updated the `checkHoldInvoiceExpired` job to handle orders in 'DISPUTE' status. - Modified the query to include 'DISPUTE' in the list of order statuses with expired hold invoices. - Added logic to update the corresponding dispute status to 'SELLER_REFUNDED' when an order in dispute is processed.
ndungudedan
force-pushed
the
change-order-status
branch
from
1cbfb07 to
68ec6b7
Compare
2 participants
checkHoldInvoiceExpiredto update orders with expired hold invoices.Ressolves: #362
Summary by CodeRabbit
New Features
Status
Localization
Tests