fix(order): drop an invoice when its take cycle ends

lib/features/order/models/order_state.dart

@@ -92,12 +92,15 @@ class OrderState {
     bool? fiatWasSent,
     UserInfo? peerReputation,
     bool clearPeerReputation = false,
+    bool clearPaymentRequest = false,
   }) {
     return OrderState(
       status: status ?? this.status,
       action: action ?? this.action,
       order: order ?? this.order,
-      paymentRequest: paymentRequest ?? this.paymentRequest,
+      paymentRequest: clearPaymentRequest
+          ? null
+          : paymentRequest ?? this.paymentRequest,
       cantDo: cantDo ?? this.cantDo,
       dispute: dispute ?? this.dispute,
       peer: peer ?? this.peer,
@@ -109,6 +112,41 @@ class OrderState {
     );
   }
 
+  /// Statuses that close the current take cycle: the order either went back
+  /// to the book or ended for good.
+  ///
+  /// Everything Mostro issued for that cycle — bond and escrow hold invoices
+  /// above all — is cancelled node-side when this happens, so no payload from
+  /// it may survive into the next take of the same order id.
+  ///
+  /// [Status.cooperativelyCanceled] is deliberately absent: here it is the
+  /// *pending* cooperative cancel (every `cooperative-cancel-initiated-*`
+  /// action maps to it) and the trade can still reach fiat-sent. The cancel
+  /// only lands with `cooperative-cancel-accepted`, which maps to
+  /// [Status.canceled].
+  static bool endsTradeCycle(Status status) =>
+      status == Status.pending ||
+      status == Status.canceled ||
+      status == Status.canceledByAdmin ||
+      status == Status.expired;
+
+  /// Actions that can only be the first message of a take cycle.
+  ///
+  /// A message that ends up rejected by the stale-transition guard is either a
+  /// late copy or the start of the *next* take of the same order id; the action
+  /// is half of what tells them apart (the other half is its event time, see
+  /// `AbstractMostroNotifier.applyToCycle`). `add-invoice` also appears in the
+  /// payout-retry flow, but that runs from `payment-failed`, never from a
+  /// status that ends a cycle, so it cannot be confused with a take here.
+  static bool opensTakeCycle(Action action) =>
+      action == Action.takeBuy ||
+      action == Action.takeSell ||
+      action == Action.payBondInvoice ||
+      action == Action.payInvoice ||
+      action == Action.addInvoice ||
+      action == Action.waitingSellerToPay ||
+      action == Action.waitingBuyerInvoice;
+
   /// Dispute statuses in which the dispute is over: a resolution has already
   /// been applied and no further admin action is expected for it.
   static const _terminalDisputeStatuses = {
@@ -210,25 +248,16 @@ class OrderState {
         message.action == Action.fiatSent ||
         message.action == Action.fiatSentOk;
 
-    // Remap generic cooperative cancel actions to semantic variants
-    // based on whether fiat was sent before the cancel was initiated
-    Action effectiveAction = message.action;
-    if (message.action == Action.cooperativeCancelInitiatedByYou) {
-      effectiveAction = newFiatWasSent
-          ? Action.cooperativeCancelFiatSentByYou
-          : Action.cooperativeCancelNoFiatByYou;
-      logger.d('Remapped ${message.action} → $effectiveAction (fiatWasSent: $newFiatWasSent)');
-    } else if (message.action == Action.cooperativeCancelInitiatedByPeer) {
-      effectiveAction = newFiatWasSent
-          ? Action.cooperativeCancelFiatSentByPeer
-          : Action.cooperativeCancelNoFiatByPeer;
-      logger.d('Remapped ${message.action} → $effectiveAction (fiatWasSent: $newFiatWasSent)');
+    // Remap generic cooperative cancel actions to semantic variants, then
+    // derive the status — the same derivation `wouldRejectAsStale` consults.
+    final transition = _transitionFor(message, fiatSent: newFiatWasSent);
+    final Action effectiveAction = transition.action;
+    final Status newStatus = transition.status;
+    if (effectiveAction != message.action) {
+      logger.d(
+          'Remapped ${message.action} → $effectiveAction (fiatWasSent: $newFiatWasSent)');
     }
 
-    // Determine the new status based on the action received
-    Status newStatus = _getStatusFromAction(
-        effectiveAction, message.getPayload<Order>()?.status);
-
     // DEBUG: Log status mapping
     logger.d('Status mapping: $effectiveAction → $newStatus');
 
@@ -256,11 +285,20 @@ class OrderState {
       logger.d('Order returned to pending: dropping the taker reputation');
     }
 
-    // Preserve PaymentRequest correctly
+    // Preserve PaymentRequest correctly — but only within the take cycle it
+    // belongs to. Mostro cancels the bond and escrow hold invoices when the
+    // cycle ends, so carrying one into the next take renders a bolt11 that
+    // can no longer be paid (INCORRECT_PAYMENT_DETAILS).
+    final bool cycleEnded = endsTradeCycle(newStatus);
     PaymentRequest? newPaymentRequest;
     if (message.payload is PaymentRequest) {
       newPaymentRequest = message.getPayload<PaymentRequest>();
       logger.d('New PaymentRequest found in message');
+    } else if (cycleEnded) {
+      newPaymentRequest = null;
+      if (paymentRequest != null) {
+        logger.d('Take cycle ended ($newStatus): dropping the stale invoice');
+      }
     } else {
       newPaymentRequest = paymentRequest; // Preserve existing
     }
@@ -400,6 +438,7 @@ class OrderState {
               ? message.getPayload<PaymentRequest>()!.order
               : order,
       paymentRequest: newPaymentRequest,
+      clearPaymentRequest: newPaymentRequest == null,
       cantDo: message.getPayload<CantDo>() ?? cantDo,
       dispute: updatedDispute,
       peer: newPeer,
@@ -460,6 +499,50 @@ class OrderState {
     return !isRepublish;
   }
 
+  /// The action and status [updateWith] would apply for [message].
+  ///
+  /// The cooperative-cancel remap depends on whether fiat was sent, so it is
+  /// computed from this state unless the caller already knows the updated
+  /// value. Kept in one place so [wouldRejectAsStale] can never disagree with
+  /// what [updateWith] actually does.
+  ({Action action, Status status}) _transitionFor(
+    MostroMessage message, {
+    bool? fiatSent,
+  }) {
+    final sent = fiatSent ??
+        (fiatWasSent ||
+            message.action == Action.fiatSent ||
+            message.action == Action.fiatSentOk);
+
+    var action = message.action;
+    if (action == Action.cooperativeCancelInitiatedByYou) {
+      action = sent
+          ? Action.cooperativeCancelFiatSentByYou
+          : Action.cooperativeCancelNoFiatByYou;
+    } else if (action == Action.cooperativeCancelInitiatedByPeer) {
+      action = sent
+          ? Action.cooperativeCancelFiatSentByPeer
+          : Action.cooperativeCancelNoFiatByPeer;
+    }
+
+    return (
+      action: action,
+      status: _getStatusFromAction(action, message.getPayload<Order>()?.status),
+    );
+  }
+
+  /// Whether [message] would be dropped by the stale-transition guard.
+  ///
+  /// Lets callers replaying persisted history tell a genuinely late copy from
+  /// the first message of a *new* take cycle: after a cancel, every later
+  /// message looks backwards to the guard, because a cancelled order outranks
+  /// every waiting phase. It is only half of the answer — see
+  /// `AbstractMostroNotifier.applyToCycle` for the rest (#731).
+  bool wouldRejectAsStale(MostroMessage message) {
+    final transition = _transitionFor(message);
+    return isStaleTransition(transition.action, transition.status);
+  }
+
   /// Maps actions to their corresponding statuses based on mostrod DM messages
   Status _getStatusFromAction(Action action, Status? payloadStatus) {
     switch (action) {

lib/features/order/notifiers/abstract_mostro_notifier.dart

@@ -41,6 +41,98 @@ class AbstractMostroNotifier extends StateNotifier<OrderState> {
   // uses the same Action.canceled. Consumed once per cancel in subscribe().
   static final Set<String> _userInitiatedCancels = <String>{};
 
+  /// Event time of the message that left this order in a cycle-ending status.
+  ///
+  /// `null` while the current cycle is still running. Maintained by
+  /// [applyToCycle], which is the only place that may restart a cycle.
+  int? cycleEndedAt;
+
+  /// Event time of the message that opened the cycle this order is in.
+  ///
+  /// `null` until a cycle has been seen to open. Everything older than it
+  /// belongs to a cycle that is over — see [precedesActiveCycle].
+  int? cycleStartedAt;
+
+  static int _eventTimeOf(MostroMessage message) =>
+      message.eventCreatedAt ?? message.timestamp ?? 0;
+
+  /// Whether [message] belongs to a take cycle this order has already left.
+  ///
+  /// Such a message would be destructive: the stale guard waves a previous
+  /// cycle's `canceled` through — a cancelled order outranks every waiting
+  /// phase — so applying it would void the invoice the user is looking at,
+  /// delete the session and navigate away. Callers use this to drop the
+  /// message and its side effects, the way
+  /// [OrderState.rejectsAdminDisputeMessage] already does.
+  ///
+  /// This is a defence, not a case observed on the live stream: the orders
+  /// stream is `watchLatestMessage`, which emits only the newest stored
+  /// message by `compareByEventTime`, so an older cycle's `canceled` stored
+  /// after the new bond is never the latest and never reaches it (#732
+  /// review). It still guards `sync()` and any future caller.
+  bool precedesActiveCycle(MostroMessage message) {
+    final startedAt = cycleStartedAt;
+    return startedAt != null && _eventTimeOf(message) < startedAt;
+  }
+
+  /// Applies [message] to [current], restarting the take cycle first when the
+  /// message opens a *new* one rather than being a late copy of the old.
+  ///
+  /// After a cancel every later message looks backwards to the stale guard —
+  /// a cancelled order outranks every waiting phase — so the guard alone can
+  /// no longer tell "the order was taken again" (#731) from "a duplicate
+  /// arrived late" (#723). Restarting therefore needs positive evidence:
+  ///
+  /// 1. the action can only open a cycle ([OrderState.opensTakeCycle]), and
+  /// 2. its event time is *strictly* later than the message that ended the
+  ///    previous cycle. `created_at` has one-second resolution and relays
+  ///    replay newest-first, so a copy from the same second as the cancel is
+  ///    a late copy: Mostro cannot cancel a take and accept the next one
+  ///    within the same second.
+  ///
+  /// Messages from before the current cycle opened are dropped outright
+  /// ([precedesActiveCycle]).
+  OrderState applyToCycle(OrderState current, MostroMessage message) {
+    if (precedesActiveCycle(message)) {
+      logger.w(
+          'Ignoring ${message.action} for order $orderId: it belongs to a take '
+          'cycle that ended at $cycleStartedAt');
+      return current;
+    }
+
+    final eventTime = _eventTimeOf(message);
+    final endedAt = cycleEndedAt;
+    var restarted = false;
+
+    if (endedAt != null &&
+        eventTime > endedAt &&
+        OrderState.endsTradeCycle(current.status) &&
+        OrderState.opensTakeCycle(message.action) &&
+        current.wouldRejectAsStale(message)) {
+      logger.i(
+          'Order $orderId was taken again: replaying ${message.action} as a new cycle');
+      current = OrderState(
+        status: Status.pending,
+        action: Action.newOrder,
+        order: current.order,
+      );
+      restarted = true;
+    }
+
+    final cameFromEndedCycle = OrderState.endsTradeCycle(current.status);
+    final next = current.updateWith(message);
+
+    // Recorded from the resulting status rather than from the transition: a
+    // replay starts from the current state, so the message that ended the
+    // cycle can be applied onto an already-ended one.
+    final ended = OrderState.endsTradeCycle(next.status);
+    cycleEndedAt = ended ? eventTime : null;
+    if (!ended && (restarted || cameFromEndedCycle)) {
+      cycleStartedAt = eventTime;
+    }
+    return next;
+  }
+
   /// Marks an order as cancelled by the user, so the matching `canceled`
   /// response is treated as a voluntary cancel (immediate session deletion)
   /// and notified as user-initiated rather than a counterparty timeout.
@@ -119,6 +211,18 @@ class AbstractMostroNotifier extends StateNotifier<OrderState> {
                 return;
               }
 
+              // Same reasoning for a message the current cycle has
+              // outlived: applying it would void the new bond invoice, and
+              // notifying or navigating on it would send the user out of a
+              // trade that is running (#731). Kept as a defence — the stream
+              // emits only the latest stored message, so it should not be
+              // able to hand one over.
+              if (precedesActiveCycle(msg)) {
+                logger.w(
+                    'Dropping ${msg.action} for order $orderId: it predates the current take cycle');
+                return;
+              }
+
               // Cancel timer on ANY response from Mostro for this order
               cancelSessionTimeoutCleanup(orderId);
 
@@ -134,7 +238,7 @@ class AbstractMostroNotifier extends StateNotifier<OrderState> {
                   _userInitiatedCancels.remove(orderId);
 
               if (mounted) {
-                state = state.updateWith(msg);
+                state = applyToCycle(state, msg);
               }
 
               if (msg.timestamp != null &&

lib/features/order/notifiers/order_notifier.dart

@@ -91,10 +91,12 @@ class OrderNotifier extends AbstractMostroNotifier {
 
       OrderState currentState = state;
 
+      // The replay rebuilds the cycle bookkeeping from the history itself.
+      cycleEndedAt = null;
+      cycleStartedAt = null;
       for (final message in messages) {
-        if (message.action != Action.cantDo) {
-          currentState = currentState.updateWith(message);
-        }
+        if (message.action == Action.cantDo) continue;
+        currentState = applyToCycle(currentState, message);
       }
 
       // A replay that lands on the same values notifies nobody:
@@ -273,9 +275,16 @@ class OrderNotifier extends AbstractMostroNotifier {
   }
 
   /// Update state from MostroMessage (used during restore)
+  ///
+  /// Goes through [applyToCycle] like `sync()` and the live stream do.
+  /// RestoreManager calls this for every restored order, cancelled and expired
+  /// included; writing the state directly left `cycleEndedAt` null, so the
+  /// next take of that order could not restart the cycle and its bond invoice
+  /// was dropped as stale — #731 again, through the one write that skipped the
+  /// rule (#732 review).
   void updateStateFromMessage(MostroMessage message) {
     if (mounted) {
-      state = state.updateWith(message);
+      state = applyToCycle(state, message);
     }
   }