IPC stability bugs

Bug 17

Wallet process hangs and keeps its wallets locked after the node is killed

Status
Fixed
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
2024-06-13
Fix
mp#101
Shipped to Bitcoin Core
bitcoin#30490
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

When the connection between bitcoin-node and a spawned bitcoin-wallet process broke (for example because the node was killed), the wallet process hung forever and kept its wallet directory locked. When a connection breaks, libmultiprocess destroys the objects the process was serving to its peer. It destroyed them in the order they were created, so the wallet process destroyed its WalletLoader (the object the node uses to load and unload wallets) before the Chain::Notifications objects that still held references to the wallets. The WalletLoader destructor waited for those references forever. The multiprocess wallet exists only on the unmerged bitcoin#10102 branch, so no release could hit it. Fixed in mp#101 by running these cleanups in reverse creation order.

What happened

Root cause

When a connection breaks, Connection::~Connection moves its m_async_cleanup_fns list onto the event loop's async cleanup thread, which runs the entries one after another. The work is done on that separate thread because destroying a server object can block, and the event loop thread must not. The list has one entry per ProxyServer whose m_impl still needs destroying; ~ProxyServerBase adds it with addAsyncCleanup. Connection::addAsyncCleanup (src/mp/proxy.cpp) inserted at the front of the list. Cap'n Proto destroys the ProxyServer objects newest first, so front insertion reversed that, and the objects' implementations were destroyed oldest first.

In the wallet process the node-owned WalletLoader is created before the Chain::Notifications handlers (the CWallet objects). WalletLoaderImpl's destructor unloads the wallets and waits for every CWallet shared_ptr to be released, but the Chain::Notifications servers holding those pointers were later entries in the same list, which the single cleanup thread would only reach after the destructor returned. So the destructor waited forever.

The code

Quoted from 6825523c6e35^. When a connection breaks, Cap'n Proto destroys the connection's ProxyServer objects, and each ~ProxyServerBase (include/mp/proxy-io.h) hands the destruction of its implementation object to the connection:

m_context.connection->addAsyncCleanup([impl=std::move(m_impl), c=std::move(m_context.cleanup)]() mutable {
    impl.reset();

Connection::addAsyncCleanup in src/mp/proxy.cpp puts each callback at the front of the list, and ~Connection then moves the list, front first, to the single async cleanup thread, which runs the callbacks one at a time in that order:

void Connection::addAsyncCleanup(std::function<void()> fn)
{
    std::unique_lock<std::mutex> lock(m_loop.m_mutex);
    m_async_cleanup_fns.emplace(m_async_cleanup_fns.begin(), std::move(fn));
}
// ...
    while (!m_async_cleanup_fns.empty()) {
        std::unique_lock<std::mutex> lock(m_loop.m_mutex);
        m_loop.m_async_fns.emplace_back(std::move(m_async_cleanup_fns.front()));
        m_async_cleanup_fns.pop_front();
    }

Cap'n Proto destroys the servers newest first, and front insertion reverses that, so the implementations are destroyed oldest first. In the wallet process, when the node dies:

  1. Event loop: the Chain::Notifications servers (the CWallets, created later) and then the WalletLoader server (created earlier) are destroyed; the list ends up [WalletLoader, Notifications...].
  2. Async thread: runs WalletLoaderImpl's destructor first, which unloads the wallets and waits for each CWallet shared_ptr to be released.
  3. The references it waits for are held by the Notifications callbacks later in the same list, which this thread only reaches after the destructor returns, so bitcoin-wallet never exits.

The fix appends instead (6825523c6e35, comments trimmed):

@@ -119,7 +127,23 @@ void Connection::removeSyncCleanup(CleanupIt it)
 void Connection::addAsyncCleanup(std::function<void()> fn)
 {
     std::unique_lock<std::mutex> lock(m_loop.m_mutex);
-    m_async_cleanup_fns.emplace(m_async_cleanup_fns.begin(), std::move(fn));
+    m_async_cleanup_fns.emplace(m_async_cleanup_fns.end(), std::move(fn));
 }

Implementations are now destroyed in the order Cap'n Proto destroyed their servers, newest first, so the Notifications objects release their CWallet references before WalletLoaderImpl's destructor waits for them. This relies on creation order matching dependency order.

Fix

The fix commit 6825523c6e35, in mp#101 ("connection: run async cleanups in LIFO not FIFO order"), inserts async cleanups at the end of the list, keeping Cap'n Proto's newest-first order, and adds comments explaining why the order of the async list matters and the order of the sync cleanup list does not (+25/-1, one functional line). In the comment diagnosing the hang, ryanofsky described the fix as "still somewhat fragile", since it relies on creation order matching dependency order.

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 Built or torn down in the wrong order: Teardown in the wrong order

  • Prevented by a design rule
  • 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. Running deferred destructors oldest-first is an ordering choice; Rust drops locals in reverse and struct fields in declaration order, but a hand-built cleanup list runs in whatever order it is pushed and drained. More on Rust

Could it have been prevented?

Yes. C++ destroys members and locals in reverse construction order, and Cap'n Proto destroys a broken connection's server objects newest first for the same reason: a later object may depend on an earlier one. Connection::addAsyncCleanup broke the convention with one word, inserting at begin() instead of end(), so the deferred destructors ran oldest first. The rule "any deferred teardown list must preserve the order the framework would have used" was knowable in 2019 and is what the fix commit (6825523c6e35) writes into the comment. It is still only a convention: the fix relies on creation order matching dependency order, which ryanofsky called "still somewhat fragile", and a wallet whose WalletLoader waits on shared_ptr counts is sensitive to that order in a way most server objects are not.

Could it have been found sooner?

Yes, by any test that kills the node while a spawned wallet process has a wallet loaded. The order had been wrong for five years, but the C++ client shuts connections down cleanly, calling destroy on each object, so the async cleanup list was only ever run with dependent objects still in it after an unclean disconnect. No test did that until bitcoin#26606 added a tool_wallet.py case that kills the node for an unrelated reason (2024-05); the first multiprocess CI run of the bitcoin#10102 branch after that rebase hung deterministically. willcl-ark had seen the locked wallet directory three weeks earlier from an external client, which is the same failure without a test around it.

What made it hard to debug?

Two deadlocks shared one symptom: the same hang was also caused by the ProxyServerCustom<WalletLoader> destructor stopping the scheduler on the event-loop thread (sibling record), so fixing the first still left a hung wallet process, and willcl-ark's report could not be attributed to either. The CI evidence was a hung job with a lock error afterwards and no stack traces; the diagnosis needed the kill test run locally with thread stacks in hand. Once seen, a single cleanup thread blocked in ~WalletLoaderImpl with Notifications entries still queued behind it reads directly, and the fix took the same day. Dumping all thread stacks from a hung test job, and named threads so the async cleanup thread is recognisable, would have made it a one-look diagnosis.

Lesson

When you defer destruction to a list or queue, keep the order the language would have used, newest first; a cleanup list that runs oldest first will deadlock the first time an early object waits on a later one.

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 connection
Root cause kind ordering
Fix kind one-line-tweak
Detection user-report
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high