fix(#273): clear trades, messages and sessions on identity deletion - #298
fix(#273): clear trades, messages and sessions on identity deletion#298codaMW wants to merge 5 commits into
Conversation
…eletion Generating a new user rotated the identity but left the previous user's data behind: delete_identity() cleared the identity row and trade-key mappings but not the trades table, the messages table, or the in-memory sessions, so the new identity inherited the old one's My Trades list and chats — a privacy issue, and dead state (the trade keys were already cleared). Add clear_trades / clear_messages to the DB trait (SQLite implemented; IndexedDB stubbed alongside the existing clear_trade_keys pending MostroP2P#233) and a SessionManager::clear_all(). Call them from delete_identity() — messages before trades for the FK — and empty the in-memory sessions. On the Dart side, invalidate rawTradesProvider after regenerate() so My Trades (and the chat rooms derived from it) reflect the clean slate immediately. Verified on a physical device (Nokia C31): after Generate New User, My Trades and chats are empty. Adds a SQLite test that the clears empty both tables.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: WalkthroughIdentity 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. ChangesIdentity cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (6)
lib/features/account/screens/account_screen.dartrust/src/api/identity.rsrust/src/db/indexeddb.rsrust/src/db/mod.rsrust/src/db/sqlite.rsrust/src/mostro/session.rs
…web messages, test invalidation - delete_identity() now propagates clear_messages/clear_trades errors instead of logging and returning Ok. These tables are not identity-scoped and have no reconcile fallback, so a silent failure would leak the previous identity's history; propagating aborts regenerate/importAndStore before the replacement identity is created (deleteIdentity runs before the new identity exists). - IndexedDB clear_messages now clears MESSAGES_STORE in a read-write transaction rather than no-op'ing: messages are persisted on web (save_message), so identity deletion must actually wipe them. clear_trades stays a no-op (no web trades store yet, MostroP2P#233). - Add a provider test: invalidating rawTradesProvider after the DB is cleared yields an empty list (the cache reset the account screen relies on).
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
…y + failure test (MostroP2P#298 review) ermeme's review: delete_identity() cleared the in-memory identity BEFORE the fallible trades/messages cleanup. On failure it returned Err with the identity already gone, so regenerate() / importAndStore() saw NoIdentity, took the fresh-install path, and created a new identity while the previous one's unscoped trades and messages survived — a privacy leak. - Reordered: clear_messages/clear_trades now run while the identity is still in memory, holding the write lock across the awaits; *guard = None only after they succeed. On failure the identity stays and the caller can retry. - Extracted delete_identity_with<S: Storage>(db) as a store-injectable seam (mirrors derive_trade_key_with); public delete_identity() passes app_db. - Folded a failure-injection case 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 (255); clippy --locked -- -D warnings clean.
…es-messages-on-regenerate
…ades/clear_messages trait additions This branch adds clear_trades / clear_messages to the Storage trait. Merging current main brought in its FailingStore test double, which predates those methods, so it no longer satisfied the trait. Added unimplemented!() stubs for both to FailingStore (matching its convention), and the two forwards for main's newer update_trade_peer_reputation / mark_trade_rated to our delegating ClearTradesFailingStore. cargo test --lib green (338); clippy --locked -- -D warnings clean.
|
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. |
|
Fixed — cleanup now runs before the in-memory identity is dropped. Added the failure-injection test you asked for, folded into |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
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_identitynever 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_MAPstill resolves both the order id and the content fingerprint to the old index. That is what stops this from healing on its own:is_minedetection 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)fromaccount_screen.dartleaves the new test green. That follows from its shape: it overridesrawTradesProviderwith a closure over a mutable local, reassigns the local, invalidates, and asserts the new value. It exercises Riverpod'sinvalidate, never the screen the PR changes. AtestWidgetsthat 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 indelete_identitybreaks nothing in the suite. Low risk — withforeign_keys=ONper 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_messagesis not cleared. Those rows were built with the deleted identity's keys, andflush_message_queuepublishes them on the next Online. I did not verify what aQueuedMessagestores, 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 touchsettings.lightningAddress(orsettings.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.
ClearTradesFailingStorewraps 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()callsdeleteIdentity()thencreateIdentity(), so the cleanup does run when the button is pressed. - The IndexedDB asymmetry is reasoned correctly:
clear_tradesis a no-op because web has no trades store (#233), whileclear_messagesreally wipes, becausesave_messagereally 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
QueuedMessageholds. - The IndexedDB message wipe was not executed; the wasm
cargo checkonly proves it compiles.
2 participants
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 thetradestable (My Trades history), themessagestable (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
clear_trades/clear_messagesto the DB trait, mirroringclear_trade_keys. SQLite implements both; IndexedDB stubs them alongside the existingclear_trade_keysstub, pending IndexedDB persistence (Web: IndexedDB storage backend is a stub — nothing persists across a reload #233).SessionManager::clear_all()to drop every in-memory session.delete_identity()clear_messagesbeforeclear_tradesfor themessages.trade_id -> trades(id)FK and empty the in-memory sessions.rawTradesProviderafterIdentityService.regenerate()so My Trades reflects the clean slate immediately. The chat rooms list (chatRoomsFromTradesProvider) derives fromrawTradesProvider, so it clears in the same pass.Testing
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 analyzeclean.Closes #273.
Summary by CodeRabbit
New Features
Bug Fixes