Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions packages/rs-platform-wallet/src/spv/runtime.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
//! SPV client runtime — manages the DashSpvClient lifecycle.

use std::sync::Arc;
use std::sync::{Arc, Mutex};

use tokio::sync::RwLock;
use tokio::task::JoinHandle;

use dashcore::sml::llmq_type::LLMQType;
use dashcore::{QuorumHash, Transaction};
Expand All @@ -29,6 +30,7 @@ pub struct SpvRuntime {
event_manager: Arc<PlatformEventManager>,
wallet_manager: Arc<RwLock<WalletManager<PlatformWalletInfo>>>,
client: RwLock<Option<SpvClient>>,
task: Mutex<Option<JoinHandle<()>>>,
}
// TODO: We want it better
impl SpvRuntime {
Expand All @@ -41,6 +43,7 @@ impl SpvRuntime {
event_manager,
wallet_manager,
client: RwLock::new(None),
task: Mutex::new(None),
}
}

Expand Down Expand Up @@ -153,27 +156,62 @@ impl SpvRuntime {
result
}

/// Stop SPV sync gracefully.
/// Stop SPV sync gracefully. Unlocks the data dir safely
pub async fn stop(&self) -> Result<(), PlatformWalletError> {
let mut client = self.client.write().await;
if let Some(c) = client.take() {
c.stop()
let taken = {
let mut client = self.client.write().await;
client.take()
};

let stop_result = match taken {
Some(c) => c
.stop()
.await
.map_err(|e| PlatformWalletError::SpvError(e.to_string())),
None => Ok(()),
};

let handle = self.task.lock().expect("spv task mutex poisoned").take();
Comment thread
ZocoLini marked this conversation as resolved.
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
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment on lines +174 to +189

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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']

}
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.

/// Spawn `run()` on the current tokio runtime and return immediately.
///
/// Call [`stop`] to stop it
pub fn spawn_in_background(self: &Arc<Self>, config: ClientConfig) {
{
let existing = self.task.lock().expect("spv task mutex poisoned");
if existing.is_some() {
Comment thread
ZocoLini marked this conversation as resolved.
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);
}
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment thread
ZocoLini marked this conversation as resolved.
Comment on lines 195 to 215

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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']


/// Get the current sync progress.
Expand Down
Loading