fix(platform-wallet): retain SPV client and track shutdown through cancellation

[lklimek]

Basic explanation

A successful SPV startup currently makes Platform lose access to the client even while synchronization continues. This fix keeps the client available and makes shutdown wait for the actual SPV work, including when an earlier stop timed out or its caller cancelled the wait.

Issue being fixed or feature implemented

Addresses the SPV lifecycle review blocker on #5307. Stacked on #5307; the diff contains only this lifecycle fix and its tests.

The migrated rust-dashcore run() returns after starting internal background tasks. Platform previously treated that return as termination and cleared the retained client. Later stop() could report success without stopping upstream synchronization. Cancelling a timed-out upstream stop() could also detach its task ownership.

What was done?

• Retain the client after successful startup, preserving queries, broadcasts and peer state.
• Run teardown in an owned task which waits for startup and then stops the upstream client. A 15-second wait timeout or cancellation of the caller leaves that same teardown tracked for retry; it is never aborted.
• Block restart until shutdown completion has been joined, serialize construction and stop, and preserve the existing already-running error.
• Keep panic/join failures non-clean on every retry.
• Add offline lifecycle regressions and a dev-only tempfile dependency for isolated storage fixtures. No upstream revision, rand, Swift or FFI ABI changes.

Existing waits for client read guards and construction serialization precede the teardown deadline and remain unbounded. A panic/join failure requires replacing the runtime; subsequent stops do not claim a clean shutdown.

How Has This Been Tested?

• Confirmed two regression failures before implementation: successful-startup client retention and shutdown tracking after caller cancellation.
• cargo test -p platform-wallet --lib spv:: --locked -- --nocapture: 27 passed, covering client/query/broadcast retention, timeout/retry, cancelled callers, concurrent stops, stop-during-startup ordering, failure cleanup, restart and panic handling.
• cargo clippy -p platform-wallet --all-targets --all-features --locked -- --no-deps -D warnings: passed.
• cargo fmt -p platform-wallet -- --check and whitespace checks: passed.

Tests construct real clients with private temporary storage and inject startup completion/stop futures. No live peers, upstream network event-loop run, mobile builds or full workspace suite were exercised.

Breaking Changes

No public API or ABI changes. Restart requires a completed, successfully joined stop; incomplete or panicked teardown cannot be bypassed.

Checklist

• I have performed a self-review of my own code
• I have added relevant regression tests
• I have documented the lifecycle and timeout behavior
• Formatting, targeted tests and Clippy passed

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Repository: dashpay/platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: cd924b39-5151-4917-b74e-120ecfe89785

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Final review complete — no blockers (commit 64d5b8a) · triage: normal

[lklimek]

/self-reviewed

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The lifecycle implementation retains the client and preserves tracked teardown across timeout and cancellation; no in-scope blocking defect was confirmed. Four non-blocking suggestions address stale shutdown documentation, skipped-startup diagnostics, and lifecycle test coverage and reliability. Validation was static only: the supplied CI snapshot shows Kotlin and metadata checks passing and one policy check queued, but does not independently verify the author-reported targeted Rust tests.

🟡 4 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Update callers' shutdown documentation for tracked, never-aborted teardown
packages/rs-platform-wallet/src/manager/mod.rs:911-918

These docs still describe an abort-escalating join, and the WalletWorker::Spv docs at lines 62–67 and shutdown's inline comment at lines 930–933 repeat that contract. The FFI stop comment in packages/rs-platform-wallet-ffi/src/spv.rs:589–592 also describes the removed client-stop budget and abort/re-park path. The new implementation instead tracks an owned teardown task that is never aborted: the 15-second timeout bounds its join wait after lock acquisition, while startup and upstream client.stop() can continue beyond that deadline. Restart is refused until a later stop successfully joins teardown; panic/join failures remain non-clean and require replacing the runtime. Update these comments together, distinguishing the bounded join wait from the unbounded lock waits and teardown work, rather than promising that every shutdown phase is bounded.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)}

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The intricate SPV lifecycle changes in runtime.rs and peers.rs affect asynchronous ownership, cancellation and shutdown coordination, but do not themselves change any of the specified critical surfaces.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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 `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:1006-1008: Bound synchronization waits in the lifecycle tests
  This polling loop has no deadline, and the new waits at lines 1128–1130 and 1179–1181 have the same problem. If stop fails to reach the expected ownership transition, yield_now keeps the test runnable indefinitely instead of letting it reach an assertion or produce a useful failure. The existing equivalent waits at lines 852–863 and 901–912 already use five-second timeouts. Apply that pattern to all three new synchronization waits so a lifecycle regression fails promptly rather than hanging the test suite.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:1144-1145: Exercise failed-startup cleanup with an owned startup handle
  Calling finish_startup directly leaves runtime.task empty, so this test can restart immediately without exercising the ownership state established by spawn_run_loop. In production, failed-startup cleanup runs inside that task, and its completed JoinHandle remains in the slot until stop joins it; ensure_joined therefore rejects an immediate restart. The separate completed-startup test checks joining in isolation, but this test does not cover its interaction with failed-startup cleanup. Execute the injected failure through an owned startup task and assert that restart is rejected before stop, stop joins cleanup successfully, and restart then succeeds, while keeping the test offline.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:378-384: Restore diagnostics when startup dispatch is skipped
  Both lifecycle guards now return silently, and spawn_run_loop returns no status. The FFI start caller invokes it after successful construction and returns only start_result, so callers receive no indication that background startup dispatch was skipped. Refusing to spawn during teardown or over an existing task is intentional and correct, but removing the previous warning also removes the diagnostic needed to distinguish that refusal from dispatched startup. Add warnings identifying lock contention and incomplete lifecycle state; this preserves the ownership guards without claiming that every skipped spawn is a synchronization defect.

In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:911-918: Update callers' shutdown documentation for tracked, never-aborted teardown
  These docs still describe an abort-escalating join, and the WalletWorker::Spv docs at lines 62–67 and shutdown's inline comment at lines 930–933 repeat that contract. The FFI stop comment in packages/rs-platform-wallet-ffi/src/spv.rs:589–592 also describes the removed client-stop budget and abort/re-park path. The new implementation instead tracks an owned teardown task that is never aborted: the 15-second timeout bounds its join wait after lock acquisition, while startup and upstream client.stop() can continue beyond that deadline. Restart is refused until a later stop successfully joins teardown; panic/join failures remain non-clean and require replacing the runtime. Update these comments together, distinguishing the bounded join wait from the unbounded lock waits and teardown work, rather than promising that every shutdown phase is bounded.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The retained-client and owned-teardown implementation addresses the main startup and stop paths, and all four prior findings are fixed. Live-client storage clearing still bypasses tracked teardown and leaves a stopped client marked running; the destroy documentation also retains an inaccurate bounded-shutdown claim. This static review verified both issues against the exact head and pinned upstream source; under the supplied non-consensus severity policy, both are suggestions, and the CI snapshot does not independently confirm the reported targeted Rust tests or Clippy run.

🟡 2 suggestion(s)

2 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Route live-client storage clearing through the tracked shutdown lifecycle
packages/rs-platform-wallet/src/spv/runtime.rs:610-615

Retaining the client after successful startup makes this branch reachable throughout a running session. At the pinned rust-dashcore revision 314f106, DashSpvClient::clear_storage() calls stop() before clearing storage, but this wrapper leaves self.client populated and the startup handle in self.task. After a successful clear, synchronization is stopped while is_started() reports true, start() returns SpvAlreadyRunning, and spawn_run_loop() refuses the unjoined handle. The direct upstream stop also bypasses cancellation-safe teardown: upstream takes its sync-loop ownership before awaiting shutdown, so cancelling this call can abandon that ownership without recording it in Platform's Shutdown state. Clear through a lifecycle-serialized operation that successfully joins tracked teardown before reopening the cached storage location, and cover running-client clear followed by restart and cancellation during clear.

_{source: gpt-6.1-sol (phase2-reviewer: general)}

🟡 Suggestion: Qualify destroy's bounded-shutdown claim for SPV lock waits
packages/rs-platform-wallet-ffi/src/manager.rs:714-716

This entry point waits for PlatformWalletManager::shutdown(), whose updated documentation explicitly excludes SPV lifecycle-lock and in-flight client-query waits from its bounds. SpvRuntime::stop() acquires both locks before starting the 15-second teardown timeout. The unqualified claim here therefore promises FFI hosts a bounded destroy call that the implementation does not provide. State the same exception here as in the manager shutdown documentation.

/// Runs the full lifecycle shutdown (quiesce + join every coordinator,
/// SPV, the payment-hook tasks, and the event adapter) and removes the
/// handle. Shutdown waits are bounded except for SPV's waits for a
/// concurrent start/stop and in-flight client queries. Always returns
/// `Success` for a live handle.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general)}

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The intricate SPV startup, cancellation, and shutdown lifecycle changes do not themselves alter consensus, funds movement, cryptography, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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 `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:610-615: Route live-client storage clearing through the tracked shutdown lifecycle
  Retaining the client after successful startup makes this branch reachable throughout a running session. At the pinned rust-dashcore revision 314f106, DashSpvClient::clear_storage() calls stop() before clearing storage, but this wrapper leaves self.client populated and the startup handle in self.task. After a successful clear, synchronization is stopped while is_started() reports true, start() returns SpvAlreadyRunning, and spawn_run_loop() refuses the unjoined handle. The direct upstream stop also bypasses cancellation-safe teardown: upstream takes its sync-loop ownership before awaiting shutdown, so cancelling this call can abandon that ownership without recording it in Platform's Shutdown state. Clear through a lifecycle-serialized operation that successfully joins tracked teardown before reopening the cached storage location, and cover running-client clear followed by restart and cancellation during clear.

In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/manager.rs:714-716: Qualify destroy's bounded-shutdown claim for SPV lock waits
  This entry point waits for PlatformWalletManager::shutdown(), whose updated documentation explicitly excludes SPV lifecycle-lock and in-flight client-query waits from its bounds. SpvRuntime::stop() acquires both locks before starting the 15-second teardown timeout. The unqualified claim here therefore promises FFI hosts a bounded destroy call that the implementation does not provide. State the same exception here as in the manager shutdown documentation.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The current diff retains the SPV client after successful startup and preserves teardown ownership across timeout, cancellation, and storage clearing; no blocking defects were confirmed. Five prior findings are fixed, while one non-blocking test-reliability concern remains. Validation was static only: the supplied CI snapshot shows successful Kotlin SDK build/tests, but the reported targeted Rust tests and Clippy checks were not independently rerun.

🟡 1 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The intricate shutdown and cancellation changes in packages/rs-platform-wallet/src/spv/runtime.rs warrant substantial review but do not change consensus, funds movement, cryptography, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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 `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:1123: Bound synchronization waits in the lifecycle tests
  (existing thread: https://github.com/dashpay/platform/pull/5311#discussion_r4197929934)
  The polling barriers now have deadlines, but this cancellation regression still waits indefinitely on `started.await`; the new clear-storage cancellation regression repeats this at line 1180. If lifecycle work stalls before invoking the injected stop closure, its captured oneshot sender remains alive, so the receiver neither receives the signal nor observes channel closure. The production deadline does not guarantee this test wait terminates: lock acquisition precedes that deadline, and a teardown timeout intentionally preserves the running task. Wrap both entry-signal receives in a five-second test-local timeout so regressions fail with a localized diagnostic. Leave the release receivers intentionally pending to preserve the stalled-work simulation.

[llbartekll]

Reviewed against the pinned dash-spv revision: DashSpvClient::run() indeed spawns its sync loop and returns, so treating that return as termination was the bug, and retaining the client is the right fix.

Verified locally (commit 64d5b8a): cargo test -p platform-wallet --lib spv:: → 31 passed; cargo clippy -p platform-wallet --all-targets -- -D warnings clean.

What I like

• Single tokio::sync::Mutex<Shutdown> replacing the atomic counter + serial mutex + abort-escalation machinery is a clear simplification, and the Idle / Running / Failed state makes the lifecycle much easier to reason about.
• Teardown runs in an owned task that is never aborted; timeout and caller cancellation both leave it in Running for the next stop to rejoin. There is no await point between taking the client/startup handle and storing the Running state, so a cancelled caller cannot strand ownership.
• start() fails fast via try_lock instead of queueing behind an in-flight stop, and still re-checks under the lock.
• Poison recovery on both task and the peer tracker mutex is the right call for code that runs inside dash-spv's event dispatch.
• Tests cover the real failure modes (timeout → retry, cancelled caller, concurrent stops, stop during startup, failed startup, panics) and run offline against temp storage. The three earlier bot suggestions (bounded test waits, owned-handle failed-startup test, spawn_run_loop warnings) are all addressed.

Non-blocking observations

• Shutdown::Failed is sticky: after a teardown panic or JoinError, every subsequent stop() errors and start() is refused until the SpvRuntime is replaced, i.e. the FFI manager is destroyed. The PR documents this and it is the conservative choice, but note it is a behavior change from the previous code, which logged a join error and allowed restart. Worth keeping in mind if we ever see panics in run() on mobile.
• start() holds the shutdown lock across PeerNetworkManager::new / DiskStorageManager::new / DashSpvClient::new, so a concurrent stop() (and therefore FFI destroy) waits unbounded for construction to finish. Documented in shutdown()'s doc comment, so fine, just flagging that this is the one unbounded wait on the destroy path.

LGTM.