IPC stability bugs

Bug 41

Node never finishes shutting down while an IPC client stays connected

Status
Fixed
Symptom
Hang; worst effect seen: hang
Where seen
Branch-build
Trigger
Shutdown
First reported
2024-12-16 (first report)
Fixed
2025-08-18
Fix
bitcoin#32345
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

bitcoin-node could not finish shutting down while a client connected through -ipcbind (the option that lets outside processes connect over a Unix socket) stayed connected. It logged Shutdown: done and then waited for every IPC client to disconnect on its own, which a client that ignored the node going away never did. Any external IPC client, such as a Stratum v2 mining client, could trigger it. v29.0 source builds with IPC enabled have the bug; fixed by bitcoin#32345 ("ipc: Handle unclean shutdowns better"), which disconnects incoming IPC clients during shutdown, before v30.0 shipped bitcoin-node.

What happened

Root cause

After Shutdown() (src/init.cpp) returns, main() destroys the interfaces::Init object, whose CapnpProtocol destructor releases its event loop reference and joins the event loop thread. The loop only exits when its reference count reaches zero, and every incoming mp::Connection accepted through -ipcbind holds a reference, so the join waited for remote clients to disconnect. Nothing in the shutdown path closed those connections.

The shutdown design came from the spawned-process model, where the only connection is to a child process the node itself tears down. -ipcbind lets arbitrary external processes connect and outlive the node, and the first out-of-tree clients did not react to the node shutting down.

The code

Quoted from 7f65aac78b95^. After Shutdown() returns, destroying the node's CapnpProtocol (src/ipc/capnp/protocol.cpp) drops its own reference to the event loop and joins the loop thread:

~CapnpProtocol() noexcept(true)
{
    m_loop_ref.reset();
    if (m_loop_thread.joinable()) m_loop_thread.join();
    assert(!m_loop);
};

The loop thread only returns once no references are left, and every connection holds one: Connection in src/ipc/libmultiprocess/include/mp/proxy-io.h has a member

EventLoopRef m_loop;

Connections accepted through -ipcbind live in EventLoop::m_incoming_connections until the remote side disconnects, and nothing in Shutdown() closes them. So with an external client still connected:

  1. Main thread: Shutdown() runs to the end and logs Shutdown: done.
  2. Main thread: ~CapnpProtocol releases m_loop_ref and blocks in join().
  3. Event loop: keeps serving the client's connection, whose EventLoopRef keeps the loop alive, until the client process exits.

The fix adds disconnectIncoming() and calls it from Shutdown() after the chain clients stop (7f65aac78b95; the interface declarations and the parent-connection bookkeeping in serve() are omitted):

@@ -374,6 +374,12 @@ void Shutdown(NodeContext& node)
         client->stop();
     }

+    // If any -ipcbind clients are still connected, disconnect them now so they
+    // do not block shutdown.
+    if (interfaces::Ipc* ipc = node.init->ipc()) {
+        ipc->disconnectIncoming();
+    }
@@ -65,9 +65,20 @@ public:
+    void disconnectIncoming() override
+    {
+        if (!m_loop) return;
+        // Delete incoming connections, except the connection to a parent
+        // process (if there is one), since a parent process should be able to
+        // monitor and control this process, even during shutdown.
+        m_loop->sync([&] {
+            m_loop->m_incoming_connections.remove_if([this](mp::Connection& c) { return &c != m_parent_connection; });
+        });
+    }

Destroying the incoming Connection objects on the loop thread releases their EventLoopRefs during Shutdown(), so by the time ~CapnpProtocol joins, the loop has nothing left keeping it running.

Fix

Commit 7f65aac78b95 ("ipc: Avoid waiting for clients to disconnect when shutting down") adds Ipc::disconnectIncoming(), implemented in src/ipc/capnp/protocol.cpp as an m_loop->sync call (which runs code on the event loop thread and waits for it) that erases every entry of m_incoming_connections except a connection to a parent process, and calls it from Shutdown() right after the chain clients are stopped (+29). The same PR added unit tests for AppInitMain/Shutdown in node_init_tests.cpp (commit 6eb09fd6141f).

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 Serving before ready, freeing before done: Shutdown waits on a peer it does not control

  • Prevented by a design rule, a review checklist item
  • Earliest detector a test with a non-C++ client (did not exist then)
  • Diagnosis easy
  • Fix design moderate
  • Debugging aids logging, symbolized stack traces
  • Rust no

Would Rust have prevented it? No. Waiting for a reference count held by peers the process does not control is a shutdown design gap; an Arc-counted loop in Rust hangs the same way. More on Rust

Could it have been prevented?

Yes. The event loop exits when its reference count reaches zero, and every incoming connection holds a reference; that was a sound shutdown design while the only peer was a child the node itself tears down. bitcoin#30509 added -ipcbind, which lets processes the node does not control connect, and Shutdown() still had no way to close them, so the node waited for strangers. The rule is "when you add a listener, add its eviction to shutdown in the same change", and the review question "what does shutdown do with a connected client" answers itself from ~CapnpProtocol. The fix (7f65aac78b95) is the eviction, Ipc::disconnectIncoming(), plus the one exception the design needed (keep a parent process's connection).

Could it have been found sooner?

Yes, by a functional test that leaves a client connected and stops the node; it is a timeout, not a race, so it fails on the first run. No in-tree client behaved that way: the C++ tests and the node/wallet pairing always disconnected first, so the first out-of-tree client (darosior's Rust wallet, three months after -ipcbind merged) was the first to sit there. ismaelsadeeq's patched bitcoin-mine hit the same hang five months later while the fix was still open. The PR added an in-process Init/Shutdown unit-test harness, not this case.

What made it hard to debug?

Nothing about the cause: Shutdown: done followed by a hang localizes it to the CapnpProtocol destructor's join, and the reference-count design explains the rest. The fix took longer to settle than to write, because the placement of disconnectIncoming() inside Shutdown() interacts with the handler-destruction crash (the sibling record) and with the bitcoin-gui parent connection that must survive shutdown; the December report also bundled this hang with that crash, and a later report on bitcoin#32297 was attributed here but was a different hang (a wait that Ctrl-C never woke).

Lesson

Shutdown may wait only on things the process controls; the moment outside parties can hold a reference, the process needs an explicit way to drop them, written in the same change that let them in.

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 hang
Worst effect observed hang
Seen in branch-build
Scope production
Trigger Shutdown (shutdown)
Phase shutdown
Side server
Component bitcoin-init
Root cause kind assumption-violated
Fix kind new-code-path
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high