fix(platform-wallet): close asset-lock resume broadcast race

[thepastaclaw]

Follow-up to merged #3985 / tracker thepastaclaw/tracker#1742.

The later automated review on #3985 found a real read-before-broadcast race: resume_asset_lock could snapshot a Built row, then a concurrent create-path Rejected cleanup could delete the row and release the reservation before resume rebroadcasted the same tx.

This PR:

• Advances Built rows to Broadcast before the resume-side broadcast so cleanup guards preserve the row/reservation while a broadcast may be in flight.
• Rolls the row back to Built if the resume-side broadcast returns definite Rejected, so future resumes can try again instead of waiting forever on a tx that was not sent.
• Adds regression coverage for the cleanup/resume race and for the resume-side Rejected rollback.

Validation:

• cargo test -p platform-wallet --lib asset_lock::
• cargo clippy -p platform-wallet --lib --tests
• cargo build -p platform-wallet --tests
• git show --check HEAD

Note: the race regression test was also verified to fail against the pre-fix implementation.

[coderabbitai]

Important

Review skipped

Draft detected.

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

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fc91d3b9-1a02-49dc-bc84-5e4e8b632bc0

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

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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]

⛔ Blockers found — Sonnet deferred (commit d59ddc4)
Canonical validated blockers: 2

[thepastaclaw]

Preliminary review — Codex only

The pre-broadcast status promotion fixes the intended single-resume interleave, but the newly added rejection rollback is not owned by the resume attempt that performed the promotion. Concurrent resume calls can therefore downgrade another successful broadcast to Built, allowing the create-side rejection cleanup to delete the row and release inputs for a transaction that may already be propagating.

Validated blockers were found in the Codex precheck. Sonnet 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 — security-auditor (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:275-305: Rollback can clobber a concurrent successful resume
  The write lock protects only the individual status mutation, not the promote/broadcast/rollback sequence across the broadcast await. Two callers can resume the same `Built` lock: caller A promotes it to `Broadcast`, while caller B snapshots either `Built` or `Broadcast` and successfully broadcasts the transaction. If A then receives `Rejected`, this filter still sees `Broadcast` because B remains at that status while waiting for proof, so A downgrades the shared row to `Built`. If the original create-side broadcast subsequently returns `Rejected`, `build.rs:530-550` sees `Built`, removes the row, and releases the funding reservation even though B handed the transaction to the network. Concurrent calls are supported by the public Rust and FFI resume entry points, and neither the manager nor the catch-up path serializes operations per outpoint. Tie rollback to exclusive attempt ownership with a per-outpoint single-flight guard or generation token, or conservatively retain `Broadcast` on rejection; the existing `Broadcast` arm already re-broadcasts on future resumes.

Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.

[thepastaclaw]

Addressed the blocking resume-race finding in amended commit afe26111249b58a786d8a4edb960f29ac1a4ba6c:

• removed the unowned Broadcast -> Built rollback after a resume-side definitive rejection
• conservatively retain Broadcast, which avoids clobbering a concurrent successful resume and keeps the row/reservation protected
• updated the focused regression test to assert the retained Broadcast state, tracked row, no deletion, and held reservation

Validation: focused test passed; cargo check -p platform-wallet passed; the pre-commit workspace cargo check and rustfmt gate passed.

[thepastaclaw]

Preliminary review — Codex only

The prior rollback race is fixed: resume-side rejection no longer performs an unowned Broadcast -> Built mutation. One blocking concurrency issue remains because the new pre-broadcast promotion can use a stale Built snapshot to overwrite a proof-bearing final status and persist the regression.

Validated blockers were found in the Codex precheck. Sonnet 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 — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:269-285: Stale Built snapshot can overwrite a concurrently attached proof
  Two callers can both snapshot `Built` at lines 221-252. After the first caller promotes the row, broadcasts, obtains a proof, and stores `InstantSendLocked` or `ChainLocked` at lines 353-361, a delayed second caller still executes this pre-broadcast promotion. `advance_asset_lock_status` assigns the requested status unconditionally (`tracking.rs:180`), so it downgrades the finalized row to `Broadcast`; passing `None` leaves the existing proof attached. The later changeset is last-write-wins, so the inconsistent `Broadcast + Some(proof)` state can also be persisted. If the second broadcast returns `Rejected` or `MaybeSent`, these return paths retain that regression. A subsequent resume selects the `Broadcast` arm, ignores the attached proof, and waits for the transaction record again, potentially timing out or waiting indefinitely despite already having a valid proof. This failure path is introduced by moving the status write before broadcast: a stale caller whose broadcast fails now mutates shared state where it previously returned before the write. Make the `Built -> Broadcast` transition a compare-and-set under the wallet write lock; if the current row has already advanced, re-dispatch from its current status and use any attached proof instead of overwriting it.

Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.

[thepastaclaw]

Blocking finding addressed: stale Built snapshot overwriting a concurrently attached proof

Verified the finding from first principles and fixed it in cf4d33de00.

Diagnosis

resume_asset_lock snapshots the tracked row under a read lock and drops it before the pre-broadcast promotion, so two callers can both observe Built. If the first broadcasts, obtains a proof and finalizes the row to InstantSendLocked/ChainLocked (step 3), the delayed second caller still executed the unconditional Built -> Broadcast write — advance_asset_lock_status assigned the status regardless of the current value (tracking.rs:180), and proof: None left the existing proof attached. Result: a finalized row downgraded to the inconsistent Broadcast + Some(proof) state, persisted as well since changesets are last-write-wins. A later resume then took the Broadcast arm, ignored the attached proof, and waited again for a proof it already held — unbounded for the user-facing funding flows. The Rejected/MaybeSent return paths retained the regression rather than unwinding it.

Fix

• New promote_built_to_broadcast (sync/tracking.rs) — compare-and-set under the wallet write lock. Promotes only while the row is still Built; otherwise mutates nothing and returns the row's current status + proof via BuiltPromotion::AlreadyAdvanced.
• resume_asset_lock (sync/recovery.rs) re-dispatches from those values, so a stale caller takes the already-have-a-proof arm instead of overwriting it.
• An untracked outpoint still errors, preserving the existing guard against re-broadcasting a rejected row whose funding reservation was released (the race closed by the previous commit).
• advance_asset_lock_status keeps its unconditional semantics — its other call sites are monotonic proof-attaching advances — with a doc note pointing racing callers at the new helper.

Regression test

stale_built_resume_does_not_downgrade_a_concurrently_finalized_row drives the interleave deterministically through a two-sided test-only gate: the resume signals once it has taken its Built snapshot (proving the snapshot is genuinely stale), the test finalizes the row to ChainLocked with a proof, then releases the resume. It asserts the row stays ChainLocked with its proof intact, that no changeset persists a Broadcast + Some(proof) pair, and that the stale caller never re-broadcast.

Confirmed it is a real regression test: with the compare-and-set temporarily neutered it fails (stale caller downgrades the row and re-broadcasts); with the fix it passes.

Validation

Check	Outcome
cargo fmt -p platform-wallet -- --check	clean
cargo check -p platform-wallet	pass
cargo clippy -p platform-wallet --all-targets	no new warnings (2 pre-existing useless use of vec! in identity/network/withdrawal.rs, untouched)
cargo test -p platform-wallet --lib asset_lock	25 passed, 0 failed
cargo test -p platform-wallet --lib	471 passed, 0 failed
cargo check -p platform-wallet-ffi	pass (FFI consumer unaffected)

Scope kept to this concurrency issue and its regression test; unrelated issue #4227 not touched.

[thepastaclaw]

Follow-up: the create-side Built -> Broadcast writer had the same stale-promotion bug

d3345783d5 closes the last unconditional Built -> Broadcast writer. Review of the previous commit correctly flagged that fixing only the resume side left the create side able to reach the identical bad state.

Diagnosis

broadcast_funded_asset_lock tracks the row as Built (step 2), then awaits broadcaster.broadcast(&tx) — an unbounded network call — before assigning Broadcast on the way out (build.rs:556).

A concurrent resume_asset_lock can pick the same outpoint up inside that window. Two production drivers reach it for any tracked row:

• the FFI catch-up scanner, asset_lock_manager_catch_up_blocking (rs-platform-wallet-ffi/src/asset_lock/sync.rs:120)
• the funding resolver's FromExistingAssetLock arm (asset_lock/orchestration.rs:458)

Interleaving:

1. create: tracks row Built, enters broadcast(&tx) and parks.
2. resume: snapshots Built, CAS-promotes to Broadcast, broadcasts, gets a proof, finalizes to InstantSendLocked / ChainLocked with the proof attached (its step 3).
3. create: broadcast returns Ok, and advance_asset_lock_status(.., Broadcast, None) downgrades the finalized row. Because that method leaves the existing proof in place when passed None, the result is Broadcast + Some(proof) — and it gets persisted, changesets being last-write-wins.

A later resume then takes the Broadcast arm, ignores the attached proof and waits for one it already holds — unbounded for the user-facing funding flows. Same end state the resume-side fix addressed, reached from the other writer.

Fix

Route the create-side promotion through the same promote_built_to_broadcast compare-and-set. If the row already advanced, its status and proof are strictly further along than anything this call could write, so they are left untouched and the successful broadcast is still reported as success.

This create-only half deliberately does not re-dispatch or wait — unlike resume_asset_lock there is nothing to re-dispatch, since the proof wait lives in wait_for_funded_asset_lock_proof, the caller's next step. sync::tracking becomes pub(super) so build.rs can name BuiltPromotion.

All Built -> Broadcast writer audit

Site	Semantics	Safe?
build.rs:580 create promotion	now CAS via promote_built_to_broadcast	fixed here
recovery.rs:297 resume promotion	CAS via promote_built_to_broadcast	fixed in cf4d33de00
tracking.rs:232	the CAS itself — only assignment of Broadcast, guarded on status == Built under the write lock	by construction
build.rs:632 create proof attach	Some(proof), terminal IS/CL status	monotonic
recovery.rs:407 resume proof attach	Some(proof), terminal IS/CL status	monotonic
shielded/fund_from_asset_lock.rs:386	ChainLocked + Some(proof) (IS→CL upgrade)	monotonic
platform_addresses/fund_from_asset_lock.rs:243	ChainLocked + Some(proof) (IS→CL upgrade)	monotonic
recovery.rs:181,183	resolve_status_with_in_memory — classification for a first insert of an untracked row, guarded by contains_key re-check under the write lock (first writer wins)	not a promotion

advance_asset_lock_status retains its unconditional semantics; every remaining call site writes a terminal status with Some(proof) and never writes Broadcast. No unconditional Built -> Broadcast writer remains.

Regression coverage

create_broadcast_does_not_downgrade_a_row_finalized_during_the_broadcast drives the interleave deterministically: FinalizeDuringBroadcastBroadcaster finalizes the row to ChainLocked with a proof inside the broadcast await — standing in for the winning resume, inline rather than via a second task so the ordering is staged, not scheduler-dependent — then returns Ok. It asserts the row was Built on entry, so it fails loudly if the flow it targets ever changes shape.

The finalize is in-memory only, so any persisted Broadcast + Some(proof) changeset could only have come from the create path itself.

Verified against the old implementation — both assertions fire independently:

• status assertion: left: Broadcast, right: ChainLocked — "a row finalized during the broadcast must not be downgraded to Broadcast on the way out"
• with that assertion removed, the persistence assertion fires on its own: "no changeset may persist a Broadcast row with a proof attached"

Green with the compare-and-set. It also asserts exactly one broadcast call, pinning the "no re-dispatch, no re-broadcast" contract for this create-only method.

Validation

• cargo fmt --all --check — clean
• cargo check -p platform-wallet / -p platform-wallet-ffi — clean
• cargo clippy -p platform-wallet --all-targets — 2 warnings, both useless use of vec! in identity/network/withdrawal.rs (untouched by this change); confirmed identical at the base commit, so pre-existing
• focused asset-lock tests — 26/26 pass
• full platform-wallet lib suite — 472/472 pass
• pre-commit hook equivalents run manually (pre-commit is not installed locally): cargo fmt --all, trailing-whitespace and line-ending checks on all three changed files — clean

Still a draft; no scope beyond the flagged path (unrelated #4227 untouched).

[thepastaclaw]

Persistence-order race between status mutation and persist enqueue (dceaa4da93)

Follow-up to the CAS commits on this branch. The CAS closed the in-memory race; this closes the durable-state one that sat behind it.

Diagnosis — verified per layer

promote_built_to_broadcast mutated the row under wallet_manager.write(), returned a Broadcast changeset, and released the wallet lock before the caller enqueued it (build.rs, recovery.rs). That post-CAS/pre-enqueue window is unguarded, and nothing downstream repairs a reversal:

Layer	Behavior	Effect on a reversed pair
wallet_manager write guard	dropped on return from the mutator	window opens
queue_asset_lock_changeset → persister.store()	no sequencing beyond call order	reversal propagates
FFIPersister::store_round (persistence.rs:723)	store_round.lock() — serializes by acquisition order	faithfully preserves the reversal
AssetLockChangeSet::merge (changeset.rs:956)	extend = last-write-wins	stale Broadcast + None wins
Swift upsert (PlatformWalletPersistenceHandler.swift:187)	unconditional statusRaw / proofBytes overwrite	proof erased on disk
Swift load	filters statusRaw < 2	restart reloads the stale row and re-drives it

So a finalizer that reaches InstantSendLocked/ChainLocked + proof and enqueues after the promoter's CAS but before its enqueue leaves memory correct and durable state regressed.

Design — one shared ordering primitive, enqueue folded into the mutator

Added AssetLockManager::status_persist_serial: tokio::sync::Mutex<()> (mirrors the existing build_persist_serial precedent in the same struct), and moved queue_asset_lock_changeset inside each status mutator under that mutex. Patching call sites would have fixed one caller while leaving others free to reorder; folding the enqueue in makes the pairing structural.

Why a dedicated mutex rather than widening the wallet write guard: queue_asset_lock_changeset calls the persister synchronously, which reenters Swift over FFI on iOS. Holding wallet_manager.write() across host I/O would park every unrelated wallet reader behind it. The mutex is not held across the unbounded broadcast / wait_for_proof awaits.

Lock ordering, documented on the field and uniform across all holders:

status_persist_serial → wallet_manager.write() → drop(wallet) → enqueue → drop(serial)

Acquired before wallet_manager, never the reverse — no cycle.

Ordering audit — every mutation+enqueue path

Path	Transition	Covered by
track_asset_lock	insert Built	status_persist_serial
untrack_asset_lock	rejected-row removal	status_persist_serial
promote_built_to_broadcast	Built → Broadcast (create and resume callers)	status_persist_serial
advance_asset_lock_status	proof attachment; IS→CL upgrades incl. both external fund_from_asset_lock.rs sites	status_persist_serial
consume_asset_lock	terminal Consumed	status_persist_serial
recover_asset_lock_blocking (phase 3)	sync recovery	status_persist_serial.blocking_lock()

No path intentionally excluded.

Tests — negative control run

Two deterministic tests in build.rs, plus a durable_asset_lock helper on CapturingPersistence that replays changesets in arrival order with real downstream semantics (LWW merge, unconditional upsert, removal delete) so assertions reflect what a restart actually reads.

• promotion_cannot_enqueue_a_stale_snapshot_after_a_concurrent_finalize — parks a promoter on a #[cfg(test)] post-CAS gate, spawns a finalizer to ChainLocked + proof, sleeps 50ms to give it a real chance to win, releases the promoter; asserts both memory and durable state are ChainLocked with the proof retained.
• resume_promotion_enqueues_under_the_shared_ordering_mutex — holds the mutex, starts a resume, asserts nothing enqueued while blocked; releases and asserts the durable row advanced past Built. Pins the resume path to the same primitive.

Negative control (mutex temporarily disabled) — both fail with exactly the predicted symptom:

promotion_cannot_enqueue_a_stale_snapshot_after_a_concurrent_finalize
  left: Broadcast   right: ChainLocked
  "the durable row must not regress below the finalized in-memory status"

resume_promotion_enqueues_under_the_shared_ordering_mutex
  left: 3           right: 2

Mutex restored; both pass.

Validation

Check	Result
cargo fmt -p platform-wallet -- --check	clean
cargo test -p platform-wallet --lib asset_lock	28 passed, 0 failed
cargo test -p platform-wallet --lib	474 passed, 0 failed
cargo check -p platform-wallet-ffi	clean
cargo test -p platform-wallet-ffi --lib	181 passed, 0 failed (incl. concurrent_stores_never_interleave_rounds, persistence restore/round-trip)
cargo clippy -p platform-wallet --lib --tests	2 warnings, both pre-existing
repo pre-commit hook	✅ Rust checks passed

The 2 clippy warnings are useless use of vec! in wallet/identity/network/withdrawal.rs, an untouched file — confirmed pre-existing by stashing the change, re-running clippy (still 2), and unstashing.

Residual notes

• recover_asset_lock_blocking uses blocking_lock(), which panics if ever called from inside a tokio async context. This matches the pre-existing blocking_write in the same method and its documented contract, but it is a second place that contract is now load-bearing.
• The guarantee is per-AssetLockManager instance, which assumes one manager per wallet id — how PlatformWallet::new constructs it (shared across sub-wallets via Arc).

Scope held to this race: no changes to #4227 or broader persistence hardening.

6 files changed, 566 insertions(+), 60 deletions(-).

[thepastaclaw]

Preliminary review — Codex only

The published stale-Built finding is fixed: create and resume now use a write-locked compare-and-set, re-dispatch from the current row, and serialize each same-manager mutation with its persistence enqueue. Two blocking lifecycle races remain: terminal proof writers can downgrade ChainLocked to InstantSendLocked, and the new ordering mutex is not shared across stale and replacement manager handles for the same wallet ID. No same-instance lock-order inversion, reentrant persistence deadlock, async/blocking misuse, or uncovered production mutation path was found.

Validated blockers were found in the Codex precheck. Sonnet 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 — security-auditor (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 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/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs:379-382: Terminal proof updates can regress ChainLocked to InstantSendLocked
  `status_persist_serial` preserves mutation/enqueue order, but proof acquisition and validation happen before that mutex is acquired and this assignment remains unconditional. Caller A can obtain and validate an InstantSend proof while the transaction record still has InstantSend context, then be descheduled; after a ChainLock arrives, caller B can store `ChainLocked` with its stronger proof, and caller A can subsequently overwrite the row with `InstantSendLocked` and the stale proof. The mutex then guarantees that this downgrade is also the last durable write. This is reachable when create, resume, catch-up, or funding-resolution flows concurrently wait on the same outpoint, and it can restore a proof that Platform has already rejected or that later expires. Compare the requested transition with the current row while holding the lock, preserve `ChainLocked` over `InstantSendLocked`, and return or otherwise use the effective current proof so a losing caller does not continue with its stale local proof.

In `packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs:144-148: Per-manager persistence mutex does not serialize stale handles for the same wallet
  `status_persist_serial` is initialized separately in every `AssetLockManager::new`, but the FFI lifecycle permits multiple live manager generations for one wallet ID. `platform_wallet_get_asset_locks` stores an independent cloned `Arc` handle, while `platform_wallet_manager_remove_wallet` removes only the wallet from the Rust manager and does not invalidate subordinate handles. Re-importing the same mnemonic creates a fresh `AssetLockManager` for the same ID over the same shared `WalletManager` and persister; the stale manager also starts addressing the replacement wallet because every access is keyed only by `wallet_id`. The two managers can therefore mutate the same row under independent ordering mutexes: one can mutate a `Built` row to `Broadcast`, the other can store `ChainLocked + proof`, and the first can enqueue its older snapshot last. `FFIPersister::store_round` preserves that call order, so durable state regresses despite memory remaining finalized. Share the ordering primitive per wallet ID across manager generations, or generation-check and invalidate stale handles and in-flight operations.

In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:2218-2222: Mutex regression tests rely on scheduler timing
  The yield and 50 ms sleep give the spawned finalizer an opportunity to run but do not establish that it reached the mutex before the promoter is released. The resume test has the same issue at lines 2384-2402: if the spawned resume is not polled until after `drop(serial)`, its pre-release assertion passes even if the resume path bypasses `status_persist_serial`. The later durable-state checks only prove that the tasks eventually ran. Add a test-only notification immediately before each attempted mutex acquisition and wait for it before releasing the competing task or asserting blockage, making both negative controls independent of scheduler timing.

Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.

[thepastaclaw]

🔴 Blocking: Per-manager persistence mutex does not serialize stale handles for the same wallet

status_persist_serial is initialized separately in every AssetLockManager::new, but the FFI lifecycle permits multiple live manager generations for one wallet ID. platform_wallet_get_asset_locks stores an independent cloned Arc handle, while platform_wallet_manager_remove_wallet removes only the wallet from the Rust manager and does not invalidate subordinate handles. Re-importing the same mnemonic creates a fresh AssetLockManager for the same ID over the same shared WalletManager and persister; the stale manager also starts addressing the replacement wallet because every access is keyed only by wallet_id. The two managers can therefore mutate the same row under independent ordering mutexes: one can mutate a Built row to Broadcast, the other can store ChainLocked + proof, and the first can enqueue its older snapshot last. FFIPersister::store_round preserves that call order, so durable state regresses despite memory remaining finalized. Share the ordering primitive per wallet ID across manager generations, or generation-check and invalidate stale handles and in-flight operations.

_{source: ['codex']}

[thepastaclaw]

Resolved in c3ca845 — Per-manager persistence mutex does not serialize stale handles for the same wallet no longer present.

_{Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.}

[thepastaclaw]

Correction: this finding remains open at d59ddc4f. The sequential remove/re-import path is improved, but remove_wallet still drops the inner entry and the outer self.wallets entry in separate critical sections. A concurrent same-ID registration can install a replacement between them; the unqualified outer removal then removes that active replacement generation without deactivating it, allowing a later re-import to recreate managers with independent persistence mutexes. The preliminary review for this head carries the blocker forward with the current code path and evidence.

[thepastaclaw]

Deterministic rendezvous for the two ordering tests

Follow-up addressing the one blocking review finding: two concurrency tests used a 50 ms sleep as their only evidence that the competing task had reached the status-persistence lock boundary.

Diagnosis (verified against dceaa4d before changing anything)

Both sites relied on elapsed time, which is not a rendezvous guarantee:

• durable_row_after_promote_finalize_interleave (build.rs ~2218) — yield_now() + 50 ms sleep, then released the parked promoter regardless of whether the finalizer had run. If the finalizer had not been scheduled, even the pre-fix implementation could enqueue in the non-regressing order and the test would falsely pass.
• resume_promotion_enqueues_under_the_shared_ordering_mutex (build.rs ~2384) — read "no new changeset after 50 ms" as proof the resume was blocked on the mutex. An unscheduled resume produces identical silence, including the case where the resume path stopped taking the mutex altogether — the exact regression the test exists to catch.

Mechanism

A test-only waiter gauge on status_persist_serial, maintained by a new lock_status_persist_serial() helper that all five async acquirers now route through. Incremented before the lock().await, RAII-dropped on acquisition — so a non-zero count means a task is queued at the boundary and cannot get past it while the test holds the lock. RAII (not a bare decrement) so a cancelled future can't leave a phantom arrival behind.

The key property: because the counter lives inside the lock helper, an implementation that stops taking the mutex never increments it, so the waiting test fails loudly instead of passing on silence.

• Test 2 now waits on await_status_serial_waiters(1) before asserting nothing was enqueued.
• The interleave helper waits for the finalizer to either enqueue or block on the mutex — mirroring the existing build_serial_gate test's regressed-or-serialized pattern — so the release happens only after the finalizer provably had its chance to go first.

No sleeps are used as proof of task progress. Production behavior is unchanged; the gauge and helper compile out of non-test builds (#[cfg(test)] / #[cfg(not(test))]).

Validation

• cargo test -p platform-wallet --lib asset_lock — 28 passed, repeated 5× with no flakes
• cargo test -p platform-wallet --lib — 474 passed, 0 failed
• cargo test -p platform-wallet-ffi --lib — 181 passed, 0 failed; cargo check -p platform-wallet-ffi clean (the changed helper is in shared production code)
• cargo fmt -p platform-wallet --check clean; cargo clippy --all-targets adds no new warnings (the 2 remaining useless use of vec! are pre-existing in withdrawal.rs)

Negative control — removing the five status_persist_serial acquisitions makes both tests fail 5/5 runs, each for the intended reason:

• test 1 on the real durable-row ordering assertion (left: Broadcast, right: ChainLocked), reached deterministically via the observed-enqueue branch;
• test 2 on the new rendezvous panic (timed out waiting for 1 task(s) to block on status_persist_serial (saw 0)) — precisely the case the sleep version misread as success.

Serialization was restored and the full suite re-run green afterwards.

Commit

c3ca845a30 — test-only; scoped to build.rs, manager.rs, sync/tracking.rs. Fast-forward over dceaa4d; no prior commits reset or dropped. Unrelated Platform #4227 deliberately untouched.

🤖 Generated with Claude Code

[thepastaclaw]

Exact-head finding confirmed and fixed — d59ddc4f53

Verified the published finding from f93f011925 before editing. It is real.

Diagnosis

advance_asset_lock_status is the single write point for asset-lock status, and at f93f011925 it assigned unconditionally:

entry.status = new_status;
if proof.is_some() {
    entry.proof = proof;
}

Serialization does not close this. status_persist_serial makes each writer's mutation + persist enqueue one indivisible unit, so durable order matches in-memory order — but the two proof-bearing writes come from independent waiters, and nothing orders them against each other. wait_for_proof returns whichever SPV event fires first, while the IS→CL upgrade paths (upgrade_to_chain_lock_proof after a Platform InvalidInstantAssetLockProofSignature rejection, validate_or_upgrade_proof on a rotated quorum) can complete a ChainLocked write with an earlier IS waiter still parked. Released afterwards, that IS waiter's write is internally consistent and perfectly serialized — and still regresses the row. The mutex serialized the downgrade rather than preventing it, exactly as reported.

Both halves of the regression matter:

• Status falls below the >= InstantSendLocked predicates the catch-up scanner and ready-to-fund UI filter on.
• Proof is worse: the caller passes Some(is_proof), so the ChainLock proof — the one that survives quorum rotation, and the one an IS-rejection retry had upgraded to — is replaced by the IS proof that rejection was about. A restart reloads the weaker proof from the durable row and re-arms the very rejection the upgrade resolved.

All advance_asset_lock_status callers were traced first: create-side finalize (build.rs), resume finalize (sync/recovery.rs), and the two post-IS-rejection ChainLock upgrades (shielded/fund_from_asset_lock.rs, platform_addresses/fund_from_asset_lock.rs).

Transition semantics

The guard sits under the same status_persist_serial the mutation and enqueue already hold, so the status it compares against is provably the one it is about to overwrite — no writer can finalize between comparison and assignment. A strictly-lower-rank new_status mutates nothing, replaces no proof, and enqueues nothing.

Ordering comes from an explicit AssetLockStatus::lifecycle_rank — an exhaustive match on named variants, not as u8 on declaration order. The two agree today, but that declaration order is separately load-bearing for the FFI discriminants (status_from_u8) and the SQLite label domain, and this is a semantic claim about the lifecycle; tying it to declaration position would let a reordering silently redefine which writes count as downgrades. No _ arm, so a new variant is a compile error here.

ChainLocked cannot regress to InstantSendLocked; Consumed cannot regress at all.

Caller-return semantics are deliberately unchanged. A refusal is Ok(empty changeset), not an error. Every production caller returns/uses the proof it already obtained, so the delayed IS caller may still submit with its own valid IS proof — that proof is not stale for submission, it simply must not overwrite shared state. Only the in-memory row and the durable row are held at the stronger proof. This is why no create/resume signature churn was needed: the losing caller's local proof remains correct to use, and the shared row is what had to be protected. Equal-rank writes are not refused, keeping two live shapes working: attaching the first proof to a row resolve_status_with_in_memory marked InstantSendLocked with None, and re-writing ChainLocked with a freshly-upgraded proof at a newer height.

Deterministic regression evidence

No sleeps in either test.

• a_late_instant_send_write_cannot_downgrade_a_chain_locked_row parks an IS writer on a test-only gate placed before the ordering mutex (a gate after it would invert the interleave — the parked writer would hold the lock and its IS write would land first, a legal forward advance), awaits the writer's arrived signal, finalizes the same row to ChainLocked + chain proof and awaits that call so the stronger write is provably complete, then releases the IS writer. Asserts in-memory and durable state both stay ChainLocked with the chain proof, that nothing extra was enqueued, and that the refusal is Ok with an empty changeset. The gate is one-shot so the competing finalize — same method — runs straight through instead of deadlocking against the gate it races.
• monotonicity_guard_allows_forward_and_same_status_proof_writes pins the other direction: forward transitions, equal-rank proof attachment, and equal-rank proof refresh all still mutate and enqueue.

Negative controls (each run, then restored):

Control	Result
Guard disabled (if false && …)	FAILS at build.rs:2873 — empty-changeset assertion
…empty-changeset assertion also neutralized	FAILS at build.rs:2892 — in-memory left: InstantSendLocked, right: ChainLocked
…in-memory assertions also neutralized	FAILS at build.rs:2902 — durable left: InstantSendLocked, right: ChainLocked
Guard written <= instead of <	FAILS monotonicity_guard_allows_… on equal-rank proof attachment

So the first test fails three times over, independently, on the exact state the finding describes, and the <= control shows the boundary is deliberate rather than incidental. Guard restored; tree clean.

Validation

• cargo test -p platform-wallet --lib asset_lock → 34 passed, 0 failed
• cargo test -p platform-wallet --lib → 480 passed, 0 failed
• cargo test -p platform-wallet --lib --all-features → 607 passed, 0 failed (covers the shielded ChainLock-upgrade caller, which default features exclude)
• cargo test -p platform-wallet-ffi → 181 + 26 + 5 passed, 0 failed (4 network-gated ignored)
• cargo check -p platform-wallet-ffi → clean
• cargo fmt -p platform-wallet -- --check → clean
• cargo clippy -p platform-wallet --all-targets → 2 warnings, both useless use of vec! at wallet/identity/network/withdrawal.rs:229,268 — pre-existing, in a file no commit in this PR range touches
• cargo check -p platform-wallet --no-default-features → fails with 4 errors in wallet/provider_key_at_index.rs. Pre-existing: reproduced at PR base afe2611124 (5 errors there), and the file is untouched by this PR.

Cumulative diff afe2611124..d59ddc4f53 is 10 files, all under packages/rs-platform-wallet/src/. No stray files; nothing touching #4227 or broadening FFI lifecycle/error-code work. Scope is monotonic asset-lock status/proof writes only; the manager-lifecycle and rendezvous fixes from prior commits are unchanged.

Commit

d59ddc4f530c8084b6a806c96e9e38ba96a6e1bb — fast-forward descendant of f93f011925, 4 files, +606/−5:

packages/rs-platform-wallet/src/wallet/asset_lock/build.rs         +378
packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs        +39
packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +104/−5
packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs        +90

Residual risk

The guard protects the row against regression, not against two callers legitimately writing at equal rank — by design, since that is how a None-proof InstantSendLocked row ever gets a proof. Last-writer-wins still applies among equal-rank proof writes; both are valid proofs for the same status, so this is not believed to be exploitable, but it is the remaining unordered case.

PR left in draft.

[thepastaclaw]

Preliminary review — Codex only

The lifecycle-rank guard correctly prevents delayed proof downgrades while preserving every forward transition and equal-rank proof attachment or refresh. Two blocking lifecycle gaps remain: concurrent registration can recreate an active stale manager generation, and retired managers can still mutate replacement-wallet state through transaction-building paths. The ordering tests now use a real mutex-arrival signal, but one rendezvous can miss an early enqueue and hang indefinitely.

Validated blockers were found in the Codex precheck. Sonnet 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 — security-auditor (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking | 🟡 1 suggestion(s)

2 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/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:653-657: Per-manager persistence mutex does not serialize stale handles for the same wallet
  The new deactivation barrier closes the sequential remove/re-import path, but removal still updates the inner `WalletManager` and `self.wallets` in separate critical sections. After the old inner entry is removed at lines 643-651, a concurrent same-seed registration can insert its replacement into the inner manager and then install its new `PlatformWallet` in `self.wallets` at registration lines 514-518. This unqualified `remove(wallet_id)` can then remove and return that replacement rather than the old, already-deactivated generation. The replacement remains active and present in the inner manager but absent from the public map. A later idempotent removal skips deactivation because the outer entry is missing, removes the inner entry, and reports success; another same-ID import then lets the retained active generation address the new replacement by `wallet_id`, recreating two managers with independent persistence mutexes and the original stale snapshot/delete race. Serialize registration and removal per wallet ID, or make both removals generation-aware so they can only detach the exact wallet generation that was captured and deactivated.

In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:67-70: Retired managers can still build against a replacement wallet
  `build_asset_lock_transaction` resolves mutable wallet state solely by the deterministic `wallet_id`, without an activity or generation check synchronized with `deactivate`. The public FFI function `asset_lock_manager_build_transaction` calls this method directly, so a retained handle can build against a same-mnemonic replacement after retirement. The builder advances the replacement's funding address index and reserves its UTXOs, and with the same seed can return a signed transaction spending those inputs. The funded path has the same race: it checks `active` before awaiting `build_persist_serial`, so an operation can pass the check, remain parked while removal and re-import complete, then build against and persist an account-pool snapshot from the replacement before `track_asset_lock` finally rejects the inactive manager. Because no lifecycle row is tracked, those reservations are orphaned. Put build mutation and account-pool persistence inside a generation-safe retirement barrier, including an authoritative check after waiting for `build_persist_serial` and before any wallet lookup.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:2269-2289: Finalizer rendezvous can miss an early enqueue and wait forever
  The persistence baseline is captured after the finalizer task is spawned. Under the intended negative control where the status mutex is removed, the finalizer can enqueue before `queued_before_finalize` is read; that enqueue is then included in the baseline, the waiter gauge remains zero, and this unbounded loop can never observe either exit condition. A finalizer panic or early return causes the same hang. Capture the persistence count before spawning the finalizer, bound the rendezvous with `tokio::time::timeout`, and surface a finalizer task that has already completed without producing either signal.

Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.

[thepastaclaw]

🟡 Suggestion: Finalizer rendezvous can miss an early enqueue and wait forever

The persistence baseline is captured after the finalizer task is spawned. Under the intended negative control where the status mutex is removed, the finalizer can enqueue before queued_before_finalize is read; that enqueue is then included in the baseline, the waiter gauge remains zero, and this unbounded loop can never observe either exit condition. A finalizer panic or early return causes the same hang. Capture the persistence count before spawning the finalizer, bound the rendezvous with tokio::time::timeout, and surface a finalizer task that has already completed without producing either signal.

_{source: ['codex']}