fix(swift-sdk): handle ErrorShutdownIncomplete on PR #3954#3957
fix(swift-sdk): handle ErrorShutdownIncomplete on PR #3954#3957thepastaclaw wants to merge 3 commits into
Conversation
…_INCOMPLETE The Rust FFI gained `ErrorShutdownIncomplete = 19` so callers can know that `platform_wallet_manager_destroy` returned without proving every background coordinator thread exited. The Swift mirror was missing the case, so `init(ffi:)` fell through to `errorUnknown` and Swift callers had no way to tell the lifecycle-specific shutdown-incomplete from a generic unknown failure. Add `errorShutdownIncomplete = 19` to `PlatformWalletResultCode`, map `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE` in `init(ffi:)`, and add a matching `PlatformWalletError.shutdownIncomplete` variant so the error sums round-trip cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`platform_wallet_manager_destroy` now returns `errorShutdownIncomplete` when one or more background coordinator OS threads did not exit before the 30 s join deadline. A lingering coordinator still holds an `Arc<FFIPersister>` / `Arc<FFIEventHandler>` whose context pointer is `Unmanaged.passUnretained(handler).toOpaque()` for the manager's `persistenceHandler` / `eventHandler` Swift objects — releasing those at `deinit` would dangle that pointer and the next callback would be a use-after-free, which is exactly the hazard the new result code was introduced to prevent. Capture the destroy result in `deinit` and, on `errorShutdownIncomplete`, move the handler references into a process-global retention slot so any final coordinator callback still resolves to live Swift memory. The clean path is unchanged: a `success` destroy releases the handlers as usual. The leak is bounded by the (rare) number of non-clean shutdowns and only retains two small objects per occurrence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
requested review from
QuantumExplorer,
ZocoLini,
llbartekll and
shumkov
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit e16bfaa) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused fix that correctly mirrors the new Rust ErrorShutdownIncomplete result code into Swift and prevents a use-after-free of the Unmanaged.passUnretained callback context when platform_wallet_manager_destroy cannot prove every coordinator thread exited. The fix is correct as written. Two non-blocking suggestions: harden the retention branch against future destroy() return codes, and add a regression test that locks in the enum mapping and the deinit retention path.
🟡 2 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`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:173: Retain on any non-success code, not just `.errorShutdownIncomplete`
The retention branch matches exactly on `.errorShutdownIncomplete`, but the Rust contract on the destroy boundary is broader: only a `Success` return guarantees no coordinator thread is still holding `Arc<FFIPersister>` / `Arc<FFIEventHandler>` whose context is `Unmanaged.passUnretained(handler).toOpaque()`. Today destroy only returns `Success` or `ErrorShutdownIncomplete`, so the current check works. However, `PlatformWalletResultCode.init(ffi:)` at `PlatformWalletResult.swift:100-101` maps unknown raw values to `.errorUnknown`, so if Rust ever grows another unclean-shutdown variant (e.g. a panicked-thread code) and Swift isn't regenerated in lockstep, the deinit will silently free the handler objects while a coordinator thread may still call back through them — re-opening the UAF. Inverting the check is the same one-line change but stays correct for any future destroy() return code.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:153-184: No test exercises the `errorShutdownIncomplete` mapping or the deinit retention path
Two regressions this PR fixes are entirely uncovered: (a) the Swift mirror drift where `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE` previously fell through to `.errorUnknown`, and (b) the deinit retention that keeps `persistenceHandler` / `eventHandler` alive when destroy reports `ErrorShutdownIncomplete`. A small `@testable` unit test that constructs the FFI code and asserts the mapping to `.errorShutdownIncomplete` + `PlatformWalletError.shutdownIncomplete` locks in (a). A test that drives destroy into the incomplete branch (e.g. via an FFI shim that always returns the incomplete code) and probes the handler via a `weak` reference that should remain non-nil after the manager goes out of scope locks in (b). Because the production failure mode requires the 30 s coordinator-join deadline to expire, this is the only realistic way to keep the fix from silently regressing. The PR checklist already acknowledges this gap.
| let destroyResult = PlatformWalletResult( | ||
| platform_wallet_manager_destroy(handle) | ||
| ) | ||
| if destroyResult.code == .errorShutdownIncomplete { |
There was a problem hiding this comment.
🟡 Suggestion: Retain on any non-success code, not just .errorShutdownIncomplete
The retention branch matches exactly on .errorShutdownIncomplete, but the Rust contract on the destroy boundary is broader: only a Success return guarantees no coordinator thread is still holding Arc<FFIPersister> / Arc<FFIEventHandler> whose context is Unmanaged.passUnretained(handler).toOpaque(). Today destroy only returns Success or ErrorShutdownIncomplete, so the current check works. However, PlatformWalletResultCode.init(ffi:) at PlatformWalletResult.swift:100-101 maps unknown raw values to .errorUnknown, so if Rust ever grows another unclean-shutdown variant (e.g. a panicked-thread code) and Swift isn't regenerated in lockstep, the deinit will silently free the handler objects while a coordinator thread may still call back through them — re-opening the UAF. Inverting the check is the same one-line change but stays correct for any future destroy() return code.
| if destroyResult.code == .errorShutdownIncomplete { | |
| if destroyResult.code != .success { |
source: ['claude']
| if handle != NULL_HANDLE { | ||
| platform_wallet_manager_platform_address_sync_stop(handle).discard() | ||
| platform_wallet_manager_shielded_sync_stop(handle).discard() | ||
| platform_wallet_manager_destroy(handle).discard() | ||
| // `destroy` joins every coordinator OS thread with a 30 s | ||
| // deadline. On `errorShutdownIncomplete` one or more | ||
| // coordinators are still alive and still hold the | ||
| // `Arc<FFIPersister>` / `Arc<FFIEventHandler>` whose context | ||
| // pointer is `Unmanaged.passUnretained(handler).toOpaque()` | ||
| // for the Swift handler objects below — freeing those Swift | ||
| // objects now would dangle that pointer and the next | ||
| // callback would be a use-after-free. Stash them in a | ||
| // process-global leak slot so any final callback still | ||
| // sees valid memory. We accept the bounded leak (two small | ||
| // objects per non-clean shutdown); a clean shutdown returns | ||
| // success and the handlers are released as usual. | ||
| let destroyResult = PlatformWalletResult( | ||
| platform_wallet_manager_destroy(handle) | ||
| ) | ||
| if destroyResult.code == .errorShutdownIncomplete { | ||
| PlatformWalletManager._leakedContextLock.lock() | ||
| if let h = persistenceHandler { | ||
| PlatformWalletManager._leakedContext.append(h) | ||
| } | ||
| if let h = eventHandler { | ||
| PlatformWalletManager._leakedContext.append(h) | ||
| } | ||
| PlatformWalletManager._leakedContextLock.unlock() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No test exercises the errorShutdownIncomplete mapping or the deinit retention path
Two regressions this PR fixes are entirely uncovered: (a) the Swift mirror drift where PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE previously fell through to .errorUnknown, and (b) the deinit retention that keeps persistenceHandler / eventHandler alive when destroy reports ErrorShutdownIncomplete. A small @testable unit test that constructs the FFI code and asserts the mapping to .errorShutdownIncomplete + PlatformWalletError.shutdownIncomplete locks in (a). A test that drives destroy into the incomplete branch (e.g. via an FFI shim that always returns the incomplete code) and probes the handler via a weak reference that should remain non-nil after the manager goes out of scope locks in (b). Because the production failure mode requires the 30 s coordinator-join deadline to expire, this is the only realistic way to keep the fix from silently regressing. The PR checklist already acknowledges this gap.
source: ['claude', 'codex']
…rm-wallet-shutdown-join Combined the errorShutdownIncomplete / shutdownIncomplete doc comments from both sides: the Swift retain-context guidance (Unmanaged.passUnretained opaque pointers handed to Rust must stay alive past destroy; PlatformWalletManager.deinit parks them in a process-global leak slot; do NOT retry destroy) and the broader destroy/stop/clear scope plus the clear-path rule (host must NOT commit its own persistence wipe -- Rust store is left intact). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review. The Swift mirror + deinit-retention fixes from prior commits remain correctly applied, and the new symmetric ErrorShutdownIncomplete contract on shielded clear/stop is well-implemented on the Rust/FFI side. One blocking issue: ShieldedService.clearLocalState (the example app's Clear button) catches the new shutdownIncomplete error, logs it, and proceeds to wipe SwiftData rows — directly violating the new FFI contract that documents 'the host must check this before wiping its own persistence'. The two prior suggestions (broader retention predicate, missing Swift coverage) are still applicable. Out of 10 budget.
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:604-624: Clear flow wipes SwiftData rows even when Rust reports ErrorShutdownIncomplete
This PR introduces a new symmetric contract on `platform_wallet_manager_shielded_clear`: when the in-flight pass fails to drain cleanly the call returns `ErrorShutdownIncomplete` and *intentionally leaves the Rust commitment-tree store intact*. The FFI doc (rs-platform-wallet-ffi/src/shielded_sync.rs:460-465) spells out the host obligation explicitly: "The host **must** check this before wiping its own persistence: a silent failure would leave the shared tree populated while the host drops its rows, and the next cold resync would gate-skip every re-downloaded position against the stale tree size."
The Swift `clearShielded()` wrapper now throws `PlatformWalletError.shutdownIncomplete` on this code (PlatformWalletManagerShieldedSync.swift:309, PlatformWalletResult.swift:252). But `ShieldedService.clearLocalState` at lines 604-611 catches *every* error from `clearShielded()`, logs it, and falls through to the `modelContext.delete(model: PersistentShielded*)` block at lines 619-624 — exactly the wipe the new contract was introduced to prevent. The stale comment at lines 602-603 ("Best-effort — failure logs but doesn't abort the wipe") is now incorrect.
On the `shutdownIncomplete` branch the function must abort the wipe and surface the error to the user; other clear failures may keep the existing best-effort behavior.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:173: Retention predicate should be `!= .success`, not exact match on `.errorShutdownIncomplete`
The retention slot exists because any non-`Success` return from `platform_wallet_manager_destroy` means a coordinator thread may still dereference the `Unmanaged.passUnretained(handler).toOpaque()` context pointer for `persistenceHandler` / `eventHandler`. Today the Rust `destroy` (manager.rs:351-388) returns only `ok()` or `ErrorShutdownIncomplete`, so the exact match works — but any future failure code added on this boundary (e.g. a propagated `InvalidHandle` from a double-destroy race) silently falls through and releases the handlers while a coordinator still holds the pointer. Widening to `!= .success` costs at most a bounded leak on currently-unreachable paths and removes the forward-compat cliff. Carry-over from prior review; the latest delta does not change line 173.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:153-184: No Swift coverage for the errorShutdownIncomplete mirror or the deinit retention path
Two cross-boundary regressions this PR fixes remain entirely uncovered on the Swift side: (a) `PlatformWalletResultCode.init(ffi:)` mapping `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE → .errorShutdownIncomplete` (PlatformWalletResult.swift:98-99) — exactly the mirror drift that previously fell through to `.errorUnknown`; and (b) the deinit retention branch at PlatformWalletManager.swift:173-182 that places `persistenceHandler` / `eventHandler` into `_leakedContext` when destroy reports incomplete shutdown. The new Rust test `shielded_shutdown_incomplete_maps_to_dedicated_code` covers the Rust→FFI mapping but does not protect the Swift enum mirror or the deinit branch from drifting again. A small Swift test that asserts the enum mapping and (with a stubbed handle whose destroy returns each code) drives a `PlatformWalletManager` through configure→tear-down and asserts the `_leakedContext` count delta would close the loop. Carry-over from prior review.
| let destroyResult = PlatformWalletResult( | ||
| platform_wallet_manager_destroy(handle) | ||
| ) | ||
| if destroyResult.code == .errorShutdownIncomplete { |
There was a problem hiding this comment.
🟡 Suggestion: Retention predicate should be != .success, not exact match on .errorShutdownIncomplete
The retention slot exists because any non-Success return from platform_wallet_manager_destroy means a coordinator thread may still dereference the Unmanaged.passUnretained(handler).toOpaque() context pointer for persistenceHandler / eventHandler. Today the Rust destroy (manager.rs:351-388) returns only ok() or ErrorShutdownIncomplete, so the exact match works — but any future failure code added on this boundary (e.g. a propagated InvalidHandle from a double-destroy race) silently falls through and releases the handlers while a coordinator still holds the pointer. Widening to != .success costs at most a bounded leak on currently-unreachable paths and removes the forward-compat cliff. Carry-over from prior review; the latest delta does not change line 173.
| if destroyResult.code == .errorShutdownIncomplete { | |
| if destroyResult.code != .success { |
source: ['claude', 'codex']
| if handle != NULL_HANDLE { | ||
| platform_wallet_manager_platform_address_sync_stop(handle).discard() | ||
| platform_wallet_manager_shielded_sync_stop(handle).discard() | ||
| platform_wallet_manager_destroy(handle).discard() | ||
| // `destroy` joins every coordinator OS thread with a 30 s | ||
| // deadline. On `errorShutdownIncomplete` one or more | ||
| // coordinators are still alive and still hold the | ||
| // `Arc<FFIPersister>` / `Arc<FFIEventHandler>` whose context | ||
| // pointer is `Unmanaged.passUnretained(handler).toOpaque()` | ||
| // for the Swift handler objects below — freeing those Swift | ||
| // objects now would dangle that pointer and the next | ||
| // callback would be a use-after-free. Stash them in a | ||
| // process-global leak slot so any final callback still | ||
| // sees valid memory. We accept the bounded leak (two small | ||
| // objects per non-clean shutdown); a clean shutdown returns | ||
| // success and the handlers are released as usual. | ||
| let destroyResult = PlatformWalletResult( | ||
| platform_wallet_manager_destroy(handle) | ||
| ) | ||
| if destroyResult.code == .errorShutdownIncomplete { | ||
| PlatformWalletManager._leakedContextLock.lock() | ||
| if let h = persistenceHandler { | ||
| PlatformWalletManager._leakedContext.append(h) | ||
| } | ||
| if let h = eventHandler { | ||
| PlatformWalletManager._leakedContext.append(h) | ||
| } | ||
| PlatformWalletManager._leakedContextLock.unlock() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No Swift coverage for the errorShutdownIncomplete mirror or the deinit retention path
Two cross-boundary regressions this PR fixes remain entirely uncovered on the Swift side: (a) PlatformWalletResultCode.init(ffi:) mapping PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE → .errorShutdownIncomplete (PlatformWalletResult.swift:98-99) — exactly the mirror drift that previously fell through to .errorUnknown; and (b) the deinit retention branch at PlatformWalletManager.swift:173-182 that places persistenceHandler / eventHandler into _leakedContext when destroy reports incomplete shutdown. The new Rust test shielded_shutdown_incomplete_maps_to_dedicated_code covers the Rust→FFI mapping but does not protect the Swift enum mirror or the deinit branch from drifting again. A small Swift test that asserts the enum mapping and (with a stubbed handle whose destroy returns each code) drives a PlatformWalletManager through configure→tear-down and asserts the _leakedContext count delta would close the loop. Carry-over from prior review.
source: ['claude', 'codex']
1 participant
Issue being fixed or feature implemented
Fixes the two CodeRabbit findings on #3954 head
2bd9501a0e:packages/rs-platform-wallet-ffi/src/error.rs128–135) — the Swift mirrorPlatformWalletResultCodeenum was missing the newerrorShutdownIncomplete = 19variant, soinit(ffi:)fell through toerrorUnknownforPLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE. Swift callers had no way to distinguish a lifecycle-specific shutdown-incomplete from a generic unknown failure.packages/rs-platform-wallet-ffi/src/manager.rs366–385, critical) — the Rust FFI addedErrorShutdownIncompleteso the host could keep its callback context alive when a coordinator OS thread did not exit before the 30 s join deadline. But the SwiftdeinitatPlatformWalletManager.swift:158callsplatform_wallet_manager_destroy(handle).discard()and unconditionally proceeds to releasepersistenceHandler/eventHandler— whoseUnmanaged.passUnretained(...).toOpaque()pointers a lingering coordinator'sArc<FFIPersister>/Arc<FFIEventHandler>still holds. Releasing them would dangle that pointer; the next callback would be a use-after-free — exactly the hazard the new result code was added to prevent.What was done?
PlatformWalletResultCode.errorShutdownIncomplete = 19and the matchinginit(ffi:)arm, plus aPlatformWalletError.shutdownIncomplete(String)payload so the error sums round-trip cleanly.PlatformWalletManager.deinit, capture the destroy result. OnerrorShutdownIncomplete, move the handler references into a process-global retention slot (_leakedContextguarded by_leakedContextLock) so any final coordinator callback resolves to live Swift memory. The clean path is unchanged.The two changes are split into separate commits so the enum mirror can land independently of the host-side safety fix if needed.
How Has This Been Tested?
cargo check -p platform-wallet-ffi— clean.swiftc -parse -swift-version 6on both modified Swift files — parses clean.DashSDKFFI.xcframeworkis not checked in (built bypackages/swift-sdk/build_ios.shfrom Rust artifacts), and the only pre-built xcframework available on this machine predates the platform-wallet FFI. ThePLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETEsymbol added in this PR followscbindgen'sErrorShutdownIncomplete→PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETErule used by every other variant in the file.Breaking Changes
None —
errorShutdownIncompleteonly fires when the new RustErrorShutdownIncompleteresult code is returned, which is itself new in PR #3954.Checklist:
🤖 Generated with Claude Code