Repository navigation
fix(disputes): sweep every dispute key of a finished order - #900
Conversation
…wept clear_dispute_keys deletes five keys, solver first, chat key share last. Rehydration decides whether a finished order still has any by reading two of them (solver, origin marker), so a clear that stopped after the solver leaves the solver's assignment time, the dispute's node and the chat key share where no later pass looks again. It is the likeliest cause of the CI failure of the_chat_key_share_survives_a_restart_for_its_solver on PR #899 (not reproduced locally: 40 runs of the dispute tests passed): another test's rehydration, walking the same global store, deleted the finished order's solver key but not yet its share when this test's own pass read the solver key and skipped the order. Refs #439 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Myq8f28gDqfJ4rUxarqYqn
Rehydration now looks for the same five keys clear_dispute_keys deletes, built from one list in settings_keys::dispute_keys, so a key added there is both cleared and swept. Until now it read only the solver and the origin marker: a clear that stopped after the solver (a failed delete, which is only logged, or a process killed between two) left the assignment time, the node and the chat key share for good, although clear_dispute_keys promised the next pass would clear them. Refs #439 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Myq8f28gDqfJ4rUxarqYqn
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared dispute keys and cleanup validation rust/src/db/mod.rs, rust/src/api/disputes.rs |
settings_keys::dispute_keys returns five persisted keys for each order. clear_dispute_keys deletes keys from this collection, and has_dispute_keys checks the same collection. A rehydration test checks that assignment-time, node, and chat-key-share settings are removed from a finished trade. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
MostroP2P/mostro#1086— The change clears persisted dispute settings during finished-trade rehydration, but does not repair the dispute row or skipped resolution messages described by the issue.
Suggested reviewers: catrya, forte11cuba
Merge Risk: 
884c4
-
No actionable merge-blocking issue is identified; the change is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title "fix(disputes): sweep every dispute key of a finished order" matches the main change. Rehydration now checks and clears the same five dispute settings keys that clear_dispute_keys deletes, t… Description check The description follows the repository template and fills in every required section. It names the linked issue (Refs #439, with an explanation of why it is a partial reference), sets the type to fix, …Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(disputes): sweep every dispute key of a finished order" matches the main change. Rehydration now checks and clears the same five dispute settings keys that clear_dispute_keys deletes, through the shared settings_keys::dispute_keys helper. The title is specific, uses the conventional fix(scope) form, and states the behaviour change without file lists or vague terms.
Full details: Description check
-
Explanation
The description follows the repository template and fills in every required section. It names the linked issue (Refs
#439, with an explanation of why it is a partial reference), sets the type to fix, and explains what changes and why in detail. It describes the affected flow, lists the blast radius (persisted data, cost, generated bindings, web, wire, identity wipe), and gives platforms tested. It also provides numbered manual testing steps with expected results, including a step that fails on main, and it lists the automated tests and a checklist. The Screenshots section states 'No visible change'. Some checklist items remain unchecked with explanations, and the manual testing steps are pending maintainer confirmation, which the author discloses. These gaps are non-critical, so the description is mostly complete.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each key in sight
Five settings vanish from the slate
Rehydration clears the finished trade
The shared key list keeps its place
Then hops away beneath the moon
Comment @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@coderabbitai review |
✅ Action performed
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Linked issue
Refs #439. This fixes one of the flaky tests it covers:
api::disputes::tests::the_chat_key_share_survives_a_restart_for_its_solver, which failed the Rust job of #899 (run 40377813795). The other tests #439 lists are untouched, so it stays open. This is a fix by a maintainer and is exempt from the accepted-issue rule (CONTRIBUTING.md § Contribution quality bar).Type
fix
What changes and why
clear_dispute_keysdeletes five settings keys for an order: solver, assignment time, node, origin marker and chat key share, in that order. Rehydration decides whether a finished order still has any of them, and it read only two, the solver and the origin marker. A clear that stopped after the solver key left the other three behind for good. The clear can stop there because of a failed delete, which is only logged, or because the process was killed between two deletes. Yetclear_dispute_keyspromised that "the stale key is seen again — and cleared again — on the next pass". Rehydration now reads the same five keys. Both functions build them from one list,settings_keys::dispute_keys, so a key added there is both cleared and swept.This is the likeliest cause of the CI failure. Six tests run
rehydrate_disputes_from_storageover the same global store, and each pass walks every test's rows. Another test's pass deleted the finished order's solver key but had not yet deleted its share. At that moment the failing test's own pass read the solver key, found nothing and skipped the order. The test then found the share still there. With this change the test's own pass still sees the share and deletes it, whatever the other pass is doing.Affected flow
No screen or user action. The change runs during dispute rehydration from storage: when the relay pool comes online (
resubscribe_active_dispute_chats) and when an identity session is restored. It touches finished orders only:SettledByAdmin,CanceledByAdmin,CompletedByAdmin,Success,Canceledand the others inis_order_finished.rust/src/api/disputes.rs(has_dispute_keys,clear_dispute_keys) andrust/src/db/mod.rs(settings_keys::dispute_keys).Blast radius
dispute_admin_at:,dispute_node:anddispute_key_shared:keys of an order whose trade is already finished. These are the rowsclear_dispute_keysdeletes on every resolution anyway. On an existing install, keys left by an earlier partial clear are swept once, at the next rehydration.crate::api; the new helper lives incrate::db.scripts/check-generated.shpasses (Flutter 3.41.9).Storagetrait, so IndexedDB behaves like SQLite.IDENTITY_SCOPED_PREFIXES.Platforms tested
Linux (Rust tests only; no app flow).
Manual testing
Setup
This branch, and
mainwith only itstest:commit (89f4cc5) applied. Run fromrust/.Steps
main) At 89f4cc5:cargo test --lib api::disputes::tests::a_finished_order_s_keys_left_by_a_partial_clear_are_sweptExpected:
FAILED, withdispute_admin_at:partial-… stayed.Expected:
ok.cargo test --libExpected:
test result: ok. 1478 passed; 0 failed; 30 ignored.cargo clippy --locked -- -D warningsExpected: no warnings.
Not covered
The race itself was not reproduced locally: 40 runs of
cargo test --lib api::disputes::tests -- --test-threads=16onmainall passed. The red test reproduces the state that race leaves behind deterministically, instead of the interleaving that produces it. A second path to the same CI failure stays open: the test's ownlist_tradesfailing while another test's pass skips the order. There is no evidence that it happened. Serialising the tests that rehydrate over the global store, which is what #439 suggests, would close it.Screenshots
No visible change.
Automated tests
a_finished_order_s_keys_left_by_a_partial_clear_are_swept(rust/src/api/disputes.rs). It leaves on a finished order the three keys a clear that stopped after the solver key leaves, runs rehydration and asserts all three are gone. It fails onmainand passes here.the_chat_key_share_survives_a_restart_for_its_solver, the test that flaked, is unchanged. It now gets a pass of its own that deletes the share, even when another test's pass has already deleted the solver key.cargo test --lib:test result: ok. 1478 passed; 0 failed; 30 ignored; 0 measured; 0 filtered outChecklist
cd rust && cargo fmt --check && cargo clippy -- -D warnings && cargo testpass locally.cargo clippy --locked -- -D warningsandcargo testpass.cargo fmt --checkalready fails onmainwith the local rustfmt 1.10.0, on lines this PR does not touch; none of its complaints are on a changed line.dart format --set-exit-if-changed .,flutter analyzeandflutter testpass locally (n/a: no Dart change)rust/src/api/: regeneration produces no diff (scripts/check-generated.sh)AI assistance
Claude Code diagnosed the CI failure from the job log and the code, then wrote the test, the fix and this description.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Myq8f28gDqfJ4rUxarqYqn
Summary by CodeRabbit