[MEDIUM] admin_cancel lacks the atomic CAS status guard that admin_settle has

[grunch]

Severity: MEDIUM

admin_cancel lacks the atomic compare-and-set status guard that admin_settle has.

Where

src/app/admin_cancel.rs vs src/app/admin_settle.rs.

Problem

admin_settle guards its transition with a conditional update (only acts if the order is still in the expected status), which makes concurrent invocations idempotent. admin_cancel does not have the equivalent guard, so two concurrent admin-cancel requests (or an admin-cancel racing another transition) can both proceed. Combined with the irreversible cancel_hold_invoice side effect (see the "refund before validation" issue), this is worth hardening.

Suggested fix

Mirror admin_settle's CAS pattern in admin_cancel: transition status atomically and only run the LN side effect when the CAS actually updated the row.

Found during the deep review accompanying #803.

[ToRyVand]

I'd like to take this one, and ideally #809 and #855 along with it — they're three instances of the same defect, so the context carries straight across.

Verified all three premises against current main first:

• [MEDIUM] admin_cancel lacks the atomic CAS status guard that admin_settle has #810 — admin_settle.rs:114 guards with if result.rows_affected() == 0; admin_cancel.rs has no rows_affected check at all. Confirmed exactly as written.
• [MEDIUM] Hold invoice settled before the CAS status guard (double-settle race) #809 — in admin_settle.rs, settle_seller_hold_invoice runs at line 95 and the CAS guard at line 114: the irreversible LN side effect fires 19 lines before the guard meant to serialize it.
• hold_invoice_paid guard is read-then-act, not an atomic claim #855 — hold_invoice_paid (src/flow.rs:54) is if !is_pre_payment || order.invoice_held_at != 0 — a read, then an unconditional act.

The tree already contains the pattern all three need, in two places: on_bond_invoice_accepted (src/app/bond/flow.rs:771, claiming with rows_affected() == 1 at :278) and admin_settle's own guard. So this is applying an established in-repo idiom to the three call sites that still read-then-act, not inventing anything. Each one gets a regression test that fails against today's ordering.

One scoping question worth a maintainer call before I start: #809 also names src/app/release.rs, which is the hot path and considerably more delicate than the two admin handlers. I'm happy to harden it too, but if you'd rather leave release alone until the pattern has been reviewed on the admin paths, say so and I'll keep it out.

Could I get assigned to the three? Asking explicitly — I've had a PR duplicate someone else's work recently because a claiming comment with no reply isn't an assignment.

[grunch]

Hi @ToRyVand I assigned this issue to you, let's keep the release.rs change to do it in a new issue/PR after this is tested and merged

[ToRyVand]

Thanks @grunch — one PR per issue, release.rs separate.

#810 and #809 both depend on #825. It rewrites the two files they touch (admin_cancel.rs, admin_settle.rs) and hoists the rejecting checks above the escrow moves — exactly where the CAS goes. Its own comment says so: "Making that tail atomic is #810."

Approved since Jul 29, and I rebased its three commits onto today's main locally — they replay cleanly. Merging it unblocks both issues. I've based #810 on fix/admin-cancel-validate-before-refund meanwhile.

One note: claiming the status before the refund strands the escrow if cancel_hold_invoice fails, since the Dispute guard rejects a retry — so the claim is released on failure. Say if you'd rather it stay loud.

[ToRyVand]

Carrying back what the #963 review established, because it changes what this issue needs — and, importantly, what it does not.

What was withdrawn is a CAS before the refund, not a CAS. This issue asks for the parity admin_settle and release_action already have: a conditional write in place of the full-row order_updated.update(pool), which today carries no status predicate and can overwrite a concurrent transition. That ask is untouched and still worth doing — @Catrya's own review notes cancel_cooperative_execution_step_2 doing the same thing. What came out of #963 is claiming the transition ahead of cancel_hold_invoice.

Why that part came out. The claim lives in SQLite; the effect it guards lives in LND. The two failures are distinguishable — is_already_settled_error in bond/slash.rs matches exactly that message — but distinguishing them gives the release of the claim no correct action: letting it go is wrong when the escrow did move, holding it is wrong when nothing moved, and neither pays the buyer. Which of the two bad end states you land in depends on how LND answers a cancel against a settled invoice, which this tree does not settle; it is recorded as an open question in #972 and is one regtest call to answer.

One ask from that review stands on its own: classify "already settled" distinctly in classify_cancel_error, so an operator can tell it from an unreachable node. That needs no claim and is worth doing either way — happy to fold it in here or file it separately, whichever you prefer.

What a claim-first fix would actually need is durable work that survives a crash. @grunch's note on #855 is about message delivery — enqueue_order_msg pushes onto an in-memory RwLock<Vec<..>>, so a crash after claiming loses the messages and suppresses the replay — and the prescription there is "a durable outbox (or writing the claim and the messages in one transaction)". The same property applied to the Lightning effect is what this would need: an extension of that mechanism rather than something #855 already covers. #809, #810 and #855 all arrive at it.

Still assigned to me. Happy to hold it and come back when the outbox question has an answer, or to hand it back so it sits unassigned — say which you'd rather.

#972 covers the release.rs half and records the same reasoning.