fix(shutdown): stop wallet backend on GUI close and clear stale shutdown-channel log#905
Conversation
…own-channel log Await every initialized wallet backend within the bounded GUI shutdown flow, including contexts completed by an in-flight network switch, and consume terminal shutdown receivers exactly once. The platform-wallet coordinator thread join race remains tracked upstream in dashpay/platform#3954 and is not fixed here. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe shutdown path now uses admission barriers, coordinated async teardown, typed outcomes, wallet-backend timeouts, context collection during cancellation, and hard-deadline viewport handling. Network switching and MCP dispatch participate in the shared shutdown lifecycle. ChangesShutdown and task lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCP
participant TaskManager
participant AppContext
participant WalletBackend
MCP->>TaskManager: register tracked dispatch
TaskManager->>AppContext: run backend task
AppContext-->>MCP: return task result and lifecycle tail
TaskManager-->>MCP: complete tracked dispatch
MCP->>WalletBackend: shut down outgoing backend
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
|
🕓 Ready for review — 38 ahead in queue (commit 7603be6) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new shutdown flow correctly consumes the terminal receiver and handles GUI network-switch results, but it does not yet guarantee that every wallet backend is stopped. Embedded MCP can own an active context outside the GUI registry, and TaskManager can accept a delayed task registration after shutdown has already drained its JoinSet. Both gaps can recreate the nondeterministic teardown this PR is intended to fix.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/app.rs`:
- [BLOCKING] src/app.rs:1840-1845: Include the active MCP context in wallet-backend shutdown
With the `mcp` feature enabled, the `network_switch` tool publishes its newly created context directly to `mcp_app_context` through `DashMcpService::swap_context`. That context is neither inserted into `network_contexts` nor returned through the GUI task-result receiver, so an MCP-initiated switch completed before GUI exit leaves the active context absent from this shutdown snapshot. The tool shuts down the outgoing backend before swapping, which means the omitted MCP context is precisely the backend still running at exit. Clone the shared ArcSwap before spawning shutdown, load its final value after the tracked MCP server has drained, and add it to the context set with pointer-based deduplication.
- [BLOCKING] src/app.rs:1848-1860: Task shutdown is not a registration barrier
Waiting for `subtasks.shutdown_async()` does not guarantee that every previously submitted task has been joined or aborted. `TaskManager::spawn_sync` records the name synchronously but launches an untracked wrapper that acquires the JoinSet mutex later; if shutdown acquires and drains that mutex first, the wrapper can register its future only after `shutdown_async()` resolves. A delayed `spawn_backend_init` then runs without checking the cancellation token and can create or start a wallet backend after this code has collected and shut down the known backends. Make task submission synchronously register or reserve the task before returning, or introduce a closed state that atomically rejects registration once shutdown begins.
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Automated grumpy-review — PR #905
Reviewed at fdeeacd2 (unchanged since review; no findings dropped as already-fixed).
The headline: this PR does not yet fully deliver its stated purpose
The PR body states the problem without qualification — "On GUI close, DET never stops the wallet backend ... it's simply abandoned at process exit" — and claims that "GUI close now awaits WalletBackend::shutdown() for every initialized network context." Two independent reviewers traced the actual call chains rather than trusting the description, and that claim does not hold in three verified cases:
- MCP-created contexts are structurally invisible to the shutdown sweep — not a race, but true under every timing once an MCP client has called
network_switch. This independently corroborates @thepastaclaw's already-open thread and is left there rather than duplicated here. - The
TaskManagerregistration barrier is not atomic, so a wallet-touching task can wire and start a backend after the sweep has already collected and shut down every known backend — the exact class of race the PR claims to have closed, via call sites theSwitchNetworkguard doesn't cover. Also left on @thepastaclaw's existing thread. on_exit's force-close fallback still never callsWalletBackend::shutdown()— posted inline below. The PR's own new comment names this path as reachable, and the 'Out of scope' section does not carve it out.
Because the 'Out of scope' section names only the upstream platform-wallet coordinator-thread join race (dashpay/platform#3954), the remaining gaps read as claimed-closed when they are not. Either close them, or explicitly narrow the title/description and file tracked follow-ups.
What's genuinely good here
- The common GUI-close path is now correct: existing contexts are snapshotted up front, and an in-flight GUI-driven
SwitchNetworkcontext is correctly observed via the shared channel before the shutdown task settles. - The stale-log fix is sound —
shutdown_receiveris consumed via.take()exactly once, so a later frame hits the newNone => truearm instead of re-triggering the false "possible panic" warning. A real sender drop/panic still reaches theClosedwarning, so the cleanup doesn't hide genuine failures. WalletBackend::shutdown()is documented idempotent, so the deliberate double-invocation between theSwitchNetworkcheck and the aggregate sweep is safe by design — that overlap is not a bug.- The sweep genuinely awaits each future rather than merely signalling stop.
- Call-tree walk found no stale callers of any changed/new signature.
cargo clippy --all-features --all-targets -- -D warnings(clean) andcargo test --lib app::(30/30) were independently reproduced, not taken from the PR's self-report.- The upstream dashpay/platform#3954 limitation is accurately disclosed.
Findings dropped as duplicates
Two blocking findings (the MCP-context blind spot and the TaskManager registration race) duplicate open @thepastaclaw threads and are not re-posted as new comments — each has a reply on its existing thread adding only what's genuinely new (root-cause mechanism, a second uncovered call site, and the secret-session lifetime angle).
Suggested action: request changes before merge — the fixes are narrow and well-scoped; no redesign needed.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
Close task registration before draining managed work, include the final MCP context, clear remembered secrets, and bound both graceful and fallback wallet backend shutdown paths. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes both prior blockers: shutdown now captures the final MCP context after managed task shutdown, and TaskManager closes registration atomically before taking its JoinSet. One cumulative blocking gap remains: timing out and aborting the tracked join watchers does not stop their already-running spawn_blocking backend operations, so wallet work can continue concurrently with wallet-backend teardown.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/utils/tasks.rs`:
- [BLOCKING] src/utils/tasks.rs:204-205: Aborting join watchers does not stop backend work
After SHUTDOWN_TIMEOUT expires, shutdown_inner aborts the remaining JoinSet entries and allows shutdown_async to report completion. The GUI entries named backend_task_join_watcher only await JoinHandles created by spawn_blocking in src/app.rs; once those blocking operations have started, aborting or dropping their JoinHandles cannot stop them. AppState then advances to shutdown_wallet_backends, so a slow wallet-touching run_backend_task operation can continue using, wiring, or writing through the same backend while its secrets are cleared and its storage is being shut down. Track actual completion of the blocking operations and wait for them to reach a terminal state, or add cooperative per-operation cancellation that reaches a terminal state before wallet teardown begins.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new-push delta merges v1.0-dev changes and does not modify the shutdown machinery, so the remaining blocker is still present at the exact head. The PR fixes the task-registration barrier and final MCP-context collection, but after the task timeout it can still begin wallet-backend teardown while an already-running blocking backend operation continues. The three focused shutdown tests pass, but none exercises this timeout and detached-operation behavior.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…down Backend tasks run inside tokio::spawn_blocking closures wrapped by an abortable async watcher. On shutdown timeout, aborting the watcher only cancelled the cancellable wrapper — the underlying spawn_blocking closure cannot be forcibly stopped and kept running, unobserved, while wallet backend secrets were cleared and storage torn down underneath it. TaskManager now tracks blocking-task completion via a Shared future kept independent of the abortable watcher, so shutdown can genuinely await real completion (bounded by a new blocking-task timeout) instead of an abortable wrapper. Both the async shutdown path and the on_exit force-close fallback route through the same two-phase wait before shutdown_wallet_backends runs. A still-running blocking task past the bound now reports a distinct degraded ShutdownOutcome::BackendTasksTimedOut instead of silently proceeding as if nothing were wrong. shutdown_hard_deadline() extended to cover both phases. Addresses PR #905 review thread PRRT_kwDOM8GK3c6R6sht. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…d in review - Latch `shutdown_started` at the first close attempt (graceful or force-close) so on_exit()'s guard blocks fallback re-entry on every branch, not only the clean-finish path -- stops a second close signal from racing wallet teardown against a still-running first attempt (SEC-001). - Call forget_all_secrets() again after the bounded wallet-shutdown wait resolves, including the timeout branch, so a backend task that re-caches a secret mid-wait is still cleared before exit (SEC-002). - Register a SwitchNetwork-created wallet backend before its slow network call resolves, and race that call against shutdown's cancellation token, so an in-flight network switch is discoverable to teardown instead of being invisible to it (CODE-003). - Prune completed blocking-task entries, drop dead TaskManager::shutdown(), centralize the shutdown budget constant, and add a CHANGELOG entry (non-blocking cleanup from the same review). Adds regression tests for all three fixes; cargo fmt/clippy/tests all clean (verified independently, not from the fixing agent's own report). Co-Authored-By: Codex Sol <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commits fix the ordinary watcher-abort race, MCP-context collection, and task-registration barrier. One blocking race remains: after blocking work times out, both shutdown paths begin wallet teardown while non-cancellable backend work is still running. This is a preliminary Codex-only checkpoint; no Sonnet coverage is claimed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/app.rs`:
- [BLOCKING] src/app.rs:1993-2004: Backend-task timeout still permits concurrent wallet teardown
`TaskShutdownOutcome::BackendTasksTimedOut` means at least one non-cancellable `spawn_blocking` closure remains active after the two bounded waits, but this code calls `finish_wallet_shutdown()` before inspecting that outcome. Those closures run wallet-touching backend operations that can still be signing, updating storage, or using coordinators, while wallet shutdown clears their secrets and calls `WalletBackend::shutdown()` on the same resources. The blocking fallback repeats this ordering at lines 2039-2055. A network switch also stores its new backend before `bootstrap_loaded_wallets()` finishes, but does not publish `NetworkContextRegistered` until that bootstrap returns, so a timed-out switch can remain absent from the collected teardown contexts. Do not start wallet teardown after `BackendTasksTimedOut`; either wait for actual terminal completion, or make every backend operation cooperatively cancel and acknowledge termination before teardown. If the bounded timeout must force process exit, skip shared wallet teardown on that degraded branch.
…wallet-backend # Conflicts: # CHANGELOG.md
The merge of origin/v1.0-dev (904ba0b) auto-merged src/app.rs cleanly at the text level, but left a semantic break: v1.0-dev's new test `panicking_scheduled_vote_sweep_clears_in_progress_guard` (added in #902) called `forward_backend_task_join_error(join_handle, ...)`, passing the raw `JoinHandle` — the pre-#905 signature. This branch's shutdown-race work changed the function to take an already-awaited `Result<(), JoinError>` (so the join can be observed via `tokio::select!` alongside shutdown), which every other call site in this file was updated for except this one new test that didn't exist when the signature changed. Add the missing `.await`, matching the two sibling tests immediately above and below it. No behavioral change to production code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The commented-out ShutdownReport check and the ShieldedShutdownIncomplete bucket TODOs referenced platform-pr3968 (rs-platform-wallet-storage, an unrelated SQLite persistence PR). The actual upstream work that restores these types is the ThreadRegistry/shutdown-report PR, platform#3954. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/wallet_backend/mod.rs (1)
1148-1150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant import.
WalletTransactionHistoryErroris already imported at the module level (line 153). This local import is redundant and will trigger anunused_importswarning undercargo clippy. As per coding guidelines, code should pass all clippy checks without warnings.♻️ Proposed refactor to remove the import
fn hydrate_persisted_transactions( &self, wallet_id: &WalletId, ) -> Result<PersistedTransactionHydration, TaskError> { - use crate::backend_task::error::WalletTransactionHistoryError; use dash_sdk::dpp::dashcore::hashes::Hash; use platform_wallet::changeset::PlatformWalletPersistence;🤖 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 `@src/wallet_backend/mod.rs` around lines 1148 - 1150, Remove the redundant local WalletTransactionHistoryError import from the function returning PersistedTransactionHydration, and rely on the existing module-level import. Keep the Hash import and surrounding logic unchanged.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 `@src/utils/tasks.rs`:
- Around line 123-170: Update spawn_blocking_sync so the on_join observer
remains awaited during shutdown instead of being dropped with state.tasks.
Ensure completion results from the blocking task continue reaching on_join,
including forward_backend_task_join_error and task_result_sender, while
preserving the existing completion-handle tracking and shutdown behavior.
---
Nitpick comments:
In `@src/wallet_backend/mod.rs`:
- Around line 1148-1150: Remove the redundant local
WalletTransactionHistoryError import from the function returning
PersistedTransactionHydration, and rely on the existing module-level import.
Keep the Hash import and surrounding logic unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b5631c2-47e5-44eb-8575-e833afd31b29
📒 Files selected for processing (5)
CHANGELOG.mdsrc/app.rssrc/backend_task/mod.rssrc/utils/tasks.rssrc/wallet_backend/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.rs
lklimek
added
the
claudius-review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR adds bounded wallet-backend teardown, but three shutdown-barrier gaps still allow wallet resources to be cleared or shut down while backend work remains active. The revised blocking-task tracker also loses late panic reporting when its ordinary observer is aborted, so the PR requires changes before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/utils/tasks.rs`:
- [BLOCKING] src/utils/tasks.rs:107-109: Rejected descendant registration detaches admitted work
When shutdown closes registration, `spawn_sync()` drops the supplied future. `await_managed_network_request_with_timeout()` transfers a timed-out request's `JoinHandle` into such a reaper future at `src/backend_task/mod.rs:97`; if registration is rejected, dropping that handle detaches the still-running request. The admitted outer backend task can then finish and let `TaskManager` report `Complete` while its request continues using the wallet backend. Registration rejection must return ownership so the caller can abort or await the request, or descendants created by admitted work must remain tracked through shutdown.
- [SUGGESTION] src/utils/tasks.rs:158-167: Shutdown can discard late blocking-task panic reports
The blocking closure's completion and its `on_join` callback have different lifetimes. After the ordinary-task timeout, `tasks.shutdown()` aborts the observer that owns `on_join`; if the blocking closure then finishes during the blocking-task phase, `shutdown_blocking_tasks()` treats the shared completion as ready but never takes its stored `JoinError`. A backend closure that panics during this interval can therefore produce a `Complete` shutdown outcome without reaching `forward_backend_task_join_error` or the task-result channel. Keep error observation attached to the tracked completion, or make the blocking shutdown phase consume the result and execute the callback.
In `src/mcp/dispatch.rs`:
- [BLOCKING] src/mcp/dispatch.rs:25-32: MCP backend operations remain outside the shutdown barrier
Embedded MCP requests execute backend operations through a raw `tokio::task::spawn_blocking`, while `TaskManager` tracks only the enclosing HTTP server future. Cancellation or the ordinary-task timeout can drop the request's join handle without stopping its non-cancellable closure, after which the PR's new shutdown path loads the MCP context and tears down its wallet backend. MCP payments, withdrawals, shielded operations, and other backend tasks can therefore overlap wallet teardown. Register MCP dispatch work with the real-completion barrier used by GUI backend tasks, or add an equivalent in-flight request barrier that completes before wallet teardown.
Track timed network requests and MCP backend work before wallet teardown, and skip secret clearing whenever managed work fails to drain. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
Pushed fixes for the 3 blocking shutdown-race findings, plus the diagnostics-loss nit:
Commit: 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head fixes the wallet-teardown ordering and rejected descendant-registration defects. Two shutdown-barrier gaps remain: the MCP network-switch handler performs context publication and outgoing teardown after its tracked dispatch completes, and the join observer can consume a panic result before an abortable callback finishes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/mcp/tools/network.rs`:
- [BLOCKING] src/mcp/tools/network.rs:247-250: MCP network switch leaves its final lifecycle work outside the barrier
`dispatch_task()` tracks only `BackendTask::SwitchNetwork`. Once that task returns the newly created context, the handler separately awaits shutdown of the outgoing backend and then publishes the new context through `swap_context()`. The MCP dispatch uses a deliberately dropped task-result receiver, so the task's earlier `NetworkContextRegistered` result cannot register the new context with GUI shutdown. If shutdown cancels or times out the MCP request during this tail, `TaskManager` can report `Complete` while `finish_wallet_shutdown()` still sees the old context in the MCP `ArcSwap`; the newly initialized backend then misses explicit teardown. Keep outgoing teardown and context publication within tracked work, or publish/register the new context before the tracked dispatch can complete.
In `src/utils/tasks.rs`:
- [SUGGESTION] src/utils/tasks.rs:214-220: Shutdown can still discard a pending join-error callback
The ordinary observer removes the stored join result with `take()` before awaiting `on_join`. If the ordinary shutdown deadline expires after that removal but before the callback completes, `tasks.shutdown()` aborts the observer. The tracked phase then sees a completed task with no stored result and can return `Complete`, even though the typed panic report or other callback side effect was discarded. The new regression test covers completion after the observer has already been aborted, but not this consume-before-callback-completion interval. Keep the result available until `on_join` completes, or track callback completion as part of the shutdown barrier.
|
Ran a comment-verification pass on this PR. One note: thepastaclaw's latest review (2026-07-21T03:37:05Z, head MCP network switch leaves its final lifecycle work outside the shutdown barrier. The one existing inline thread (tasks.rs consume-before-callback race) was also verified still open — replied there directly. 🤖 Co-authored by Claudius the Magnificent AI Agent |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claudius grumpy-review — PR #905 (shutdown/wallet-backend teardown)
I set my crew of three loose on this — security, structural, and adversarial-QA — and, credit where it's due, this is a tidy piece of concurrency work. The shutdown state machine, the registration barrier, the bounded 55s viewport deadline, and the network-switch TOCTOU fix all survived a full call-tree walk and a live test run (utils::tasks 8/8, app::shutdown_tests 8/8, genuinely executed — I don't take "trust me" for an answer). The removed blocking TaskManager::shutdown left zero orphaned callers. Nicely done.
Verdict: not blocking, but one Medium worth your attention.
- 🟡 1 Medium — the degraded
BackendTasksTimedOut/TaskManagerFailedshutdown branches skipforget_all_secrets, contradicting this PR's own CHANGELOG promise of unconditional secret clearing. Inline comment onsrc/app.rs. Cheap, safe fix (decouple the sync secret-clear from the coordinator-joiningshutdown()), or reword the CHANGELOG. The headlesssrc/mcp/server.rspath shares the omission. - ⚪ 6 Low —
spawn_tracked_sync/register_tracked_completiongate mismatch droppingon_joinin a narrow shutdown race; duplicated prologue between the async and blocking shutdown drivers; the undocumented doubleforget_all_secrets; a tautological deadline test; and a call-tree consistency note. All optional polish — noted here but not posted individually per Medium-and-above policy.
One prior review thread (src/utils/tasks.rs — the .take()-before-on_join abort window) remains legitimately open; I re-verified it against HEAD and replied there. Not approving while that and the Medium stand.
Full severity-ranked report with reproductions and code snippets was generated alongside this review.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio: security-engineer + project-reviewer + qa-engineer)
📊 View full HTML review report
| match task_shutdown_outcome { | ||
| Some(TaskShutdownOutcome::Complete) => wallet_shutdown.await, | ||
| Some(TaskShutdownOutcome::BackendTasksTimedOut) => { | ||
| ShutdownOutcome::BackendTasksTimedOut | ||
| } | ||
| None => ShutdownOutcome::TaskManagerFailed, | ||
| } |
There was a problem hiding this comment.
🟡 Medium: the degraded shutdown branches skip forget_all_secrets, quietly breaking this PR's own CHANGELOG promise
Independently flagged by two of my reviewers (security + QA), and it holds up on inspection. finish_shutdown_after_tasks runs the wallet-teardown future — the only caller of forget_all_secrets() on the exit path — solely on TaskShutdownOutcome::Complete. On BackendTasksTimedOut and None/TaskManagerFailed it 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 to NETWORK_REQUEST_TIMEOUT = 90s. A user closing the app while a slow token/contract/network task is still in flight lands here — and SecretAccess::forget_all (src/wallet_backend/secret_access.rs:656-663) is a synchronous, I/O-free RwLock::write().clear() with no dependency on any backend task finishing, so there's no good reason to gate it behind the tracked-join phase. Both start_async_shutdown and run_blocking_shutdown_fallback share this function, so both paths inherit the gap — and backend_task_timeout_skips_wallet_teardown asserts teardown_calls == 0 for 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. The Zeroizing drop 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-scoped Zeroizing copy), and keep the coordinator-joining backend.shutdown() gated on Complete. The deliberate choice to skip shutdown() 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 path shutdown_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.
Re-verified against current HEAD (26cb9d6d + local merge of v1.0-dev + a CI compile fix, not yet pushed): still valid, not addressed.
finish_shutdown_after_tasks (src/app.rs) still routes forget_all_secrets() exclusively through finish_wallet_shutdown, which only runs on TaskShutdownOutcome::Complete. The BackendTasksTimedOut and None (TaskManagerFailed) branches return early with no secret clearing — confirmed by the backend_task_timeout_skips_wallet_teardown test itself, which asserts teardown_calls == 0 for 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 on Complete) 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.
Fixed in 7603be6c.
finish_shutdown_after_tasks now calls forget_all_secrets() on every collected wallet backend unconditionally, before branching on the outcome — only the full coordinator backend.shutdown() join stays gated on Complete. Applies to both start_async_shutdown and run_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.
github-actions
Bot
removed
the
claudius-review
Keep join-error callbacks and MCP network-switch lifecycle work inside the bounded shutdown barrier. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/mcp/server.rs`:
- Around line 11-31: Update shutdown_app_context_wallet_backend so the
BackendTasksTimedOut and Err(_) branches synchronously clear wallet secrets via
the backend’s existing forget-all-secrets operation before logging and skipping
the potentially blocking backend.shutdown() call; retain the Complete branch’s
normal async teardown.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 968cd1fa-7dc4-4783-b005-987901b457db
📒 Files selected for processing (13)
src/app.rssrc/backend_task/error.rssrc/backend_task/mod.rssrc/bin/det_cli/headless.rssrc/context/wallet_lifecycle/bootstrap.rssrc/context/wallet_lifecycle/registration.rssrc/context/wallet_lifecycle/removal.rssrc/context/wallet_lifecycle/spv.rssrc/context/wallet_lifecycle/unlock.rssrc/mcp/dispatch.rssrc/mcp/server.rssrc/mcp/tools/network.rssrc/utils/tasks.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/tasks.rs
- src/app.rs
The base-branch merge and this PR's NetworkContextRegistered handler both landed changes to AppState without a textual conflict, but the handler referenced a `self.secret_prompt_host` field that never existed — the Arc<dyn SecretPrompt> built in AppState::new() was only installed on the initial network contexts and then dropped. This broke compilation (E0609) for both the Test Suite and Clippy CI jobs. Store the host as an AppState field so contexts registered later (via an in-flight network switch surviving into NetworkContextRegistered) can also have the secret prompt installed on them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Merged
Also ran a comment-verification pass (fresh fetch, re-checked against current
Both are pre-existing findings, not regressions from this push — flagging for a follow-up rather than blocking on them here. 🤖 Co-authored by Claudius the Magnificent AI Agent |
Two independent reviewers (CodeRabbit and a Claude Code automated review) found that finish_shutdown_after_tasks (src/app.rs) and shutdown_app_context_wallet_backend (src/mcp/server.rs) only cleared session-cached wallet secrets on the Complete outcome. On BackendTasksTimedOut or a failed task manager, the wallet-backend Arc<AppContext> set was collected and then dropped unpolled, so forget_all_secrets() never ran — contradicting this PR's own CHANGELOG entry promising secrets are cleared on close. Split the two concerns: forget_all_secrets() now runs unconditionally on every collected wallet backend regardless of outcome (cheap, synchronous, no dependency on any task finishing), while the full coordinator shutdown() join stays gated on Complete only, since backend tasks may still be using those coordinators on a degraded outcome. Applies to both the async and blocking-fallback GUI shutdown paths, and to the standalone MCP shutdown helper. Adds regression tests proving secrets are forgotten with zero coordinator-shutdown calls on both degraded outcomes, in both src/app.rs::shutdown_tests and the new src/mcp/server.rs::tests. Co-Authored-By: Codex Sol <noreply@openai.com>
|
Implemented the fix for both remaining open findings (degraded-shutdown outcomes skipping
Verified independently (isolated build/target-dir, not reused from the implementing pass): Replied on both threads; left them for the original reviewers/human to resolve. 🤖 Co-authored by Claudius the Magnificent AI Agent |
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)
CHANGELOG.md (1)
365-369: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse “afterward” for American English.
Change “afterwards” on Line 369 to “afterward.”
🤖 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 `@CHANGELOG.md` around lines 365 - 369, Update the changelog wording in the onboarding entry to use the American English spelling “afterward” instead of “afterwards,” without changing the surrounding content.Source: Linters/SAST tools
🤖 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 `@CHANGELOG.md`:
- Around line 365-369: Update the changelog wording in the onboarding entry to
use the American English spelling “afterward” instead of “afterwards,” without
changing the surrounding content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4afe8960-3079-422b-8ea5-f6736745b6bb
📒 Files selected for processing (6)
CHANGELOG.mdsrc/app.rssrc/backend_task/error.rssrc/backend_task/mod.rssrc/mcp/server.rssrc/mcp/tools/network.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/backend_task/error.rs
- src/mcp/tools/network.rs
- src/backend_task/mod.rs
- src/app.rs
|
Ran a fresh Verdict: no blockers. 6 non-blocking findings survive dedup (1 MEDIUM, 4 LOW, 1 INFO) — polish and hardening on an otherwise sound fix, not regressions:
Full report: Treating these as optional follow-up rather than blocking this PR further — happy to implement any of them on request. 🤖 Co-authored by Claudius the Magnificent AI Agent |
Brings in: shutdown fix (dashpay#905), duplicate-DPNS-name error message (dashpay#915), startup banner clearing (dashpay#916), nav pointer cursor + tooltips and wallet-less masternode indication (dashpay#917), onboarding disconnected- banner suppression (dashpay#907), masternode dialog/nav/passphrase fixes (dashpay#913), DAPI auto-refresh during pre-1.0 migration (dashpay#908), "Add Receiving Address" wiring + its test hardening (dashpay#914, dashpay#920), and a CI timeout bump (dashpay#912). dashpay#906 (shielded re-enable) was already pulled in individually last session, so its squashed commit merged as a no-op. Conflicts (6 files) were rebrand-naming overlaps (dash_evo_tool:: vs orchardpay:: imports) plus one real merge in left_panel.rs, where OrchardPay's green-icon tint had to combine with upstream's new nav tooltip. Also fixed 5 files upstream's auto-merged (non-conflicting) additions left un-rebranded: a stray DASH_EVO_DATA_DIR_LOCK/env-var name in a new app.rs test, and dash_evo_tool:: references in three kittest test files. Added tooltip strings for OrchardPay's own nav entries (OrchardPay, DashPay) so the new every_nav_entry_has_a_tooltip test covers them — upstream's version only knows its own nav items. Fixed the new nav_label_hover_shows_pointer_cursor kittest test: OrchardPay's nav rail carries two more always-visible entries than upstream's, pushing "Settings" below the scrollable list's default-size visible viewport; scroll it into view first, matching what a real user would do. Verified: cargo check (both feature modes), cargo clippy --all-features --all-targets -- -D warnings, cargo fmt --all, cargo test --all-features --workspace (2066 lib + 257 kittest + doc tests, 0 failed), all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Why this PR exists
WARN dash_evo_tool::app: Shutdown channel closed unexpectedly (possible panic)even though nothing panicked — confusing when triaging a real shutdown issue. More substantively, the wallet backend's SPV client/coordinators are never explicitly asked to shut down through the app's own shutdown path, relying instead on whatever happens at process death rather than a deterministic teardown.What was done
src/app.rs: GUI close now awaitsWalletBackend::shutdown()for every initialized network context (including one that finishes initializing mid-shutdown via an in-flight network switch) before the viewport actually closes, bounded by the existing shutdown timeout. Theshutdown_receiveroneshot is consumed exactly once (cleared once resolved) so a later frame can't re-poll an already-drained channel and log a false "possible panic" warning.src/backend_task/mod.rs: closes a related race where an in-flight network-switch task could create/publish a new backend context after shutdown had already begun.shutdown_startedlatch is set on the very first shutdown attempt (graceful or force-close) and never cleared, soon_exit()'s force-close guard blocks fallback re-entry on every branch — clean, panicked, or deadline-exceeded — not only after a clean finish. This prevents a second close signal from racing wallet-backend teardown against a still-running first attempt.forget_all_secrets()is now also called again after the bounded wallet-shutdown wait resolves, including the timeout path, so a backend task that re-caches a secret mid-wait is still cleared before exit.SwitchNetworknow registers its wallet-backend context before its slow network call resolves, racing that call against the shutdown cancellation token — an in-flight network switch is discoverable to the shutdown teardown pass instead of being invisible to it.TaskManager::shutdown(), pruned completed blocking-task tracking entries instead of letting them grow unbounded for the app session's lifetime, and centralized the shutdown budget constant instead of duplicating its arithmetic.platform-walletcoordinator-thread join race (discardedJoinHandles,quiesce()not waiting for actual thread exit) is tracked separately at feat(platform-wallet)!: shared ThreadRegistry for coordinator lifecycle + shutdown UAF/data-loss fixes platform#3954 — this PR makes DET correctly ask the wallet backend to shut down with a bounded wait, but can't fully close that upstream race until the fix there lands.Testing
cargo fmt --all -- --check— cleancargo clippy --all-features --all-targets -- -D warnings— cleancargo test app:: --all-features— 35 passed, 0 failedcargo test backend_task:: --all-features— 374 passed, 0 failedcargo test utils::tasks:: --all-features— 5 passed, 0 failed3b32bed2, the commitv1.0.0-weekly.20260717was built from): closed via a real ICCCMWM_DELETE_WINDOWrequest (not a forced X11 destroy) with a fresh isolated data directory. Confirmed via logs that both the crash-safety and the log-line regression are resolved — no panics, and the shutdown sequence now shows SPV/wallet-backend components tearing down in order (SyncCoordinator→ per-task shutdown → peer network manager → storage flush) before the viewport closes.Breaking changes
None.
Checklist
-D warnings)cargo fmt --all)second_exit_after_async_attempt_does_not_repeat_wallet_teardown,wallet_shutdown_clears_secrets_again_after_teardown_timeout,switch_network_registers_wired_backend_before_cancellation_teardown)Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit