fix: validate the dispute initiator before refunding the escrow (#805)

[grunch]

Closes #805.

The bug

admin_cancel_action refunded the seller's escrow before it finished validating the request:

if order.hash.is_some() {
    ln_client.cancel_hold_invoice(hash).await?;   // irreversible
}
...
let dispute_initiator = match (order.seller_dispute, order.buyer_dispute) {
    (true, false) => "seller",
    (false, true) => "buyer",
    (_, _) => return Err(MostroInternalErr(ServiceError::DisputeEventError)),  // rejects — too late
};

When neither initiator flag is set (or both are), the handler returns DisputeEventError — but cancel_hold_invoice has already returned the funds to the seller. The order stays in Dispute, the transition to CanceledByAdmin never runs, and the dispute row is never moved to SellerRefunded. The Lightning side and the DB disagree permanently, and the refund cannot be undone.

The fix

Move the initiator resolution above the refund, so every check that can reject the call runs before the escrow is touched. This is the same ordering discipline already applied to the status guards and the bond-resolution validation just above it (see the existing Phase 2 comment). The valid path — a legitimately flagged initiator — is unchanged.

Test

New regression test dispute_without_initiator_flag_errors_before_refunding: an order with a hold-invoice hash and no initiator flag must fail with DisputeEventError and be left in Dispute.

The test is a genuine before/after discriminator thanks to the existing dead_lnd() harness (a real LndConnector against a dead endpoint, so any RPC fails fast):

• Before the fix it failed with Err(MostroInternalErr(LnNodeError("code=Unknown message=client error (Connect)"))) — proof the refund RPC had already been dispatched.
• After the fix it returns DisputeEventError without ever reaching LND.

dispute_with_hash_reaches_ln_cancel_seam still passes, confirming the legitimate refund path is untouched.

Verification

• cargo test — 1016 passed, 0 failed
• cargo clippy --all-targets -- -D warnings — clean
• cargo fmt --all — clean

Note

admin_settle.rs has related ordering/CAS concerns tracked separately in #809 and #810; this PR deliberately stays scoped to #805.

[coderabbitai]

Warning

Review limit reached

@grunch, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: afd6faf9-113c-4c5b-97b2-48e59eb8a703

📥 Commits

Reviewing files that changed from the base of the PR and between a086c22 and ef12edd.

📒 Files selected for processing (1)

• src/app/admin_cancel.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/admin-cancel-validate-before-refund

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[grunch]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[ermeme]

Strict review completed on the current head.

The change fixes the funds-safety ordering bug without altering the valid cancellation path: malformed/ambiguous initiator flags are now rejected before the irreversible hold-invoice cancellation, while the existing valid-hash test still proves that a legitimate request reaches the LND cancellation seam. The new regression test is a real before/after discriminator because the old ordering returns LnNodeError against the dead-LND harness before it can produce DisputeEventError.

I also checked authorization, dispute-status and bond-resolution ordering, the seller/buyer initiator mapping used in the kind-38386 dispute event, retry behavior in the malformed-state path, and the tracked CAS/atomicity concerns outside this PR's scope. No blocking regression found.

CI is green for build, tests, fmt, clippy, and the Rust 1.94.0 MSRV build.

[ermeme]

Non-blocking: Please scope this statement to the initiator validation. The handler still has fallible operations after the refund (new_dispute_event, update_order_event, DB updates, pubkey parsing, and DM delivery), so “Every check that can reject the call” is broader than the invariant this patch establishes and may imply atomicity that is still tracked separately in #810. “This initiator check now precedes the refund” would be precise.