fix: serialize order release under per-order mutex with state recheck - #836
Conversation
Run the hold-invoice settle in the release flow under the same per-order mutex used by payHoldInvoice and the cancel/expiry jobs, and re-read the order under the lock before settling. The order is only settled when it is still in a releasable state (ACTIVE, FIAT_SENT or DISPUTE). Order validation runs before the lock is acquired, so the order could be canceled, settled or otherwise advanced in between. Rechecking the state under the lock closes the release-vs-cancel race over the same hold invoice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 10 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe release command's settlement flow is refactored to serialize execution under a per-order mutex. The command now re-fetches and validates the current order state inside the lock before settling the hold invoice, ensuring atomic verification that the order remains releasable and that dispute updates occur safely under concurrency control. ChangesRelease Settlement Concurrency Control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@bot/commands.ts`:
- Around line 917-924: The code updates and saves dispute.status = 'RELEASED'
(Dispute.findOne result) before calling settleHoldInvoice, risking a stale
released state if settle fails; move the mutation and await dispute.save() to
after the successful await settleHoldInvoice({ secret: currentOrder.secret }) so
the database is only updated once the Lightning settle completes, or
alternatively add a compensating retry/write on settle failure to revert/repair
the dispute state; locate the dispute handling in this function (the dispute
variable from Dispute.findOne) and ensure bot/modules/dispute/actions.ts logic
(lines cited) will only observe the released state after a confirmed settle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Move the dispute RELEASED status update to after settleHoldInvoice succeeds. Previously the dispute was saved as RELEASED before the Lightning settle, so a failed settle left a stale RELEASED dispute that makes solvers believe the seller already released (bot/modules/dispute/actions.ts short-circuits on that status). 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>
|
@codex review |
2 participants
Summary
Hardens the order release flow against a race condition that could allow a hold invoice to be settled after the order had already moved on (e.g. been canceled), by serializing the settle under the per-order mutex and re-checking the order state under the lock.
Previously,
validateReleaseOrderran without holding any lock, and the settle proceeded based on that earlier read. Between validation and the actual settle, the order could be canceled, settled or otherwise advanced by a concurrent action or background job, leaving room for a release-vs-cancel race over the same hold invoice.Changes
bot/commands.ts— Wrap the settle inPerOrderIdMutex.instance.runExclusive(orderId, ...), the same per-order lock used bypayHoldInvoiceand the cancel/expiry jobs. Inside the lock, re-read the order and only settle when it is still in a releasable state (ACTIVE,FIAT_SENT,DISPUTE); otherwise log and skip. The dispute status update and thesettleHoldInvoicecall now happen against the order re-read under the lock.Testing
npx tsc --noEmitpasses.Summary by CodeRabbit