feat(swift-sdk): add async off-main loadFromPersistor() overload - #4487
Conversation
The synchronous createWallet blocks the calling thread for the whole native create - key derivation for every account plus a synchronous persistence flush, seconds of work - and every production caller is @mainactor, so wallet creation froze the UI for its full duration. Add an additive async overload with identical semantics that runs the blocking FFI on the shared destroyQueue (same rationale as the async shutdown(): park a plain GCD thread, never the main thread or a cooperative-pool thread), with: - a MainActor prologue (ensureConfigured + handle/call-table snapshot) and a direct continuation (no Task wrapper), so an admitted create is enqueued FIFO-before any later shutdown's teardown block; - a MainActor epilogue that re-checks the handle: a shutdown that landed during the off-main window discards the created wallet (its wrapper destroy is a registry no-op after manager teardown) instead of publishing into a torn-down manager; - performCreateWallet timing + offMain logging in parity with performNativeTeardown, behind a new PlatformWalletNativeCreateCalls test seam; - tests covering off-main execution, error mapping, create-after- shutdown, the shutdown-during-create race (FIFO proven via a shared event log), and concurrent creates. The sync overload stays for sync contexts (loadFromPersistor recovery, MainActor.run test bodies). Async call sites resolve to the new overload per SE-0296: SpvLateWalletBackfillIntegrationTests:56 switches silently (behavior equivalent, now off-main); ContentView.recoverWallet and WalletDetailView.enableNetwork needed try await - which also retires enableNetwork's 50ms paint-a-frame sleep hack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up (P1): the epilogue-throw design could fail an admitted create RETROACTIVELY - the native create had already persisted wallet data through the persister when a concurrent shutdown() took the handle, so the invalidHandle throw made the app-side caller roll back its mnemonic and orphan the persisted rows (watch-only wallet without a mnemonic; retry hits WalletAlreadyExists). The destroy-queue FIFO only ordered the native calls, not the MainActor epilogue. shutdown() now closes admission first (shutdownRequested - new creates throw invalidHandle up front, before any native work) and then waits for every in-flight create's FULL transaction (native create + publish epilogue) to finish before the take-once. The idempotency/no-op checks re-run after each drain await. The epilogue handle re-check stays as defense in depth (assertionFailure) but is unreachable from the production shutdown path. Tests reworked to prove the new properties: the shutdown-during-create race asserts the handle stays live until the gated create finishes and that create:end precedes the first teardown step in a shared event log; a new case pins create-during-drain rejection; the concurrent-creates test now measures maxInFlight == 1 through the seam. Also adopts the async overload in the example app's CreateWalletView (its per-network create loop ran synchronously inside MainActor.run, freezing the UI - the second reviewer note). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPlatform wallet management now supports asynchronous persistence restoration, generalized native-operation admission, coordinated shutdown draining, and expanded concurrency tests. SDK destruction now logs thread context and elapsed time. ChangesPlatform Wallet Restoration
SDK Destruction Logging
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds an off-main asynchronous wallet restore path while preserving synchronous behavior, with reported full-suite and consumer validation. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant SharedSerialQueue
participant PlatformWalletNativeLoadCalls
participant MainActor
PlatformWalletManager->>SharedSerialQueue: run loadFromPersistor native phase
SharedSerialQueue->>PlatformWalletNativeLoadCalls: bulk restore and retrieve wallet IDs
PlatformWalletNativeLoadCalls-->>SharedSerialQueue: restoration results and wallet IDs
SharedSerialQueue->>PlatformWalletNativeLoadCalls: look up wallet handles
PlatformWalletNativeLoadCalls-->>SharedSerialQueue: wallet handles and lookup errors
SharedSerialQueue->>MainActor: return restoration results
MainActor->>PlatformWalletManager: publish wallets and process unlock outcomes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🕓 Ready for review — 1 ahead in queue (commit 5d943c9) |
Etap-C telemetry in dashwallet attributed the remaining Add Wallet main-thread stalls to loadFromPersistor: roughly 400ms per persisted wallet on the MainActor (414/846/1261ms for 1/2/3 wallets, matching 449-1436ms runloop stalls), while SDK construction, the ModelContainer and configure measured 0-9ms. The cost repeats across temporary managers, so the marker-cached seed verify is not the bulk of it - the bulk restore's persister reads and Rust wallet reconstruction are. Additive async overload with identical semantics (bulk restore, per-wallet handle lookup, publish, best-effort keychain unlock, asset-lock catch-up): the bulk FFI, the SwiftData id fetch (the handler serializes on its own queue and background context) and the per-wallet lookups run on the shared destroyQueue via performLoadFromPersistor (direct continuation - same FIFO-before-teardown argument as the async create), with a MainActor epilogue that builds the wrappers and publishes. The keychain unlock stays in the epilogue (actor-isolated, marker-cached) and is timed as a block - its measured share decides whether it moves too. Telemetry parity: 'native load finished in Xms offMain=Y wallets=N' + 'load unlock loop finished in Xms'. The shutdown drain generalizes from creates to native ops (admitNativeOp/finishNativeOp; activeCreateCount -> activeNativeOpCount): an admitted load completes its full transaction before teardown takes the handle, and new loads are rejected while a drain runs. The per-wallet unlock body is factored into unlockRestoredWalletLoggingOutcome, shared verbatim by both overloads. New PlatformWalletNativeLoadCalls seam (bulk + id list + lookup; the id list is part of the seam so unit tests drive the lookup loop without a ModelContainer) + PlatformWalletLoadFromPersistorTests mirroring the create suite: off-main execution, bulk error mapping, skip-and-continue parity, shutdown-drain wait, and drain-window rejection. SE-0296 audit: both existing sync call sites stay sync (a MainActor.run closure in IntegrationTestEnv and the example app's synchronous activate) - no silent re-resolution, no source breaks in this repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e guard; isolate load tests Review follow-ups on the async loadFromPersistor, on top of the rebase onto the parent's close-create-admission fix (5b80766): 1. The SYNC loadFromPersistor checked only ensureConfigured. During the shutdown drain the MainActor is reentrant and the handle is intentionally live, so a sync load could be admitted without touching activeNativeOpCount and run a SECOND Rust loader concurrently with the admitted async one on the destroy queue. That is a real race: load.rs inserts into wallet_manager and self.wallets in two steps, and its already-present short-circuit ('fully hydrated by the earlier call') only holds for sequential loaders - a parallel loader that sees the first insert skips the wallet before the second lands. The parent's ensureWalletCreationAllowed generalizes into ensureSyncNativeOpAllowed, shared by sync createWallet, createWalletFromSeed and loadFromPersistor: rejected after shutdown closes admission AND while any async native op is in flight (async ops keep admitNativeOp - they serialize on the destroy queue, so async-with-async stays legal). Error messages are now per-op; the parent's all-overloads drain test is adapted to the new contract (its empty-seed poll now expects the in-flight rejection until shutdown lands - invalidParameter can no longer surface there). 2. Handle ownership in the async load epilogue: the wrappers are now built BEFORE the defensive handle re-check, so if that guard ever fired the wrapper deinits would release the Rust-side aliases instead of leaking raw handles from outcome.loaded. Publishing still happens only after the guard passes. 3. Test isolation: the load recorder returned fake wallet handles (200+n) that ManagedPlatformWallet.deinit destroys through the LIVE FFI - Rust handles are process-global and monotonic, so a test could free another test's entry. Successful fake lookups now return NULL_HANDLE (wallet ids are sufficient to tell results apart), the suite doc no longer claims the tests never touch FFI, and the new rejection tests use manager handles outside any realistic registry range so even a guard regression could only produce a safe miss. New tests: sync load rejected while an async load is in flight, sync load rejected during the shutdown drain, sync create rejected while an async create is in flight (no shutdown). Also adds a cheap timing + offMain stamp to SDK.deinit's dash_sdk_destroy - the data for deciding whether SDK destruction ever needs an explicit off-main hop (follow-up candidate; the last reference is often released on the MainActor). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The async overload successfully moves persisted-wallet restoration off the main thread and coordinates its lifetime with shutdown. Two in-scope correctness issues remain: wallet deletion can race restoration and be undone, and per-wallet failures no longer update lastError in the synchronous overload's order.
Source: Codex general, FFI engineer, and security auditor reviewer lanes; final verifier: Claude Agent SDK. Exact backend model IDs were not present in the supplied evidence. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:1287-1292: Serialize wallet deletion with an in-flight restore
This overload releases the MainActor while native restoration and handle lookup run, but `deleteWallet(walletId:)` does not participate in `activeNativeOpCount` or the shared queue. If deletion runs after the persistence snapshot is read but before Rust inserts the wallet, the idempotent native removal succeeds and the loader can subsequently insert the deleted snapshot into Rust after SwiftData and Keychain have been wiped. If deletion instead runs after `getWallet` produced a retained handle but before this epilogue, deletion removes the native registration and persisted data, then these lines republish the removed generation into `self.wallets`. Both interleavings violate `deleteWallet`'s documented full-wipe semantics. Serialize deletion and restoration through one lifecycle mechanism, or reject deletion while a native operation is active, and test both deletion-before-insert and deletion-before-publication.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:1294-1304: Preserve per-wallet FFI error ordering from the synchronous overload
The synchronous overload processes lookup, publication, and unlock together for each wallet, so `lastError` reflects failures in wallet-ID order. The async path instead collapses all lookup failures into `lastSkipError`, assigns that once, and then performs every unlock. For IDs `[A, B]`, if A's lookup succeeds but its seed verification fails while B's later lookup fails, the synchronous overload leaves B's lookup diagnostic in `lastError`; this overload assigns B's error first and then overwrites it with A's earlier verification error. That breaks the promised identical semantics and can hide the most recent Rust lookup failure. Preserve an ordered sequence containing either each copied lookup error or its successful wallet handle, then replay lookup-error assignment and publication/unlock in that sequence on the MainActor.
4048891 to
8835b09
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The async overload moves persisted-wallet reconstruction off the MainActor and coordinates admitted loads with shutdown, but deletion remains admissible during an in-flight restore and can undo a completed full wipe. The async epilogue also does not preserve the synchronous overload's wallet-ID ordering for lastError updates.
Source: Codex general and FFI-engineer reviewer lanes (exact backend model IDs were not supplied); final verifier: Anthropic Claude via Claude Agent SDK (exact backend model ID was not supplied). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:1904: Serialize wallet deletion with an in-flight restore
(existing thread: https://github.com/dashpay/platform/pull/4487#discussion_r3864060462)
The new async overload releases the MainActor while native restoration and wallet-handle lookup run, but `deleteWallet(walletId:)` only calls `ensureConfigured()` and remains admissible while `activeNativeOpCount` is nonzero. Rust's loader owns its persistence snapshot before inserting wallets, while native removal treats an absent wallet as success. Deletion after the snapshot read but before insertion can therefore wipe SwiftData and Keychain and remove nothing from Rust, after which the loader inserts the deleted wallet from its captured snapshot. If deletion instead occurs after `platform_wallet_manager_get_wallet` has retained a registry handle but before the MainActor epilogue, deletion removes the native manager generation and persisted data, then the epilogue republishes that removed alias into `self.wallets`. Reject deletion through the synchronous native-operation admission gate, or otherwise serialize it with restoration, and cover both interleavings.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:1336-1346: Preserve per-wallet FFI error ordering from the synchronous overload
(existing thread: https://github.com/dashpay/platform/pull/4487#discussion_r3864060473)
The synchronous overload processes each wallet's lookup, publication, and unlock before advancing to the next wallet ID, so failures update `lastError` in wallet-ID order. The async path instead collapses all lookup failures into `lastSkipError`, assigns that error once, and then unlocks every successful wallet. For IDs `[A, B]`, if A's lookup succeeds but its seed verification fails and B's later lookup fails, the synchronous overload leaves B's lookup diagnostic in `lastError`; this path assigns B's error first and then overwrites it with A's earlier verification error. Preserve an ordered outcome for every wallet ID—either its copied lookup error or its owned wallet handle—and replay error assignment, publication, and unlock in that original order on the MainActor.
…outcomes in sequence Two review findings on the async loadFromPersistor: 1. deleteWallet (blocking): a synchronous native op (native removal + Keychain + SwiftData wipe) guarded only by ensureConfigured, so it could interleave with an in-flight async restore - removing the native registration between the restore's snapshot read and its publication, or wiping data the loader then re-inserts, violating its full-wipe semantics. It now goes through the same ensureSyncNativeOpAllowed gate as the sync creates and load: rejected during the shutdown drain and while any async native op is in flight, before ANY of its work (keychain sweep included) runs. No app flow deletes concurrently with a restore - both run inside serialized lifecycle operations - so the gate only forbids what was already unsupported. 2. lastError ordering (suggestion): the epilogue collapsed all lookup failures into one last-skip error assigned before the unlock loop, so for ids [A(unlock fails), B(lookup fails)] it would leave A's unlock error where the sync overload leaves B's lookup diagnostic. The off-main half now returns ONE ORDERED entry per restorable id (restored handle or copied lookup error) and the MainActor epilogue replays them in sequence - publish + unlock per wallet, error assignment in place - so lastError ends up reflecting the same latest-in-order failure as the sync overload. Handle ownership is unchanged: wrappers are built for every restored entry before the defensive guard, publication only after it. The unlock timing now accumulates across the interleaved calls (same log line). New tests: deleteWallet rejected while an async load is in flight (the admitted restore completes untouched); lastError holds the later failure in id order (per-id failure codes in the recorder). Full suite: 391 tests, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ync-load-from-persistor # Conflicts: # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift # packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletCreateWalletTests.swift
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head changes resolve both prior findings: deleteWallet now fails admission before any native, Keychain, or SwiftData work while an async restore is active, and async restore outcomes are replayed in wallet-ID order so lastError matches the synchronous overload. The targeted PlatformWalletLoadFromPersistorTests suite passed locally with 9 tests and 0 failures, including regression coverage for deletion admission and ordered failure replay. No remaining in-scope findings were identified.
Source: Codex general and Codex FFI-engineer reviewer lanes (exact backend model IDs were not supplied); final verifier: Anthropic Claude via Claude Agent SDK (exact backend model ID was not supplied). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
Issue being fixed or feature implemented
Stacked on #4483 (async createWallet). dashwallet's etap-C telemetry attributed the remaining Add Wallet main-thread stalls to
loadFromPersistor: roughly 400ms per persisted wallet on the MainActor (414/846/1261ms for 1/2/3 wallets, matching 449–1436ms runloop stalls attributed by timestamp), while SDK construction, the ModelContainer andconfiguremeasured 0–9ms. The cost repeats across temporary managers, so the marker-cached seed verify is not the bulk of it — the bulk restore's persister reads and Rust wallet reconstruction are.What was done?
Additive async overload of
loadFromPersistor()with identical semantics (bulk restore, per-wallet handle lookup, publish, best-effort keychain unlock, asset-lock catch-up):performLoadFromPersistoron the shareddestroyQueue, direct continuation — same FIFO-before-teardown argument as the async create): the bulk FFI (its persistence callbacks fire synchronously on the queue — the handler is thread-safe by design, the same callbacks fire from Rust sync threads in steady state), the SwiftData id fetch (the handler serializes on its own queue and background context), and the per-walletget_walletlookups. FFI results are mapped on the queue; raw handles cross the continuation, theManagedPlatformWalletwrappers are built in the MainActor epilogue.native load finished in 403ms offMain=true wallets=1andload unlock loop finished in 2ms— the unlock share is negligible, so no further split is warranted.admitNativeOp/finishNativeOp;activeCreateCount→activeNativeOpCount): an admitted load completes its full transaction before teardown takes the handle; new loads are rejected up front while a drain runs.unlockRestoredWalletLoggingOutcome, shared verbatim by both overloads (no second copy).PlatformWalletNativeLoadCallsseam (bulk + id list + lookup; the id list is part of the seam so unit tests drive the lookup loop without a ModelContainer) +PlatformWalletLoadFromPersistorTestsmirroring the create suite: off-main execution, bulk error mapping, per-wallet skip-and-continue parity (lastError), shutdown-drain wait, and drain-window rejection.No Rust changes.
How Has This Been Tested?
swift test: full suite green — 386 tests (5 new), 0 failures.Breaking Changes
Source-compatibility caveat, same shape as #4483: the same-name async overload changes overload resolution —
try await manager.loadFromPersistor()re-resolves to it, andtry manager.loadFromPersistor()inside an async context stops compiling untilawaitis added. Audit of this repo: both existing call sites are synchronous contexts (aMainActor.runclosure inIntegrationTestEnv, the example app's synchronousactivate) — no silent re-resolution, no source breaks here.Checklist:
🤖 Generated with Claude Code
Review follow-ups (second commit)
Rebased onto the parent's close-create-admission fix (
5b80766d) and hardened per review:loadFromPersistoronly checkedensureConfigured, so during the shutdown drain (MainActor reentrant, handle intentionally live) it could run a second Rust loader concurrently with the admitted async one. That race is real:load.rshydrateswallet_managerandself.walletsin two steps, and its already-present short-circuit only holds for sequential loaders. The parent'sensureWalletCreationAllowedgeneralizes intoensureSyncNativeOpAllowed(sync create, seed create, sync load): rejected after shutdown closes admission and while any async native op is in flight. Async ops keepadmitNativeOp(they serialize on the destroy queue). The all-overloads drain test is adapted to the per-op messages and the new in-flight rejection.ManagedPlatformWalletwrappers before the defensive handle re-check, so a firing guard drops them through deinit instead of leaking raw registry aliases; publishing still happens only after the guard.NULL_HANDLE(wrapper deinit destroys through the live FFI and Rust handles are process-global/monotonic); new rejection tests use manager handles outside any realistic registry range; suite docs no longer claim "without calling FFI".SDK.deinittiming stamp —dash_sdk_destroyis now timed with anoffMain=marker (SDKLifecyclecategory): the data for the follow-up question of whether SDK destruction (often released on the MainActor during stop/error unwind) ever needs an explicit off-main hop.New tests: sync-load-during-async-load rejection, sync-load-during-drain rejection, sync-create-during-async-create rejection. Full suite: 389 tests, 0 failures.
Review comments addressed (third commit,
0eef234d20)deleteWalletvs in-flight restore:deleteWallet(a synchronous native op: native removal + Keychain + SwiftData wipe) now goes through the sameensureSyncNativeOpAllowedgate — rejected during the shutdown drain and while any async native op is in flight, before any of its work runs. Test:testDeleteWalletIsRejectedWhileAsyncLoadIsInFlight.lastErrorordering parity: the off-main half returns one ordered entry per restorable id (restored handle or copied lookup error); the epilogue replays publish + unlock + error assignment per wallet in sequence, matching the sync overload's interleaving. Test:testLastErrorReflectsTheLatestFailureInSequenceOrder.Full suite after: 391 tests, 0 failures; dashwallet builds unchanged against this head.
Summary by CodeRabbit
New Features
Bug Fixes
Diagnostics