Skip to content

fix: detect waiting-state timeouts and clean up local state - #274

Merged
Catrya merged 9 commits into
mainfrom
fix/timeout-session-cleanup
Aug 5, 2026
Merged

fix: detect waiting-state timeouts and clean up local state#274
Catrya merged 9 commits into
mainfrom
fix/timeout-session-cleanup

Conversation

@Catrya

@Catrya Catrya commented Aug 5, 2026

Copy link
Copy Markdown
Member

fix #148
When a trade in waiting-buyer-invoice / waiting-payment timed out on the
daemon, the client mishandled it on every front:

  • The taker's trade stayed in My Trades forever (as Canceled, or Pending via
    the live chip) — no session/state cleanup existed at all.
  • The republished order intermittently vanished from the order book until an
    app restart: mostrod publishes the pending 38383 BEFORE sending
    Canceled, and the handler's blind remove_order raced it.
  • The add-invoice screen had no cancellation listener: the form stayed up and
    every submit died with a 10s NoDaemonResponse (mostrod ignores messages
    for canceled orders). Pay-invoice only reacted to polled terminal statuses,
    which a timeout republish never produces.
  • Nothing reconciled timeouts that fired while the app was closed.

What changed

  • DB: new delete_trade_by_order_id (SQLite + IndexedDB no-op).
  • Canceled handler: a trade still in pending/waiting (never active — no
    peer, no chat) is wiped together with its in-memory session, mirroring v1's
    session cleanup; trades that progressed keep their row and chat as history.
    No more blind book removal — the 38383 feed stays authoritative.
  • New on_trade_updated push stream: polling cannot observe a wipe (no
    row left) or a republish (book reads pending); the invoice screens and
    the My Trades list now react to the push (notice + navigation + refresh).
  • Stale-state sweep: 60s after subscribing and every 30 min, waiting
    trades past their window are checked against the public book — pending
    republish wipes taker rows / resyncs maker rows to Pending; outright
    cancel wipes; no positive daemon signal → no action. Also drops keyless
    in-memory sessions older than 24h. Counters go to the bridge log.
  • Specs: contracts/orders.md + data-model.md updated (cancellation
    semantics, stream, sweep).

Manual testing

  1. Taker timeout: take an order as buyer, don't submit the invoice, stay
    on My Trades. On timeout: log line Canceled before active — removed trade for order=…, the trade disappears from My Trades, and the order reappears
    in the order book immediately (no restart).
  2. Screen reaction: same scenario but waiting on the add-invoice (or
    pay-invoice) screen → "order no longer active" notice + navigation home.
  3. Maker side: your order gets taken, taker times out → order returns to
    Pending in book and My Trades (chip AND filter bucket after refresh).
  4. App-closed timeout: take an order, kill the app, let it time out,
    reopen and wait ~60s → stale sweep: examined=1 wiped=1 … in the log and
    the trade is gone.

Out of scope (issues to be opened separately)

Back-navigation showing "Take this order" again; My Trades filter using
stale status; countdown driven by real expiration_seconds; polling→push
migration for status chips; configurable history retention.

Summary by CodeRabbit

  • New Features

    • Added real-time trade status updates for order screens.
    • Canceled or expired orders now automatically return to the home screen with a localized notification.
    • Trade lists refresh automatically when lifecycle changes occur.
    • Added cleanup and synchronization for stale, canceled, and republished orders.
  • Bug Fixes

    • Improved removal of inactive trades and expired sessions.
    • Preserved progressed trades as canceled history while cleaning up pending trades.
  • Documentation

    • Documented trade update events, cancellation handling, stale-state reconciliation, and timeout tracking.

Catrya added 8 commits August 4, 2026 20:26
- Adds the trait method with SQLite and IndexedDB implementations.
- SQLite deletes via the nested JSON order id, mirroring get_trade_by_order_id,
  because trades.id is a fresh UUID for takers.
- IndexedDB is a no-op since trades are not persisted on web yet (#233).
- Chat messages are deliberately untouched; groundwork for the timeout cleanup.
…e trade

- On Canceled, a trade still in pending/waiting states is deleted together
  with its in-memory session instead of lingering as a Canceled history row.
- Mirrors v1, which deletes pending/waiting sessions on cancel; trades that
  progressed keep their row and chat as history.
- InProgress rows are kept: it only enters via the Kind 38383 sync, where it
  masks both waiting and active phases, so it is ambiguous.
- mostrod republishes the order as pending BEFORE sending Canceled on a
  taker-responsible timeout, so the blind remove raced the republish and
  left the order missing from the book until an app restart.
- The book is fed only by Kind 38383 events: a genuine cancel arrives as a
  status update and the UI already filters non-pending orders.
…stream

- New broadcast channel + TradeUpdatesStream mirroring the bond-slashed
  pattern; the Canceled arm emits for both the wipe and mark-canceled paths.
- Needed because polling cannot observe a cancellation anymore: the wiped
  trade has no DB row left, and a timeout republish reads pending again.
- Regenerates frb bindings.
- add-invoice had no status listener at all: after a waiting-state timeout
  the form stayed up and every submit died with a 10s NoDaemonResponse.
- pay-invoice only watched polled terminal statuses, which a wipe or a
  timeout republish never produces; both screens now also listen to the
  on_trade_updated push, show a notice, refresh My Trades and leave.
- Runs 60s after subscribing and every 30 min (v1's cadence): waiting trades
  past their window are checked against the public book — pending republish
  wipes taker rows and resyncs maker rows, an outright cancel wipes; absence
  or ambiguous statuses change nothing, the daemon stays the authority.
- Also drops keyless in-memory sessions older than 24h (first real caller of
  cleanup_stale_sessions) and logs sweep counters.
- Documents the on_trade_updated stream, the differentiated Canceled
  handling (wipe never-active trades vs keep history), the no-book-removal
  rule and its race rationale, and the stale-state sweep.
- Notes timeout_at's real semantics and the trade-row deletion exception
  in the data model.
…the update

- rawTradesProvider now invalidates itself on every on_trade_updated push,
  so a daemon cancel wipe (or a sweep resync) refreshes the list from any
  screen instead of only via pull-to-refresh or the invoice screens.
- The wipe log moves to the bridge logger so it is visible while testing.
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a744f7a-32aa-44ac-8d90-b99b266d8725

📥 Commits

Reviewing files that changed from the base of the PR and between 26af62c and e1d71fc.

📒 Files selected for processing (2)
  • rust/src/api/orders.rs
  • specs/004-mostro-p2p-client/data-model.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • specs/004-mostro-p2p-client/data-model.md
  • rust/src/api/orders.rs

Walkthrough

Changes

The PR adds Rust trade cancellation and stale-state reconciliation, storage cleanup by order ID, a Flutter Rust Bridge update stream, and Flutter listeners that refresh trades and navigate inactive invoice screens home.

Trade lifecycle synchronization

Layer / File(s) Summary
Trade contracts and persistence
rust/src/api/types.rs, rust/src/db/..., rust/src/mostro/session.rs, specs/004-mostro-p2p-client/...
Defines TradeUpdate, Trade.timeout_at, storage deletion by order ID, session cleanup counts, and lifecycle contracts.
Cancellation and stale-state processing
rust/src/api/orders.rs
Classifies canceled trades, deletes inactive records and sessions, resynchronizes republished orders, sweeps stale state, and emits lifecycle updates. Tests cover these decisions and stream delivery.
Rust stream bridge
rust/src/frb_generated.rs
Adds generated encoding, decoding, dispatch, Dart conversion, stream ownership, and native or WASM reference management for TradeUpdatesStream and TradeUpdate.
Flutter update reactions
lib/features/order/providers/trade_state_provider.dart, lib/features/trades/providers/trades_providers.dart, lib/features/order/screens/...
Consumes trade updates, invalidates cached trades, and handles matching cancellation-like statuses with guarded navigation and localized feedback.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Daemon
  participant orders_api
  participant TradeUpdatesStream
  participant tradeUpdatesProvider
  participant InvoiceScreen
  Daemon->>orders_api: send cancellation or stale-order status
  orders_api->>TradeUpdatesStream: publish TradeUpdate
  tradeUpdatesProvider->>TradeUpdatesStream: read next update
  TradeUpdatesStream-->>tradeUpdatesProvider: return TradeUpdate
  tradeUpdatesProvider-->>InvoiceScreen: deliver matching update
  InvoiceScreen->>InvoiceScreen: refresh trades and navigate home
Loading

Possibly related issues

Possibly related PRs

  • MostroP2P/app#255 — Both modify per-trade/order event subscriptions in rust/src/api/orders.rs.
  • MostroP2P/app#258 — Both modify cancellation handling and session cleanup.
  • MostroP2P/app#271 — Both modify trade-status synchronization in rust/src/api/orders.rs.

Suggested reviewers: ermeme

Poem

A rabbit watched the trade stream glow,
As canceled orders ceased to flow.
Stale sessions hopped away,
Fresh statuses marked the day,
And invoice screens went home. 🐇

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements timeout reconciliation, session cleanup, and logging, but does not implement the required visible countdown behavior. Add the required countdown timer while users decide whether to take an order, and verify dispute sessions are not deleted early.
Docstring Coverage ⚠️ Warning Docstring coverage is 32.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary timeout detection and local cleanup changes.
Out of Scope Changes check ✅ Passed The trade update stream, provider refreshes, storage changes, and generated bindings support timeout cleanup and state synchronization.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/timeout-session-cleanup

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/src/api/orders.rs`:
- Around line 3224-3226: Update TradeUpdatesStream::next to handle
RecvError::Lagged(_) as an explicit stable status/code exposed through the Dart
API instead of skipping it and continuing; preserve normal update handling, and
add coverage that sends more than TRADE_UPDATES_CAPACITY updates to a slow
subscriber and verifies the wiped invoice’s Canceled update is surfaced as lag.

In `@specs/004-mostro-p2p-client/data-model.md`:
- Around line 114-117: Revise the trade-row persistence statement in the data
model to cover only daemon cancellation behavior: retain deletion for trades
canceled while pending or waiting and never active, without asserting that
normal updates are limited to status. Ensure the wording remains consistent with
the allowed hold_invoice and amount_sats updates described elsewhere.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 50c81419-9e8f-4898-9caa-98d417b0e8f6

📥 Commits

Reviewing files that changed from the base of the PR and between a149b8f and 26af62c.

📒 Files selected for processing (13)
  • lib/features/order/providers/trade_state_provider.dart
  • lib/features/order/screens/add_lightning_invoice_screen.dart
  • lib/features/order/screens/pay_lightning_invoice_screen.dart
  • lib/features/trades/providers/trades_providers.dart
  • rust/src/api/orders.rs
  • rust/src/api/types.rs
  • rust/src/db/indexeddb.rs
  • rust/src/db/mod.rs
  • rust/src/db/sqlite.rs
  • rust/src/frb_generated.rs
  • rust/src/mostro/session.rs
  • specs/004-mostro-p2p-client/contracts/orders.md
  • specs/004-mostro-p2p-client/data-model.md

Comment thread rust/src/api/orders.rs
Comment thread specs/004-mostro-p2p-client/data-model.md Outdated
- Log dropped trade-updates on broadcast lag instead of skipping silently;
  the Dart-facing lag API was declined as disproportionate (see PR thread).
- Fix the data-model persistence wording: trade rows mutate in place
  (status, hold_invoice, amount_sats), they are not status-only.

@ermeme ermeme Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — reviewed at head e1d71fc2.

Verification

  • CI green on this head: Rust (build/test/clippy/wasm), Flutter (analyze/test), Web (wasm build).
  • Local: full cargo test --lib on e1d71fc2 — 239 passed, 0 failed, including the four new tests (cancellation_wipes_history_only_for_never_active_trades, sweep_action_requires_a_positive_book_signal, trade_updates_reach_subscribers, delete_trade_by_order_id_removes_only_the_matching_row).
  • git diff --check clean.

Design review

  • Removing the blind remove_order from the Canceled handler is correct: the 38383 feed stays the single authority on the book, eliminating the republish race. The two remaining remove_order call sites are legitimate (user-initiated optimistic cancel, order-ID reconciliation).
  • cancellation_wipes_history is appropriately conservative — keeping InProgress rows given mostrod's ambiguous in-progress masking is the right call, and the fallback on DB lookup error (keep + mark Canceled) fails safe.
  • The sweep only acts on positive book signals, so a missing/incomplete book fetch can never wipe a live trade; the timeout_at/started_at + 900 age gate matches the daemon's waiting window and the spec.
  • Wipe + remove_session + emit_trade_update are idempotent, so a live Canceled racing the 30-min sweep is harmless; Dart screens have one-shot _navigated guards against duplicate emissions.
  • delete_trade_by_order_id correctly matches get_trade_by_order_id's nested-JSON filter (taker rows have a fresh UUID as trades.id), with test coverage proving only the matching row is removed.
  • Both CodeRabbit threads are addressed on this head (Lagged now logged-and-skipped with a sound degradation argument; data-model wording corrected).

Non-blocking nits

  • SWEEP_ACTIVE never resets: if the sweep task ever panics, no sweep runs again until app restart. Fine to leave as-is.
  • Worth confirming the claim about mostrod's publish order (pending 38383 before Canceled) against the current mostrod release — the whole race fix rests on it, and it matches my reading of the daemon's scheduler.

Nice work — the push stream closes a real gap that polling structurally could not observe.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatic timeout detection + session cleanup

1 participant