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 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.1-dev #3954 +/- ##
============================================
+ Coverage 87.43% 87.45% +0.02%
============================================
Files 2646 2647 +1
Lines 333981 335334 +1353
============================================
+ Hits 292023 293274 +1251
- Misses 41958 42060 +102
🚀 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
|
⛔ Blockers found — Sonnet deferred (commit 32e665d) |
|
🟠 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 |
17e1c07 to
a89e0f9
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
No carried-forward prior findings from the 17e1c07 review; the earlier shutdown-gate issue remains fixed. I verified one new in-scope latest-delta regression: the SQLite same-path open guard was removed while buffers remain per-persister. The destroy finding was dropped because the PR explicitly documents destroy as terminal and the Swift wrapper implements that contract by retaining callback contexts on incomplete shutdown.
🔴 2 blocking
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
packages/rs-platform-wallet-storage/src/sqlite/persister.rs:205-208: Same-path SQLite opens now create divergent buffers — The latest delta removed the process-wide canonical-path registry and theAlreadyOpenerror, soopen()now returns a freshSqlitePersisterwith its ownBuffer::new()for a database path that may already be live in this process. SQLite can serialize committed transactions, but it cannot co...
🤖 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-storage/src/sqlite/persister.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:205-208: Same-path SQLite opens now create divergent buffers
The latest delta removed the process-wide canonical-path registry and the `AlreadyOpen` error, so `open()` now returns a fresh `SqlitePersister` with its own `Buffer::new()` for a database path that may already be live in this process. SQLite can serialize committed transactions, but it cannot coordinate these Rust-side buffers: `store()` stages data in the handle-local buffer, `flush()` drains only that buffer, and `load()` reads through only that persister's connection. Two in-process managers using the same wallet DB can therefore miss each other's pending state or commit competing buffered changes in last-flush-wins order. Restore the same-path guard, or move the buffer into a shared path-scoped owner before allowing multiple live handles for one path.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:205-208: Same-path SQLite opens now create divergent buffers
The latest delta removed the process-wide canonical-path registry and the `AlreadyOpen` error, so `open()` now returns a fresh `SqlitePersister` with its own `Buffer::new()` for a database path that may already be live in this process. SQLite can serialize committed transactions, but it cannot coordinate these Rust-side buffers: `store()` stages data in the handle-local buffer, `flush()` drains only that buffer, and `load()` reads through only that persister's connection. Two in-process managers using the same wallet DB can therefore miss each other's pending state or commit competing buffered changes in last-flush-wins order. Restore the same-path guard, or move the buffer into a shared path-scoped owner before allowing multiple live handles for one path.
… join [dashpay#3954, independent on v3.1-dev] Introduce a shared `ThreadRegistry<WalletWorker>` lifecycle engine in `dash-async` (weight-ordered graceful shutdown: lower weights drain first, equal weights concurrently; reap-or-park orphan handling; cancellation-safe `RefcountedFlagGuard`). Migrate the three periodic sync coordinators (identity / platform-address / shielded) onto a shared `CoordinatorLifecycle` that drives them through the registry and gates passes via an `is_syncing` / `quiescing` handshake. `PlatformWalletManager::shutdown()` now holds every coordinator's quiescing gate across the whole teardown, quiesces the weight-0 coordinators concurrently, then drains the weight-10 wallet-event adapter and any parked orphans, returning a per-worker `ShutdownReport`. `clear_shielded` refuses (leaving the commitment-tree store intact) when the in-flight pass does not drain cleanly, surfaced as `ShieldedShutdownIncomplete` / `ErrorShutdownIncomplete = 19`. FFI `destroy` reports the incomplete-shutdown code so the Swift host defers freeing its callback context. Reconstructed independently on a clean v3.1-dev base: only the graceful- shutdown contribution is included; the drifted dashpay#3692 rehydration/storage work merged into the source branch is excluded. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Re: thepastaclaw's 2026-06-29 re-review ( This is a storage-crate concern that belongs to #3968, not this PR. #3954 ( The concern itself is real and already addressed in #3968: the process-wide canonical-path registry + Dispositioning as out of scope for #3954; tracked and fixed in #3968. 🤖 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
Thanks for the correction. I rechecked current head a89e0f97daefc9363005644a77c0b1b4fa000509 and agree the same-path SQLite open finding is out of scope for #3954: this PR's diff has no rs-platform-wallet-storage files, and the storage persister guard belongs to the sibling storage PR #3968 / its branch context rather than this ThreadRegistry + wallet lifecycle PR.
I am superseding my 2026-06-29 CHANGES_REQUESTED review for #3954. I do not see any remaining in-scope blockers on the current #3954 head from the prior shutdown-gate review trail.
There was a problem hiding this comment.
Code Review
The prior blocking finding (same-path SQLite opens creating divergent buffers) is FIXED at 6594bf3 — the process-wide open_path_registry, register_open_path/release_open_path, AlreadyOpen guard, and Drop-based release are all restored in packages/rs-platform-wallet-storage/src/sqlite/persister.rs (lines 87-111, 234). The latest delta (a89e0f9..6594bf3) is a documentation/test-name comment-hygiene sweep on registry.rs/coordinator_lifecycle.rs/manager/mod.rs plus a new FFI regression test proving platform_wallet_manager_shielded_sync_stop is cancel-only while platform_wallet_manager_destroy is the actual join point. No new in-scope blocking or suggestion-level defects identified across six parsed Claude-family reviewer outputs.
_Note: Normal review_poster dry-run failed (Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 1163, in
result = post_review(
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 874, in post_review
diff_text = _gh("pr", "diff", str(pr_number), "--repo", repo)
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 96, in gh
raise RuntimeError(f"gh {' '.join(args)} failed: {result.stderr.strip()}")
RuntimeError: gh pr diff 3954 --repo dashpay/platform failed: could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of lines (20000) (https://api.github.com/repos/dashpay/platform/pulls/3954)
PullRequest.diff too_large), so I posted the verified findings as a top-level exact-SHA review body.
Reviewed commit: 6594bf3
…ed ThreadRegistry The four periodic sync coordinators (platform-address, identity, dashpay, shielded) run their !Send loops on detached OS threads. Previously each `start()` discarded the spawned thread's JoinHandle, so `shutdown()` only soft-drained the in-flight pass (the is_syncing barrier) and never joined the thread -- a host that drops the tokio runtime right after shutdown could race a coordinator still unwinding out of Handle::block_on and panic with "A Tokio 1.x context was found, but it is being shutdown". Extend rs-dash-async's ThreadRegistry with `register_thread`: a token-less, join/status-only handle adoption. Each coordinator now hands its loop thread's JoinHandle to a shared registry (parking any still-draining prior on restart), while its existing LoopCancelGuard stays the sole canceller -- the registry sits alongside purely for the join. `shutdown()` quiesces all four coordinators, then joins their threads via `registry.shutdown()`, returning a `ShutdownReport<WalletWorker>` that surfaces a panicked / timed-out / detached loop instead of dropping it silently. clear_shielded holds the registry's per-key clearing latch across its quiesce->wipe, and shielded `start()` refuses under that latch, so a concurrent shielded start can't re-persist into the store being cleared. This rebases PR #3954's shutdown-join design onto v4.1-dev's LoopCancelGuard coordinators and extends coverage to the dashpay coordinator (new since #3954). rs-dash-async gains atomic.rs (AtomicFlagGuard) + registry.rs (ThreadRegistry) on top of its existing block_on module. Tests: rs-dash-async 44 unit tests (incl. 6 new register_thread cases), rs-platform-wallet 516 lib tests (shielded), clippy + rustfmt clean on all three crates. BREAKING CHANGE: `PlatformWalletManager::shutdown()` now returns `ShutdownReport<WalletWorker>` instead of `()`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6594bf3 to
22043fa
Compare
There was a problem hiding this comment.
Code Review
Source: reviewers: codex-general, codex-security-auditor, codex-rust-quality, codex-ffi-engineer (gpt-5.5 completed); claude-general, claude-security-auditor, claude-rust-quality, claude-ffi-engineer (opus failed: extra-usage quota, resets Jul 10 08:00 America/Chicago). verifier: codex (gpt-5.5).
Carried-forward prior findings from 6594bf3: none; the prior review was clean and there are no active prior findings to reconcile. New latest-delta findings: the current head regresses the shutdown contract by returning FFI destroy success on a non-clean report, and the reimplemented coordinator starts spawn threads outside the registry’s atomic clear/shutdown barriers.
🔴 2 blocking
🤖 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:365-377: Destroy reports success after non-clean shutdown
`manager.shutdown()` returns a `ShutdownReport` whose `all_clean()` is the documented gate before freeing host callback context, because `Timeout` or `Detached` means a coordinator thread or orphan may still be alive. This path removes the manager handle, logs `!report.all_clean()`, and still returns `ok()`. Swift retains the persistence/event callback owners only for the manager lifetime and `deinit` discards the destroy result, so a timed-out or detached worker can later invoke raw callback context pointers after Swift has released the objects. This is a latest-delta regression: the prior reviewed code returned `ErrorShutdownIncomplete` when the report was not clean. Restore that error path or otherwise keep the handle/context alive until shutdown is clean.
In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/shielded_sync.rs:230-275: Coordinator starts can bypass registry clear and shutdown barriers
The start path checks `is_clearing`, installs its own cancel token, spawns the OS thread, and only then hands the handle to `ThreadRegistry::register_thread`. That leaves a real pre-registration window outside the registry’s atomic barriers. For `clear_shielded`, a racing start can observe `is_clearing == false`, then `clear_shielded()` can take the clearing latch, quiesce while no token/thread is visible, lower `quiescing`, and begin wiping; when start resumes, `register_thread()` only parks the already-running handle as an orphan and does not cancel the token, so the loop can run `sync_now(false)` into the store being cleared. The same spawn-before-register pattern in the other coordinator starts also undermines the public Rust `shutdown()` contract: a direct Rust start can create a worker after that manager’s `quiesce()` and after `registry.shutdown()` has snapshotted no slot, leaving a late orphan that was not included in the clean report. Use registry-owned spawning (`start_thread`) or a manager/registry API that acquires the closing/clearing gate, installs cancellation, spawns, and registers atomically before any worker can run.
… faults Harden the coordinator lifecycle against a start() racing shutdown()/destroy, so no live, never-cancelled loop can outlive the FFI host context. - ThreadRegistry gains a public `is_closing()` mirroring `is_clearing()`. All four coordinators (identity/platform-address/dashpay/shielded) now gate start() on it before spawning, and re-check after installing the cancel token (check-lock-check) — cancelling and releasing the slot rather than spawning a loop teardown has stopped waiting for. - shielded start() also re-checks the clearing latch after install, closing the TOCTOU where a fresh pass could re-persist notes right after a wipe. - register_thread logs an error (was silent) when it must park a live worker as an orphan because the registry is closing/clearing. - shutdown() re-drains the orphan list after the reap so a register_thread that parks late (racing the same teardown) cannot let all_clean() false-pass. - Restart reap now classifies a joined/dropped prior generation and logs a non-clean exit instead of discarding the join result. - FFI destroy retries shutdown once on a non-clean report and, if still not clean, returns the new ErrorShutdownIncomplete code instead of only warning. - coordinator_worker_config uses dash_async::DEFAULT_JOIN_BUDGET directly, dropping the duplicate SHUTDOWN_JOIN_TIMEOUT_SECS constant. - Drop the unused `test-util` feature; the reap seam is `cfg(test)`-only. - Soften the panic=abort canary test doc (manual-only, not CI-enforced). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… surface `AtomicFlagGuard` and `RefcountedFlagGuard` have zero code callers: the registry gates on a raw `AtomicBool` (`closing`) and a `ClearingGuard` refcount, not these guards, and the wallet coordinators use raw atomics directly. They are speculative surface staged ahead of a future consumer. Remove `atomic.rs`, its `lib.rs` export, and the doc-prose mentions on the registry's panic=abort caveat that referenced the moved types (EpilogueGuard remains and carries that caveat in base). Also fix a broken intra-doc link in the new `is_closing` rustdoc. The guards are re-added on the stacked follow-up branch claudius/3954-followup-registry-extras. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ge-races-body # Conflicts: # packages/rs-platform-wallet-ffi/src/error.rs
…tResult Add the Swift host mirror for the new FFI result code `ErrorShutdownIncomplete = 22` that `platform_wallet_manager_destroy` can now return. Mirrors the established pattern for every other code: the `PlatformWalletResultCode` case, its `init(ffi:)` mapping arm from the cbindgen constant, a typed `PlatformWalletError.shutdownIncomplete(String)` case, and the arms in the two exhaustive switches (`errorDescription`, `init(result:)`) — which would otherwise fail to compile once the result-code case is added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus (failed); claude general opus (failed); claude rust-quality opus (failed); codex ffi-engineer gpt-5.5; codex general gpt-5.5; codex rust-quality gpt-5.5; verifier = codex gpt-5.5; specialists = rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
Prior reconciliation: prior-destroy-non-clean-success is fixed at the Rust FFI boundary because destroy now retries shutdown and returns ErrorShutdownIncomplete if the retry is still non-clean; prior-start-registry-window remains valid for the spawn-before-register shutdown race, while the shielded clear-latch portion is fixed. Carried-forward prior findings: one blocking coordinator lifecycle race remains. New findings in latest delta: Swift now maps the new shutdown-incomplete error but the primary deinit path still discards it and releases callback owners.
🔴 2 blocking
🤖 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/identity_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/identity_sync.rs:423-464: Coordinator starts still spawn outside the shutdown registration barrier
The start path checks `registry.is_closing()`, installs the coordinator-owned cancel token, checks `is_closing()` again, spawns the OS thread, and only then calls `register_thread`. That leaves a pre-registration window where `shutdown()` can latch closing, snapshot/reap no registered slot or orphan, see the orphan list empty, and return `all_clean()` before the starter reaches `register_thread` and parks the already-running handle. The latest registry late-orphan loop only catches handles that have already been parked before its final empty check; it does not make spawn plus registration atomic. The FFI start/destroy entry points are serialized by the handle-storage lock, so this is specifically the public Rust lifecycle/shutdown contract, but it is still in scope for this PR because the new shutdown report is meant to prove no coordinator worker can outlive a clean shutdown. Use a registry-owned start API, or add an API that reserves the slot, installs cancellation, spawns, and stores the handle under the registry's shutdown gate before the worker can run.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:223-230: Swift deinit discards shutdown-incomplete and frees callback contexts
Rust now returns `ErrorShutdownIncomplete` from `platform_wallet_manager_destroy` when shutdown still reports a live or detached coordinator after retry, and the Swift result layer maps that code to `PlatformWalletError.shutdownIncomplete`. This deinit path still calls `platform_wallet_manager_destroy(handle).discard()`, so the non-success result is freed and ignored; then `PlatformWalletManager` releases `persistenceHandler` and `eventHandler`, which are the only strong owners for callback contexts passed to Rust with `Unmanaged.passUnretained`. If Rust returned shutdown-incomplete, its own contract says a worker may still outlive destroy and call through those raw context pointers, so this path can still produce use-after-free, crash, or persistence corruption. The deinit path must not silently drop this result; it needs to keep the callback owners alive or otherwise fail closed when destroy reports an incomplete shutdown.
CI note: the Swift SDK build failure is pre-existing on
|
|
Update: found it. #4063 (currently draft) already fixes this — it bumps the 🤖 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']
…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>
3 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.1-dev; the new FFI error code below takes slot22(slot
21is already used byErrorAddressNonceMismatch, merged via #4047).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.PlatformWalletFFIResultCode::ErrorShutdownIncomplete = 22;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