feat(disputes): make the dispute chat survive a restart - #256
Conversation
The dispute record is in-memory by design — status and resolution come back from daemon events. The solver pubkey does not: it arrives exactly once, in `admin-took-dispute`, and without it the chat keys cannot be derived again. So a restart mid-dispute left the party unable to reach the solver at all, and `resubscribe_active_dispute_chats` (added in the #254 review round) found an empty store to re-arm from. `handle_admin_took_dispute` now writes the pubkey to the settings KV under `dispute_admin:<order_id>`, on both the existing-record and the peer-opened-dispute paths. The key builder already existed but nothing read or wrote it. Rehydration folds into the function that already runs on reconnect rather than adding a second entry point: before re-arming, records are rebuilt for persisted trades that have a stored solver. Trades are the enumeration source, so no key-prefix scan is needed. Records already in memory win — they are at least as fresh as storage. Restored status is InReview, which is what a stored solver means; a later resolution arrives as a daemon event. That also restores `submit_evidence`, which refuses without a record. Persistence is best-effort: a storage failure logs and leaves the live listener working for the session rather than undoing an applied dispute update. Tests: the pubkey survives closing and reopening the store (the restart this exists for), and each order gets its own key. 209 Rust tests, clippy at the baseline 24, wasm32 check passes.
WalkthroughDispute handling now coordinates atomic local opens and admin assignments, persists solver pubkeys by order, and rehydrates active ChangesDispute lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client as open_dispute
participant Store as DisputeStore
participant Admin as handle_admin_took_dispute
participant Storage
participant Reconnect as resubscribe_active_dispute_chats
Client->>Store: Mark pending and insert local dispute
Admin->>Store: Atomically create or update assignment
Store-->>Admin: Preserve, claim, or replace record
Admin->>Storage: Persist solver pubkey by order
Reconnect->>Storage: Read trades and persisted solver keys
Storage-->>Reconnect: Return statuses and solver pubkeys
Reconnect->>Store: Restore missing InReview disputes
Store->>Storage: Clear keys for finished trades
Reconnect->>Store: Restart dispute listeners
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 703370b9b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dispute_store() | ||
| .upsert(Dispute { | ||
| id: uuid::Uuid::new_v4().to_string(), | ||
| trade_id: order_id.clone(), | ||
| status: DisputeStatus::InReview, |
There was a problem hiding this comment.
When a stored trade has already reached SettledByAdmin or CanceledByAdmin, list_trades() still returns it and the solver setting is never removed, so this unconditionally resurrects the dispute as InReview on every restart. The daemon dispatcher in orders.rs routes those terminal actions only through trade-status synchronization and never calls the dispute resolution handlers, leaving the restored dispute active indefinitely, starting an unnecessary listener and allowing further evidence submission. Filter terminal trade statuses here or clear the persisted solver key when resolving.
Useful? React with 👍 / 👎.
| let Some(db) = crate::db::app_db::db() else { | ||
| log::warn!("[disputes] no store — solver pubkey will not survive a restart"); | ||
| return; |
There was a problem hiding this comment.
On Flutter web, lib/main.dart explicitly skips initDb, so app_db::db() is always None and every new persistence attempt returns here without writing. Even if the IndexedDB settings store were initialized, its list_trades() currently always returns an empty list, leaving rehydration with no enumeration source. Consequently, browser reloads still lose the solver pubkey and dispute chat despite this feature; web needs an initialized store plus an enumeration strategy that does not depend on unimplemented trade persistence.
Useful? React with 👍 / 👎.
| /// would stay invisible for the rest of the process. Idempotent — the | ||
| /// per-channel single-owner guard makes a spawn for an already-listening | ||
| /// dispute a no-op. | ||
| /// Rebuild dispute records for orders with a persisted solver pubkey. |
There was a problem hiding this comment.
This introduces externally observable persistence and restart-rehydration behavior for get_dispute and submit_evidence, but specs/004-mostro-p2p-client/contracts/disputes.md remains unchanged and documents only the in-process lifecycle. Add the persisted solver-key and rehydration semantics, including terminal-state handling and platform limitations, to the matching contract as required for behavior changes.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/src/api/disputes.rs (1)
656-698: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSolid persistence tests, but the rehydration logic itself is untested.
the_solver_pubkey_outlives_the_in_memory_recordandeach_order_gets_its_own_solver_keyonly exercise the rawSqliteStorageget/set primitive. The more complex piece added in this PR —rehydrate_disputes_from_storage(trade enumeration, in-memory presence check,Disputereconstruction) — has no direct test, so the race and staleness issues flagged above wouldn't have been caught by this suite.As per coding guidelines, "Add targeted tests when expanding complex logic, asynchronous workflows, or protocol handling."
🤖 Prompt for 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. In `@rust/src/api/disputes.rs` around lines 656 - 698, Extend the dispute tests to directly cover rehydrate_disputes_from_storage rather than only SqliteStorage primitives: seed persisted dispute-admin data and the required trade records, invoke rehydrate_disputes_from_storage, and assert each eligible dispute is reconstructed in memory with the correct order and solver key. Also cover the existing in-memory presence/staleness behavior so rehydration does not overwrite current records or retain stale entries.Source: Coding guidelines
🤖 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/disputes.rs`:
- Around line 414-469: The resolve_dispute() flow must delete the persisted
dispute_admin(order_id) setting when resolution succeeds, in addition to
updating the in-memory dispute record. Reuse the existing app database and
dispute_admin key helper, and handle database access/errors consistently with
nearby persistence operations so rehydrate_disputes_from_storage() cannot
restore resolved disputes as InReview.
- Around line 453-466: Update the rehydrated Dispute construction in the
dispute_store upsert flow to set is_read to false for restored InReview
disputes, ensuring active disputes surface as needing attention after restart.
---
Nitpick comments:
In `@rust/src/api/disputes.rs`:
- Around line 656-698: Extend the dispute tests to directly cover
rehydrate_disputes_from_storage rather than only SqliteStorage primitives: seed
persisted dispute-admin data and the required trade records, invoke
rehydrate_disputes_from_storage, and assert each eligible dispute is
reconstructed in memory with the correct order and solver key. Also cover the
existing in-memory presence/staleness behavior so rehydration does not overwrite
current records or retain stale entries.
🪄 Autofix (Beta)
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: 36e3bbb2-925d-4ed7-baab-7ae222433b09
📒 Files selected for processing (1)
rust/src/api/disputes.rs
…ault, contract - Clear the persisted `dispute_admin:<order_id>` key when a dispute resolves, and have rehydration skip (and clear) trades whose order status is finished. The stored solver key is what rehydration reads as "this dispute is live", so left behind it resurrected resolved disputes as InReview on every restart, armed a listener and kept accepting evidence. The trade status is the durable signal: the daemon's admin-settled/admin-canceled are persisted by the status-sync arm in orders.rs without being routed into the dispute store. - Rehydrated records default to unread. The pre-restart read state cannot be recovered, and an active dispute must surface rather than be marked seen. - Document the persistence and restart semantics in the disputes contract: the persisted field, rehydration rules, terminal-state handling, and the web limitation (no init_db off native, IndexedDB list_trades still the stub of #233), which is also noted at the rehydration site. Tests: terminal vs live status classification, and the cleared key not coming back across a store reopen.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/api/disputes.rs (1)
484-512: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClear stale solver keys before returning from the cache hit.
When
dispute_store().get(&order_id)returns a record, the loop continues beforeis_order_finished()can delete a leftover dispute-admin setting. If that record was created while the order was live, it can stay in memory while the order is terminal, so the persisted key is never cleaned up until the dispute record is dropped and rehydration runs again. Check the order status and callclear_admin_pubkey(&order_id).awaitbefore this earlycontinue.🤖 Prompt for 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. In `@rust/src/api/disputes.rs` around lines 484 - 512, Update the cache-hit branch in the trade rehydration loop so that, when dispute_store().get(&order_id) returns a record, it checks is_order_finished(&trade.order.status) and calls clear_admin_pubkey(&order_id).await before continuing. Preserve the existing continue behavior for cached records and avoid changing the handling of cache misses.
🤖 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/disputes.rs`:
- Around line 759-820: Replace the primitive-only coverage in
clearing_the_solver_key_stops_the_dispute_coming_back,
finished_orders_are_not_rehydrated, and a_live_dispute_is_still_rehydrated with
a native SqliteStorage integration test that initializes app_db’s dispute store
and invokes rehydrate_disputes_from_storage. Set up persisted live and finished
dispute keys, verify live disputes are restored, finished entries are removed
via clear_admin_pubkey, and confirm an already in-memory dispute is not
duplicated.
---
Outside diff comments:
In `@rust/src/api/disputes.rs`:
- Around line 484-512: Update the cache-hit branch in the trade rehydration loop
so that, when dispute_store().get(&order_id) returns a record, it checks
is_order_finished(&trade.order.status) and calls
clear_admin_pubkey(&order_id).await before continuing. Preserve the existing
continue behavior for cached records and avoid changing the handling of cache
misses.
🪄 Autofix (Beta)
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: 6ae7174b-e432-4b6c-ac64-3a16e9cf0ec6
📒 Files selected for processing (2)
rust/src/api/disputes.rsspecs/004-mostro-p2p-client/contracts/disputes.md
…lity Conflict in `handle_admin_took_dispute`: main replaced the check-then-act create path with a single `upsert_or_update` under one store write lock (PR #253 review — a separate check races `open_dispute`'s post-publish insert in both directions). Took main's version; the persistence this branch adds needed no special case there, since the single `persist_admin_pubkey` call after the store write already covers both the create and the update path.
Review round 2 on #256: the previous tests asserted on `is_order_finished` and on the KV round trip in isolation, so nothing covered the pass that actually uses them. This drives `rehydrate_disputes_from_storage` over a real `SqliteStorage` with the app_db store initialized, seeding persisted trades and solver keys, and asserts the three rules: a live dispute is restored as InReview and unread, a finished one is skipped *and* its stale key deleted, and an already-in-memory record is not overwritten by the older stored solver.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/api/disputes.rs (1)
201-217: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEntry guard misses a concurrent in-flight open for the same trade.
The check only consults
dispute_store(), which is still empty while anotheropen_disputefor the sametrade_idis between publish and insert. Both calls publishAction::Dispute; only the loser's insert fails.pending_opens()already tracks exactly this state — reuse it here.🛡️ Proposed fix: reject when an open is already in flight
if let Some(existing) = dispute_store().get(&trade_id).await { if existing.status != DisputeStatus::Resolved { bail!("DisputeAlreadyOpen: dispute already exists for trade {trade_id}"); } } - pending_opens() - .lock() - .expect("pending_opens poisoned") - .insert(trade_id.clone()); + // Claim the in-flight slot atomically: a false return means another + // open for this trade is already between publish and insert. + if !pending_opens() + .lock() + .map(|mut set| set.insert(trade_id.clone())) + .unwrap_or(false) + { + bail!("DisputeAlreadyOpen: an open request is already in flight for trade {trade_id}"); + } let _pending = PendingOpenGuard(trade_id.clone());🤖 Prompt for 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. In `@rust/src/api/disputes.rs` around lines 201 - 217, Update the entry guard in open_dispute to also reject when pending_opens() already contains the same trade_id, before publishing the dispute request. Reuse the existing pending_opens tracking and preserve the current rejection behavior for existing unresolved disputes and the subsequent PendingOpenGuard insertion.
🤖 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.
Outside diff comments:
In `@rust/src/api/disputes.rs`:
- Around line 201-217: Update the entry guard in open_dispute to also reject
when pending_opens() already contains the same trade_id, before publishing the
dispute request. Reuse the existing pending_opens tracking and preserve the
current rejection behavior for existing unresolved disputes and the subsequent
PendingOpenGuard insertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b244d4bc-eee2-4b8d-b112-016023f96f52
📒 Files selected for processing (1)
rust/src/api/disputes.rs
1 participant
Third in the series, stacked on #254 — re-target to
mainonce that merges.Problem
The dispute record is in-memory by design: status and resolution come back from daemon events. The solver pubkey does not. It arrives exactly once, in
admin-took-dispute, and without it the dispute chat keys cannot be derived again.So a restart mid-dispute left the party unable to reach the solver at all — and
resubscribe_active_dispute_chats, added in the #254 review round, iterates the in-memory store, which after a restart is empty. It correctly handles the reconnect case (a listener that failed to start while online gets re-armed); the restart case had nothing to work with.The
dispute_admin:<order_id>key builder was already indb/mod.rs, but nothing read or wrote it.Change
handle_admin_took_disputewrites the solver pubkey to the settings KV, on both paths (existing record, and the peer-opened dispute that PR feat(disputes): route admin-took-dispute so the app learns the solver #253 creates).Details worth reviewing:
Storagetrait has none.InReview, which is exactly what a stored solver means. A later resolution arrives as a daemon event.submit_evidencework after a restart — it refuses without one.Scope note
This is a deliberate, narrow exception to "the dispute store is in-memory by design" (CLAUDE.md). Only the one field that cannot be re-derived is persisted — not status, not resolution, not the reason.
Test plan
cargo test— 209 passing. New: the solver pubkey survives closing and reopening the store (the restart this exists for), and each order gets its own key.cargo clippy --all-targets— 24 warnings, themainbaselinecargo check --locked --target wasm32-unknown-unknown— passesSummary by CodeRabbit