-
Notifications
You must be signed in to change notification settings - Fork 13
fix(shutdown): stop wallet backend on GUI close and clear stale shutdown-channel log #905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
fdeeacd
fix(shutdown): stop wallet backend on GUI close and clear stale shutd…
lklimek 80e27aa
fix(shutdown): make wallet teardown race-safe
lklimek 3848372
Merge branch 'v1.0-dev' into fix/gui-shutdown-wallet-backend
lklimek e5ddd8d
fix(shutdown): wait for backend-task blocking work before wallet tear…
lklimek 578e74f
fix(shutdown): close double-teardown and secret re-caching races foun…
lklimek cd4820b
Merge remote-tracking branch 'origin/v1.0-dev' into fix/gui-shutdown-…
lklimek a978ff3
fix: update merged test to new forward_backend_task_join_error signature
lklimek 0290608
fix: correct mislabeled platform PR reference in shutdown TODOs
lklimek 625e08b
Merge remote-tracking branch 'origin/v1.0-dev' into fix/gui-shutdown-…
lklimek 38dbb78
fix(shutdown): keep wallet teardown behind task barrier
lklimek 824bd2e
Merge branch 'v1.0-dev' into fix/gui-shutdown-wallet-backend
lklimek 26cb9d6
fix(shutdown): close remaining barrier gaps
lklimek 177aea3
Merge remote-tracking branch 'origin/v1.0-dev' into fix/gui-shutdown-…
lklimek a3a8da6
fix(app): store the secret prompt host on AppState
lklimek 7603be6
fix(shutdown): forget wallet secrets on degraded shutdown outcomes
lklimek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium: the degraded shutdown branches skip
forget_all_secrets, quietly breaking this PR's own CHANGELOG promiseIndependently flagged by two of my reviewers (security + QA), and it holds up on inspection.
finish_shutdown_after_tasksruns the wallet-teardown future — the only caller offorget_all_secrets()on the exit path — solely onTaskShutdownOutcome::Complete. OnBackendTasksTimedOutandNone/TaskManagerFailedit returns early and clears nothing.That branch is reachable in practice, not just in theory:
graceful_shutdown_budget()is 20s (2×SHUTDOWN_TIMEOUT), while backend network calls routinely run up toNETWORK_REQUEST_TIMEOUT= 90s. A user closing the app while a slow token/contract/network task is still in flight lands here — andSecretAccess::forget_all(src/wallet_backend/secret_access.rs:656-663) is a synchronous, I/O-freeRwLock::write().clear()with no dependency on any backend task finishing, so there's no good reason to gate it behind the tracked-join phase. Bothstart_async_shutdownandrun_blocking_shutdown_fallbackshare this function, so both paths inherit the gap — andbackend_task_timeout_skips_wallet_teardownassertsteardown_calls == 0for exactly this outcome, enshrining it.Meanwhile CHANGELOG.md (
### Fixed) promises, unconditionally, that closing the app "finishes wallet activity and clears in-memory secrets." The code doesn't, on these branches. TheZeroizingdrop backstop still fires at process teardown, so this is a widened plaintext-in-memory window rather than a permanent leak — hence Medium, not High — but for a wallet it's worth closing.Recommendation: split the teardown. Run
forget_all_secrets()on every collected context unconditionally (safe to run alongside a stuck/aborted task — it only clears the session cache; any in-flight op holds its own op-scopedZeroizingcopy), and keep the coordinator-joiningbackend.shutdown()gated onComplete. The deliberate choice to skipshutdown()on the degraded branch (avoiding teardown of resources still in use) is fine to keep — only the cheap secret clear needs decoupling. If the skip is genuinely intended, reword the CHANGELOG so it stops promising unconditional clearing. The sibling headless pathshutdown_app_context_wallet_backend(src/mcp/server.rs:16-33) has the same omission and deserves the same fix.🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (security + project + QA trio)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-verified against current
HEAD(26cb9d6d+ local merge ofv1.0-dev+ a CI compile fix, not yet pushed): still valid, not addressed.finish_shutdown_after_tasks(src/app.rs) still routesforget_all_secrets()exclusively throughfinish_wallet_shutdown, which only runs onTaskShutdownOutcome::Complete. TheBackendTasksTimedOutandNone(TaskManagerFailed) branches return early with no secret clearing — confirmed by thebackend_task_timeout_skips_wallet_teardowntest itself, which assertsteardown_calls == 0for that outcome. The commit that landed since your review (26cb9d6d, "close remaining barrier gaps") fixed the join-error-callback and MCP network-switch barrier gaps but didn't touch this one.Leaving unresolved — the recommended split (unconditional
forget_all_secrets()on every collected context,backend.shutdown()still gated onComplete) hasn't been implemented yet. Out of scope for this pass (CI-fix + comment triage only); tracking as a follow-up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
7603be6c.finish_shutdown_after_tasksnow callsforget_all_secrets()on every collected wallet backend unconditionally, before branching on the outcome — only the full coordinatorbackend.shutdown()join stays gated onComplete. Applies to bothstart_async_shutdownandrun_blocking_shutdown_fallback. New regression tests (backend_task_timeout_forgets_secrets_without_wallet_teardown,task_manager_failure_forgets_secrets_without_wallet_teardown) assert secrets are cleared with zero coordinator-shutdown calls on both degraded outcomes.