feat(platform-wallet)!: shared ThreadRegistry for coordinator lifecycle + shutdown UAF/data-loss fixes - #3954
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces a shared ChangesCoordinator Shutdown Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant ThreadRegistry
participant WalletCoordinators
participant FFI
PlatformWalletManager->>WalletCoordinators: quiesce periodic workers
PlatformWalletManager->>ThreadRegistry: shutdown()
ThreadRegistry->>WalletCoordinators: cancel and join workers
ThreadRegistry-->>PlatformWalletManager: ShutdownReport
FFI->>PlatformWalletManager: destroy()
FFI->>PlatformWalletManager: retry if report is not clean
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #3954 +/- ##
============================================
+ Coverage 87.54% 87.56% +0.02%
============================================
Files 2670 2671 +1
Lines 338763 340127 +1364
============================================
+ Hits 296583 297846 +1263
- Misses 42180 42281 +101
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/rs-platform-wallet/src/manager/identity_sync.rs (1)
418-459: 🩺 Stability & Availability | 🟠 MajorDon't overwrite an unjoined coordinator generation.
stop()removes onlybackground_canceland leaves the previousbackground_joinunjoined. Astop()→start()sequence passes thecancel_guard.is_some()check and overwritesbackground_joinwith a new handle, losing the old OS-thread handle before shutdown can join it. Gate restart on confirming the prior handle has been joined, or ensure every generation's handle is tracked and joined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/identity_sync.rs` around lines 418 - 459, The start() method can overwrite an existing background_join handle before it has been properly joined, causing resource leaks. Before spawning the new identity-sync thread and storing its handle in background_join, add a check to ensure any existing join handle in background_join has been properly cleaned up or joined first. This can be done either by joining the existing handle before proceeding, or by verifying that background_join is None before allowing the new thread spawn to proceed. This ensures the prior thread's OS handle is not lost and can be properly shutdown.packages/rs-platform-wallet/src/manager/platform_address_sync.rs (1)
218-255: 🩺 Stability & Availability | 🟠 MajorAdd generation guard to prevent thread handle loss after stop/start cycles.
After
stop()cancels the token,background_cancelbecomesNonewhile the old thread keeps running. A subsequentstart()seescancel_guard.is_some() == falseand spawns a new thread, unconditionally overwritingbackground_join. The old thread's join handle is lost, making it impossible to join its cleanup later. Additionally, without a generation counter, the exiting old thread clears the new generation'sbackground_canceltoken as it shuts down, creating a race where the new loop runs but appears stopped tois_running().Both
IdentitySyncManagerandShieldedSyncManageralready implement this pattern: they incrementbackground_generationon eachstart(), passmy_gento the spawned thread, and checkbackground_generation.load() == my_genbefore clearingbackground_cancelon exit. Apply the same approach here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/platform_address_sync.rs` around lines 218 - 255, The thread handle for the platform address sync loop can be lost during stop/start cycles because a new thread spawns and unconditionally overwrites the background_join handle before the old thread finishes cleanup. Additionally, the old thread clears the new generation's background_cancel token, creating a race condition. Implement the generation guard pattern already used in IdentitySyncManager and ShieldedSyncManager: add a background_generation atomic counter to the struct, increment it at the start of the start() method, capture the current generation as my_gen before spawning the thread, pass my_gen into the spawned thread closure, and modify the cleanup code (where background_cancel is set to None) to only clear the token if background_generation.load() equals my_gen, ensuring old exiting threads do not interfere with new generations.packages/rs-platform-wallet/src/manager/shielded_sync.rs (1)
245-300: 🩺 Stability & Availability | 🔴 CriticalDon't replace a pending shielded-sync join handle on rapid stop/start cycles.
The
start()method checks onlybackground_cancelto guard against concurrent starts. Whenstop()removes the token frombackground_cancel, a subsequentstart()proceeds to spawn a new thread and overwritesbackground_join, even though the previous generation's thread is still winding down. The generation check (line 290) only prevents the old thread from clearingbackground_cancel—it does not protectbackground_join. This leaves prior-generation handles permanently lost.To fix: either require
quiesce()beforestart()to join all pending generations, or track join handles per-generation and join all pending before spawning a new thread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/shielded_sync.rs` around lines 245 - 300, The `start()` method only checks `background_cancel` to guard against concurrent starts, but when `stop()` clears this token, a subsequent `start()` can spawn a new thread and overwrite the `background_join` handle before the previous generation's thread has finished cleaning up. The generation check at the cleanup section prevents the old thread from clearing `background_cancel`, but does not protect `background_join` from being overwritten. Fix this by either adding logic to join all pending prior-generation join handles before storing the new one (by tracking handles per-generation), or by ensuring that before assigning a new join handle to `background_join` in the `start()` method, any existing pending handle from a prior generation is properly joined and waited for completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 706-710: The while loop waiting for handler_started to become true
has no timeout, which will cause the test to hang indefinitely if the slow
callback never executes. Wrap the entire while loop (that checks
handler_started.load(AO::Acquire)) with tokio::time::timeout() and provide an
appropriate timeout duration, then handle the timeout error case with an
assertion that explicitly fails the test with a useful message. This ensures CI
fails fast with clear feedback instead of timing out.
- Around line 483-489: In the wallet event adapter task join error handling, the
else branch that handles non-panic JoinErrors currently returns
CoordinatorThreadStatus::Ok, which should instead return
CoordinatorThreadStatus::Error with an appropriate error message. Change the
else clause that follows the is_panic() check to map the error to a
CoordinatorThreadStatus::Error variant containing details about the join error,
rather than treating it as a successful completion.
- Around line 175-181: The current implementation uses
tokio::task::spawn_blocking to wrap handle.join(), but this pattern prevents the
timeout from effectively interrupting the blocking task if the coordinator
thread hangs. Replace the spawn_blocking closure approach with explicit polling:
repeatedly check if the JoinHandle is finished using is_finished() in a loop
until the deadline is reached, and only call join() once the handle confirms it
is finished. This ensures the timeout boundary is enforced even if the
coordinator thread misbehaves or fails to clear is_syncing before join() is
called.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/identity_sync.rs`:
- Around line 418-459: The start() method can overwrite an existing
background_join handle before it has been properly joined, causing resource
leaks. Before spawning the new identity-sync thread and storing its handle in
background_join, add a check to ensure any existing join handle in
background_join has been properly cleaned up or joined first. This can be done
either by joining the existing handle before proceeding, or by verifying that
background_join is None before allowing the new thread spawn to proceed. This
ensures the prior thread's OS handle is not lost and can be properly shutdown.
In `@packages/rs-platform-wallet/src/manager/platform_address_sync.rs`:
- Around line 218-255: The thread handle for the platform address sync loop can
be lost during stop/start cycles because a new thread spawns and unconditionally
overwrites the background_join handle before the old thread finishes cleanup.
Additionally, the old thread clears the new generation's background_cancel
token, creating a race condition. Implement the generation guard pattern already
used in IdentitySyncManager and ShieldedSyncManager: add a background_generation
atomic counter to the struct, increment it at the start of the start() method,
capture the current generation as my_gen before spawning the thread, pass my_gen
into the spawned thread closure, and modify the cleanup code (where
background_cancel is set to None) to only clear the token if
background_generation.load() equals my_gen, ensuring old exiting threads do not
interfere with new generations.
In `@packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- Around line 245-300: The `start()` method only checks `background_cancel` to
guard against concurrent starts, but when `stop()` clears this token, a
subsequent `start()` can spawn a new thread and overwrite the `background_join`
handle before the previous generation's thread has finished cleaning up. The
generation check at the cleanup section prevents the old thread from clearing
`background_cancel`, but does not protect `background_join` from being
overwritten. Fix this by either adding logic to join all pending
prior-generation join handles before storing the new one (by tracking handles
per-generation), or by ensuring that before assigning a new join handle to
`background_join` in the `start()` method, any existing pending handle from a
prior generation is properly joined and waited for completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01cab324-105d-4c5d-afe0-0ceb6faff13e
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/platform_address_sync.rspackages/rs-platform-wallet/src/manager/shielded_sync.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-platform-wallet/src/manager/mod.rs (2)
405-408: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle unclean shielded quiesce before clearing state.
quiesce()now returns a meaningful shutdown status. Ignoring it letsclear_shielded()proceed tocoord.clear()afterTimeout,Stopped, orPanicked, which can race a still-running shielded pass that the quiesce barrier was meant to stop.Proposed fix
- self.shielded_sync_manager.quiesce().await; + let status = self.shielded_sync_manager.quiesce().await; + if !status.is_clean() { + return Err(crate::error::PlatformWalletError::ShieldedStoreError( + format!("shielded sync did not stop cleanly before clear: {status:?}"), + )); + } if let Some(coord) = self.shielded_coordinator().await { coord.clear().await?; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/mod.rs` around lines 405 - 408, The clear_shielded function ignores the return value from self.shielded_sync_manager.quiesce().await, which now provides meaningful shutdown status information. Instead of ignoring this result, capture the return value and check if the quiesce completed cleanly. If quiesce returns a status indicating Timeout, Stopped, or Panicked, return an appropriate error from clear_shielded rather than continuing to call coord.clear(), which could race with a still-running shielded pass. Only proceed with coord.clear() when quiesce has successfully shut down cleanly.
463-477: 🩺 Stability & Availability | 🟠 MajorWrap
quiescinginAtomicFlagGuardto ensure cancellation-safe reset in all coordinatorquiesce()implementations.The current implementations set
quiescing = truebefore the awaited drain loop and reset it only after. Iftokio::time::timeoutdrops the future during the loop, the reset never executes, permanently wedgingquiescingand blocking all future syncs.All three coordinators (
platform_address_sync.rs:291,identity_sync.rs:494,shielded_sync.rs:334) have identical patterns. Use the sameAtomicFlagGuardapproach already correctly applied tois_syncinginsync_now():pub async fn quiesce(&self) -> super::CoordinatorThreadStatus { self.quiescing.store(true, Ordering::Release); let _quiescing_guard = AtomicFlagGuard::new(&self.quiescing); self.stop(); while self.is_syncing.load(Ordering::Acquire) { tokio::time::sleep(Duration::from_millis(20)).await; } // quiescing.store(false) removed — guard handles reset on all exit paths // ...rest of implementation }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/manager/mod.rs` around lines 463 - 477, The `quiesce()` method implementations in all three coordinators (the ones containing the timeout calls for `platform_address_sync_manager`, `identity_sync_manager`, and `shielded_sync_manager`) don't properly handle cancellation when `tokio::time::timeout` drops the future. Wrap the `quiescing` flag reset in an `AtomicFlagGuard` to ensure it's reset on all exit paths including early cancellation. In each coordinator's `quiesce()` method, after setting `quiescing` to true, immediately create an `AtomicFlagGuard` using `AtomicFlagGuard::new(&self.quiescing)`, and remove any manual `quiescing.store(false)` reset call at the end since the guard will handle it automatically. Use the same pattern already correctly implemented in `sync_now()` for the `is_syncing` flag.
🧹 Nitpick comments (1)
packages/rs-dash-async/src/atomic.rs (1)
8-15: 📐 Maintainability & Code Quality | 🔵 TrivialAdd
#[must_use]annotation toAtomicFlagGuard.The guard is dropped as a temporary if not bound, silently resetting the flag immediately. This breaks the intended guarded scope behavior. Mark the type with
#[must_use]to catch accidental non-binding at compile time.Proposed fix
-pub struct AtomicFlagGuard<'a>(&'a AtomicBool); +#[must_use = "AtomicFlagGuard clears the flag on drop; bind it to keep the flag set for the guarded scope"] +pub struct AtomicFlagGuard<'a>(&'a AtomicBool);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-dash-async/src/atomic.rs` around lines 8 - 15, The AtomicFlagGuard struct can be accidentally dropped without being bound to a variable, causing the flag to be reset immediately and breaking the guarded scope behavior. Add the #[must_use] attribute to the AtomicFlagGuard struct definition to make the compiler warn when the guard is not explicitly bound to a variable, ensuring the developer catches this mistake at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 671-695: The test
`event_adapter_non_panic_join_error_maps_to_stopped_and_is_not_clean` is
non-deterministic because it accepts both `CoordinatorThreadStatus::Stopped` and
`CoordinatorThreadStatus::Ok` as valid outcomes. This allows the test to pass
without actually exercising the non-panic JoinError branch. To fix this, replace
the current task abort approach with a task that is guaranteed to be pending and
never complete on its own, such as a task that awaits on a channel or a
never-resolving future. This ensures the abort always triggers the Stopped path
deterministically, and update the assertion to only expect
`CoordinatorThreadStatus::Stopped`.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 405-408: The clear_shielded function ignores the return value from
self.shielded_sync_manager.quiesce().await, which now provides meaningful
shutdown status information. Instead of ignoring this result, capture the return
value and check if the quiesce completed cleanly. If quiesce returns a status
indicating Timeout, Stopped, or Panicked, return an appropriate error from
clear_shielded rather than continuing to call coord.clear(), which could race
with a still-running shielded pass. Only proceed with coord.clear() when quiesce
has successfully shut down cleanly.
- Around line 463-477: The `quiesce()` method implementations in all three
coordinators (the ones containing the timeout calls for
`platform_address_sync_manager`, `identity_sync_manager`, and
`shielded_sync_manager`) don't properly handle cancellation when
`tokio::time::timeout` drops the future. Wrap the `quiescing` flag reset in an
`AtomicFlagGuard` to ensure it's reset on all exit paths including early
cancellation. In each coordinator's `quiesce()` method, after setting
`quiescing` to true, immediately create an `AtomicFlagGuard` using
`AtomicFlagGuard::new(&self.quiescing)`, and remove any manual
`quiescing.store(false)` reset call at the end since the guard will handle it
automatically. Use the same pattern already correctly implemented in
`sync_now()` for the `is_syncing` flag.
---
Nitpick comments:
In `@packages/rs-dash-async/src/atomic.rs`:
- Around line 8-15: The AtomicFlagGuard struct can be accidentally dropped
without being bound to a variable, causing the flag to be reset immediately and
breaking the guarded scope behavior. Add the #[must_use] attribute to the
AtomicFlagGuard struct definition to make the compiler warn when the guard is
not explicitly bound to a variable, ensuring the developer catches this mistake
at compile time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 192ea517-ff8a-4b43-9773-c096391c8a49
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/rs-dash-async/src/atomic.rspackages/rs-dash-async/src/lib.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/platform_address_sync.rspackages/rs-platform-wallet/src/manager/shielded_sync.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-platform-wallet/src/manager/platform_address_sync.rs
- packages/rs-platform-wallet/src/manager/shielded_sync.rs
|
🔍 Review in progress — actively reviewing now (commit 31e22d5) |
|
🟠 MEDIUM — Sibling Stop/Clear paths call Posted at PR level because the affected call sites are not part of this PR's diff.
Fix: apply the same timeout bound (or a Stop/Clear-specific one) around these 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Solid lifecycle hardening overall — joining coordinator threads and the RAII is_syncing guard close real races. Three in-scope blockers undermine the new shutdown contract: the FFI destroy still returns success on non-clean shutdown (Swift may free a context a coordinator thread still owns), the bounded timeout doesn't actually bound anything because spawn_blocking tasks are non-abortable, and start-after-stop overwrites the saved JoinHandle so a later shutdown cannot join the stranded thread. Two suggestions and two test-quality nits round out the review.
🔴 3 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/manager.rs:366-376: Destroy returns ok() on non-clean shutdown — re-opens callback-after-free window
`shutdown()` can now legitimately return `CoordinatorThreadStatus::Timeout` (and `Stopped`/`Panicked`/`Error`) meaning a coordinator's OS thread or the event-adapter task did not actually join. This FFI entry point logs that outcome and still returns `PlatformWalletFFIResult::ok()`. The Swift host (`PlatformWalletManager.swift` deinit) is documented to free the callback `context` once `platform_wallet_manager_destroy` returns; meanwhile the still-alive coordinator thread holds an `Arc<FFIEventHandler>` / `Arc<FFIPersister>` and can fire `on_*_sync_completed` or `persister.store(...)` through the now-dangling `context` pointer. That is precisely the use-after-free this PR set out to close — the previous unbounded wait would have hung instead of returning false success. Surface non-clean shutdowns as a distinct, non-ok result code so the host knows not to free its context (or keep the FFI-owned handler `Arc` alive on the non-clean path, e.g. `mem::forget`, so any lingering callback remains memory-safe).
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:178-192: Timeout doesn't actually bound shutdown — spawn_blocking is non-abortable
`shutdown()` wraps each `quiesce()` in `tokio::time::timeout`, but `join_coordinator_thread` moves the `std::thread::JoinHandle` into `spawn_blocking(move || handle.join())`. Tokio blocking tasks cannot be aborted once started: dropping the outer timeout future stops `await`ing the `JoinHandle` but the underlying blocking task is still parked inside `handle.join()`, keeping the coordinator thread's `Arc<…SyncManager>` (and the host callback contexts it transitively holds) alive. When the caller then drops the multi-thread runtime, `Runtime::drop` returns without waiting for blocking tasks, leaving an OS thread plus a stranded blocking thread alive on a freed runtime — which is the very `"Tokio 1.x context … being shutdown"` race the PR cites in its motivation. Make the join cancellation-aware by polling `handle.is_finished()` (with a short async sleep) before the final `handle.join()`, so dropping the timeout future actually releases all state.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:441-459: assert! on runtime flavor is incorrect — spawn_blocking works on current_thread
The promotion from `debug_assert!` to `assert!` is justified in the docstring by the claim that `spawn_blocking` 'is not available on `current_thread` runtimes and will panic there.' That's not how Tokio works: `spawn_blocking` dispatches to the runtime's shared blocking pool, which both `multi_thread` and `current_thread` runtimes provision; awaiting the returned `JoinHandle` simply yields the runtime task. Today's only in-tree caller (`platform-wallet-ffi/src/runtime.rs:34`) builds a multi-thread runtime, but `shutdown()` is now a public Rust API. Any downstream consumer using `#[tokio::main(flavor = "current_thread")]` (or `Builder::new_current_thread()`) will hit a release-mode panic on a configuration that would have worked. Revert to `debug_assert!` (or drop it entirely) and correct the docstring rationale.
In `packages/rs-platform-wallet/src/manager/identity_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/identity_sync.rs:405-448: start-after-stop overwrites background_join, dropping the previous thread handle
`stop()` takes `background_cancel` (leaving it `None`) but never touches `background_join`. A subsequent `start()` passes the `cancel_guard.is_some()` early-return check, spawns a fresh thread, and at line 447 unconditionally overwrites `background_join` with the new handle — the previous handle is dropped (detached). The old coordinator thread is cancellation-bound but not yet exited; it can still be inside its `block_on` polling `tokio::time` and feeding the event manager when the new handle replaces it. A later `shutdown()` then can only join the new thread, so the original thread can outlive `shutdown()` and touch the host's freed callback context — recreating exactly the runtime-drop race this PR is meant to eliminate. The same pattern applies to `platform_address_sync` and `shielded_sync`. Fix by taking-and-joining (or refusing) any non-`None` handle inside the `start()` lock before installing the new one.
In `packages/rs-platform-wallet/src/manager/platform_address_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/platform_address_sync.rs:291-304: quiescing gate is reopened before the join completes — sync_now can slip a pass through
`quiesce()` clears `self.quiescing` to `false` *before* taking `background_join` and awaiting `join_coordinator_thread`. The join can block for up to `SHUTDOWN_JOIN_TIMEOUT_SECS` (30s). During that window the loop is cancelled and won't start new passes, but an external caller invoking `sync_now` (e.g. an FFI on-demand sync) finds a fully open gate — `quiescing=false`, `is_syncing=false` — and runs a complete pass, including the `on_platform_address_sync_completed` host callback. That breaks the documented `quiesce()` contract that no further callback can fire after it returns, undermining the manager-level shutdown guarantee. Same pattern in `identity_sync::quiesce` and `shielded_sync::quiesce`. Move `quiescing.store(false, …)` to *after* the join completes (or have `sync_now`/`sync_wallet` also consult `background_join.is_some()`).
|
Update — SEC-002 fixed in 🤖 Co-authored by Claudius the Magnificent AI Agent |
…d start_thread Collapse each sync coordinator's hand-rolled (spawn thread -> install LoopCancelGuard -> register_thread) dance into one atomic `ThreadRegistry::start_thread` call. The registry now owns the whole loop lifecycle under a single slot lock — it takes the closing/clearing teardown latches, installs the cancellation token, spawns the OS thread, and reaps any still-draining prior generation — closing the check-then-spawn gap the manual check-lock-check only papered over. - identity_sync, platform_address_sync, dashpay_sync, shielded_sync: start() -> registry.start_thread(); stop() -> registry.cancel(); is_running() -> registry.is_running(). LoopCancelGuard field removed. - Delete manager/loop_cancel.rs entirely (no remaining references). - The quiescing/is_syncing full-pass-drain barrier is untouched: each coordinator keeps its own quiesce(), and the manager still quiesces all four before registry.shutdown() joins them. - The stop()+quick-start() generation guard is now carried by the registry's SlotState.generation + EpilogueGuard (a Drop guard, so it also clears the running flag when a loop panics — strictly better than LoopCancelGuard's fall-through clear_if_current). Add WorkerConfig::stack_size so start_thread can honour DashPay's 8 MiB stack (its GroveDB proof descent overflows the default and SIGBUSes on device); coordinator_worker_config() defaults it to None. Rewrite dashpay's stale-loop regression test to drive real start()/stop() on live OS-thread loops through the registry, and pin the stack_size path + default in dash-async's suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers = general: claude opus (ok), codex gpt-5.6-sol (failed); security-auditor: claude opus (ok), codex gpt-5.6-sol (failed); rust-quality: claude opus (ok), codex gpt-5.6-sol (failed); ffi-engineer: claude opus (ok), codex gpt-5.6-sol (failed); verifier = claude opus (ok); policy gate = review_policy.enforce_backport_prereq_policy.
Carried-Forward Prior Findings
One blocking prior finding remains: PlatformWalletManager.deinit discards ErrorShutdownIncomplete and releases the passUnretained callback-context owners even though a wedged Rust coordinator may still call them.
New Findings In Latest Delta
None. The coordinator migration to ThreadRegistry::start_thread fixes the prior spawn-before-registration shutdown race; the LoopCancelGuard removal, WorkerConfig.stack_size, and rewritten tests are sound.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:223-231: Swift deinit discards ErrorShutdownIncomplete, then ARC frees the passUnretained callback contexts Rust may still call (UAF)
This PR's entire purpose is to stop a background coordinator from firing host callbacks after teardown. Rust now enforces that contract: `platform_wallet_manager_destroy` runs `manager.shutdown()`, retries once, and returns `PlatformWalletFFIResultCode::ErrorShutdownIncomplete` when a coordinator thread still cannot be cleanly joined (packages/rs-platform-wallet-ffi/src/manager.rs:379-393) — its own log states 'a worker may outlive destroy.' The Swift result layer maps code 22 to `PlatformWalletError.shutdownIncomplete` (PlatformWalletResult.swift:70,121,296).
But this `deinit` calls `platform_wallet_manager_destroy(handle).discard()`, dropping that non-success result on the floor, and then returns — at which point ARC releases `persistenceHandler` and `eventHandler`. Those two properties are the only strong owners of the callback contexts handed to Rust via `Unmanaged.passUnretained` (PlatformWalletPersistenceHandler.swift:1032 and PlatformWalletManagerAddressSync.swift:49). If destroy returned `ErrorShutdownIncomplete`, a still-live worker holds now-dangling `context` pointers and its next `persister.store(...)` or event-callback invocation is a use-after-free — crash or persistence corruption on the primary wrapper-lifetime path. The deinit must fail closed on a non-clean destroy: keep the callback owners alive (intentionally leak them) rather than letting ARC free contexts a wedged worker can still call through.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-dash-async/src/lib.rs`:
- Around line 5-8: The crate-level documentation unconditionally links to
cfg-gated ThreadRegistry, which breaks wasm32 rustdoc builds. In the module
documentation near the ThreadRegistry reference, replace the intra-doc link with
plain code formatting or conditionally include the link using the same wasm32
cfg as ThreadRegistry.
In `@packages/rs-platform-wallet-ffi/src/manager.rs`:
- Around line 365-394: The shutdown handling around manager.shutdown() can
overwrite a first-pass terminal failure with a clean retry result. Preserve any
non-clean terminal status such as Panicked, Stopped, or Error when evaluating
the initial report, or restrict retries to transient Timeout/Detached statuses;
ensure the final result remains an error for those terminal failures. Add a
regression test covering an initial Panicked report followed by retry
NotRunning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60087b6d-89ad-49a0-b4cd-55c964185453
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/rs-dash-async/Cargo.tomlpackages/rs-dash-async/src/lib.rspackages/rs-dash-async/src/registry.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/platform_address_sync.rspackages/rs-platform-wallet/src/manager/shielded_sync.rs
💤 Files with no reviewable changes (6)
- packages/rs-platform-wallet/Cargo.toml
- packages/rs-platform-wallet/src/manager/identity_sync.rs
- packages/rs-platform-wallet/src/manager/platform_address_sync.rs
- packages/rs-platform-wallet/src/manager/shielded_sync.rs
- packages/rs-platform-wallet/src/manager/dashpay_sync.rs
- packages/rs-platform-wallet/src/manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dash-async/Cargo.toml
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior finding: prior-swift-deinit-shutdown-incomplete remains blocking because Swift still discards the incomplete-shutdown result before releasing callback owners. New findings in the latest merge delta: none. Cumulative current-head review confirms two additional blockers: coordinator drain can wait forever before the registry timeout, and the shutdown report can return clean while SPV-driven persistence work remains live.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:229-236: Swift deinit releases callback contexts after an incomplete Rust shutdown
`platform_wallet_manager_destroy` removes the manager handle and returns `ErrorShutdownIncomplete` when coordinator workers may remain live after its retry. This deinitializer discards that result, after which ARC releases `persistenceHandler` and `eventHandler`, the only strong owners of the objects passed to Rust through `Unmanaged.passUnretained`. A surviving coordinator can therefore invoke a persistence or event callback through a dangling Swift pointer, causing a crash or wallet-state corruption. Because the Rust handle has already been removed, Swift cannot retry; it must retain these callback owners when destroy is non-clean.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:455-466: Unbounded coordinator drains prevent the shutdown timeout from running
`shutdown` awaits all four coordinator `quiesce()` methods before invoking `ThreadRegistry::shutdown()`. Each coordinator polls `is_syncing` without a deadline, while the registry's 30-second join budget applies only afterward. A stalled network, persistence, or host-callback await therefore blocks destroy indefinitely instead of returning `ErrorShutdownIncomplete`. Under unwind builds, a panic during `sync_now` also leaves `is_syncing` set because it is cleared only on normal fall-through. Bound the drain phase as part of the shutdown budget and use panic-safe cleanup for each coordinator's `is_syncing` flag.
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:466-478: Shutdown can report clean while SPV persistence callbacks remain live
The new shutdown report covers the four periodic coordinators and the wallet-event adapter, but it never stops or joins `SpvRuntime`. Its spawned run loop retains `Arc<SpvRuntime>` and therefore the event manager after the manager handle is removed. SPV events can reach `DashPayPaymentHandler`, which creates untracked Tokio tasks that clone the FFI persister; those tasks may invoke Swift persistence callbacks after `destroy` returned success and ARC released `persistenceHandler`. Stop and fully join SPV, prevent further event fan-out, and track or drain payment-hook tasks before allowing `all_clean()` to authorize freeing callback state.
| pub async fn shutdown(&self) -> ShutdownReport<WalletWorker> { | ||
| self.platform_address_sync_manager.quiesce().await; | ||
| self.identity_sync_manager.quiesce().await; | ||
| self.dashpay_sync_manager.quiesce().await; | ||
| #[cfg(feature = "shielded")] | ||
| self.shielded_sync_manager.quiesce().await; | ||
|
|
||
| // 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 report = self.registry.shutdown().await; |
There was a problem hiding this comment.
🔴 Blocking: Unbounded coordinator drains prevent the shutdown timeout from running
shutdown awaits all four coordinator quiesce() methods before invoking ThreadRegistry::shutdown(). Each coordinator polls is_syncing without a deadline, while the registry's 30-second join budget applies only afterward. A stalled network, persistence, or host-callback await therefore blocks destroy indefinitely instead of returning ErrorShutdownIncomplete. Under unwind builds, a panic during sync_now also leaves is_syncing set because it is cleared only on normal fall-through. Bound the drain phase as part of the shutdown budget and use panic-safe cleanup for each coordinator's is_syncing flag.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Unbounded coordinator drains prevent the shutdown timeout from running no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let report = self.registry.shutdown().await; | ||
|
|
||
| // 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, so it is joined here rather than in the report. | ||
| 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"); | ||
| } | ||
| } | ||
|
|
||
| report |
There was a problem hiding this comment.
🔴 Blocking: Shutdown can report clean while SPV persistence callbacks remain live
The new shutdown report covers the four periodic coordinators and the wallet-event adapter, but it never stops or joins SpvRuntime. Its spawned run loop retains Arc<SpvRuntime> and therefore the event manager after the manager handle is removed. SPV events can reach DashPayPaymentHandler, which creates untracked Tokio tasks that clone the FFI persister; those tasks may invoke Swift persistence callbacks after destroy returned success and ARC released persistenceHandler. Stop and fully join SPV, prevent further event fan-out, and track or drain payment-hook tasks before allowing all_clean() to authorize freeing callback state.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Shutdown can report clean while SPV persistence callbacks remain live no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…stroy retry Three findings from a PR #3954 review-comment audit, all verified against current code before fixing: - `ThreadRegistry::quiesce`'s classify path takes a finished worker's handle without re-parking it, so a first-pass Panicked/Stopped/Error status silently became NotRunning (clean) on `platform_wallet_manager_destroy`'s retry pass, swallowing the panic. `ShutdownReport::merged_with_retry` now carries any non-transient first-pass failure (worker or orphan) through to the final verdict, while still letting Timeout/Detached resolve cleanly on retry as before. Regression tests cover both the preserved-failure and the still-resolves-cleanly cases. - `rs-dash-async`'s crate doc had an unconditional intra-doc link to `ThreadRegistry`, a cfg(not(wasm32)) item -- harmless today (nothing denies the lint) but a latent break on a wasm32 doc build. Changed to plain code formatting. - The Swift SDK's only `platform_wallet_manager_destroy` call site (`PlatformWalletManager.deinit`) discarded the result unconditionally, contradicting this PR's own doc comment for the new `errorShutdownIncomplete` code ("treat this as a real teardown fault, not a silent success"). `deinit` now logs any non-success result via `os.log`, matching the existing `KeychainManager` logging convention. Verified via the project's verification wrapper for -p dash-async -p platform-wallet -p platform-wallet-ffi: format check clean; full nextest run green (736 tests incl. the 3 new ones). The lint pass for the same scope fails only on pre-existing warnings in untouched files (core_wallet_types.rs, persistence.rs, withdrawal.rs) -- confirmed unrelated to this change. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: all three lifecycle blockers remain valid at exact head 32e665d. Swift still releases unretained callback owners after incomplete destruction, coordinator drains remain unbounded before the registry timeout, and the clean-shutdown verdict excludes SPV and spawned payment-hook work. New findings in the latest delta: none; the retry-report merge correctly preserves terminal first-pass failures, and its three focused tests pass.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:241-246: Swift releases callback contexts after incomplete shutdown
`platform_wallet_manager_destroy` removes the Rust handle before shutdown and returns `ErrorShutdownIncomplete` when callback-capable coordinator work may remain alive. This deinitializer only logs that result and then returns, allowing ARC to release `persistenceHandler` and `eventHandler`. Those objects are the strong owners behind callback contexts created with `Unmanaged.passUnretained`, so a surviving Rust worker can subsequently dereference a dangling Swift pointer and cause a crash or wallet-state corruption. Because the handle has already been removed, Swift cannot retry destruction; it must retain the callback owners when shutdown is incomplete.
| 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 ?? "<no detail from Rust>", privacy: .public)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Swift releases callback contexts after incomplete shutdown
platform_wallet_manager_destroy removes the Rust handle before shutdown and returns ErrorShutdownIncomplete when callback-capable coordinator work may remain alive. This deinitializer only logs that result and then returns, allowing ARC to release persistenceHandler and eventHandler. Those objects are the strong owners behind callback contexts created with Unmanaged.passUnretained, so a surviving Rust worker can subsequently dereference a dangling Swift pointer and cause a crash or wallet-state corruption. Because the handle has already been removed, Swift cannot retry destruction; it must retain the callback owners when shutdown is incomplete.
| 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 ?? "<no detail from Rust>", privacy: .public)" | |
| ) | |
| } | |
| 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 ?? "<no detail from Rust>", privacy: .public)" | |
| ) | |
| if destroyResult.code == .errorShutdownIncomplete { | |
| if let handler = persistenceHandler { | |
| _ = Unmanaged.passRetained(handler) | |
| } | |
| if let handler = eventHandler { | |
| _ = Unmanaged.passRetained(handler) | |
| } | |
| } | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in eb3941c — Swift releases callback contexts after incomplete shutdown no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…own-channel log (#905) * fix(shutdown): stop wallet backend on GUI close and clear stale shutdown-channel log Await every initialized wallet backend within the bounded GUI shutdown flow, including contexts completed by an in-flight network switch, and consume terminal shutdown receivers exactly once. The platform-wallet coordinator thread join race remains tracked upstream in dashpay/platform#3954 and is not fixed here. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> * fix(shutdown): make wallet teardown race-safe Close task registration before draining managed work, include the final MCP context, clear remembered secrets, and bound both graceful and fallback wallet backend shutdown paths. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> * fix(shutdown): wait for backend-task blocking work before wallet teardown Backend tasks run inside tokio::spawn_blocking closures wrapped by an abortable async watcher. On shutdown timeout, aborting the watcher only cancelled the cancellable wrapper — the underlying spawn_blocking closure cannot be forcibly stopped and kept running, unobserved, while wallet backend secrets were cleared and storage torn down underneath it. TaskManager now tracks blocking-task completion via a Shared future kept independent of the abortable watcher, so shutdown can genuinely await real completion (bounded by a new blocking-task timeout) instead of an abortable wrapper. Both the async shutdown path and the on_exit force-close fallback route through the same two-phase wait before shutdown_wallet_backends runs. A still-running blocking task past the bound now reports a distinct degraded ShutdownOutcome::BackendTasksTimedOut instead of silently proceeding as if nothing were wrong. shutdown_hard_deadline() extended to cover both phases. Addresses PR #905 review thread PRRT_kwDOM8GK3c6R6sht. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * fix(shutdown): close double-teardown and secret re-caching races found in review - Latch `shutdown_started` at the first close attempt (graceful or force-close) so on_exit()'s guard blocks fallback re-entry on every branch, not only the clean-finish path -- stops a second close signal from racing wallet teardown against a still-running first attempt (SEC-001). - Call forget_all_secrets() again after the bounded wallet-shutdown wait resolves, including the timeout branch, so a backend task that re-caches a secret mid-wait is still cleared before exit (SEC-002). - Register a SwitchNetwork-created wallet backend before its slow network call resolves, and race that call against shutdown's cancellation token, so an in-flight network switch is discoverable to teardown instead of being invisible to it (CODE-003). - Prune completed blocking-task entries, drop dead TaskManager::shutdown(), centralize the shutdown budget constant, and add a CHANGELOG entry (non-blocking cleanup from the same review). Adds regression tests for all three fixes; cargo fmt/clippy/tests all clean (verified independently, not from the fixing agent's own report). Co-Authored-By: Codex Sol <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> * fix: update merged test to new forward_backend_task_join_error signature The merge of origin/v1.0-dev (904ba0b) auto-merged src/app.rs cleanly at the text level, but left a semantic break: v1.0-dev's new test `panicking_scheduled_vote_sweep_clears_in_progress_guard` (added in #902) called `forward_backend_task_join_error(join_handle, ...)`, passing the raw `JoinHandle` — the pre-#905 signature. This branch's shutdown-race work changed the function to take an already-awaited `Result<(), JoinError>` (so the join can be observed via `tokio::select!` alongside shutdown), which every other call site in this file was updated for except this one new test that didn't exist when the signature changed. Add the missing `.await`, matching the two sibling tests immediately above and below it. No behavioral change to production code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: correct mislabeled platform PR reference in shutdown TODOs The commented-out ShutdownReport check and the ShieldedShutdownIncomplete bucket TODOs referenced platform-pr3968 (rs-platform-wallet-storage, an unrelated SQLite persistence PR). The actual upstream work that restores these types is the ThreadRegistry/shutdown-report PR, platform#3954. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(shutdown): keep wallet teardown behind task barrier Track timed network requests and MCP backend work before wallet teardown, and skip secret clearing whenever managed work fails to drain. Co-Authored-By: OpenAI Codex <noreply@openai.com> * fix(shutdown): close remaining barrier gaps Keep join-error callbacks and MCP network-switch lifecycle work inside the bounded shutdown barrier. Co-Authored-By: OpenAI Codex GPT-5 <noreply@openai.com> * fix(app): store the secret prompt host on AppState The base-branch merge and this PR's NetworkContextRegistered handler both landed changes to AppState without a textual conflict, but the handler referenced a `self.secret_prompt_host` field that never existed — the Arc<dyn SecretPrompt> built in AppState::new() was only installed on the initial network contexts and then dropped. This broke compilation (E0609) for both the Test Suite and Clippy CI jobs. Store the host as an AppState field so contexts registered later (via an in-flight network switch surviving into NetworkContextRegistered) can also have the secret prompt installed on them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(shutdown): forget wallet secrets on degraded shutdown outcomes Two independent reviewers (CodeRabbit and a Claude Code automated review) found that finish_shutdown_after_tasks (src/app.rs) and shutdown_app_context_wallet_backend (src/mcp/server.rs) only cleared session-cached wallet secrets on the Complete outcome. On BackendTasksTimedOut or a failed task manager, the wallet-backend Arc<AppContext> set was collected and then dropped unpolled, so forget_all_secrets() never ran — contradicting this PR's own CHANGELOG entry promising secrets are cleared on close. Split the two concerns: forget_all_secrets() now runs unconditionally on every collected wallet backend regardless of outcome (cheap, synchronous, no dependency on any task finishing), while the full coordinator shutdown() join stays gated on Complete only, since backend tasks may still be using those coordinators on a degraded outcome. Applies to both the async and blocking-fallback GUI shutdown paths, and to the standalone MCP shutdown helper. Adds regression tests proving secrets are forgotten with zero coordinator-shutdown calls on both degraded outcomes, in both src/app.rs::shutdown_tests and the new src/mcp/server.rs::tests. Co-Authored-By: Codex Sol <noreply@openai.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: OpenAI Codex GPT-5 <noreply@openai.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…r-shutdown-uaf-fixes-595cc9 # Conflicts: # packages/rs-platform-wallet-ffi/src/error.rs # packages/rs-platform-wallet/src/manager/mod.rs # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
…ment-hook/adapter state into the clean-shutdown verdict Resolves the three lifecycle blockers from review: 1. Swift deinit now retains persistenceHandler/eventHandler (the only strong owners behind the passUnretained callback contexts) when platform_wallet_manager_destroy returns errorShutdownIncomplete, so a straggling coordinator dereferences live memory instead of a dangling pointer. The JNI nativeDestroy mirrors the same contract by leaking the Kotlin context boxes on a non-clean destroy. 2. Coordinator quiesce() is now bounded (COORDINATOR_DRAIN_BUDGET, 10s) and returns whether the drain completed; a timed-out drain leaves the quiescing gate up and surfaces as WorkerStatus::Timeout in the ShutdownReport, so a wedged network/persister await can no longer hang FFI destroy forever ahead of the registry's join budget. sync_now passes hold an RAII SyncSlotGuard so a panicking pass clears is_syncing during unwind instead of wedging every later drain. clear_shielded / reset_platform_address_sync_state fail closed (PlatformWalletError::ShutdownIncomplete -> ErrorShutdownIncomplete) when the drain does not complete, and the shielded FFI stop surfaces the same code instead of a false success. 3. The ShutdownReport now covers every callback-capable background worker, not just the four registry coordinators: SPV stop failures land under WalletWorker::Spv, the DashPay payment-hook tracker drains under a bounded budget (abort-escalating, with un-abortable stragglers kept tracked for the destroy retry) under WalletWorker::DashPayPayments, and the wallet-event adapter joins under a bounded budget (re-parking its live handle on timeout) under WalletWorker::EventAdapter — so all_clean() can no longer authorize freeing host callback state while SPV-driven persistence work is live. Also merges current v4.1-dev (ErrorShutdownIncomplete moves to slot 27; base took 22-26 for the asset-lock/core-funds codes) and adds regression tests: bounded-drain timeout per coordinator, SyncSlotGuard panic unwind, payment-tracker abort/survivor paths, and report coverage of the non-registry workers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 1d9d23f (merge of current `v4.1-dev`) + 93d0bd4, resolving the merge conflict and all three outstanding lifecycle blockers: Merge with v4.1-dev — `ErrorShutdownIncomplete` moves from slot 22 to 27 (base took 22–26 for the core-funds / asset-lock / broadcast-rejected codes); the Swift mirror moves with it. `shutdown()` keeps base's SPV-stop + payment-handler quiesce ordering and this PR's registry join + `ShutdownReport`. Blocker 1 — Swift releases callback owners after incomplete shutdown. `PlatformWalletManager.deinit` now retains `persistenceHandler` / `eventHandler` via `Unmanaged.passRetained` when destroy returns non-success, so a straggling worker's `passUnretained` context pointers stay valid (a bounded, deliberate leak on the fault path — the handle is already gone, so no retry is possible). The JNI `nativeDestroy` had the identical hazard ("destroy is documented to always return ok" + unconditional context-box drop); it now leaks the Kotlin context boxes on a non-clean destroy, mirroring the Swift contract. Blocker 2 — unbounded coordinator drains. Every coordinator `quiesce()` is now bounded (`COORDINATOR_DRAIN_BUDGET` = 10 s; the four drains run concurrently under `tokio::join!`) and returns whether the drain completed. A timed-out drain leaves the `quiescing` gate up and is folded into the report as `WorkerStatus::Timeout` — this covers a wedged direct `sync_now` on a host thread that the registry join can never see. Panic-safety: passes hold an RAII `SyncSlotGuard` that clears `is_syncing` during unwind. `clear_shielded` / `reset_platform_address_sync_state` fail closed with the new `PlatformWalletError::ShutdownIncomplete` → `ErrorShutdownIncomplete`, and the shielded FFI stop surfaces the same code instead of a false success. Blocker 3 — verdict excluded SPV and spawned passes. `ShutdownReport` now carries three non-registry keys: `WalletWorker::Spv` (SPV stop outcome; its join was already bounded+abort-escalating), `WalletWorker::DashPayPayments` (payment-hook tracker drained under a bounded budget with abort escalation; an un-abortable straggler — e.g. stuck in a synchronous persister call — stays tracked for the destroy retry and reports `Timeout`), and `WalletWorker::EventAdapter` (bounded join, live handle re-parked on timeout). `all_clean()` can no longer pass while SPV-driven persistence work is live. Regression tests (all RED-provable): `quiesce_within_times_out_and_leaves_gate_up_when_pass_never_drains` (× dashpay / identity / platform-address), `sync_slot_guard_clears_flag_on_panic_unwind`, `quiesce_within_aborts_await_parked_straggler_and_reports_clean`, `quiesce_within_keeps_unabortable_straggler_tracked_and_reports_false`, and the shutdown-report test now asserts the three non-registry keys are present and clean. Verification: CI-equivalent scoped clippy (`platform-wallet`, `platform-wallet-storage`, `platform-wallet-ffi`, `rs-unified-sdk-ffi`, `rs-unified-sdk-jni`, `--all-features -D warnings`) clean; `cargo nextest` 1075/1075 (wallet crates, all-features, non-shielded filter) + 132/132 shielded-named + 47/47 `dash-async` passing; `cargo fmt --check` clean; no new rustdoc private-link warnings. 🤖 Generated with Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The prior Swift teardown use-after-free finding is fixed: Swift now retains both callback owners after a non-clean destroy, and JNI mirrors that fail-closed behavior. Two blocking lifecycle gaps remain: clear/reset can race concurrent one-shot syncs after quiesce reopens its gate, and SPV teardown still contains unbounded waits that can prevent the FFI boundary from returning a shutdown report. The payment-task drain and clear/reset error mapping also need smaller boundedness and API-contract corrections.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:501-576: Clear and reset reopen sync admission before state mutation finishes
Both reset flows successfully quiesce their coordinator and then mutate state, but `quiesce_within` clears the coordinator's `quiescing` flag immediately before returning. `clear_shielded` keeps a registry clearing latch raised, but direct `ShieldedSyncManager::sync_now` and `sync_wallet` calls do not consult that latch; `reset_platform_address_sync_state` does not hold a clearing latch at all. The FFI operations use shared `HandleStorage::with_item` read guards, while Swift dispatches one-shot sync and reset operations through independent detached tasks and Kotlin uses `Dispatchers.IO`, so these calls may execute concurrently. A shielded pass can snapshot the old account set and refill the commitment tree after `coord.clear()` resets it, while an address pass can acquire a wallet's provider lock after that wallet was reset and persist a new watermark or balances. Hold lifecycle exclusion across the complete quiesce-to-mutation section, make every direct shielded entry point observe the clearing latch after claiming `is_syncing`, and add equivalent per-key exclusion for the platform-address reset.
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:629-640: SPV teardown can still block the FFI boundary indefinitely
`shutdown` now documents every phase as bounded, but it awaits `spv_manager.stop()` before producing a `ShutdownReport`. `SpvRuntime::stop` first awaits `DashSpvClient::stop()` without a timeout (`spv/runtime.rs:237-243`), then `join_spv_task` only bounds the initial graceful join: after 15 seconds it calls `abort()` and awaits the handle without a post-abort deadline (`spv/runtime.rs:32-46`). Dash SPV dispatches host event callbacks synchronously from monitor tasks and its run loop joins those monitors; Tokio cancellation cannot interrupt a callback blocked in synchronous host code. The newly added Swift deinitializer also calls the same SPV stop FFI before calling destroy, so either boundary can hang before `ErrorShutdownIncomplete` is returned and before the retain/leak fallback runs. Bound both the client stop and post-abort confirmation phase, preserve or re-park any still-live handle, and report it as non-clean so the destroy retry cannot mistake a detached callback-capable task for a clean stop.
In `packages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rs:121-145: Payment abort grace is applied separately to every straggler
The graceful joins share one deadline, but after that deadline each remaining handle is aborted and awaited under a fresh `PAYMENT_ABORT_GRACE`. Those waits run serially, so the phase can take approximately `budget + survivor_count * PAYMENT_ABORT_GRACE`, despite shutdown describing it as bounded by `PAYMENT_DRAIN_BUDGET`. Abort all handles that survive the shared graceful deadline first, then confirm their termination concurrently under one shared post-abort deadline and re-park whichever handles remain live.
In `packages/rs-platform-wallet-ffi/src/shielded_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_sync.rs:437-443: Clear and reset flatten shutdown-incomplete into a generic error code
This PR adds `PlatformWalletError::ShutdownIncomplete`, maps it to `ErrorShutdownIncomplete`, and documents clear/reset drain timeouts as producing that typed FFI result. This wrapper instead converts every `clear_shielded` failure to `ErrorWalletOperation`; `platform_wallet_manager_platform_address_sync_reset` repeats the same flattening at `platform_address_sync.rs:231-236`. Hosts currently fail closed on any error, so the wipe remains guarded, but callers cannot distinguish callback-capable work that is still running from an ordinary reset failure. Preserve the existing generic mapping for store errors while routing `ShutdownIncomplete` through the new typed conversion, and apply the same change to the address-reset wrapper.
| pub async fn shutdown(&self) -> ShutdownReport<WalletWorker> { | ||
| // 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()) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: SPV teardown can still block the FFI boundary indefinitely
shutdown now documents every phase as bounded, but it awaits spv_manager.stop() before producing a ShutdownReport. SpvRuntime::stop first awaits DashSpvClient::stop() without a timeout (spv/runtime.rs:237-243), then join_spv_task only bounds the initial graceful join: after 15 seconds it calls abort() and awaits the handle without a post-abort deadline (spv/runtime.rs:32-46). Dash SPV dispatches host event callbacks synchronously from monitor tasks and its run loop joins those monitors; Tokio cancellation cannot interrupt a callback blocked in synchronous host code. The newly added Swift deinitializer also calls the same SPV stop FFI before calling destroy, so either boundary can hang before ErrorShutdownIncomplete is returned and before the retain/leak fallback runs. Bound both the client stop and post-abort confirmation phase, preserve or re-park any still-live handle, and report it as non-clean so the destroy retry cannot mistake a detached callback-capable task for a clean stop.
source: ['codex']
| let deadline = tokio::time::Instant::now() + budget; | ||
| let mut survivors = Vec::new(); | ||
| for mut handle in handles { | ||
| let joined = match tokio::time::timeout_at(deadline, &mut handle).await { | ||
| Ok(result) => { | ||
| if let Err(error) = result { | ||
| tracing::warn!(?error, "DashPay payment task join error"); | ||
| } | ||
| true | ||
| } | ||
| Err(_) => { | ||
| handle.abort(); | ||
| match tokio::time::timeout(crate::manager::PAYMENT_ABORT_GRACE, &mut handle) | ||
| .await | ||
| { | ||
| Ok(result) => { | ||
| if let Err(error) = result { | ||
| if !error.is_cancelled() { | ||
| tracing::warn!(?error, "DashPay payment task abort join error"); | ||
| } | ||
| } | ||
| true | ||
| } | ||
| Err(_) => false, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Payment abort grace is applied separately to every straggler
The graceful joins share one deadline, but after that deadline each remaining handle is aborted and awaited under a fresh PAYMENT_ABORT_GRACE. Those waits run serially, so the phase can take approximately budget + survivor_count * PAYMENT_ABORT_GRACE, despite shutdown describing it as bounded by PAYMENT_DRAIN_BUDGET. Abort all handles that survive the shared graceful deadline first, then confirm their termination concurrently under one shared post-abort deadline and re-park whichever handles remain live.
source: ['codex']
…ollisions The 29 collision is resolved and the renumber has now landed on dashpay#4185's branch: dashpay#4184 keeps 29 (ErrorAssetLockInsufficientFunds), dashpay#4185 takes 30 (ErrorReservationWalletMismatch). Table rows updated to match the code. Fixes the "30 is both free and assigned" inconsistency: the next-free line claimed 27-33 were claimed while the table showed 30 unallocated. 30 is now genuinely allocated to dashpay#4185, so the two agree. Adds allocations the survey had omitted, verified 2026-08-01 by reading error.rs at the head of all 62 open PRs: - dashpay#3968 numbers 26/27/28 (Persister* + a pre-merge TransactionBroadcastRejected) -> contradicts merged ABI at 26 and collides with dashpay#4185 at 27 and 28 - dashpay#3954 numbers ErrorShutdownIncomplete = 27 -> collides with dashpay#4185 at 27 - dashpay#4259 carries ErrorSigningKeyUnavailable = 31, inherited from dashpay#4183 rather than a new allocation The same sweep confirms no open PR anywhere defines a code 30.
…cord dashpay#4196 scope Clears the two review blockers on dashpay#4261 and re-syncs the registry with what the code on each branch actually does, re-read at every head rather than trusted from this file. Blocker (a) — dashpay#3968 / dashpay#3954 / dashpay#4259 were described in prose but had no rows, which is exactly what rule 2 forbids. They now have them: - A "Non-conforming allocations" table for dashpay#3968 (26/27/28) and dashpay#3954 (27). These are deliberately kept out of the proposed table: each row is a claim to be withdrawn and reissued, not an allocation of record. - An inherited-code table for the 31 that dashpay#4204 and dashpay#4259 carry but did not allocate (dashpay#4183 owns it), so it is not double-counted. - dashpay#4196 is recorded as claiming no integer at all: it routes a new token-less `StaleReservation` variant through the existing `ErrorStaleReservationToken`. The dashpay#3968 half is the serious one and is called out as such. Its 28 is not a new claim — it *moves the already-shipped* `ErrorTransactionBroadcastRejected` off 26 to make room for its own persister code. Rule 3 forbids that: a host compiled against merged ABI returns 26 for a broadcast rejection, and after dashpay#3968 the same condition returns 28 while 26 means a transient persister failure. Neither branch's diff shows the contradiction. Blocker (b) — 30 marked both free and assigned was already resolved by the preceding commit; verified consistent here (30 is allocated to dashpay#4185 throughout, frontier is 34, and the one remaining "genuinely free" is past tense explaining why dashpay#4185 could take it). Also corrected, all verified against the branches: - Survey provenance had dashpay#4185 at `0b0d5c76d6` labelled "(post-renumber)". Wrong twice: that commit is the *parent* of the renumber `d854debb`, and the head has since moved to `6c37e8679e`. dashpay#4184, dashpay#4247 and dashpay#4256 SHAs refreshed too. - dashpay#4256 has now taken 30 (`9481e5783b`) and dropped its stale "30 is reserved for the consent code" rationale; the equivalent comments on dashpay#4183 and dashpay#4204 are flagged as still present. - dashpay#4184 has a comment-only drift: it reserves "Codes 27-28" but names three codes. Correct when the trio was 27/28/29; it is now 27/28/30. Its discriminant is right and is the resolution of record — only the prose is stale, and dashpay#4184 is left untouched. - The dashpay#4196 section now records why the restack has not happened: its three own commits conflict in 3 files / 10 hunks against dashpay#4185's head, and the registry redesign underneath it (mandatory `registered_height`, new `WalletRemoved` variant, owner-stamped funding token) makes it author work rather than conflict resolution. Its trio numbers come from the dashpay#4185 copy it carries, so the restack fixes 28 -> 30 for free; the number dashpay#4196 itself must chase is 27, not 30. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 738 passed / 0 failed. Docs-only change.
…d SPV teardown Three lifecycle gaps the last review round flagged, plus the Swift 6 compile error that was failing the Swift SDK CI job. **Clear / reset reopened sync admission before their mutation finished.** `quiesce()` reopened the `quiescing` gate the instant it returned, so `clear_shielded` and `reset_platform_address_sync_state` ran their wipe with admission already open — a direct `sync_now` / `sync_wallet` on a host thread could snapshot pre-wipe state and re-persist it right after. The per-coordinator `AtomicBool` becomes a shared `QuiesceGate` (mutex-serialized transitions, one atomic flag on the pass hot path) with an RAII `QuiesceGuard`: `quiesce_held()` drains AND keeps admission shut until the guard drops, so both reset paths now hold it across the whole quiesce -> mutate section. Overlapping holders compose; the gate reopens only on the last drop. `reset_platform_address_sync_state` also takes the registry clearing latch, which only `clear_shielded` held before. Shutdown now uses `quiesce_sealed_within`: it is terminal, so admission must never reopen — the FFI resolves the manager under a shared read guard, so a `sync_now` dispatched on a host thread can sit between its slot CAS and its gate check while `destroy` runs. `PlatformAddressSyncManager::sync_wallet` bypassed both the `is_syncing` slot and the gate; it now goes through the same admission as `sync_now` and is refused (typed `AddressSync` error) while a reset holds the gate. **SPV teardown could still hang the FFI boundary.** `SpvRuntime::stop` awaited `DashSpvClient::stop()` with no deadline, and `join_spv_task` bounded only the graceful join — after aborting it awaited the handle forever, which an abort cannot interrupt when the task is parked in synchronous host-callback code. Both phases are bounded now, and a run loop that survives the post-abort grace is re-parked (never dropped, which would detach a callback-capable task) with `stop` returning an error so the shutdown verdict is non-clean and a destroy retry re-joins it. **Payment-hook drain applied its abort grace per straggler**, so the phase could take `budget + survivors x PAYMENT_ABORT_GRACE` despite shutdown advertising a fixed bound. Survivors are now all aborted first, then confirmed against one shared deadline. **FFI clear/reset flattened `ShutdownIncomplete`** into `ErrorWalletOperation`; both wrappers now route that one case through the typed conversion so hosts can tell "callback-capable work is still running" from an ordinary failure. **Swift:** `PlatformWalletEventHandler` is `@unchecked Sendable`, matching `PlatformWalletPersistenceHandler`. It is a cross-thread callback context by construction, and the nonisolated `deinit` added by this PR cannot touch a non-Sendable stored property — that was the CI compile error. Tests: gate held across the mutation, overlapping guards, sealed gate never reopening, `sync_wallet` refused under a held gate, reset failing closed on a wedged pass with no latch left stuck, and an SPV run loop surviving abort being handed back for re-parking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed Blocker 1 — clear/reset reopened sync admission before the mutation finished. Rather than adding an Two related fixes fell out of the same review: Blocker 2 — SPV teardown could still block the FFI boundary. Suggestion — per-straggler payment abort grace. All survivors are aborted Suggestion — clear/reset flattened Swift CI (was red on this PR, and it was ours). The nonisolated Tests (new, all failing against the prior behavior): Verification: 🤖 Generated with Claude Code |
|
Superseded by #4268, which carries this branch's work forward (every
The review that happened here was not wasted: every lifecycle race found 🤖 Generated with Claude Code |
4 participants
Human-generated TL;DR
dash-evo-tool crashes on shutdown, and mismanages spv sync start/stop, because some start/stop coordination between tokio and OS threads is not working well.
Issue being fixed or feature implemented
The four background sync coordinators (platform-address/BLAST, per-identity
token state, DashPay, shielded) each run a
!Sendsync loop on a detached OSthread. At manager teardown / FFI
destroynothing joined those threads:a loop mid-pass could keep calling
persister.store(...)or fire hostcompletion callbacks through a context the host frees the instant
destroyreturns — a use-after-free. Cancellation + join were also racy: a
stop()→quick
start()could leave a newer loop uncancellable, and astart()racingshutdown()could leave a live, never-cancelled loop the registry then joinswith a 30s timeout while it keeps firing callbacks.
What was done?
Merged onto current
v4.2-dev; the new FFI error code below takes slot27(slots
21–26are already used by the address-nonce / core-funds /asset-lock / broadcast-rejected codes on the base branch).
dash-async: new sharedThreadRegistry<K>lifecycle engine (registry.rs).Tracks background OS-thread / tokio-task workers, owns their spawn +
cancellation + join, and reports teardown as a
ShutdownReport. Surface usedby the wallet:
start_thread(registry spawns, owns cancellation, and joins— one slot lock covers check → spawn → install, no gap to race), weight-ordered
shutdown→ShutdownReport,quiesce, per-key clearing latch(
hold_clearing/is_clearing), and the teardown latchis_closing. Orphanreap keeps a still-draining prior generation UAF-safe (parked, never
dropped-and-detached).
WorkerConfiggained astack_sizefield (None=platform default) so a caller with a deep-recursion loop body (DashPay's
GroveDB proof descent — see below) can request a larger OS-thread stack
without hand-rolling its own
Builder. TheAtomicFlagGuard/RefcountedFlagGuardhelper types originally drafted alongside the registry had zero callers
anywhere in this diff or in the wallet, so they were removed rather than kept
speculatively.
Wallet: all four coordinators spawn through
registry.start_thread, full stop.Each coordinator's
start()used to spawn its ownstd::thread, install ahand-rolled
LoopCancelGuard(a per-coordinator, duplicated generation-guard),and hand the
JoinHandleto the registry after the fact — two independentlychecked gates instead of one. That's gone:
start()now callsregistry.start_thread(key, cfg, |cancel| { ...loop... })and the registryowns the whole lifecycle — the
is_closing()/is_clearing()gate check, thespawn, and the cancellation-token install all happen under the same slot
lock, so there's no window between "checked it's safe" and "spawned anyway."
loop_cancel.rsis deleted outright — zero remaining references repo-wide.See Why this instead of keeping
LoopCancelGuardbelow for the fullerrationale.
PlatformWalletManager::shutdownquiesces each coordinator(drains any in-flight pass incl. its persister/callback fan-out), then
hard-joins the loop threads via the registry, then drains the wallet-event
adapter, returning a
ShutdownReportkeyed byWalletWorker.FFI:
platform_wallet_manager_destroygates on the report. Runs the fullshutdown; on a non-clean report it retries
shutdown()once (the re-quiescecancels any loop that raced the first pass); if it still can't cleanly join
every coordinator it returns the new
ErrorShutdownIncompletecode instead ofonly logging. The Swift host mirror
(
PlatformWalletResultCode.errorShutdownIncomplete+ typedPlatformWalletError.shutdownIncomplete) is included in this PR's diff.Hardening fixes folded in:
shutdownre-drains late-parked orphans soall_clean()can't false-pass on a straggler; the restart reap classifies ajoined/dropped prior generation (surfacing a panicked prior) instead of
discarding the join result;
coordinator_worker_configusesdash_async::DEFAULT_JOIN_BUDGETdirectly.Why this instead of keeping
LoopCancelGuard?The first version of this PR fixed the three races with defense-in-depth:
keep each coordinator's hand-rolled
LoopCancelGuardfor cancellation, addthe registry alongside it purely for join/status. That closes the races
(verified — regression tests for all three), but it does it with two
separately-checked gates instead of one atomic operation, and it's four
near-identical copies of the same generation-guard logic.
ThreadRegistryalready had a stronger primitive sitting unused:
start_thread(), whichperforms the shutdown-state check, the spawn, and the cancel-token install
under one slot lock. This revision migrates all four coordinators onto it and
deletes
loop_cancel.rs.LoopCancelGuard(this PR's first version)start_thread()(this revision)std::thread::Builder::spawnstart_thread's factory closure, under its own slot lockSlotStateowns the token for every workeris_closing/is_clearing, then install) — a real, if narrow, gap between themstop()+ quickstart()LoopCancelGuard's original reason to existSlotState's own generation bumpEpilogueGuard'sDropruns on every exit path including panic, and clears the running flag — closes a real gap: the oldclear_if_currentwas fall-through and could leaveis_running() == trueafter a panicConcretely, this also tightens RUST-003 (shielded-sync TOCTOU wipe/resurrect):
ShieldedSyncManager::start()used to do its own check → install → re-checkdance around
is_closing()/is_clearing()before spawning; that's now asingle
registry.start_thread(...)call with no application-level recheckneeded, because the registry's own start-slot lock covers both latches
already.
One deliberate addition to make the move backward-compatible: DashPay's loop
needs an 8 MiB stack (GroveDB
verify_layer_proof_v1recursion SIGBUSeson-device on the default), previously set via a manual
Builder::stack_size.start_threadspawned with the default, soWorkerConfiggained astack_size: Option<NonZeroUsize>field (Nonefor the other threecoordinators, honored only by
start_thread) rather than silently droppingthe larger stack.
Full visual comparison (spectrum diagram + row-by-row table):
https://claude.ai/code/artifact/aa6e2bbc-6c34-4384-abdd-b9339909c6d0
How Has This Been Tested?
Scoped through the cached cargo wrapper (
-p dash-async -p platform-wallet -p platform-wallet-ffi,--features platform-wallet/shielded):cargo clippy --all-targets -- -D warnings→ clean (re-verified after thestart_threadmigration, independently re-run).cargo nextest run→ passing, incl.dash-async44 + 1 doctest and thefull
platform-walletsuite with theshieldedfeature on.RUSTDOCFLAGS="-D warnings" cargo doc -p dash-async --no-deps→ clean.cargo fmt --check→ clean.Registry regression tests assert the intended contract (RED-provable):
is_closing_tracks_shutdown_latch,register_thread_after_shutdown_wedged_orphan_flips_all_clean(a livestraggler flips
all_clean),register_thread_restart_reaps_panicked_prior(a panicked prior is joined+classified, the restarting caller neither hangs
nor inherits the panic),
start_thread_honors_custom_stack_size(the newWorkerConfig::stack_sizepath), alongside the existing quiesce/shutdown/generation-guard/orphan-reap suite.
Each coordinator's stale-loop regression test was rewritten to drive real
start()/stop()on live OS-thread loops through the registry instead ofpoking the (now-deleted) guard directly — e.g.
stop_then_quick_start_keeps_new_loop_cancellable— and a fullshutdown().awaitat the end of each assertsreport.all_clean().The Swift mirror addition (
PlatformWalletResult.swift) has not beencompiled on this branch — no Swift toolchain in this environment. It was
hand-verified against the file's established pattern (exhaustive switch
coverage, brace balance, cbindgen-generated constant naming). Note: this
PR's Swift CI check is currently red for an unrelated, pre-existing reason —
see PR comments below.
Breaking Changes
PlatformWalletManager::shutdown(&self)return type:()→ShutdownReport<WalletWorker>.registry: Arc<ThreadRegistry<WalletWorker>>parameter:IdentitySyncManager::new,PlatformAddressSyncManager::new,DashPaySyncManager::new,ShieldedSyncManager::new.dash_async::WorkerConfig(public struct) gains astack_size: Option<NonZeroUsize>field — a full struct-literal construction outside this diff needs
..WorkerConfig::default()or an explicit value.PlatformAddressSyncManager::sync_walletnow goes through the sameadmission as
sync_now(claims theis_syncingslot, then honors thequiescing gate) and returns
PlatformWalletError::AddressSyncwhen a passis already in flight or a reset / teardown holds admission shut.
Previously it took neither and always ran.
PlatformWalletFFIResultCode::ErrorShutdownIncomplete = 27;platform_wallet_manager_destroycan now return a non-Successresult when acoordinator thread can't be cleanly joined even after a retry (previously it
always returned
Success).Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes