fix: address grunch review on handle-delayed-payment PR#845
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
@Matobi98 please the CI is failing |
Luquitasjeffrey
left a comment
Luquitasjeffrey
left a comment
There was a problem hiding this comment.
Please handle my inline comments
| i18nCtx: I18nContext, | ||
| pending?: IPendingPayment, | ||
| ): Promise<boolean> => { | ||
| const won = await Order.findOneAndUpdate( |
There was a problem hiding this comment.
Why dont you update the IOrder passed as a parameter to the function and later call await order.save()?
There was a problem hiding this comment.
Why findOneAndUpdate instead of order.save():
Because if you use order.save() you get a race condition, let me explain:
With order.save() the flow is:
- Load the order into memory → status = "PENDING"
- Set status = "SUCCESS"
- order.save()
The problem shows up when steps 1–2–3 run in parallel. An order can be completed from two places at once: the pending-payments job and the buyer's /setinvoice.
Job: reads order (PENDING) ... sets SUCCESS ... save → notifies + bumps trades
/setinvoice: reads order (PENDING) ... sets SUCCESS ... save → notifies + bumps trades
Both read "PENDING" before the other one saved, so both think it's their job to complete it. Result: the buyer gets notified twice and trades_completed is incremented twice. That's the race condition.
order.save() can't prevent this because it's two separate operations — read first, then write — and the other process slips in between.
With findOneAndUpdate it's a single atomic operation:
Order.findOneAndUpdate(
{ _id: order._id, status: { $ne: 'SUCCESS' } }, // "only if it's NOT already SUCCESS"
{ $set: { status: 'SUCCESS' } }
)
This tells the database: "switch to SUCCESS only if it isn't already, and tell me whether you actually changed it". The DB guarantees that of the two processes, only one gets the order back (won); the other gets null. So only the winner notifies and bumps trades. That's idempotency: the routine runs exactly once, no matter how many callers trigger it.
| // so that if two callers race (e.g. this job and a concurrent /setinvoice), only | ||
| // the first one runs the side effects: no double notification, no double trades. | ||
| // Returns true if this caller won the race and ran the routine. | ||
| export const completeOrderAsSuccess = async ( |
There was a problem hiding this comment.
I think this function shoumd be located in utils/
Luquitasjeffrey
merged commit cf3ccd2
into
lnp2pBot:handle_delayed_payment
3 participants
No description provided.