Follow-up to #1230, which ported the 19 read-only FFI endpoints onto with_initialized_lightclient_read. The PR's caveat stands: readers no longer contend with each other, but any window where an endpoint holds LIGHTCLIENT.write() still stalls all of them. This issue records the map of those windows, the agreed criterion for when a stall counts as a problem, and the instrumentation and test work that would answer whether the residual windows matter in practice.
The write-hold windows, mapped
Reading dev and the pinned zingolib (zingolib/src/lightclient/sync.rs, our rust/lib/src/lib.rs):
The sync engine itself never touches LIGHTCLIENT. It runs as a spawned tokio task holding the wallet lock. The windows where sync machinery holds LIGHTCLIENT.write() are the FFI calls that drive it:
- Steady-state
poll_sync (the UI calls it on a 5 s interval): the hold is a now_or_never() on the sync JoinHandle. Microseconds, no wallet-lock await.
- Launch (
run_sync): LightClient::sync() awaits wallet().read() to clone the sync config, spawns the task, then spins at 50 ms until the task reports Running, bounded by SYNC_START_TIMEOUT (3 s). The whole wait happens under LIGHTCLIENT.write(). The 5 s re-launch ticks during an active sync short-circuit at SyncAlreadyRunning and are brief.
- Completion:
poll_sync on Ready runs block_on(capture_migration_witnesses()) under the write lock. One-time per sync, and a no-op without a scheduled migration.
- Rescan (
run_rescan): stop, drain at 500 ms polls, clear_all, relaunch, all under the write lock. Unbounded but user-initiated.
- Batch operations (
execute_due_parts, drain_orchard_to_ironwood, quick_split): the write lock is held across the whole prove-and-broadcast loop, including the spacing_ms sleeps between parts. Duration is N×(prove+broadcast) plus (N−1)×spacing, so tens of seconds easily. The BATCH_PROGRESS/DRAIN_PROGRESS/SPLIT_PROGRESS side channels exist precisely because of this hold, but they rescue only the progress poll. Every other reader stalls for the full batch. One mitigation: zingolib pauses sync for the batch's critical section, so the batch window and the sync windows never stack.
The criterion
A reader waiting more than 100 ms to acquire LIGHTCLIENT.read() during steady-state sync is a problem. The bounded spikes (launch ≤3 s, completion witness capture, rescan) are expected and acceptable. These terms are now in CONTEXT.md under "LIGHTCLIENT lock".
Classification still open: whether the batch window joins the spike class. The recommendation is yes, measure first. The batch is a foreground operation with its own progress surface, and the UI parks on the progress screen by design. If instrumentation then shows real balance polls stalling for tens of seconds behind batches, that is the evidence for a zingolib issue about releasing the lock across spacing sleeps (a per-part API, so the FFI can release between parts), instead of arguing from structure.
Proposed work
- Instrumentation with holder attribution. Wrap the three accessors (
with_lightclient_write, with_initialized_lightclient, with_initialized_lightclient_read) with timing: lock-acquire wait and hold duration, tagged by call site via #[track_caller] and Location::caller(), so none of the ~38 call sites change. The write wrapper additionally stamps a "current write-holder" slot while it holds the lock, so a reader that waits past the threshold logs both its wait and which endpoint it waited behind. Report over-threshold events through log::warn. Cost is two Instant::now() calls per FFI op at a 5 s cadence.
- Regression-fence test in
rust/lib/src/lock_discipline_tests.rs: hold the wallet write lock (impersonating the scanner), call steady-state poll_sync with a live sync handle, and assert it returns within a bound. This pins the property that makes the steady state cheap today: the hot poll path never awaits the wallet lock under LIGHTCLIENT.write(). It fails loudly if that path ever grows one.
The TS layer already has a crude version of (1): SyncCoordinator.fetchSyncPoll logs pollSyncInfo calls over 4 s. It cannot separate lock wait from work, and 4 s is far above the criterion, which is why the measurement belongs in the Rust accessors.
Follow-up to #1230, which ported the 19 read-only FFI endpoints onto
with_initialized_lightclient_read. The PR's caveat stands: readers no longer contend with each other, but any window where an endpoint holdsLIGHTCLIENT.write()still stalls all of them. This issue records the map of those windows, the agreed criterion for when a stall counts as a problem, and the instrumentation and test work that would answer whether the residual windows matter in practice.The write-hold windows, mapped
Reading dev and the pinned zingolib (
zingolib/src/lightclient/sync.rs, ourrust/lib/src/lib.rs):The sync engine itself never touches
LIGHTCLIENT. It runs as a spawned tokio task holding the wallet lock. The windows where sync machinery holdsLIGHTCLIENT.write()are the FFI calls that drive it:poll_sync(the UI calls it on a 5 s interval): the hold is anow_or_never()on the sync JoinHandle. Microseconds, no wallet-lock await.run_sync):LightClient::sync()awaitswallet().read()to clone the sync config, spawns the task, then spins at 50 ms until the task reports Running, bounded bySYNC_START_TIMEOUT(3 s). The whole wait happens underLIGHTCLIENT.write(). The 5 s re-launch ticks during an active sync short-circuit atSyncAlreadyRunningand are brief.poll_synconReadyrunsblock_on(capture_migration_witnesses())under the write lock. One-time per sync, and a no-op without a scheduled migration.run_rescan): stop, drain at 500 ms polls,clear_all, relaunch, all under the write lock. Unbounded but user-initiated.execute_due_parts,drain_orchard_to_ironwood,quick_split): the write lock is held across the whole prove-and-broadcast loop, including thespacing_mssleeps between parts. Duration is N×(prove+broadcast) plus (N−1)×spacing, so tens of seconds easily. TheBATCH_PROGRESS/DRAIN_PROGRESS/SPLIT_PROGRESSside channels exist precisely because of this hold, but they rescue only the progress poll. Every other reader stalls for the full batch. One mitigation: zingolib pauses sync for the batch's critical section, so the batch window and the sync windows never stack.The criterion
A reader waiting more than 100 ms to acquire
LIGHTCLIENT.read()during steady-state sync is a problem. The bounded spikes (launch ≤3 s, completion witness capture, rescan) are expected and acceptable. These terms are now inCONTEXT.mdunder "LIGHTCLIENT lock".Classification still open: whether the batch window joins the spike class. The recommendation is yes, measure first. The batch is a foreground operation with its own progress surface, and the UI parks on the progress screen by design. If instrumentation then shows real balance polls stalling for tens of seconds behind batches, that is the evidence for a zingolib issue about releasing the lock across spacing sleeps (a per-part API, so the FFI can release between parts), instead of arguing from structure.
Proposed work
with_lightclient_write,with_initialized_lightclient,with_initialized_lightclient_read) with timing: lock-acquire wait and hold duration, tagged by call site via#[track_caller]andLocation::caller(), so none of the ~38 call sites change. The write wrapper additionally stamps a "current write-holder" slot while it holds the lock, so a reader that waits past the threshold logs both its wait and which endpoint it waited behind. Report over-threshold events throughlog::warn. Cost is twoInstant::now()calls per FFI op at a 5 s cadence.rust/lib/src/lock_discipline_tests.rs: hold the wallet write lock (impersonating the scanner), call steady-statepoll_syncwith a live sync handle, and assert it returns within a bound. This pins the property that makes the steady state cheap today: the hot poll path never awaits the wallet lock underLIGHTCLIENT.write(). It fails loudly if that path ever grows one.The TS layer already has a crude version of (1):
SyncCoordinator.fetchSyncPolllogspollSyncInfocalls over 4 s. It cannot separate lock wait from work, and 4 s is far above the criterion, which is why the measurement belongs in the Rust accessors.