IPC stability bugs

Bug 16

Wallet process hangs holding its wallet lock after the node is killed

Status
Open, fix under review
Symptom
Deadlock; worst effect seen: deadlock
Where seen
Bitcoin Core functional tests, branch-build
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2024-05-24 (first report)
Fixed
Not yet
Fix
bitcoin#10102
In released binaries
No
Defect ever on master
No

Summary

On the unmerged multiprocess branch where bitcoin-node spawns a separate bitcoin-wallet process (bitcoin#10102, "Multiprocess bitcoin"), killing the node could leave the wallet process hung forever, still holding its wallet directory lock, so the wallet could not be opened again. The wallet process runs a scheduler thread for periodic wallet tasks, and some of those tasks make IPC calls to the node. When the connection broke, the wallet's WalletLoader server object stopped that scheduler from its destructor, which runs on the IPC event loop thread, while the scheduler thread could be waiting on that same event loop for an IPC call to finish. Each thread waited for the other. The code only exists on the bitcoin#10102 branch (and on bitcoin#29409, "multiprocess: Add capnp wrapper for Chain interface", which it builds on); it was never on master or in a release. The fix is in that branch too, so it will only be merged along with the PR.

What happened

Root cause

When the wallet runs in its own process it cannot share the node's CScheduler, so the ChainClient.start handler in src/ipc/capnp/wallet.cpp creates a SchedulerThread (a CScheduler plus a thread running serviceQueue) for the WalletLoader server. Before the fix the SchedulerThread was a member of ProxyServerCustom<WalletLoader>, so it was destroyed in that object's destructor. When a connection breaks, Cap'n Proto destroys the server objects that belonged to it, and it does so on the event loop thread, the one thread that handles all IPC traffic for the process.

~SchedulerThread calls m_scheduler.stop() and waits for the scheduler thread to finish. If the scheduler thread is inside an IPC call at that moment (for example chain().isReadyToBroadcast() from the wallet's periodic rebroadcast task), that call needs the event loop to make progress, but the event loop thread is blocked in the destructor waiting for the scheduler thread. Neither can proceed, and the wallet process never exits, so its wallet lock is never released.

Fix

The fix, pushed to bitcoin#10102 on 2024-06-13 (diff between the pushed branches pr/ipc.207 and pr/ipc.208), keeps the SchedulerThread out of the server object: the ChainClient.start handler registers its destruction in m_context.cleanup_fns instead. Cleanup functions are callbacks a server object registers to run when it is destroyed. For a server whose connection broke, ~ProxyServerBase (the base class destructor of every server object) queues them to run on the event loop's separate async cleanup thread instead of the event loop thread, so the event loop keeps running and the wait for the scheduler thread can finish. bitcoin#10102 is maintained as a rewritten branch, so there is no standalone fix commit: the change is part of its "Add capnp wrapper for Wallet interface" commit. A companion test change ("test: Fix multiprocess test for unclean shutdown on kill") also kills the wallet process so the test still exercises an unclean wallet shutdown.

At the same time ryanofsky documented the general rule in libmultiprocess, in the documentation commit 537c645652cb ("doc: Improve ProxyServerCustom class documentation", part of mp#100, which added code comments and documentation): ProxyServerCustom state must be destroyable without blocking, and blocking cleanup belongs in the cleanup list.

No fix is merged, because the affected code has only ever lived on the unmerged branch.

Next steps

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern Work or cleanup on the wrong thread: Cleanup runs on a thread that does not own the state

  • Prevented by a design rule, a written spec or documentation
  • Earliest detector a test that kills a process (did not exist then)
  • Diagnosis moderate
  • Fix design easy
  • Debugging aids thread names, symbolized stack traces, a reproduction script
  • Rust no

Would Rust have prevented it? No. A deadlock between a destructor on the loop thread and a scheduler thread waiting for that loop is an ordering bug; capnproto-rust also drops server objects on its loop thread, and the scheduler thread's call would still have to be handed to that loop and awaited, so the same two-thread cycle compiles. More on Rust

Could it have been prevented?

Yes, and the fix wrote the rule down the same day. A ProxyServer destructor runs on the event-loop thread when Cap'n Proto collects the objects of a broken connection, so anything it destroys must not wait on work that needs that loop; ~SchedulerThread did (stop() then get() on a thread that could be inside an IPC call). The fix moves the destruction into m_context.cleanup_fns, which ~ProxyServerBase already ran on the async cleanup thread, and mp#100 (537c645652cb) added the rule to the ProxyServerCustom comment: extra state must be destroyable without blocking, blocking cleanup goes in the cleanup list. The rule was knowable in 2019 (the async cleanup thread exists because of it) but was never stated, and a class whose destructor can block is indistinguishable from any other member at review time.

Could it have been found sooner?

Yes, by any test that kills the node while the spawned wallet process has a wallet loaded and its scheduler running. The branch had run for years with no such test; the tool_wallet.py kill test from bitcoin#26606 was written for another purpose and hung the wallet process on the first multiprocess CI run after the rebase. The wallet's periodic rebroadcast task is what puts the scheduler thread inside an IPC call, so the window is a few milliseconds every few seconds, which is why the same test hung only sometimes.

What made it hard to debug?

Two deadlocks hid behind one symptom: the same hang was also caused by the FIFO cleanup order fixed in mp#101, and willcl-ark's earlier report of a locked wallet directory could have been either. The diagnosis had to separate them from a hung CI job with no stack traces; once thread stacks were in hand the cycle (loop thread in a destructor, scheduler thread waiting for the loop) reads directly. Named threads (schedqueue, the event loop) and a habit of dumping all thread stacks from a hung test job would have made it a one-look diagnosis.

Lesson

Whatever thread runs your destructors and callbacks is a thread that must never wait on the work it dispatches; give blocking cleanup its own thread and state the rule where people add members to the class.

Notes

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field Value
Symptom deadlock
Worst effect observed deadlock
Seen in functional-test, branch-build
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase shutdown
Side server
Component bitcoin-ipc
Root cause kind blocking-eventloop
Fix kind local-fix
Detection user-report
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master no
Released no
Confidence high