diff --git a/Cargo.lock b/Cargo.lock index 066dfe24847..6fabfe1fe26 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1640,8 +1640,10 @@ dependencies = [ name = "dash-async" version = "4.1.0" dependencies = [ + "futures", "thiserror 2.0.18", "tokio", + "tokio-util", "tracing", ] @@ -5195,6 +5197,7 @@ dependencies = [ "async-trait", "bimap", "bs58", + "dash-async", "dash-sdk", "dash-spv", "dashcore", diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt index 0dfbbedc89d..40d92a980b1 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt @@ -7,11 +7,14 @@ package org.dashfoundation.dashsdk.ffi * * [nativeCreate] takes the SDK handle plus the two Kotlin bridge objects * (persistence + event), builds the native persistence / event vtables - * with boxed `GlobalRef` contexts, hands them to - * `platform_wallet_manager_create_with_persistence_capabilities`, and returns a boxed **bundle** pointer - * as a `jlong`. The bundle owns the two context boxes for the manager's - * lifetime; [nativeDestroy] shuts the manager down (quiescing every - * callback-firing task) and only then frees them. + * with boxed `GlobalRef` contexts, hands them — **with ownership** (the + * vtables carry a `release_fn`) — to + * `platform_wallet_manager_create_with_persistence_capabilities`, and + * returns a boxed **bundle** pointer as a `jlong`. The native manager + * frees each context box exactly once, when its last worker reference + * drops; [nativeDestroy] shuts the manager down (bounded quiesce + join + * of every callback-firing task) and a worker that straggles past it + * keeps its bridge `GlobalRef` alive until that worker exits. * * The raw manager `Handle` used by the sync / wallet-accessor calls is * read from the bundle via [nativeManagerHandle]. diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt index 9dce9df0798..afda8682ad6 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt @@ -147,9 +147,14 @@ class PlatformWalletPersistenceHandler( /** * Shut down the owned executor (no-op for an injected dispatcher). - * Callers must quiesce native callbacks first — the manager invokes - * this only after `nativeDestroy` has freed the callback contexts, so - * nothing can dispatch onto the executor afterwards. + * The manager invokes this after `nativeDestroy`'s bounded shutdown + * has quiesced the callback-firing tasks. A native worker that + * straggled past that shutdown holds its own strong reference to the + * bridge (Rust owns the callback context and frees it when the worker + * exits), so a late callback is memory-safe; if it dispatches onto + * the already-closed executor it is rejected and dropped, which is + * fine — every persistence hook is reconciliation-based and re-runs + * on the next launch. */ override fun close() { ownedDispatcher?.close() diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt index 1c1f11f41b0..3bd11eadffa 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt @@ -154,10 +154,12 @@ internal fun initializePlatformWalletNativeManager( * ## Lifecycle * * [AutoCloseable]. [close] stops the sync loops, destroys the native - * bundle (which runs the Rust `shutdown()` — quiescing every callback - * before its context `GlobalRef`s are freed), then closes the resolver + - * signer children. Order matters: the native manager must be torn down - * before the children whose bridges its callbacks reference. + * bundle (which runs the Rust `shutdown()` — a bounded quiesce + join of + * every callback-firing task; the context `GlobalRef`s themselves are + * owned and freed by the native manager when its last worker reference + * drops), then closes the resolver + signer children. Order matters: the + * native manager must be torn down before the children whose bridges its + * callbacks reference. * * Blocking natives are wrapped `suspend` / [withContext] `(Dispatchers.IO)` * and errors pass through `mapNativeErrors` at the public boundary. diff --git a/packages/rs-dash-async/Cargo.toml b/packages/rs-dash-async/Cargo.toml index 26e2c8fdeb9..69d180e5682 100644 --- a/packages/rs-dash-async/Cargo.toml +++ b/packages/rs-dash-async/Cargo.toml @@ -13,6 +13,8 @@ tracing = "0.1.41" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] tokio = { version = "1.40", features = ["rt", "rt-multi-thread", "time", "net"] } +tokio-util = { version = "0.7.12" } +futures = { version = "0.3.30" } [dev-dependencies] -tokio = { version = "1.40", features = ["macros", "rt-multi-thread", "sync"] } +tokio = { version = "1.40", features = ["macros", "rt-multi-thread", "sync", "time"] } diff --git a/packages/rs-dash-async/src/lib.rs b/packages/rs-dash-async/src/lib.rs index 0ef7785253b..88f96eba42c 100644 --- a/packages/rs-dash-async/src/lib.rs +++ b/packages/rs-dash-async/src/lib.rs @@ -2,7 +2,17 @@ //! //! Provides [`block_on`] -- a function that bridges async futures into sync code, //! handling multiple tokio runtime flavors (no runtime, current-thread, multi-thread). +//! +//! Also provides `ThreadRegistry` — a shared lifecycle engine for background +//! OS-thread workers (start, cancel, bounded quiesce + join, orphan reap). mod block_on; +#[cfg(not(target_arch = "wasm32"))] +mod registry; pub use block_on::{block_on, AsyncError}; +#[cfg(not(target_arch = "wasm32"))] +pub use registry::{ + ClearingGuard, RegistryKey, ShutdownReport, ThreadRegistry, WorkerConfig, WorkerStatus, + DEFAULT_JOIN_BUDGET, DEFAULT_REAP_BACKSTOP, +}; diff --git a/packages/rs-dash-async/src/registry.rs b/packages/rs-dash-async/src/registry.rs new file mode 100644 index 00000000000..e85efffb259 --- /dev/null +++ b/packages/rs-dash-async/src/registry.rs @@ -0,0 +1,1834 @@ +//! Shared lifecycle engine for background workers (`ThreadRegistry`). +//! +//! Centralizes the dangerous 80% of a background OS-thread worker's +//! lifecycle — the generation-match exit epilogue, the reap-or-park of a +//! restarted worker's prior thread, and the orphan drain — into one +//! tested place. The domain-specific 20% (the "is a pass in flight?" +//! drain barrier) stays with the consumer, which drains its own passes +//! *before* calling [`shutdown`](ThreadRegistry::shutdown). +//! +//! Workers are dedicated OS threads +//! ([`start_thread`](ThreadRegistry::start_thread)), for loops that +//! `block_on` `!Send` futures internally — the `!Send` value never +//! crosses the spawn boundary; the body itself is `Send`. +//! +//! # Why join at all +//! +//! Host callback contexts are owned by the workers themselves (the FFI +//! layer's persister/event wrappers release them on last-`Arc`-drop), so +//! joining is not about callback memory safety. It is about the +//! **runtime**: a consumer that owns its tokio runtime (e.g. +//! dash-evo-tool) must not drop it while a worker thread is still inside +//! `Handle::block_on`. [`shutdown`](ThreadRegistry::shutdown) is that +//! barrier, and the orphan accounting below exists so a wedged thread is +//! *reported* (`Timeout` / `Detached`) instead of silently detached — +//! the caller can then decide whether dropping the runtime is safe. +//! +//! # Safety invariants +//! +//! - **A timed-out or dropped quiesce never detaches a live thread.** +//! Every join path takes `&self`; the live join handle stays owned by +//! the slot and is never moved into a cancellable future's frame. A +//! dropped/timed-out [`quiesce`](ThreadRegistry::quiesce) therefore +//! cannot drop-and-detach the handle — on timeout (or on an external +//! drop) the handle is deterministically re-parked into the orphan +//! list, and the slot reports [`WorkerStatus::Timeout`], never a clean +//! `NotRunning`. +//! - **A restart never detaches a still-draining prior generation.** The +//! prior handle is parked and bounded-joined; a genuine wedge stays +//! parked for teardown to account for. + +use std::collections::BTreeMap; +use std::num::NonZeroUsize; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use tokio::runtime::RuntimeFlavor; +use tokio_util::sync::CancellationToken; + +// --------------------------------------------------------------------- +// Key +// --------------------------------------------------------------------- + +/// Worker identity — a consumer supplies a fixed enum. +/// Blanket-implemented — consumers just derive the listed bounds on +/// their own key type. +pub trait RegistryKey: Copy + Ord + Eq + std::fmt::Debug + Send + Sync + 'static {} +impl RegistryKey for T {} + +// --------------------------------------------------------------------- +// Status +// --------------------------------------------------------------------- + +/// Terminal status of one worker as classified by the registry at the end +/// of [`quiesce`](ThreadRegistry::quiesce) or the orphan reap. +/// +/// Consumers may re-export this directly on their public surface; the +/// variants distinguish clean exits (`Ok`, `NotRunning`) from every +/// non-clean outcome a host UAF-safety check must observe — see +/// [`is_clean`](Self::is_clean). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum WorkerStatus { + /// The loop exited and its thread/task joined cleanly. + Ok, + /// A tokio task ended for a non-panic, non-clean reason (cancelled / + /// aborted at the runtime level). Never produced by the registry + /// itself (its workers are OS threads); provided so a consumer + /// classifying its own tokio tasks into a [`ShutdownReport`] (e.g. + /// the wallet's event-adapter join) has a variant to use. + Stopped(Option), + /// The thread/task panicked; carries the best-effort panic message. + Panicked(String), + /// The managed join exceeded this worker's `join_budget`. The live + /// handle was re-parked into the orphan list — UAF-safe, non-clean. + Timeout, + /// A parked orphan was still alive after the reap grace — UAF-safe, + /// non-clean. + Detached, + /// No thread/task was running to join — never started, or already + /// joined by a prior teardown. + NotRunning, + /// Infrastructural join failure that is neither a timeout nor a + /// panic (unreachable in normal operation). + Error(String), +} + +impl WorkerStatus { + /// `true` only for a fully clean outcome: joined normally (`Ok`) or + /// never ran (`NotRunning`). + pub fn is_clean(&self) -> bool { + matches!(self, Self::Ok | Self::NotRunning) + } +} + +/// Aggregate result of [`ThreadRegistry::shutdown`]. +#[derive(Clone, Debug, PartialEq, Eq)] +#[must_use = "inspect all_clean() before dropping the runtime: a non-clean status flags a still-live worker or orphan"] +pub struct ShutdownReport { + /// Per-worker terminal status, keyed by worker id. + pub per_worker: BTreeMap, + /// Number of parked orphans still alive at the reap deadline. + pub detached: usize, + /// Aggregate terminal status from the orphan reap. Reaped orphans are + /// not keyed in `per_worker`, so without this a panicked/errored orphan + /// that finished within the reap grace (`detached == 0`) would silently + /// pass `all_clean()`. First non-clean classification wins; `Ok` when + /// every reaped orphan was clean (or none were parked). + pub orphan_status: WorkerStatus, +} + +impl ShutdownReport { + /// `true` only when every per-worker status is clean, every reaped + /// orphan was clean, and no orphan survived the reap. + pub fn all_clean(&self) -> bool { + self.detached == 0 + && self.orphan_status.is_clean() + && self.per_worker.values().all(WorkerStatus::is_clean) + } +} + +// --------------------------------------------------------------------- +// Per-worker registration options +// --------------------------------------------------------------------- + +/// Default managed-join budget when a [`WorkerConfig`] does not override +/// it. Pinned so an accidental change surfaces in tests. +pub const DEFAULT_JOIN_BUDGET: Duration = Duration::from_secs(30); + +/// Default orphan reap backstop (start-time reap and shutdown grace). +pub const DEFAULT_REAP_BACKSTOP: Duration = Duration::from_secs(1); + +/// Per-worker registration options. +#[derive(Clone, Copy, Debug)] +pub struct WorkerConfig { + /// Managed-join timeout for this worker. + pub join_budget: Duration, + /// OS-thread stack size. `None` uses the platform default. Raise it + /// for a loop whose body recurses deeply — e.g. GroveDB proof + /// verification, which overflows the default stack and faults with + /// SIGBUS on the guard page. + pub stack_size: Option, +} + +impl Default for WorkerConfig { + fn default() -> Self { + Self { + join_budget: DEFAULT_JOIN_BUDGET, + stack_size: None, + } + } +} + +// --------------------------------------------------------------------- +// Internal handle + slot state +// --------------------------------------------------------------------- + +/// A live worker's OS-thread join handle. Kept owned by its slot so a +/// cancellable caller can never move it into a future frame and detach it +/// on drop. +struct WorkerHandle(std::thread::JoinHandle<()>); + +impl WorkerHandle { + fn is_finished(&self) -> bool { + self.0.is_finished() + } + + /// Classify a **finished** handle: an OS thread yields only `Ok` / + /// `Panicked`. + fn classify(self) -> WorkerStatus { + match self.0.join() { + Ok(()) => WorkerStatus::Ok, + Err(payload) => WorkerStatus::Panicked(panic_message(payload)), + } + } +} + +/// Best-effort extraction of a panic message (`&str` / `String` cases). +fn panic_message(payload: Box) -> String { + if let Some(s) = payload.downcast_ref::<&str>() { + (*s).to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "".to_string() + } +} + +/// One key's slot. The entry is created on first start and never removed, +/// so `generation` stays monotonic across the key's whole lifetime — a +/// parked prior-generation thread can therefore always tell that its +/// generation is stale. `cancel.is_some()` is the running indicator; +/// `handle` is the join handle, reaped by the next start or by quiesce. +struct SlotState { + generation: u64, + cancel: Option, + handle: Option, + join_budget: Duration, +} + +// Manual `Default` (not `#[derive(Default)]`): the derived impl would +// initialise `join_budget` to `Duration::ZERO`, but the dormant slot must +// carry [`DEFAULT_JOIN_BUDGET`] so a key created on first-touch (via +// `BTreeMap::entry().or_default()`) is still join-bounded. +impl Default for SlotState { + fn default() -> Self { + Self { + generation: 0, + cancel: None, + handle: None, + join_budget: DEFAULT_JOIN_BUDGET, + } + } +} + +impl SlotState { + /// Rotate the slot onto a new generation: take the prior handle, + /// install a fresh cancellation token, bump generation, and write + /// `cfg`'s teardown config. Returns `(prior_handle, new_token, + /// new_generation)`. + fn prepare(&mut self, cfg: WorkerConfig) -> (Option, CancellationToken, u64) { + let prior = self.handle.take(); + let token = CancellationToken::new(); + self.cancel = Some(token.clone()); + self.generation += 1; + let my_gen = self.generation; + self.join_budget = cfg.join_budget; + (prior, token, my_gen) + } +} + +// --------------------------------------------------------------------- +// The registry +// --------------------------------------------------------------------- + +/// Shared lifecycle engine for background workers. See the module docs. +/// +/// Parked orphans carry their originating key so restart reaps and +/// teardown accounting stay key-scoped. +pub struct ThreadRegistry { + slots: Mutex>, + orphans: Mutex>, + reap_backstop: Duration, + /// One-way teardown latch. [`shutdown`](Self::shutdown) sets it under + /// the slot lock before snapshotting tiers; `start_thread`/`start_task` + /// honour it under the same lock and refuse to register a new worker + /// once teardown has begun, so a start racing shutdown can never leave + /// an un-joined worker behind. + closing: AtomicBool, + /// Per-key clearing latch — refcounted live-holder count per key + /// whose owner is mid clear-then-wipe (e.g. shielded + /// `clear_shielded`). `start_thread`/`start_task` refuse a (re)start + /// for any key with `count > 0`, so a fresh worker cannot slip past + /// the "no new pass" barrier and re-persist into the store the + /// clear is about to wipe. Resettable (mirror of + /// [`closing`](Self::closing) but per-key and scoped to a + /// [`ClearingGuard`]); the guard's `Drop` decrements the count and + /// removes the entry only when the count reaches zero — so two + /// concurrent / nested holders for the same key both keep the latch + /// raised until the LAST guard drops. + clearing: Mutex>, + /// Test seam: when set, the next OS-thread spawn returns an injected + /// `io::Error` instead of really spawning, so the spawn-failure + /// rollback path can be exercised deterministically. + #[cfg(test)] + force_spawn_failure: AtomicBool, +} + +impl std::fmt::Debug for ThreadRegistry { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ThreadRegistry") + .field("live_slots", &self.lock_slots().len()) + .field("orphans", &self.lock_orphans().len()) + .field("reap_backstop", &self.reap_backstop) + .field("closing", &self.closing.load(Ordering::Acquire)) + .field("clearing", &self.lock_clearing().len()) + .finish() + } +} + +/// Process-wide latch for the panic=abort startup warn. Hoisted out of the +/// constructor so the in-crate regression test can assert it tripped without +/// peeking at function-local state. +#[cfg(panic = "abort")] +static PANIC_ABORT_WARNED: std::sync::Once = std::sync::Once::new(); + +impl ThreadRegistry { + /// New registry with the default reap backstop ([`DEFAULT_REAP_BACKSTOP`]). + pub fn new() -> Arc { + Self::with_reap_backstop(DEFAULT_REAP_BACKSTOP) + } + + /// New registry with an explicit orphan reap backstop (the wallet + /// uses 1s — the same grace separates "finishing" from "wedged"). + /// + /// Under `panic = "abort"` builds (e.g. iOS release profiles) this + /// constructor emits a single startup-time `tracing::warn!` so + /// operators can audit the risk that an `EpilogueGuard` panic during + /// teardown aborts the process before `Drop` can release the + /// orphan-liveness gate. The warn is fired at most once per process via + /// [`std::sync::Once`]. + pub fn with_reap_backstop(backstop: Duration) -> Arc { + // Stable Rust has no runtime API to query the active panic strategy, + // so the gate is compile-time. iOS release builds intentionally pick + // abort — this is observability, not a hard error. + #[cfg(panic = "abort")] + PANIC_ABORT_WARNED.call_once(|| { + tracing::warn!( + "dash-async registry built with panic=abort: an EpilogueGuard \ + panic during teardown aborts the process instead of unwinding, \ + so the orphan-liveness gate may stay held — see the EpilogueGuard \ + doc caveat. iOS release builds choose abort intentionally; non-iOS \ + targets should prefer panic=unwind." + ); + }); + Arc::new(Self { + slots: Mutex::new(BTreeMap::new()), + orphans: Mutex::new(Vec::new()), + reap_backstop: backstop, + closing: AtomicBool::new(false), + clearing: Mutex::new(BTreeMap::new()), + #[cfg(test)] + force_spawn_failure: AtomicBool::new(false), + }) + } + + /// Start an OS-thread worker for `!Send` loops. `body` runs on a + /// fresh `std::thread` and may build and `block_on` `!Send` futures + /// internally — the `!Send` value never crosses the spawn boundary + /// (`body` itself is `Send`). Starting a key that already has a live + /// worker is a no-op; a key whose prior thread has not been reaped is + /// reaped-or-parked first (the restart-reap path). After + /// [`shutdown`](Self::shutdown) has begun the call is also a no-op (the + /// one-way closing latch). + /// + /// **Requires a multi-thread runtime**: the worker drives its loop + /// via `Handle::block_on` and needs the shared timer/IO driver. + /// + /// **Blocks the calling thread on restart-reap**: when restarting a + /// key whose prior OS thread is still finishing, this call SPINS + /// SYNCHRONOUSLY for up to `WorkerConfig::reap_backstop` (default + /// `DEFAULT_REAP_BACKSTOP` = 1 s) waiting for the prior to exit. Do + /// not call it directly from an async context — drive it via + /// `tokio::task::spawn_blocking` or a dedicated host thread. + /// + /// # Panics + /// + /// Panics if called outside a multi-thread Tokio runtime (see + /// [`shutdown`](Self::shutdown)). It does **not** panic on thread-spawn + /// failure: a failed spawn (e.g. the OS thread-count limit) is rolled + /// back — the prior handle is re-installed rather than detached and the + /// slot returns to not-running — and the call simply does not start a + /// worker. + pub fn start_thread(self: &Arc, key: K, cfg: WorkerConfig, body: F) + where + F: FnOnce(CancellationToken) + Send + 'static, + { + Self::assert_multi_thread("start_thread"); + let prior_tid = { + let mut slots = self.lock_slots(); + // One-way teardown latch: refuse new workers once shutdown has + // begun, under the same lock shutdown snapshots tiers with. + if self.closing.load(Ordering::Acquire) { + return; + } + // Per-key clearing latch: refuse a (re)start while this key's + // owner is mid clear-then-wipe, so a fresh worker cannot slip + // past the "no new pass" barrier and re-persist into the store + // the clear is about to wipe. + if self.lock_clearing().contains_key(&key) { + return; + } + let slot = slots.entry(key).or_default(); + if slot.cancel.is_some() { + return; + } + // Snapshot the slot's pre-start config so a spawn failure can roll + // the slot back to exactly its prior state: a re-installed prior + // worker must keep its OWN join budget, not inherit the failed + // start's. Generation is rolled back too — the bump is only ever + // observed under this lock and a failed start spawns no thread to + // reference it, so the rollback is net-zero and the + // externally-visible generation stays monotonic. + let prev_generation = slot.generation; + let prev_join_budget = slot.join_budget; + // `stack_size` is spawn-time only (not persisted on the slot): + // read it out before `prepare` consumes `cfg`. + let stack_size = cfg.stack_size; + // Rotate the slot atomically: take prior handle, install fresh + // cancellation token, bump generation, and write this start's + // teardown config — all under THIS slot lock so a prior thread's + // epilogue observes the post-swap generation. + let (prior, token, my_gen) = slot.prepare(cfg); + + let reg = Arc::clone(self); + let body_token = token; + // Build the epilogue drop-guard INSIDE the worker closure, not + // here: on a spawn failure the closure is dropped while we still + // hold the slot lock, and a guard constructed out here would run + // `run_epilogue` (which re-locks `slots`) on that drop and + // deadlock. Constructing it inside means it only exists once the + // thread is actually running. A panicking `body` then still + // clears this generation's running flag via the guard's Drop + // (under `panic = "unwind"`), and the panic keeps unwinding so + // the join handle still classifies as `Panicked`. + match self.spawn_os_thread(key, stack_size, move || { + let _epilogue = EpilogueGuard { reg, key, my_gen }; + body(body_token); + }) { + Ok(join) => { + // Store the new handle, then park the prior into orphans — + // both still under THIS slot lock, so `shutdown`'s + // under-lock snapshot can never see the new slot + // without also seeing the prior accounted (R1: store handle + // -> park prior -> drop guard -> THEN bounded reap below). + // See `park_prior_locked` for the lock-order rationale; the + // bounded join stays out of the lock in `reap_parked_prior`. + slot.handle = Some(WorkerHandle(join)); + self.park_prior_locked(key, prior) + } + Err(e) => { + // Spawn failed (e.g. EAGAIN at the OS thread ceiling). Roll + // the slot back to exactly its pre-start state: clear the + // running flag, re-install the prior handle (never + // detached), and restore the prior join budget + generation + // so nothing of the failed start lingers. Generation + // returns to its pre-bump value (the bump was never + // observed outside this lock and spawned no thread). + // Nothing was parked, so there is no prior to reap below. + tracing::error!( + ?key, + error = %e, + "failed to spawn registry worker thread; rolling back \ + start (prior handle re-installed, not detached)" + ); + slot.cancel = None; + slot.handle = prior; + slot.generation = prev_generation; + slot.join_budget = prev_join_budget; + None + } + } + }; + + // The prior thread was cancellation-signalled by a preceding + // cancel(); with the slot lock released its epilogue completes + // promptly and the join lands in milliseconds — `reap_parked_prior` + // then removes it from orphans and joins it. The backstop fires only + // on a genuine wedge, in which case the still-live handle is left + // parked (not dropped) so teardown can account for it. + self.reap_parked_prior(key, prior_tid); + } + + /// Whether a worker is currently registered and running for `key`. + pub fn is_running(&self, key: K) -> bool { + self.lock_slots() + .get(&key) + .map(|s| s.cancel.is_some()) + .unwrap_or(false) + } + + /// Signal-only cancellation of one worker. + pub fn cancel(&self, key: K) { + if let Some(slot) = self.lock_slots().get_mut(&key) { + if let Some(token) = slot.cancel.take() { + token.cancel(); + } + } + } + + /// Mark `key` as mid clear-then-wipe and refuse any + /// `start_thread`/`start_task` for it until the returned + /// [`ClearingGuard`] drops. Per-key (other keys are unaffected) and + /// resettable (subsequent clears can reacquire). The caller is + /// expected to hold the guard across the full quiesce → liveness → + /// wipe sequence, so a racing `(re)start` for the same key cannot + /// install a fresh worker that re-persists into the store mid-clear. + /// + /// **Refcounted**: two concurrent / nested holders for the same key + /// both keep the latch raised until the LAST guard drops. Each call + /// increments a per-key counter; `Drop` decrements it and removes the + /// entry only when the count reaches zero. This composes safely with + /// re-entrant or concurrent clears on the same key (e.g. a host + /// driving two FFI `shielded_clear` invocations through a read-locked + /// handle). + /// + /// Returns a guard, not a `begin/end` pair, so the latch is released + /// on every drop path — including panic unwinding — and a caller + /// cannot leak it. + pub fn hold_clearing(self: &Arc, key: K) -> ClearingGuard { + let mut clearing = self.lock_clearing(); + let next = match clearing.get(&key) { + Some(n) => n + .checked_add(1) + .expect("ClearingGuard count overflowed usize::MAX"), + None => NonZeroUsize::new(1).expect("1 is non-zero"), + }; + clearing.insert(key, next); + drop(clearing); + ClearingGuard { + reg: Arc::clone(self), + key, + } + } + + /// Whether `key` is currently held under a [`ClearingGuard`] (count + /// ≥ 1). Exposed so a coordinator can observe the latch BEFORE + /// side-effects that would otherwise leak into the clear flow (e.g. + /// lowering a continuously-held quiescing gate) even when its + /// `start_thread`/`start_task` would be refused. + pub fn is_clearing(&self, key: K) -> bool { + self.lock_clearing().contains_key(&key) + } + + /// Whether [`shutdown`](Self::shutdown) has latched the registry closed. + /// + /// The latch is one-way: once teardown begins it never reopens. + /// `start_thread` honours it internally; the accessor exists so a + /// consumer can observe teardown state before side effects of its own. + pub fn is_closing(&self) -> bool { + self.closing.load(Ordering::Acquire) + } + + /// Cancel this worker, then join within its budget. The live handle is + /// owned by the slot and is **never** moved into this future's frame, + /// so a dropped/timed-out call cannot detach it; on the managed + /// timeout — or if this future is dropped mid-poll — the handle is + /// re-parked into the orphan list. + /// + /// The registry owns no domain drain semantics: a consumer whose + /// worker has an "in-flight pass" concept drains it itself before + /// calling this (or [`shutdown`](Self::shutdown)). + pub async fn quiesce(&self, key: K) -> WorkerStatus { + // Snapshot the budget + generation, and bail early if nothing is + // registered for this key. The generation is the anchor for the + // supersede guard below. + // + // The inhabited check accepts a finished-but-unreaped handle + // (`handle.is_some()`, not a liveness probe): it must still be + // classified into its terminal status here rather than + // short-circuited to `NotRunning`, which would drop the result on + // the floor. + let (budget, my_gen) = { + let slots = self.lock_slots(); + match slots.get(&key) { + Some(s) if s.cancel.is_some() || s.handle.is_some() => { + (s.join_budget, s.generation) + } + _ => return WorkerStatus::NotRunning, + } + }; + + // Signal-only cancel — but only if this is still the generation we + // snapshotted. A concurrent restart (which can proceed the instant + // we take `cancel` below) bumps the generation; taking the new + // token here would silently un-track the fresh worker. + if let Some(slot) = self.lock_slots().get_mut(&key) { + if slot.generation == my_gen { + if let Some(token) = slot.cancel.take() { + token.cancel(); + } + } + } + + // Poll-join within budget. The re-park guard moves the slot's + // still-live handle into orphans if this future is dropped before + // the loop finishes — the handle is never owned by this frame. Both + // the guard and the loop are generation-scoped, so a concurrent + // same-key restart's live handle is never parked or classified by + // the quiesce that cancelled the *prior* generation. + let _repark = Repark { + reg: self, + key, + my_gen, + }; + let deadline = Instant::now() + budget; + loop { + enum Step { + Classify(WorkerHandle), + Park(WorkerHandle), + NotRunning, + Superseded, + Wait, + } + let step = { + let mut slots = self.lock_slots(); + match slots.get_mut(&key) { + None => Step::NotRunning, + // A restart replaced the generation we were draining: + // the handle now in the slot belongs to a newer, live + // worker the restart owns. Leave it untouched. + Some(slot) if slot.generation != my_gen => Step::Superseded, + Some(slot) => match slot.handle.take_if(|h| h.is_finished()) { + Some(h) => Step::Classify(h), + None if slot.handle.is_none() => Step::NotRunning, + None if Instant::now() >= deadline => { + Step::Park(slot.handle.take().expect("handle present")) + } + None => Step::Wait, + }, + } + }; + match step { + Step::Classify(h) => return h.classify(), + Step::Park(h) => { + self.lock_orphans().push((key, h)); + return WorkerStatus::Timeout; + } + Step::NotRunning | Step::Superseded => return WorkerStatus::NotRunning, + Step::Wait => tokio::time::sleep(Duration::from_millis(5)).await, + } + } + } + + /// Reap parked orphans with a short grace; survivors are re-parked and + /// reported as [`WorkerStatus::Detached`] (idempotent retry). + pub async fn reap_orphans(&self, grace: Duration) -> WorkerStatus { + self.reap_orphans_impl(grace).await.0 + } + + /// Teardown: every worker's (cancel -> join) runs concurrently; + /// orphan reap runs last. **Requires a multi-thread runtime.** + /// + /// Latches the registry closed first (under the slot lock, before the + /// key snapshot), so any `start_thread` racing teardown is either + /// already in the snapshot or refused outright — shutdown is a + /// one-way door and never leaves a worker un-joined. Idempotent. + /// + /// # Panics + /// + /// Panics if called outside a multi-thread Tokio runtime: an OS-thread + /// worker drives its loop via `Handle::block_on` and needs the shared + /// timer/IO driver, so a `current_thread` runtime would deadlock the + /// join. + pub async fn shutdown(&self) -> ShutdownReport { + Self::assert_multi_thread("shutdown"); + + // Snapshot the registered keys. Latch the registry closed under + // the same lock and before the snapshot so a racing start is + // serialized: it either landed before this lock (and is in the + // snapshot) or sees `closing` and bails. + let keys: Vec = { + let slots = self.lock_slots(); + self.closing.store(true, Ordering::Release); + slots.keys().copied().collect() + }; + + // Cancel + join every worker concurrently; `join_all` polls them + // on one task so the joins interleave. + let mut per_worker = BTreeMap::new(); + let drained = keys + .into_iter() + .map(|key| async move { (key, self.quiesce(key).await) }); + for (key, status) in futures::future::join_all(drained).await { + per_worker.insert(key, status); + } + + // Account for parked orphans last. The terminal status is folded + // into the report so a panicked/errored reaped orphan that finished + // within the grace (`detached == 0`) still flips `all_clean()`. + let (mut orphan_status, mut detached) = self.reap_orphans_impl(self.reap_backstop).await; + + // Late parkers: a quiesce racing this teardown can re-park a handle + // into orphans AFTER the reap above snapshotted the list. Re-drain + // until the orphan list is stable so such a straggler cannot slip + // through and let `all_clean()` false-pass. Bounded: `closing` is + // one-way so no new worker can start, and each pass either drains a + // finite backlog or re-parks genuine survivors (`detached > 0`) — + // which we fold in and stop, leaving them for an idempotent retry + // rather than spinning on a wedged thread. + while detached == 0 && !self.lock_orphans().is_empty() { + let (late_status, late_detached) = self.reap_orphans_impl(self.reap_backstop).await; + if orphan_status.is_clean() && !late_status.is_clean() { + orphan_status = late_status; + } + detached += late_detached; + } + ShutdownReport { + per_worker, + detached, + orphan_status, + } + } + + // ----------------------------------------------------------------- + // Internal helpers + // ----------------------------------------------------------------- + + fn lock_slots(&self) -> std::sync::MutexGuard<'_, BTreeMap> { + self.slots.lock().unwrap_or_else(|e| e.into_inner()) + } + + fn lock_orphans(&self) -> std::sync::MutexGuard<'_, Vec<(K, WorkerHandle)>> { + self.orphans.lock().unwrap_or_else(|e| e.into_inner()) + } + + fn lock_clearing(&self) -> std::sync::MutexGuard<'_, BTreeMap> { + self.clearing.lock().unwrap_or_else(|e| e.into_inner()) + } + + fn assert_multi_thread(ctx: &str) { + assert!( + matches!( + tokio::runtime::Handle::current().runtime_flavor(), + RuntimeFlavor::MultiThread + ), + "ThreadRegistry::{ctx}() requires a multi-thread Tokio runtime: an \ + OS-thread worker drives its loop via Handle::block_on and needs the \ + runtime's timer/IO driver, but a current_thread runtime can only \ + drive one block_on at a time" + ); + } + + /// Gen-gated exit epilogue, run on the worker after its body returns + /// (or unwinds): clear this slot's running flag only if a newer start + /// has not since installed a replacement. + fn run_epilogue(&self, key: K, my_gen: u64) { + if let Some(slot) = self.lock_slots().get_mut(&key) { + if slot.generation == my_gen { + slot.cancel = None; + } + } + } + + /// Spawn the named OS worker thread, surfacing a spawn failure as + /// `io::Result` instead of panicking so the caller can roll back. The + /// `#[cfg(test)]` seam forces a synthetic failure to exercise that path. + fn spawn_os_thread( + &self, + key: K, + stack_size: Option, + closure: C, + ) -> std::io::Result> + where + C: FnOnce() + Send + 'static, + { + #[cfg(test)] + if self.force_spawn_failure.load(Ordering::Acquire) { + return Err(std::io::Error::other("forced spawn failure (test seam)")); + } + let mut builder = std::thread::Builder::new().name(format!("tr-worker-{key:?}")); + if let Some(stack_size) = stack_size { + builder = builder.stack_size(stack_size.get()); + } + builder.spawn(closure) + } + + /// Park a restarted key's prior handle into orphans. **Must be called + /// while the slot lock is held** — the resulting `slots`->`orphans` + /// nesting is the only such nesting in this module and is deadlock-free + /// (no path ever acquires `slots` while holding `orphans`, so there is no + /// cycle). Parking the prior here, rather than after the slot lock is + /// released, is what lets `shutdown`'s under-lock snapshot never + /// miss it: the take-prior and the park-prior are then atomic from + /// `shutdown`'s view. Returns the parked thread's id so + /// [`reap_parked_prior`](Self::reap_parked_prior) can find and + /// bounded-join it. + fn park_prior_locked( + &self, + key: K, + prior: Option, + ) -> Option { + match prior { + Some(h) => { + let tid = h.0.thread().id(); + self.lock_orphans().push((key, h)); + Some(tid) + } + None => None, + } + } + + /// Bounded reap of an OS-thread prior that [`park_prior_locked`](Self::park_prior_locked) + /// parked under `key` at restart. Must be called with no registry lock + /// held (it spins synchronously). The instant the parked thread finishes + /// it is removed from orphans and joined — the join itself stays OUT of + /// any lock (only the bookkeeping is taken under the orphans lock). A + /// genuine wedge past the reap backstop is left parked, so teardown can + /// still account for it. No-op when no OS thread was parked (`None`), or + /// when the orphan was already taken by a concurrent reaper / `shutdown` + /// (which then owns the join). + fn reap_parked_prior(&self, key: K, prior_tid: Option) { + let Some(tid) = prior_tid else { + return; + }; + let deadline = Instant::now() + self.reap_backstop; + loop { + // Bookkeeping under the orphans lock only: locate our parked + // prior by thread id and, once it has finished, take it out to + // join after the lock is released. Never hold the lock across the + // join. + let taken = { + let mut orphans = self.lock_orphans(); + let pos = orphans + .iter() + .position(|(k, h)| *k == key && h.0.thread().id() == tid); + match pos { + // Already taken by a concurrent reaper / shutdown: it owns + // the join now. + None => return, + Some(i) if orphans[i].1.is_finished() => Some(orphans.remove(i).1), + Some(_) if Instant::now() >= deadline => { + tracing::warn!( + ?key, + backstop = ?self.reap_backstop, + "prior worker thread did not finish within the reap \ + backstop after cancellation; leaving it parked as an \ + orphan for teardown to join rather than detaching it" + ); + return; + } + Some(_) => None, + } + }; + if let Some(handle) = taken { + // Join through `classify` rather than discarding the result: a + // prior generation that panicked must not vanish silently at + // restart. `classify` performs the actual join. + let status = handle.classify(); + if !status.is_clean() { + tracing::error!( + ?key, + ?status, + "prior-generation worker ended non-cleanly during restart reap" + ); + } + return; + } + std::thread::sleep(Duration::from_millis(5)); + } + } + + /// Drain the orphan list, polling until `grace`. Returns the terminal + /// status and the number of survivors re-parked for an idempotent + /// retry. + async fn reap_orphans_impl(&self, grace: Duration) -> (WorkerStatus, usize) { + let mut pending: Vec<(K, WorkerHandle)> = { + let mut guard = self.lock_orphans(); + std::mem::take(&mut *guard) + }; + if pending.is_empty() { + return (WorkerStatus::Ok, 0); + } + + let deadline = Instant::now() + grace; + // Keep the first non-clean terminal status; a live survivor still + // takes precedence at the deadline. + let mut non_clean: Option = None; + loop { + let mut still_live = Vec::with_capacity(pending.len()); + for (key, handle) in pending.drain(..) { + if handle.is_finished() { + let status = handle.classify(); + if !status.is_clean() { + non_clean.get_or_insert(status); + } + } else { + still_live.push((key, handle)); + } + } + pending = still_live; + + if pending.is_empty() { + return (non_clean.unwrap_or(WorkerStatus::Ok), 0); + } + if Instant::now() >= deadline { + let survivors = pending.len(); + self.lock_orphans().extend(pending); + return (WorkerStatus::Detached, survivors); + } + tokio::time::sleep(Duration::from_millis(5)).await; + } + } + + /// Test-only seam: park a raw thread handle as an orphan under `key`. + /// Injects a wedged prior-generation thread into the reap path without + /// driving the full restart-reap dance. In-crate tests only. + #[cfg(test)] + fn park_orphan_for_test(&self, key: K, handle: std::thread::JoinHandle<()>) { + self.lock_orphans().push((key, WorkerHandle(handle))); + } + + /// Test-only liveness probe spanning live slots and parked orphans — + /// the assertion the re-park/reap tests are built on. + #[cfg(test)] + fn any_alive(&self) -> bool { + { + let slots = self.lock_slots(); + for slot in slots.values() { + if slot.cancel.is_some() || slot.handle.as_ref().is_some_and(|h| !h.is_finished()) { + return true; + } + } + } + self.lock_orphans().iter().any(|(_, h)| !h.is_finished()) + } +} + +/// Re-park guard for [`ThreadRegistry::quiesce`]. If the poll-join future +/// is dropped before it finishes (e.g. an outer timeout fires), this moves +/// the slot's still-live handle into the orphan list instead of letting it +/// be dropped-and-detached. On normal completion the handle has already +/// been taken from the slot, so this is a no-op. +/// +/// Generation-scoped: it only re-parks the handle if the slot still holds +/// the generation `quiesce` was draining. A concurrent same-key restart +/// bumps the generation and installs its own live handle; this guard leaves +/// that fresh handle alone. +struct Repark<'a, K: RegistryKey> { + reg: &'a ThreadRegistry, + key: K, + my_gen: u64, +} + +impl Drop for Repark<'_, K> { + fn drop(&mut self) { + // Take the handle under the slot lock, release it, then push to + // orphans. This path holds only one lock at a time; the single + // sanctioned nesting in the module is `slots`->`orphans` in + // `park_prior_locked`, and nothing ever takes `slots` while holding + // `orphans`, so the ordering stays acyclic. Skip if a restart + // superseded our generation (the handle is the new worker's, not + // ours). + let handle = self + .reg + .lock_slots() + .get_mut(&self.key) + .filter(|slot| slot.generation == self.my_gen) + .and_then(|slot| slot.handle.take()); + if let Some(handle) = handle { + self.reg.lock_orphans().push((self.key, handle)); + } + } +} + +/// Worker-side exit guard. Runs the generation-gated [`run_epilogue`] +/// from its `Drop`, so a worker whose `body` returns normally **or** +/// unwinds on panic still clears its running flag — `is_running()` then +/// reflects reality and `start()` can relaunch a crashed loop. +/// +/// Panic-strategy caveat: the clear-on-panic half relies on `Drop` running +/// while the stack unwinds, so it holds under +/// `panic = "unwind"`. Under `panic = "abort"` a worker panic aborts the +/// process and there is no "after" to gate. When the binary is built with +/// `panic = "abort"`, [`ThreadRegistry::with_reap_backstop`] emits a +/// one-shot `tracing::warn!` so operators can audit the risk. +struct EpilogueGuard { + reg: Arc>, + key: K, + my_gen: u64, +} + +impl Drop for EpilogueGuard { + fn drop(&mut self) { + self.reg.run_epilogue(self.key, self.my_gen); + } +} + +/// RAII guard returned by [`ThreadRegistry::hold_clearing`]. While at +/// least one guard for a key is alive, the registry refuses any +/// `start_thread`/`start_task` for that key. Drop decrements the +/// per-key holder count and removes the entry only when the count +/// reaches zero — so nested or concurrent holders for the same key +/// compose, and the latch stays raised until the LAST guard drops. +/// Drop runs on every exit path, including panic unwinding, so a +/// caller cannot leak the latch by forgetting to call an end function. +pub struct ClearingGuard { + reg: Arc>, + key: K, +} + +impl Drop for ClearingGuard { + fn drop(&mut self) { + let mut clearing = self.reg.lock_clearing(); + match clearing.get(&self.key) { + // Decrement; remove the entry when the last holder drops. + Some(n) => match NonZeroUsize::new(n.get() - 1) { + Some(remaining) => { + clearing.insert(self.key, remaining); + } + None => { + clearing.remove(&self.key); + } + }, + // Defensive: a balanced hold/drop should always find the + // entry. A missing entry means someone removed it out of + // band — refuse to underflow. + None => { + debug_assert!( + false, + "ClearingGuard::drop saw no entry for its key — \ + someone removed the latch out of band" + ); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::panic::{catch_unwind, AssertUnwindSafe}; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::mpsc; + use tokio::runtime::{Builder, Handle}; + + type Reg = Arc>; + + /// Start an OS-thread worker that exits cleanly when cancelled. The + /// runtime handle is captured from the caller's context (the worker + /// thread is not itself a tokio worker, so it can't fetch its own). + fn start_clean(reg: &Reg, key: &'static str, cfg: WorkerConfig) { + let handle = Handle::current(); + reg.start_thread(key, cfg, move |cancel| { + handle.block_on(async move { cancel.cancelled().await }); + }); + } + + /// Body for a worker wedged in a non-yielding section: blocks on a + /// channel and ignores its cancellation token (stands in for a thread + /// stuck in a `Drop` that never observes cancel). + fn wedged_body(rx: mpsc::Receiver<()>) -> impl FnOnce(CancellationToken) + Send + 'static { + move |_cancel| { + let _ = rx.recv(); + } + } + + fn orphan_len(reg: &Reg) -> usize { + reg.lock_orphans().len() + } + + // ----- Group 1: F1 regression ------------------------------------- + + /// A `quiesce` whose outer future is dropped (a tiny enclosing + /// timeout) must re-park the live handle, never drop-and-detach it. The + /// slot is cleared (`is_running == false`) but the handle lives in + /// orphans and `any_alive()` stays true. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn quiesce_drop_reparks_handle_not_detach() { + let reg = ThreadRegistry::<&str>::new(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + reg.start_thread("alpha", WorkerConfig::default(), wedged_body(release_rx)); + assert!(reg.is_running("alpha")); + + // The wedged worker never observes cancel, so the internal 30s + // budget can't fire here; the tiny outer timeout drops the quiesce + // future mid-poll. A naive by-value-into-future impl would detach + // the handle (orphans empty, any_alive false); the slot-owned + // handle is re-parked instead. + let result = tokio::time::timeout(Duration::from_millis(100), reg.quiesce("alpha")).await; + assert!( + result.is_err(), + "outer timeout must fire on the wedged worker" + ); + + assert!(reg.any_alive(), "re-parked handle keeps any_alive true"); + assert!(!reg.is_running("alpha"), "slot cleared (cancel taken)"); + assert_eq!(orphan_len(®), 1, "handle was re-parked, not detached"); + assert!(!WorkerStatus::Timeout.is_clean()); + + // Release + reap: the orphan joins cleanly and liveness clears. + release_tx.send(()).unwrap(); + assert_eq!( + reg.reap_orphans(Duration::from_secs(2)).await, + WorkerStatus::Ok + ); + assert!(!reg.any_alive()); + } + + /// Internal-budget variant: a wedged worker with a tiny `join_budget` + /// makes `quiesce` itself time out, re-park, and return `Timeout` (no + /// outer drop involved). + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn quiesce_internal_budget_timeout_reparks() { + let reg = ThreadRegistry::<&str>::new(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + let cfg = WorkerConfig { + join_budget: Duration::from_millis(50), + ..WorkerConfig::default() + }; + reg.start_thread("alpha", cfg, wedged_body(release_rx)); + + let status = reg.quiesce("alpha").await; + assert_eq!(status, WorkerStatus::Timeout); + assert_eq!(orphan_len(®), 1); + assert!(reg.any_alive()); + assert!(!reg.is_running("alpha")); + + release_tx.send(()).unwrap(); + assert_eq!( + reg.reap_orphans(Duration::from_secs(2)).await, + WorkerStatus::Ok + ); + assert!(!reg.any_alive()); + } + + /// A wedged worker reached through the `shutdown()` path: with a tiny + /// budget it surfaces as `Timeout` in the report, its handle is + /// re-parked (`detached == 1`, `any_alive`), and the result is + /// non-clean — never a clean detach. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn shutdown_path_reparks_wedged_worker() { + let reg = ThreadRegistry::<&str>::new(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + let cfg = WorkerConfig { + join_budget: Duration::from_millis(50), + ..WorkerConfig::default() + }; + reg.start_thread("alpha", cfg, wedged_body(release_rx)); + + let report = tokio::time::timeout(Duration::from_secs(10), reg.shutdown()) + .await + .expect("shutdown must complete within bound"); + assert_eq!(report.per_worker.get("alpha"), Some(&WorkerStatus::Timeout)); + assert_eq!(report.detached, 1, "wedged handle re-parked, survived reap"); + assert!(!report.all_clean()); + assert!(reg.any_alive()); + + // Cleanup. + release_tx.send(()).unwrap(); + let _ = reg.reap_orphans(Duration::from_secs(5)).await; + assert!(!reg.any_alive()); + } + + // ----- Group 3: registry unit suite ------------------------------- + + /// A slow prior-generation thread's epilogue must NOT clear a newer + /// generation's token. Restarting reaps the prior generation fully (its + /// epilogue runs); the new generation stays tracked. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn generation_match_epilogue_preserves_new_token() { + let reg = ThreadRegistry::<&str>::new(); + start_clean(®, "beta", WorkerConfig::default()); // gen 1 + assert!(reg.is_running("beta")); + + // Cancel gen 1, then restart. start_thread's reap joins gen 1 + // (running its gen-gated epilogue) before returning, so this is + // deterministic: if the epilogue ignored generation it would have + // cleared gen 2's token during that join. + reg.cancel("beta"); + start_clean(®, "beta", WorkerConfig::default()); // gen 2 + + assert!( + reg.is_running("beta"), + "gen-2 token must survive gen-1's epilogue" + ); + assert_eq!(reg.quiesce("beta").await, WorkerStatus::Ok); + } + + /// A worker started with an explicit `WorkerConfig::stack_size` spawns, + /// runs its body, and joins cleanly — the custom-stack spawn path is + /// wired through `spawn_os_thread`. (A deliberate stack-overflow test is + /// avoided: an overflow aborts the whole process, not just the test.) + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn start_thread_honors_custom_stack_size() { + let reg = ThreadRegistry::<&str>::new(); + let cfg = WorkerConfig { + stack_size: Some(NonZeroUsize::new(8 * 1024 * 1024).expect("8 MiB is non-zero")), + ..WorkerConfig::default() + }; + start_clean(®, "big-stack", cfg); + assert!(reg.is_running("big-stack")); + assert_eq!(reg.quiesce("big-stack").await, WorkerStatus::Ok); + } + + /// A naturally-finished prior thread is joined cleanly on restart, with + /// no parking. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn restart_reaps_finished_prior_without_parking() { + let reg = ThreadRegistry::<&str>::new(); + start_clean(®, "gamma", WorkerConfig::default()); + // Cancel so the prior exits, then restart: the reap must join it, + // not park it. + reg.cancel("gamma"); + start_clean(®, "gamma", WorkerConfig::default()); + assert_eq!(orphan_len(®), 0, "finished prior was joined, not parked"); + assert!(reg.is_running("gamma")); + assert_eq!(reg.quiesce("gamma").await, WorkerStatus::Ok); + } + + /// A prior thread wedged past the reap backstop is parked in orphans + /// (not dropped), then drained after release. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn restart_parks_wedged_prior() { + let reg = ThreadRegistry::with_reap_backstop(Duration::from_millis(100)); + let (release_tx, release_rx) = mpsc::channel::<()>(); + + // gen 1: wedged (ignores cancel). + reg.start_thread("delta", WorkerConfig::default(), wedged_body(release_rx)); + reg.cancel("delta"); + + // gen 2: clean. The restart reaps gen 1 — wedged past the 100ms + // backstop, so it is parked. Run off the runtime workers since the + // reap spins synchronously. + let reg_for_start = Arc::clone(®); + let parent = Handle::current(); + tokio::task::spawn_blocking(move || { + let handle = parent.clone(); + reg_for_start.start_thread("delta", WorkerConfig::default(), move |cancel| { + handle.block_on(async move { cancel.cancelled().await }); + }); + }) + .await + .unwrap(); + + assert_eq!(orphan_len(®), 1, "wedged prior parked, not dropped"); + assert!(reg.any_alive()); + assert!(reg.is_running("delta"), "gen-2 loop started"); + + // Release the wedged prior; reap drains it. + release_tx.send(()).unwrap(); + assert_eq!( + reg.reap_orphans(Duration::from_secs(2)).await, + WorkerStatus::Ok + ); + assert_eq!(orphan_len(®), 0); + + // Cleanup gen 2. + assert_eq!(reg.quiesce("delta").await, WorkerStatus::Ok); + } + + /// Orphan drain: a survivor at the grace deadline is reported + /// `Detached` and re-parked; once released it reaps `Ok`. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn orphan_drain_detached_then_ok() { + let reg = ThreadRegistry::<&str>::new(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + let wedged = std::thread::spawn(move || { + let _ = release_rx.recv(); + }); + reg.park_orphan_for_test("orphan", wedged); + + assert_eq!( + reg.reap_orphans(Duration::from_millis(50)).await, + WorkerStatus::Detached + ); + assert_eq!(orphan_len(®), 1, "survivor re-parked for retry"); + assert!(reg.any_alive()); + + release_tx.send(()).unwrap(); + assert_eq!( + reg.reap_orphans(Duration::from_secs(2)).await, + WorkerStatus::Ok + ); + assert_eq!(orphan_len(®), 0); + assert!(!reg.any_alive()); + } + + /// A reaped orphan whose body PANICKED (finishes within the reap grace, + /// so `detached == 0`) must surface in `ShutdownReport::orphan_status` + /// and flip `all_clean()`. + /// + /// Non-vacuous: `orphan_status` is the only place a panicked-but-reaped + /// orphan is observable — without it, an empty/clean `per_worker` plus + /// `detached == 0` would let `all_clean()` return true and swallow the + /// panic. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn shutdown_report_surfaces_panicked_reaped_orphan() { + let reg = ThreadRegistry::<&str>::new(); + // Park a panicking thread directly as an orphan via the test seam. + // The body panics immediately, so the thread is finished by the + // time `shutdown()` runs the reap and classifies as `Panicked`. + let panicker = std::thread::spawn(|| { + panic!("deliberate orphan-body panic"); + }); + reg.park_orphan_for_test("k", panicker); + + let report = reg.shutdown().await; + + assert_eq!( + report.detached, 0, + "panicked orphan finished within the reap grace" + ); + assert!( + matches!(report.orphan_status, WorkerStatus::Panicked(_)), + "reaped orphan's panic must surface in orphan_status, got {:?}", + report.orphan_status + ); + assert!( + !report.all_clean(), + "all_clean() must reflect the panicked reaped orphan, not pass it" + ); + assert!(!reg.any_alive()); + } + + /// Complement: a clean reaped orphan (`Ok`) leaves `all_clean()` true, + /// so the orphan-status fold doesn't over-trigger on the common case of + /// orphans that drained cleanly within the grace. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn shutdown_report_clean_reaped_orphan_is_clean() { + let reg = ThreadRegistry::<&str>::new(); + let clean = std::thread::spawn(|| { /* exits cleanly */ }); + reg.park_orphan_for_test("k", clean); + + let report = reg.shutdown().await; + assert_eq!(report.detached, 0); + assert_eq!(report.orphan_status, WorkerStatus::Ok); + assert!(report.all_clean()); + } + + /// `shutdown()` panics with a documented message on a current-thread + /// runtime. + #[test] + fn shutdown_asserts_multi_thread_runtime() { + let rt = Builder::new_current_thread().enable_all().build().unwrap(); + let reg = ThreadRegistry::<&str>::new(); + let result = catch_unwind(AssertUnwindSafe(|| { + // We're proving `shutdown()` panics — its return value is + // moot here, but `#[must_use]` requires an explicit drop. + let _ = rt.block_on(async { reg.shutdown().await }); + })); + let payload = result.expect_err("shutdown must panic on current_thread"); + let msg = payload + .downcast_ref::() + .map(String::as_str) + .or_else(|| payload.downcast_ref::<&str>().copied()) + .unwrap_or(""); + assert!( + msg.contains("multi-thread"), + "panic must name the runtime constraint, got: {msg}" + ); + } + + // ----- Group 4: DrainHook ordering -------------------------------- + + // ----- Group 5: status classification ----------------------------- + + /// An `OsThread` worker yields `Ok` (clean) or `Panicked` (`&str` and + /// `String` payloads), never `Stopped`. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn os_thread_ok_and_panicked_never_stopped() { + let reg = ThreadRegistry::<&str>::new(); + start_clean(®, "os_clean", WorkerConfig::default()); + let ok = reg.quiesce("os_clean").await; + assert_eq!(ok, WorkerStatus::Ok); + assert!(ok.is_clean()); + + // &str panic payload. + reg.start_thread("os_panic_str", WorkerConfig::default(), |_cancel| { + panic!("deliberate test panic"); + }); + match reg.quiesce("os_panic_str").await { + WorkerStatus::Panicked(msg) => assert!(msg.contains("deliberate test panic")), + other => panic!("expected Panicked, got {other:?}"), + } + + // String panic payload. + reg.start_thread("os_panic_string", WorkerConfig::default(), |_cancel| { + std::panic::panic_any(String::from("deliberate string panic")); + }); + match reg.quiesce("os_panic_string").await { + WorkerStatus::Panicked(msg) => assert!(msg.contains("deliberate string panic")), + other => panic!("expected Panicked, got {other:?}"), + } + } + + // ----- Gaps ------------------------------------------------------- + + /// `shutdown()` is idempotent: a second call finds every slot already + /// joined and reports `NotRunning`, still clean. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn shutdown_is_idempotent() { + let reg = ThreadRegistry::<&str>::new(); + start_clean(®, "alpha", WorkerConfig::default()); + + let first = reg.shutdown().await; + assert_eq!(first.per_worker.get("alpha"), Some(&WorkerStatus::Ok)); + assert!(first.all_clean()); + + let second = reg.shutdown().await; + assert_eq!( + second.per_worker.get("alpha"), + Some(&WorkerStatus::NotRunning) + ); + assert!(second.all_clean()); + } + + /// `cancel(key)` is selective: cancelling A does not touch B. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn cancel_is_selective() { + let reg = ThreadRegistry::<&str>::new(); + start_clean(®, "a", WorkerConfig::default()); + start_clean(®, "b", WorkerConfig::default()); + + reg.cancel("a"); + assert!(reg.is_running("b"), "cancel(a) must not cancel b"); + assert_eq!(reg.quiesce("a").await, WorkerStatus::Ok); + assert!(reg.is_running("b"), "b still running after a drains"); + assert_eq!(reg.quiesce("b").await, WorkerStatus::Ok); + } + + /// `WorkerConfig::default()` values are pinned. + #[test] + fn worker_config_defaults_pinned() { + let cfg = WorkerConfig::default(); + assert_eq!(cfg.join_budget, DEFAULT_JOIN_BUDGET); + assert!(cfg.stack_size.is_none()); + } + + /// `hold_clearing(key)` refuses `start_thread` for that key, but ONLY + /// for that key — other keys are unaffected. After the guard drops the + /// latch releases and starts succeed again. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn hold_clearing_blocks_starts_for_latched_key_only() { + let reg = ThreadRegistry::<&str>::new(); + let _gate = reg.hold_clearing("shielded"); + + // start_thread on the latched key is a no-op. + start_clean(®, "shielded", WorkerConfig::default()); + assert!( + !reg.is_running("shielded"), + "start_thread must be refused while the key is latched" + ); + + // An unrelated key starts cleanly — the latch is per-key. + start_clean(®, "identity", WorkerConfig::default()); + assert!(reg.is_running("identity")); + + // Drop the latch; the same key now starts. + drop(_gate); + start_clean(®, "shielded", WorkerConfig::default()); + assert!( + reg.is_running("shielded"), + "latch release allows the key to start again" + ); + + // Cleanup. + assert_eq!(reg.quiesce("shielded").await, WorkerStatus::Ok); + assert_eq!(reg.quiesce("identity").await, WorkerStatus::Ok); + } + + /// Refcounted nesting: two concurrent / nested holders for the same + /// key keep the latch raised until the LAST guard drops. The inner + /// guard's drop must NOT release the latch the outer still holds — + /// otherwise a re-entrant or concurrent caller silently lapses the + /// invariant. The `start_clean` between `drop(inner)` and `drop(outer)` + /// below stays refused and `is_clearing` stays true, proving the outer + /// holder's protection survives the inner drop. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn hold_clearing_inner_drop_does_not_lapse_outer_protection() { + let reg = ThreadRegistry::<&str>::new(); + let outer = reg.hold_clearing("shielded"); + let inner = reg.hold_clearing("shielded"); + + // While both guards are live, starts are refused and the latch + // reports clearing. + start_clean(®, "shielded", WorkerConfig::default()); + assert!(!reg.is_running("shielded")); + assert!(reg.is_clearing("shielded")); + + // Drop the INNER guard while the outer is still alive. + drop(inner); + + // The outer protection MUST survive: the latch is still raised + // and a fresh start is still refused. + assert!( + reg.is_clearing("shielded"), + "outer ClearingGuard must keep the latch raised after the inner drops" + ); + start_clean(®, "shielded", WorkerConfig::default()); + assert!( + !reg.is_running("shielded"), + "start must still be refused while the outer guard is alive" + ); + + // After the outer drops too, the latch fully releases. + drop(outer); + assert!(!reg.is_clearing("shielded")); + assert_eq!(reg.lock_clearing().len(), 0); + + // And a fresh start succeeds. + start_clean(®, "shielded", WorkerConfig::default()); + assert!(reg.is_running("shielded")); + assert_eq!(reg.quiesce("shielded").await, WorkerStatus::Ok); + } + + /// The latch holds across panic unwinding (RAII guarantee). A + /// closure that holds the guard and panics still removes the key + /// from the clearing set on its drop path. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn hold_clearing_releases_on_panic_unwind() { + let reg = ThreadRegistry::<&str>::new(); + let reg_clone = Arc::clone(®); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _gate = reg_clone.hold_clearing("shielded"); + assert_eq!(reg_clone.lock_clearing().len(), 1); + panic!("simulated clear-flow panic"); + })); + assert!(result.is_err()); + assert_eq!( + reg.lock_clearing().len(), + 0, + "ClearingGuard's Drop must release the latch even when the clear flow panics" + ); + + // The key is startable again post-panic. + start_clean(®, "shielded", WorkerConfig::default()); + assert!(reg.is_running("shielded")); + assert_eq!(reg.quiesce("shielded").await, WorkerStatus::Ok); + } + + // ----- Group 6: concurrency-hazard regressions -------------------- + + /// `quiesce` is generation-guarded. A same-key restart that lands after + /// quiesce takes the prior's cancel must not have its fresh, live handle + /// parked or reported `Timeout`: the superseded quiesce returns + /// `NotRunning` and the new generation survives. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn quiesce_generation_guard_spares_concurrent_restart() { + let reg = ThreadRegistry::<&str>::new(); + // gen-1: a worker that ignores cancellation (wedged on a channel), + // with a tiny join budget so a non-guarded quiesce would Timeout + // quickly. + let (gen1_release_tx, gen1_release_rx) = mpsc::channel::<()>(); + reg.start_thread( + "k", + WorkerConfig { + join_budget: Duration::from_millis(150), + ..WorkerConfig::default() + }, + wedged_body(gen1_release_rx), + ); + + // Drive quiesce concurrently; it snapshots gen=1, cancels (ignored), + // and enters the poll loop with cancel already taken. + let reg_q = Arc::clone(®); + let q = tokio::spawn(async move { reg_q.quiesce("k").await }); + + // Let quiesce pass cancel.take() so a restart can proceed. + tokio::time::sleep(Duration::from_millis(40)).await; + + // Restart: cancel is now None, so this proceeds — it takes gen-1's + // live handle as its prior (parked) and installs gen-2. Release the + // wedge first so the restart's bounded prior-reap can join it. + gen1_release_tx.send(()).expect("release gen-1"); + start_clean(®, "k", WorkerConfig::default()); + + // The superseded quiesce must NOT park gen-2 / report Timeout. + let status = q.await.unwrap(); + assert_eq!( + status, + WorkerStatus::NotRunning, + "superseded quiesce returns NotRunning, never a spurious Timeout" + ); + assert!(reg.is_running("k"), "gen-2 survives the racing quiesce"); + + // gen-2 quiesces cleanly. + assert_eq!(reg.quiesce("k").await, WorkerStatus::Ok); + } + + /// A thread-spawn failure must neither panic nor detach the live prior + /// handle: it rolls back (prior re-installed, running flag cleared) and + /// the slot stays usable / reapable. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn spawn_failure_reparks_live_prior_without_panic() { + let reg = ThreadRegistry::<&str>::new(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + // gen-1: wedged (ignores cancel), stays live until released. + reg.start_thread("k", WorkerConfig::default(), wedged_body(release_rx)); + // cancel() takes the token (slot.cancel = None) but the wedged thread + // keeps running — the slot now holds a LIVE prior handle with cancel + // cleared, the exact shape a racing restart would take as its prior. + reg.cancel("k"); + assert!(!reg.is_running("k")); + + // Force the restart's spawn to fail; it must not panic. + reg.force_spawn_failure.store(true, Ordering::Release); + reg.start_thread("k", WorkerConfig::default(), |_cancel| {}); + assert!( + !reg.is_running("k"), + "failed spawn clears the running flag, never leaves it wedged" + ); + assert!(reg.any_alive(), "live prior re-installed, never detached"); + + // Recover: release the prior; quiesce reaps the now-finished handle + // cleanly, proving it was owned (not leaked/detached) and the slot is + // not wedged. + reg.force_spawn_failure.store(false, Ordering::Release); + release_tx.send(()).unwrap(); + assert_eq!(reg.quiesce("k").await, WorkerStatus::Ok); + assert!(!reg.any_alive()); + } + + /// A thread-spawn failure must roll the slot back to its PRIOR config, not + /// leave the failed start's weight / drain / join_budget / generation + /// behind: the re-installed prior worker keeps its own teardown config for + /// a later quiesce/shutdown. + /// + /// Non-vacuous: against a partial rollback (only cancel/handle restored), + /// the slot would carry the failed start's join budget and the bumped + /// generation. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn spawn_failure_restores_prior_slot_config() { + let reg = ThreadRegistry::<&str>::new(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + + // gen-1 with a DISTINCTIVE (non-default) join budget. Wedged so it + // stays the live prior after cancel. + let cfg1 = WorkerConfig { + join_budget: Duration::from_secs(11), + ..WorkerConfig::default() + }; + reg.start_thread("k", cfg1, wedged_body(release_rx)); + reg.cancel("k"); + let gen_after_gen1 = reg.lock_slots().get("k").unwrap().generation; + + // Failed restart with a DIFFERENT config; the rollback must discard it. + reg.force_spawn_failure.store(true, Ordering::Release); + let cfg2 = WorkerConfig { + join_budget: Duration::from_secs(99), + ..WorkerConfig::default() + }; + reg.start_thread("k", cfg2, |_cancel| {}); + reg.force_spawn_failure.store(false, Ordering::Release); + + { + let slots = reg.lock_slots(); + let slot = slots.get("k").expect("slot present"); + assert_eq!( + slot.join_budget, + Duration::from_secs(11), + "join_budget restored to prior" + ); + assert_eq!( + slot.generation, gen_after_gen1, + "generation rolled back to its pre-bump value" + ); + assert!( + slot.cancel.is_none(), + "running flag cleared after failed spawn" + ); + assert!( + slot.handle.is_some(), + "prior handle re-installed (alive), not detached" + ); + } + assert!(reg.any_alive(), "live prior still accounted for"); + + // Recover: release + quiesce reaps the prior cleanly. + release_tx.send(()).unwrap(); + assert_eq!(reg.quiesce("k").await, WorkerStatus::Ok); + assert!(!reg.any_alive()); + } + + /// A panicking worker body still runs its epilogue (via the drop-guard), + /// so `is_running()` reflects the crash and `start()` can relaunch the + /// loop instead of silently no-op'ing. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn panicked_worker_clears_running_and_allows_restart() { + let reg = ThreadRegistry::<&str>::new(); + // A worker whose body panics immediately. + reg.start_thread("k", WorkerConfig::default(), |_cancel| { + panic!("deliberate worker-body panic"); + }); + + // The drop-guard epilogue clears the running flag despite the panic. + let mut waited = Duration::ZERO; + while reg.is_running("k") && waited < Duration::from_secs(2) { + tokio::time::sleep(Duration::from_millis(5)).await; + waited += Duration::from_millis(5); + } + assert!( + !reg.is_running("k"), + "panicked worker clears its running flag via the epilogue guard" + ); + + // start() can relaunch a crashed loop. + let ran = Arc::new(AtomicBool::new(false)); + let ran_w = Arc::clone(&ran); + let handle = Handle::current(); + reg.start_thread("k", WorkerConfig::default(), move |cancel| { + ran_w.store(true, Ordering::Release); + handle.block_on(async move { cancel.cancelled().await }); + }); + assert!( + reg.is_running("k"), + "start() relaunches a previously-panicked worker" + ); + assert_eq!(reg.quiesce("k").await, WorkerStatus::Ok); + assert!( + ran.load(Ordering::Acquire), + "restarted worker body executed" + ); + } + + /// `shutdown()` latches the registry closed: a start racing (or + /// following) teardown is refused, so no worker is left un-joined. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn shutdown_latches_closed_refusing_new_workers() { + let reg = ThreadRegistry::<&str>::new(); + start_clean(®, "live", WorkerConfig::default()); + let report = reg.shutdown().await; + assert!(report.all_clean()); + + // One-way door: a start after shutdown is refused. + start_clean(®, "late_thread", WorkerConfig::default()); + assert!( + !reg.is_running("late_thread"), + "start_thread after shutdown is refused" + ); + assert!(!reg.any_alive(), "nothing started post-shutdown"); + } + + /// `start_thread` must park a restarted key's still-wedged prior into the + /// orphan list UNDER the slot lock — at the START of the restart, not only + /// after the out-of-lock reap backstop elapses. + /// Otherwise a `shutdown()` that snapshots tiers in the window between + /// "prior taken out of the slot" and "prior parked" sees neither the + /// prior (already moved out of the slot) nor an orphan, and reports + /// clean while the wedged prior is still live and un-joined. + /// + /// Deterministic via a long backstop: parking under the slot lock makes + /// the prior observable in orphans well before the backstop could elapse, + /// so the early assertion lands. Parking only at the end of the + /// out-of-lock spin would fail it. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn start_thread_parks_wedged_prior_under_slot_lock_at_restart() { + // Long backstop so the under-lock parking is observable well before + // it could possibly elapse. + let reg = ThreadRegistry::with_reap_backstop(Duration::from_secs(10)); + let (release_tx, release_rx) = mpsc::channel::<()>(); + + // gen-1: wedged (ignores cancel), stays live until released. + reg.start_thread("k", WorkerConfig::default(), wedged_body(release_rx)); + reg.cancel("k"); + + // gen-2 restart on a blocking thread: its bounded reap of the wedged + // gen-1 spins the (long) backstop, so start_thread does not return + // promptly. gen-1 is parked under the slot lock at the start of + // this call, before that spin. + let reg2 = Arc::clone(®); + let parent = Handle::current(); + let restart = tokio::task::spawn_blocking(move || { + let handle = parent.clone(); + reg2.start_thread("k", WorkerConfig::default(), move |cancel| { + handle.block_on(async move { cancel.cancelled().await }); + }); + }); + + // The wedged prior must appear in orphans far sooner than the 10s + // backstop — it was parked under the slot lock at restart. + let mut waited = Duration::ZERO; + while orphan_len(®) == 0 && waited < Duration::from_secs(2) { + tokio::time::sleep(Duration::from_millis(10)).await; + waited += Duration::from_millis(10); + } + assert_eq!( + orphan_len(®), + 1, + "wedged prior must be parked under the slot lock at restart, not \ + only after the backstop spin" + ); + assert!(reg.is_running("k"), "gen-2 installed under the same lock"); + + // Release the wedged prior: the restart's bounded reap then finds it + // finished, removes it from orphans, and joins it. + release_tx.send(()).unwrap(); + restart.await.unwrap(); + assert_eq!( + orphan_len(®), + 0, + "finished prior removed from orphans by the bounded reap" + ); + + // gen-2 quiesces cleanly. + assert_eq!(reg.quiesce("k").await, WorkerStatus::Ok); + } + + /// `with_reap_backstop` MUST emit a one-shot `tracing::warn!` when + /// compiled under `panic = "abort"` so an operator can audit the + /// orphan-liveness-gate risk documented on `EpilogueGuard`. + /// + /// Aspirational / manual-only: the standard `cargo test` profile is + /// `panic = "unwind"`, so this test is cfg-compiled OUT of every normal CI + /// run. It exercises the warn path only under a deliberate + /// `RUSTFLAGS="-C panic=abort"` build (mirroring the iOS release profile); + /// treat it as a local audit tool, not a signal CI enforces on its own. + /// + /// Functional assertion is on the process-wide `Once` latch, which is + /// the most reliable artifact we can probe without subscribing to + /// tracing from a `#[test]`. + #[cfg(panic = "abort")] + #[test] + fn with_reap_backstop_emits_panic_abort_warn_under_abort_builds() { + let _reg = ThreadRegistry::<&'static str>::with_reap_backstop(Duration::from_secs(1)); + assert!( + super::PANIC_ABORT_WARNED.is_completed(), + "with_reap_backstop must trip the panic=abort warn latch on first call" + ); + // Second construction must NOT re-fire — `Once` guarantees this, but + // we exercise it to lock the one-shot contract into the test. + let _reg2 = ThreadRegistry::<&'static str>::with_reap_backstop(Duration::from_secs(1)); + assert!(super::PANIC_ABORT_WARNED.is_completed()); + } + + /// Sentinel for the no-op cfg branch: under `panic = "unwind"` (the + /// dev-profile default) `EpilogueGuard`'s `Drop` runs and releases the + /// orphan slot, so the operator warn is unnecessary. This test just + /// proves the unwind branch compiles and `with_reap_backstop` keeps + /// behaving like a plain constructor — no observable warn-related state + /// to assert because the gated `static` doesn't exist on this build. + #[cfg(not(panic = "abort"))] + #[test] + fn with_reap_backstop_no_warn_under_unwind() { + let reg = ThreadRegistry::<&'static str>::with_reap_backstop(Duration::from_millis(250)); + assert!(!reg.any_alive(), "fresh registry has no live workers"); + } + + // ----- Group: register_thread (join/status-only, token-less) ------ + + /// `is_closing` reflects the one-way teardown latch: `false` on a fresh + /// registry, `true` once `shutdown` has begun. Consumers gate their own + /// out-of-registry `start` on it so they never spawn a loop teardown has + /// stopped waiting for. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn is_closing_tracks_shutdown_latch() { + let reg = ThreadRegistry::<&str>::new(); + assert!(!reg.is_closing(), "fresh registry is not closing"); + assert!(reg.shutdown().await.all_clean()); + assert!( + reg.is_closing(), + "shutdown latched the registry closed (one-way)" + ); + } +} diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 44532de8638..54a18c27832 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -172,6 +172,16 @@ pub enum PlatformWalletFFIResultCode { /// rejected the transaction, so its UTXO reservation was released and the /// host may safely retry after addressing the rejection reason. ErrorTransactionBroadcastRejected = 26, + /// A quiesce/drain barrier did not complete within its budget: an + /// in-flight sync pass was still running when a Clear / reset / + /// sync-stop needed it provably drained. The operation failed closed + /// (no state was wiped) and the host should retry once sync is idle. + /// NOT returned by `platform_wallet_manager_destroy` — with owned + /// callback contexts (`release_fn`) a straggling worker keeps its + /// context alive and releases it on exit, so destroy logs a non-clean + /// join instead of erroring. Swift mirror: + /// `PlatformWalletResultCode.errorShutdownIncomplete`. + ErrorShutdownIncomplete = 27, NotFound = 98, // Used exclusively for all the Option that are retuned as errors ErrorUnknown = 99, @@ -336,6 +346,12 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::AssetLockFundingMismatch { .. } => { PlatformWalletFFIResultCode::ErrorAssetLockFundingMismatch } + // A quiesce/drain barrier that did not complete within budget + // (clear/reset paths). The host must fail closed: keep its + // callback context alive and skip any paired persistence wipe. + PlatformWalletError::ShutdownIncomplete(..) => { + PlatformWalletFFIResultCode::ErrorShutdownIncomplete + } _ => PlatformWalletFFIResultCode::ErrorUnknown, }; PlatformWalletFFIResult::err(code, error.to_string()) diff --git a/packages/rs-platform-wallet-ffi/src/event_handler.rs b/packages/rs-platform-wallet-ffi/src/event_handler.rs index 1213eece587..79830b91635 100644 --- a/packages/rs-platform-wallet-ffi/src/event_handler.rs +++ b/packages/rs-platform-wallet-ffi/src/event_handler.rs @@ -91,6 +91,24 @@ pub struct EventHandlerCallbacks { pub on_shielded_tree_progress_fn: Option< unsafe extern "C" fn(context: *mut c_void, leaves_committed: u64, total_target: u64), >, + /// Destructor for `context`, called by Rust **exactly once** when the + /// last internal reference to this vtable drops — that is, when the + /// manager and every background worker that can still dispatch an + /// event have finished. Appended at the END so the struct layout + /// stays stable. + /// + /// Setting this transfers ownership of `context` to Rust: the host + /// hands over a strong reference (Swift `Unmanaged.passRetained`, JNI + /// a boxed `GlobalRef`) and must NOT free the context itself. The + /// callback may fire on any thread. + /// + /// **Required whenever `context` is non-null** — + /// `platform_wallet_manager_create` rejects a context-carrying vtable + /// without a destructor, because `destroy` returns without proving + /// every worker joined and only ownership keeps a straggler's + /// callbacks memory-safe. A context needing no cleanup takes a no-op + /// `release_fn`; `None` is valid only alongside a null `context`. + pub release_fn: Option, } // SAFETY: The context pointer is managed by the FFI caller who must ensure @@ -109,6 +127,26 @@ impl FFIEventHandler { } } +/// Releases the host callback context when the handler's last owner drops. +/// The handler is constructed exactly once per manager into the +/// `Arc` every event dispatcher clones, so this +/// `Drop` runs exactly once — after the manager and every worker that +/// could still fire an event (including a straggler that outlived a +/// non-clean shutdown) are done. See [`FFIPersister`]'s `Drop` for the +/// full ownership rationale. +/// +/// [`FFIPersister`]: crate::persistence::FFIPersister +impl Drop for FFIEventHandler { + fn drop(&mut self) { + if let Some(release) = self.callbacks.release_fn { + // SAFETY: `release_fn` was supplied together with `context` by + // the host, which contracted for exactly one call on any + // thread. This is the only call site and `Drop` runs once. + unsafe { release(self.callbacks.context) }; + } + } +} + // SAFETY: Same as EventHandlerCallbacks. unsafe impl Send for FFIEventHandler {} unsafe impl Sync for FFIEventHandler {} @@ -272,3 +310,44 @@ impl PlatformEventHandler for FFIEventHandler { } } } + +#[cfg(test)] +mod release_tests { + use super::*; + + /// Mirror of the persister-level test: the event context is released + /// exactly once, only when the last `Arc` + /// clone drops — so a straggling event dispatcher keeps the host + /// handler alive rather than firing into freed memory. + #[test] + fn release_fires_once_when_the_last_arc_clone_drops() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + unsafe extern "C" fn count_release(context: *mut c_void) { + if let Some(counter) = (context as *const AtomicUsize).as_ref() { + counter.fetch_add(1, Ordering::SeqCst); + } + } + + let releases = Box::leak(Box::new(AtomicUsize::new(0))); + let handler: Arc = + Arc::new(FFIEventHandler::new(EventHandlerCallbacks { + context: releases as *const AtomicUsize as *mut c_void, + on_wallet_event_fn: None, + on_error_fn: None, + on_platform_address_sync_completed_fn: None, + on_shielded_sync_completed_fn: None, + on_shielded_sync_progress_fn: None, + on_shielded_tree_progress_fn: None, + release_fn: Some(count_release), + })); + let straggler = Arc::clone(&handler); + + drop(handler); + assert_eq!(releases.load(Ordering::SeqCst), 0); + + drop(straggler); + assert_eq!(releases.load(Ordering::SeqCst), 1); + } +} diff --git a/packages/rs-platform-wallet-ffi/src/manager.rs b/packages/rs-platform-wallet-ffi/src/manager.rs index ed90edaad74..c3aba1491b1 100644 --- a/packages/rs-platform-wallet-ffi/src/manager.rs +++ b/packages/rs-platform-wallet-ffi/src/manager.rs @@ -40,8 +40,22 @@ fn persistence_capabilities_declaration( /// that cbindgen cannot expose without dragging the entire crate's /// internal layout into the C ABI. /// -/// `persistence` and `event_handler` are callback vtables whose `context` -/// pointers must remain valid for the lifetime of the manager. +/// `persistence` and `event_handler` are callback vtables. A vtable that +/// carries a non-null `context` MUST also set `release_fn` — creation +/// fails with `ErrorInvalidParameter` otherwise. Ownership of the context +/// then transfers to Rust: the manager keeps it alive for exactly as long +/// as any internal worker can still invoke a callback, and calls +/// `release_fn` once — possibly on a background thread, possibly *after* +/// a later `destroy` returns if a worker straggles — when the last +/// reference drops. A borrowed (non-null context, null `release_fn`) +/// vtable is rejected rather than accepted-and-hoped-for, because nothing +/// but ownership can make a straggling worker safe: `destroy` returns +/// `Success` without proving quiescence, so a borrowed context would be +/// freeable by the host while a straggler can still call through it. A +/// host whose context needs no cleanup passes a no-op `release_fn`. +/// +/// On a non-`Success` return Rust has NOT taken ownership of either +/// context; a host that pre-retained them must release them itself. #[no_mangle] pub unsafe extern "C" fn platform_wallet_manager_create( sdk_ptr: *const c_void, @@ -94,6 +108,33 @@ unsafe fn platform_wallet_manager_create_impl( check_ptr!(event_handler); check_ptr!(out_handle); + // Ownership is mandatory for a context-carrying vtable: `destroy` + // returns `Success` without proving every worker joined, which is only + // sound because a straggler's `Arc` keeps the host context alive via + // `release_fn`. A borrowed context (non-null, no destructor) would + // reintroduce the freed-context callback exactly on the non-clean + // path, so it is rejected up front instead of accepted unsafely. + if !(*persistence).context.is_null() && (*persistence).release_fn.is_none() { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + "persistence callbacks carry a context but no release_fn; the manager owns \ + callback contexts (released when its last worker drops) and cannot accept \ + a borrowed context — pass a release_fn (a no-op one if the context needs \ + no cleanup)" + .to_string(), + ); + } + if !(*event_handler).context.is_null() && (*event_handler).release_fn.is_none() { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + "event-handler callbacks carry a context but no release_fn; the manager owns \ + callback contexts (released when its last worker drops) and cannot accept \ + a borrowed context — pass a release_fn (a no-op one if the context needs \ + no cleanup)" + .to_string(), + ); + } + let sdk = Arc::new((*(sdk_ptr as *const Sdk)).clone()); let persister = Arc::new(FFIPersister::new_with_persistence_capabilities( std::ptr::read(persistence), @@ -422,20 +463,45 @@ pub unsafe extern "C" fn platform_wallet_manager_get_wallet( } /// Destroy a PlatformWalletManager handle. +/// +/// Runs the full lifecycle shutdown (bounded: quiesce + join every +/// coordinator, SPV, the payment-hook tasks, and the event adapter) and +/// removes the handle. Always returns `Success` for a live handle. +/// +/// A non-clean shutdown — a worker that outlived its join budget — is +/// logged, **not** surfaced as an error, because it is no longer a +/// safety problem the host could act on: a straggling worker holds a +/// strong reference to the callback vtables, and creation guarantees +/// every context-carrying vtable is owned (`release_fn` required), so +/// the host objects stay alive until that worker exits, at which point +/// Rust releases them. Nothing dangles, nothing needs a retry, nothing +/// needs a deliberate leak on the host side. #[no_mangle] pub unsafe extern "C" fn platform_wallet_manager_destroy( handle: Handle, ) -> PlatformWalletFFIResult { if let Some(manager) = PLATFORM_WALLET_MANAGER_STORAGE.remove(handle) { // Run the full lifecycle shutdown to completion, not just the - // platform-address sync. Every background task (identity sync, - // shielded sync, the wallet-event adapter) can fire callbacks - // through the host-owned `context` pointer; once `destroy` - // returns the host may free that context, so no task may be - // left alive to fire a callback against freed memory. - // `shutdown()` is idempotent, so this is safe even if the host - // already stopped some sync managers before calling destroy. - runtime().block_on(manager.shutdown()); + // platform-address sync. `shutdown()` is idempotent, so this is + // safe even if the host already stopped some sync managers + // before calling destroy. + let report = runtime().block_on(manager.shutdown()); + if !report.all_clean() { + // A worker panicked, exceeded its join budget, or stayed + // detached. Its persister/event-handler Arcs keep the host + // callback contexts alive until it actually exits, so this + // is diagnostic, not a UAF hazard. + tracing::warn!( + ?report, + "platform wallet manager shutdown did not join every worker \ + cleanly; stragglers keep their callback contexts alive and \ + release them on exit" + ); + } + // Dropping the manager here releases its persister/event-handler + // references; the host contexts are released (via `release_fn`) + // as soon as the last worker's reference drops — typically right + // now, or later if a straggler is still draining. } PlatformWalletFFIResult::ok() } @@ -504,9 +570,144 @@ mod tests { on_shielded_sync_completed_fn: None, on_shielded_sync_progress_fn: None, on_shielded_tree_progress_fn: None, + release_fn: None, + } + } + + /// Counts invocations through a `*mut AtomicUsize` context — stands in + /// for the host's release trampoline. + unsafe extern "C" fn counting_release(context: *mut c_void) { + if let Some(counter) = (context as *const std::sync::atomic::AtomicUsize).as_ref() { + counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); } } + /// A context-carrying vtable without a `release_fn` must be rejected + /// at creation. `destroy` returns `Success` without proving every + /// worker joined; that is only sound because a straggler's `Arc` + /// keeps the host context alive via ownership. Accepting a borrowed + /// context would let a legacy caller free it after a "successful" + /// destroy while a straggler can still call through it — the exact + /// use-after-free this FFI exists to prevent. + #[test] + fn create_rejects_context_carrying_vtable_without_release_fn() { + let sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + let sentinel = 0xC0FFEEusize as *mut c_void; + + // Persistence vtable with a context but no destructor. + let mut callbacks = persistence_callbacks(); + callbacks.context = sentinel; + let event_cbs = event_callbacks(); + let mut handle = 0; + let result = unsafe { + platform_wallet_manager_create( + &sdk as *const Sdk as *const c_void, + &callbacks, + &event_cbs, + &mut handle, + ) + }; + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorInvalidParameter, + "borrowed persistence context must be rejected" + ); + + // Event vtable with a context but no destructor. + let callbacks = persistence_callbacks(); + let mut event_cbs = event_callbacks(); + event_cbs.context = sentinel; + let result = unsafe { + platform_wallet_manager_create( + &sdk as *const Sdk as *const c_void, + &callbacks, + &event_cbs, + &mut handle, + ) + }; + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorInvalidParameter, + "borrowed event context must be rejected" + ); + + // Null contexts stay valid without a destructor (the + // `configure(modelContainer: nil)` shape). + let callbacks = persistence_callbacks(); + let event_cbs = event_callbacks(); + let result = unsafe { + platform_wallet_manager_create( + &sdk as *const Sdk as *const c_void, + &callbacks, + &event_cbs, + &mut handle, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::Success); + let result = unsafe { platform_wallet_manager_destroy(handle) }; + assert_eq!(result.code, PlatformWalletFFIResultCode::Success); + } + + /// The owned-context contract end to end through the public FFI: when + /// both vtables set `release_fn`, destroying the manager releases each + /// context exactly once — after shutdown has joined every worker, so + /// the release IS the proof that nothing can call back into the host + /// anymore. This is the contract that lets Swift `passRetained` / + /// JNI box-transfer their callback objects instead of leaking them + /// whenever teardown is not provably clean. + #[test] + fn destroy_releases_owned_callback_contexts_exactly_once() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + let persistence_releases = AtomicUsize::new(0); + let event_releases = AtomicUsize::new(0); + + let sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + let mut callbacks = persistence_callbacks(); + callbacks.context = &persistence_releases as *const AtomicUsize as *mut c_void; + callbacks.release_fn = Some(counting_release); + let mut event_cbs = event_callbacks(); + event_cbs.context = &event_releases as *const AtomicUsize as *mut c_void; + event_cbs.release_fn = Some(counting_release); + + let mut handle = 0; + let result = unsafe { + platform_wallet_manager_create( + &sdk as *const Sdk as *const c_void, + &callbacks, + &event_cbs, + &mut handle, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::Success); + assert_eq!( + persistence_releases.load(Ordering::SeqCst), + 0, + "context must stay alive while the manager lives" + ); + assert_eq!(event_releases.load(Ordering::SeqCst), 0); + + let result = unsafe { platform_wallet_manager_destroy(handle) }; + assert_eq!(result.code, PlatformWalletFFIResultCode::Success); + + assert_eq!( + persistence_releases.load(Ordering::SeqCst), + 1, + "destroy must release the persistence context exactly once" + ); + assert_eq!( + event_releases.load(Ordering::SeqCst), + 1, + "destroy must release the event context exactly once" + ); + + // Destroying a stale handle must not double-release. + let result = unsafe { platform_wallet_manager_destroy(handle) }; + assert_eq!(result.code, PlatformWalletFFIResultCode::Success); + assert_eq!(persistence_releases.load(Ordering::SeqCst), 1); + assert_eq!(event_releases.load(Ordering::SeqCst), 1); + } + fn query(handle: Handle) -> PersistenceCapabilitiesFFI { let mut out = PersistenceCapabilitiesFFI { version: 0, diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index e2e776b4544..be30d937dd0 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -662,6 +662,24 @@ pub struct PersistenceCallbacks { removed_count: usize, ) -> i32, >, + /// Destructor for `context`, called by Rust **exactly once** when the + /// last internal reference to this vtable drops — that is, when the + /// manager *and every background worker that cloned its persister* + /// have finished. Appended at the END so the struct layout stays + /// stable. + /// + /// Setting this transfers ownership of `context` to Rust: the host + /// hands over a strong reference (Swift `Unmanaged.passRetained`, JNI + /// a boxed `GlobalRef`) and must NOT free the context itself. The + /// callback may fire on any thread. + /// + /// **Required whenever `context` is non-null** — + /// `platform_wallet_manager_create` rejects a context-carrying vtable + /// without a destructor, because `destroy` returns without proving + /// every worker joined and only ownership keeps a straggler's + /// callbacks memory-safe. A context needing no cleanup takes a no-op + /// `release_fn`; `None` is valid only alongside a null `context`. + pub release_fn: Option, } // SAFETY: The context pointer is managed by the FFI caller who must ensure @@ -725,6 +743,7 @@ impl Default for PersistenceCallbacks { on_load_shielded_viewing_keys_fn: None, #[cfg(feature = "shielded")] on_load_shielded_viewing_keys_free_fn: None, + release_fn: None, } } } @@ -791,6 +810,29 @@ pub struct FFIPersister { round_lock: Mutex, } +/// Releases the host callback context when the persister's last owner +/// drops. The persister is constructed exactly once per manager +/// ([`platform_wallet_manager_create`]) into an `Arc` that the manager and +/// every background worker clone — so this `Drop` runs exactly once, after +/// the manager AND all of its workers (including any straggler that +/// outlived a non-clean shutdown) are provably done calling back into the +/// host. That converts the old borrowed-context contract — where the host +/// had to keep the callback object alive past `destroy` "just in case" or +/// deliberately leak it — into plain ownership: the host hands Rust a +/// strong reference and Rust frees it when nothing can touch it anymore. +/// +/// [`platform_wallet_manager_create`]: crate::manager::platform_wallet_manager_create +impl Drop for FFIPersister { + fn drop(&mut self) { + if let Some(release) = self.callbacks.release_fn { + // SAFETY: `release_fn` was supplied together with `context` by + // the host, which contracted for exactly one call on any + // thread. This is the only call site and `Drop` runs once. + unsafe { release(self.callbacks.context) }; + } + } +} + impl FFIPersister { pub fn new(callbacks: PersistenceCallbacks) -> Self { Self::new_with_persistence_capabilities(callbacks, PersistenceCapabilities::NONE) @@ -5550,6 +5592,48 @@ mod tests { ) { } + /// The owned-context contract at the persister level: the host context + /// is released exactly once, and only when the LAST `Arc` clone drops. + /// A worker that outlives the manager (the straggler `destroy` used to + /// have to defend against with a deliberate host-side leak) therefore + /// keeps the host callback object alive for exactly as long as it can + /// still call into it, and frees it on exit — no earlier, no twice. + #[test] + fn release_fires_once_when_the_last_arc_clone_drops() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + unsafe extern "C" fn count_release(context: *mut c_void) { + if let Some(counter) = (context as *const AtomicUsize).as_ref() { + counter.fetch_add(1, Ordering::SeqCst); + } + } + + let releases = Box::leak(Box::new(AtomicUsize::new(0))); + let callbacks = PersistenceCallbacks { + context: releases as *const AtomicUsize as *mut c_void, + release_fn: Some(count_release), + ..PersistenceCallbacks::default() + }; + + let persister = Arc::new(FFIPersister::new(callbacks)); + let straggler = Arc::clone(&persister); + + drop(persister); + assert_eq!( + releases.load(Ordering::SeqCst), + 0, + "a live straggler clone must keep the host context alive" + ); + + drop(straggler); + assert_eq!( + releases.load(Ordering::SeqCst), + 1, + "the last clone dropping must release the host context exactly once" + ); + } + /// A callback-free persister (the `configure(modelContainer: nil)` shape) /// silently drops every write, so it must NOT attest durability — this is /// the concrete fail-open hole the fail-closed default exists to catch: @@ -5674,21 +5758,21 @@ mod tests { assert_eq!(ffi.bits, 0x81); assert_eq!(std::mem::size_of::(), 16); // Capability negotiation is deliberately NOT appended to the legacy - // callback vtable. Pin its historical size and prove invitations remain - // the terminal field so old clients are never over-read. + // callback vtable. Pin the vtable size (invitations + the appended + // `release_fn` context destructor) and prove `release_fn` is the + // terminal field so old clients are never over-read past it. #[cfg(not(feature = "shielded"))] assert_eq!( std::mem::size_of::(), - 21 * std::mem::size_of::() + 22 * std::mem::size_of::() ); #[cfg(feature = "shielded")] assert_eq!( std::mem::size_of::(), - 37 * std::mem::size_of::() + 38 * std::mem::size_of::() ); assert_eq!( - std::mem::offset_of!(PersistenceCallbacks, on_persist_invitations_fn) - + std::mem::size_of::(), + std::mem::offset_of!(PersistenceCallbacks, release_fn) + std::mem::size_of::(), std::mem::size_of::() ); assert_eq!( diff --git a/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs b/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs index e8fcf9febcb..dc9893a8869 100644 --- a/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs @@ -230,6 +230,16 @@ pub unsafe extern "C" fn platform_wallet_manager_platform_address_sync_reset( }); let result = unwrap_option_or_return!(option); if let Err(e) = result { + // Mirrors `platform_wallet_manager_shielded_clear`: an incomplete + // drain is surfaced with its own code so the host can distinguish + // "callback-capable work is still running" from an ordinary reset + // failure. + if matches!( + e, + platform_wallet::PlatformWalletError::ShutdownIncomplete(_) + ) { + return PlatformWalletFFIResult::from(e); + } return PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorWalletOperation, format!("reset_platform_address_sync_state failed: {e}"), diff --git a/packages/rs-platform-wallet-ffi/src/shielded_sync.rs b/packages/rs-platform-wallet-ffi/src/shielded_sync.rs index abba216142e..8b41f702b8e 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_sync.rs @@ -78,14 +78,32 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_start( /// load-bearing part; hosts that must ignore a trailing UI event should /// gate their handler on their own post-stop/post-clear state (the /// example app drops events while unbound). +/// +/// **Bounded**: the drain waits at most the coordinator quiesce budget. +/// If the in-flight pass is wedged past that deadline this returns +/// `ErrorShutdownIncomplete` instead of a false success — the pass may +/// still fire persistence/completion callbacks, so the host must keep +/// its callback context alive and must not treat sync as stopped. #[no_mangle] pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_stop( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(manager.shielded_sync().quiesce()); + runtime().block_on(manager.shielded_sync().quiesce()) }); - unwrap_option_or_return!(option); + let drained = unwrap_option_or_return!(option); + if !drained { + // The in-flight pass did not drain within the quiesce budget — + // it may still fire persistence / completion callbacks. Surface + // that instead of a silent success so the host keeps its callback + // context alive and does not treat sync as stopped. + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorShutdownIncomplete, + "shielded sync pass did not drain within the quiesce budget; \ + a pass may still be running" + .to_string(), + ); + } PlatformWalletFFIResult::ok() } @@ -418,6 +436,18 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_clear( }); let result = unwrap_option_or_return!(option); if let Err(e) = result { + // A drain that did not complete is NOT an ordinary store failure: + // it means callback-capable work may still be running, which the + // host must be able to tell apart (it keeps its callback context + // alive rather than just retrying the wipe). Route that one case + // through the typed conversion and keep the generic mapping for + // every other failure. + if matches!( + e, + platform_wallet::PlatformWalletError::ShutdownIncomplete(_) + ) { + return PlatformWalletFFIResult::from(e); + } return PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorWalletOperation, format!("clear_shielded failed: {e}"), diff --git a/packages/rs-platform-wallet/Cargo.toml b/packages/rs-platform-wallet/Cargo.toml index 05d4f933086..5e72b3c2deb 100644 --- a/packages/rs-platform-wallet/Cargo.toml +++ b/packages/rs-platform-wallet/Cargo.toml @@ -31,6 +31,7 @@ bimap = "0.6" # Async runtime tokio = { version = "1", features = ["sync", "rt", "time", "macros"] } tokio-util = { version = "0.7.12" } +dash-async = { path = "../rs-dash-async" } # Logging tracing = "0.1" diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 6e11514beaa..6f34b0cee70 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -330,6 +330,16 @@ pub enum PlatformWalletError { #[error("Shielded sync failed: {0}")] ShieldedSyncFailed(String), + /// A background sync pass did not drain within its quiesce budget, so + /// the operation that required a "no more persister stores" barrier + /// (manager shutdown, `clear_shielded`, a sync-state reset) aborted + /// fail-closed. The wedged pass may still fire persistence / event + /// callbacks; the host must keep its callback context alive and must + /// not commit any wipe it was about to pair with this call. + /// FFI mirror: `PlatformWalletFFIResultCode::ErrorShutdownIncomplete`. + #[error("Background sync did not quiesce: {0}")] + ShutdownIncomplete(String), + #[error("Shielded commitment tree update failed: {0}")] ShieldedTreeUpdateFailed(String), diff --git a/packages/rs-platform-wallet/src/manager/dashpay_sync.rs b/packages/rs-platform-wallet/src/manager/dashpay_sync.rs index 00c861dee72..3ffdf2fdd1c 100644 --- a/packages/rs-platform-wallet/src/manager/dashpay_sync.rs +++ b/packages/rs-platform-wallet/src/manager/dashpay_sync.rs @@ -45,6 +45,7 @@ //! entry points stay available for pull-to-refresh. use std::collections::BTreeMap; +use std::num::NonZeroUsize; use std::sync::{ atomic::{AtomicBool, AtomicU64, Ordering}, Arc, @@ -53,8 +54,13 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tokio::sync::RwLock; +use dash_async::{ThreadRegistry, WorkerConfig}; + use crate::error::PlatformWalletError; -use crate::manager::loop_cancel::LoopCancelGuard; +use crate::manager::{ + coordinator_worker_config, drain_pass, QuiesceGate, QuiesceGuard, SyncSlotGuard, WalletWorker, + COORDINATOR_DRAIN_BUDGET, +}; use crate::wallet::platform_wallet::WalletId; use crate::wallet::PlatformWallet; @@ -67,6 +73,17 @@ use crate::wallet::PlatformWallet; /// traffic by 4. Tunable at runtime via [`DashPaySyncManager::set_interval`]. pub const DEFAULT_SYNC_INTERVAL_SECS: u64 = 15; +/// Stack size for the DashPay sync loop's OS thread. +/// +/// DashPay sync verifies GroveDB *document-query* proofs (contactRequest / +/// profile fetches), whose recursive `verify_layer_proof_v1` descent +/// overflows the platform default thread stack (SIGBUS on the stack guard, +/// observed on-device 2026-06-12). The sibling sync loops survive on the +/// default only because their proofs are shallower. Matches the FFI worker +/// convention (`runtime.rs` WORKER_STACK_BYTES) since `Handle::block_on` +/// polls the future on the registry's worker thread. +const DASHPAY_SYNC_STACK_BYTES: usize = 8 * 1024 * 1024; + /// Outcome of syncing a single wallet's DashPay state in a pass. #[derive(Debug)] pub enum WalletDashPaySyncOutcome { @@ -116,30 +133,36 @@ impl DashPaySyncSummary { /// token registry, so DashPay-only identities are never skipped. pub struct DashPaySyncManager { wallets: Arc>>>, - /// Generation-guarded cancel-token slot for the background loop — - /// see [`LoopCancelGuard`] for the stale-loop shutdown invariant. - cancel_guard: LoopCancelGuard, + /// Shared registry that owns this loop's lifecycle: it spawns the + /// OS thread (with the deep-stack config below), owns its cancellation + /// token, and joins it at shutdown. A generation-guarded slot handles a + /// `stop()` + quick `start()` without a stale loop clobbering the new one. + registry: Arc>, interval_secs: AtomicU64, is_syncing: AtomicBool, - /// Set by [`quiesce`](Self::quiesce) to gate new passes while it - /// drains an in-flight one. `sync_now` bails (after taking the - /// `is_syncing` slot) when this is set, so once `quiesce` observes + /// Gates new passes while a [`quiesce`](Self::quiesce) drains an + /// in-flight one, while a [`QuiesceGuard`] holder mutates state, and + /// terminally once shutdown seals it. `sync_now` bails (after taking the + /// `is_syncing` slot) when it is closed, so once a drain observes /// `is_syncing == false` no further pass can start — giving shutdown /// a real "no more host-visible persister stores" barrier that /// cancel-only [`stop`](Self::stop) does not provide. - quiescing: AtomicBool, + quiescing: QuiesceGate, /// Unix seconds of the last completed pass. `0` = never. last_sync_unix: AtomicU64, } impl DashPaySyncManager { - pub fn new(wallets: Arc>>>) -> Self { + pub fn new( + wallets: Arc>>>, + registry: Arc>, + ) -> Self { Self { wallets, - cancel_guard: LoopCancelGuard::new(), + registry, interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS), is_syncing: AtomicBool::new(false), - quiescing: AtomicBool::new(false), + quiescing: QuiesceGate::default(), last_sync_unix: AtomicU64::new(0), } } @@ -159,7 +182,7 @@ impl DashPaySyncManager { /// Whether the background loop is currently running. pub fn is_running(&self) -> bool { - self.cancel_guard.is_running() + self.registry.is_running(WalletWorker::DashPaySync) } /// Whether a sync pass is in flight right now. @@ -190,44 +213,41 @@ impl DashPaySyncManager { /// /// The first pass runs immediately; subsequent passes fire every /// [`interval`](Self::interval). + /// + /// **Blocks briefly on restart**: the shared registry synchronously + /// reaps a still-draining prior-generation thread, spinning up to the + /// registry reap backstop (default 1 s) before returning. Call it from + /// the FFI host thread, not an async task. pub fn start(self: Arc) { - let Some((cancel, my_generation)) = self.cancel_guard.install() else { - return; - }; - let handle = tokio::runtime::Handle::current(); + let registry = Arc::clone(&self.registry); let this = self; - std::thread::Builder::new() - .name("dashpay-sync".into()) - // DashPay sync verifies GroveDB *document-query* proofs - // (contactRequest / profile fetches), whose recursive - // `verify_layer_proof_v1` descent overflows the platform - // default thread stack (SIGBUS on the stack guard, observed - // on-device 2026-06-12). The sibling sync threads survive on - // the default only because their proofs are shallower; match - // the FFI worker convention (`runtime.rs` WORKER_STACK_BYTES) - // since `Handle::block_on` polls the future on THIS thread. - .stack_size(8 * 1024 * 1024) - .spawn(move || { - handle.block_on(async move { - loop { - if cancel.is_cancelled() { - break; - } - - this.sync_now().await; - - let interval = this.interval(); - tokio::select! { - _ = tokio::time::sleep(interval) => {} - _ = cancel.cancelled() => break, - } + // Deep stack for the GroveDB proof descent — see + // [`DASHPAY_SYNC_STACK_BYTES`]. The registry spawns the OS thread + // with this size and owns the whole lifecycle (see + // `IdentitySyncManager::start`): teardown latch, cancellation token, + // thread spawn, and prior-generation reap under one slot lock. + let cfg = WorkerConfig { + stack_size: NonZeroUsize::new(DASHPAY_SYNC_STACK_BYTES), + ..coordinator_worker_config() + }; + registry.start_thread(WalletWorker::DashPaySync, cfg, move |cancel| { + handle.block_on(async move { + loop { + if cancel.is_cancelled() { + break; } - this.cancel_guard.clear_if_current(my_generation); - }); - }) - .expect("failed to spawn dashpay-sync thread"); + this.sync_now().await; + + let interval = this.interval(); + tokio::select! { + _ = tokio::time::sleep(interval) => {} + _ = cancel.cancelled() => break, + } + } + }); + }); } /// Stop the background sync loop. No-op if not running. @@ -239,9 +259,7 @@ impl DashPaySyncManager { /// by manager shutdown so the host can free the persister context — /// use [`quiesce`](Self::quiesce). pub fn stop(&self) { - if let Some(token) = self.cancel_guard.take() { - token.cancel(); - } + self.registry.cancel(WalletWorker::DashPaySync); } /// Cancel the background loop **and wait for any in-flight sync pass @@ -254,7 +272,7 @@ impl DashPaySyncManager { /// persister context the FFI handed to us) cannot be raced by a pass /// that calls `persister.store(...)` through a now-dangling pointer. /// - /// Mechanism: set the `quiescing` gate so any pass that hasn't yet + /// Mechanism: close the `quiescing` gate so any pass that hasn't yet /// taken the `is_syncing` slot bails, cancel the loop, then wait for /// `is_syncing` to clear. `is_syncing` is held for the whole pass /// including the per-wallet persister fan-out (`sync_now` clears it @@ -262,13 +280,59 @@ impl DashPaySyncManager { /// falling edge (with the gate up) is a sound "fully drained" /// signal. The gate is reopened before returning so a later /// start/sync works normally. - pub async fn quiesce(&self) { - self.quiescing.store(true, Ordering::Release); - self.stop(); - while self.is_syncing.load(Ordering::Acquire) { - tokio::time::sleep(Duration::from_millis(20)).await; - } - self.quiescing.store(false, Ordering::Release); + /// + /// **Bounded** by `COORDINATOR_DRAIN_BUDGET`: returns `false` if + /// the in-flight pass did not drain in time — see + /// `quiesce_within` for the timeout contract. + #[must_use = "a false return means the pass did NOT drain; the caller must fail closed"] + pub async fn quiesce(&self) -> bool { + self.quiesce_within(COORDINATOR_DRAIN_BUDGET).await + } + + /// [`quiesce`](Self::quiesce) with an explicit drain budget. + /// + /// Returns `true` when the drain completed: no pass is running and + /// none can start until the next `start`/`sync_now`. Returns `false` + /// when `is_syncing` was still held at the deadline — a pass is + /// wedged (stalled network / persister / host-callback await). On + /// that path the `quiescing` gate is deliberately **left closed** so the + /// wedged pass cannot be followed by a fresh one; the caller must + /// treat the coordinator as non-clean (shutdown reports it, clear / + /// reset paths abort fail-closed). A later successful `quiesce` + /// reopens the gate. + pub(crate) async fn quiesce_within(&self, budget: Duration) -> bool { + // The guard drops here, reopening the gate — this is the + // "drain only" flavor. + self.quiesce_held_within(budget).await.is_some() + } + + /// [`quiesce_within`](Self::quiesce_within) that **keeps sync admission + /// shut** until the returned guard drops — the barrier a caller needs + /// when it mutates state a pass touches right after draining. + /// + /// `None` means the in-flight pass did not drain within `budget`; the + /// gate is left closed and the caller must fail closed. + #[must_use = "None means the pass did NOT drain; the caller must fail closed"] + pub(crate) async fn quiesce_held_within(&self, budget: Duration) -> Option> { + drain_pass(&self.quiescing, &self.is_syncing, || self.stop(), budget).await + } + + /// [`quiesce_within`](Self::quiesce_within) that **seals** the gate: + /// admission never reopens on this coordinator instance. + /// + /// Used by manager shutdown. Reopening there would let a direct + /// `sync_now` that was already dispatched on a host thread — the FFI + /// resolves the manager under a shared read guard, so it can be + /// mid-flight while `destroy` runs — start a fresh pass *after* the + /// drain concluded and fire persister / completion callbacks through + /// a context the host has since freed. + pub(crate) async fn quiesce_sealed_within(&self, budget: Duration) -> bool { + let guard = self.quiesce_held_within(budget).await; + let drained = guard.is_some(); + // Seal before the guard drops so its Drop cannot reopen. + self.quiescing.seal(); + drop(guard); + drained } /// Run one DashPay sync pass across every registered wallet. @@ -287,13 +351,15 @@ impl DashPaySyncManager { { return DashPaySyncSummary::default(); } + // Clears `is_syncing` on every exit path — including panic unwind — + // so a failed pass can never wedge `quiesce()`'s drain. + let _slot = SyncSlotGuard(&self.is_syncing); // A `quiesce()` may have raised the gate between our CAS and // here; if so, release the slot and bail without running a pass // so the drain can complete and shutdown gets a true barrier // (no further `persister.store(...)` after quiesce returns). - if self.quiescing.load(Ordering::Acquire) { - self.is_syncing.store(false, Ordering::Release); + if self.quiescing.is_closed() { return DashPaySyncSummary::default(); } @@ -327,8 +393,6 @@ impl DashPaySyncManager { summary.sync_unix_seconds = now; self.last_sync_unix.store(now, Ordering::Release); - self.is_syncing.store(false, Ordering::Release); - summary } @@ -643,11 +707,47 @@ mod tests { .await .expect("quiesce did not return after the pass drained"); - assert!(!mgr.quiescing.load(Ordering::Acquire)); + assert!(!mgr.quiescing.is_closed()); assert!(!mgr.is_syncing()); pass.await.unwrap(); } + /// A pass that never drains must NOT hang `quiesce_within` forever: + /// the drain returns `false` at its deadline and deliberately leaves + /// the `quiescing` gate closed (so the wedged pass cannot be followed by + /// a fresh one). A later successful quiesce reopens the gate. This is + /// the bound that keeps FFI `destroy` from blocking indefinitely on a + /// stalled network / persister await. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn quiesce_within_times_out_and_leaves_gate_up_when_pass_never_drains() { + let manager = make_manager(); + let mgr = manager.dashpay_sync_arc(); + + // Wedge: take the slot exactly as a real pass would and never + // release it (stands in for a pass stalled in an await). + assert!(mgr + .is_syncing + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok()); + + let drained = tokio::time::timeout( + Duration::from_secs(2), + mgr.quiesce_within(Duration::from_millis(100)), + ) + .await + .expect("bounded quiesce must return at its deadline"); + assert!(!drained, "a wedged pass must be reported as non-drained"); + assert!( + mgr.quiescing.is_closed(), + "gate must stay up after a timed-out drain" + ); + + // Release the wedge; the next quiesce drains and reopens the gate. + mgr.is_syncing.store(false, Ordering::Release); + assert!(mgr.quiesce().await); + assert!(!mgr.quiescing.is_closed()); + } + /// A `sync_now()` invoked while `quiescing` is set must bail without /// running the pass — the gate that prevents a pass slipping in /// between `quiesce`'s `stop()` and its drain. @@ -657,8 +757,9 @@ mod tests { let _wallet_id = register_test_wallet(&manager).await; let mgr = manager.dashpay_sync_arc(); - // Raise the gate as `quiesce()` would. - mgr.quiescing.store(true, Ordering::Release); + // Raise the gate as an in-flight `quiesce()` would (a drain holds + // the gate from its first instruction). + let gate_hold = mgr.quiescing.hold(); let summary = mgr.sync_now().await; @@ -666,75 +767,54 @@ mod tests { // released so a later (post-quiesce) pass can still run. assert!(summary.is_empty()); assert!(!mgr.is_syncing()); + drop(gate_hold); } - /// Regression: a stale, draining loop's cleanup must **not** clobber a - /// newer loop's cancel token. + /// Regression: a `stop()` + quick `start()` must leave the NEW loop + /// running and cancellable — a stale prior generation's exit epilogue + /// must not clobber the new generation's cancellation token. /// - /// The failure this pins is a use-after-free across the FFI persister. - /// `stop()` is cancel-only — it takes + cancels loop A's token but loop - /// A keeps draining its in-flight pass. A quick `start()` then installs - /// loop B's token. When loop A *finally* exits, the old code ran an - /// unconditional `*guard = None`, nulling **loop B's live token** — - /// after which `is_running()` lies (`false` while B runs) and a - /// shutdown `stop()`/`quiesce()` silently no-ops while loop B keeps + /// The failure this pins is a use-after-free across the FFI persister: + /// if the old loop's exit nulled the new loop's token, `is_running()` + /// would lie (`false` while the new loop runs) and a shutdown + /// `stop()`/`quiesce()` would silently no-op while the new loop kept /// fanning out `persister.store(...)` through a freed context. /// - /// We drive the token lifecycle directly (the guard's `install` / - /// `clear_if_current`) rather than spawning the real loop: the - /// loop runs on an OS thread under `Handle::block_on`, so its exit - /// timing can't be pinned deterministically. The pure-guard variant - /// lives with [`LoopCancelGuard`]; this one pins the manager-level - /// wiring (`stop()` / `is_running()` route through the guard). - #[tokio::test] - async fn stale_loop_cleanup_does_not_clobber_newer_loop_token() { + /// `stop()` / `is_running()` now route through the shared + /// `ThreadRegistry`, whose generation-guarded slot enforces this: a + /// restart reaps the prior generation under the start slot lock and the + /// prior's epilogue is gen-gated. The registry's own + /// `generation_match_epilogue_preserves_new_token` test pins the + /// primitive; this one pins the manager wiring through real + /// `start()`/`stop()` on live OS-thread loops. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn stop_then_quick_start_keeps_new_loop_cancellable() { let manager = make_manager(); let mgr = manager.dashpay_sync_arc(); - // Loop A starts: installs token_A at generation G_A. - let (token_a, gen_a) = mgr - .cancel_guard - .install() - .expect("first install starts a loop"); + // Loop A starts (empty wallet set → each pass is a no-op, no I/O). + Arc::clone(&mgr).start(); assert!(mgr.is_running()); - // Shutdown of loop A: stop() cancels + takes token_A immediately - // (cancel-only), but loop A is still "draining" — its cleanup has - // not run yet. + // stop() cancels loop A; the running flag clears immediately. mgr.stop(); - assert!(token_a.is_cancelled()); - assert!( - !mgr.is_running(), - "stop() clears the stored token immediately" - ); - - // Loop B starts BEFORE loop A's cleanup runs: installs token_B at a - // newer generation G_B. - let (token_b, _gen_b) = mgr - .cancel_guard - .install() - .expect("second install starts a new loop"); - assert!(mgr.is_running()); + assert!(!mgr.is_running(), "stop() clears the running flag at once"); - // Loop A FINALLY drains and runs its cleanup with its own (now - // stale) generation. The guard must make this a no-op; the old - // unconditional clear would null loop B's token here. - mgr.cancel_guard.clear_if_current(gen_a); + // Loop B starts before loop A has necessarily drained. The registry + // reaps the prior generation under the start slot lock and installs a + // fresh generation, so A's later epilogue cannot clear B's token. + Arc::clone(&mgr).start(); + assert!(mgr.is_running(), "loop B must be running after the restart"); - // Loop B's token must still be installed and uncancelled. - assert!( - mgr.is_running(), - "stale loop A cleanup must not clobber loop B's live token" - ); - assert!(!token_b.is_cancelled()); - - // …and a real shutdown can still cancel loop B. + // A real shutdown still cancels loop B and joins it cleanly — proof + // B stayed cancellable after A's stale exit. mgr.stop(); + assert!(!mgr.is_running()); + let report = manager.shutdown().await; assert!( - token_b.is_cancelled(), - "loop B must remain cancellable after the stale cleanup" + report.all_clean(), + "clean shutdown after restart: {report:?}" ); - assert!(!mgr.is_running()); } /// `set_interval` clamps to >=1s and round-trips through `interval`. diff --git a/packages/rs-platform-wallet/src/manager/identity_sync.rs b/packages/rs-platform-wallet/src/manager/identity_sync.rs index 9fe2efa1a6d..7584db6e3c3 100644 --- a/packages/rs-platform-wallet/src/manager/identity_sync.rs +++ b/packages/rs-platform-wallet/src/manager/identity_sync.rs @@ -62,8 +62,13 @@ use dash_sdk::platform::tokens::identity_token_balances::{ }; use dash_sdk::platform::FetchMany; +use dash_async::ThreadRegistry; + use crate::changeset::{PlatformWalletPersistence, TokenBalanceChangeSet}; -use crate::manager::loop_cancel::LoopCancelGuard; +use crate::manager::{ + coordinator_worker_config, drain_pass, QuiesceGate, QuiesceGuard, SyncSlotGuard, WalletWorker, + COORDINATOR_DRAIN_BUDGET, +}; use crate::wallet::platform_wallet::WalletId; /// Default cadence for the identity-token sync loop. @@ -158,18 +163,21 @@ where /// over `P` so every `persister.store(...)` call on the hot sync /// loop dispatches statically. persister: Arc

, - /// Generation-guarded cancel-token slot for the background loop — - /// see [`LoopCancelGuard`] for the stale-loop shutdown invariant. - cancel_guard: LoopCancelGuard, + /// Shared registry that owns this loop's lifecycle: it spawns the + /// OS thread, owns its cancellation token, and joins it at shutdown. + /// A generation-guarded slot handles a `stop()` + quick `start()` + /// without a stale loop clobbering the new one. + registry: Arc>, interval_secs: AtomicU64, is_syncing: AtomicBool, - /// Set by [`quiesce`](Self::quiesce) to gate new passes while it - /// drains an in-flight one. `sync_now` bails (after taking the - /// `is_syncing` slot) when this is set, so once `quiesce` observes + /// Gates new passes while a [`quiesce`](Self::quiesce) drains an + /// in-flight one, while a [`QuiesceGuard`] holder mutates state, and + /// terminally once shutdown seals it. `sync_now` bails (after taking the + /// `is_syncing` slot) when it is closed, so once a drain observes /// `is_syncing == false` no further pass can start — giving shutdown /// a real "no more host-visible persister stores" barrier that /// cancel-only [`stop`](Self::stop) does not provide. - quiescing: AtomicBool, + quiescing: QuiesceGate, /// Unix seconds of the last completed pass across all identities. /// `0` = never. Identity-level timestamps live on the per-identity /// rows in [`IdentitySyncManager::state`]. @@ -196,14 +204,18 @@ where /// writes). The registry starts empty — call /// [`register_identity`](Self::register_identity) before /// [`start`](Self::start). - pub fn new(sdk: Arc, persister: Arc

) -> Self { + pub fn new( + sdk: Arc, + persister: Arc

, + registry: Arc>, + ) -> Self { Self { sdk, persister, - cancel_guard: LoopCancelGuard::new(), + registry, interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS), is_syncing: AtomicBool::new(false), - quiescing: AtomicBool::new(false), + quiescing: QuiesceGate::default(), last_sync_unix: AtomicU64::new(0), state: RwLock::new(BTreeMap::new()), } @@ -316,7 +328,7 @@ where /// Whether the background loop is currently running. pub fn is_running(&self) -> bool { - self.cancel_guard.is_running() + self.registry.is_running(WalletWorker::IdentitySync) } /// Whether a sync pass is in flight right now. @@ -387,16 +399,24 @@ where /// /// The first pass runs immediately; subsequent passes fire every /// [`interval`](Self::interval). + /// + /// **Blocks briefly on restart**: the shared registry synchronously + /// reaps a still-draining prior-generation thread, spinning up to the + /// registry reap backstop (default 1 s) before returning. Call it from + /// the FFI host thread, not an async task. pub fn start(self: Arc) { - let Some((cancel, my_generation)) = self.cancel_guard.install() else { - return; - }; - let handle = tokio::runtime::Handle::current(); + let registry = Arc::clone(&self.registry); let this = self; - std::thread::Builder::new() - .name("identity-sync".into()) - .spawn(move || { + // The registry owns the whole lifecycle: it takes the `closing` / + // `clearing` latches, installs the cancellation token, spawns the + // thread, and parks any still-draining prior generation — all under + // one slot lock, so there is no check-then-spawn gap to race. A + // no-op if a worker is already live, or if teardown has begun. + registry.start_thread( + WalletWorker::IdentitySync, + coordinator_worker_config(), + move |cancel| { handle.block_on(async move { loop { if cancel.is_cancelled() { @@ -411,11 +431,9 @@ where _ = cancel.cancelled() => break, } } - - this.cancel_guard.clear_if_current(my_generation); }); - }) - .expect("failed to spawn identity-sync thread"); + }, + ); } /// Stop the background sync loop. No-op if not running. @@ -427,9 +445,7 @@ where /// by manager shutdown so the host can free the persister context — /// use [`quiesce`](Self::quiesce). pub fn stop(&self) { - if let Some(token) = self.cancel_guard.take() { - token.cancel(); - } + self.registry.cancel(WalletWorker::IdentitySync); } /// Cancel the background loop **and wait for any in-flight sync pass @@ -442,7 +458,7 @@ where /// persister context the FFI handed to us) cannot be raced by a pass /// that calls `persister.store(...)` through a now-dangling pointer. /// - /// Mechanism: set the `quiescing` gate so any pass that hasn't yet + /// Mechanism: close the `quiescing` gate so any pass that hasn't yet /// taken the `is_syncing` slot bails, cancel the loop, then wait for /// `is_syncing` to clear. `is_syncing` is held for the whole pass /// including the persister fan-out (`sync_now` clears it only after @@ -450,13 +466,56 @@ where /// so its falling edge (with the gate up) is a sound "fully drained" /// signal. The gate is reopened before returning so a later /// start/sync works normally. - pub async fn quiesce(&self) { - self.quiescing.store(true, Ordering::Release); - self.stop(); - while self.is_syncing.load(Ordering::Acquire) { - tokio::time::sleep(Duration::from_millis(20)).await; - } - self.quiescing.store(false, Ordering::Release); + /// + /// **Bounded** by `COORDINATOR_DRAIN_BUDGET`: returns `false` if + /// the in-flight pass did not drain in time — see + /// `quiesce_within` for the timeout contract. + #[must_use = "a false return means the pass did NOT drain; the caller must fail closed"] + pub async fn quiesce(&self) -> bool { + self.quiesce_within(COORDINATOR_DRAIN_BUDGET).await + } + + /// [`quiesce`](Self::quiesce) with an explicit drain budget. + /// + /// Returns `true` when the drain completed. Returns `false` when + /// `is_syncing` was still held at the deadline — a wedged pass. On + /// that path the `quiescing` gate is deliberately **left closed** so the + /// wedged pass cannot be followed by a fresh one; the caller must + /// treat the coordinator as non-clean. A later successful `quiesce` + /// reopens the gate. + pub(crate) async fn quiesce_within(&self, budget: Duration) -> bool { + // The guard drops here, reopening the gate — this is the + // "drain only" flavor. + self.quiesce_held_within(budget).await.is_some() + } + + /// [`quiesce_within`](Self::quiesce_within) that **keeps sync admission + /// shut** until the returned guard drops — the barrier a caller needs + /// when it mutates state a pass touches right after draining. + /// + /// `None` means the in-flight pass did not drain within `budget`; the + /// gate is left closed and the caller must fail closed. + #[must_use = "None means the pass did NOT drain; the caller must fail closed"] + pub(crate) async fn quiesce_held_within(&self, budget: Duration) -> Option> { + drain_pass(&self.quiescing, &self.is_syncing, || self.stop(), budget).await + } + + /// [`quiesce_within`](Self::quiesce_within) that **seals** the gate: + /// admission never reopens on this coordinator instance. + /// + /// Used by manager shutdown. Reopening there would let a direct + /// `sync_now` that was already dispatched on a host thread — the FFI + /// resolves the manager under a shared read guard, so it can be + /// mid-flight while `destroy` runs — start a fresh pass *after* the + /// drain concluded and fire persister / completion callbacks through + /// a context the host has since freed. + pub(crate) async fn quiesce_sealed_within(&self, budget: Duration) -> bool { + let guard = self.quiesce_held_within(budget).await; + let drained = guard.is_some(); + // Seal before the guard drops so its Drop cannot reopen. + self.quiescing.seal(); + drop(guard); + drained } /// Run one sync pass across every registered identity. @@ -477,13 +536,15 @@ where { return; } + // Clears `is_syncing` on every exit path — including panic unwind — + // so a failed pass can never wedge `quiesce()`'s drain. + let _slot = SyncSlotGuard(&self.is_syncing); // A `quiesce()` may have raised the gate between our CAS and // here; if so, release the slot and bail without running a pass // so the drain can complete and shutdown gets a true barrier // (no further `persister.store(...)` after quiesce returns). - if self.quiescing.load(Ordering::Acquire) { - self.is_syncing.store(false, Ordering::Release); + if self.quiescing.is_closed() { return; } @@ -509,7 +570,6 @@ where .map(|d| d.as_secs()) .unwrap_or(0); self.last_sync_unix.store(now, Ordering::Release); - self.is_syncing.store(false, Ordering::Release); } /// Sync a single identity's watched tokens against Platform. @@ -726,7 +786,11 @@ mod tests { fn make_manager() -> Arc> { let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); let persister = Arc::new(NoopPersister); - Arc::new(IdentitySyncManager::new(sdk, persister)) + Arc::new(IdentitySyncManager::new( + sdk, + persister, + ThreadRegistry::::new(), + )) } fn make_recording_manager() -> ( @@ -736,7 +800,11 @@ mod tests { let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); let persister = Arc::new(RecordingPersister::new()); ( - Arc::new(IdentitySyncManager::new(sdk, Arc::clone(&persister))), + Arc::new(IdentitySyncManager::new( + sdk, + Arc::clone(&persister), + ThreadRegistry::::new(), + )), persister, ) } @@ -909,11 +977,42 @@ mod tests { .expect("quiesce did not return after the pass drained"); // The gate is reopened before quiesce returns. - assert!(!mgr.quiescing.load(Ordering::Acquire)); + assert!(!mgr.quiescing.is_closed()); assert!(!mgr.is_syncing()); pass.await.unwrap(); } + /// A pass that never drains must NOT hang `quiesce_within` forever: + /// the drain returns `false` at its deadline and deliberately leaves + /// the `quiescing` gate closed (so the wedged pass cannot be followed by + /// a fresh one). A later successful quiesce reopens the gate. Sibling + /// of the dashpay / platform-address regression tests. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn quiesce_within_times_out_and_leaves_gate_up_when_pass_never_drains() { + let mgr = make_manager(); + + assert!(mgr + .is_syncing + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok()); + + let drained = tokio::time::timeout( + Duration::from_secs(2), + mgr.quiesce_within(Duration::from_millis(100)), + ) + .await + .expect("bounded quiesce must return at its deadline"); + assert!(!drained, "a wedged pass must be reported as non-drained"); + assert!( + mgr.quiescing.is_closed(), + "gate must stay up after a timed-out drain" + ); + + mgr.is_syncing.store(false, Ordering::Release); + assert!(mgr.quiesce().await); + assert!(!mgr.quiescing.is_closed()); + } + /// A `sync_now()` invoked while `quiescing` is set must bail without /// running the pass — in particular, without calling /// `persister.store(...)`. This is the gate that prevents a pass @@ -925,8 +1024,9 @@ mod tests { let token_x = Identifier::from([10u8; 32]); mgr.register_identity(id_a, [token_x]).await; - // Raise the gate as `quiesce()` would. - mgr.quiescing.store(true, Ordering::Release); + // Raise the gate as an in-flight `quiesce()` would (a drain holds + // the gate from its first instruction). + let gate_hold = mgr.quiescing.hold(); mgr.sync_now().await; @@ -934,6 +1034,7 @@ mod tests { // later (post-quiesce) pass can still run. assert_eq!(persister.stores.load(AtomicOrdering::SeqCst), 0); assert!(!mgr.is_syncing()); + drop(gate_hold); } /// Round-trip: register → read → update_watched_tokens → read. diff --git a/packages/rs-platform-wallet/src/manager/loop_cancel.rs b/packages/rs-platform-wallet/src/manager/loop_cancel.rs deleted file mode 100644 index b8b7e114cce..00000000000 --- a/packages/rs-platform-wallet/src/manager/loop_cancel.rs +++ /dev/null @@ -1,164 +0,0 @@ -//! Generation-guarded cancel-token slot shared by the background sync -//! managers ([`DashPaySyncManager`](super::dashpay_sync::DashPaySyncManager), -//! [`IdentitySyncManager`](super::identity_sync::IdentitySyncManager), -//! [`PlatformAddressSyncManager`](super::platform_address_sync::PlatformAddressSyncManager), -//! and the shielded coordinator). -//! -//! Every manager runs its loop on a dedicated OS thread whose exit is -//! asynchronous with respect to `stop()`/`start()` calls, so they all -//! share the same shutdown hazard — and must all share the same guard. -//! Keeping the invariant in one type (instead of a per-manager copy) -//! means a fix or audit here covers every loop at once. - -use std::sync::atomic::{AtomicU64, Ordering}; -use std::sync::Mutex as StdMutex; - -use tokio_util::sync::CancellationToken; - -/// Cancel-token slot for a background sync loop, guarded by a -/// monotonically increasing **loop generation** so a stale, draining -/// loop can never clobber a newer loop's token. -/// -/// Without the guard a `stop()` + quick `start()` is a use-after-free -/// hazard: `stop()` takes + cancels loop A's token and `start()` -/// installs loop B's token, but loop A keeps draining its in-flight -/// pass. When loop A finally exits, an unconditional `slot = None` -/// would null **loop B's** live token, leaving loop B uncancellable — -/// a later shutdown `stop()`/`quiesce()` silently no-ops while loop B -/// keeps calling `persister.store(...)` (or firing host callbacks) -/// through a freed FFI context. -pub(crate) struct LoopCancelGuard { - /// The active loop's cancel token, if one is running. - slot: StdMutex>, - /// Bumped on every [`install`](Self::install). The background loop - /// captures its generation at install time and clears the slot on - /// exit **only if its generation is still current** (see - /// [`clear_if_current`](Self::clear_if_current)). - generation: AtomicU64, -} - -impl LoopCancelGuard { - pub fn new() -> Self { - Self { - slot: StdMutex::new(None), - generation: AtomicU64::new(0), - } - } - - /// Install a fresh cancel token for a new background loop, returning - /// the token (for the loop to watch) and its **generation** (for the - /// loop to pass to [`clear_if_current`](Self::clear_if_current) on - /// exit). Returns `None` if a loop is already running — preserving - /// the managers' `start()` idempotency. - /// - /// The generation bump happens under the same lock that stores the - /// token, so a draining older loop reading the generation under that - /// lock always observes whether a newer loop has since replaced it. - pub fn install(&self) -> Option<(CancellationToken, u64)> { - let mut guard = self.slot.lock().expect("bg_cancel poisoned"); - if guard.is_some() { - return None; - } - let cancel = CancellationToken::new(); - *guard = Some(cancel.clone()); - let generation = self - .generation - .fetch_add(1, Ordering::AcqRel) - .wrapping_add(1); - Some((cancel, generation)) - } - - /// Clear the stored cancel token **only if it still belongs to the - /// loop identified by `my_generation`** — i.e. no later `install` - /// has handed out a replacement. Called by the loop on exit. - pub fn clear_if_current(&self, my_generation: u64) { - let mut guard = self.slot.lock().expect("bg_cancel poisoned"); - if self.generation.load(Ordering::Acquire) == my_generation { - *guard = None; - } - } - - /// Take the active loop's token out of the slot, if any. The - /// managers' cancel-only `stop()` is `take()` + `token.cancel()`. - pub fn take(&self) -> Option { - self.slot.lock().expect("bg_cancel poisoned").take() - } - - /// Whether a loop's token is currently installed. - pub fn is_running(&self) -> bool { - self.slot.lock().map(|g| g.is_some()).unwrap_or(false) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - /// Regression: a stale, draining loop's cleanup must **not** clobber - /// a newer loop's cancel token. - /// - /// We drive the token lifecycle directly (`install` / - /// `clear_if_current`) rather than spawning a real loop: the loops - /// run on OS threads under `Handle::block_on`, so their exit timing - /// can't be pinned deterministically. With the generation guard - /// removed (`*guard = None` unconditional) this test fails on the - /// final assertions; with the guard it passes. - #[test] - fn stale_loop_cleanup_does_not_clobber_newer_loop_token() { - let slot = LoopCancelGuard::new(); - - // Loop A starts: installs token_A at generation G_A. - let (token_a, gen_a) = slot.install().expect("first install starts a loop"); - assert!(slot.is_running()); - - // Shutdown of loop A: stop() cancels + takes token_A immediately - // (cancel-only), but loop A is still "draining" — its cleanup - // has not run yet. - slot.take().expect("token_A installed").cancel(); - assert!(token_a.is_cancelled()); - assert!(!slot.is_running(), "take() clears the slot immediately"); - - // Loop B starts BEFORE loop A's cleanup runs: installs token_B - // at a newer generation G_B. - let (token_b, _gen_b) = slot.install().expect("second install starts a new loop"); - assert!(slot.is_running()); - - // Loop A FINALLY drains and runs its cleanup with its own (now - // stale) generation. The guard must make this a no-op; an - // unconditional clear would null loop B's token here. - slot.clear_if_current(gen_a); - - // Loop B's token must still be installed and uncancelled. - assert!( - slot.is_running(), - "stale loop A cleanup must not clobber loop B's live token" - ); - assert!(!token_b.is_cancelled()); - - // …and a real shutdown can still cancel loop B. - slot.take().expect("token_B still installed").cancel(); - assert!( - token_b.is_cancelled(), - "loop B must remain cancellable after the stale cleanup" - ); - assert!(!slot.is_running()); - } - - /// `install` while a loop is running returns `None` (start - /// idempotency), and a loop that exits *without* being replaced - /// clears its own slot so a later install succeeds. - #[test] - fn install_is_exclusive_and_clear_reopens_slot() { - let slot = LoopCancelGuard::new(); - - let (_token, generation) = slot.install().expect("fresh slot installs"); - assert!(slot.install().is_none(), "second install must be refused"); - - slot.clear_if_current(generation); - assert!(!slot.is_running()); - assert!( - slot.install().is_some(), - "slot must be reusable after the loop clears it" - ); - } -} diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index 9cf9dead654..ade7ac6e0a3 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -4,14 +4,15 @@ pub mod accessors; pub mod dashpay_sync; pub mod identity_sync; mod load; -mod loop_cancel; pub mod platform_address_sync; #[cfg(feature = "shielded")] pub mod shielded_sync; mod wallet_lifecycle; use std::sync::Arc; +use std::time::Duration; +use dash_async::{ShutdownReport, ThreadRegistry, WorkerConfig, WorkerStatus, DEFAULT_JOIN_BUDGET}; use tokio::sync::{Notify, RwLock}; use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; @@ -32,6 +33,288 @@ use crate::wallet::identity::network::DashPayPaymentHandler; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::wallet::PlatformWallet; +/// Registry key identifying each background worker the manager joins at +/// shutdown. +/// +/// The four periodic sync coordinators run their `!Send` loops on OS +/// threads the shared [`ThreadRegistry`] spawns and owns end to end: it +/// installs each loop's cancellation token, and +/// [`shutdown`](PlatformWalletManager::shutdown) cancels and joins them — +/// surfacing a panicked loop — before the host drops the tokio runtime. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum WalletWorker { + /// Platform-address (BLAST / DIP-17) balance sync coordinator. + PlatformAddressSync, + /// Per-identity token-state sync coordinator. + IdentitySync, + /// DashPay (contact requests + profiles) sync coordinator. + DashPaySync, + /// Shielded (Orchard) note sync coordinator. + ShieldedSync, + /// SPV runtime — the network event source feeding every persister- + /// visible wallet event. Not a registry worker: `SpvRuntime::stop` + /// owns its (bounded, abort-escalating) join, and + /// [`shutdown`](PlatformWalletManager::shutdown) folds the stop + /// outcome into the report so a failed SPV stop can never hide + /// behind a clean coordinator join. + Spv, + /// DashPay payment-hook tasks spawned by `DashPayPaymentHandler` in + /// response to SPV wallet events. Not a registry worker: the + /// handler's own tracker closes admission and joins the admitted + /// tasks; its drain outcome is folded into the report because those + /// tasks clone the FFI persister and can fire host callbacks. + DashPayPayments, + /// The wallet-event adapter task — the sink coordinator stores feed + /// into. Not a registry worker: joined by + /// [`shutdown`](PlatformWalletManager::shutdown) under a bounded + /// budget, with the live handle re-parked on timeout so a destroy + /// retry can re-join it. + EventAdapter, +} + +// `dash_async::RegistryKey` is a blanket impl over +// `Copy + Ord + Eq + Debug + Send + Sync + 'static`, which the derives above +// satisfy — no explicit impl needed. + +/// Deadline for a coordinator `quiesce()` drain — how long we wait for an +/// in-flight pass (its `is_syncing` slot) to fall before giving up and +/// reporting the coordinator non-clean. Without a bound, a pass wedged in +/// a network / persister / host-callback await blocks `shutdown()` (and +/// therefore the FFI's `destroy`) forever, *before* the registry's +/// per-worker join budget ever gets a chance to run. A timed-out drain is +/// surfaced as [`WorkerStatus::Timeout`](dash_async::WorkerStatus::Timeout) +/// so `all_clean()` fails and the host keeps its callback context alive. +pub(crate) const COORDINATOR_DRAIN_BUDGET: Duration = Duration::from_secs(10); + +/// Deadline for draining the DashPay payment-hook tasks at shutdown. +/// After it lapses each straggler is aborted and given +/// [`PAYMENT_ABORT_GRACE`] to confirm termination; anything still alive +/// is kept tracked and reported non-clean. +pub(crate) const PAYMENT_DRAIN_BUDGET: Duration = Duration::from_secs(10); + +/// Post-abort confirmation grace for one payment-hook task. An abort only +/// takes effect at the task's next await point, so a task stuck inside a +/// synchronous persister call cannot be interrupted — after this grace it +/// is left tracked (for a retry to re-join) and reported non-clean. +pub(crate) const PAYMENT_ABORT_GRACE: Duration = Duration::from_secs(1); + +/// Deadline for joining the wallet-event adapter task at shutdown. The +/// adapter exits promptly on cancellation; the bound exists so a persister +/// `store` it is blocked in cannot hang `destroy`. On timeout the live +/// handle is re-parked so a destroy retry re-joins it, and the report +/// carries [`WorkerStatus::Timeout`](dash_async::WorkerStatus::Timeout). +const EVENT_ADAPTER_JOIN_BUDGET: Duration = Duration::from_secs(10); + +/// RAII holder for a coordinator's `is_syncing` slot: clears the flag on +/// drop, **including panic unwind out of a pass body**. Every pass must +/// hold one of these instead of storing `false` manually — a panicking +/// pass that leaves `is_syncing` latched would wedge `quiesce()`'s drain +/// until its budget lapses on every subsequent teardown. +pub(crate) struct SyncSlotGuard<'a>(pub(crate) &'a std::sync::atomic::AtomicBool); + +impl Drop for SyncSlotGuard<'_> { + fn drop(&mut self) { + self.0.store(false, std::sync::atomic::Ordering::Release); + } +} + +/// Sync-pass admission gate shared by all four coordinators. +/// +/// A pass claims its coordinator's `is_syncing` slot and then checks this +/// gate; when the gate is closed it releases the slot and bails without +/// touching any state. The gate is what turns "no pass is in flight right +/// now" into "no pass is in flight *and none can start*", which is the +/// barrier Clear / reset / shutdown need before they mutate or free the +/// state a pass would touch. +/// +/// Four independent reasons close it, deliberately kept apart: +/// - an active drain ([`drain_pass`]) — the drain **counts as a holder +/// from its first instruction**, so overlapping drains keep the gate +/// shut for each other (see the race note on [`GateBookkeeping::holds`]), +/// - a live [`QuiesceGuard`] holder — a caller that drained and is still +/// mutating; the gate reopens when the last hold drops, +/// - the latch — a drain that timed out leaves the gate stuck closed with +/// no holder, so the wedged pass cannot be followed by a fresh one; the +/// next *successful* drain clears it, +/// - [`seal`](Self::seal) — terminal, set by +/// [`shutdown`](PlatformWalletManager::shutdown); never reopens, so a +/// direct `sync_now` that was already dispatched on a host thread +/// cannot start a fresh pass after the drain concluded and the FFI +/// freed the callback context. +#[derive(Default)] +pub(crate) struct QuiesceGate { + /// The single flag every pass reads — one atomic load on the hot path + /// instead of taking `bookkeeping`. Only ever written while holding + /// that lock, so it is always consistent with the state below. + closed: std::sync::atomic::AtomicBool, + /// Serializes every transition. Without it, a guard dropping (reopen) + /// can interleave with another caller closing + taking a hold, and the + /// stale reopen wins — leaving admission open under a live holder. + /// Held for a handful of instructions and never across an `.await`. + bookkeeping: std::sync::Mutex, +} + +#[derive(Default)] +struct GateBookkeeping { + /// Live [`QuiesceGuard`]s — including every drain still in flight, + /// which takes its hold at [`drain_pass`] entry rather than after its + /// final `is_syncing` observation. The early hold is load-bearing: + /// were a drain not counted until it finished, a concurrent holder's + /// drop could reopen the gate in the window between the drain's last + /// `is_syncing` load and its own hold, letting a direct sync claim + /// the slot and pass the gate check — and the drain would then return + /// "success" to a caller about to wipe state under that live pass. + holds: usize, + /// A drain timed out with the pass still holding `is_syncing`; keeps + /// the gate closed with no holder until a later drain succeeds. + latched: bool, + /// Terminal close. Wins over everything. + sealed: bool, +} + +impl GateBookkeeping { + fn should_close(&self) -> bool { + self.sealed || self.latched || self.holds > 0 + } +} + +impl QuiesceGate { + /// Whether new sync passes are currently barred. + pub(crate) fn is_closed(&self) -> bool { + self.closed.load(std::sync::atomic::Ordering::Acquire) + } + + fn bookkeeping(&self) -> std::sync::MutexGuard<'_, GateBookkeeping> { + // The critical sections are straight-line counter updates that + // cannot panic, so the lock cannot actually be poisoned. + self.bookkeeping + .lock() + .expect("quiesce gate mutex poisoned") + } + + /// Recompute the hot-path flag from the bookkeeping — the ONLY writer + /// of `closed`, always under the lock. + fn publish_locked(&self, bookkeeping: &GateBookkeeping) { + self.closed.store( + bookkeeping.should_close(), + std::sync::atomic::Ordering::Release, + ); + } + + /// Take a hold, closing the gate. Called at [`drain_pass`] entry (the + /// drain itself is a holder) — there is no close-without-hold except + /// the timeout latch and the seal. + fn hold(&self) -> QuiesceGuard<'_> { + let mut bookkeeping = self.bookkeeping(); + bookkeeping.holds += 1; + self.publish_locked(&bookkeeping); + QuiesceGuard(self) + } + + /// A drain observed the pass fully drained while holding the gate: + /// clear any latch left by a previously timed-out drain. The caller + /// still holds its guard, so the gate stays closed until that drops. + fn drain_succeeded(&self) { + let mut bookkeeping = self.bookkeeping(); + bookkeeping.latched = false; + self.publish_locked(&bookkeeping); + } + + /// A drain gave up with the pass still holding `is_syncing`: latch the + /// gate closed so dropping the drain's own hold cannot reopen it. + fn latch_closed(&self) { + let mut bookkeeping = self.bookkeeping(); + bookkeeping.latched = true; + self.publish_locked(&bookkeeping); + } + + /// Drop a hold, reopening the gate only if nothing else closes it. + fn release(&self) { + let mut bookkeeping = self.bookkeeping(); + bookkeeping.holds = bookkeeping.holds.saturating_sub(1); + self.publish_locked(&bookkeeping); + } + + /// Close the gate permanently. Used by manager shutdown, after which + /// no pass may ever start again on this manager instance. + pub(crate) fn seal(&self) { + let mut bookkeeping = self.bookkeeping(); + bookkeeping.sealed = true; + self.publish_locked(&bookkeeping); + } +} + +/// RAII hold on a closed [`QuiesceGate`]: keeps new passes barred for as +/// long as the holder is mutating state a pass would touch, and reopens +/// the gate on drop — including `?` early-return and panic unwind. +/// +/// Without this, `quiesce()` reopened the gate the instant it returned, so +/// `clear_shielded` / `reset_platform_address_sync_state` ran their wipe +/// with admission already re-opened: a direct `sync_now` on a host thread +/// could snapshot pre-wipe state and re-persist it right after the wipe. +#[must_use = "dropping the guard immediately reopens sync admission, which defeats the barrier"] +pub(crate) struct QuiesceGuard<'a>(&'a QuiesceGate); + +impl Drop for QuiesceGuard<'_> { + fn drop(&mut self) { + self.0.release(); + } +} + +/// Shared drain body behind every coordinator's `quiesce*` family: take a +/// hold on the gate so no new pass can start, cancel the loop, then wait +/// for the in-flight pass (if any) to release `is_syncing`. +/// +/// `is_syncing` is held across a pass's persister / host-callback fan-out, +/// so its falling edge *with the gate closed* is a sound "fully drained, +/// nothing more will fire" signal. The hold is taken at ENTRY — before the +/// first `is_syncing` observation — so the gate is closed continuously +/// from here to the returned guard's drop, and no concurrent holder's +/// release can open an admission window mid-drain (the race a +/// close-then-hold-at-the-end sequence has). +/// +/// Returns a [`QuiesceGuard`] that keeps the gate closed until it drops. +/// Returns `None` when the pass was still holding `is_syncing` at the +/// deadline; the gate is latched closed on that path (the wedged pass +/// must not be followed by a fresh one — a later successful drain clears +/// the latch) and the caller must fail closed. +pub(crate) async fn drain_pass<'a>( + gate: &'a QuiesceGate, + is_syncing: &std::sync::atomic::AtomicBool, + stop: impl FnOnce(), + budget: Duration, +) -> Option> { + let guard = gate.hold(); + stop(); + let deadline = tokio::time::Instant::now() + budget; + while is_syncing.load(std::sync::atomic::Ordering::Acquire) { + if tokio::time::Instant::now() >= deadline { + // Latch BEFORE the guard drops so there is no instant in + // which the gate is open on the timeout path. + gate.latch_closed(); + drop(guard); + return None; + } + tokio::time::sleep(Duration::from_millis(20)).await; + } + gate.drain_succeeded(); + Some(guard) +} + +/// Base [`WorkerConfig`] each coordinator starts its loop thread with — the +/// registry's default managed-join budget ([`DEFAULT_JOIN_BUDGET`]) so a +/// wedged loop pass surfaces as +/// [`WorkerStatus::Timeout`](dash_async::WorkerStatus::Timeout) instead of +/// hanging shutdown forever, and the platform default OS-thread stack. A +/// coordinator that needs a deeper stack (e.g. DashPay's GroveDB proof +/// descent) overrides `stack_size` on top of this. +pub(crate) fn coordinator_worker_config() -> WorkerConfig { + WorkerConfig { + join_budget: DEFAULT_JOIN_BUDGET, + stack_size: None, + } +} + /// Multi-wallet coordinator with SPV sync and event handling. /// /// Events are dispatched through [`PlatformEventManager`] to all registered @@ -102,6 +385,12 @@ pub struct PlatformWalletManager { /// is torn down. pub(super) event_adapter_cancel: CancellationToken, pub(super) event_adapter_join: tokio::sync::Mutex>>, + /// Shared lifecycle registry for the periodic coordinator threads. + /// Each coordinator spawns its loop through `registry.start_thread` at + /// `start`, handing the registry ownership of the OS thread and its + /// cancellation token; [`shutdown`](Self::shutdown) cancels, joins, and + /// reports per-worker terminal status. + pub(super) registry: Arc>, /// Host-visible hard sync-fault latch (dashpay/platform#4069). Set /// (and never cleared) by the wallet-event adapter the first time it /// freezes a durable watermark after a persistence `store()` rejection @@ -135,6 +424,9 @@ impl PlatformWalletManager

{ let wallet_manager = Arc::new(RwLock::new(wallet_manager_inner)); let wallets = Arc::new(RwLock::new(std::collections::BTreeMap::new())); let lock_notify = Arc::new(Notify::new()); + // Shared registry that owns the coordinators' loop-thread join + // handles for a clean, panic-aware shutdown join. + let registry = ThreadRegistry::::new(); // Host-visible hard sync-fault latch (dashpay/platform#4069). The // adapter raises it the first time it freezes a durable watermark. @@ -183,14 +475,19 @@ impl PlatformWalletManager

{ let platform_address_sync = Arc::new(PlatformAddressSyncManager::new( Arc::clone(&wallets), Arc::clone(&event_manager), + Arc::clone(®istry), )); let identity_sync = Arc::new(IdentitySyncManager::new( Arc::clone(&sdk), Arc::clone(&persister), + Arc::clone(®istry), )); // DashPay sync shares the `wallets` map (not the token // registry) so DashPay-only identities sync on every sweep. - let dashpay_sync = Arc::new(DashPaySyncManager::new(Arc::clone(&wallets))); + let dashpay_sync = Arc::new(DashPaySyncManager::new( + Arc::clone(&wallets), + Arc::clone(®istry), + )); #[cfg(feature = "shielded")] let shielded_coordinator: Arc< RwLock>>, @@ -199,6 +496,7 @@ impl PlatformWalletManager

{ let shielded_sync = Arc::new(ShieldedSyncManager::new( Arc::clone(&event_manager), Arc::clone(&shielded_coordinator), + Arc::clone(®istry), )); Self { sdk, @@ -219,6 +517,7 @@ impl PlatformWalletManager

{ persister, event_adapter_cancel, event_adapter_join: tokio::sync::Mutex::new(Some(event_adapter_join)), + registry, sync_fault, } } @@ -375,7 +674,30 @@ impl PlatformWalletManager

{ /// fails; the host must not commit its own persistence wipe in that case. #[cfg(feature = "shielded")] pub async fn clear_shielded(&self) -> Result<(), crate::error::PlatformWalletError> { - self.shielded_sync_manager.quiesce().await; + // Hold the registry's per-key clearing latch across the WHOLE + // quiesce -> wipe. While it is up, `ShieldedSyncManager::start` + // (and any registry (re)start) is a no-op, so no fresh pass can + // slip between the quiesce and the wipe and re-persist notes into + // the store `coord.clear()` is about to reset. The guard's Drop + // releases the latch on every exit path (including `?` and panic). + let _clearing = self.registry.hold_clearing(WalletWorker::ShieldedSync); + // Hold sync admission shut for the WHOLE quiesce -> wipe as well. + // The clearing latch only bars registry (re)starts; a direct + // `sync_now` / `sync_wallet` on a host thread does not consult it, + // and a plain `quiesce()` reopens admission the instant it returns + // — so such a pass could snapshot the old account set and refill + // the commitment tree right after `coord.clear()` reset it. The + // guard's Drop reopens admission on every exit path (`?`, panic). + let Some(_quiesced) = self.shielded_sync_manager.quiesce_held().await else { + // Fail closed: a pass is still holding `is_syncing` after the + // drain budget, so wiping the store now would race its + // persister fan-out. The host must NOT commit its own wipe. + return Err(crate::error::PlatformWalletError::ShutdownIncomplete( + "shielded sync pass did not drain within the quiesce budget; \ + clear aborted — retry once sync is idle" + .to_string(), + )); + }; match self.shielded_coordinator().await { Some(coord) => coord.clear().await, None => { @@ -412,7 +734,23 @@ impl PlatformWalletManager

{ pub async fn reset_platform_address_sync_state( &self, ) -> Result<(), crate::error::PlatformWalletError> { - self.platform_address_sync_manager.quiesce().await; + // Same two-part exclusion as `clear_shielded`: the registry's + // clearing latch bars a loop (re)start, and the held quiesce guard + // bars a direct `sync_now` / `sync_wallet` for the whole + // quiesce -> reset section. Both Drops run on every exit path. + let _clearing = self + .registry + .hold_clearing(WalletWorker::PlatformAddressSync); + let Some(_quiesced) = self.platform_address_sync_manager.quiesce_held().await else { + // Fail closed, mirroring `clear_shielded`: resetting the + // watermark while a wedged pass still holds `is_syncing` + // would let its tail re-write the state this reset clears. + return Err(crate::error::PlatformWalletError::ShutdownIncomplete( + "platform-address sync pass did not drain within the quiesce budget; \ + reset aborted — retry once sync is idle" + .to_string(), + )); + }; // Snapshot Arc clones under a short read lock; never hold the // `wallets` read guard across the per-wallet `.await`s below — @@ -429,45 +767,425 @@ impl PlatformWalletManager

{ Ok(()) } - /// Stop all background tasks and wait for them to exit. + /// Stop all background tasks, join their threads, and report how each + /// one ended. /// /// Stops SPV and **quiesces** the periodic coordinators /// (`PlatformAddressSyncManager`, `IdentitySyncManager`, - /// `DashPaySyncManager`, `ShieldedSyncManager`) — cancelling each - /// loop *and draining any in-flight pass to completion*, including - /// its persister / host-callback fan-out — then drains the - /// wallet-event adapter task. - /// Idempotent. Call before dropping the manager when a clean - /// shutdown is required (e.g. on app termination); a dirty drop - /// simply leaks the tasks until the runtime exits. + /// `DashPaySyncManager`, `ShieldedSyncManager`) — cancelling each loop + /// *and draining any in-flight pass to completion*, including its + /// persister / host-callback fan-out — then **joins** their loop OS + /// threads through the shared [`ThreadRegistry`] and finally drains the + /// wallet-event adapter task. Idempotent. /// - /// Ordering matters: SPV is stopped and joined first so it cannot dispatch - /// more wallet events. Payment-task admission is then closed and all - /// admitted work is joined. A cancel-only `stop()` would let a pass already - /// inside `sync_now` keep running and call `persister.store(...)` / - /// fire a host completion callback after the FFI's `destroy` - /// returned and the host freed the persister / event-handler - /// context — a use-after-free. So we `quiesce()` the sync managers - /// FIRST (so no further persister store or host callback can start), - /// and only THEN cancel + join the event adapter, which is the sink - /// those stores feed into. - pub async fn shutdown(&self) { - if let Err(error) = self.spv_manager.stop().await { - tracing::warn!(?error, "SPV shutdown failed"); - } + /// Ordering matters and is fourfold: + /// 1. SPV is stopped and joined FIRST so it cannot dispatch more wallet + /// events, then payment-task admission is closed and all admitted + /// DashPay payment-hook work is joined. + /// 2. `quiesce()` each coordinator. Cancel-only `stop()` would + /// let a pass already inside `sync_now` keep running and call + /// `persister.store(...)` / fire a host completion callback after + /// the FFI's `destroy` returned and the host freed the persister / + /// event-handler context — a use-after-free. + /// 3. `registry.shutdown()` then JOINS the coordinator OS threads. + /// `quiesce`'s `is_syncing` barrier only proves no pass is *in + /// flight*; the detached thread may still be unwinding out of + /// `Handle::block_on`, touching `tokio::time` on a runtime the host + /// is about to drop. Joining guarantees it has fully exited, and + /// surfaces a panicked loop as a non-clean [`WorkerStatus`] rather + /// than silently dropping it. + /// 4. The event adapter — the sink those stores feed into — drains + /// LAST. + /// + /// **Every phase is bounded.** SPV stop owns its own abort-escalating + /// join; the payment-hook drain is bounded by `PAYMENT_DRAIN_BUDGET`; + /// the coordinator drains run concurrently under + /// `COORDINATOR_DRAIN_BUDGET`; the registry join uses each worker's + /// join budget; the adapter join is bounded too (its live handle is + /// re-parked on timeout so a retry re-joins it). A wedged await + /// therefore surfaces as a non-clean report instead of hanging the + /// FFI's `destroy` forever. + /// + /// Returns a [`ShutdownReport`] keyed by [`WalletWorker`] — including + /// the non-registry workers [`WalletWorker::Spv`], + /// [`WalletWorker::DashPayPayments`], and + /// [`WalletWorker::EventAdapter`], so no callback-capable background + /// work is excluded from the verdict. Inspect + /// [`ShutdownReport::all_clean`] before freeing the host callback + /// context. A non-clean status flags a still-live worker or orphan. + /// + /// [`WorkerStatus`]: dash_async::WorkerStatus + pub async fn shutdown(&self) -> ShutdownReport { + // SPV first: it is the event source feeding everything below, and + // its `stop` owns a bounded, abort-escalating join of the run-loop + // task. Its outcome lands in the report — a failed stop must not + // hide behind a clean coordinator join. + let spv_status = match self.spv_manager.stop().await { + Ok(()) => WorkerStatus::Ok, + Err(error) => { + tracing::warn!(?error, "SPV shutdown failed"); + WorkerStatus::Error(error.to_string()) + } + }; + + // Close payment-hook admission and join the admitted tasks — + // they clone the FFI persister, so a straggler is exactly the + // callback-after-destroy hazard the report exists to catch. + let payments_drained = self + .dashpay_payment_handler + .quiesce_within(PAYMENT_DRAIN_BUDGET) + .await; - self.dashpay_payment_handler.quiesce().await; - self.platform_address_sync_manager.quiesce().await; - self.identity_sync_manager.quiesce().await; - self.dashpay_sync_manager.quiesce().await; + // Drain the coordinators concurrently against one shared budget so + // the drain phase as a whole is bounded (a wedged pass surfaces as + // `Timeout` in the report instead of hanging destroy forever). + // + // `_sealed_` (not plain `quiesce_within`): shutdown is terminal, so + // sync admission must NOT reopen when the drain returns. The FFI + // resolves the manager under a shared read guard, so a `sync_now` + // dispatched on a host thread can still be between its slot CAS and + // its gate check while `destroy` runs; a reopened gate would let it + // run a full pass — and fire persister / completion callbacks — + // after `destroy` returned and the host freed those contexts. #[cfg(feature = "shielded")] - self.shielded_sync_manager.quiesce().await; + let (pa_drained, id_drained, dp_drained, sh_drained) = tokio::join!( + self.platform_address_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + self.identity_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + self.dashpay_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + self.shielded_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + ); + #[cfg(not(feature = "shielded"))] + let (pa_drained, id_drained, dp_drained) = tokio::join!( + self.platform_address_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + self.identity_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + self.dashpay_sync_manager + .quiesce_sealed_within(COORDINATOR_DRAIN_BUDGET), + ); + + // Hard-join the coordinator loop threads now that every in-flight + // pass has drained. This is the barrier `quiesce` cannot give: + // it waits for the actual OS thread to terminate before the host + // drops the runtime. + let mut report = self.registry.shutdown().await; + + // Fold drain timeouts in. A timed-out drain means a pass — + // possibly a *direct* `sync_now` running on a host FFI thread the + // registry never sees — may still hold `is_syncing` and fire + // persister callbacks, so a clean registry join must not mask it. + let drains = [ + (WalletWorker::PlatformAddressSync, pa_drained), + (WalletWorker::IdentitySync, id_drained), + (WalletWorker::DashPaySync, dp_drained), + #[cfg(feature = "shielded")] + (WalletWorker::ShieldedSync, sh_drained), + ]; + for (worker, drained) in drains { + if !drained { + let status = report + .per_worker + .entry(worker) + .or_insert(WorkerStatus::Timeout); + if status.is_clean() { + *status = WorkerStatus::Timeout; + } + } + } + + report.per_worker.insert(WalletWorker::Spv, spv_status); + report.per_worker.insert( + WalletWorker::DashPayPayments, + if payments_drained { + WorkerStatus::Ok + } else { + WorkerStatus::Timeout + }, + ); + // The wallet-event adapter is the sink the coordinators' stores + // feed into, so it drains AFTER them. It is a plain tokio task, + // not a registry worker; on a join timeout the live handle is + // re-parked so a destroy retry can re-join it rather than + // silently detaching the task. self.event_adapter_cancel.cancel(); - if let Some(handle) = self.event_adapter_join.lock().await.take() { - if let Err(e) = handle.await { - tracing::warn!(error = ?e, "Wallet event adapter task join error"); + let adapter_status = { + let mut slot = self.event_adapter_join.lock().await; + match slot.take() { + None => WorkerStatus::NotRunning, + Some(mut handle) => { + match tokio::time::timeout(EVENT_ADAPTER_JOIN_BUDGET, &mut handle).await { + Ok(Ok(())) => WorkerStatus::Ok, + Ok(Err(e)) if e.is_panic() => WorkerStatus::Panicked(e.to_string()), + Ok(Err(e)) => WorkerStatus::Stopped(Some(e.to_string())), + Err(_) => { + tracing::warn!( + "wallet event adapter did not join within {:?}; re-parking", + EVENT_ADAPTER_JOIN_BUDGET + ); + *slot = Some(handle); + WorkerStatus::Timeout + } + } + } } + }; + report + .per_worker + .insert(WalletWorker::EventAdapter, adapter_status); + + report + } +} + +#[cfg(test)] +mod tests { + use super::*; + + use dash_async::WorkerStatus; + + use crate::changeset::{ + ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + }; + use crate::events::{EventHandler, PlatformEventHandler}; + + struct NoopPersister; + impl PlatformWalletPersistence for NoopPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + fn make_manager() -> Arc> { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + Arc::new(PlatformWalletManager::new( + sdk, + Arc::new(NoopPersister), + Arc::new(NoopEventHandler) as Arc, + )) + } + + /// `shutdown()` joins every started coordinator through the shared + /// [`ThreadRegistry`], reports each as cleanly joined, and is + /// idempotent — a second call finds nothing running and still reports + /// clean. This is the barrier the previous discard-the-handle `start` + /// could not give: proof the loop OS threads have terminated before the + /// host drops the runtime. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn shutdown_joins_started_coordinators_and_is_idempotent() { + let mgr = make_manager(); + // Empty wallet/identity state, so each coordinator's first pass is a + // no-op and no network I/O happens; the point is thread lifecycle. + Arc::clone(&mgr.identity_sync_manager).start(); + Arc::clone(&mgr.platform_address_sync_manager).start(); + Arc::clone(&mgr.dashpay_sync_manager).start(); + + let report = mgr.shutdown().await; + assert!(report.all_clean(), "clean shutdown: {report:?}"); + for worker in [ + WalletWorker::IdentitySync, + WalletWorker::PlatformAddressSync, + WalletWorker::DashPaySync, + ] { + assert_eq!( + report.per_worker.get(&worker), + Some(&WorkerStatus::Ok), + "{worker:?} must join cleanly" + ); } + // The verdict must also cover the non-registry workers: SPV, the + // DashPay payment-hook tracker, and the wallet-event adapter. A + // report that omitted them could pass `all_clean()` while + // callback-capable background work stayed live. + for worker in [ + WalletWorker::Spv, + WalletWorker::DashPayPayments, + WalletWorker::EventAdapter, + ] { + assert!( + report + .per_worker + .get(&worker) + .is_some_and(WorkerStatus::is_clean), + "{worker:?} must be present and clean in the report: {report:?}" + ); + } + + // Second shutdown: the coordinators already joined, so the registry + // reports them NotRunning and the report stays clean. + let again = mgr.shutdown().await; + assert!(again.all_clean(), "idempotent shutdown: {again:?}"); + } + + /// `reset_platform_address_sync_state` must fail closed when the + /// in-flight pass does not drain: resetting watermarks and balances + /// under a live pass would let that pass's tail re-persist the state + /// the reset just cleared. + /// + /// It must also leave no lifecycle latch stuck on the failure path — + /// the registry's clearing latch is released by its guard's `Drop`, so + /// a later retry (or a normal `start`) is not permanently barred. + #[tokio::test(start_paused = true)] + async fn reset_platform_address_state_fails_closed_on_a_wedged_pass() { + let mgr = make_manager(); + + // Wedge a pass: take the slot and never release it, as a pass stuck + // in a network / persister await would. + assert!(mgr.platform_address_sync().wedge_sync_slot_for_test()); + + let error = tokio::time::timeout( + Duration::from_secs(30), + mgr.reset_platform_address_sync_state(), + ) + .await + .expect("the reset must be bounded by the drain budget, not hang") + .expect_err("a wedged pass must abort the reset"); + assert!( + matches!( + error, + crate::error::PlatformWalletError::ShutdownIncomplete(_) + ), + "expected ShutdownIncomplete so the FFI surfaces the typed code, got {error:?}" + ); + + assert!( + !mgr.registry.is_clearing(WalletWorker::PlatformAddressSync), + "the clearing latch must be released on the failure path" + ); + } + + /// A concurrent holder's drop must NOT reopen admission while another + /// drain is still in flight. + /// + /// RED against the close-then-hold-at-the-end gate: drain B closed the + /// gate but only became a *holder* after its final `is_syncing` + /// observation, so holder A dropping in that window stored + /// `closed = false` — a direct `sync_now` could then claim the slot, + /// pass the gate check, and run a full pass that B's caller (a + /// clear/reset about to wipe state) believed was impossible. With the + /// hold taken at drain entry, the gate is closed continuously from + /// B's first instruction to its guard's drop. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn concurrent_drain_keeps_gate_closed_across_another_holders_drop() { + use std::sync::atomic::{AtomicBool, Ordering}; + + let gate = Arc::new(QuiesceGate::default()); + let is_syncing = Arc::new(AtomicBool::new(false)); + + // Holder A: a completed drain (idle coordinator) holding its guard. + let guard_a = drain_pass(&gate, &is_syncing, || {}, Duration::from_secs(1)) + .await + .expect("idle drain must succeed"); + + // Drain B: in flight against a wedged pass, parked in its poll + // loop. The guard cannot cross the task boundary (it borrows the + // task-local gate Arc), so B holds it in-task and is driven over + // channels. + is_syncing.store(true, Ordering::Release); + let gate_b = Arc::clone(&gate); + let is_syncing_b = Arc::clone(&is_syncing); + let (b_drained_tx, b_drained_rx) = tokio::sync::oneshot::channel::(); + let (b_release_tx, b_release_rx) = tokio::sync::oneshot::channel::<()>(); + let b = tokio::spawn(async move { + let guard = drain_pass(&gate_b, &is_syncing_b, || {}, Duration::from_secs(5)).await; + let _ = b_drained_tx.send(guard.is_some()); + // Keep the guard held (B's caller "is mutating") until driven. + let _ = b_release_rx.await; + drop(guard); + }); + // Let B take its entry hold and enter the poll loop. + tokio::time::sleep(Duration::from_millis(50)).await; + + // A finishes its mutation and drops. The gate must STAY closed: + // B's drain is still deciding whether the pass has drained. + drop(guard_a); + assert!( + gate.is_closed(), + "a holder's drop must not reopen admission while a drain is in flight" + ); + + // Release the wedge; B's drain completes and its guard keeps the + // gate closed until B's caller is done mutating. + is_syncing.store(false, Ordering::Release); + let b_drained = tokio::time::timeout(Duration::from_secs(2), b_drained_rx) + .await + .expect("drain B must complete once the pass drains") + .expect("channel"); + assert!(b_drained, "drain B must succeed"); + assert!(gate.is_closed()); + + b_release_tx.send(()).expect("drive B's guard drop"); + tokio::time::timeout(Duration::from_secs(2), b) + .await + .expect("B must finish") + .expect("join"); + assert!(!gate.is_closed(), "last hold gone — admission restored"); + } + + /// The timeout latch composes with the entry-hold: a timed-out drain + /// leaves the gate closed even though its own hold is gone, and only + /// a later successful drain clears the latch. + #[tokio::test(start_paused = true)] + async fn timed_out_drain_latches_gate_closed_until_a_successful_drain() { + use std::sync::atomic::{AtomicBool, Ordering}; + + let gate = QuiesceGate::default(); + let is_syncing = AtomicBool::new(true); + + assert!( + drain_pass(&gate, &is_syncing, || {}, Duration::from_millis(50)) + .await + .is_none(), + "a wedged pass must time the drain out" + ); + assert!(gate.is_closed(), "timed-out drain leaves the gate latched"); + + // The wedge clears; the next drain succeeds, clears the latch, and + // its guard's drop restores admission. + is_syncing.store(false, Ordering::Release); + let guard = drain_pass(&gate, &is_syncing, || {}, Duration::from_millis(50)) + .await + .expect("drain must succeed once the pass drained"); + assert!(gate.is_closed()); + drop(guard); + assert!(!gate.is_closed(), "successful drain clears the latch"); + } + + /// `SyncSlotGuard` must clear the `is_syncing` slot on panic unwind, + /// not just on normal fall-through. Without this, a pass that panics + /// leaves the flag latched and every subsequent `quiesce()` drain + /// burns its full budget before reporting non-clean — turning one + /// panicked pass into a permanently wedged (slow, never-clean) + /// teardown. + #[test] + fn sync_slot_guard_clears_flag_on_panic_unwind() { + let flag = std::sync::atomic::AtomicBool::new(true); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _slot = SyncSlotGuard(&flag); + panic!("pass body panicked"); + })); + assert!(result.is_err(), "the pass body must have panicked"); + assert!( + !flag.load(std::sync::atomic::Ordering::Acquire), + "guard must clear the slot during unwind" + ); } } diff --git a/packages/rs-platform-wallet/src/manager/platform_address_sync.rs b/packages/rs-platform-wallet/src/manager/platform_address_sync.rs index 36fe1449c28..f6e971d891e 100644 --- a/packages/rs-platform-wallet/src/manager/platform_address_sync.rs +++ b/packages/rs-platform-wallet/src/manager/platform_address_sync.rs @@ -22,9 +22,14 @@ use key_wallet::PlatformP2PKHAddress; use crate::wallet::PlatformAddressTag; use tokio::sync::RwLock; +use dash_async::ThreadRegistry; + use crate::error::PlatformWalletError; use crate::events::PlatformEventManager; -use crate::manager::loop_cancel::LoopCancelGuard; +use crate::manager::{ + coordinator_worker_config, drain_pass, QuiesceGate, QuiesceGuard, SyncSlotGuard, WalletWorker, + COORDINATOR_DRAIN_BUDGET, +}; use crate::wallet::platform_wallet::WalletId; use crate::wallet::PlatformWallet; @@ -95,18 +100,21 @@ impl PlatformAddressSyncSummary { pub struct PlatformAddressSyncManager { wallets: Arc>>>, event_manager: Arc, - /// Generation-guarded cancel-token slot for the background loop — - /// see [`LoopCancelGuard`] for the stale-loop shutdown invariant. - cancel_guard: LoopCancelGuard, + /// Shared registry that owns this loop's lifecycle: it spawns the + /// OS thread, owns its cancellation token, and joins it at shutdown. + /// A generation-guarded slot handles a `stop()` + quick `start()` + /// without a stale loop clobbering the new one. + registry: Arc>, interval_secs: AtomicU64, is_syncing: AtomicBool, - /// Set by [`quiesce`](Self::quiesce) to gate new passes while it - /// drains an in-flight one. `sync_now` bails (after taking the - /// `is_syncing` slot) when this is set, so once `quiesce` observes + /// Gates new passes while a [`quiesce`](Self::quiesce) drains an + /// in-flight one, while a [`QuiesceGuard`] holder mutates state, and + /// terminally once shutdown seals it. `sync_now` bails (after taking the + /// `is_syncing` slot) when it is closed, so once a drain observes /// `is_syncing == false` no further pass can start — giving shutdown /// a real "no more host-visible sync-completed callbacks" barrier /// that cancel-only [`stop`](Self::stop) does not provide. - quiescing: AtomicBool, + quiescing: QuiesceGate, /// Unix seconds of the last completed pass. `0` = never. last_sync_unix: AtomicU64, /// Shared config applied uniformly across wallets and accounts. @@ -121,14 +129,15 @@ impl PlatformAddressSyncManager { pub fn new( wallets: Arc>>>, event_manager: Arc, + registry: Arc>, ) -> Self { Self { wallets, event_manager, - cancel_guard: LoopCancelGuard::new(), + registry, interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS), is_syncing: AtomicBool::new(false), - quiescing: AtomicBool::new(false), + quiescing: QuiesceGate::default(), last_sync_unix: AtomicU64::new(0), config: ArcSwapOption::empty(), } @@ -161,7 +170,7 @@ impl PlatformAddressSyncManager { /// Whether the background loop is currently running. pub fn is_running(&self) -> bool { - self.cancel_guard.is_running() + self.registry.is_running(WalletWorker::PlatformAddressSync) } /// Whether a sync pass is in flight right now. @@ -192,16 +201,22 @@ impl PlatformAddressSyncManager { /// /// The first pass runs immediately; subsequent passes fire every /// [`interval`](Self::interval). + /// + /// **Blocks briefly on restart**: the shared registry synchronously + /// reaps a still-draining prior-generation thread, spinning up to the + /// registry reap backstop (default 1 s) before returning. Call it from + /// the FFI host thread, not an async task. pub fn start(self: Arc) { - let Some((cancel, my_generation)) = self.cancel_guard.install() else { - return; - }; - let handle = tokio::runtime::Handle::current(); + let registry = Arc::clone(&self.registry); let this = self; - std::thread::Builder::new() - .name("platform-address-sync".into()) - .spawn(move || { + // The registry owns the whole lifecycle (see `IdentitySyncManager::start`): + // it takes the teardown latch, installs the cancellation token, spawns + // the thread, and reaps any prior generation under one slot lock. + registry.start_thread( + WalletWorker::PlatformAddressSync, + coordinator_worker_config(), + move |cancel| { handle.block_on(async move { loop { if cancel.is_cancelled() { @@ -216,11 +231,9 @@ impl PlatformAddressSyncManager { _ = cancel.cancelled() => break, } } - - this.cancel_guard.clear_if_current(my_generation); }); - }) - .expect("failed to spawn platform-address-sync thread"); + }, + ); } /// Stop the background sync loop. No-op if not running. @@ -233,9 +246,7 @@ impl PlatformAddressSyncManager { /// the host can free the event-handler context — use /// [`quiesce`](Self::quiesce). pub fn stop(&self) { - if let Some(token) = self.cancel_guard.take() { - token.cancel(); - } + self.registry.cancel(WalletWorker::PlatformAddressSync); } /// Cancel the background loop **and wait for any in-flight sync pass @@ -249,21 +260,75 @@ impl PlatformAddressSyncManager { /// pass that fires `on_platform_address_sync_completed` through a /// now-dangling pointer. /// - /// Mechanism: set the `quiescing` gate so any pass that hasn't yet + /// Mechanism: close the `quiescing` gate so any pass that hasn't yet /// taken the `is_syncing` slot bails, cancel the loop, then wait for /// `is_syncing` to clear. `is_syncing` is held for the whole pass /// including the completion-event dispatch (`sync_now` clears it only /// after `on_platform_address_sync_completed` returns), so its - /// falling edge (with the gate up) is a sound "fully drained" signal. + /// falling edge (with the gate closed) is a sound "fully drained" signal. /// The gate is reopened before returning so a later start/sync works /// normally. - pub async fn quiesce(&self) { - self.quiescing.store(true, Ordering::Release); - self.stop(); - while self.is_syncing.load(Ordering::Acquire) { - tokio::time::sleep(Duration::from_millis(20)).await; - } - self.quiescing.store(false, Ordering::Release); + /// + /// **Bounded** by `COORDINATOR_DRAIN_BUDGET`: returns `false` if + /// the in-flight pass did not drain in time — see + /// `quiesce_within` for the timeout contract. + #[must_use = "a false return means the pass did NOT drain; the caller must fail closed"] + pub async fn quiesce(&self) -> bool { + self.quiesce_within(COORDINATOR_DRAIN_BUDGET).await + } + + /// [`quiesce`](Self::quiesce) with an explicit drain budget. + /// + /// Returns `true` when the drain completed. Returns `false` when + /// `is_syncing` was still held at the deadline — a wedged pass. On + /// that path the `quiescing` gate is deliberately **left closed** so the + /// wedged pass cannot be followed by a fresh one; the caller must + /// treat the coordinator as non-clean. A later successful `quiesce` + /// reopens the gate. + pub(crate) async fn quiesce_within(&self, budget: Duration) -> bool { + // The guard drops here, reopening the gate — this is the + // "drain only" flavor. + self.quiesce_held_within(budget).await.is_some() + } + + /// [`quiesce`](Self::quiesce) that **keeps sync admission shut** until + /// the returned guard drops. + /// + /// `quiesce()` alone reopens the gate the instant it returns, so a + /// caller that then mutates state a pass touches (`reset_platform_address_sync_state`'s watermark + balance reset) runs its + /// mutation with admission already re-opened — a direct pass on a host + /// thread can snapshot pre-mutation state and re-persist it right + /// after. Holding the guard across the whole quiesce → mutate section + /// closes that window. + /// + /// `None` means the in-flight pass did not drain within + /// `COORDINATOR_DRAIN_BUDGET`; the caller must fail closed. + #[must_use = "None means the pass did NOT drain; the caller must fail closed"] + pub(crate) async fn quiesce_held(&self) -> Option> { + self.quiesce_held_within(COORDINATOR_DRAIN_BUDGET).await + } + + /// [`quiesce_held`](Self::quiesce_held) with an explicit drain budget. + pub(crate) async fn quiesce_held_within(&self, budget: Duration) -> Option> { + drain_pass(&self.quiescing, &self.is_syncing, || self.stop(), budget).await + } + + /// [`quiesce_within`](Self::quiesce_within) that **seals** the gate: + /// admission never reopens on this coordinator instance. + /// + /// Used by manager shutdown. Reopening there would let a direct + /// `sync_now` that was already dispatched on a host thread — the FFI + /// resolves the manager under a shared read guard, so it can be + /// mid-flight while `destroy` runs — start a fresh pass *after* the + /// drain concluded and fire persister / completion callbacks through + /// a context the host has since freed. + pub(crate) async fn quiesce_sealed_within(&self, budget: Duration) -> bool { + let guard = self.quiesce_held_within(budget).await; + let drained = guard.is_some(); + // Seal before the guard drops so its Drop cannot reopen. + self.quiescing.seal(); + drop(guard); + drained } /// Run one sync pass across every registered wallet. @@ -278,14 +343,16 @@ impl PlatformAddressSyncManager { { return PlatformAddressSyncSummary::default(); } + // Clears `is_syncing` on every exit path — including panic unwind — + // so a failed pass can never wedge `quiesce()`'s drain. + let _slot = SyncSlotGuard(&self.is_syncing); // A `quiesce()` may have raised the gate between our CAS and // here; if so, release the slot and bail without running a pass // so the drain can complete and shutdown gets a true barrier // (no further `on_platform_address_sync_completed` host callback // after quiesce returns). - if self.quiescing.load(Ordering::Acquire) { - self.is_syncing.store(false, Ordering::Release); + if self.quiescing.is_closed() { return PlatformAddressSyncSummary::default(); } @@ -325,24 +392,67 @@ impl PlatformAddressSyncManager { // free the host event-handler context while this completion // event (FFI callback → host handler) is still pending — a // use-after-free. Holding the flag across the dispatch makes - // quiesce's barrier cover the host callback too. Mirrors the - // ordering in `ShieldedSyncManager::sync_now`. + // quiesce's barrier cover the host callback too (`_slot` drops — + // and clears the flag — only after this dispatch returns). + // Mirrors the ordering in `ShieldedSyncManager::sync_now`. self.event_manager .on_platform_address_sync_completed(&summary); - self.is_syncing.store(false, Ordering::Release); - summary } - /// Sync a single wallet on demand. Does not set the global - /// `is_syncing` flag — callers that care about exclusion should - /// gate on [`is_syncing`] themselves. + /// Test-only: claim the `is_syncing` slot and never release it, + /// standing in for a pass wedged in a network / persister await. + /// Returns `false` if the slot was already taken. + #[cfg(test)] + pub(crate) fn wedge_sync_slot_for_test(&self) -> bool { + self.is_syncing + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + } + + /// Sync a single wallet on demand. + /// + /// Goes through the same admission as [`sync_now`](Self::sync_now) — + /// claim the manager-wide `is_syncing` slot, then honor the quiescing + /// gate — so a per-wallet sync is covered by the drain barrier too. + /// Without that, a reset/teardown that only watched `is_syncing` could + /// conclude "nothing is running" while this call was about to take a + /// wallet's provider lock and persist a fresh watermark over the state + /// just cleared. + /// + /// Returns [`PlatformWalletError::AddressSync`] when another pass holds + /// the slot or admission is shut (a reset/Clear is mutating, or the + /// manager is shutting down) — the caller should retry once sync is + /// idle rather than treat it as a sync failure. pub async fn sync_wallet( &self, wallet_id: &WalletId, ) -> Result, PlatformWalletError> { + if self + .is_syncing + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { + return Err(PlatformWalletError::AddressSync( + "a platform-address sync pass is already in flight; retry once it completes" + .to_string(), + )); + } + // Clears `is_syncing` on every exit path — including panic unwind. + let _slot = SyncSlotGuard(&self.is_syncing); + + // A drain may have closed the gate between our CAS and here (see + // `sync_now`); bail so the drain can complete and its caller gets + // a true barrier. + if self.quiescing.is_closed() { + return Err(PlatformWalletError::AddressSync( + "platform-address sync is quiescing; retry once the reset / teardown completes" + .to_string(), + )); + } + let wallet = { let wallets = self.wallets.read().await; wallets.get(wallet_id).cloned() @@ -410,7 +520,11 @@ mod tests { Arc::clone(&counter) as Arc ])); ( - Arc::new(PlatformAddressSyncManager::new(wallets, event_manager)), + Arc::new(PlatformAddressSyncManager::new( + wallets, + event_manager, + ThreadRegistry::::new(), + )), counter, ) } @@ -467,11 +581,42 @@ mod tests { .await .expect("quiesce did not return after the pass drained"); - assert!(!mgr.quiescing.load(Ordering::Acquire)); + assert!(!mgr.quiescing.is_closed()); assert!(!mgr.is_syncing()); pass.await.unwrap(); } + /// A pass that never drains must NOT hang `quiesce_within` forever: + /// the drain returns `false` at its deadline and deliberately leaves + /// the `quiescing` gate closed (so the wedged pass cannot be followed by + /// a fresh one). A later successful quiesce reopens the gate. Sibling + /// of the dashpay / identity regression tests. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn quiesce_within_times_out_and_leaves_gate_up_when_pass_never_drains() { + let (mgr, _counter) = make_manager(); + + assert!(mgr + .is_syncing + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok()); + + let drained = tokio::time::timeout( + Duration::from_secs(2), + mgr.quiesce_within(Duration::from_millis(100)), + ) + .await + .expect("bounded quiesce must return at its deadline"); + assert!(!drained, "a wedged pass must be reported as non-drained"); + assert!( + mgr.quiescing.is_closed(), + "gate must stay up after a timed-out drain" + ); + + mgr.is_syncing.store(false, Ordering::Release); + assert!(mgr.quiesce().await); + assert!(!mgr.quiescing.is_closed()); + } + /// A `sync_now()` invoked while `quiescing` is set must bail without /// running the pass — in particular, without firing the /// `on_platform_address_sync_completed` host callback. This is the @@ -481,8 +626,9 @@ mod tests { async fn sync_now_bails_when_quiescing() { let (mgr, counter) = make_manager(); - // Raise the gate as `quiesce()` would. - mgr.quiescing.store(true, Ordering::Release); + // Raise the gate as an in-flight `quiesce()` would (a drain holds + // the gate from its first instruction). + let gate_hold = mgr.quiescing.hold(); let summary = mgr.sync_now().await; @@ -491,5 +637,106 @@ mod tests { assert!(summary.is_empty()); assert_eq!(counter.completions.load(AtomicOrdering::SeqCst), 0); assert!(!mgr.is_syncing()); + drop(gate_hold); + } + + /// The barrier `reset_platform_address_sync_state` needs: admission + /// must stay shut for as long as the caller holds the guard, not just + /// until the drain returns. + /// + /// RED before the fix — `quiesce()` reopened the gate on return, so a + /// direct `sync_now` on a host thread could run a full pass (and fire + /// the completion callback) while the reset was still rewriting + /// watermarks and balances, then persist state the reset had cleared. + #[tokio::test] + async fn quiesce_held_bars_passes_until_the_guard_drops() { + let (mgr, counter) = make_manager(); + + let guard = mgr + .quiesce_held() + .await + .expect("an idle coordinator must drain immediately"); + + // Stand-in for the mutation the holder performs (the per-wallet + // reset): a pass dispatched concurrently must not run. + assert!(mgr.sync_now().await.is_empty()); + assert_eq!( + counter.completions.load(AtomicOrdering::SeqCst), + 0, + "no pass — and so no host callback — may run while the guard is held" + ); + + drop(guard); + + // Admission is restored for ordinary use once the mutation is done. + assert!(!mgr.quiescing.is_closed()); + mgr.sync_now().await; + assert_eq!(counter.completions.load(AtomicOrdering::SeqCst), 1); + } + + /// Overlapping holders compose: the gate reopens only when the LAST + /// guard drops. A per-guard boolean would reopen admission at the + /// first drop while the outer holder was still mutating. + #[tokio::test] + async fn overlapping_quiesce_guards_reopen_only_after_the_last_drop() { + let (mgr, _counter) = make_manager(); + + let outer = mgr.quiesce_held().await.expect("drain"); + let inner = mgr.quiesce_held().await.expect("drain"); + + drop(inner); + assert!( + mgr.quiescing.is_closed(), + "the outer holder is still mutating; admission must stay shut" + ); + + drop(outer); + assert!(!mgr.quiescing.is_closed()); + } + + /// Shutdown seals the gate: admission must never reopen, because the + /// FFI frees the host callback context the moment `destroy` returns. + /// + /// A `sync_now` already dispatched on a host thread (the FFI resolves + /// the manager under a shared read guard, so it can be mid-flight + /// while `destroy` runs) must find the gate shut and bail rather than + /// run a pass that fires callbacks into freed memory. + #[tokio::test] + async fn quiesce_sealed_never_reopens_admission() { + let (mgr, counter) = make_manager(); + + assert!(mgr.quiesce_sealed_within(Duration::from_secs(1)).await); + assert!(mgr.quiescing.is_closed(), "seal must leave the gate shut"); + + assert!(mgr.sync_now().await.is_empty()); + assert_eq!(counter.completions.load(AtomicOrdering::SeqCst), 0); + + // Not even an explicit drain reopens a sealed gate. + assert!(mgr.quiesce().await); + assert!(mgr.quiescing.is_closed()); + assert!(mgr.sync_now().await.is_empty()); + assert_eq!(counter.completions.load(AtomicOrdering::SeqCst), 0); + } + + /// `sync_wallet` is a second entry point into the same per-wallet + /// state, so it must observe the same admission as `sync_now` — it + /// used to bypass both the `is_syncing` slot and the gate entirely, + /// which let a per-wallet sync take a wallet's provider lock and + /// persist a fresh watermark right after a reset cleared it. + #[tokio::test] + async fn sync_wallet_is_refused_while_admission_is_shut() { + let (mgr, _counter) = make_manager(); + + let _guard = mgr.quiesce_held().await.expect("drain"); + + let error = mgr + .sync_wallet(&[7u8; 32]) + .await + .expect_err("a per-wallet sync must be refused while a reset holds the gate"); + assert!( + matches!(error, PlatformWalletError::AddressSync(_)), + "expected an AddressSync refusal, got {error:?}" + ); + assert!(!mgr.is_syncing(), "the slot must be released on the bail"); } } diff --git a/packages/rs-platform-wallet/src/manager/shielded_sync.rs b/packages/rs-platform-wallet/src/manager/shielded_sync.rs index 609e9820464..dac831cb809 100644 --- a/packages/rs-platform-wallet/src/manager/shielded_sync.rs +++ b/packages/rs-platform-wallet/src/manager/shielded_sync.rs @@ -34,8 +34,13 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tokio::sync::RwLock; +use dash_async::ThreadRegistry; + use crate::events::PlatformEventManager; -use crate::manager::loop_cancel::LoopCancelGuard; +use crate::manager::{ + coordinator_worker_config, drain_pass, QuiesceGate, QuiesceGuard, SyncSlotGuard, WalletWorker, + COORDINATOR_DRAIN_BUDGET, +}; use crate::wallet::platform_wallet::WalletId; use crate::wallet::shielded::{NetworkShieldedCoordinator, ShieldedSyncSummary}; @@ -139,18 +144,22 @@ pub struct ShieldedSyncManager { /// run first, so an empty slot guarantees no shielded state /// exists). coordinator_slot: Arc>>>, - /// Generation-guarded cancel-token slot for the background loop — - /// see [`LoopCancelGuard`] for the stale-loop shutdown invariant. - cancel_guard: LoopCancelGuard, + /// Shared registry that owns this loop's lifecycle: it spawns the + /// OS thread, owns its cancellation token, and joins it at shutdown. + /// A generation-guarded slot handles a `stop()` + quick `start()` + /// without a stale loop clobbering the new one, and its per-key + /// clearing latch bars a (re)start mid `clear_shielded`. + registry: Arc>, interval_secs: AtomicU64, is_syncing: AtomicBool, - /// Set by [`quiesce`](Self::quiesce) to gate new passes while it - /// drains an in-flight one. `sync_now` / `sync_wallet` bail (after - /// taking the `is_syncing` slot) when this is set, so once `quiesce` - /// observes `is_syncing == false` no further pass can start — giving - /// Clear / stop a real "no more host-visible mutations" barrier that - /// cancel-only [`stop`](Self::stop) does not provide. - quiescing: AtomicBool, + /// Gates new passes while a [`quiesce`](Self::quiesce) drains an + /// in-flight one, while a [`QuiesceGuard`] holder mutates state, and + /// terminally once shutdown seals it. `sync_now` / `sync_wallet` bail + /// (after taking the `is_syncing` slot) when it is closed, so once a + /// drain observes `is_syncing == false` no further pass can start — + /// giving Clear / stop a real "no more host-visible mutations" + /// barrier that cancel-only [`stop`](Self::stop) does not provide. + quiescing: QuiesceGate, /// Unix seconds of the last completed pass. `0` = never. last_sync_unix: AtomicU64, } @@ -159,14 +168,15 @@ impl ShieldedSyncManager { pub fn new( event_manager: Arc, coordinator_slot: Arc>>>, + registry: Arc>, ) -> Self { Self { event_manager, coordinator_slot, - cancel_guard: LoopCancelGuard::new(), + registry, interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS), is_syncing: AtomicBool::new(false), - quiescing: AtomicBool::new(false), + quiescing: QuiesceGate::default(), last_sync_unix: AtomicU64::new(0), } } @@ -186,7 +196,7 @@ impl ShieldedSyncManager { /// Whether the background loop is currently running. pub fn is_running(&self) -> bool { - self.cancel_guard.is_running() + self.registry.is_running(WalletWorker::ShieldedSync) } /// Whether a sync pass is in flight right now. @@ -210,16 +220,25 @@ impl ShieldedSyncManager { /// the underlying `dash-sdk` shielded-sync future is `!Send` (the /// GRPC client state isn't `Send + Sync`). Same trade-off as /// [`PlatformAddressSyncManager::start`](super::platform_address_sync::PlatformAddressSyncManager::start). + /// + /// **Blocks briefly on restart**: the shared registry synchronously + /// reaps a still-draining prior-generation thread, spinning up to the + /// registry reap backstop (default 1 s) before returning. Call it from + /// the FFI host thread, not an async task. pub fn start(self: Arc) { - let Some((cancel, my_generation)) = self.cancel_guard.install() else { - return; - }; - let handle = tokio::runtime::Handle::current(); + let registry = Arc::clone(&self.registry); let this = self; - std::thread::Builder::new() - .name("shielded-sync".into()) - .spawn(move || { + // The registry owns the whole lifecycle under one slot lock: it + // refuses the start if teardown has latched `closing` OR a + // `clear_shielded` holds this key's clearing latch (so no fresh pass + // can re-persist notes into the store the clear is about to wipe), + // installs the cancellation token, spawns the thread, and reaps any + // prior generation. No check-then-spawn gap to race. + registry.start_thread( + WalletWorker::ShieldedSync, + coordinator_worker_config(), + move |cancel| { handle.block_on(async move { loop { if cancel.is_cancelled() { @@ -241,11 +260,9 @@ impl ShieldedSyncManager { _ = cancel.cancelled() => break, } } - - this.cancel_guard.clear_if_current(my_generation); }); - }) - .expect("failed to spawn shielded-sync thread"); + }, + ); } /// Stop the background sync loop. No-op if not running. @@ -257,9 +274,7 @@ impl ShieldedSyncManager { /// nothing more will be persisted" barrier — required by Clear, /// unregister, and rebind — use [`quiesce`](Self::quiesce). pub fn stop(&self) { - if let Some(token) = self.cancel_guard.take() { - token.cancel(); - } + self.registry.cancel(WalletWorker::ShieldedSync); } /// Cancel the background loop **and wait for any in-flight sync pass @@ -273,19 +288,76 @@ impl ShieldedSyncManager { /// cannot be raced by a pass that re-persists notes after the caller /// believed sync had stopped. /// - /// Mechanism: set the `quiescing` gate so any pass that hasn't yet + /// Mechanism: close the `quiescing` gate so any pass that hasn't yet /// taken the `is_syncing` slot bails, cancel the loop, then wait for /// `is_syncing` to clear. `is_syncing` is held for the whole pass /// including the persister fan-out, so its falling edge (with the - /// gate up) is a sound "fully drained" signal. The gate is reopened - /// before returning so a later start/sync works normally. - pub async fn quiesce(&self) { - self.quiescing.store(true, Ordering::Release); - self.stop(); - while self.is_syncing.load(Ordering::Acquire) { - tokio::time::sleep(Duration::from_millis(20)).await; - } - self.quiescing.store(false, Ordering::Release); + /// gate closed) is a sound "fully drained" signal. The gate is + /// reopened before returning so a later start/sync works normally — + /// a caller that must keep admission shut across a follow-up state + /// mutation uses [`quiesce_held`](Self::quiesce_held) instead. + /// + /// **Bounded** by `COORDINATOR_DRAIN_BUDGET`: returns `false` if + /// the in-flight pass did not drain in time — see + /// `quiesce_within` for the timeout contract. + #[must_use = "a false return means the pass did NOT drain; the caller must fail closed"] + pub async fn quiesce(&self) -> bool { + self.quiesce_within(COORDINATOR_DRAIN_BUDGET).await + } + + /// [`quiesce`](Self::quiesce) with an explicit drain budget. + /// + /// Returns `true` when the drain completed. Returns `false` when + /// `is_syncing` was still held at the deadline — a wedged pass. On + /// that path the `quiescing` gate is deliberately **left closed** so + /// the wedged pass cannot be followed by a fresh one; the caller must + /// treat the coordinator as non-clean (Clear aborts fail-closed, the + /// FFI stop surfaces `ErrorShutdownIncomplete`). A later successful + /// `quiesce` reopens the gate. + pub(crate) async fn quiesce_within(&self, budget: Duration) -> bool { + // The guard drops here, reopening the gate — this is the + // "drain only" flavor. + self.quiesce_held_within(budget).await.is_some() + } + + /// [`quiesce`](Self::quiesce) that **keeps sync admission shut** until + /// the returned guard drops. + /// + /// Required by `clear_shielded`: `quiesce()` alone reopens the gate the + /// instant it returns, so the wipe that follows runs with admission + /// already re-opened and a direct `sync_now` / `sync_wallet` on a host + /// thread can snapshot the old account set and refill the commitment + /// tree right after `coord.clear()` reset it. Holding the guard across + /// the whole quiesce → mutate section closes that window. + /// + /// `None` means the in-flight pass did not drain within + /// `COORDINATOR_DRAIN_BUDGET`; the caller must fail closed. + #[must_use = "None means the pass did NOT drain; the caller must fail closed"] + pub(crate) async fn quiesce_held(&self) -> Option> { + self.quiesce_held_within(COORDINATOR_DRAIN_BUDGET).await + } + + /// [`quiesce_held`](Self::quiesce_held) with an explicit drain budget. + pub(crate) async fn quiesce_held_within(&self, budget: Duration) -> Option> { + drain_pass(&self.quiescing, &self.is_syncing, || self.stop(), budget).await + } + + /// [`quiesce_within`](Self::quiesce_within) that **seals** the gate: + /// admission never reopens on this coordinator instance. + /// + /// Used by manager shutdown. Reopening there would let a direct + /// `sync_now` that was already dispatched on a host thread — the FFI + /// resolves the manager under a shared read guard, so it can be + /// mid-flight while `destroy` runs — start a fresh pass *after* the + /// drain concluded and fire persister / completion callbacks through + /// a context the host has since freed. + pub(crate) async fn quiesce_sealed_within(&self, budget: Duration) -> bool { + let guard = self.quiesce_held_within(budget).await; + let drained = guard.is_some(); + // Seal before the guard drops so its Drop cannot reopen. + self.quiescing.seal(); + drop(guard); + drained } /// Run one sync pass across every registered wallet. @@ -307,12 +379,14 @@ impl ShieldedSyncManager { { return ShieldedSyncPassSummary::default(); } + // Clears `is_syncing` on every exit path — including panic unwind — + // so a failed pass can never wedge `quiesce()`'s drain. + let _slot = SyncSlotGuard(&self.is_syncing); // A `quiesce()` may have raised the gate between our CAS and // here; if so, release the slot and bail without running a pass // so the drain can complete and Clear/stop get a true barrier. - if self.quiescing.load(Ordering::Acquire) { - self.is_syncing.store(false, Ordering::Release); + if self.quiescing.is_closed() { return ShieldedSyncPassSummary::default(); } @@ -355,11 +429,10 @@ impl ShieldedSyncManager { // while this completion event (FFI callback → Swift // `handleShieldedSyncCompleted`) is still pending — surfacing a // stale post-stop/post-clear event. Holding the flag across the - // dispatch makes quiesce's barrier cover the event too. + // dispatch makes quiesce's barrier cover the event too (`_slot` + // drops — and clears the flag — only after this dispatch returns). self.event_manager.on_shielded_sync_completed(&summary); - self.is_syncing.store(false, Ordering::Release); - summary } @@ -402,16 +475,17 @@ impl ShieldedSyncManager { { return Ok(None); } + // Clears `is_syncing` on every exit path — including panic unwind — + // so a failed pass can never wedge `quiesce()`'s drain. + let _slot = SyncSlotGuard(&self.is_syncing); // Bail if a `quiesce()` raised the gate after our CAS (see // `sync_now`) so the drain barrier holds. - if self.quiescing.load(Ordering::Acquire) { - self.is_syncing.store(false, Ordering::Release); + if self.quiescing.is_closed() { return Ok(None); } let pass = coordinator.sync(force).await; - self.is_syncing.store(false, Ordering::Release); // Extract this wallet's slice from the network-wide pass // summary. If the wallet is registered, we'll get back an diff --git a/packages/rs-platform-wallet/src/spv/runtime.rs b/packages/rs-platform-wallet/src/spv/runtime.rs index 2dae6fa2ebe..b94a3e0100c 100644 --- a/packages/rs-platform-wallet/src/spv/runtime.rs +++ b/packages/rs-platform-wallet/src/spv/runtime.rs @@ -25,22 +25,70 @@ use crate::wallet::platform_wallet::PlatformWalletInfo; type SpvClient = DashSpvClient, PeerNetworkManager, DiskStorageManager>; +/// Graceful join budget for the SPV run loop before escalating to `abort`. const SPV_STOP_TIMEOUT: Duration = Duration::from_secs(15); -/// Join a stopped SPV runner, escalating to cancellation after `timeout` but -/// never returning until Tokio confirms that the task has terminated. -async fn join_spv_task(mut handle: JoinHandle<()>, timeout: Duration) { +/// Budget for `DashSpvClient::stop()` itself. +/// +/// dash-spv's stop joins its internal monitors, and those monitors dispatch +/// host event callbacks **synchronously** — a callback blocked in host code +/// (an FFI persister `store`, say) makes the stop unbounded. Since +/// [`SpvRuntime::stop`] sits on the path the FFI `destroy` must return +/// through, an unbounded stop hangs teardown outright instead of surfacing +/// `ErrorShutdownIncomplete`. On timeout the partially-stopped client is +/// dropped and the stop is reported as an error, so the caller treats SPV +/// as non-clean. +const SPV_CLIENT_STOP_BUDGET: Duration = Duration::from_secs(15); + +/// Post-`abort` confirmation grace for the SPV run loop. +/// +/// An abort only lands at the task's next await point, so a task parked in +/// synchronous host-callback code cannot be interrupted at all. Without this +/// bound the post-abort `handle.await` waits forever — the same hang the +/// graceful timeout above was meant to escape. +const SPV_ABORT_GRACE: Duration = Duration::from_secs(2); + +/// Join a stopped SPV runner, escalating to cancellation after `timeout`. +/// +/// Returns `None` once Tokio has confirmed the task terminated. Returns +/// `Some(handle)` when it is *still live* after the post-abort grace: the +/// caller must re-park that handle (so a teardown retry re-joins it rather +/// than silently detaching a callback-capable task) and report SPV as +/// non-clean. +#[must_use = "a returned handle is a still-live task that must be re-parked and reported non-clean"] +async fn join_spv_task(handle: JoinHandle<()>, timeout: Duration) -> Option> { + join_spv_task_within(handle, timeout, SPV_ABORT_GRACE).await +} + +/// [`join_spv_task`] with an explicit post-abort grace, so tests can drive +/// the survived-the-abort path without waiting out [`SPV_ABORT_GRACE`]. +#[must_use = "a returned handle is a still-live task that must be re-parked and reported non-clean"] +async fn join_spv_task_within( + mut handle: JoinHandle<()>, + timeout: Duration, + abort_grace: Duration, +) -> Option> { match tokio::time::timeout(timeout, &mut handle).await { - Ok(Ok(())) => {} + Ok(Ok(())) => None, Ok(Err(error)) => { tracing::warn!(?error, "SPV background run loop join error"); + None } Err(_) => { tracing::warn!("SPV stop: background run loop did not unwind in time; aborting it"); handle.abort(); - if let Err(error) = handle.await { - if !error.is_cancelled() { + match tokio::time::timeout(abort_grace, &mut handle).await { + Ok(Err(error)) if !error.is_cancelled() => { tracing::warn!(?error, "SPV background run loop abort join error"); + None + } + Ok(_) => None, + Err(_) => { + tracing::warn!( + "SPV stop: background run loop survived abort for {abort_grace:?}; \ + keeping the handle for a teardown retry" + ); + Some(handle) } } } @@ -227,7 +275,20 @@ impl SpvRuntime { result } - /// Stop SPV sync gracefully. Unlocks the data dir safely + /// Stop SPV sync gracefully. Unlocks the data dir safely. + /// + /// **Every phase is bounded** — `stop` runs on the path + /// [`PlatformWalletManager::shutdown`](crate::manager::PlatformWalletManager::shutdown) + /// and therefore the FFI's `destroy` must return through, so a wedged + /// host callback has to surface as an error rather than hang teardown: + /// the client stop is capped at [`SPV_CLIENT_STOP_BUDGET`], the run-loop + /// join at [`SPV_STOP_TIMEOUT`] with an [`SPV_ABORT_GRACE`] post-abort + /// confirmation. A run loop that outlives all of that is **re-parked**, + /// not detached, so a teardown retry re-joins it — and the error return + /// keeps it out of a clean shutdown verdict. + /// + /// Idempotent: a second call finds no client and re-joins whatever the + /// first call re-parked. pub async fn stop(&self) -> Result<(), PlatformWalletError> { let taken = { let mut client = self.client.write().await; @@ -235,20 +296,47 @@ impl SpvRuntime { }; let stop_result = match taken { - Some(c) => c - .stop() - .await - .map_err(|e| PlatformWalletError::SpvError(e.to_string())), + Some(c) => match tokio::time::timeout(SPV_CLIENT_STOP_BUDGET, c.stop()).await { + Ok(result) => result.map_err(|e| PlatformWalletError::SpvError(e.to_string())), + Err(_) => { + // The client is dropped with the timed-out future. The + // data-dir lock may outlive this call, which is strictly + // better than never returning from `destroy`. + tracing::warn!( + "SPV client stop did not complete within {:?}; abandoning it", + SPV_CLIENT_STOP_BUDGET + ); + Err(PlatformWalletError::SpvError(format!( + "SPV client stop did not complete within {SPV_CLIENT_STOP_BUDGET:?}" + ))) + } + }, None => Ok(()), }; self.peer_tracker.clear(); let handle = self.task.lock().expect("spv task mutex poisoned").take(); - if let Some(handle) = handle { - join_spv_task(handle, SPV_STOP_TIMEOUT).await; - } + let join_result = match handle { + None => Ok(()), + Some(handle) => match join_spv_task(handle, SPV_STOP_TIMEOUT).await { + None => Ok(()), + Some(live) => { + // Re-park rather than drop: dropping a `JoinHandle` + // detaches the task, and this one can still reach host + // callbacks. Keeping it lets a teardown retry re-join. + *self.task.lock().expect("spv task mutex poisoned") = Some(live); + Err(PlatformWalletError::SpvError( + "SPV background run loop did not terminate after abort; \ + it is still tracked for a retry" + .to_string(), + )) + } + }, + }; - stop_result + // A failed client stop is the more informative diagnosis, so it wins; + // either one makes the caller's shutdown verdict non-clean. + stop_result.and(join_result) } /// Spawn the sync loop of an already-[`start`]ed client on the current @@ -458,13 +546,45 @@ mod shutdown_tests { }); started_rx.await.expect("SPV task should start"); - join_spv_task(handle, SPV_STOP_TIMEOUT).await; + assert!( + join_spv_task(handle, SPV_STOP_TIMEOUT).await.is_none(), + "an abortable task must be confirmed terminated, not returned as live" + ); assert!( dropped.load(Ordering::SeqCst), "abort must be joined so task-owned callback state is dropped" ); } + + /// A run loop parked in **synchronous** code cannot be interrupted by + /// `abort` — the cancellation only lands at the next await point, which + /// never comes. The post-abort confirmation must therefore be bounded + /// and hand the still-live handle back, so `stop` can re-park it (a + /// dropped `JoinHandle` detaches the task, and this one can still reach + /// host callbacks) and report SPV non-clean. + /// + /// Without the post-abort deadline this test hangs: that is exactly the + /// hang that reached the FFI's `destroy` before this fix. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn spv_task_surviving_abort_is_returned_for_reparking() { + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + let handle = tokio::spawn(async move { + let _ = started_tx.send(()); + // Stands in for a monitor blocked inside a synchronous host + // callback: no await point for the abort to land on. + std::thread::sleep(Duration::from_millis(500)); + }); + started_rx.await.expect("SPV task should start"); + + let live = + join_spv_task_within(handle, Duration::from_millis(10), Duration::from_millis(20)) + .await + .expect("an un-abortable task must be handed back, never silently detached"); + + // Cleanup: the blocking section does end eventually. + let _ = live.await; + } } impl std::fmt::Debug for SpvRuntime { diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rs index db9423efa35..62d174bb650 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rs @@ -88,18 +88,86 @@ impl PaymentTaskTracker { true } + /// Unbounded drain — test-only convenience over + /// [`quiesce_within`](Self::quiesce_within) with an effectively + /// infinite budget. + #[cfg(test)] async fn quiesce(&self) { + let _ = self + .quiesce_within(std::time::Duration::from_secs(60 * 60)) + .await; + } + + /// Close admission and join every admitted task, bounded by `budget`. + /// + /// Returns `true` when every task terminated. Two phases, each under a + /// **shared** deadline so the whole drain is bounded by + /// `budget + PAYMENT_ABORT_GRACE` regardless of how many stragglers + /// there are: every task gets until `budget` to finish on its own, then + /// every survivor is aborted *first* and only then confirmed, all + /// against one [`PAYMENT_ABORT_GRACE`](crate::manager::PAYMENT_ABORT_GRACE) + /// deadline. (Aborting and confirming one straggler at a time would + /// cost `survivors × PAYMENT_ABORT_GRACE`, which is not the bound + /// `shutdown` advertises.) + /// + /// A task stuck in a synchronous call (e.g. an FFI persister `store`) + /// cannot be interrupted by an abort — it is put **back into the + /// tracker** (so a destroy retry re-joins it instead of silently + /// detaching a callback-capable task) and the method returns `false`. + /// + /// Aborting is safe here: the payment hooks are reconciliation-based + /// (re-derivable from wallet transaction records on the next launch), + /// so a hook cancelled between awaits loses no unrecoverable state. + async fn quiesce_within(&self, budget: std::time::Duration) -> bool { let handles = { let mut state = self.state.lock().expect("payment task mutex poisoned"); state.accepting = false; std::mem::take(&mut state.handles) }; - for handle in handles { - if let Err(error) = handle.await { - tracing::warn!(?error, "DashPay payment task join error"); + // Phase 1 — graceful, one shared deadline for all tasks. + let deadline = tokio::time::Instant::now() + budget; + let mut survivors = Vec::new(); + for mut handle in handles { + match tokio::time::timeout_at(deadline, &mut handle).await { + Ok(Ok(())) => {} + Ok(Err(error)) => tracing::warn!(?error, "DashPay payment task join error"), + Err(_) => survivors.push(handle), + } + } + + if survivors.is_empty() { + return true; + } + + // Phase 2 — abort EVERY straggler before awaiting any of them, so + // the grace below is paid once rather than once per straggler. + for handle in &survivors { + handle.abort(); + } + let abort_deadline = tokio::time::Instant::now() + crate::manager::PAYMENT_ABORT_GRACE; + let mut still_live = Vec::new(); + for mut handle in survivors { + match tokio::time::timeout_at(abort_deadline, &mut handle).await { + Ok(Err(error)) if !error.is_cancelled() => { + tracing::warn!(?error, "DashPay payment task abort join error"); + } + Ok(_) => {} + Err(_) => still_live.push(handle), } } + + if still_live.is_empty() { + return true; + } + tracing::warn!( + survivors = still_live.len(), + "DashPay payment tasks did not terminate within the drain budget; \ + keeping them tracked for a retry" + ); + let mut state = self.state.lock().expect("payment task mutex poisoned"); + state.handles.extend(still_live); + false } #[cfg(test)] @@ -124,9 +192,15 @@ impl DashPayPaymentHandler { } /// Stop admitting callback-bearing work and join every task admitted - /// before the gate closed. Idempotent. - pub(crate) async fn quiesce(&self) { - self.tasks.quiesce().await; + /// before the gate closed, bounded by `budget`. Idempotent. + /// + /// Returns `false` when a task outlived the budget (and its post-abort + /// grace); the straggler stays tracked so a retry can re-join it, and + /// the caller must report the shutdown non-clean — these tasks clone + /// the FFI persister and can fire host callbacks. + #[must_use = "a false return means a callback-capable task is still live; report non-clean"] + pub(crate) async fn quiesce_within(&self, budget: std::time::Duration) -> bool { + self.tasks.quiesce_within(budget).await } } @@ -439,4 +513,63 @@ mod tests { tasks.quiesce().await; assert!(!tasks.spawn(async {})); } + + /// A straggler parked at an await point must be reaped by the abort + /// escalation: `quiesce_within` stays bounded and still reports a + /// clean drain because the task verifiably terminated. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn quiesce_within_aborts_await_parked_straggler_and_reports_clean() { + let tasks = Arc::new(PaymentTaskTracker::new()); + assert!(tasks.spawn(async { + std::future::pending::<()>().await; + })); + + let drained = tokio::time::timeout( + std::time::Duration::from_secs(5), + tasks.quiesce_within(std::time::Duration::from_millis(50)), + ) + .await + .expect("bounded quiesce must return"); + assert!( + drained, + "an await-parked task is abortable and must count as terminated" + ); + assert!(!tasks.spawn(async {}), "admission must stay closed"); + } + + /// A straggler stuck in a synchronous call (no await point — an abort + /// cannot land) must NOT be silently detached: `quiesce_within` + /// returns `false` and keeps the handle tracked so a retry re-joins + /// it once the blocking call finally returns. + #[tokio::test(flavor = "multi_thread", worker_threads = 3)] + async fn quiesce_within_keeps_unabortable_straggler_tracked_and_reports_false() { + let tasks = Arc::new(PaymentTaskTracker::new()); + let (started_tx, started_rx) = tokio::sync::oneshot::channel(); + assert!(tasks.spawn(async move { + let _ = started_tx.send(()); + // Stands in for a synchronous FFI persister call: blocks the + // task without ever reaching an await point, so the abort in + // `quiesce_within` cannot take effect. + std::thread::sleep(std::time::Duration::from_millis(2500)); + })); + started_rx.await.expect("straggler should start"); + + let drained = tasks + .quiesce_within(std::time::Duration::from_millis(50)) + .await; + assert!( + !drained, + "an uninterruptible straggler must be reported non-clean" + ); + + // The handle stayed tracked: a retry joins it once the blocking + // call returns. + let retry = tokio::time::timeout( + std::time::Duration::from_secs(10), + tasks.quiesce_within(std::time::Duration::from_secs(10)), + ) + .await + .expect("retry quiesce must return"); + assert!(retry, "retry must re-join the tracked straggler"); + } } diff --git a/packages/rs-unified-sdk-jni/src/events.rs b/packages/rs-unified-sdk-jni/src/events.rs index e648101bf75..3be99418b8f 100644 --- a/packages/rs-unified-sdk-jni/src/events.rs +++ b/packages/rs-unified-sdk-jni/src/events.rs @@ -39,9 +39,11 @@ use std::panic::{catch_unwind, AssertUnwindSafe}; /// Boxed context for the event-handler vtable. Holds the Kotlin /// `NativeWalletEventBridge` as a `GlobalRef` so it survives across the -/// vtable's lifetime and across threads. Owned by the manager bundle in -/// [`crate::wallet_manager`]; dropped only after the native manager's -/// `shutdown()` has quiesced every callback-firing task. +/// vtable's lifetime and across threads. Ownership transfers to the +/// native manager at create (the vtable's `release_fn` is +/// [`release_event_ctx`]): Rust frees the box — and with it the +/// `GlobalRef` — exactly once, when the manager and every worker that +/// could still dispatch an event have dropped their references. pub(crate) struct KotlinEventCtx { pub(crate) bridge: GlobalRef, } @@ -309,5 +311,21 @@ pub(crate) fn build_event_vtable(context: *mut c_void) -> EventHandlerCallbacks on_shielded_sync_completed_fn: Some(tramp_shielded_sync_completed), on_shielded_sync_progress_fn: Some(tramp_shielded_sync_progress), on_shielded_tree_progress_fn: Some(tramp_shielded_tree_progress), + release_fn: Some(release_event_ctx), + } +} + +/// `release_fn` for the event vtable: frees the boxed [`KotlinEventCtx`] +/// when the native manager's last event-handler reference drops. The FFI +/// guarantees exactly one call, which may land on any Rust thread — +/// `GlobalRef`'s own `Drop` attaches that thread to the JVM before +/// deleting the reference, so no manual attach is needed here. +/// +/// # Safety +/// `context` must be the live boxed [`KotlinEventCtx`] this vtable was +/// built around, never freed elsewhere. +unsafe extern "C" fn release_event_ctx(context: *mut c_void) { + if !context.is_null() { + drop(Box::from_raw(context as *mut KotlinEventCtx)); } } diff --git a/packages/rs-unified-sdk-jni/src/persistence.rs b/packages/rs-unified-sdk-jni/src/persistence.rs index e444148cc22..5c5e11cd808 100644 --- a/packages/rs-unified-sdk-jni/src/persistence.rs +++ b/packages/rs-unified-sdk-jni/src/persistence.rs @@ -77,7 +77,11 @@ use platform_wallet_ffi::shielded_persistence::{ /// Boxed context handed to every trampoline via `callbacks.context`. /// Holds the Kotlin bridge as a `GlobalRef` so it survives across the -/// vtable's lifetime and across threads. +/// vtable's lifetime and across threads. Ownership transfers to the +/// native manager at create (the vtable's `release_fn` is +/// [`release_persistence_ctx`]): Rust frees the box — and with it the +/// `GlobalRef` — exactly once, when the manager and every worker that +/// cloned its persister have dropped their references. pub struct KotlinPersistenceCtx { pub(crate) bridge: GlobalRef, } @@ -172,6 +176,23 @@ pub(crate) fn build_vtable(context: *mut c_void) -> PersistenceCallbacks { // invitation flow refuses to run on Android rather than create a // non-durable voucher whose one-time key could be reused on restart. on_persist_invitations_fn: None, + release_fn: Some(release_persistence_ctx), + } +} + +/// `release_fn` for the persistence vtable: frees the boxed +/// [`KotlinPersistenceCtx`] when the native manager's last persister +/// reference drops. The FFI guarantees exactly one call, which may land +/// on any Rust thread — `GlobalRef`'s own `Drop` attaches that thread to +/// the JVM before deleting the reference, so no manual attach is needed +/// here. +/// +/// # Safety +/// `context` must be the live boxed [`KotlinPersistenceCtx`] this vtable +/// was built around, never freed elsewhere. +unsafe extern "C" fn release_persistence_ctx(context: *mut c_void) { + if !context.is_null() { + drop(Box::from_raw(context as *mut KotlinPersistenceCtx)); } } diff --git a/packages/rs-unified-sdk-jni/src/wallet_manager.rs b/packages/rs-unified-sdk-jni/src/wallet_manager.rs index 8e0f3e676b5..0a27da829b1 100644 --- a/packages/rs-unified-sdk-jni/src/wallet_manager.rs +++ b/packages/rs-unified-sdk-jni/src/wallet_manager.rs @@ -13,20 +13,21 @@ //! manager, matching `PlatformWalletManager.swift`, which holds only a //! persistence handler + event handler. //! -//! ## Context ownership (the subtle part) +//! ## Context ownership //! -//! `platform_wallet_manager_create` consumes both vtables by value via -//! `std::ptr::read`, copying the `context` pointer into the manager's -//! `FFIPersister` / `FFIEventHandler`. Neither has a `Drop`, so Rust -//! never frees the context — exactly the `passUnretained` model the Swift -//! SDK uses (the host owns the callback object's lifetime). -//! -//! We therefore box each context (persistence bridge + event bridge as -//! JNI `GlobalRef`s) and keep the two box pointers alongside the manager -//! handle in a [`ManagerBundle`]. [`Java_..._nativeDestroy`] runs -//! `platform_wallet_manager_destroy` first — which calls `shutdown()` to -//! quiesce every callback-firing task — and only then drops the context -//! boxes, so no task can fire against a freed `GlobalRef`. +//! Both vtables are built with a `release_fn`, so +//! `platform_wallet_manager_create` takes **ownership** of the boxed +//! contexts (persistence bridge + event bridge as JNI `GlobalRef`s): +//! the native manager keeps each box alive for exactly as long as any +//! worker can still fire a callback through it, and frees it — on +//! whatever thread the last reference drops on — via the vtable's +//! `release_fn` (`GlobalRef`'s own `Drop` re-attaches the thread to the +//! JVM). [`Java_..._nativeDestroy`] therefore only destroys the manager; +//! it never touches the context boxes, and a worker that straggles past +//! destroy keeps its bridge alive instead of dereferencing a freed +//! `GlobalRef`. The create-failure path is the one place this JNI layer +//! still frees the boxes itself, because a failed create never took +//! ownership. //! //! ## Result convention //! @@ -65,14 +66,13 @@ use rs_sdk_ffi::{dash_sdk_get_inner_sdk_ptr, SDKHandle}; // ── Manager bundle ──────────────────────────────────────────────────── -/// Owns the native manager handle plus the two context boxes whose -/// `GlobalRef`s back the persistence + event vtables the manager copied. -/// Boxed and returned to Kotlin as a single `jlong`; freed by +/// Owns the native manager handle. The persistence/event context boxes +/// are owned by the native manager itself (their vtables carry a +/// `release_fn`), so the bundle no longer tracks them. Boxed and +/// returned to Kotlin as a single `jlong`; freed by /// [`Java_..._nativeDestroy`]. struct ManagerBundle { manager_handle: Handle, - persistence_ctx: *mut KotlinPersistenceCtx, - event_ctx: *mut KotlinEventCtx, } // ── Exports: lifecycle ──────────────────────────────────────────────── @@ -188,7 +188,10 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_n ) }; if take_pwffi_error(env, result) { - // Manager was not created — drop both context boxes. + // Manager was not created, so it never took ownership of the + // context boxes — reclaim them here (the only place this JNI + // layer frees them; every success path leaves that to the + // native manager's `release_fn`). unsafe { drop(Box::from_raw(persistence_ctx)); drop(Box::from_raw(event_ctx)); @@ -196,11 +199,7 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_n return 0; } - let bundle = Box::new(ManagerBundle { - manager_handle, - persistence_ctx, - event_ctx, - }); + let bundle = Box::new(ManagerBundle { manager_handle }); Box::into_raw(bundle) as jlong }) } @@ -286,10 +285,15 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_n }) } -/// Destroy a manager bundle: shut down the native manager (quiesces every -/// callback-firing task), then drop the persistence + event context -/// boxes. Safe on 0. Idempotent is the caller's responsibility (Kotlin's -/// `AtomicLong` handle guard calls this exactly once). +/// Destroy a manager bundle: shut down the native manager (bounded +/// quiesce + join of every callback-firing task). The persistence/event +/// context boxes are owned by the native manager, which frees them via +/// each vtable's `release_fn` once its last worker reference drops — at +/// destroy for a clean shutdown, or when a straggling worker finally +/// exits otherwise. Either way this layer has nothing to free and +/// nothing to deliberately leak. Safe on 0. Idempotent is the caller's +/// responsibility (Kotlin's `AtomicLong` handle guard calls this exactly +/// once). #[no_mangle] pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_nativeDestroy( mut env: JNIEnv, @@ -303,23 +307,12 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_n // SAFETY: bundle is a live ManagerBundle pointer from nativeCreate, // consumed exactly once here. let b = unsafe { Box::from_raw(bundle as *mut ManagerBundle) }; - // shutdown() runs to completion before returning — no task may - // fire a callback after this. - let result = + let mut result = unsafe { platform_wallet_ffi::platform_wallet_manager_destroy(b.manager_handle) }; - // destroy is documented to always return ok; free the message if any. - let mut result = result; - unsafe { platform_wallet_ffi_result_free(&mut result) }; - // Now safe to drop the context boxes (their GlobalRefs release the - // Kotlin bridges). - unsafe { - if !b.persistence_ctx.is_null() { - drop(Box::from_raw(b.persistence_ctx)); - } - if !b.event_ctx.is_null() { - drop(Box::from_raw(b.event_ctx)); - } + if result.code != PlatformWalletFFIResultCode::Success { + log::error!("manager destroy failed with code {:?}", result.code); } + unsafe { platform_wallet_ffi_result_free(&mut result) }; }) } diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift index c0ed0526514..fe232afabd0 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift @@ -2,6 +2,7 @@ import Foundation import SwiftData import Combine import DashSDKFFI +import os.log /// Lock-guarded monotonic generation counter, safe to read and bump from /// any thread. Used to drop sync completion events that belong to a @@ -85,6 +86,11 @@ public struct PlatformWalletPersistenceCapabilities: Equatable, Sendable { /// class in the middle. @MainActor public class PlatformWalletManager: ObservableObject { + fileprivate nonisolated static let log = Logger( + subsystem: "dashpay.SwiftDashSDK", + category: "PlatformWallet" + ) + // MARK: - Published observables /// Whether [`configure`] has been called successfully. @@ -226,8 +232,10 @@ public class PlatformWalletManager: ObservableObject { /// FFI handle; `NULL_HANDLE` until [`configure`] is called. internal private(set) var handle: Handle = NULL_HANDLE - /// Retained for the lifetime of the FFI handle so the callback - /// context pointer remains valid. + /// Convenience access for Swift-side callers (e.g. `persistence`). + /// Lifetime for the FFI callback context is NOT this reference's job: + /// Rust holds its own retained reference (transferred at `configure`) + /// and releases it when its last worker drops. private var persistenceHandler: PlatformWalletPersistenceHandler? /// SwiftData container + network captured at `configure`, used to build a @@ -237,8 +245,9 @@ public class PlatformWalletManager: ObservableObject { private var modelContainer: ModelContainer? private var signerNetwork: Network? - /// Retained for the lifetime of the FFI handle so the event-handler - /// context pointer remains valid. + /// Convenience reference; the FFI callback context's lifetime is + /// owned by Rust (retained reference transferred at `configure`, + /// released when its last worker drops), not by this property. private var eventHandler: PlatformWalletEventHandler? /// Background task that polls SPV progress. @@ -259,15 +268,25 @@ public class PlatformWalletManager: ObservableObject { deinit { progressPollTask?.cancel() if handle != NULL_HANDLE { - // Stop the network event source before releasing the manager's - // unretained callback contexts. Rust's destroy path provides the - // authoritative join barrier; this explicit stop is defense in - // depth for the Swift wrapper's teardown order. + // Stop the network event source first as defense in depth for + // the teardown order; Rust's destroy path provides the + // authoritative join barrier. platform_wallet_manager_spv_stop(handle).discard() platform_wallet_manager_platform_address_sync_stop(handle).discard() platform_wallet_manager_shielded_sync_stop(handle).discard() platform_wallet_manager_dashpay_sync_stop(handle).discard() - platform_wallet_manager_destroy(handle).discard() + // Rust OWNS the persistence/event callback handlers (they were + // handed over retained at `configure`, with a `release_fn`): + // any worker that outlives destroy keeps its handler alive + // through that retain and Rust releases it when the worker + // exits. Nothing to leak, retain, or gate on here — ARC + // releasing this class's own references below is always safe. + let destroyResult = PlatformWalletResult(platform_wallet_manager_destroy(handle)) + if !destroyResult.isSuccess { + Self.log.error( + "Platform wallet manager teardown failed with \(String(describing: destroyResult.code), privacy: .public): \(destroyResult.message ?? "", privacy: .public)" + ) + } } } @@ -332,13 +351,26 @@ public class PlatformWalletManager: ObservableObject { let eventHandler = PlatformWalletEventHandler(manager: self) var eventHandlerCallbacks = eventHandler.makeCallbacks() - try platform_wallet_manager_create_with_persistence_capabilities( - sdkPointer, - &persistence, - &eventHandlerCallbacks, - &declaredCapabilities, - &handle - ).check() + do { + try platform_wallet_manager_create_with_persistence_capabilities( + sdkPointer, + &persistence, + &eventHandlerCallbacks, + &declaredCapabilities, + &handle + ).check() + } catch { + // A failed create never took ownership of the retained callback + // contexts (`makeCallbacks` pre-retains for the transfer), so + // balance the retains here or the handlers leak. + if let context = persistence.context { + Unmanaged.fromOpaque(context).release() + } + if let context = eventHandlerCallbacks.context { + Unmanaged.fromOpaque(context).release() + } + throw error + } var effectiveCapabilities = PersistenceCapabilitiesFFI( version: 0, diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift index 64c40d85213..90550d26e2d 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swift @@ -37,16 +37,38 @@ public struct PlatformAddressSyncEvent: Sendable { } } -final class PlatformWalletEventHandler { +/// `@unchecked Sendable`, matching `PlatformWalletPersistenceHandler`: this +/// object *is* a cross-thread callback context by construction — Rust owns +/// a retained reference to it and invokes the callbacks below from its own +/// background threads. `manager` is written once at `init` and only read +/// afterwards (weak loads are atomic), and every touch of the main-actor +/// manager already hops through `Task { @MainActor }`. +final class PlatformWalletEventHandler: @unchecked Sendable { weak var manager: PlatformWalletManager? init(manager: PlatformWalletManager) { self.manager = manager } + /// Build `EventHandlerCallbacks` that point to this handler. + /// + /// **Transfers ownership of a strong reference to Rust**: the context + /// is `passRetained`, and `release_fn` balances that retain exactly + /// once — when the Rust manager and every worker that can still + /// dispatch an event have dropped their references (possibly on a + /// Rust thread, possibly after `destroy` returns if a worker + /// straggles). ARC therefore cannot free this handler while any Rust + /// worker can still call back into it. + /// + /// If manager creation fails, Rust never took the reference — the + /// caller must balance the retain itself (see `configure`). func makeCallbacks() -> EventHandlerCallbacks { var callbacks = EventHandlerCallbacks() - callbacks.context = Unmanaged.passUnretained(self).toOpaque() + callbacks.context = Unmanaged.passRetained(self).toOpaque() + callbacks.release_fn = { context in + guard let context else { return } + Unmanaged.fromOpaque(context).release() + } callbacks.on_platform_address_sync_completed_fn = platformAddressSyncCompletedCallback callbacks.on_shielded_sync_completed_fn = shieldedSyncCompletedCallback callbacks.on_shielded_sync_progress_fn = shieldedSyncProgressCallback diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift index 5eb568cfbaa..f3edc95db0a 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift @@ -1204,11 +1204,24 @@ public final class PlatformWalletPersistenceHandler: @unchecked Sendable { /// Build `PersistenceCallbacks` that point to this handler. /// - /// The returned struct must not outlive `self`. + /// **Transfers ownership of a strong reference to Rust**: the context + /// is `passRetained`, and `release_fn` balances that retain exactly + /// once — when the Rust manager and every background worker holding + /// its persister have dropped their references (possibly on a Rust + /// thread, possibly after `destroy` returns if a worker straggles). + /// ARC therefore cannot free this handler while any Rust worker can + /// still call back into it, no matter how teardown went. + /// + /// If manager creation fails, Rust never took the reference — the + /// caller must balance the retain itself (see `configure`). func makeCallbacks() -> PersistenceCallbacks { - let contextPtr = Unmanaged.passUnretained(self).toOpaque() + let contextPtr = Unmanaged.passRetained(self).toOpaque() var cb = PersistenceCallbacks() cb.context = contextPtr + cb.release_fn = { context in + guard let context else { return } + Unmanaged.fromOpaque(context).release() + } cb.on_changeset_begin_fn = changesetBeginCallback cb.on_changeset_end_fn = changesetEndCallback cb.on_persist_address_balances_fn = persistAddressBalancesCallback diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 3834ca71b41..d22383d5f17 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -69,6 +69,13 @@ public enum PlatformWalletResultCode: Int32, Sendable { /// Core definitively rejected the transaction. Its reserved inputs were /// released and a corrected transaction may be submitted again. case errorTransactionBroadcastRejected = 26 + /// A quiesce/drain barrier did not complete within its budget: an + /// in-flight sync pass was still running when a Clear / reset / + /// sync-stop needed it provably drained. The operation failed closed — + /// no state was wiped — and the caller should retry once sync is idle. + /// (Not returned by `destroy`: Rust owns the callback contexts, so a + /// straggling worker is memory-safe and merely logged there.) + case errorShutdownIncomplete = 27 case notFound = 98 case errorUnknown = 99 @@ -128,6 +135,8 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorAssetLockFundingMismatch case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_BROADCAST_REJECTED: self = .errorTransactionBroadcastRejected + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE: + self = .errorShutdownIncomplete case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: self = .notFound case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UNKNOWN: @@ -250,6 +259,10 @@ public enum PlatformWalletError: LocalizedError { /// to retry, and the retry re-fetches the address nonce so the mismatch /// self-heals. The submitted/expected nonce values are in the message. case addressNonceMismatch(String) + /// A quiesce/drain barrier (Clear / reset / sync-stop) timed out with a + /// sync pass still in flight. The operation failed closed — retry once + /// sync is idle. + case shutdownIncomplete(String) case notFound(String) case unknown(String) @@ -272,6 +285,7 @@ public enum PlatformWalletError: LocalizedError { .transactionBroadcastUnconfirmed(let m), .transactionBroadcastRejected(let m), .addressNonceMismatch(let m), + .shutdownIncomplete(let m), .notFound(let m), .unknown(let m): return m } @@ -313,6 +327,8 @@ public enum PlatformWalletError: LocalizedError { self = .transactionBroadcastRejected(detail) case .errorAddressNonceMismatch: self = .addressNonceMismatch(detail) + case .errorShutdownIncomplete: + self = .shutdownIncomplete(detail) case .notFound: self = .notFound(detail) case .errorUnknown: self = .unknown(detail) }