fix(platform-wallet)!: tell a retryable SPV stop from one that needs a restart

[Claudius-Maginificent]

TL;DR

When stopping wallet sync does not finish cleanly, the app now learns which of two things happened: "still stopping, try again" or "something broke, restart the app".

User story

As an app developer, I want a failed sync stop to tell me whether to retry or to restart the app, so that I can show the right action instead of a generic error.

Scenario

Actual behavior

Every incomplete sync stop returned the same generic error. The app could not tell a stop that would complete on a second try from one that never would.

Expected behavior

• Stop is still in progress: the app gets a "try again" error, and a repeated stop completes it.
• Stop or start crashed internally: the app gets a "restart required" error from that call and from every later sync start, stop or storage clear.
• After a successful stop, sync has released its storage: clearing it or starting again is safe.
• The "is sync running" query answers "no" once background sync has failed on its own.

Detailed discussion

Contract (SpvRuntime; stated once in its doc comment)

Call	Stopped	Started	Teardown still running	Startup or teardown panicked
start	starts	SpvAlreadyRunning	ShutdownIncomplete (27)	SpvProcessRestartRequired (59)
stop	Ok	Ok, or 27 after the 15 s wait, or 59 on a panic	joins it: Ok, 27 or 59	59
clear_storage	clears	stops, then clears	27, nothing cleared	59, nothing cleared

• stop returning Ok means no SPV task of this runtime runs and its storage directory is released.
• 27 is retryable: teardown is never cancelled and a later stop joins it.
• 59 is permanent for that runtime. Nothing else is promised after it: the host must restart the process. In particular this PR does not guard a second manager, or a recreated one, opening the same directory in the same process.
• is_running is true while sync runs or starts, false once background sync has failed (the client stays started until stop). It never waits, so an event callback may call it.

Implementation

• ensure_joined and stop_locked return the two typed errors instead of SpvError. The Shutdown state machine is unchanged.
• start stops the storage writer right after constructing storage. Upstream starts it at construction, restarts it in run, and stops it only for a client that ran; one stop at construction therefore covers a client that never ran, a failed startup and a failed client construction.
• is_running polls upstream's is_running once instead of awaiting it; the FFI getter platform_wallet_manager_spv_is_running now reports it.
• New FFI code ErrorSpvProcessRestartRequired = 59 (registry updated), typed in Swift and Kotlin; Kotlin also types 27. Both hosts show fixed text for 59 and keep the panic detail for logs.

Non-goals

• Recovering from a panic without a process restart.
• Stopping upstream tasks left behind by a panic (needs a rust-dashcore change).

Stacked on #5307.

Testing

• cargo test -p platform-wallet --lib spv:: — 35 passed.
• cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warnings — clean.
• Swift and Kotlin changes are verified by CI only.

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

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Repository: dashpay/platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0f994964-b604-4189-9889-68e8f22f27fc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit 744c8e2) · triage: normal

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The typed teardown errors and host mappings address the intended retry-versus-restart distinction, but four blocking issues remain: callback re-entry can abort the host, and three gaps undermine process-wide storage quarantine. Verification was static against head 4249735, including the pinned upstream lifecycle and storage implementations; no builds or tests were run. The supplied CI snapshot shows CodeRabbit successful and other listed checks pending or queued, without completed Rust or mobile build/test results.

🔴 4 blocking | 🟡 2 suggestion(s)

2 finding(s) not shown inline (the lines are not part of this PR's diff)

🔴 Blocking: Enforce directory quarantine at the shared storage-access boundary
packages/rs-platform-wallet/src/spv/runtime.rs:748-754

The new process-wide quarantine is checked only by start(), but clear_storage() also opens that directory. Runtime A can start directory D and stop cleanly, retaining last_config; runtime B can subsequently start D and panic during teardown, quarantining it; A.clear_storage() then checks only A's own shutdown state and constructs DiskStorageManager without consulting ORPHANED_STORAGE. Wherever the upstream lock no longer prevents reopening, this clears storage that the new quarantine explicitly treats as unsafe because orphaned work may still access it. Use a shared storage-admission check before constructing DiskStorageManager in both start() and clear_storage(), returning SpvProcessRestartRequired for a quarantined directory regardless of which runtime recorded the failure. Add a regression where a previously stopped runtime attempts to clear another runtime's quarantined directory.

_{source: gpt-6.1-sol (phase2-reviewer: architecture-layering)}

🟡 Suggestion: isSpvRunning docs still promise a non-parking try_read
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:233-240

The native implementation now blocks through block_on_worker and SpvRuntime::is_running(), so the statement that this is a 'try_read, never parks' is false. The caller can wait for the client read lock and then up to 100 ms for the upstream running-state query; the total call is not bounded to 100 ms. Update the wrapper documentation to describe the blocking behavior and the new distinction between a retained client and active or starting sync, so callers do not rely on this getter being suitable for frequent main-actor polling.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general)}

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change intricately modifies SPV lifecycle concurrency, storage-writer cleanup, and cross-language error reporting, but does not itself change consensus, funds movement, cryptography, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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/spv.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/spv.rs:356-359: Keep the running-state getter safe when called from a host callback
  The getter now calls block_on_worker, whose implementation unconditionally calls Runtime::block_on on the calling thread. At the pinned upstream revision, the background sync task invokes EventHandler::on_error inline; PlatformEventManager and FFIEventHandler then synchronously dispatch the host callback. A host error callback that queries platform_wallet_manager_spv_is_running therefore attempts a nested Tokio block_on and panics. Because this happens inside an extern "C" export, the process aborts rather than returning an FFI error. The previous is_started()/try_read implementation did not enter Tokio, and the public callback contract does not prohibit this status read. Preserve callback-safe querying through a nonblocking running-state snapshot or a bridge that safely handles runtime-thread callers, and add a regression that queries status from the error callback.

In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:463-468: Record quarantine failures independently of the stop caller
  Only the caller joining teardown invokes failed() and registers the directory in ORPHANED_STORAGE. If stop times out or is cancelled, teardown continues independently. Destroying the manager after a non-clean shutdown can then drop Shutdown::Running and its JoinHandle, detaching the task; the teardown closure retains the client, storage and peer tracker, but not the runtime that would observe its result. If upstream stop subsequently panics, the inner task's JoinError becomes an unobserved error result and no directory is quarantined. A replacement runtime therefore misses the promised restart-required refusal. Register failure within an owned teardown supervisor that survives the waiting caller and runtime destruction, including failure of the teardown task itself, while retaining the result for local shutdown state. Cover a delayed panic after timeout or cancellation, release the original runtime, and verify refusal from a replacement without rejoining the original teardown.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:142-145: Key the storage quarantine by filesystem identity, not the supplied path
  start() looks up the raw config.storage_path, and failed() inserts the original PathBuf. Relative versus absolute paths, paths containing resolving '..' components, and symlink aliases can therefore miss the registry while addressing the same quarantined storage. This is not reliably caught by upstream locking: at the pinned revision, DiskStorageManager::lock_file_path derives a sibling lock file from the supplied directory name, so a final-component symlink such as /data/spv-alias uses a different lock file from /data/spv while opening the same storage. A replacement runtime can consequently reopen storage still reachable by orphaned sync work. Resolve and retain a consistent directory identity for registration and admission, handle initially nonexistent directories, and fail closed if identity cannot be established safely. Add regressions for equivalent relative/absolute spellings and a symlink alias; lexical cleanup alone does not resolve the symlink bypass.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:748-754: Enforce directory quarantine at the shared storage-access boundary
  The new process-wide quarantine is checked only by start(), but clear_storage() also opens that directory. Runtime A can start directory D and stop cleanly, retaining last_config; runtime B can subsequently start D and panic during teardown, quarantining it; A.clear_storage() then checks only A's own shutdown state and constructs DiskStorageManager without consulting ORPHANED_STORAGE. Wherever the upstream lock no longer prevents reopening, this clears storage that the new quarantine explicitly treats as unsafe because orphaned work may still access it. Use a shared storage-admission check before constructing DiskStorageManager in both start() and clear_storage(), returning SpvProcessRestartRequired for a quarantined directory regardless of which runtime recorded the failure. Add a regression where a previously stopped runtime attempts to clear another runtime's quarantined directory.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:233-240: isSpvRunning docs still promise a non-parking try_read
  The native implementation now blocks through block_on_worker and SpvRuntime::is_running(), so the statement that this is a 'try_read, never parks' is false. The caller can wait for the client read lock and then up to 100 ms for the upstream running-state query; the total call is not bounded to 100 ms. Update the wrapper documentation to describe the blocking behavior and the new distinction between a retained client and active or starting sync, so callers do not rely on this getter being suitable for frequent main-actor polling.

In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:824: Record the public error enum addition as source-breaking
  PlatformWalletError is publicly re-exported and is not marked #[non_exhaustive]. Adding SpvProcessRestartRequired therefore breaks compilation of downstream exhaustive Rust matches, even though adding C error code 59 preserves ABI compatibility. The PR's Breaking Changes section describes host behavior changes but omits this Rust source-compatibility change, and the title lacks the project's breaking-change marker. Document the exhaustive-match migration and mark the title fix(platform-wallet)! accordingly. This client-only change does not require PlatformVersion activation or an independent crate-version bump.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Move storage-writer lifecycle ownership into dash-spv — Out of scope and not carried as a separate follow-up: this is the pre-existing upstream ownership problem explicitly documented in the PR, and the defensive platform-wallet cleanup is intentional. Requesting an upstream lifecycle redesign would expand the review beyond defects introduced by this change.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

🟡 Suggestion: Record the public error enum addition as source-breaking

PlatformWalletError is publicly re-exported and is not marked #[non_exhaustive]. Adding SpvProcessRestartRequired therefore breaks compilation of downstream exhaustive Rust matches, even though adding C error code 59 preserves ABI compatibility. The PR's Breaking Changes section describes host behavior changes but omits this Rust source-compatibility change, and the title lacks the project's breaking-change marker. Document the exhaustive-match migration and mark the title fix(platform-wallet)! accordingly. This client-only change does not require PlatformVersion activation or an independent crate-version bump.

_{source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)}

[thepastaclaw]

Resolved (re-reviewed at 1646f1e2): Your current title uses fix(platform-wallet)!, explicitly marking the public error-enum addition as breaking. The description also identifies the new error and lifecycle classifications, so this compatibility-disclosure finding is addressed.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The exact head preserves the retryable versus restart-required distinction across Rust and the host mappings, and fixes the previously reported nested-runtime callback panic. Two storage-quarantine defects remain: publication waits for further cleanup after a panic is observed, and missing-directory lookups can bypass a previously canonicalized quarantine entry. This was a static review; no builds or tests were run, and the supplied CI snapshot contains no completed build/test validation.

🔴 2 blocking | 🟡 2 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change is intricate and cross-language, but the SPV lifecycle, storage-writer shutdown, and error mapping changes do not alter consensus, funds movement, cryptography, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:479-496: Quarantine storage immediately when a task panic is observed
  The startup and upstream-stop join results are available before storage cleanup, but a failure is published to ORPHANED_STORAGE only after storage.lock().await.stop().await finishes and the outer observer runs. If cleanup remains pending, another runtime still passes ensure_storage_usable despite the already-observed panic. A symlink to the storage directory can then reopen it: at the pinned upstream revision, DiskStorageManager derives its sibling lock-file path from the supplied spelling, so the alias uses a different lock file. Cleanup completing also leaves a scheduling window before the observer publishes quarantine. Record a failed join immediately, before subsequent cleanup awaits, while retaining the independently owned cleanup and outer panic observer. Add a regression that holds storage cleanup pending after a stop panic and verifies that a replacement runtime using a symlink is already refused.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:56-58: Keep quarantine keys consistent after the storage directory is removed
  Quarantine recording uses the canonical path while the directory exists, but lookup falls back to an unresolved absolute path when canonicalize fails. For example, quarantine /real/root/wallet, remove that directory, and request it through a symlinked parent /alias/wallet: the fallback key differs from the recorded key, so ensure_storage_usable succeeds and DiskStorageManager::new can recreate the same location. The upstream lock is not sufficient protection when orphaned work retains individual stores after the disk manager has been dropped. This permits reuse without the process restart that the new contract requires. Resolve existing ancestors and normalize the missing suffix consistently, or refuse access when identity cannot be established. Cover a quarantined directory that is removed and subsequently requested through an aliased parent.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:61-69: Compute storage_key before taking the quarantine lock
  Rust evaluates the method receiver before its argument here, so orphaned_storage() acquires the process-wide mutex before storage_key(path) performs filesystem canonicalization. Consequently, an unrelated slow filesystem lookup blocks every quarantine lookup and publication, not just access to that directory. Compute the key before acquiring the guard so the global critical section contains only the map lookup and detail cloning.

In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:824: Record the public error enum addition as source-breaking
  (existing thread: https://github.com/dashpay/platform/pull/5322#discussion_r4207526411)
  PlatformWalletError is publicly re-exported and is not marked #[non_exhaustive], so adding SpvProcessRestartRequired makes downstream exhaustive matches fail to compile. The title's breaking-change marker and the existing host behavior notes do not describe this separate Rust source-compatibility consequence; the Breaking changes section currently discusses only the additive C ABI and host behavior. Add an explicit note that native Rust consumers must update exhaustive matches, and preserve that migration information in release notes. No PlatformVersion slot or consensus serialization change is involved.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The exact head addresses seven of the nine prior findings, including callback-safe running-state queries and panic recording independent of the stop caller. Two blocking issues remain: a combined missing-component/symlink path can bypass storage quarantine, and an observed panic can still produce a retryable error while cleanup is pending. This was a static review; the supplied CI snapshot establishes no completed compilation or test results, with policy checks pending and PR-title and milestone checks failing.

🔴 2 blocking | 🟡 2 suggestion(s)

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The intricate SPV lifecycle and cross-language error changes are substantial, but they do not themselves change consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:552-555: Report an observed panic as restart-required while cleanup is pending
  The observing task now records a startup or stop panic before awaiting subsequent cleanup, but this timeout branch still unconditionally returns `ShutdownIncomplete`. If upstream stop panics while storage cleanup is blocked, the original runtime's stop and clear calls report retryable code 27; start also returns 27 through `ensure_joined(Shutdown::Running)` before checking quarantine. A fresh runtime correctly returns code 59 for the same directory. The new `should_refuse_storage_as_soon_as_a_stop_panic_is_observed` regression explicitly asserts this retryable result on the original runtime. Once the panic is known, retrying cannot restore SPV usability—even successful cleanup eventually leads to the permanent failed state—so the host receives the wrong recovery instruction. Make the observed failure take precedence in the public error selected by stop, start, and clear, while retaining ownership of the pending cleanup task.
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:73-79: Key the storage quarantine by filesystem identity, not the supplied path
  (existing thread: https://github.com/dashpay/platform/pull/5322#discussion_r4207526401)
  Replaying the missing remainder lexically can expose an existing symlink without resolving it. Let `/parent/alias` point to a quarantined directory and `/parent/unused` not exist. For `/parent/unused/../alias`, canonicalization falls back to `/parent`; this loop then appends `unused`, pops it, and appends `alias`, producing `/parent/alias` instead of the quarantined target. The shared guard therefore permits the path. At the pinned dash-spv revision, `with_storage_path` preserves that spelling and `DiskStorageManager::new` calls `create_dir_all` before opening storage: directory creation can create `unused`, making the original path usable through the symlink. Its sibling lock file also differs from the target directory's lock file, so upstream locking does not prevent reopening storage still accessible to orphaned SPV work. Resolve existing components exposed while replaying the remainder, or reject paths whose identity cannot safely be established, and add coverage combining a nonexistent component, `..`, and a symlink.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:328-329: Qualify panic recovery for the shipping iOS panic strategy
  The new wrapper documentation says an SPV startup or teardown panic becomes a catchable `spvProcessRestartRequired` error. However, `build_ios.sh` maps its supported profiles to `release-ios` or `dev-ios`, and both set `panic = "abort"` in the workspace manifest. In those artifacts, a Rust panic terminates the app before Tokio can report a panic through `JoinError` and before the FFI call can return code 59. The mapping remains useful for unwind-enabled native builds, but the documented panic-recovery path is unavailable in standard iOS artifacts. Qualify the new Swift start/stop documentation and the PR limitations; changing the existing panic strategy is not required here.

In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:824: Record the public error enum addition as source-breaking
  (existing thread: https://github.com/dashpay/platform/pull/5322#discussion_r4207526411)
  `PlatformWalletError` is publicly re-exported and has no `#[non_exhaustive]` attribute. Adding `SpvProcessRestartRequired` therefore breaks downstream Rust consumers that exhaustively match the existing enum. The title's `!` signals a breaking change, but the supplied Breaking changes section describes only host behavior changes and the additive C ABI code. Explicitly document that downstream exhaustive Rust matches must handle the new variant, while retaining the distinction that existing C ABI codes and layouts are unchanged. No consensus protocol-version change is required.

[thepastaclaw]

🔴 Blocking: Report an observed panic as restart-required while cleanup is pending

The observing task now records a startup or stop panic before awaiting subsequent cleanup, but this timeout branch still unconditionally returns ShutdownIncomplete. If upstream stop panics while storage cleanup is blocked, the original runtime's stop and clear calls report retryable code 27; start also returns 27 through ensure_joined(Shutdown::Running) before checking quarantine. A fresh runtime correctly returns code 59 for the same directory. The new should_refuse_storage_as_soon_as_a_stop_panic_is_observed regression explicitly asserts this retryable result on the original runtime. Once the panic is known, retrying cannot restore SPV usability—even successful cleanup eventually leads to the permanent failed state—so the host receives the wrong recovery instruction. Make the observed failure take precedence in the public error selected by stop, start, and clear, while retaining ownership of the pending cleanup task.

_{source: gpt-6.1-sol (phase2-reviewer: general)}

[Claudius-Maginificent]

Out of scope. The contract in the PR description covers this state: while teardown is still running the answer is ShutdownIncomplete ("stop again"), and the stop that joins it returns SpvProcessRestartRequired. Giving a known panic precedence over a pending cleanup is a new guarantee and needs extra state in the runtime, so it is not part of this PR.

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

[thepastaclaw]

Deferred (re-reviewed at 744c8e29): Your join-based interpretation matches the current code: unfinished teardown remains code 27, and the stop that joins an unsuccessful task establishes sticky code 59. Exposing an intermediate observed panic before cleanup finishes would require an additional failure signal, so it is not a merge requirement for this contract.

[thepastaclaw]

✅ Withdrawn at 1646f1e2; see the replies below.

🟡 Suggestion: Qualify panic recovery for the shipping iOS panic strategy

The new wrapper documentation says an SPV startup or teardown panic becomes a catchable spvProcessRestartRequired error. However, build_ios.sh maps its supported profiles to release-ios or dev-ios, and both set panic = "abort" in the workspace manifest. In those artifacts, a Rust panic terminates the app before Tokio can report a panic through JoinError and before the FFI call can return code 59. The mapping remains useful for unwind-enabled native builds, but the documented panic-recovery path is unavailable in standard iOS artifacts. Qualify the new Swift start/stop documentation and the PR limitations; changing the existing panic strategy is not required here.

Suggested change

	/// — and `spvProcessRestartRequired` after SPV startup or teardown
	/// panicked (restart the app).
	/// — and `spvProcessRestartRequired` after a panic captured by an
	/// unwind-enabled native build (restart the app). The standard iOS
	/// native profiles use `panic = "abort"`, so a Rust panic instead
	/// terminates the app process before this call can throw.

_{source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)}

[thepastaclaw]

Withdrawn (re-reviewed at 1646f1e2): Your current stopSpv comment explains what to do when restart-required is returned, rather than promising that Swift can catch every native panic. Although the iOS profiles abort on panic, the shared error mapping and conditional recovery guidance do not contradict that strategy.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The typed Rust, C, Swift, and Kotlin error mappings are consistent, but three client-local lifecycle issues remain: an observed startup panic can remain classified as retryable, concurrent start loses the retryable classification, and failed background sync can temporarily report itself as running during cleanup. These are non-consensus correctness suggestions under the supplied severity policy. Validation was static only; the supplied CI snapshot shows Kotlin build/tests passing, policy reconciliation queued, and no Rust or Swift validation results.

🟡 3 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Preserve the retryable classification while stop holds the shutdown lock
packages/rs-platform-wallet/src/spv/runtime.rs:172-177

stop_with holds the shutdown mutex while joining teardown. After stop_locked removes the client, a concurrent start passes its initial client check but fails this try_lock and returns SpvError, which maps to ErrorUnknown (99), rather than ShutdownIncomplete (27). The existing should_refuse_a_start_while_a_stop_is_joining_the_run_loop test exercises this interleaving and explicitly expects SpvError. Thus the same pending teardown produces a generic error while the first stop is waiting and the typed retryable error after it times out. Distinguish active teardown from concurrent construction when selecting this error, and update the test to require ShutdownIncomplete for the stop-in-progress case.

_{source: gpt-6.1-sol (phase2-reviewer: rust-quality)}

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The change spans an asynchronous SPV lifecycle state machine and cross-language error handling, but the diff does not change consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:239-241: Keep failed background sync from reporting running during cleanup
  At the pinned rust-dashcore revision 314f10600e35311f3d875a6e8740476ab6c79c0c, background failure cancels the sync token, invokes on_error, and spawns stop_failed. That cleanup holds the sync_loop mutex throughout stop_locked, including coordinator, network, and storage shutdown. The retained client's is_running future therefore returns Pending during cleanup, and this branch converts that into true even though sync has already failed. The getter can report false inside on_error and then revert to true until cleanup releases the lock. Preserve a callback-safe failure/running snapshot rather than interpreting every contended lock as running, and test the answer while automatic failure cleanup remains pending.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:172-177: Preserve the retryable classification while stop holds the shutdown lock
  stop_with holds the shutdown mutex while joining teardown. After stop_locked removes the client, a concurrent start passes its initial client check but fails this try_lock and returns SpvError, which maps to ErrorUnknown (99), rather than ShutdownIncomplete (27). The existing should_refuse_a_start_while_a_stop_is_joining_the_run_loop test exercises this interleaving and explicitly expects SpvError. Thus the same pending teardown produces a generic error while the first stop is waiting and the typed retryable error after it times out. Distinguish active teardown from concurrent construction when selecting this error, and update the test to require ShutdownIncomplete for the stop-in-progress case.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:415-420: Report an observed panic as restart-required while cleanup is pending
  (existing thread: https://github.com/dashpay/platform/pull/5322#discussion_r4208931829)
  The teardown task can receive a startup panic JoinError from task.await, retain that error locally, and then await stop_client before exposing it. If client cleanup remains pending beyond the deadline, stop_locked returns ShutdownIncomplete (27), and later lifecycle calls cannot obtain the restart-required classification until the outer teardown finishes. Collapsing cleanup into one task does not eliminate this intermediate state: the known startup failure and cleanup completion are still separate events. This concerns the originating runtime, not the deliberately excluded cross-manager quarantine guarantee. Record the observed failure independently of cleanup completion, retain ownership of the continuing cleanup task, and consult that failure when classifying lifecycle calls. Add a regression combining a panicked startup handle with a retained client and deliberately pending cleanup.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Audit panic containment at direct native entry points — The block_on_worker helper's expect("tokio worker panicked") and the direct C construction boundary are unchanged from the supplied PR base. This PR does not newly establish general panic containment for direct native entry points, so a broader boundary audit is outside this review.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

✅ Resolved at 744c8e29; see the replies below.

🟡 Suggestion: Keep failed background sync from reporting running during cleanup

At the pinned rust-dashcore revision 314f10600e35311f3d875a6e8740476ab6c79c0c, background failure cancels the sync token, invokes on_error, and spawns stop_failed. That cleanup holds the sync_loop mutex throughout stop_locked, including coordinator, network, and storage shutdown. The retained client's is_running future therefore returns Pending during cleanup, and this branch converts that into true even though sync has already failed. The getter can report false inside on_error and then revert to true until cleanup releases the lock. Preserve a callback-safe failure/running snapshot rather than interpreting every contended lock as running, and test the answer while automatic failure cleanup remains pending.

_{source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)}

[thepastaclaw]

Resolved (re-reviewed at 744c8e29): Your change in 744c8e2 returns false when upstream's lifecycle query is pending during failed-sync cleanup. The updated regression checks both the callback read and repeated reads during cleanup, addressing the reported flap.

[Claudius-Maginificent]

Re the review finding "Preserve the retryable classification while stop holds the shutdown lock" (runtime.rs:172-177): out of scope. Those lines are unchanged by this PR, and the same lock is also held by a concurrent start, so returning ShutdownIncomplete there correctly needs new state. The contract in the PR description describes calls made one after another.

The other open suggestion, is_running reporting running during upstream cleanup, is fixed in 744c8e2.

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

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The lifecycle changes preserve teardown ownership across timeouts and distinguish retryable shutdown from permanent runtime failure without changing consensus behavior or existing FFI codes. No blocking defects remain; two documentation suggestions concern transient running-state answers and public-API source compatibility. Validation was static only: the supplied exact-head CI snapshot shows pending or queued policy, title, and runner-selection checks, not completed build or test validation.

🟡 2 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The SPV runtime lifecycle and cross-language error changes are intricate but do not themselves change consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort high); agent phase2-reviewer

🤖 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/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:217-219: Document the transient false answer during fork recovery
  The single-poll getter returns false whenever upstream's lifecycle query is pending. At the pinned rust-dashcore revision, handle_fork holds the same sync_loop mutex across stopping, truncating storage, and restarting sync, so callers can receive false during automatic recovery without requesting a stop or encountering a terminal failure. The implementation comment and commit message acknowledge this trade-off, but the public Rust, FFI, and Swift documentation does not disclose it. Document the conservative snapshot semantics consistently across those APIs, including that false does not confirm completed teardown or released storage. This also explains why Kotlin's progress poll can temporarily clear its overlay during recovery; an upstream API redesign is not necessary to address the documentation mismatch.

In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:815-822: Record the public error enum addition as source-breaking
  (existing thread: https://github.com/dashpay/platform/pull/5322#discussion_r4207526411)
  PlatformWalletError is public and exhaustive, so adding SpvProcessRestartRequired requires downstream Rust consumers with exhaustive matches to update their source. The Kotlin sealed error hierarchy also gains cases that affect exhaustive consumer handling. The PR description documents the recovery behavior but does not identify this compatibility consequence or the migration. Add a public-API compatibility note directing consumers to update exhaustive handling and distinguish ShutdownIncomplete from SpvProcessRestartRequired. The title's ! does not replace that note: book/src/contributing/coding-conventions.md reserves that marker for consensus-breaking changes, whereas this change is client-API-only and needs no protocol activation.

Out-of-scope follow-up suggestions (2)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Preserve the retryable classification while stop holds the shutdown lock — The shutdown.try_lock contention branch is identical at the PR base and head and can represent concurrent construction as well as teardown. Sequential calls against tracked teardown receive ShutdownIncomplete through ensure_joined; distinguishing overlapping operations would add a concurrency guarantee outside the stated contract.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Report an observed panic as restart-required while cleanup is pending — The current state machine retains Shutdown::Running until the owned teardown joins, returns ShutdownIncomplete on timeout, and selects sticky restart-required failure from an unsuccessful join. Immediate precedence for an intermediate startup panic would require another observable failure state and is explicitly outside the clarified join-based contract.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[llbartekll]

Reviewed the diff against upstream dash-spv at the pinned rev 314f106 and ran the Rust side locally on 744c8e29:

• cargo test -p platform-wallet --lib spv:: — 35 passed
• cargo test -p platform-wallet-ffi --lib spv_teardown_errors_keep_distinct_pinned_codes — passed
• cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warnings — clean
• Mutation check: deleting the new StorageManager::stop(&mut storage_manager) call in start makes both should_leave_no_storage_writer_* tests fail with "a storage writer outlived its client", so the regression guard is real.
• The generated platform-wallet-ffi.h exposes PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SPV_PROCESS_RESTART_REQUIRED = 59, matching what the Swift mapping references.

What I checked in the code:

• is_running is genuinely non-blocking: try_read on the client, then a single poll of upstream DashSpvClient::is_running under tokio::task::unconstrained with a noop waker. Upstream's is_running only awaits the sync_loop tokio mutex, and a dropped Acquire future dequeues itself, so answering false on Pending is safe. The nested block_on callback panic from the earlier revision is gone.
• Storage writer: confirmed against upstream that DiskStorageManager::new spawns the 5 s persist worker, start_sync restarts it last, stop_locked stops it, and nothing aborts it on drop. Stopping it right after construction is the right place for a client that never ran or failed construction.
• The 27 vs 59 split is consistent across ensure_joined, stop_locked, the FFI From impl, the registry, Swift and Kotlin.
• The open thread at runtime.rs:437 is about an "observed panic before cleanup finishes" design that is no longer in this PR; the bot deferred it at 744c8e29. Agree it is out of scope for the join-based contract.

Non-blocking notes:

1. CI on this base branch only ran the Kotlin job, so the Swift changes were not compiled anywhere (the description says they are "verified by CI only"). They are mechanical and mirror the code-58 wiring, and the header name checks out, but please make sure the iOS build runs before this stack lands on the dev branch.
2. The title carries ! (the new public PlatformWalletError variant breaks downstream exhaustive matches) but the body has no "Breaking Changes" section from the template. A one-liner there would also close the open suggestion thread.