Skip to content

EventQueue: never lose a consumer wake-up in next_event_async - #115

Open
hash-money wants to merge 1 commit into
lightningdevkit:masterfrom
emergent-money:fix/event-queue-wakeup
Open

hash-money wants to merge 1 commit into
lightningdevkit:masterfrom
emergent-money:fix/event-queue-wakeup

Conversation

@hash-money

Copy link
Copy Markdown
Contributor

EventFuture::poll uses try_lock on the queue mutex and on the waker slot. A poll that finds either one locked returns Pending without registering a waker, and nothing polls such a future again: the only wake-ups come from push_event / event_handled taking the slot, so every later push finds it empty and wakes nobody.

The sequence that reaches it is ordinary once two pushers overlap (the LDK handler, the rebalancer and the trusted-tier listener each run as their own task, and finish_mpp_setup pushes from the pay path):

  1. Pusher A finishes its persist, releases the queue lock, and wakes the consumer from inside if let Some(waker) = self.waker.lock().await.take() { waker.wake(); }. The scrutinee's MutexGuard temporary lives through the body, so wake() runs with the slot locked.
  2. The consumer, blocked on next_event_async (Wallet::wait_next_event, or a caller's own block_on), is unparked by that wake() and polls on its own thread, so its poll can land before A's task drops the guard, while pusher B holds the queue inside its own persist_queue (a VSS round trip from a phone takes hundreds of milliseconds or more).
  3. Both try_locks fail: Poll::Pending, no waker registered. B completes, finds None, wakes nobody. From then on the wallet keeps queueing events that wait_next_event never returns.

We hit this on a mainnet phone behind a VSS store. The process had been up 27 hours; for at least the last twelve the consumer sat parked in the block_on driving next_event_async (JDWP thread dump), a PaymentReceived was logged and its queue write completed without ever being delivered, and two PaymentSuccessful and a PaymentFailed from the same period never arrived either:

16:35:33.343 Got ldk-node event PaymentReceived { payment_id: 9bd2173d…, payment_hash: ddeeeb9d… }
16:35:33.344 vss PutObjectRequest …            ← the queue write
16:40Z  thread dump: the consumer thread inside the native wait_next_event call
         nothing delivered until the process was restarted at 17:18Z

Fix: replace the waker slot with a tokio::sync::Notify. Pushes and acknowledgements call notify_one() after releasing the queue lock, and next_event_async registers its wait (Notified::enable) before it locks and checks the queue, so a change between the check and the wait is never lost (notify_one with no waiter stores a permit). A wait dropped after being notified passes the notification on (Notified::drop), so racing the future against a shutdown cannot swallow an event. EventFuture goes away. This is also where this queue had diverged from ldk-node's, whose EventFuture::poll locks a std::sync::Mutex and never try_locks. That shape does not transfer: ldk-node encodes under a std lock and persists outside it, while this queue holds a tokio::sync::Mutex across persist_queue so pushes persist in order, and a tokio mutex cannot be taken synchronously from poll. Notify is the primitive for that; the loop is the pattern from tokio's own Notified::enable docs.

Test: a_poll_that_lands_on_two_busy_pushers_is_still_woken forces the interleaving with a store whose writes wait on a gate and a waker that polls the consumer synchronously, standing in for the block_on thread. On master it fails with polled 2 time(s), never handed the event. It is deterministic on the current-thread runtime #[tokio::test] uses: tokio's mutex is FIFO, so A's release hands the queue to B before B runs; the test's waker polls the consumer synchronously inside A's wake(); and B cannot run until A's task yields after it.

Checked: cargo fmt --check, the clippy line from ci/check-lint.sh, RUSTDOCFLAGS=-D warnings cargo doc --no-deps, cargo test -p orange-sdk --lib, cargo check --features _test-utils --tests. The _test-utils integration suite needs bitcoind/electrs and was not run here.

Until this lands we carry it on a fork pin.

`EventFuture::poll` used `try_lock` on the queue and on the waker slot. A
poll that found either one locked returned `Pending` without registering a
waker, and nothing polls such a future again: the only wake-ups come from
`push_event` / `event_handled` taking the slot, so every later push found
it empty and woke nobody. The sequence that reaches it is ordinary: pusher
A wakes the consumer from inside its `if let … .lock().await.take()` body
(the guard lives through the body), the consumer's poll lands while pusher
B holds the queue inside `persist_queue` (a VSS write on a phone takes
hundreds of milliseconds), both `try_lock`s fail, and from then on the
wallet keeps queueing events that `wait_next_event` never returns.

Replace the slot with a `tokio::sync::Notify`: pushes and acknowledgements
`notify_one` after releasing the queue lock, and `next_event_async`
registers its wait (`Notified::enable`) before it checks the queue, so a
change between the check and the wait is never lost. A dropped wait hands a
notification it received on to the next waiter, so racing the future
against a shutdown cannot swallow an event either.

The new test reproduces the interleaving with a store whose writes are
gated and a waker that polls the consumer synchronously, as the `block_on`
thread does; it fails on the previous code ("polled 2 time(s), never
handed the event").

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant