fix(platform-wallet): free SPV data dir on stop#3811
Conversation
📝 WalkthroughWalkthroughSpvRuntime now stores the background task JoinHandle in a mutex-protected field, records it when spawning, and on stop() awaits the client stop then waits up to 15 seconds for the background task before aborting and logging a warning. ChangesGraceful Task Shutdown
Sequence DiagramsequenceDiagram
participant Caller
participant SpvRuntime
participant DashSpvClient
participant BackgroundTask
participant Tokio
Caller->>SpvRuntime: stop()
SpvRuntime->>SpvRuntime: take self.client
SpvRuntime->>DashSpvClient: stop()
DashSpvClient-->>SpvRuntime: (complete)
SpvRuntime->>SpvRuntime: lock and take self.task
SpvRuntime->>Tokio: timeout(Duration::from_secs(15), join task)
Tokio->>BackgroundTask: wait for completion
alt Task completes within timeout
BackgroundTask-->>Tokio: (finished)
Tokio-->>SpvRuntime: Ok(Ok(()))
else Task timeout
Tokio->>BackgroundTask: (still running)
Tokio-->>SpvRuntime: Err(Elapsed)
SpvRuntime->>BackgroundTask: abort()
SpvRuntime->>SpvRuntime: log warning
end
SpvRuntime-->>Caller: (return)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
✅ Review complete (commit 13a1d2e) |
![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
🤖 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 `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 195-205: spawn_in_background currently overwrites self.task's
JoinHandle and can orphan a running task; change spawn_in_background
(SpvRuntime::spawn_in_background) to first lock self.task and check if there is
an existing Some(handle) that is still running (not finished) and if so return
early or log and avoid spawning a new task, or alternatively abort the existing
handle before replacing it; use the existing task mutex and JoinHandle methods
to detect finished/aborted state and only set the new handle when safe so stop()
will always await or abort the actual background task.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aef7f4b0-041f-4538-ab90-1cfb9c4fe971
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/spv/runtime.rs
1330eb2 to
59a284e
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR correctly tracks the spawned SPV task so stop() can wait for the data-dir lock to be released, but two real bugs undermine that guarantee: spawn_in_background unconditionally overwrites self.task (orphaning a still-running prior task), and stop() racing an in-flight spawn_in_background can take None from self.client before run()'s start() assigns it — leaving DiskStorageManager owned after stop() returns Ok(()). Five agents (incl. CodeRabbit) converged on the overwrite bug; one (codex-rust-quality) caught the start-race.
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:195-205: spawn_in_background silently overwrites a live JoinHandle, defeating the stop()/data-dir-release guarantee
`spawn_in_background` unconditionally writes the new handle into `self.task`. If it is called twice without an intervening successful `stop()`, the previous `JoinHandle` is dropped — in Tokio that detaches the task, which keeps running, retains its `SpvClient` clone, and continues to hold the SPV data-dir lock. The second task itself usually exits fast (`start()` returns `SpvAlreadyRunning` because `self.client.is_some()`), so a subsequent `stop()` awaits the already-completed duplicate handle and returns `Ok(())` while the *original* task is still running and still holding the lock — re-introducing the exact failure mode this PR set out to fix. Guard against this by checking the existing slot before replacing it: if it holds an unfinished handle, log/return without spawning (or, alternatively, abort+await the prior handle first).
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:160-190: stop() racing in-flight spawn_in_background can leave self.client populated and the data dir locked
`stop()` snapshots `self.client` once at line 162-163. But `spawn_in_background` -> `run()` -> `start()` only assigns `self.client = Some(spv_client)` after asynchronously constructing `PeerNetworkManager` and `DiskStorageManager` (lines 59-84). If `stop()` is called shortly after `spawn_in_background`, the take at 163 returns `None`, `stop_result` is `Ok(())`, and we then wait on the JoinHandle while the background task happily finishes `start()` and enters `client.run().await`. After the 15s timeout `abort.abort()` cancels the task at its next await — but `self.client` has already been populated with the live `DashSpvClient` (and its `DiskStorageManager` lock), and nothing in `stop()` ever calls `DashSpvClient::stop()` on it. `stop()` returns `Ok(())` while the storage dir remains locked, which is precisely the invariant this PR claims to establish. Either acquire `self.task` before checking `self.client` and serialize start vs stop through a single state-machine lock, or after the join/abort path re-check `self.client.take()` and call `stop()` on whatever was assigned during the race.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:174-189: stop() returns Ok even when the background task had to be force-aborted
When the 15-second join times out, `abort.abort()` cancels the task — but `stop_result` (computed earlier from the inner `client.stop()` signal) is returned unchanged, so callers can't distinguish a clean shutdown from a forced cancellation. This matters because Tokio's `abort` cancels at the next await point: file handles and RocksDB drops may still be in flight, and a `start()` called immediately after may still hit a locked data dir. Surface the timeout as a distinct error variant (e.g. `PlatformWalletError::SpvStopTimedOut`) so callers know not to assume resources are fully released.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #3811 adds a guard in spawn_in_background to refuse spawning when self.task already contains a handle, partially addressing prior finding #1. However, all three prior findings remain valid at the current head: the guard is not atomic across check/spawn/store, the stop()/start() race still leaks the data directory, and stop() still reports Ok after force-aborting. The latest delta also introduces a new failure mode: a naturally-exited background task leaves a stale handle in self.task that permanently blocks future spawn_in_background calls. The PR's stated goal — freeing the SPV data directory on stop — is not achieved in the cases the carried-forward findings describe.
🔴 3 blocking | 🟡 1 suggestion(s)
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:195-215: [carried-forward #1: STILL VALID] spawn_in_background guard is not atomic and ignores finished handles
The new guard at lines 196-204 addresses the simple serial double-start case but has two residual defects that defeat the data-dir-release guarantee:
1. TOCTOU window: the task mutex is locked at 197, released at 204, then re-acquired at 214 to store the handle. Two concurrent callers can both observe `None` during the early check, both proceed to tokio::spawn, and the second store at 214 overwrites the first JoinHandle. In Tokio, dropping a JoinHandle detaches the task — the first SPV task continues running, retains its SpvClient clone, and keeps the data directory locked outside the runtime's tracking.
2. Finished-handle blindness: `is_some()` does not distinguish a live handle from a finished one. `run()` at lines 153-154 only clears `self.client`, never `self.task`; the wrapper at lines 208-212 also doesn't clear the task slot when the closure exits. So if start() fails or client.run() returns on its own, the stale JoinHandle remains in `self.task` forever, and every subsequent `spawn_in_background` call silently no-ops at the new guard. The runtime becomes unrestartable without an intervening `stop()` whose only purpose is to reap the dead handle.
The fix needs to hold the task mutex across the check, spawn, and store as one atomic critical section, and treat `handle.is_finished()` as 'no task running'. Alternatively the spawned closure should clear `self.task` on exit.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:160-190: [carried-forward #2: STILL VALID] stop() racing in-flight start() leaves self.client populated and the data dir locked
Unchanged by the latest delta. `stop()` snapshots `self.client` once at lines 161-164 and proceeds to wait on the task handle. But `start()` (called by `run()` on the spawned task) only publishes `self.client = Some(spv_client)` at lines 83-84 after asynchronously constructing PeerNetworkManager and DiskStorageManager. If `stop()` runs while `start()` is still mid-construction:
1. stop() takes `None` from self.client; stop_result = Ok(()).
2. The spawned task finishes start(), writes `self.client = Some(spv_client)`, enters `client.run().await`.
3. Nothing has signaled the freshly-published client to stop, so the run loop continues.
4. The 15s join at line 177 times out; abort.abort() at line 185 cancels the task at its current await point — somewhere inside client.run(), before the cleanup at lines 153-154.
5. self.client remains `Some(spv_client)`; the storage manager / data directory lock is leaked. stop() returns Ok(()).
This is exactly the scenario the PR title ("free SPV data dir on stop") aims to fix. The fix requires either serializing start vs. stop under a single lifecycle lock, or re-reading `self.client` after the join completes and running `client.stop()` on any client that was published mid-stop.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:174-189: [carried-forward #3: STILL VALID] stop() returns Ok even after force-aborting the background task
Unchanged by the latest delta. `stop_result` is computed at lines 166-172 from the taken-client.stop() result (or Ok(()) when no client was present). When the 15s join at lines 177-179 times out, `abort.abort()` at line 185 force-cancels the task, but line 189 returns the original `stop_result` regardless. Callers cannot distinguish a clean unwind from a forced cancellation that may have skipped storage flushes, FFI destructors, or data-dir release. Given the data-dir-locking regression this PR is trying to defend against, a forced abort is precisely the case where callers most need an error signal so they refuse to immediately re-start the runtime. Return a distinct error variant (or at least an Err) on the timeout/abort path.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:196-215: [new latest-delta] Completed background handles permanently block restart
The new guard rejects any `Some(JoinHandle)` regardless of whether the underlying task is still running. Nothing clears `self.task` except `stop()` at line 174: `run()` at lines 153-154 only clears `self.client`, and the spawned closure at 208-212 does not touch `self.task` when it exits. So when the background task exits on its own — e.g., `start(config)` fails to open the SPV storage directory (so the wrapper logs the error and the closure returns), or `client.run()` returns Err normally — `self.task` keeps the dead JoinHandle and `self.client` is `None`.
From that point, every subsequent `spawn_in_background()` call hits the new guard at 198, logs a misleading 'task is already running' warning, and returns without doing anything. The FFI start path returns success immediately after spawning, so callers have no signal that SPV is wedged. The only recovery is to call `stop()` purely to reap the stale handle — even though there is nothing to stop. Either check `handle.is_finished()` in the guard, or have the spawned closure clear `self.task` as its tail.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "eb444c43ce4e3c0d8d7b6bd8a9fe15a119f3e7c5cea19251ad9a5349f9e5a34d"
)Xcode manual integration:
|
59a284e to
a4f1c63
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta from 59a284e to a4f1c63 contains no changes to packages/rs-platform-wallet/src/spv/runtime.rs — the file is identical. All four prior findings re-validate against current head as STILL_VALID. No new latest-delta findings. Three blocking concurrency/lifecycle defects (TOCTOU in spawn_in_background, stop/start race, stale-handle restart deadlock) and one suggestion (stop hides force-abort outcome) carry forward.
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:195-215: spawn_in_background guard has a TOCTOU window and never clears finished handles
The task-tracking guard is not atomic with the spawn-and-store. The std::sync::Mutex is acquired in an inner block (lines 197-204), released, the task is spawned (line 208), and the JoinHandle is then stored under a fresh lock acquisition (line 214). Two callers reaching spawn_in_background concurrently can both observe existing.is_none(), both proceed past the guard, and both spawn an SPV run loop; the later store at line 214 silently overwrites the first JoinHandle, leaking that task while two SPV clients fight over the same data directory — the exact resource-locking class the PR is meant to fix. Compounding this, the guard rejects any Some(JoinHandle) without consulting handle.is_finished(). Neither the spawned closure (lines 208-212) nor run() (lines 148-156, which only clears self.client) ever clears self.task. Once the background run exits on its own (SPV error, peer collapse, internal completion), self.task remains Some(<finished handle>) and every subsequent spawn_in_background call short-circuits with a warning forever. Close this by holding the std::sync::Mutex across the entire check/spawn/store sequence (JoinHandle construction is sync) and treating is_finished() handles as absent, or clearing self.task at the tail of the spawned closure.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:160-190: stop() races in-flight start() and can leave the SPV data dir locked
stop() takes a single snapshot of self.client at lines 161-164 and then performs cleanup based solely on that snapshot. start() (lines 51-87) checks self.client at lines 52-57, releases the read guard, then performs asynchronous PeerNetworkManager::new, DiskStorageManager::new, and DashSpvClient::new (lines 59-81) before finally publishing the client at lines 83-84. If stop() acquires the client write lock during that async construction window, it sees None, returns Ok from the client branch, then waits or aborts the background task. start() afterward publishes the freshly-constructed SpvClient — whose DiskStorageManager still holds the data-directory lock — and run() proceeds into client.run(). On the 15s abort path the future is dropped before run()'s cleanup at lines 153-154 can execute, so self.client stays Some with a live client and the data dir remains locked. This is precisely the failure mode this PR claims to fix, but only the simple serial case is closed. Closing the race requires encoding the lifecycle as a single state machine (e.g. Idle/Starting/Running/Stopping) that both start() and stop() observe under one lock, or holding the client write lock across the entire start() construction.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:174-189: stop() returns the pre-abort stop_result even after forced cancellation
stop_result is bound at lines 166-172 from the synchronous c.stop() outcome (or Ok when no client was present). The 15s join at lines 177-186 may time out and trigger abort.abort() at line 185, but stop() still returns stop_result unchanged at line 189. Callers cannot distinguish a clean shutdown from a 15-second forced cancellation, even though the latter likely leaves in-flight I/O or held resources behind (see the related stop/start race above). Surface the timeout as a typed PlatformWalletError variant (e.g. SpvStopTimedOut) so callers that need to retry start() — the very behavior this PR enables — can branch on the failure mode.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a4f1c63 to
13a1d2e
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validated all three prior findings against current head 13a1d2e; packages/rs-platform-wallet/src/spv/runtime.rs is byte-identical to prior head a4f1c63 (the PR commit modifies only this one file). No new latest-delta findings. Carried-forward findings: two blocking lifecycle races (TOCTOU in spawn_in_background; stop()/start() race that can still leave the SPV data dir locked, defeating the PR's stated goal) and one suggestion (stop() returns Ok even after forced cancellation). The codex-rust-quality FFI stop-bridge finding describes pre-existing code in packages/rs-platform-wallet-ffi/src/spv.rs that this PR does not touch; recorded as an out-of-scope follow-up rather than a PR finding. No CodeRabbit inline findings required reaction.
🔴 1 blocking | 🟡 1 suggestion(s)
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 `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:195-215: spawn_in_background guard is non-atomic and never clears finished JoinHandles
The duplicate-task guard is split across two separate std::sync::Mutex acquisitions: the inner scope at lines 197-204 checks `existing.is_some()` and releases the lock, then `tokio::spawn` runs at line 208 with no lock held, and the JoinHandle is stored under a fresh `lock()` call at line 214. Two concurrent callers (e.g. FFI plus UI) can both observe `None`, both spawn `run()`, and the later assignment at 214 silently overwrites the earlier handle. The detached task continues to run `client.run().await`, its DiskStorageManager continues to hold the SPV data-dir lockfile, and stop() can no longer await or abort it — which is exactly the failure mode this PR is trying to fix.
The same guard also treats any `Some(handle)` as 'still running' without calling `handle.is_finished()`. When the background `run()` returns naturally (e.g. it logged 'exited with error' at line 210), `self.task` still holds `Some(JoinHandle)`. Every subsequent `spawn_in_background` call then warns and refuses to start, even though nothing is actually running, so the caller cannot restart SPV without dropping the whole SpvRuntime.
Fix shape: hold a single mutex (or a unified lifecycle state) across the check, the spawn, and the store; and before treating `Some(handle)` as busy, drop it if `handle.is_finished()` (or have the spawned closure clear `self.task` on exit).
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:174-189: stop() returns Ok even after forced cancellation, hiding incomplete shutdown
stop_result is computed at lines 166-172 from the snapshot taken before the JoinHandle await. When the 15s timeout fires at line 177 and `abort.abort()` is called at 185, line 189 returns the original stop_result unchanged. In the common case where the client-stop call succeeded (or no client was present at snapshot time), callers get Ok(()) even though the background run task did not unwind cleanly and the post-loop `client.take()` at line 154 was skipped — meaning the data-dir lock may still be held by a client sitting in self.client.
Because this PR is specifically about 'free SPV data dir on stop', the return value should distinguish a clean stop from a forced abort. Either introduce a `PlatformWalletError::SpvStopTimedOut` variant returned from the timeout branch, or downgrade Ok to Err in that branch, so callers (and the FFI bridge) can decide whether a follow-up start is safe.
| pub fn spawn_in_background(self: &Arc<Self>, config: ClientConfig) { | ||
| { | ||
| let existing = self.task.lock().expect("spv task mutex poisoned"); | ||
| if existing.is_some() { | ||
| tracing::warn!( | ||
| "spawn_in_background called while a task is already running; ignoring" | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let this = Arc::clone(self); | ||
| tokio::spawn(async move { | ||
|
|
||
| let handle = tokio::spawn(async move { | ||
| if let Err(e) = this.run(config).await { | ||
| tracing::warn!("SpvRuntime background run exited with error: {}", e); | ||
| } | ||
| }); | ||
|
|
||
| *self.task.lock().expect("spv task mutex poisoned") = Some(handle); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: spawn_in_background guard is non-atomic and never clears finished JoinHandles
The duplicate-task guard is split across two separate std::sync::Mutex acquisitions: the inner scope at lines 197-204 checks existing.is_some() and releases the lock, then tokio::spawn runs at line 208 with no lock held, and the JoinHandle is stored under a fresh lock() call at line 214. Two concurrent callers (e.g. FFI plus UI) can both observe None, both spawn run(), and the later assignment at 214 silently overwrites the earlier handle. The detached task continues to run client.run().await, its DiskStorageManager continues to hold the SPV data-dir lockfile, and stop() can no longer await or abort it — which is exactly the failure mode this PR is trying to fix.
The same guard also treats any Some(handle) as 'still running' without calling handle.is_finished(). When the background run() returns naturally (e.g. it logged 'exited with error' at line 210), self.task still holds Some(JoinHandle). Every subsequent spawn_in_background call then warns and refuses to start, even though nothing is actually running, so the caller cannot restart SPV without dropping the whole SpvRuntime.
Fix shape: hold a single mutex (or a unified lifecycle state) across the check, the spawn, and the store; and before treating Some(handle) as busy, drop it if handle.is_finished() (or have the spawned closure clear self.task on exit).
source: ['claude', 'codex']
| let handle = self.task.lock().expect("spv task mutex poisoned").take(); | ||
| if let Some(handle) = handle { | ||
| let abort = handle.abort_handle(); | ||
| if tokio::time::timeout(std::time::Duration::from_secs(15), handle) | ||
| .await | ||
| .map_err(|e| PlatformWalletError::SpvError(e.to_string()))?; | ||
| .is_err() | ||
| { | ||
| tracing::warn!( | ||
| "SPV stop: background run loop did not unwind within 15s; aborting it" | ||
| ); | ||
|
|
||
| abort.abort(); | ||
| } | ||
| } | ||
| Ok(()) | ||
|
|
||
| stop_result |
There was a problem hiding this comment.
🟡 Suggestion: stop() returns Ok even after forced cancellation, hiding incomplete shutdown
stop_result is computed at lines 166-172 from the snapshot taken before the JoinHandle await. When the 15s timeout fires at line 177 and abort.abort() is called at 185, line 189 returns the original stop_result unchanged. In the common case where the client-stop call succeeded (or no client was present at snapshot time), callers get Ok(()) even though the background run task did not unwind cleanly and the post-loop client.take() at line 154 was skipped — meaning the data-dir lock may still be held by a client sitting in self.client.
Because this PR is specifically about 'free SPV data dir on stop', the return value should distinguish a clean stop from a forced abort. Either introduce a PlatformWalletError::SpvStopTimedOut variant returned from the timeout branch, or downgrade Ok to Err in that branch, so callers (and the FFI bridge) can decide whether a follow-up start is safe.
source: ['claude', 'codex']
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3811 +/- ##
============================================
+ Coverage 70.73% 71.20% +0.47%
============================================
Files 20 20
Lines 2788 2837 +49
============================================
+ Hits 1972 2020 +48
- Misses 816 817 +1
🚀 New features to boost your workflow:
|
3 participants
The spv client wrapper stop method wasn't waiting for the spv task to stop, making run calls made right after it fail due to a locked storage dir. With this PR the stop method waits for the run method to finish by waiting for the JoinHandle
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit