From 3025a2796d971912a71c5e3e902f4d3dc8c34495 Mon Sep 17 00:00:00 2001
From: Tori <toryvand@gmail.com>
Date: Mon, 24 Aug 2026 10:19:03 -0500
Subject: [PATCH] fix(dispute): create the dispute row before flagging the
 order (#921)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`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.
---
 src/app/dispute.rs | 61 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/src/app/dispute.rs b/src/app/dispute.rs
index 51613fec..eab00fea 100644
--- a/src/app/dispute.rs
+++ b/src/app/dispute.rs
@@ -168,22 +168,28 @@ pub async fn dispute_action(
     // Create new dispute record
     let dispute = Dispute::new(order_id, order.status.clone());
 
-    // Setup dispute
+    // Setup dispute. In-memory only, so a rejection here writes nothing.
     order
         .setup_dispute(is_buyer_dispute)
         .map_err(MostroCantDo)?;
-    order
-        .clone()
-        .update(pool)
-        .await
-        .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?;
 
-    // Save dispute to database
+    // Row first, then the status flip — the ordering `job_escrow_deadline`
+    // already documents and relies on: a dispute row on a still-`active` or
+    // `fiat-sent` order is recoverable (its pass finishes the transition),
+    // whereas a `dispute` order with no row is invisible to solvers and,
+    // because `get_valid_order` admits only `Active`/`FiatSent`, can no
+    // longer be disputed by *either* party.
     let dispute = dispute
         .create(pool)
         .await
         .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?;
 
+    order
+        .clone()
+        .update(pool)
+        .await
+        .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?;
+
     // Get pubkeys of initiator and counterpart
     let (initiator_pubkey, counterpart_pubkey) = if is_buyer_dispute {
         (
@@ -733,4 +739,45 @@ mod tests {
         let dispute = find_dispute_by_order_id(&pool, order.id).await.unwrap();
         assert_eq!(dispute.status, DisputeStatus::Settled.to_string());
     }
+
+    /// The `disputes` insert is made to fail deterministically by dropping the
+    /// table, so this pins the write ordering rather than any timing: the order
+    /// must not carry the dispute flag (nor the `Dispute` status) unless the
+    /// row that makes the dispute visible was written first.
+    #[tokio::test]
+    async fn dispute_action_leaves_the_order_untouched_when_the_dispute_row_fails() {
+        let pool = create_test_pool().await;
+        let ctx = build_ctx(&pool);
+        let buyer = Keys::generate().public_key();
+        let seller = Keys::generate().public_key();
+
+        let order = create_order(Some(buyer), Some(seller), Status::Active)
+            .create(&pool)
+            .await
+            .unwrap();
+
+        sqlx::query("DROP TABLE disputes")
+            .execute(&pool)
+            .await
+            .unwrap();
+
+        let event = create_event(buyer);
+        let result = dispute_action(
+            &ctx,
+            dispute_msg_for(Some(order.id)),
+            &event,
+            &Keys::generate(),
+        )
+        .await;
+
+        assert!(matches!(
+            result,
+            Err(MostroInternalErr(ServiceError::DbAccessError(_)))
+        ));
+
+        let stored_order = Order::by_id(&pool, order.id).await.unwrap().unwrap();
+        assert!(!stored_order.buyer_dispute);
+        assert!(!stored_order.seller_dispute);
+        assert_eq!(stored_order.status, Status::Active.to_string());
+    }
 }