Late payment flow - #705
Conversation
…oice which just got paid (subscribeToInvoice) to run at the same time
… all invoices for being processed when the invoice is being hold
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a per-order mutex (PerOrderIdMutex) and uses it to serialize invoice/payment flows: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
jobs/cancel_orders.ts(2 hunks)ln/subscribe_invoice.ts(3 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
ln/subscribe_invoice.ts
[failure] 10-10:
'order' is defined but never used
[failure] 57-57:
Expected space or tab after '//' in comment
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
| //We need to get an updated version of the order because there is a chance of the cancelOrders coroutine to modify the state of the order | ||
| const updatedOrder = await Order.findById(order._id); | ||
| if (updatedOrder === null) throw new Error('order was not found after locking'); | ||
| if (updatedOrder.status !== 'WAITING_PAYMENT') { | ||
| logger.error(`Order ${updatedOrder._id} status is not WAITING_PAYMENT on subscribeToInvoice. Actual status: ${updatedOrder.status}`); | ||
| throw new Error('Order status is not WAITING_PAYMENT'); | ||
| } |
There was a problem hiding this comment.
Don’t throw when the order is already processed.
Inside the invoice listener we now throw if the refreshed order isn’t WAITING_PAYMENT. When that happens (e.g., cancel job just ran), the rejection bubbles out of the event handler and ends up as an unhandled promise rejection, jeopardizing the subscriber. Log and return instead of throwing so the handler finishes cleanly.
- if (updatedOrder.status !== 'WAITING_PAYMENT') {
- logger.error(`Order ${updatedOrder._id} status is not WAITING_PAYMENT on subscribeToInvoice. Actual status: ${updatedOrder.status}`);
- throw new Error('Order status is not WAITING_PAYMENT');
- }
+ if (updatedOrder.status !== 'WAITING_PAYMENT') {
+ logger.warn(
+ `Order ${updatedOrder._id} status moved to ${updatedOrder.status} before handling held invoice; skipping`,
+ );
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //We need to get an updated version of the order because there is a chance of the cancelOrders coroutine to modify the state of the order | |
| const updatedOrder = await Order.findById(order._id); | |
| if (updatedOrder === null) throw new Error('order was not found after locking'); | |
| if (updatedOrder.status !== 'WAITING_PAYMENT') { | |
| logger.error(`Order ${updatedOrder._id} status is not WAITING_PAYMENT on subscribeToInvoice. Actual status: ${updatedOrder.status}`); | |
| throw new Error('Order status is not WAITING_PAYMENT'); | |
| } | |
| // We need to get an updated version of the order because there is a chance of the cancelOrders coroutine to modify the state of the order | |
| const updatedOrder = await Order.findById(order._id); | |
| if (updatedOrder === null) throw new Error('order was not found after locking'); | |
| if (updatedOrder.status !== 'WAITING_PAYMENT') { | |
| logger.warn( | |
| `Order ${updatedOrder._id} status moved to ${updatedOrder.status} before handling held invoice; skipping`, | |
| ); | |
| return; | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 57-57:
Expected space or tab after '//' in comment
🤖 Prompt for AI Agents
In ln/subscribe_invoice.ts around lines 57 to 63, the handler currently throws
when the refreshed order is missing or its status is not WAITING_PAYMENT; change
this so the function logs the condition and returns instead of throwing (e.g.,
logger.error(...) then return) for the non-WAITING_PAYMENT case (and likewise
handle a null updatedOrder by logging and returning) so the event handler
finishes cleanly and does not propagate a rejection.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fix the comment spacing to keep eslint happy. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ln/subscribe_invoice.ts (1)
59-62: Don't throw when the order is already processed.This issue was flagged in a previous review but remains unaddressed. Inside the invoice listener, throwing when the refreshed order isn't
WAITING_PAYMENTcauses an unhandled promise rejection that can destabilize the subscriber. Log and return instead.if (updatedOrder.status !== 'WAITING_PAYMENT') { - logger.error(`Order ${updatedOrder._id} status is not WAITING_PAYMENT on subscribeToInvoice. Actual status: ${updatedOrder.status}`); - throw new Error('Order status is not WAITING_PAYMENT'); + logger.warn( + `Order ${updatedOrder._id} status moved to ${updatedOrder.status} before handling held invoice; skipping`, + ); + return; }
🧹 Nitpick comments (1)
ln/subscribe_invoice.ts (1)
34-34: Use strict equality operator.For consistency with the codebase, use
===instead of==.-if (mtx.lockCount==0) { +if (mtx.lockCount === 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ln/subscribe_invoice.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
ln/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Encapsulate Lightning Network operations (hold invoices, payments, subscriptions) under ln/
Files:
ln/subscribe_invoice.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use Winston for logging (avoid console) and include contextual details and timeout monitoring where relevant
Wrap risky operations in try-catch with meaningful error context
Files:
ln/subscribe_invoice.ts
🧬 Code graph analysis (1)
ln/subscribe_invoice.ts (3)
models/index.ts (2)
Order(9-9)User(9-9)logger.ts (1)
logger(59-59)util/index.ts (3)
getUserI18nContext(615-615)getEmojiRate(607-607)decimalRound(608-608)
🪛 GitHub Actions: NodeJS Continuous Integration
ln/subscribe_invoice.ts
[error] 1-1: Formatting did not match. Run 'npm run format' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
ln/subscribe_invoice.ts (1)
1-1: Prettier CLI not found; install dependencies and re-run formatting
The formatting check failed becauseprettierwasn’t available. Runnpm installto restore devDependencies and thennpm run formatto fix style issues in ln/subscribe_invoice.ts.
| await PerOrderIdMutex.instance.runExclusive(String(order._id), async() => { | ||
| // We need to get an updated version of the order because there is a chance of the cancelOrders coroutine to modify the state of the order | ||
| const updatedOrder = await Order.findById(order._id); | ||
| if (updatedOrder === null) throw new Error('order was not found after locking'); |
There was a problem hiding this comment.
Also handle missing order gracefully.
Similarly, throwing when updatedOrder is null will cause an unhandled rejection. Log and return instead.
const updatedOrder = await Order.findById(order._id);
-if (updatedOrder === null) throw new Error('order was not found after locking');
+if (updatedOrder === null) {
+ logger.error(`Order ${order._id} not found after locking; skipping`);
+ return;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (updatedOrder === null) throw new Error('order was not found after locking'); | |
| const updatedOrder = await Order.findById(order._id); | |
| if (updatedOrder === null) { | |
| logger.error(`Order ${order._id} not found after locking; skipping`); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In ln/subscribe_invoice.ts around line 58, the code currently throws an Error
when updatedOrder is null which can create an unhandled rejection; change this
to handle the missing order gracefully by logging the condition (use the
module's logger or console.error with contextual info such as order id or
invoice id) and then return early from the function instead of throwing so
execution stops cleanly and no unhandled rejection occurs.
…ING_PAYMENT, just debug the message and return without exceptions
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ln/subscribe_invoice.ts (1)
52-129: Wrap the invoice_updated handler in try/catch to prevent unhandled rejections.Throws inside async event handlers aren’t caught by EventEmitter and can surface as unhandled rejections. Wrap the handler body.
- sub.on('invoice_updated', async invoice => { - if (invoice.is_held && !resub) { + sub.on('invoice_updated', async invoice => { + try { + if (invoice.is_held && !resub) { // ... - ); - } - if (invoice.is_confirmed) { + ); + } + if (invoice.is_confirmed) { // ... - await payHoldInvoice(bot, order); - } - }); + await payHoldInvoice(bot, order); + } + } catch (error) { + logger.error(`invoice_updated handler error for invoice ${id}`, error); + } + });
🧹 Nitpick comments (8)
ln/subscribe_invoice.ts (5)
34-37: Use strict equality for lockCount.Prefer
===to avoid coercion.- if (mtx.lockCount == 0) { + if (mtx.lockCount === 0) { this.mutexes.delete(orderId); }
11-43: Extract PerOrderIdMutex to a small shared module.Importing it from ln/subscribe_invoice creates cross‑layer coupling for jobs. Move it to a dedicated util (e.g., util/concurrency/perOrderIdMutex.ts) and import from there in both places.
64-67: Downgrade log severity to warn for expected status drift.When another coroutine advances the order, this is a normal race outcome; use warn.
- logger.error( + logger.warn( `Order ${updatedOrder._id} status is not WAITING_PAYMENT on subscribeToInvoice. Actual status: ${updatedOrder.status}`, );
72-80: Parallelize user lookups and i18n context building.Cuts latency by issuing independent awaits concurrently.
- const buyerUser = await User.findOne({ - _id: updatedOrder.buyer_id, - }); - if (buyerUser === null) throw new Error('buyerUser was not found'); - const sellerUser = await User.findOne({ - _id: updatedOrder.seller_id, - }); - if (sellerUser === null) - throw new Error('sellerUser was not found'); + const [buyerUser, sellerUser] = await Promise.all([ + User.findOne({ _id: updatedOrder.buyer_id }), + User.findOne({ _id: updatedOrder.seller_id }), + ]); + if (buyerUser === null) throw new Error('buyerUser was not found'); + if (sellerUser === null) throw new Error('sellerUser was not found'); @@ - const i18nCtxBuyer = await getUserI18nContext(buyerUser); - const i18nCtxSeller = await getUserI18nContext(sellerUser); + const [i18nCtxBuyer, i18nCtxSeller] = await Promise.all([ + getUserI18nContext(buyerUser), + getUserI18nContext(sellerUser), + ]);Also applies to: 83-85
21-33: Optionally add a timeout when acquiring locks.If a critical section stalls, queued tasks may wait indefinitely. Consider
withTimeout(mutex.acquire(), ms, err)when manual control is acceptable.Based on learnings
jobs/cancel_orders.ts (3)
32-50: Also serialize WAITING_BUYER_INVOICE cancellations.Mirror the same per‑order lock to avoid racing with other order updates.
- } else { - await cancelAddInvoice(bot as CommunityContext, order, true); - } + } else { + await PerOrderIdMutex.instance.runExclusive( + String(order._id), + async () => { + const updatedOrder = await Order.findById(order._id); + if ( + !updatedOrder || + updatedOrder.status !== 'WAITING_BUYER_INVOICE' + ) + return; + await cancelAddInvoice( + bot as CommunityContext, + updatedOrder, + true, + ); + }, + ); + }
37-41: Log when skipping due to status change.A debug/warn helps trace why a cancel didn’t run.
- if (!updatedOrder || updatedOrder.status !== 'WAITING_PAYMENT') - return; + if (!updatedOrder || updatedOrder.status !== 'WAITING_PAYMENT') { + logger.debug( + `Skip cancel for order ${order._id}: status is ${updatedOrder?.status ?? 'missing'}`, + ); + return; + }
9-9: Decouple mutex location.Import PerOrderIdMutex from a small concurrency util (e.g., util/concurrency/perOrderIdMutex) rather than ln/subscribe_invoice to avoid cross‑module coupling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jobs/cancel_orders.ts(2 hunks)ln/subscribe_invoice.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
jobs/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
jobs/**/*.ts: Implement background jobs for pending payments, order expiration/cleanup, community earnings, node health, and solver availability
Use the specified scheduling intervals: pending payments every 5m (PENDING_PAYMENT_WINDOW), order cancellation every 20s, order deletion hourly at :25, community earnings every 10m, node health every 1m, solver availability daily at midnight
Files:
jobs/cancel_orders.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use Winston for logging (avoid console) and include contextual details and timeout monitoring where relevant
Wrap risky operations in try-catch with meaningful error context
Files:
jobs/cancel_orders.tsln/subscribe_invoice.ts
ln/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Encapsulate Lightning Network operations (hold invoices, payments, subscriptions) under ln/
Files:
ln/subscribe_invoice.ts
🧠 Learnings (1)
📚 Learning: 2025-10-09T21:28:38.451Z
Learnt from: Luquitasjeffrey
PR: lnp2pBot/bot#705
File: ln/subscribe_invoice.ts:17-42
Timestamp: 2025-10-09T21:28:38.451Z
Learning: In JavaScript, synchronous code sections between `await` points execute atomically without interleaving from other async operations. Check-then-act patterns are safe if both the check and act occur in the same synchronous section before any `await`. Race conditions in JavaScript async code only occur when state is checked before an `await` and then acted upon after the `await` completes.
Applied to files:
ln/subscribe_invoice.ts
🧬 Code graph analysis (2)
jobs/cancel_orders.ts (3)
ln/subscribe_invoice.ts (1)
PerOrderIdMutex(203-203)models/index.ts (1)
Order(9-9)bot/commands.ts (1)
cancelShowHoldInvoice(880-880)
ln/subscribe_invoice.ts (3)
models/index.ts (2)
Order(9-9)User(9-9)logger.ts (1)
logger(59-59)util/index.ts (3)
getUserI18nContext(615-615)getEmojiRate(607-607)decimalRound(608-608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: ci_to_main
🔇 Additional comments (2)
ln/subscribe_invoice.ts (1)
18-43: LGTM: Per-order mutex serialization is correct.The per-key Mutex + lockCount cleanup ensures only one critical section per order id, and JS run‑to‑completion semantics make the pre‑await map ops atomic. Nice.
jobs/cancel_orders.ts (1)
34-47: LGTM: Serialized cancellation for WAITING_PAYMENT.Refreshing inside the lock before canceling avoids conflicts with the invoice handler. Good.
* Preventing the cancelOrders job and the code processing of a hold invoice which just got paid (subscribeToInvoice) to run at the same time * Improving performance by locking only by order id instead of blocking all invoices for being processed when the invoice is being hold * Deleting the subscribeToInvoicesMutex because is no longer being used * Removing debug logs on cancelOrders job (late-payment-flow) * Remove import of unused variable "order" Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update ln/subscribe_invoice.ts Fix the comment spacing to keep eslint happy. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Instead of throwing an error when the status of the order is not WAITING_PAYMENT, just debug the message and return without exceptions * Formatting the code after running 'npm run format' --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Improve the behaviour of the bot when user pays the invoice just before thr payment window exipres
Summary by CodeRabbit