feat(platform-wallet): registry-owned coordinator lifecycle with Rust-owned FFI callback contexts - #4268
Merged
Merged
Conversation
…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>
… 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>
…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>
…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>
…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>
…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>
Invert the callback-context ownership contract that made shutdown a memory-safety problem. Both wallet-manager vtables (`PersistenceCallbacks`, `EventHandlerCallbacks`) gain a `release_fn` appended at the end: setting it transfers ownership of `context` to Rust. `FFIPersister` / `FFIEventHandler` release it in `Drop` — they are constructed exactly once per manager into the `Arc`s every background worker clones, so the release fires exactly once, when the manager AND its last worker are provably done calling into the host. That deletes the "prove all_clean() before the host may free memory" doctrine at every boundary: - `platform_wallet_manager_destroy` runs one bounded shutdown, logs a non-clean join, and always returns Success. The retry pass and the `ErrorShutdownIncomplete` return are gone from destroy — a straggling worker now keeps the host callback objects alive through its own Arc and releases them on exit, so there is nothing for the host to act on. (`ErrorShutdownIncomplete` remains for the clear/reset/sync-stop drain barriers, which are state-coherence gates, not memory safety.) - Swift hands both handlers over with `Unmanaged.passRetained` + a release trampoline, balances the retain itself on the create-failure path, and deletes the deliberate leak-on-incomplete-destroy block from `deinit`. - JNI wires release trampolines that drop the boxed `GlobalRef` contexts (jni 0.21's `GlobalRef::drop` self-attaches on a detached thread), slims `ManagerBundle` to just the manager handle, and deletes its leak-on-incomplete-destroy path. Kotlin needs no code change; docs updated. Null `release_fn` keeps the legacy borrowed contract, so out-of-tree callers are unaffected until they opt in. This also fixes a latent hazard the old contract could not: a wallet or manager-adjacent handle outliving `destroy` kept Rust-side clones of the persister pointing at host memory the host was free to release; those clones now own the host objects for exactly as long as they exist. Tests: vtable-level release-once-on-last-Arc-drop for both wrappers, end-to-end destroy-releases-exactly-once (including stale-handle no-double-release) through the public FFI, and the vtable layout pin updated to prove `release_fn` is the terminal field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 2, 2026 17:29
Contributor
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds shared bounded worker shutdown, quiescence reporting, typed shutdown errors, and exact-once callback-context ownership across Rust, JNI, Swift, and Kotlin integrations. ChangesShutdown lifecycle and callback ownership
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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 |
Collaborator
|
🕓 Ready for review — next in queue (commit 429667e) |
QuantumExplorer
changed the title
thepastaclaw
suggested changes
Aug 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The PR’s ownership inversion is sound for callback vtables that provide release functions, but two teardown/admission races remain. Borrowed callback contexts can still be used after a successful destroy, and concurrent drain acquisition can briefly reopen sync admission during a clear/reset barrier; both issues require changes before merge.
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— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 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/rs-platform-wallet-ffi/src/manager.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/manager.rs:457-474: Non-clean destroy is still unsafe for borrowed callback contexts
Both callback vtables explicitly preserve the legacy borrowed-context contract when `release_fn` is null, but this path now removes the manager and returns `Success` even when shutdown reports a callback-capable straggler. An `Arc<FFIPersister>` or `Arc<FFIEventHandler>` retains only the Rust wrapper and its raw `context` pointer in borrowed mode; it does not retain the host allocation behind that pointer. A legacy caller can therefore release its context after the successful destroy while a surviving worker later invokes a callback through freed memory. Preserve `ErrorShutdownIncomplete` or another actionable failure whenever either context is borrowed, reject borrowed contexts at creation, or retain the previous retry/fail-closed behavior for borrowed managers.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:259-268: Concurrent drains can reopen admission before the second barrier is established
A drain closes the atomic gate without recording itself in `GateBookkeeping`, and increments `holds` only after observing `is_syncing == false`. With two concurrent drains, the first can acquire and immediately drop its guard while the second is between its final `is_syncing` load and `gate.hold()`. That drop sets `closed` to false, allowing a direct sync to claim `is_syncing` and pass its gate check; the second drain then installs its hold and returns without rechecking the slot. A clear/reset using that guard can consequently mutate or wipe state while the newly admitted pass is running and later re-persisting stale state. Track active drain attempts as closing reasons, or serialize drain acquisition and recheck `is_syncing` after establishing the exclusive hold.
Comment on lines
457
to
474
| if !report.all_clean() { | ||
| // A coordinator thread panicked, exceeded its join budget, or | ||
| // stayed detached — possibly a loop that raced this teardown and | ||
| // installed its cancellation after our first quiesce. Retry once: | ||
| // `shutdown()` re-quiesces (cancelling any now-installed loop) and | ||
| // re-joins, which clears that race. The host frees its callback | ||
| // context after we return, so a still-live worker is a real UAF | ||
| // hazard, not just noise. | ||
| // A worker panicked, exceeded its join budget, or stayed | ||
| // detached. Its persister/event-handler Arcs keep the host | ||
| // callback contexts alive until it actually exits, so this | ||
| // is diagnostic, not a UAF hazard. | ||
| tracing::warn!( | ||
| ?report, | ||
| "platform wallet manager shutdown did not join every coordinator \ | ||
| thread cleanly on the first pass; retrying" | ||
| "platform wallet manager shutdown did not join every worker \ | ||
| cleanly; stragglers keep their callback contexts alive and \ | ||
| release them on exit" | ||
| ); | ||
| let retry = runtime().block_on(manager.shutdown()); | ||
| let merged = report.merged_with_retry(retry); | ||
| if !merged.all_clean() { | ||
| tracing::error!( | ||
| ?merged, | ||
| "platform wallet manager shutdown still could not join every \ | ||
| coordinator thread after a retry; a worker may outlive destroy" | ||
| ); | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorShutdownIncomplete, | ||
| format!( | ||
| "shutdown could not cleanly join all coordinator threads after \ | ||
| a retry: {merged:?}" | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| // Dropping the manager here releases its persister/event-handler | ||
| // references; the host contexts are released (via `release_fn`) | ||
| // as soon as the last worker's reference drops — typically right | ||
| // now, or later if a straggler is still draining. | ||
| } | ||
| PlatformWalletFFIResult::ok() |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Non-clean destroy is still unsafe for borrowed callback contexts
Both callback vtables explicitly preserve the legacy borrowed-context contract when release_fn is null, but this path now removes the manager and returns Success even when shutdown reports a callback-capable straggler. An Arc<FFIPersister> or Arc<FFIEventHandler> retains only the Rust wrapper and its raw context pointer in borrowed mode; it does not retain the host allocation behind that pointer. A legacy caller can therefore release its context after the successful destroy while a surviving worker later invokes a callback through freed memory. Preserve ErrorShutdownIncomplete or another actionable failure whenever either context is borrowed, reject borrowed contexts at creation, or retain the previous retry/fail-closed behavior for borrowed managers.
source: ['codex']
…mers use With callback-context ownership moved into Rust, the registry's job is runtime-drop safety (join workers before a host drops its tokio runtime) and lifecycle coherence (closing/clearing latches, generation-guarded restart) — not proving quiescence so a host can free memory. Delete the surface that existed for the latter doctrine or for consumers that never materialized: - `ShutdownWeight` / weight-ordered tiers (a single tier was ever used; shutdown now cancels + joins all workers concurrently) - `DrainHook` / `WorkerConfig::drain` (every caller passed `None`; the wallet drains its own passes before calling shutdown) - `start_task` (zero consumers — the wallet's tokio tasks are tracked by their own handlers) and with it the task half of `WorkerHandle` - `register_thread` (zero consumers since the coordinators migrated onto `start_thread`) - `merged_with_retry` (its caller, the FFI destroy retry, is gone) - `any_alive` / `any_alive_for` (demoted to a test-only assertion helper), `cancel_all`, `DRAIN_HOOK_WARN_THRESHOLD` Kept, deliberately: orphan park/reap and the Timeout/Detached accounting (a wedged thread must be reported, not silently detached, so a runtime-owning host knows dropping the runtime is unsafe), the closing/clearing latches, the generation guard, spawn-failure rollback, and `stack_size`. registry.rs 2,779 → ~1,760 lines; 19 tests of deleted APIs removed, the 28 covering the surviving surface still pass. `WorkerStatus::Stopped` survives as a consumer-side classification variant (the wallet's event-adapter join uses it) and is documented as such. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QuantumExplorer
changed the base branch from
feat/platform-wallet-shutdown-join
to
v4.2-dev
August 2, 2026 18:06
QuantumExplorer
changed the title
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/rs-platform-wallet/src/manager/mod.rs (1)
87-106: 🩺 Stability & Availability | 🔵 TrivialConsider the aggregate teardown ceiling.
Each phase is bounded, which is the goal of this change. The phases run in sequence, so the worst-case
shutdownduration is the sum of the phase ceilings: SPV stop (15 s) plus run-loop join (15 s) plus abort grace (2 s), payment drain (10 s) plus abort grace (1 s), the concurrent coordinator drain (10 s), the registry join (DEFAULT_JOIN_BUDGET= 30 s, concurrent across workers) plus the reap backstop, and the adapter join (10 s). That is roughly 90 s before the FFIdestroyreturns.The registry join budget of 30 s is the largest single term, and it applies after the drain already proved every pass released
is_syncing. A loop thread that has drained should exit in milliseconds. Consider setting a shorterjoin_budgetincoordinator_worker_configso a genuinely wedged thread surfaces asTimeoutsooner. A wedged thread is re-parked either way, so a shorter budget does not lose safety.Also consider documenting the worst-case ceiling on
PlatformWalletManager::shutdownso hosts can size their own teardown watchdog.🤖 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 87 - 106, The aggregate shutdown sequence can approach 90 seconds because registry joining uses the 30-second default budget. Update coordinator_worker_config to use a shorter explicit join_budget, preserving timeout re-parking behavior, and document the resulting worst-case ceiling in PlatformWalletManager::shutdown so callers can size teardown watchdogs.packages/rs-platform-wallet/src/manager/identity_sync.rs (1)
461-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a shared trait for the four identical quiesce wrappers.
quiesce,quiesce_within,quiesce_held_within, andquiesce_sealed_withinare byte-identical acrossidentity_sync.rs,platform_address_sync.rs,dashpay_sync.rs, andshielded_sync.rs, including the doc comments and the#[must_use]attributes.drain_passalready factors out the mechanism, so only the wrappers and thequiescing/is_syncingfield access differ.A small crate-internal trait would collapse the four copies into one implementation:
pub(crate) trait Quiescable { fn gate(&self) -> &QuiesceGate; fn sync_slot(&self) -> &std::sync::atomic::AtomicBool; fn cancel_loop(&self); async fn quiesce_held_within(&self, budget: Duration) -> Option<QuiesceGuard<'_>> { drain_pass(self.gate(), self.sync_slot(), || self.cancel_loop(), budget).await } // ... the other three as provided methods }Each coordinator then implements three one-line accessors. This is a follow-up, not a blocker: the current duplication is correct, and four copies means four places to edit if the drain contract changes again.
🤖 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 461 - 518, Introduce a crate-internal Quiescable trait to centralize the shared quiesce, quiesce_within, quiesce_held_within, and quiesce_sealed_within implementations and their existing documentation and must_use attributes. Define accessors for the coordinator’s QuiesceGate and sync AtomicBool plus a cancel_loop method, implement the trait with three small accessors for each coordinator, and preserve the current drain, timeout, gate-reopening, and sealing behavior.packages/rs-dash-async/src/registry.rs (3)
253-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale
start_taskreferences.The PR removed the task-based worker API, but the docs still describe it.
start_taskis referenced here and in several other doc comments (start_threaddocs,hold_clearingdocs,is_clearingdocs), yet nostart_taskmethod exists in this module. A reader will look for an API that is not there.The test module also carries two stale section headers with no tests under them:
// ----- Group 4: DrainHook ordering ---(Line 1333) and// ----- Group: register_thread (join/status-only, token-less) ---(Line 1818).Replace the
start_taskmentions withstart_thread, and drop the empty group headers.🤖 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/registry.rs` around lines 253 - 270, Remove all stale start_task references from the registry documentation, including the comments for closing, start_thread, hold_clearing, and is_clearing, replacing them with start_thread where appropriate. In the test module, delete the empty section headers “Group 4: DrainHook ordering” and “Group: register_thread (join/status-only, token-less)” without changing surrounding tests.
278-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the lock-order note to include this
Debugimpl.
Debug::fmtholds all three guards at once. Eachlock_*()temporary lives until the end of thedebug_struct(...).finish()statement, soslots,orphans, andclearingare held simultaneously. The order isslots->orphans->clearing, which matchespark_prior_lockedandstart_thread, so there is no cycle and no deadlock.The module comment on
park_prior_locked(Lines 757-760) states that theslots->orphansnesting there "is the only such nesting in this module". That statement is now inaccurate. Adjust the comment, or snapshot the values before building thedebug_structsoDebugholds one lock at a time.♻️ Optional: snapshot before formatting
impl<K: RegistryKey> std::fmt::Debug for ThreadRegistry<K> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let live_slots = self.lock_slots().len(); + let orphans = self.lock_orphans().len(); + let clearing = self.lock_clearing().len(); f.debug_struct("ThreadRegistry") - .field("live_slots", &self.lock_slots().len()) - .field("orphans", &self.lock_orphans().len()) + .field("live_slots", &live_slots) + .field("orphans", &orphans) .field("reap_backstop", &self.reap_backstop) .field("closing", &self.closing.load(Ordering::Acquire)) - .field("clearing", &self.lock_clearing().len()) + .field("clearing", &clearing) .finish() } }🤖 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/registry.rs` around lines 278 - 288, Update the lock-order documentation near park_prior_locked to acknowledge the Debug implementation’s simultaneous slots → orphans → clearing lock nesting, or snapshot those values before constructing the debug output so Debug::fmt holds only one guard at a time; preserve the existing lock order and avoid claiming park_prior_locked is the module’s only nested locking site.
348-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the backstop reference in the doc comment.
The text names
WorkerConfig::reap_backstop.WorkerConfighas no such field. The reap backstop is a registry-level value set byThreadRegistry::with_reap_backstopand defaulted byDEFAULT_REAP_BACKSTOP. A reader who follows this doc will look for a per-worker knob that does not exist.📝 Proposed doc fix
/// **Blocks the calling thread on restart-reap**: when restarting a /// key whose prior OS thread is still finishing, this call SPINS - /// SYNCHRONOUSLY for up to `WorkerConfig::reap_backstop` (default - /// `DEFAULT_REAP_BACKSTOP` = 1 s) waiting for the prior to exit. Do + /// SYNCHRONOUSLY for up to this registry's reap backstop (set by + /// [`with_reap_backstop`](Self::with_reap_backstop); default + /// [`DEFAULT_REAP_BACKSTOP`] = 1 s) waiting for the prior to exit. Do /// not call it directly from an async context — drive it via /// `tokio::task::spawn_blocking` or a dedicated host thread.🤖 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/registry.rs` around lines 348 - 353, Update the doc comment for the restart-reap operation to reference the registry-level backstop configured through ThreadRegistry::with_reap_backstop, rather than the nonexistent WorkerConfig::reap_backstop field, while retaining the DEFAULT_REAP_BACKSTOP default.
🤖 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-ffi/src/manager.rs`:
- Around line 435-475: Update platform_wallet_manager_destroy in
packages/rs-platform-wallet-ffi/src/manager.rs:435-475 so a non-clean shutdown
returns ErrorShutdownIncomplete when either callback context has
release_fn.is_none(), while retaining the existing warning and Success result
when both contexts are owned. Add a regression test covering a borrowed context
and wedged worker. In packages/rs-unified-sdk-jni/src/wallet_manager.rs:298-317,
make no code change; its non-Success branch is corrected by the manager fix.
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 179-263: Track active drains in GateBookkeeping so release()
cannot reopen the gate while any drain_pass invocation remains in progress.
Increment the drain count before waiting, decrement it only after the wait
succeeds and the guard is acquired, and add an abandon_drain path for timeout
that decrements the count while leaving closed set. Update reopening logic to
require both holds and drainers to be zero, and add a regression test covering
an outer guard dropping during a concurrent quiesce_held wait while asserting
the gate remains closed.
In `@packages/rs-platform-wallet/src/manager/platform_address_sync.rs`:
- Around line 428-454: Update the PlatformWalletError-to-PlatformWalletFFIResult
conversion to map AddressSync admission failures from sync_wallet to a dedicated
retryable FFI code/classification instead of ErrorUnknown. Add the corresponding
Swift and Kotlin binding mappings so hosts treat both “already in flight” and
“quiescing” AddressSync messages as retry-after-idle, while preserving existing
classifications for other errors.
---
Nitpick comments:
In `@packages/rs-dash-async/src/registry.rs`:
- Around line 253-270: Remove all stale start_task references from the registry
documentation, including the comments for closing, start_thread, hold_clearing,
and is_clearing, replacing them with start_thread where appropriate. In the test
module, delete the empty section headers “Group 4: DrainHook ordering” and
“Group: register_thread (join/status-only, token-less)” without changing
surrounding tests.
- Around line 278-288: Update the lock-order documentation near
park_prior_locked to acknowledge the Debug implementation’s simultaneous slots →
orphans → clearing lock nesting, or snapshot those values before constructing
the debug output so Debug::fmt holds only one guard at a time; preserve the
existing lock order and avoid claiming park_prior_locked is the module’s only
nested locking site.
- Around line 348-353: Update the doc comment for the restart-reap operation to
reference the registry-level backstop configured through
ThreadRegistry::with_reap_backstop, rather than the nonexistent
WorkerConfig::reap_backstop field, while retaining the DEFAULT_REAP_BACKSTOP
default.
In `@packages/rs-platform-wallet/src/manager/identity_sync.rs`:
- Around line 461-518: Introduce a crate-internal Quiescable trait to centralize
the shared quiesce, quiesce_within, quiesce_held_within, and
quiesce_sealed_within implementations and their existing documentation and
must_use attributes. Define accessors for the coordinator’s QuiesceGate and sync
AtomicBool plus a cancel_loop method, implement the trait with three small
accessors for each coordinator, and preserve the current drain, timeout,
gate-reopening, and sealing behavior.
In `@packages/rs-platform-wallet/src/manager/mod.rs`:
- Around line 87-106: The aggregate shutdown sequence can approach 90 seconds
because registry joining uses the 30-second default budget. Update
coordinator_worker_config to use a shorter explicit join_budget, preserving
timeout re-parking behavior, and document the resulting worst-case ceiling in
PlatformWalletManager::shutdown so callers can size teardown watchdogs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18b62f9c-bfa7-4829-aed3-45ed55477b78
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/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/event_handler.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet-ffi/src/shielded_sync.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/loop_cancel.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/platform_address_sync.rspackages/rs-platform-wallet/src/manager/shielded_sync.rspackages/rs-platform-wallet/src/spv/runtime.rspackages/rs-platform-wallet/src/wallet/identity/network/payment_handler.rspackages/rs-unified-sdk-jni/src/events.rspackages/rs-unified-sdk-jni/src/persistence.rspackages/rs-unified-sdk-jni/src/wallet_manager.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
💤 Files with no reviewable changes (1)
- packages/rs-platform-wallet/src/manager/loop_cancel.rs
Comment on lines
428
to
+454
| pub async fn sync_wallet( | ||
| &self, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<AddressSyncResult<PlatformAddressTag, PlatformP2PKHAddress>, PlatformWalletError> | ||
| { | ||
| if self | ||
| .is_syncing | ||
| .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) | ||
| .is_err() | ||
| { | ||
| return Err(PlatformWalletError::AddressSync( | ||
| "a platform-address sync pass is already in flight; retry once it completes" | ||
| .to_string(), | ||
| )); | ||
| } | ||
| // Clears `is_syncing` on every exit path — including panic unwind. | ||
| let _slot = SyncSlotGuard(&self.is_syncing); | ||
|
|
||
| // A drain may have closed the gate between our CAS and here (see | ||
| // `sync_now`); bail so the drain can complete and its caller gets | ||
| // a true barrier. | ||
| if self.quiescing.is_closed() { | ||
| return Err(PlatformWalletError::AddressSync( | ||
| "platform-address sync is quiescing; retry once the reset / teardown completes" | ||
| .to_string(), | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace platform-address sync_wallet callers and how they classify AddressSync errors.
set -euo pipefail
echo '--- Rust callers of platform-address sync_wallet ---'
rg -nP -C 6 '\bplatform_address_sync\w*\(\)\s*\n?\s*\.\s*sync_wallet\s*\(' --type=rust || true
rg -nP -C 6 '\bsync_wallet\s*\(' --type=rust -g '!**/manager/shielded_sync.rs' || true
echo '--- FFI mapping of AddressSync ---'
rg -nP -C 6 'AddressSync' --type=rust -g 'packages/rs-platform-wallet-ffi/**'
echo '--- Host SDK callers ---'
rg -nP -C 6 'sync_wallet|syncWallet' -g '*.swift' -g '*.kt' || trueRepository: dashpay/platform
Length of output: 207
Handle the new AddressSync refusal at the FFI boundary.
PlatformAddressSyncManager::sync_wallet() returns PlatformWalletError::AddressSync when a background pass holds the slot or the manager is quiescing, and the doc says the caller should retry once sync is idle. The current From<PlatformWalletError> for PlatformWalletFFIResult falls these cases through to ErrorUnknown, so a host-initiated per-wallet sync can look like a generic failure instead of a retryable admission failure. Add a dedicated FFI code/classification for these AddressSync strings and update Swift/Kotlin bindings to treat them as retry-after-idle.
🤖 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 428 - 454, Update the PlatformWalletError-to-PlatformWalletFFIResult
conversion to map AddressSync admission failures from sync_wallet to a dedicated
retryable FFI code/classification instead of ErrorUnknown. Add the corresponding
Swift and Kotlin binding mappings so hosts treat both “already in flight” and
“quiescing” AddressSync messages as retry-after-idle, while preserving existing
classifications for other errors.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4268 +/- ##
============================================
+ Coverage 87.54% 87.56% +0.01%
============================================
Files 2670 2672 +2
Lines 338763 339758 +995
============================================
+ Hits 296583 297509 +926
- Misses 42180 42249 +69
🚀 New features to boost your workflow:
|
…borrowed callback contexts at create Two review findings, both real: **Concurrent drains could reopen admission mid-barrier.** `drain_pass` closed the gate but only became a *holder* after its final `is_syncing` observation, so a concurrent holder's drop in that window stored `closed = false` — a direct `sync_now` could claim the slot, pass the gate check, and run a pass the drain's caller (a clear/reset about to wipe state) believed impossible. The drain now takes its hold at entry, so the gate is closed continuously from the drain's first instruction to the returned guard's drop; the timeout path latches the gate closed (cleared by the next successful drain) before releasing the drain's own hold, and `closed` is recomputed from one bookkeeping state (holds/latched/sealed) under the lock rather than written ad hoc. **Borrowed contexts made a non-clean destroy unsafe again.** With `release_fn` null (the legacy contract), `destroy` returning `Success` despite a straggling worker let the host free a context that worker can still call through — the exact UAF the ownership change eliminates. Ownership is now mandatory: creation rejects a context-carrying vtable without a `release_fn` (`ErrorInvalidParameter`); `None` stays valid only alongside a null context (the no-persistence configure shape). A context needing no cleanup takes a no-op release. All in-tree hosts already comply. Tests: `concurrent_drain_keeps_gate_closed_across_another_holders_drop` (RED against the close-then-hold gate), `timed_out_drain_latches_gate_closed_until_a_successful_drain`, and `create_rejects_context_carrying_vtable_without_release_fn` (both vtables, plus the null-context shape staying valid). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Pushed Blocker 2 (gate race) — fixed structurally. Blocker 1 (borrowed contexts) — fixed by rejecting the unsound mode. Verification: 🤖 Generated with Claude Code |
QuantumExplorer
commented
Aug 2, 2026
QuantumExplorer
left a comment
QuantumExplorer
left a comment
Member
Author
There was a problem hiding this comment.
I guess this is fine.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 3, 2026
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 3, 2026
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with the `ErrorStaleReservationToken = 27` this branch carries alongside dashpay#4185. Renumber the deferred build/broadcast trio to the contiguous block 34-36, matching dashpay#4185: ErrorStaleReservationToken 27 -> 34 ErrorReservationTokenConsumed 28 -> 35 ErrorReservationWalletMismatch 30 -> 36 34-36 sits above every code claimed by a merged commit or an open PR (27 dashpay#4268 merged, 29 dashpay#4184, 31 dashpay#4183/dashpay#4259, 32 dashpay#4247/dashpay#4256, 33 dashpay#4256), so it ends the renumbering churn. 28 and 30 are vacated and return to the free pool. This branch's own `ErrorTransactionBuild` (32) and `ErrorTransactionSigning` (33) are unaffected; their numbering-rationale rustdoc is updated to name dashpay#4268 as the owner of 27 and to record where the trio went. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (no compile-time cross-ABI check — verified by grep). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 3, 2026
dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev ABI on 2026-08-02, taking the number dashpay#4185 had held. dashpay#4185 and dashpay#4256 moved the deferred-token trio to the contiguous block 34-36 in response. Registry changes: * 27 enters the merged table, owned by dashpay#4268. * The proposed table moves the trio to 34/35/36 and marks 28 and 30 free but deliberately not reissued. Next free integer is now 37. * New "Collision history" section records all three numberings of the trio (26/27/28 -> 27/28/30 -> 34/35/36) and, more usefully, corrects this file's own reasoning: on 2026-08-01 it recorded dashpay#3954's `ErrorShutdownIncomplete = 27` as a non-conforming claim that had to be withdrawn because dashpay#4185's claim was older. Seniority among open PRs does not decide an ABI number — merging does. dashpay#3954 was closed, its work landed as dashpay#4268, and 27 is now merged ABI. The trio therefore moved above every claimed number rather than into the next free gap, so nothing currently in flight can hit it again. * dashpay#3968's 27 is re-characterised: it was a proposed-vs-proposed collision, and is now a contradiction of merged ABI. Its frontier is 37+. * dashpay#4196 is now two moves behind at 26/27/28; the doc reference it owns has to chase 34, not 27. * Records a mirror gap found while grepping for this move: dashpay#4256 declares `ErrorTransactionBuild` (32) and `ErrorTransactionSigning` (33) in Rust and maps both in Kotlin, but declares neither in Swift, so both reach Swift hosts as `.errorUnknown`. Rule 5's Swift clause; left for that PR's author. * Provenance re-verified against v4.2-dev `5d68612a45`, including the check that 32 and 33 were already taken — which is why the trio went to 34-36 and not 32-34. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human-generated TL;DR
Supersedes #3954 (same underlying work by @lklimek / Claudius, carried
forward here). dash-evo-tool crashes on shutdown and the mobile hosts
could UAF their persistence/event callbacks, because the four background
sync coordinators ran
!Sendloops on detached OS threads that nothingjoined or owned at teardown.
Issue being fixed or feature implemented
Three distinct problems, fixed by construction rather than by defensive
checking:
Handle::block_onon the host's tokio runtime; at exit a detachedloop mid-pass touches
tokio::timeon a dropped runtime. Nothingjoined those threads.
were borrowed (
Unmanaged.passUnretained/ caller-ownedGlobalRefboxes): Rust-side
Arcclones of the persister/event wrappers couldoutlive
destroyand fire callbacks into memory only the hostcontrolled.
clear_shieldedand the platform-address resetmutate state a concurrently-running sync pass also touches (commitment
tree, watermarks, balances); stop was cancel-only and couldn't prevent
a pass from re-persisting what was just wiped.
What was done?
dash-async:ThreadRegistry<K>(from #3954, slimmed). Owns thecoordinator OS-thread lifecycle end to end: the closing/clearing-latch
check, the spawn, and the cancellation-token install happen under one
slot lock (no check-then-spawn gap); restart is generation-guarded with
the prior thread parked and bounded-joined, never detached;
shutdown()cancels + joins everything concurrently and returns aShutdownReportso a wedged thread is reported (Timeout/Detached)instead of silently detached — the signal a runtime-owning host needs
before dropping its runtime.
WorkerConfig::stack_sizecovers DashPay'sdeep GroveDB proof descent. Deliberately absent (present in earlier
revisions of this work, deleted here as dead weight): weight-ordered
shutdown tiers,
DrainHook,start_task,register_thread,merged_with_retry,any_alive*— none had a consumer.Wallet: all four coordinators spawn through
registry.start_thread,replacing four hand-rolled copies of the same generation-guard
(
loop_cancel.rsdeleted). Every pass claims anis_syncingslot (RAII,panic-safe) and honors a shared
QuiesceGate;quiesce_held()drains anin-flight pass and keeps admission shut until its guard drops, which
clear_shielded/reset_platform_address_sync_statehold across theirwhole quiesce → wipe section — a drain that times out fails closed with
the typed
ShutdownIncompleteerror instead of wiping under a live pass.PlatformWalletManager::shutdownseals the gates (terminal), stops SPV(bounded, abort-escalating, still-live run loop re-parked), drains the
DashPay payment-hook tasks (one shared abort deadline), joins the
coordinator threads via the registry, then the event adapter — every
phase bounded, so the FFI
destroycan never hang.FFI: Rust owns the callback contexts. Both vtables
(
PersistenceCallbacks,EventHandlerCallbacks) gain a trailingrelease_fn; setting it transfers ownership ofcontextto Rust, whichreleases it exactly once, when the manager and its last worker are
provably done calling into the host (the wrappers are built once per
manager into the
Arcevery worker clones;Dropfires the release). Aworker straggling past destroy keeps the host object alive through its
own Arc and frees it on exit. Because of this,
destroyneeds no retry,no error return, and no host-side leak discipline: it runs one bounded
shutdown, logs a non-clean join, and returns
Success. Swift hands itshandlers over with
passRetained+ release trampolines (balancing theretain on the create-failure path); JNI transfers its boxed
GlobalRefcontexts the same way (jni 0.21's
GlobalRef::dropself-attaches on adetached thread). Null
release_fnkeeps the legacy borrowed contract.This also fixes a latent hazard by construction: a
PlatformWallethandle outliving manager destroy previously kept persister clones
pointing at host-freed memory.
How Has This Been Tested?
cargo test -p dash-async→ 28/28 on the slimmed registry(generation guard, orphan park/reap, spawn-failure rollback,
closing/clearing latches, custom stack size).
cargo test -p platform-wallet --features shielded --lib→ 650/650,incl. new gate tests:
quiesce_held_bars_passes_until_the_guard_drops,overlapping_quiesce_guards_reopen_only_after_the_last_drop,quiesce_sealed_never_reopens_admission,sync_wallet_is_refused_while_admission_is_shut,reset_platform_address_state_fails_closed_on_a_wedged_pass,spv_task_surviving_abort_is_returned_for_reparking.cargo test -p platform-wallet-ffi --lib→ 207/207 (both featureconfigs), incl.
release_fires_once_when_the_last_arc_clone_drops(persister + event variants) and
destroy_releases_owned_callback_contexts_exactly_once(end to endthrough the public FFI, stale handle cannot double-release); the vtable
layout pin proves
release_fnis the terminal field.cargo check --all-targetsclean forrs-unified-sdk-jni; clippyclean on every touched file;
cargo fmt --checkclean;RUSTDOCFLAGS="-D warnings" cargo doc -p dash-async --no-depsclean.identical Swift-side pattern passed the Swift SDK job on the
predecessor branch, run 30757672140).
Breaking Changes
registry: Arc<ThreadRegistry<WalletWorker>>parameter.PlatformWalletManager::shutdown(&self)return type:()→ShutdownReport<WalletWorker>.dash_async::WorkerConfigis now{ join_budget, stack_size }.PersistenceCallbacks/EventHandlerCallbacks(#[repr(C)]) gain atrailing
release_fnfield; in-tree Swift/Kotlin consumers areregenerated in lockstep.
PlatformWalletFFIResultCode::ErrorShutdownIncomplete = 27, returnedby the clear/reset/sync-stop drain barriers (NOT by
destroy, whichalways returns
Successfor a live handle).Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features