fix(dispute): create the dispute row before flagging the order (#921) - #923
fix(dispute): create the dispute row before flagging the order (#921)#923ToRyVand wants to merge 1 commit into
Conversation
…oP2P#921) `dispute_action` persisted the order's dispute flag and its `Dispute` status before inserting the `disputes` row, in two statements with no transaction. A `DbAccessError` on the insert returned to the client and left the order flagged, in status `Dispute`, with no row. That state is unrecoverable. `get_valid_order` admits only `Active` and `FiatSent`, and it runs before the sender is identified, so every retry fails with `CantDo(NotAllowedByStatus)` — for the counterparty as much as for the initiator. With no row, solvers cannot see the dispute either, so the order is stuck with its escrow. `job_escrow_deadline` already does these two writes in the opposite order and documents why: Row first, then the status flip: a dispute row pointing at a `fiat-sent` order is recoverable (the next tick sees it and stops), a `dispute` order with no row would be invisible. Its recovery pass finds a half-completed transition *by the row*, and its test notes the half-write "can only mean a previous pass (or a user dispute) died between the two writes". Reordering here puts `dispute_action`'s failure mode back inside that existing recovery path. Only the `update` moves: `setup_dispute` is in-memory validation, so a rejection there still writes nothing, and `Dispute::new` keeps capturing the pre-dispute status for `order_previous_status`. A transaction would be stronger, but `Crud::create`/`Crud::update` take `&Pool<Sqlite>` rather than a generic executor, so it would require a mostro-core change or raw sqlx at both call sites. The reorder needs neither. The test drops the `disputes` table to make the insert fail deterministically, so it pins the ordering with no timing involved: it fails on the previous order at `!stored_order.buyer_dispute` and passes on this one.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesDispute persistence
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change creates the dispute record before updating the order’s dispute status, keeping failed transitions recoverable; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking change required
The reordered writes fix the original insert-failure direction, but the inverse failure window is not recoverable for an Active order. If the dispute insert succeeds and the subsequent order update fails, the durable initiated dispute has no notifications or event, while the order remains active. Retries are rejected as DisputeAlreadyExists; the deadline worker only resumes this half-transition for fiat-sent and cancels active escrow instead.
Please make the two writes atomic, or recover an existing live dispute before the Active deadline-cancellation path. Add failure-injection coverage for "create succeeds, update fails" from both Active and FiatSent.
Local verification on this head: cargo fmt --all -- --check, the new focused regression test, the existing FiatSent recovery test, and cargo clippy --all-targets --all-features -- -D warnings all passed.
| .await | ||
| .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?; | ||
|
|
||
| order |
There was a problem hiding this comment.
A successful Dispute::create followed by a failed order.update now leaves an initiated row plus an active order. That state is not safely recoverable: dispute_action permits Active, retries hit DisputeAlreadyExists, and the deadline worker resumes existing rows only in its FiatSent branch; its Active branch cancels the escrow. Make the writes atomic or add recovery before the active cancellation path, with failure-injection tests for both Active and FiatSent.
1 participant
Closes #921. Taken with @21Mill's go-ahead (comment).
The problem
dispute_actionpersisted the order's dispute flag and itsDisputestatus before inserting thedisputesrow — two statements, no transaction:A
DbAccessErroron the insert returns to the client and leaves the order flagged, in statusDispute, with no row. Nothing reconciles it.That state is unrecoverable, and worse than the issue originally described (corrected here):
get_valid_orderadmits onlyActive/FiatSent, and it runs before the sender is identified, so every retry fails withCantDo(NotAllowedByStatus)— for the counterparty as much as for the initiator. Not a one-sided lockout.disputesrow, solvers cannot see it either.So the order sits in
Dispute, invisible, undisputable by anyone, with its escrow held.The fix
Reorder: row first, then the status flip. Only the
updatemoves.setup_disputeis in-memory validation, so a rejection there still writes nothing, andDispute::newkeeps capturing the pre-dispute status fororder_previous_status.This is not a new invariant —
job_escrow_deadlinealready does these two writes in this order and documents why (src/scheduler.rs:988):Its recovery pass finds a half-completed transition by the row, and its own test notes that such a row "can only mean a previous pass (or a user dispute) died between the two writes". The reorder puts
dispute_action's failure mode back inside a recovery path that already exists and already anticipates it.Why not a transaction
Stronger, and
pool.begin()does exist in the tree (src/app/rate_user.rs,src/db.rs) — butCrud::create/Crud::update(mostro-core 0.14.5) take&Pool<Sqlite>, not a generic executor. A tx therefore means either changing mostro-core (separate crate, separate release) or replacing both calls with raw sqlx. The reorder needs neither and yields the recoverable ordering. Happy to revisit if you'd rather carry it in mostro-core.Test
dispute_action_leaves_the_order_untouched_when_the_dispute_row_failsdrops thedisputestable so the insert fails deterministically — the ordering is pinned with no timing involved.Verified it actually detects the bug rather than merely running: on the previous write order it fails at
assertion failed: !stored_order.buyer_dispute; on this one it passes.Verification
cargo test— 1234 passed, 0 failed, 3 ignoredcargo clippy --all-targets --all-features— cleancargo fmt --check— cleanDiff is one file, +54/−7.
Summary by CodeRabbit