Skip to content

Ex-taker cannot retake an order: it disappears from their order book, and a retake leaves duplicate trade rows #417

Description

@Catrya

A taker whose take ends before the trade goes active (they cancel it, or the daemon times them out) can never take that order again from the same device. Mostrod republishes the order as pending and every other client sees it, but for the ex-taker it disappears from the order book: the local entry stays at the status it had (WaitingBuyerInvoice after a timeout, Canceled after their own cancel), and the book screen only lists pending orders (home_order_providers.dart:257). There is nothing to tap. Even called directly, take_order would refuse it with OrderAlreadyTaken (orders.rs:864).

Fixing only that would make the retake possible but broken: a retake leaves two trade rows for the same order, and every lookup by order id can return the old one. The two problems have to be fixed together.

This also matters for #335. The retake is exactly what #375 (merged) protects: a taker cancel does leave the previous session behind, and once the retake is reachable, install_session is what replaces it. Today that code never runs in a normal flow, because this issue blocks the retake first.

1. The order disappears from the ex-taker's order book

Measured against a live mostrod (v0.18.7) with a scratch probe that drives the real take_order / cancel_order:

  • Taker cancels (cancel_order). The optimistic update marks the row Canceled (orders.rs:1520), so the daemon's Canceled is skipped (skip replayed Canceled: already Canceled), and the republished 38383 pending is refused:

    status order=5de3ace7 wire=Pending local=Canceled applies=false src=38383/d-tag
    

    wire_status_applies (status.rs:116) accepts only terminal wire statuses once a non-Pending row exists. The row stays Canceled and survives restarts, so the ex-taker never sees this order as pending again.

  • Daemon-side cancel of a take that never went active (measured as a taker cancel published without the optimistic update, which is equivalent to a waiting timeout from the client's point of view). Mostrod publishes the 38383 pending before the Canceled. When it arrives the row is still WaitingBuyerInvoice, so it is refused. The Canceled then wipes the row and the session (Canceled before active — removed trade, orders.rs:2276), but no further 38383 arrives, and the book entry stays WaitingBuyerInvoice until restart.
    This contradicts the comment at orders.rs:2335 ("its wiped row leaves the book's pending as the local status"): the book holds whatever take_order wrote into it, not pending.

In both cases the order is pending in the daemon's database, yet it vanishes from the ex-taker's order book, because the local entry is no longer pending and the book screen filters everything else out (home_order_providers.dart:257). This matches what shows up when testing by hand: after losing an order to a timeout, the taker never sees it in the book again, even though it has been republished.

2. A retake leaves duplicate trade rows

Forcing the book back to Pending in the probe (the daemon accepted the retake with trade_key_index=2) leaves the local database with two rows for the same order:

5b5de7df  trade_key_index=1  order.id=5de3ace7
a2fb425f  trade_key_index=2  order.id=5de3ace7

take_order saves each take under a fresh TradeInfo.id (orders.rs:1058, :1118), and save_trade is INSERT OR REPLACE keyed by that id (sqlite.rs:239), so nothing ever replaces the first take's row.

What this breaks. The first bullet was observed; the other three follow from reading the code and were not exercised end to end:

  • get_trade_by_order_id is LIMIT 1 with no ORDER BY (sqlite.rs:541-548). In the probe it returned the first take's row.
  • current_local_status / local_trade_status (orders.rs:2814, :3010) read through it. After a taker cancel that old row is Canceled, which is hard-terminal. So status_sync_blocked_by_terminal (orders.rs:2828) would skip every daemon message of the retaken trade as "already Canceled", including the peer reveal, which carries the same guard. The retaken trade would freeze with no chat.
  • update_trade_fields updates every row with that order id (WHERE json_extract(data, '$.ord- The #381 session rebuild reads the row's trade_key_index`, so after a restart the retaken trade's chat would be rebuilt from the first take's key. That is Session: a retake keeps the stale session, and messages from a superseded take act on the current one #335's symptom again, this time from the durable copy.

Why together

Fixing 1 alone makes the retake reachable and walks straight into 2. Fixing 2 alone changes nothing a user can see, because the retake stays blocked.
er.id') = ?, sqlite.rs:587`).

Suggested direction (not prescriptive)

  • Taker cancel of a never-active trade: wipe the row and remove the session, as the daemon Canceled arm already does for pending / waiting-* (cancellation_wipes_history), instead of marking the row Canceled. The republished pending then applies, and there is no stale session or duplicate row left to trip over.
  • Timeout path: after the Canceled arm wipes the row, the book entry needs to return to pending, since the 38383 that said so arrived first and was refused.
  • Defensively: a confirmed take could replace any existing row for the same order id, so a retake never leaves two rows even on paths not covered above.

Acceptance criteria

  • After a taker cancel of a never-active trade, the order reappears as pending in the ex-taker's order book and can be taken again from the same session.
  • Same after a waiting timeout.
  • After a retake there is exactly one trade row for the order, carrying the new trade_key_index.
  • The retaken trade receives its daemon messages (status progresses, peer reveal lands, chat works), and its session carries the new index (the fix(#335): replace-not-discard the session on a confirmed retake #375 path, now reachable by hand).
  • Tests pin both halves, going through the real functions rather than copies of the logic.

Docs to correct in the same PR

Left over from #375, whose text describes a trigger that does not create a stale session. A take the daemon rejects, or one that hits the 10 s client timeout, returns from take_order before any session is installed. What leaves a stale session is an earlier take that was confirmed and never cleaned up, which is exactly the taker-cancel path above.

  • contracts/orders.md:124: "a prior failed or timed-out attempt left a stale one" → describe the real source, and add that a rejected or timed-out take installs nothing.
  • contracts/orders.md:495: the binding is "written by take_order on every attempt""on every confirmed take" (it is written at orders.rs:1100, after the reply is accepted).
  • session.rs:167 (install_session docstring), the comments at orders.rs:1129-1135, and the test comment at orders.rs:7594: same false trigger.
  • The docstring of retake_replaces_stale_session_trade_key_index claims the take_order → install_session seam "is covered by the manual regtest run". That run never retook an order. Reword it, or better, replace it with the new retake test.
  • orders.rs:2335: the "wiped row leaves the book's pending" comment, which should be true once this is fixed.
  • orders.rs:1148: log::warn!blog_warn. log:: records are discarded in the app because install_log_bridge never runs.
  • Update the contract for the new cancel / retake behaviour itself.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingpriority: highHigh priority

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions