IPC stability bugs

Bug 45

Server deadlocks when a client disconnects just as a call returns

Status
Fixed
Symptom
Deadlock; worst effect seen: crash
Where seen
A production node, libmultiprocess tests (mptest)
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2025-07-01 (first report)
Fixed
2025-09-17
Fix
mp#201
Shipped to Bitcoin Core
bitcoin#33412, bitcoin#33519
In released binaries
Yes (v30.0)
Defect ever on master
Yes

Summary

An IPC server could deadlock (or, with different timing, crash) when a client disconnected just as a server method was returning, because a worker thread and the event loop thread both tried to destroy the same per-connection ProxyClient<Thread>, the server's handle for the client thread that made the call. It made the mptest "disconnecting and blocking" test hang intermittently in Bitcoin Core CI, and the same lock cycle stopped a v30.0 bitcoin-node from answering IPC calls under a Rust Stratum v2 client with no disconnect involved. Fixed in libmultiprocess by mp#201 and shipped to Bitcoin Core master and the 30.x branch.

What happened

Root cause

When a server worker thread runs a call, it records in its thread-local ThreadContext (the request_threads map) a ProxyClient<Thread> for the calling client thread, so that callbacks go back to that thread. Two code paths destroyed this entry for a connection with no ordering between them:

Deadlock: the worker holds Waiter::m_mutex inside the erase, and ~ProxyClient<Thread> calls loop->sync() to remove its cleanup callback, blocking in EventLoop::post until the loop thread is free. The loop thread is inside the disconnect callback, waiting for Waiter::m_mutex (annotated trace). Holding Waiter::m_mutex while blocking on the event loop is the cycle. In the v30.0 hang the second party was Waiter::post instead of the disconnect callback: the loop thread was posting the client's next getBlock request to the same worker (the client sends each new request on the same Context.thread) while the worker was still in the erase.

With other timing, both threads erased the same map node, giving the TSan race and the occasional segfault: erasing through an invalidated m_disconnect_cb iterator and a use-after-free of the ProxyClient<Thread> (problems 1 and 2 in mp#189; problem 3 is covered under Notes).

The code

Quoted from 4a269b21b8c8^. On the worker thread, PassField (include/mp/type-context.h) adds a request_threads entry for the connection and erases it under Waiter::m_mutex after the method returns:

auto [request_thread, inserted]{SetThread(
    GuardedRef{thread_context.waiter->m_mutex, request_threads},
    server.m_context.connection,
    [&] { return context_arg.getCallbackThread(); })};
// ...
KJ_DEFER(if (erase_thread) {
    Lock lock(thread_context.waiter->m_mutex);
    // ...
    request_threads.erase(server.m_context.connection);
});
fn.invoke(server_context, args...);

SetThread (src/mp/proxy.cpp) registers a callback that ~Connection runs on the event-loop thread after a disconnect. It erases the same entry, locking the same mutex (threads.mutex is Waiter::m_mutex):

thread->second->setDisconnectCallback([threads, thread] {
    // ...
    const Lock lock(threads.mutex);
    thread->second->m_disconnect_cb.reset();
    threads.ref.erase(thread);
});

Erasing the entry destroys a ProxyClient<Thread>, whose base-class cleanup (ProxyClientBase, include/mp/proxy-io.h) waits for the event-loop thread:

m_context.loop->sync([&]() {
    // ...
    if (m_context.connection) m_context.connection->removeSyncCleanup(disconnect_cb);

If the client disconnects as the method returns:

  1. Worker: the KJ_DEFER locks Waiter::m_mutex; erase() runs ~ProxyClient<Thread>, whose cleanup blocks in loop->sync() until the event-loop thread is free.
  2. Event loop: ~Connection runs the SetThread callback, which blocks on threads.mutex, held by the worker. Deadlock. (In the v30.0 hang the event loop was instead in Waiter::post, handing the client's next request to the same worker, which also locks Waiter::m_mutex.)

In step 1, before blocking, ~ProxyClient<Thread> also calls removeSyncCleanup(*m_disconnect_cb) on a callback that ~Connection has already spliced out of its cleanup list to run: the invalid-iterator error in mp#189. With other timings the two erasers overlap instead of blocking, giving the TSan race and the segfault.

The fix moves the erase and the callback removal into loop->sync(), on the event-loop thread (4a269b21b8c8). The same commit moves the SetThread call into loop->sync() too; that hunk, comments, asserts and the inlining of setDisconnectCallback are omitted below:

--- a/include/mp/type-context.h
+++ b/include/mp/type-context.h
@@ -89,29 +89,39 @@
                     KJ_DEFER(if (erase_thread) {
-                        Lock lock(thread_context.waiter->m_mutex);
-                        request_threads.erase(server.m_context.connection);
+                        server.m_context.loop->sync([&] {
+                            ConnThreads::node_type removed;
+                            {
+                                Lock lock(thread_context.waiter->m_mutex);
+                                removed = request_threads.extract(server.m_context.connection);
+                            }
+                        });
                     });
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -338,17 +349,18 @@ ProxyClient<Thread>::~ProxyClient()
     if (m_disconnect_cb) {
-        m_context.connection->removeSyncCleanup(*m_disconnect_cb);
+        m_context.loop->sync([&]() {
+            if (m_disconnect_cb) {
+                m_context.connection->removeSyncCleanup(*m_disconnect_cb);
+            }
+        });
     }

The worker now waits in loop->sync() without holding Waiter::m_mutex, and every add, erase and callback removal for these entries runs on the same thread as ~Connection, so the disconnect callback runs either entirely before the erase, which then finds nothing, or after it, by which time the destructor has unregistered it.

Fix

The fix commit 4a269b21b8c8 moves all creation and destruction of these entries onto the event loop thread. PassField calls SetThread inside server.m_context.loop->sync(), and the deferred erase also runs inside sync(), using request_threads.extract() under Waiter::m_mutex and destroying the node after releasing the mutex. ~ProxyClient<Thread> calls removeSyncCleanup inside loop->sync() and re-checks m_disconnect_cb there, and the SetThread callback resets m_disconnect_cb before taking the mutex. Asserts check that ~Connection, removeSyncCleanup and SetThread run on the loop thread. Other commits in the PR are preparation: std::optional in ConnThreads and try_emplace in SetThread (@Sjors), Waiter::m_mutex thread-safety annotations and GuardedRef, a ThreadContext doc comment, and -Wthread-safety in CI. No new test was added.

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 thread-affinity assertion, a design rule
  • Earliest detector a disconnect test (did not exist then)
  • Diagnosis hard
  • Debugging aids a hook to force the timing, other, a reproduction script
  • Rust partly

Would Rust have prevented it? Partly. The deadlock itself, holding Waiter::m_mutex while blocking on sync(), is a lock-ordering bug Rust does not prevent, but the interleaving that produces it, a worker thread destroying a ProxyClient<Thread> that the event loop also tears down, is ruled out by capnproto-rust's single-threaded (Rc, !Send) capabilities, which is what moving every erase onto the loop thread achieved by convention; the trade-off is a different threading model for blocking calls. More on Rust

Could it have been prevented?

Probably. The fix is the rule: every add, erase and callback removal for a per-connection entry runs on the event-loop thread, and the commit adds assert(loop->isLoopThread()) style checks to ~Connection, removeSyncCleanup and SetThread. The introducing commit (8ba0d03b4439, mp#106) created a second eraser on a second thread for state that until then had one owner; a standing rule "connection-scoped state is mutated only on the loop thread, asserted" would have made that commit fail its own assert in the disconnect test the day it was written. The second half of the rule, never block in loop->sync() while holding Waiter::m_mutex, was only stated after the fix and the PR note stated it backwards, so it was not knowable as a rule in 2024.

Could it have been found sooner?

Yes, by any test that disconnects a client between a server method returning and its request_threads entry being erased, run under TSan in a loop. Neither existed when the code was introduced: the library had no disconnect test until mp#160 (2025-06) and no TSan job until the same month. Once both existed the race showed within weeks, and ryanofsky could reproduce the TSan error in minutes by looping mptest. The Bitcoin Core TSan job existed in 2024 but ipc_tests never disconnected mid-call, so it had nothing to see.

What made it hard to debug?

The report was a TSan trace from a fork's CI, then a 2400 s ctest timeout that TheCharlatan needed about 50,000 runs to reproduce. The issue (mp#189) lists three sub-problems, an abandoned fix branch (pr/threadclean.2) and four candidate designs and ends "there is a bug and I am not sure how to fix it yet"; the correct fix came ten weeks later, after a gdb capture of the three-thread deadlock. Two things would have shortened this: a hook to force the interleaving deterministically (ryanofsky wanted one and deferred it; the test still relies on timing), and -Wthread-safety annotations on Waiter::m_mutex, which the same PR added afterwards and which make "who holds this mutex while blocking" a compile-time question. The v30.0 field hang (bitcoin#33647) was diagnosed in a day because the gdb recipe already existed.

Lesson

When state is owned by one thread, do not add a cleanup path from another thread to fix a leak; move the cleanup to the owning thread and assert the affinity. Any lock held across a blocking hand-off to another thread is a deadlock waiting for the right timing.

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, crash
Seen in production-node, mptest
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component type-context
Root cause kind lock-order
Fix kind local-fix
Detection tsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence high