feat(platform-wallet)!: shared ThreadRegistry for coordinator lifecycle + shutdown UAF/data-loss fixes

[Claudius-Maginificent]

Why this PR exists

Problem. The SPV wallet's three sync coordinators (identity / platform-address / shielded) and the event-adapter each ran their own ad-hoc worker lifecycle. The coordinators must run on dedicated OS threads (std::thread + Handle::block_on) because the dash-sdk sync futures are !Send, so a tokio task group cannot host them. The teardown logic was triplicated across the three coordinators plus two parallel bookkeeping structures, and two teardown paths could leave a live coordinator thread detached while the host freed its context.

What breaks without it (concrete):

1. F1 — use-after-free on shutdown. shutdown()/quiesce() joined a coordinator by taking its JoinHandle by value into a future wrapped in a 30 s tokio::time::timeout. If the timeout fired mid-poll, the future dropped and the handle dropped with it — silently detaching a still-live OS thread without parking it. The next destroy saw background_join == None → NotRunning → reported clean, so the host freed the callback context while the detached thread could still fire into it.
2. F2 — shielded-store data-loss / UAF. clear_shielded() wiped the shielded store gated only on the current coordinator quiesce being clean. A prior-generation shielded thread parked after a stop→start race still held Arc refs to the persister being wiped, so the clear could run under a live writer.

Blocking relationship. Builds on the in-branch shutdown-join work. Introduces a generic engine in the shared dash-async crate that rs-dapi could adopt later (no rs-dapi change here).

What was done?

• New ThreadRegistry<K> in dash-async — one generic engine managing both !Send OS-thread workers and Send tokio tasks behind a uniform API: a generation-match epilogue, reap-or-park orphan tracking, weight-ordered graceful shutdown(), and per-worker async drain hooks. The is_syncing/quiescing domain barrier stays in each coordinator and is handed to the registry as a DrainHook — the registry owns the lifecycle machinery, not the domain semantics.
• Migrated the three coordinators + the event-adapter onto the registry; deleted the triplicated lifecycle code and both parallel structures (background_* fields, the CoordinatorOrphans type); extracted a CoordinatorLifecycle helper to dedup the remaining shared is_syncing/quiescing protocol — and after the latest review pass, the triplicated coordinator start() poll-loop body itself was hoisted into CoordinatorLifecycle::spawn_periodic_loop (each coordinator now provides only its domain-specific pass thunk).
• F1 fixed structurally — joins keep the JoinHandle owned by the registry slot (never moved into a cancellable future) and deterministically re-park on timeout; F2 fixed — a single key-scoped any_alive_for() gate over live + parked workers blocks the store wipe.
• Lifecycle hardening (from three internal review rounds): generation-guarded quiesce, graceful thread-spawn-failure re-park with full slot-config rollback, panic-safe epilogue, a terminal closing latch, a restarted worker's prior parked under the slot lock, clear_shielded's drain bounded by a timeout (no host ANR) and its quiescing gate held continuously across drain → liveness-check → wipe, and the quiescing↔is_syncing handshake made self-fencing (SeqCst).
• Cancel-only shielded_sync_stop FFI — the bridge no longer raises its own AtomicFlagGuard on quiescing, eliminating a race where a concurrent stop's guard Drop could lower the gate clear_shielded holds continuously. The single join point for shielded teardown is now destroy (or shielded_clear for the clear flow); shielded_sync_stop matches the cancel-only convention of identity_sync_stop and platform_address_sync_stop.
• Per-key clearing latch — ThreadRegistry::hold_clearing(key) -> ClearingGuard RAII latch refuses start_thread/start_task for any key being cleared; wired into clear_shielded_inner alongside the quiescing gate. Closes the residual where a full shielded_sync_start() racing clear_shielded could otherwise reopen the gate and re-persist into the just-wiped store.
• Event-adapter cancel-drain — wallet_event_adapter_loop now drains the broadcast::Receiver via bounded try_recv() on cancel before exiting, so an event emitted just before stop (notably TransactionInstantLocked, which P2P does not replay) is still persisted. Bound (CANCEL_DRAIN_BUDGET = 4096) counts every attempt — including Lagged — toward the cap so a flood cannot stall teardown.
• Type collapse — the wallet-side CoordinatorThreadStatus / CoordinatorExitStatus wrappers were collapsed onto dash_async::WorkerStatus / ShutdownReport<WalletWorker> directly (both were introduced in this PR, never released — no released-API break). shutdown() now returns ShutdownReport<WalletWorker>; consumers re-export via platform_wallet::{ShutdownReport, WorkerStatus}. A never-started worker is encoded as key-absent-from-per_worker rather than a materialized NotRunning slot; the all_clean() verdict is unchanged.
• #[must_use] on ShutdownReport and CoordinatorExitStatus so silently dropping the UAF-avoidance status now warns at compile time.

How Has This Been Tested?

• platform-wallet lib: 212 (default) / 313 (--features shielded) passing — at/above baseline, no coverage regression. Net +3 over the baseline includes new T5/T11/cancel_all regressions.
• dash-async: 35 unit tests (+1 doctest), including the F1/F2 / cancel-drain bound / per-key clearing latch (3 latch tests) regressions. The F1/F2 + lifecycle fixes are proven non-vacuous (reverting each fix makes its regression test fail, then restored).
• platform-wallet-ffi: 108 (--features shielded) passing.
• cargo clippy --workspace --features shielded --all-targets -- -D warnings clean; cargo fmt --all --check clean.
• Validated by a functional/spec QA pass and three full three-lens code-review rounds (security/concurrency, Rust quality, project consistency); the latest round adversarially re-verified the four critical invariants (F1, F2, SEC-001, SEC-002) intact after the cancel-only stop + per-key latch — concurrent stop can no longer lower a continuously-held clear gate, and a concurrent (re)start for the shielded key is no-op'd by the registry for the whole clear flow.
• All review threads from the two earlier rounds (T1–T11) addressed; their landing commits are referenced inline in the resolved threads.

Breaking Changes

• PlatformWalletManager::shutdown() returns dash_async::ShutdownReport<WalletWorker> (was ()); FFI destroy maps !all_clean() → ErrorShutdownIncomplete — hence the ! in the title. A never-started worker is encoded as absent-from-per_worker rather than NotRunning in a materialized field; all_clean() is unchanged.
• The wallet-side wrapper types CoordinatorThreadStatus / CoordinatorExitStatus (both introduced in this PR, never released) were collapsed onto the dash_async equivalents; consumers re-export via platform_wallet::{ShutdownReport, WorkerStatus}.
• New additive public API in dash-async (ThreadRegistry, WorkerStatus, WorkerConfig, ShutdownWeight, ClearingGuard, ShutdownReport, …) and a new dev-only dash-async test-util feature.
• FFI shielded_sync_stop is now cancel-only (does not return ErrorShutdownIncomplete); the join + orphan-liveness contract for hosts moves to destroy and shielded_clear. Swift PlatformWalletManager.deinit updated accordingly.
• No breaking change to the remaining platform-wallet public/FFI surface.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section
• I have made corresponding changes to the documentation if needed

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[thepastaclaw]

Code Review

All three prior findings (911f99f) are resolved at 748c4f8: CoordinatorLifecycle::quiesce now self-raises the quiescing gate and drains in-flight direct passes (coordinator_lifecycle.rs:166-173, with a regression test); the shielded_sync_stop FFI now consults manager.shielded_worker_alive() and returns ErrorShutdownIncomplete when a prior-generation shielded orphan is parked (shielded_sync.rs:127-157); and start_thread now parks the prior worker under the slot lock via park_prior_locked before the out-of-lock reap, with a regression test in the registry. One in-scope cumulative concern remains: the shared quiescing AtomicBool means a concurrent quiesce()'s AtomicFlagGuard drop can lower the gate that clear_shielded's hold_quiescing_gate is supposed to keep raised continuously — the mechanism does not match the doc's continuous-hold promise in clear_shielded_inner. In practice this is gated by the host's UI-thread serialization, so it is a suggestion, not a blocker.

🟡 1 suggestion(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/src/manager/coordinator_lifecycle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:166-173: clear_shielded's "continuously held" gate can be lowered by a concurrent quiesce()
  `clear_shielded_inner` holds `_clearing_gate = shielded_sync_manager.hold_quiescing_gate()` across drain → liveness check → wipe, expecting the shielded `quiescing` boolean to stay `true` for the whole sequence (and the doc at manager/mod.rs:517-520 explicitly promises "the quiescing gate is raised continuously" so concurrent direct `sync_now`/`sync_wallet` are held off). But the same `AtomicBool` backs `CoordinatorLifecycle::quiesce`, which builds its own `AtomicFlagGuard` (coordinator_lifecycle.rs:171); `AtomicFlagGuard::drop` unconditionally stores `false` (rs-dash-async/src/atomic.rs:25-29). If `platform_wallet_manager_shielded_sync_stop` is invoked concurrently with `clear_shielded` (the FFI handle store only takes a brief read lock in `with_item`, so two calls on the same handle can interleave), the stop's `quiesce()` returns and its dropping guard clears the flag while clear is still between its drain and its wipe. A direct `sync_now`/`sync_wallet` then sees `quiescing=false` in `begin_pass`, claims `is_syncing`, and can re-persist into the store `coord.clear()` is about to wipe — a store/host desync. The host's UI-thread serialization mitigates this today, but the documented invariant on `clear_shielded` only excludes concurrent `shielded_sync_start`, not `_stop`. Fix options: refcount the gate (so the inner drop doesn't lower it under the outer hold), use a clear-specific second flag that `begin_pass` also consults, or tighten the host-serialization doc to enumerate `_stop` too.

[lklimek]

why not just impl Default?

[Claudius-Maginificent]

Addressed in 0e73de7: fn dormant() replaced with a manual impl Default for SlotState. Manual (not derived) — #[derive(Default)] would zero join_budget, but the dormant slot must carry DEFAULT_JOIN_BUDGET = 30s. Both call sites switched to or_default().

(Reply-only — leaving for you to resolve.)

[lklimek]

Same code in both cases. DRY.

[Claudius-Maginificent]

Addressed in 0e73de7: extracted the duplicated 8-line slot-rotation block into SlotState::prepare(cfg) -> (Option<WorkerHandle>, CancellationToken, u64). start_thread keeps its rollback-snapshot lines (captured BEFORE calling prepare) so a spawn failure can restore the prior config; start_task just calls prepare.

(Reply-only — leaving for you to resolve.)

[lklimek]

If we don't have a timeout here, at least measure time and tracing::debug! it if operation takes more than expected

[Claudius-Maginificent]

Addressed in 0e73de7: drain hook is now timed via Instant::elapsed() with DEBUG/WARN logging in quiesce. WARN fires above DRAIN_HOOK_WARN_THRESHOLD = 10s (1/3 of DEFAULT_JOIN_BUDGET). No hard timeout — the caller still bounds the whole teardown; this is observability for a hung drain.

(Reply-only — leaving for you to resolve.)

[lklimek]

why not use slot_alive() ?

[Claudius-Maginificent]

Addressed in 0e73de7: added an inline comment in quiesce explaining why s.cancel.is_some() || s.handle.is_some() is used rather than slot_alive() — a finished-but-unreaped handle still needs classifying into its terminal status; slot_alive() treats handle.is_finished() as not-alive and would short-circuit to NotRunning, dropping the result.

(Reply-only — leaving for you to resolve.)

[lklimek]

can we detect panic mode and log a warning if it's wrong?

[thepastaclaw]

Yes. I added a small follow-up on thepastaclaw:tracker-1583-panic-mode-warning that detects the build panic strategy with cfg(panic = "abort") and emits a one-shot tracing::warn! from ThreadRegistry::with_reap_backstop when the crate is built with abort semantics. It also updates the nearby EpilogueGuard docs and adds a focused idempotence/cfg regression test.

Commit: thepastaclaw@7f353680000c

Validation: cargo test -p dash-async --lib registry::, cargo check -p dash-async --tests, cargo clippy -p dash-async --tests -- -D warnings, cargo fmt --all --check.

[Claudius-Maginificent]

The panic-strategy caveat is the correct handling: stable Rust has no runtime API to query the active panic strategy — only #[cfg(panic = "...")] is available, and iOS release intentionally uses panic = "abort", so a hard compile_error! gate would be wrong for that target. The doc note stays as a known limitation. No code change.

(Reply-only — leaving open for you to resolve after review.)

[thepastaclaw]

Code Review

Incremental review of 748c4f8..6ed5200. Latest delta adds the server-advertised rate-limit ban path in rs-dapi-client + dashmate gateway wiring, simplifies platform_wallet_manager_shielded_sync_stop to cancel-only, drains buffered wallet events on cancel in event-adapter (T7), and threads orphan_status through ShutdownReport. No new blocking defects found. The prior architectural finding around clear_shielded's continuously-held gate vs. CoordinatorLifecycle::quiesce on the same AtomicBool is STILL VALID but now latent: ShieldedSyncManager::quiesce remains pub on the same atomic, however no FFI export or in-tree Rust caller races it against clear_shielded (verified). Carried forward as suggestion only. Model coverage degraded: Codex (general/rust-quality/ffi-engineer) did NOT run this round due to ACP/native auth stall; review is Claude-only.

Model coverage note: Claude general, Claude rust-quality, and Claude ffi-engineer ran. Codex general/rust-quality/ffi-engineer did not run because ACP-Codex authentication stalled during the authenticate handshake and native Codex CLI returned 401/missing bearer; this review is therefore degraded from the usual Claude+Codex matrix.

🟡 1 suggestion(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/src/manager/coordinator_lifecycle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:166-249: Public ShieldedSyncManager::quiesce can still lower clear_shielded's continuously-held gate (carry-forward of prior 748c4f82-1)
  `clear_shielded_inner` (manager/mod.rs:548) holds `_clearing_gate = shielded_sync_manager.hold_quiescing_gate()` across the entire drain → liveness-check → store-wipe sequence, expecting the shielded `quiescing` boolean to stay raised so no direct `sync_now`/`sync_wallet` slips past and re-persists into the store being wiped (mod.rs docs 515-524). The hold guard at coordinator_lifecycle.rs:243-249 and the RAII guard inside `CoordinatorLifecycle::quiesce` at line 171 both wrap the SAME `Arc<AtomicBool>`, and `AtomicFlagGuard::drop` unconditionally `store(false, Release)`s. A concurrent invocation of `ShieldedSyncManager::quiesce` (which is still `pub` at shielded_sync.rs:280) would lower the gate mid-Clear, opening exactly the window the docstring promises is closed. 

  Reachability on this head: verified there is no FFI export that triggers this — `platform_wallet_manager_shielded_sync_stop` is cancel-only in this delta, `destroy` goes through the registry shutdown path whose drain hook only stores `true` with no RAII guard (lines 121-131), and `shielded_clear` uses `quiesce_under_held_gate`. The only in-tree caller of `shielded_sync().quiesce()` is the internal test at coordinator_lifecycle.rs:346. So the cross-boundary exploit path is closed by surface; the residual is a fragile in-process Rust contract that future callers can break silently. The module docs at mod.rs:515-524 already acknowledge this as a follow-up (per-key clearing latch / refcount on the registry). Suggested cheap structural fixes: narrow `ShieldedSyncManager::quiesce` to `pub(crate)` (no FFI consumer today), refcount the gate, or force every caller through `quiesce_under_held_gate` after `hold_quiescing_gate`. Non-blocking — flagged so the latent fragility does not regrow once new shielded entry points are added.

[thepastaclaw]

Code Review

Two blocking issues found at HEAD 284752a. (1) ShieldedSyncManager::start()'s spawn_periodic_loop calls reopen_quiescing_gate() before start_thread's clearing-latch check — a racing FFI shielded_sync_start during clear_shielded silently lowers the clear's continuously-held quiescing gate, partially defeating the new latch. (2) The Swift ShieldedService.clearLocalState catches clearShielded()'s ShieldedShutdownIncomplete error and continues to wipe SwiftData, violating the explicit PlatformWalletResult contract ("must NOT commit its own persistence wipe — the Rust store was left intact"). Prior carry-forward finding (public ShieldedSyncManager::quiesce can lower the held gate) is STILL VALID as a latent suggestion. Several lower-severity issues around the ClearingGuard's set-membership semantics and its test coverage are kept as suggestions/nitpicks.

🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

1 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/rs-platform-wallet/src/manager/coordinator_lifecycle.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:131-161: Refused shielded (re)start still lowers clear_shielded's continuously-held quiescing gate
  `spawn_periodic_loop` calls `self.reopen_quiescing_gate()` at line 137 (which `store(false, Release)`s `quiescing`) before invoking `registry.start_thread(...)` at line 142. The new per-key clearing latch correctly refuses the start inside `start_thread` (registry.rs:414-416), but by then the gate has already been lowered. `clear_shielded_inner` (manager/mod.rs:390-415) holds the gate up via `hold_quiescing_gate()` precisely to keep direct `sync_now`/`sync_wallet` callers blocked across drain → liveness-check → wipe, and a single shared `AtomicFlagGuard` always clears the flag on drop — the clear path has no way to re-raise it. So an FFI `platform_wallet_manager_shielded_sync_start` (exported at rs-platform-wallet-ffi/src/shielded_sync.rs:57) landing during a clear will: (a) call `ShieldedSyncManager::start()` → `spawn_periodic_loop` → `reopen_quiescing_gate()` setting `quiescing=false`, (b) be refused by the clearing latch at the registry, (c) leave the gate down for the remainder of the clear. A direct `sync_now`/`sync_wallet` going through `begin_pass` then observes `quiescing=false` and slips past the barrier, re-persisting into the store the clear is about to wipe — the exact race the new latch was added to close. Fix by checking the clearing latch BEFORE reopening the gate (e.g. early-return from `spawn_periodic_loop` if `registry.is_clearing(worker)`), or ordering the gate reopen so it only fires when the registry actually accepts the start (e.g. inside the closure passed to `start_thread`, or only after a non-rejecting outcome). The same hazard exists structurally for the identity-sync and platform-address coordinators if they ever gain a clearing latch.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:608-628: Swift clear flow wipes SwiftData rows after Rust reports the shielded store was left intact
  `platform_wallet_manager_shielded_clear` maps `PlatformWalletError::ShieldedShutdownIncomplete` to `ErrorShutdownIncomplete` and returns BEFORE touching the Rust shielded store (rs-platform-wallet-ffi/src/shielded_sync.rs:446-461). The Swift error contract states this explicitly: `PlatformWalletResult.swift:188-193` says on `shutdownIncomplete` "the host ... must NOT commit its own persistence wipe — the Rust store was left intact so it can be retried once the pass settles." But `ShieldedService.clearLocalState` catches the thrown error, logs it via `SDKLogger.error`, and falls through into `modelContext.delete(model: PersistentShieldedNote/OutgoingNote/SyncState/Activity)` at lines 624-627. If Rust refused the clear because a coordinator didn't drain in time, SwiftData rows are deleted while the Rust commitment tree remains populated — the exact cross-language desync the new FFI result code was added to prevent. The legacy `// Best-effort — failure logs but doesn't abort the wipe` comment at lines 606-607 predates this contract and is now wrong. The catch must distinguish `.shutdownIncomplete` from other errors and return early without wiping host rows.

In `packages/rs-dash-async/src/registry.rs`:
- [SUGGESTION] packages/rs-dash-async/src/registry.rs:596-613: ClearingGuard uses set-membership semantics — nested or overlapping guards release the latch on the first drop
  `hold_clearing` inserts the key into a `BTreeSet<K>` and `ClearingGuard::drop` removes it unconditionally. With two live `ClearingGuard`s for the same key (nested call by future code, OR two concurrent FFI `platform_wallet_manager_shielded_clear` invocations on the same handle — `HandleStorage::with_item` only holds a read lock), the first guard to drop removes the set entry and the surviving guard's protection lapses silently. Today only `clear_shielded_inner` takes the latch and the test `hold_clearing_nested_guards_share_the_latch` (registry.rs:1857) documents the quirk via comment — but the public `pub fn hold_clearing` signature gives no hint to callers, and a concurrent host-thread clear is reachable through the FFI surface. Convert the backing store to `BTreeMap<K, NonZeroUsize>` so guards refcount/compose, or serialize `clear_shielded` per key at the manager boundary so re-entrant clears can't overlap.

[thepastaclaw]

Code Review

Latest delta (97e373b) only adds nonisolated(unsafe) to two Swift handler properties for Swift 6 strict-concurrency compliance — that change is narrow and sound. All five prior findings remain unaddressed at HEAD, including two blocking issues: (1) spawn_periodic_loop unconditionally lowers the shared quiescing gate before the per-key clearing latch can refuse the (re)start, opening a window where a direct sync_now/sync_wallet can persist into the store being cleared, and (2) Swift ShieldedService.clearLocalState wipes SwiftData rows even when Rust returns ErrorShutdownIncomplete — the comment now explicitly codifies 'best-effort, failure logs but doesn't abort the wipe', which is the opposite of the documented FFI contract.

🔴 2 blocking | 🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

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/rs-platform-wallet/src/manager/coordinator_lifecycle.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:131-161: [prior-1, carried forward] `spawn_periodic_loop` lowers the shared quiescing gate before the per-key clearing latch can refuse the (re)start
  `spawn_periodic_loop` calls `self.reopen_quiescing_gate()` at line 137 — an unconditional `quiescing.store(false, Release)` on the `Arc<AtomicBool>` — BEFORE handing off to `registry.start_thread(...)` at line 142, where the per-key `hold_clearing(ShieldedSync)` latch refuses the start. During `clear_shielded_inner`, the clear flow continuously holds both the registry clearing latch and the quiescing gate (via `hold_quiescing_gate`, which is a non-restoring RAII guard). Race: (T1) clear holds latch + gate=true; (T2) host calls `shielded_sync_start()` → `spawn_periodic_loop` → `reopen_quiescing_gate` writes `false` → `start_thread` is refused by the latch, but the gate is already down; (T3) a direct `sync_now`/`sync_wallet` enters `begin_pass`, observes `quiescing == false`, takes the `is_syncing` slot, and runs to completion — persisting into the shielded store inside the clear-then-wipe window. The `quiesce_under_held_gate` debug_assert at line 219-222 catches the inconsistency only in debug builds; release builds proceed silently. Fix: only reopen the gate once `start_thread` confirms acceptance — either consult `lock_clearing` in `spawn_periodic_loop` and skip the reopen on refusal, move the reopen inside the registry's slot critical section after the latch check, or use the registry's accept signal to gate the store.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:196-203: [prior-3, carried forward] Public `quiesce()` drops an `AtomicFlagGuard` on the same shared atomic that `clear_shielded` holds raised
  `CoordinatorLifecycle::quiesce` constructs `_quiescing_gate = AtomicFlagGuard::new(&self.quiescing)` over the same `Arc<AtomicBool>` that `clear_shielded_inner` holds raised via `hold_quiescing_gate`. `AtomicFlagGuard::Drop` unconditionally writes `false`, so any concurrent caller routed through the public `quiesce()` while a clear is in flight will lower the clear-flow's continuously-held gate. Today the wallet's `shutdown` routes through `registry.shutdown()` rather than this method, but `ShieldedSyncManager::quiesce` remains `pub` and reachable via the `shielded_sync_arc` accessor — the trap is one accidental call away from re-opening the same lapse Clear was hardened to prevent. The structural fix is to give the gate a refcount/holder-aware re-raise (so concurrent owners compose), or to gate the public `quiesce` against the clearing latch and assert no other holder is active.

In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:606-628: [prior-2, carried forward] `clearLocalState` wipes SwiftData rows even when `clearShielded()` throws `ErrorShutdownIncomplete` — violates the documented FFI contract
  `PlatformWalletFFIResultCode::ErrorShutdownIncomplete` is the explicit signal that the Rust shielded store was left intact: the FFI doc in `packages/rs-platform-wallet-ffi/src/error.rs` states 'the manager is NOT torn down and the store was left intact (Clear aborted before touching it). The host may retry the clear, and must not commit its own persistence wipe — doing so would desync the host's rows from the still-populated shared tree.' The current implementation at lines 608-616 catches the thrown error, logs it via `SDKLogger.error`, then falls through to step 2 (lines 623-628), unconditionally deleting `PersistentShieldedNote`, `PersistentShieldedOutgoingNote`, `PersistentShieldedSyncState`, and `PersistentShieldedActivity`. The comment at line 606-607 even codifies this: 'Best-effort — failure logs but doesn't abort the wipe.' That is the exact split-brain the FFI contract was designed to prevent: when Rust correctly preserves notes (orphan-alive backstop, drain timeout, non-clean drain status), Swift discards its mirror, and the next `bindShielded` + sync repopulates host rows from the surviving Rust state — the user-visible 'Clear' becomes a no-op on the Rust side while host data is lost. Minimum fix: inspect the thrown error code and short-circuit the SwiftData wipe (surfacing the error to the user / scheduler) on `errorShutdownIncomplete`.

[thepastaclaw]

Code Review

Carried forward from prior review (still valid at bcf9388): prior-2 (blocking — Swift clearLocalState still wipes SwiftData on ErrorShutdownIncomplete), prior-3 (suggestion — public quiesce() drops AtomicFlagGuard on the shared atomic), prior-4 (suggestion — ClearingGuard uses set membership), prior-5 (nitpick — nested-guards test doesn't exercise the documented lapse). prior-1 is partially addressed: the new is_clearing short-circuit + SEC-002 regression test close the common race, but the check and the gate-write are still two distinct critical sections, leaving a narrow residual TOCTOU window — carried forward as a suggestion. No new defects identified in the latest delta (rs-platform-wallet-storage / secrets / keyless rehydration look careful: fail-closed topology check, transactional rollback, per-row skip taxonomy, structured FFI outcome).

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:608-616: [prior-2, carried forward] `clearLocalState` wipes SwiftData rows even when `clearShielded()` throws `ErrorShutdownIncomplete` — violates the documented FFI contract
  The Rust FFI contract for `ErrorShutdownIncomplete` on `platform_wallet_manager_shielded_clear` (rs-platform-wallet-ffi/src/error.rs around the `ShieldedShutdownIncomplete → ErrorShutdownIncomplete` mapping, plus shielded_sync.rs:454) is explicit: when the manager surfaces this code, the Rust shielded store was left intact because Clear aborted before touching it (drain non-clean / orphan-alive backstop). The host must retry the clear and must NOT commit its own persistence wipe; doing so desyncs host rows from the still-populated shared tree. ShieldedService.swift:608-616 catches the throw, logs via `SDKLogger.error`, then falls through unconditionally into the SwiftData delete block at 623-628 (`PersistentShieldedNote`, `PersistentShieldedOutgoingNote`, `PersistentShieldedSyncState`, `PersistentShieldedActivity`). The comment at 606-607 ("Best-effort — failure logs but doesn't abort the wipe") codifies the exact split-brain the FFI contract was designed to prevent. The Swift surface already distinguishes this code: `PlatformWalletResultCode.errorShutdownIncomplete` exists (PlatformWalletResult.swift:47) and decodes into `PlatformWalletError.shutdownIncomplete(detail)` (line 240), so the host can branch on it. Minimum fix: on the typed `shutdownIncomplete` case (or any thrown error from `clearShielded()`, since the Rust contract is fail-closed there), short-circuit the SwiftData wipe and surface the error to the user instead of proceeding.

In `packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:137-148: [prior-1, partial fix landed] Residual TOCTOU between `is_clearing` check and `reopen_quiescing_gate`
  The new `is_clearing(self.worker)` short-circuit at line 145 closes the common race (clear is already holding the latch when spawn arrives), and the SEC-002 regression test in mod.rs:1071-1108 covers that ordering. But the check (line 145) and the gate-lower (line 148) are not done under a single critical section: `is_clearing` takes `lock_clearing`, returns the boolean, releases the lock; then `reopen_quiescing_gate` writes `false` to the shared atomic. If `clear_shielded_inner` acquires the latch AND raises the quiescing gate between those two points, the reopen at 148 lowers a gate Clear believes it holds continuously across drain → liveness → wipe. In that window a direct `sync_now`/`sync_wallet` going through `begin_pass` (lines 306-341) observes `quiescing == false`, claims the `is_syncing` slot, runs the pass to completion, and persists into the store about to be wiped. `quiesce_under_held_gate`'s `debug_assert!` (line 230) catches the inconsistency only in debug builds; release silently proceeds with the drain. The window is microseconds-narrow, but the design invariant is continuous gate-hold. Structural fix: have the registry expose an atomic observe-and-act under one `lock_clearing()` scope (e.g. `with_clearing_check`) so the gate write is sequenced with the latch read, or move the gate write inside the registry slot critical section alongside the latch check.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:207-214: [prior-3, carried forward] Public `quiesce()` drops an `AtomicFlagGuard` on the same shared atomic that `clear_shielded` holds raised
  `CoordinatorLifecycle::quiesce` constructs `_quiescing_gate = AtomicFlagGuard::new(&self.quiescing)` over the same `Arc<AtomicBool>` that `clear_shielded_inner` holds raised via `hold_quiescing_gate`. `AtomicFlagGuard::Drop` unconditionally writes `false`, so any concurrent caller routed through the public `quiesce()` while a clear is in flight will lower the clear-flow's continuously-held gate. `ShieldedSyncManager::quiesce` remains `pub` and reachable through the manager's shielded accessor — one accidental call away from re-opening the same lapse Clear was hardened to prevent. Structural fix: refcount/owner-aware re-raise (so concurrent owners compose), or gate the public `quiesce` against the clearing latch and assert no other holder is active before its guard drop is allowed to fire.

In `packages/rs-dash-async/src/registry.rs`:
- [SUGGESTION] packages/rs-dash-async/src/registry.rs:596-613: [prior-4, carried forward] `ClearingGuard` uses set-membership — overlapping guards release the latch on the first drop
  `hold_clearing` does `lock_clearing().insert(key)` into a `BTreeSet<K>`, and `ClearingGuard::Drop` (lines 1099-1103) unconditionally removes the key. With two live guards for the same key (a nested or overlapping clear from a future caller, a test, or an accidental shielded re-clear before the first guard drops), the first drop removes the key while the second guard is still alive — silently lapsing the latch protection the still-live guard is supposed to provide. The new `is_clearing()` check (line 620) inherits the same accounting weakness because it reads the same set membership. The test at lines 1859-1883 acknowledges this in its comment but does not fix the type-level mismatch between the RAII guard's compositional appearance and its set-based semantics. Minimum fix: refcount per key (`BTreeMap<K, NonZeroUsize>`), or add a `debug_assert!` that the insert was novel so accidental nesting fails loudly in debug.

Note: GitHub refused to serve this PR's diff via gh pr diff because it exceeds the 20,000-line limit, so this exact-SHA review is posted body-only. Existing inline threads are reused rather than duplicated.

[thepastaclaw]

Code Review

Latest delta resolves prior-4 and prior-5: ClearingGuard now refcounts via BTreeMap<K, NonZeroUsize> so nested/concurrent guards compose, and the new test hold_clearing_inner_drop_does_not_lapse_outer_protection is non-vacuous and asserts the lapse window. prior-2 (Swift ShieldedService wipe-on-error) remains unchanged and blocking. prior-3 is partially addressed by the new is_clearing bail in public quiesce(), but the same check-then-act TOCTOU pattern from prior-1 now exists at both spawn_periodic_loop (lines 145-148) and quiesce (lines 217-224); collapsed into a single structural finding.

🔴 1 blocking | 🟡 1 suggestion(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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:608-616: [prior-2, carried forward] clearLocalState wipes SwiftData rows even when clearShielded() throws — violates the FFI contract
  Unchanged at 6b655aab. The Rust FFI contract for `platform_wallet_manager_shielded_clear` surfaces `ErrorShutdownIncomplete` specifically when the Rust shielded store was left intact (Clear aborted before touching it via the drain/orphan-alive backstop). The host MUST NOT commit its own persistence wipe in that case, or SwiftData rows desync from the still-populated shared tree — the exact split-brain this stack is designed to prevent.

  Lines 608-616 catch the throw, log via `SDKLogger.error`, then fall through unconditionally into the SwiftData delete block at 623-628 (`PersistentShieldedNote`, `PersistentShieldedOutgoingNote`, `PersistentShieldedSyncState`, `PersistentShieldedActivity`). The comment at 606-607 ("Best-effort — failure logs but doesn't abort the wipe") codifies the wrong invariant. The Swift surface already distinguishes this code path: `PlatformWalletResultCode.errorShutdownIncomplete` decodes into `PlatformWalletError.shutdownIncomplete(detail)`, so the host can branch on it. Minimum fix: on any thrown error from `clearShielded()` (since the Rust contract is fail-closed there), short-circuit the SwiftData wipe and surface the error to the user.

In `packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/coordinator_lifecycle.rs:145-224: [prior-1 + residual prior-3] Check-then-act TOCTOU around the shared `quiescing` atomic at both `spawn_periodic_loop` and `quiesce` call sites
  Same structural pattern at two call sites — neither performs the `is_clearing` check atomically with the subsequent write to the shared `quiescing` atomic.

  1. `spawn_periodic_loop` (145-148): `is_clearing(self.worker)` at 145 takes `lock_clearing`, returns the boolean, releases the lock; `reopen_quiescing_gate()` at 148 then writes `false` to the shared atomic. If `clear_shielded_inner` acquires the clearing latch AND raises the gate between those two operations, this `reopen_quiescing_gate()` lowers a gate Clear believes it holds continuously across drain → liveness → wipe — opening a window for a direct `sync_now`/`sync_wallet` to pass `begin_pass` and persist into the store about to be wiped (even though `start_thread` itself will subsequently be refused by the latch).

  2. `quiesce` (217-224): the new fix to bail when `is_clearing` is true closes the dominant case (clear already holds the latch), but introduces the same race shape — `is_clearing` at 217 → unconditional `self.quiescing.store(true, ...)` + `AtomicFlagGuard::new(&self.quiescing)` at 223-224, whose `Drop` (`atomic.rs:25-29`) unconditionally writes `false`. If `clear_shielded_inner` acquires the latch + `hold_quiescing_gate` in the sub-microsecond window between 217 and 223, the racing quiesce installs an `AtomicFlagGuard` over the same atomic the in-flight clear is holding raised; on the quiesce's return the guard drops and lowers the gate, lapsing Clear's continuously-held barrier. The new regression test `shielded_quiesce_during_clear_preserves_quiescing_gate` only exercises the ordering where the clearing latch is acquired BEFORE the quiesce arrives.

  Structural fix (resolves both call sites): expose an atomic observe-and-act on the registry under one `lock_clearing()` scope (e.g. a `with_clearing_check(F)` callback that runs F only while the clearing-set lock is held), or move the gate write inside the registry slot's critical section alongside the latch check. Alternatively, refcount the `quiescing` atomic so concurrent owners compose like `ClearingGuard` now does post-prior-4.

Note: Posted body-only because normal inline poster failed: command failed 1: python3 scripts/review_poster.py dashpay/platform 3954 6b655aa --dry-run.