fix(#273): clear trades, messages and sessions on identity deletion

[codaMW]

Problem

"Generate new user" rotated the identity but never deleted the data derived from the old one. delete_identity() cleared the in-memory identity, the persisted identity row, the trade-key mappings and the logs but the trades table (My Trades history), the messages table (chat history), and the in-memory sessions survived. The new identity started with fresh keys yet inherited the previous user's entire trade list and conversations a privacy issue, and dead state (those trade keys were already cleared, so nothing could operate on the orders).

Fix

• Add clear_trades / clear_messages to the DB trait, mirroring clear_trade_keys. SQLite implements both; IndexedDB stubs them alongside the existing clear_trade_keys stub, pending IndexedDB persistence (Web: IndexedDB storage backend is a stub — nothing persists across a reload #233).
• Add SessionManager::clear_all() to drop every in-memory session.
• Call all three from delete_identity() clear_messages before clear_trades for the messages.trade_id -> trades(id) FK and empty the in-memory sessions.
• Dart: invalidate rawTradesProvider after IdentityService.regenerate() so My Trades reflects the clean slate immediately. The chat rooms list (chatRoomsFromTradesProvider) derives from rawTradesProvider, so it clears in the same pass.

Testing

• SQLite test: after clear_messages + clear_trades, both tables are empty; clearing again on empty tables is a no-op.
• cargo test --lib (255) / clippy -D warnings / wasm check all green; flutter analyze clean.
• Verified on a physical device (Nokia C31): create an order, Generate New User → My Trades and chats are empty.

Closes #273.

Summary by CodeRabbit

• New Features

  • Added comprehensive identity deletion cleanup, including persisted trades, chat messages, and active sessions.
  • Newly generated identities now immediately clear cached trade data.
  • Repeated cleanup operations are handled safely without errors.

• Bug Fixes

  • Prevented deleted identity data from remaining accessible through cached or persisted records.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: a7a4cd18-ceba-42f1-8638-164bbcc93d99

Walkthrough

Identity cleanup now removes persisted trades, messages, and in-memory sessions. Identity regeneration also invalidates the cached trades provider. SQLite supports the cleanup operations, while IndexedDB uses successful no-op implementations.

Changes

Identity cleanup

Layer / File(s)	Summary
Storage cleanup rust/src/db/mod.rs, rust/src/db/sqlite.rs, rust/src/db/indexeddb.rs	The storage interface and backends now expose trade and message clearing. SQLite deletes all rows and tests repeated clearing. IndexedDB returns success without changing persistence.
Session clearing rust/src/mostro/session.rs	SessionManager::clear_all removes every in-memory session and returns the previous count.
Identity and UI cleanup rust/src/api/identity.rs, lib/features/account/screens/account_screen.dart	Identity deletion clears persisted data and sessions while logging non-fatal cleanup errors. Identity regeneration invalidates rawTradesProvider.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• fix(account): "Generate new user" keeps the previous identity's trades and chats #273 — Covers identity deletion, database cleanup, session clearing, and trade-provider invalidation.

Possibly related PRs

• MostroP2P/app#204 — Related to identity recovery and trade/session state management.
• MostroP2P/app#242 — Also changes in-memory cleanup in rust/src/api/identity.rs.
• MostroP2P/app#274 — Directly related to trade cleanup and session state.

Suggested reviewers: grunch, catrya

Poem

A rabbit cleared the trades away,
And messages followed without delay.
Old sessions hopped out of sight,
Fresh identity brought cache-light.
SQLite now starts clean and bright.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: clearing trades, messages, and sessions when an identity is deleted.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@lib/features/account/screens/account_screen.dart`:
- Around line 390-395: Add a focused provider test for the identity-regeneration
cache reset around rawTradesProvider: override it with a mutable fetcher, verify
an initial non-empty result, change the fetcher to return an empty list,
invalidate rawTradesProvider, and assert the subsequent read is empty without
invoking native identity APIs.

In `@rust/src/api/identity.rs`:
- Around line 307-317: The delete_identity cleanup path must not report success
when clear_messages or clear_trades fails. Update delete_identity to propagate
either cleanup error and stop before identity replacement, ensuring
importAndStore and regenerate cannot proceed; alternatively, make both cleanup
operations atomic in one transaction.

In `@rust/src/db/indexeddb.rs`:
- Around line 243-249: Implement clear_messages to open a read-write IndexedDB
transaction for MESSAGES_STORE and clear its object store, awaiting the
operation and propagating any errors through Result. Leave clear_trades
unchanged as a no-op.

🪄 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: 4af5cd31-2de4-4bfc-b866-8b97070cbe53

📥 Commits

Reviewing files that changed from the base of the PR and between 7625b24 and 9aca693.

📒 Files selected for processing (6)

• lib/features/account/screens/account_screen.dart
• rust/src/api/identity.rs
• rust/src/db/indexeddb.rs
• rust/src/db/mod.rs
• rust/src/db/sqlite.rs
• rust/src/mostro/session.rs

[ermeme]

Reviewed current head 6a8bbbb7fc2658d756e4a13afce20c9c824ef7be.

Blocking change required

delete_identity() clears the global in-memory identity at rust/src/api/identity.rs:292 before running the new database cleanup. If clear_messages() or clear_trades() then fails, the function returns an error while the old identity is already gone and the old rows may remain. This is not fully recoverable through the current callers: a later regenerate() treats NoIdentity as the fresh-install case and proceeds to createIdentity(), while importAndStore() retries the same NoIdentity path and imports the replacement. Both can therefore create/use a new identity while the previous identity’s unscoped trades/messages remain in the database, violating the PR’s privacy guarantee. The comment claiming there is “no half-rotated state to unwind” does not hold because the in-memory state is cleared before the fallible cleanup.

Please make deletion failure-safe: perform the required cleanup before dropping the in-memory identity, make cleanup atomic where possible, or persist an explicit deletion-pending/failed state that prevents replacement until cleanup succeeds. Add a failure-injection test covering a cleanup error followed by a retry/replacement attempt.

The existing success-path tests and CI checks do not cover this failure/retry sequence. Local Rust identity tests passed (15 tests); Flutter could not be run because the flutter executable is unavailable in the review environment.

[codaMW]

Fixed cleanup now runs before the in-memory identity is dropped.

`delete_identity` delegates to `delete_identity_with<S: Storage>(db)`; the load-bearing `clear_messages` / `clear_trades` run while the identity is still in memory (the write lock is held across the awaits so no replacement can slip in), and `*guard = None` only happens after they succeed. So a cleanup failure returns `Err` with the identity intact, and `regenerate` / `importAndStore` can't create a fresh identity over the previous one's rows closing the privacy leak.

Added the failure-injection test you asked for, folded into `load_derive_then_delete_identity_lifecycle` (the singleton-owning test, so it can't race other identity-lock tests): a `ClearTradesFailingStore` that delegates to a real store and fails only `clear_trades` proves the failure returns `Err`, the identity survives, and a retry against a working store completes the deletion.

Rebased onto current main. `cargo test --lib` green (338); `clippy --locked -- -D warnings` clean.

[codaMW]

Fixed — cleanup now runs before the in-memory identity is dropped. delete_identity delegates to delete_identity_with<S: Storage>(db); the load-bearing clear_messages / clear_trades run while the identity is still in memory (write lock held across the awaits), and *guard = None only after they succeed. A cleanup failure returns Err with the identity intact, so regenerate / importAndStore can't create a fresh identity over the old rows.

Added the failure-injection test you asked for, folded into load_derive_then_delete_identity_lifecycle (the singleton-owning test): a ClearTradesFailingStore that delegates to a real store and fails only clear_trades proves the failure returns Err, the identity survives, and a retry against a working store completes the deletion. cargo test --lib green (338); clippy --locked -- -D warnings clean.

[Catrya]

Changes requested — the persistent half is solid; the in-memory half the issue also names is missing

The reordering is right and the failure-path test added in the last round is genuinely good. What is left is the in-memory state, and that is not out of scope: issue #273 says in as many words that "the in-memory sessions are not emptied either". This empties the sessions and leaves the rest.

Blocking: the order book and the trade-key maps survive identity deletion

Measured by calling the real delete_identity() with this PR applied, against the same globals the app uses:

PROBE before: book=1 mine=1 map_order=Some(3) map_fingerprint=Some(3)
PROBE after:  book=1 mine=1 map_order=Some(3) map_fingerprint=Some(3)
PROBE db trade_keys rows left: Ok(None)

The database row is gone (Ok(None)), but:

• The order book still holds the previous identity's order, still flagged is_mine = true. order_book().clear() exists and is used on a node switch (orders.rs:3043); delete_identity never calls it. Home keeps showing the previous user's orders as theirs — with the "you are selling/buying" pill, and tapping one routes to the own-order screen — for the rest of the session.
• TRADE_KEY_MAP still resolves both the order id and the content fingerprint to the old index. That is what stops this from healing on its own: is_mine detection during ingest looks up that fingerprint, so when those Kind 38383 events arrive again from the relay the order is re-marked as the new identity's own. It only clears when the process restarts and the map rebuilds from an empty table. TRADE_KEY_MISSES (the negative cache from #362) is in the same position.

The fix is the same size as what the PR already does: call order_book().clear() and empty the two maps inside delete_identity, next to the clear_all() for sessions that is already there.

Minor

• The Dart test is a tautology, and mutation shows it. Deleting ref.invalidate(rawTradesProvider) from account_screen.dart leaves the new test green. That follows from its shape: it overrides rawTradesProvider with a closure over a mutable local, reassigns the local, invalidates, and asserts the new value. It exercises Riverpod's invalidate, never the screen the PR changes. A testWidgets that pumps the account screen and asserts the invalidation would be a different thing; as written the test protects nothing.

• The messages-before-trades ordering is documented but untested. The comment explains the FK correctly (messages.trade_id REFERENCES trades(id), confirmed in the schema), but the SQLite test only ever calls them in the right order, so swapping the two calls in delete_identity breaks nothing in the suite. Low risk — with foreign_keys=ON per connection since #351 it would fail at runtime — but it is free to pin.

• Two things that survive and are outside #273's scope, but belong to the same button:

  • queued_messages is not cleared. Those rows were built with the deleted identity's keys, and flush_message_queue publishes them on the next Online. I did not verify what a QueuedMessage stores, so I am not claiming a leak — but publishing under the old identity after "Generate new user" would be the same problem through another channel, and it is worth a look.
  • The saved Lightning address. regenerate() rewrites the mnemonic, trade-key index, privacy mode and creation date (identity_service.dart:144-154) but does not touch settings.lightningAddress (or settings.fiatCode) in SharedPreferences. An LN address identifies a person rather better than a trade list does, and it survives the reset.

What I verified

• The probe above, against the real globals.
• The failure-path test is good, and that deserves saying. ClearTradesFailingStore wraps a real store instead of reimplementing the trait, so trait growth cannot silently break it, and the test asserts the thing that matters: after the failure the identity is still present, and a retry against a working store completes the deletion. That is exactly what the previous round asked for.
• The reordering is correct and load-bearing: the write lock is held across the cleanup awaits, so no replacement can slip into the window, and none of the DB calls re-enter the identity module, so there is no deadlock path.
• The Dart route reaches it: regenerate() calls deleteIdentity() then createIdentity(), so the cleanup does run when the button is pressed.
• The IndexedDB asymmetry is reasoned correctly: clear_trades is a no-op because web has no trades store (#233), while clear_messages really wipes, because save_message really writes. That is the right distinction and it is commented.
• Full CI on the tree merged with current main (merges clean, 4 commits behind): cargo test --locked → 340 passed, 0 failed; cargo clippy --locked -- -D warnings → clean; cargo check --locked --target wasm32-unknown-unknown → clean; flutter test → 314 passed; flutter analyze → zero issues in hand-written code.

Not verified

• The on-device repro in the description (Nokia C31). What it checked — My Trades and chats empty — is real; what that check does not cover is Home, which is where the blocker lives.
• Whether the outbox would publish under the old identity: I did not inspect what QueuedMessage holds.
• The IndexedDB message wipe was not executed; the wasm cargo check only proves it compiles.

[Catrya]

Closing this as superseded by #533, whose scope covers this bug end to end, and which is implemented in #543 (merged).

The direction here was right and the review rounds on this branch got the persistent half into good shape: clear_trades / clear_messages on the Storage trait, the SQLite implementations, SessionManager::clear_all(), the reordering so the cleanup runs before the in-memory identity is dropped, and the failure-injection test that wraps a real store instead of reimplementing the trait. #533's step 1 and 2 are that plan.

Rather than lose it, here is what a review of this branch measured against main (at the time: main 5f2dda2, this head 5ad4451). Several of these are things #533/#543 address; the rest are recorded so they are not rediscovered from scratch.

Blocking, measured

1. Merged with main, the tests did not compile. ClearTradesFailingStore lacked update_trade_counterparty, which Peer chat: the counterparty pubkey is never persisted, so the maker has no chat room and nobody can send after a restart #334 added to the trait. cargo clippy passes because it does not build test code, so only cargo test catches it (E0046). With that one method delegated, the tree gave 460 passed, 0 failed.
   Worth recording the lesson: a hand-written delegating double still has to enumerate every method, so it breaks on trait growth exactly like a reimplementing one. Only the doc comment claimed otherwise.
2. The order book and the trade-key map survived the rotation. Re-measured live against a daemon: after "Generate new user", order X created by identity A stayed in the book as (is_mine = true, Pending), and TRADE_KEY_MAP still resolved it to A's index.
3. The process kept A's trade secret keys and stayed subscribed to them. GLOBAL_DM_KEYS is union-only by design, so after the swap it still held A's Keys (verified: the retained secret equals A's derived trade secret). When B created its first order the bulk feed's coverage was [15ad1e34 (A), 34876b68 (B)], and mostro-dm's REQ is built from that map — one filter carrying both identities' trade pubkeys, which is exactly what a new identity is supposed to prevent. A's traffic also kept decrypting inside B's session.
   Note for whoever touches this: replace_global_dm_filter returns early when the map is empty, so clearing the map alone does not retire the REQ — it needs an explicit close.
4. Web was still leaking. clear_trades was Ok(()) with "No trades store on web yet (Web: IndexedDB storage backend is a stub — nothing persists across a reload #233)", but since feat: Linux accessibility contract and Web persistence for Mortsom #408 the web backend persists trades (save_trade → TRADES_STORE) and init_db runs on web, so the rotation would have kept the previous identity's trades on the deployed web build. (Verified by reading; the repo has no wasm test harness.)
5. A failed wipe damaged the surviving identity. With a store whose clear_trades fails: the identity stays in memory, but its persisted row and its trade_keys mappings are already gone, because the best-effort deletes run before the fallible ones. After a restart the surviving user's trades have no key mapping and lookup_trade_key_index has no fallback to the trade row, so release / fiat-sent / cancel would fail with "trade key not found".

Test coverage, by mutation

• Removing clear_messages from delete_identity: 460 tests still green.
• Removing the session clear_all(): green.
• Swapping the messages/trades order: green.
• The Dart test is a tautology: deleting ref.invalidate(rawTradesProvider) from the screen leaves it passing, because it exercises Riverpod's invalidate over a local, never the screen.
• Reverting the persist-before-drop reorder was caught, by load_derive_then_delete_identity_lifecycle.

Smaller things measured

• The comment "messages before trades: messages.trade_id is an FK onto trades(id)" is stale — main dropped that FK in a migration, so the ordering is moot.
• Identity-scoped settings keys survived the rotation: chat_cursor:, dispute_admin:, dispute_mine:, status_cursor:. Dispute rehydration walks the trades rows, so once those are wiped the keys are orphaned and never swept.
• The Dart notifications store (sembast) survived, ledger included. v1 clears it on rotation — deliberately, since "chore: Clear notifications on master key regeneration" in Restore orders feature  mobile#355.
• Two concerns raised in an earlier round turned out not to be real: nothing calls queue_message, so the outbox is always empty in production, and registerToken has no callers, so no trade pubkey is registered with the push server.
• v1's better reference for this work is restore_manager.dart's _clearAll, not _generateNewMasterKey: the restore path also clears the order cache and invalidates the chat and dispute-chat providers, while the new-user path does not.

@codaMW thank you for the rounds on this one — the trait additions and the persist-before-drop ordering are the backbone of what shipped in #543. Further work on this bug belongs in #533.