fix(platform-wallet): poll Sync Now FFI passes on big-stack threads to stop SIGBUS crash#4033
Conversation
…o stop SIGBUS crash The shielded, platform-address, and identity-token sync_now FFI entry points polled their whole sync future via runtime().block_on on the host's calling thread. iOS dispatch/concurrency threads have ~512 KB of stack, and the notes-sync / proof-verification recursion now exceeds it, so tapping "Sync Now" under Shielded Sync Status aborted the app with SIGBUS "Thread stack size exceeded" (reported by TestFlight users on the latest build; reproduced on-sim with a matching crash log). Move the polling off the calling thread the same way dashpay_sync already does: block_on_worker dispatches the pass onto the runtime's 8 MB-stack workers. The platform-address pass can't satisfy block_on_worker's Send + 'static bounds (rustc issue #100013), so add a run_on_big_stack_thread escape hatch that creates and polls the future entirely on a scoped 8 MB OS thread. Verified on-sim against wallets with 1,142 synced notes: repeated Shielded/DashPay/Platform Sync Now taps complete without crashing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a ChangesFFI sync execution context refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FFI as FFI Caller
participant Entry as Sync Entrypoint
participant Worker as block_on_worker
participant BigStack as run_on_big_stack_thread
participant Mgr as Sync Manager
FFI->>Entry: sync_now() / sync_wallet()
alt identity_sync / shielded_sync
Entry->>Worker: block_on_worker(async move { mgr.sync_now().await })
Worker->>Mgr: sync_now()/sync_wallet()
Mgr-->>Worker: result
Worker-->>Entry: result
else platform_address_sync
Entry->>BigStack: run_on_big_stack_thread(closure)
BigStack->>Mgr: runtime().block_on(sync_now())
Mgr-->>BigStack: result
BigStack-->>Entry: result or spawn error
end
Entry-->>FFI: PlatformWalletFFIResult
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit b97316f) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-platform-wallet-ffi/src/runtime.rs (2)
76-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPanics in the helper will abort the process, undermining the crash-fix goal.
Both
.expect("failed to spawn big-stack FFI thread")(spawn failure, e.g. OS resource exhaustion) and.expect("big-stack FFI thread panicked")(any panic insidef, including panics deep in sync/proof-verification logic) will unwind out of thispub(crate)helper into the callingextern "C"FFI entrypoint. Since Rust automatically converts an unwind that would escape anextern "C"function into a process abort, either failure mode crashes the app — the exact class of failure this PR is trying to eliminate for shielded/platform-address/identity sync.This mirrors the existing
block_on_workerconvention (.expect("tokio worker panicked")), so it's consistent with established crate style, but it's worth considering whether sync failures should be surfaced as a gracefulPlatformWalletFFIResulterror instead of a hard abort, since the whole point of this refactor is avoiding crashes on the sync path.💡 Possible direction (illustrative, not a full fix)
-pub(crate) fn run_on_big_stack_thread<T: Send>(f: impl FnOnce() -> T + Send) -> T { - std::thread::scope(|scope| { - std::thread::Builder::new() - .name("pw-ffi-bigstack".into()) - .stack_size(WORKER_STACK_BYTES) - .spawn_scoped(scope, f) - .expect("failed to spawn big-stack FFI thread") - .join() - .expect("big-stack FFI thread panicked") - }) -} +pub(crate) fn run_on_big_stack_thread<T: Send>( + f: impl FnOnce() -> T + Send, +) -> std::thread::Result<T> { + std::thread::scope(|scope| { + std::thread::Builder::new() + .name("pw-ffi-bigstack".into()) + .stack_size(WORKER_STACK_BYTES) + .spawn_scoped(scope, f) + .expect("failed to spawn big-stack FFI thread") + .join() + }) +}Callers would then translate an
Errinto aPlatformWalletFFIResulterror rather than letting it abort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/runtime.rs` around lines 76 - 86, The `run_on_big_stack_thread` helper currently uses `.expect(...)` on both `spawn_scoped` and `join`, which can turn thread creation failures or panics inside `f` into a process abort when the error escapes an `extern "C"` FFI path. Update `run_on_big_stack_thread` to stop hard-panicking here and instead propagate failure back to the caller in a form the FFI entrypoints can convert into `PlatformWalletFFIResult`, using the existing `block_on_worker`/`PlatformWalletFFIResult` pattern as a reference. Keep the big-stack thread setup in `runtime.rs`, but make the failure path return an error rather than aborting.
63-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit test added for the new helper.
run_on_big_stack_threadhas no accompanying test in this diff. It's a small, self-contained utility (spawn scoped thread, run closure, join) that would be easy to unit test (e.g., verifying the return value round-trips and that the stack size is sufficient for deep recursion), improving confidence in this crash-fix helper.As per coding guidelines, "Place unit and integration tests alongside each package, and keep end-to-end tests in
packages/platform-test-suite."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/runtime.rs` around lines 63 - 87, Add a unit test for run_on_big_stack_thread in the same package to cover the new helper. The test should exercise the scoped thread path, verify the closure’s return value is propagated correctly, and ideally include a deep-recursion or large-stack case to confirm the 8 MB stack behavior. Keep the test alongside the runtime module so the helper’s spawn/join behavior is validated directly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/runtime.rs`:
- Around line 76-86: The `run_on_big_stack_thread` helper currently uses
`.expect(...)` on both `spawn_scoped` and `join`, which can turn thread creation
failures or panics inside `f` into a process abort when the error escapes an
`extern "C"` FFI path. Update `run_on_big_stack_thread` to stop hard-panicking
here and instead propagate failure back to the caller in a form the FFI
entrypoints can convert into `PlatformWalletFFIResult`, using the existing
`block_on_worker`/`PlatformWalletFFIResult` pattern as a reference. Keep the
big-stack thread setup in `runtime.rs`, but make the failure path return an
error rather than aborting.
- Around line 63-87: Add a unit test for run_on_big_stack_thread in the same
package to cover the new helper. The test should exercise the scoped thread
path, verify the closure’s return value is propagated correctly, and ideally
include a deep-recursion or large-stack case to confirm the 8 MB stack behavior.
Keep the test alongside the runtime module so the helper’s spawn/join behavior
is validated directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d72dec3f-5514-4580-8a1c-7a19aa42d8af
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/identity_sync.rspackages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet-ffi/src/runtime.rspackages/rs-platform-wallet-ffi/src/shielded_sync.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus (failed); claude general opus (failed); claude rust-quality opus (failed); claude security-auditor opus (failed); codex ffi-engineer gpt-5.5; codex general gpt-5.5; codex rust-quality gpt-5.5; codex security-auditor gpt-5.5; verifier = codex gpt-5.5; specialists = security-auditor, rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
The PR’s stack handoff is scoped to the FFI sync-now paths and the main design matches the stated goal of avoiding small Swift/dispatch caller stacks. I verified one in-scope error-handling issue in the newly added big-stack thread helper: per-call thread creation failure is currently turned into a panic across an extern "C" boundary instead of an FFI error.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/runtime.rs:81-84: Return an FFI error when the big-stack thread cannot be spawned
`run_on_big_stack_thread` is newly used by `platform_wallet_manager_platform_address_sync_sync_now`, an exported `extern "C"` function that otherwise reports failures through `PlatformWalletFFIResult`. If `spawn_scoped` fails under OS resource pressure, this `.expect(...)` panics on the FFI caller path; unwinding across the non-`C-unwind` boundary aborts the Swift host process. Thread creation is the new fallible operation introduced by this PR, so convert that `io::Error` into the existing FFI error channel at the sync-now call site instead of panicking. The `join().expect(...)` also re-panics through the FFI boundary if the worker panics; if that behavior is intentional for internal invariants, keep it explicit, but the spawn failure should be handled as a recoverable FFI failure.
…rror Review follow-up: run_on_big_stack_thread now returns io::Result instead of panicking on spawn failure, so the extern "C" address-sync entry point reports OS resource pressure through PlatformWalletFFIResult rather than aborting the host. A panic inside the closure still propagates (same convention as block_on_worker). Adds unit tests for the return-value round-trip and deep recursion past host-thread stack sizes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note on the failing |
|
The pre-existing wallet-test failures are root-caused and fixed in #4034: rust-dashcore#836 (in the #4022 bump) intentionally restricts asset-lock funding to final inputs, and the shared test fixture still funded via a mempool tx. Once #4034 lands on v4.1-dev, re-running this PR's checks should go green. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus (failed); claude general opus (failed); claude rust-quality opus (failed); claude security-auditor opus (failed); codex ffi-engineer gpt-5.5; codex general gpt-5.5; codex rust-quality gpt-5.5; codex security-auditor gpt-5.5; verifier = codex gpt-5.5; specialists = security-auditor, rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
Prior reconciliation: prior-1 is FIXED; current head returns std::io::Result<T> from run_on_big_stack_thread and maps thread-spawn failure to PlatformWalletFFIResult::err in platform_wallet_manager_platform_address_sync_sync_now. Carried-forward prior findings: none. New findings in latest delta: none; the cumulative PR remains scoped to moving FFI sync-now polling onto larger-stack execution paths.
2 participants
Issue being fixed or feature implemented
TestFlight users report the app instantly crashes when tapping "Sync Now" under Shielded Sync Status on the latest build (splawik21, hilawe, JGCMiner77 in the iOS channel).
Reproduced on-simulator with a matching crash log:
EXC_BAD_ACCESS / SIGBUS "Thread stack size exceeded"on acom.apple.root.user-initiated-qos.cooperativethread, faulting inside the shielded notes-sync future withplatform_wallet_manager_shielded_sync_sync_now→Runtime::block_onat the base of the stack.Root cause: the shielded (and platform-address / identity-token)
sync_nowFFI entry points poll their entire sync future viaruntime().block_on(...)on the host's calling thread. Swift calls them from dispatch/concurrency threads with ~512 KB of stack, and the sync future's recursion (DAPI request stack, GroveDB proof verification) has grown past it — the exact failure mode already documented and fixed for DashPay sync in #3841 ("SIGBUS observed on-device 2026-06-12", fixed withblock_on_worker). The background sync loop was unaffected because it polls on a dedicated 2 MBstd::thread, which is why only the button crashed.What was done?
platform_wallet_manager_shielded_sync_sync_nowand..._shielded_sync_walletnow useblock_on_worker, which parks the calling thread and polls the pass on the runtime's 8 MB-stack workers — same fixdashpay_sync_sync_nowalready ships.platform_wallet_manager_identity_sync_sync_now: same change (identical latent crash one tap away).platform_wallet_manager_platform_address_sync_sync_now: its future trips rustc's implied-lifetime-bound limitation (Unexpected higher-ranked lifetime error in GAT usage rust-lang/rust#100013) againstblock_on_worker'sSend + 'staticbounds, so added arun_on_big_stack_threadescape hatch inruntime.rsthat creates and polls the future entirely on a scoped 8 MB OS thread (noSend/'staticproof needed).No Swift-side changes; no behavior change beyond which thread polls the pass.
How Has This Been Tested?
cargo check -p platform-wallet-ffi --features shieldedand--all-features --all-targets; clippy clean;cargo fmt.build_ios.sh --target sim) and verified on the simulator that previously crashed (wallets with 1,142 synced shielded notes): tapped shielded Sync Now 4×, DashPay Sync Now, and Platform Sync Now — all passes complete (shielded "Queries Since Launch: 5 syncs", last-sync stamps update), app stays alive, no new crash reports in DiagnosticReports. Before the fix the same tap on the same data aborted the app instantly.Breaking Changes
None.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit