[llbartekll]

Issue being fixed or feature implemented

asset_lock_manager_catch_up_blocking and asset_lock_manager_resume poll resume_asset_lock with a bare runtime().block_on(...) on the caller's thread. Hosts call them from dispatch / Swift-concurrency threads with ~512 KB of stack (PlatformWalletManager.catchUpStuckAssetLocks runs the catch-up from Task.detached at every launch).

With a dev-profile FFI (build_ios.sh --profile dev, what simulator builds use) that overflows: the app faults in the stack guard (EXC_BAD_ACCESS, "Could not determine thread index for stack guard region", frames resume_asset_lock_with → Runtime::block_on → asset_lock_manager_catch_up_blocking → PlatformWalletManager.runCatchUp) on every launch that finds a Built/Broadcast asset lock. The app cannot be started again until the lock is resolved some other way. Found while testing dashpay/dashwallet-ios#1251 (app killed mid Core → Shielded drain, then relaunched).

This is the same class of bug as #4033 (shielded Sync Now crashing TestFlight builds) and 1264957560 (identity registration); the crate documents it in rs-platform-wallet-ffi/src/runtime.rs and provides block_on_worker / run_on_big_stack_thread for it. These two entry points were left on the caller's stack.

Does it affect release builds?

Not today, by measurement. Prologue frame sizes along the crashing call chain (dev: the dev-ios sim xcframework at 84851397d7; release: a release-ios sim library built from a descendant with no changes in rs-platform-wallet/src/wallet/asset_lock, rs-platform-wallet-ffi/src/asset_lock or runtime.rs):

Frame	dev	release
resume_asset_lock_with::{closure} poll	~448 KiB	~66 KiB
resume_asset_lock::{closure} poll	~56 KiB	~3 KiB
tokio Runtime::block_on + park/enter	~61 KiB	~12 KiB
FFI entry + closures	~50 KiB	~37 KiB
Total up to the fault point	~619 KiB	~118 KiB

The deeper release calls on this path (wait_for_proof, await_broadcast_ready, SPV broadcast, proof upgrade/validate) are ≤ ~17 KiB each, and no GroveDB proof verification is involved, so release builds keep a wide margin. The fix is therefore about dev builds and about following the crate's own convention, not an urgent production crash.

What was done?

• asset_lock_manager_catch_up_blocking and asset_lock_manager_resume run the resume_asset_lock poll inside run_on_big_stack_thread (8 MB stack), as platform_wallet_manager_platform_address_sync_sync_now does since fix(platform-wallet): poll Sync Now FFI passes on big-stack threads to stop SIGBUS crash #4033. block_on_worker is not usable because the future borrows manager and out_point (not 'static).
• A failure to spawn the thread is returned as ErrorWalletOperation (the catch-up's Swift caller already treats that code as a retryable miss).
• No Swift changes. (An earlier revision of this PR worked around it in swift-sdk with a large-stack Thread; that is reverted in favour of fixing it at the source for every host.)

How Has This Been Tested?

• cargo check, cargo clippy --all-targets -D warnings and cargo fmt --check on platform-wallet-ffi.
• iOS Simulator (iPhone 17, testnet), dashwallet-ios linked against a dev-profile sim xcframework built from this branch, unchanged swift-sdk:
  • Started a Core → Shielded Max drain and killed the app 3 s after SPV broadcast accepted, leaving the asset lock persisted at Broadcast (statusRaw = 1), the state that crashed every launch with the previous FFI (two crash reports with the stack above).
  • Relaunch: no crash. asset_lock_manager_catch_up_blocking: entered → resume_asset_lock: lock looked up … status=Broadcast → bounded transport wait → resume_asset_lock failed (deferred retry, expected) returned to Swift; deferred resume: completed once the transport came up 50 s later, lock InstantSendLocked.
  • "Finish now" then completed the shield (Shielded fund-from-asset-lock succeeded), lock DrainCompleted. No crash report at any point.

Breaking Changes

None.

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation (inline)

🤖 Generated with Claude Code

PR Hygiene · 9038a51

• Bots — thepastaclaw ✓
• Self-review — posted; again after any push
• Build green
• Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when resuming or catching up asset lock operations by running the work on a separate thread. If that work cannot be started, the operation returns an error.
  • Existing timeout behavior remains unchanged.
• Documentation
  • Clarified that catch-up blocks the calling thread for up to the specified timeout while the resume work runs separately.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: dashpay/platform
• Branch: claude/asset-lock-catch-up-large-stack
• File: .coderabbit.yaml
• Commit: 9038a51
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 096932bc-b851-4eae-9909-86986135a5a9

Commits

Reviewing files that changed from the base of the PR and between 4e32be5 and 9038a51.

Files selected for processing: 1

• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The resume and catch-up FFI entry points now poll asset locks on a big-stack thread while the caller blocks. Both return ErrorWalletOperation if thread spawning fails. Test builds forward timeout-probe records to the caller. Rust and Swift documentation describes the blocking behavior.

Changes: Asset-lock polling

Layer / File(s)	Summary
Big-stack polling wrapper packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs	A new wrapper runs a closure on a big-stack thread. In test builds, it also forwards timeout-probe records to the calling thread.
Resume and catch-up entry points packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift	Both entry points use the wrapper to poll asset locks. If thread spawning fails, they return ErrorWalletOperation; catch-up also logs the failure. Rust and Swift documentation states that the caller blocks while polling runs on the big-stack thread.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant FFIEntryPoints
  participant resume_on_big_stack
  participant BigStackThread
  participant resume_asset_lock
  FFIEntryPoints->>resume_on_big_stack: submit polling closure
  resume_on_big_stack->>BigStackThread: run closure
  BigStackThread->>resume_asset_lock: poll asset lock
  resume_asset_lock-->>BigStackThread: return polling result
  BigStackThread-->>FFIEntryPoints: return result or spawn failure

Loading

Possibly related PRs

• dashpay/platform#5581: Changes proof-backed asset-lock consumption and persisted drain recovery, which share the asset-lock recovery flow.

Suggested reviewers: thepastaclaw, hashengineering

Merge Risk: Minimal · up to 9038a

No actionable merge-blocking risk was identified.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main change: the asset-lock resume FFIs now poll on a big-stack thread. The raw summary and objectives confirm that asset_lock_manager_catch_up_blocking and asset_lock_manager_resu…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: the asset-lock resume FFIs now poll on a big-stack thread. The raw summary and objectives confirm that asset_lock_manager_catch_up_blocking and asset_lock_manager_resume poll resume_asset_lock via run_on_big_stack_thread. The title uses the conventional fix(scope) prefix, is one sentence, and avoids file lists or vague terms. It omits the stack-overflow motivation and the Swift documentation update, which is acceptable.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 2 files.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · 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 9038a51) · triage: normal

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The production change correctly moves both asset-lock resume polls onto the existing 8 MB stack helper and preserves the manager handle lifetime. However, the exported-path timeout tests now record through thread-local storage on the spawned big-stack thread and read it on the caller thread, so the three affected tests observe an empty vector and fail. The Rust workspace check is still queued, but this deterministic test regression independently requires changes.

🔴 1 blocking

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

🔴 Blocking: Preserve timeout probe data across the spawned big-stack thread
packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs:69-80

forwarded(timeout) is now evaluated inside run_on_big_stack_thread at the exported resume and catch-up call sites (lines 205-207 and 299-301). Its timeout_probe::record call therefore writes to the spawned thread's thread-local RECORDED buffer, while resume_extern_with and catch_up_extern_with call timeout_probe::take() on the original test thread (lines 502 and 516). Those are different thread-local instances, so the caller observes []; the three exported-path tests expecting one_year(), None, and Some(300s) fail, and the extreme-timeout tests fail before their subsequent indexing. Make the probe cross-thread, or return the recorded value from the big-stack closure and assert it on the caller thread.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality)}

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: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a small, contained Rust concurrency and FFI error-propagation change in asset-lock resumption, but it changes runtime behavior and thread-stack execution rather than being merely trivial.
• 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 quota lookup failed), 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 — rust-quality (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/asset_lock/sync.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs:69-80: Preserve timeout probe data across the spawned big-stack thread
  `forwarded(timeout)` is now evaluated inside `run_on_big_stack_thread` at the exported resume and catch-up call sites (lines 205-207 and 299-301). Its `timeout_probe::record` call therefore writes to the spawned thread's thread-local `RECORDED` buffer, while `resume_extern_with` and `catch_up_extern_with` call `timeout_probe::take()` on the original test thread (lines 502 and 516). Those are different thread-local instances, so the caller observes `[]`; the three exported-path tests expecting `one_year()`, `None`, and `Some(300s)` fail, and the extreme-timeout tests fail before their subsequent indexing. Make the probe cross-thread, or return the recorded value from the big-stack closure and assert it on the caller thread.

[github-actions]

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix.
Full checklist in the description.

[llbartekll]

Fixed in 7e477ca — thanks, valid. Both resume polls now go through resume_on_big_stack, which under cfg(test) takes the timeout_probe records on the big-stack thread and appends them on the calling thread, so forwarded(timeout) stays in argument position and the probe stays per-test; production code is unchanged. cargo test -p platform-wallet-ffi: 428 passed (incl. the_exported_resume_clamps_an_extreme_timeout, the_exported_catch_up_clamps_an_extreme_timeout, the_exported_paths_leave_ordinary_timeouts_alone); clippy --all-targets -D warnings and fmt clean. (My earlier push ran clippy but not the crate tests — my miss.)

🤖 Addressed by Claude Code

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

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

The head correctly moves both asset-lock resume polls onto the existing 8 MiB scoped-thread helper and carries test-only timeout observations back to the caller, resolving the prior blocking regression. I found one in-scope documentation nit: a test helper still describes polling on the caller's stack even though the implementation now polls on the spawned big-stack thread. No blocking code issues were found; the relevant CI setup checks remain queued or in progress.

💬 1 nitpick(s)

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

💬 Nitpick: Update stale test-helper documentation after moving the poll off the caller
packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs:504-506

The comment says that this entry point performs runtime().block_on(...) on the host thread, but the current implementation routes the call through resume_on_big_stack, so the future is polled on the scoped 8 MiB thread and the caller only waits for that thread to join. The guidance not to wrap this synchronous helper in an outer block_on remains correct, but the explanation should describe the new execution model.

    /// Called from the plain test thread: the exported entry point blocks
    /// synchronously while `resume_asset_lock` is polled on the scoped
    /// big-stack thread. Do not invoke it from an outer `block_on`.

_{source: glm-5.3-flash (phase1-reviewer: general, rust-quality)}

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (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: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — This is a small, contained Rust threading and error-propagation change that preserves asset-lock behavior while moving polling to the established large-stack execution pattern.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — rust-quality (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 99% left, weekly 97% left; passed over gemini-3.8-flash-high (antigravity quota lookup failed)
• 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 medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); 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/asset_lock/sync.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs:504-506: Update stale test-helper documentation after moving the poll off the caller
  The comment says that this entry point performs `runtime().block_on(...)` on the host thread, but the current implementation routes the call through `resume_on_big_stack`, so the future is polled on the scoped 8 MiB thread and the caller only waits for that thread to join. The guidance not to wrap this synchronous helper in an outer `block_on` remains correct, but the explanation should describe the new execution model.

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.

• Big-stack spawn-error handling is now copy-pasted across three entry points — The duplicated error handling is small, explicit, and preserves entry-point-specific context and logging. It does not create a correctness problem, and consolidating it would be routine refactoring outside the focused stack-safety fix.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[llbartekll]

@thepastaclaw thanks, fixed in 4e32be5. The test helper now says the entry point blocks while resume_asset_lock is polled on the scoped big-stack thread, using your wording. Two public doc comments in the same file had the same stale "runtime().block_on(...) on the calling thread" description, on the asset_lock_manager_resume timeout notes and on asset_lock_manager_catch_up_blocking. They now say the caller blocks while the poll runs on the big-stack thread. Doc-only change; cargo fmt --check and cargo test -p platform-wallet-ffi asset_lock pass.

On the out-of-scope note about the spawn-error handling repeated across three entry points: agreed it is small and explicit, so I left it as is in this PR.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

@llbartekll on it — I've queued this as job cj-20261027-161205-f3c8 and will reply here or link a PR when there's something to look at.

[thepastaclaw]

Thanks for addressing the documentation nit. reviewsys handles the re-review on the updated head.

[thepastaclaw]

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

The implementation correctly moves both asset-lock resume polls onto the existing big-stack thread helper, preserves synchronous FFI behavior, and forwards thread-spawn failures through the typed result. One stale Swift documentation comment still says that runtime().block_on(...) parks the caller, although the poll now runs on the big-stack thread; this is a documentation-only nitpick. No relevant code check is failing, though Kotlin CI and changed-package determination were still pending in the supplied snapshot.

💬 1 nitpick(s)

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

💬 Nitpick: Swift catchUpBlocking doc still names the removed bare runtime().block_on mechanism
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift:299-301

This documentation describes the caller as being parked by Rust's runtime().block_on(...), but the updated FFI now polls resume_asset_lock on the scoped big-stack thread and blocks the caller while waiting for that thread to join. The guidance to schedule this method on a background queue remains correct, but the mechanism should be updated to match the implementation.

    /// Blocking: the Rust side polls the resume on its own big-stack
    /// thread while this call blocks the calling thread for up to
    /// `timeoutSeconds`. Callers MUST schedule this on a background
    /// queue / `Task.detached`.

_{source: glm-5.3-flash (phase1-reviewer: general)}

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (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: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — This is a small, well-contained Rust FFI change that moves an existing poll onto a larger stack and propagates thread-spawn failure without altering wallet or consensus behavior.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — rust-quality (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 98% left, weekly 96% left; passed over gemini-3.8-flash-high (antigravity quota lookup failed)
• 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 medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift`:
- [NITPICK] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift:299-301: Swift catchUpBlocking doc still names the removed bare runtime().block_on mechanism
  This documentation describes the caller as being parked by Rust's `runtime().block_on(...)`, but the updated FFI now polls `resume_asset_lock` on the scoped big-stack thread and blocks the caller while waiting for that thread to join. The guidance to schedule this method on a background queue remains correct, but the mechanism should be updated to match the implementation.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[llbartekll]

Fixed the Swift doc nitpick in 9038a51, using the suggested wording: catchUpBlocking now says the Rust side polls the resume on its own big-stack thread while the call blocks the calling thread. That was the last doc comment in swift-sdk describing these two entry points with the old bare runtime().block_on(...) mechanism. Doc-only change.

[github-actions]

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

[thepastaclaw]

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

The exact-head diff correctly moves both asset-lock resume polls onto the existing scoped 8 MiB thread while preserving synchronous caller blocking, timeout forwarding, handle lifetime, and error mapping. The timeout probe and Rust/Swift documentation updates are correct, and no in-scope defects remain; code-related CI jobs are still queued, with no failures reported.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

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: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — This is a contained runtime and FFI-threading fix around asset-lock resume operations, but it changes blocking behavior and error propagation at a cross-language wallet boundary without altering funds movement, consensus, cryptography, or key handling.
• 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 quota lookup failed), 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 — rust-quality (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 — rust-quality (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[llbartekll]

/self-reviewed

[github-actions]

Policy satisfied — this can merge.
Full checklist in the description.

[romchornyi]

✅ No blockers · 5 non-blocking recommendations — reviewed at 9038a518ff

Level	Verdict
1 · Idea	✅ valid	A real crash on every launch for dev-profile sim builds that find a Built/Broadcast lock. It was reproduced, and the release margin was measured honestly.
2 · Architecture	✅ right layer	Fixed at the FFI source, following the crate's own runtime.rs convention. The swift-sdk workaround was reverted.
3 · Completeness	✅ complete for reachable callers	Two sibling entry points still poll on the caller's stack, but nothing in swift-sdk or dashwallet-ios calls them today (rec. 1).
4 · Code	✅ clean	Caller blocking, timeout forwarding, handle lifetime and error mapping are preserved. A spawn failure maps to a retryable ErrorWalletOperation.

5 non-blocking recommendations

1. The same class remains in build.rs. asset_lock_manager_build_transaction and asset_lock_manager_create_funded_proof still drive the build → broadcast → proof-wait chain with a bare runtime().block_on on the caller's stack. Only their own swift-sdk wrappers call them, and the app does not, so this is not reachable today. Please wrap them the same way or file an issue, so the next caller does not hit the dev crash.
2. The block_on_worker rationale does not hold. sync.rs#L326 says the future borrows manager and out_point. The storage holds an Arc<AssetLockManager> and OutPoint is Copy, so let m = Arc::clone(manager); block_on_worker(async move { m.resume_asset_lock(&out_point, t).await }) compiles. The same future already runs on tokio workers: the deferred retry in recovery.rs, and the tokio::spawn in the build.rs tests. Today every resume spawns a fresh 8 MB OS thread instead of using a pooled worker, and the comment will steer future call sites to the costlier helper.
3. The storage read guard is held for the whole wait. This predates the PR. with_item keeps the parking_lot read guard while the caller blocks, for up to the 300 s catch-up timeout. parking_lot is writer-fair, so an insert or remove on ASSET_LOCK_MANAGER_STORAGE (wallet load or teardown) waits for that time, and every other asset-lock FFI call queues behind the writer. The Arc::clone from rec. 2 fixes this as well: take the clone inside with_item and block after the guard is dropped. (The guard is verified; whether an insert happens on the main thread during the catch-up is not.)
4. No test pins the fix. If either entry point went back to a bare block_on, every test would still pass. A cheap regression test: call the entry point from a std::thread::Builder::new().stack_size(512 * 1024) thread under cfg(debug_assertions).
5. Root cause, for later. The fix covers entry points one at a time, while the root is the dev-profile poll frame of resume_asset_lock_with (~448 KiB). Box::pin on its large inner awaits would cover every caller at once. (not verified)

Review details

Layered review: idea, architecture and completeness inline, plus a /code-review high lane on the full diff. I checked the remaining runtime().block_on sites in rs-platform-wallet-ffi/src/asset_lock/ and their swift-sdk and dashwallet-ios callers. Recommendations 1–4 are verified in code at this head. No local build.

🤖 Reviewed with Claude Code