IPC stability bugs

Bug 32

Destroying a client proxy races with the event loop closing its connection

Status
Fixed
Symptom
Data race; worst effect seen: sanitizer report only
Where seen
Libmultiprocess tests (mptest)
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2025-06-27 (first report)
Fixed
2025-07-01
Fix
mp#186
Shipped to Bitcoin Core
bitcoin#32345
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

When a ProxyClient object was destroyed while its connection was being torn down, its cleanup handler called Connection::removeSyncCleanup on the destroying thread. Nothing synchronized that call with the event loop thread, which could be destroying the same Connection at that moment. ThreadSanitizer reported it as a data race in the mptest "disconnecting and blocking during the call" test. With different timing it could have crashed, but no crash was attributed to it. Fixed in mp#186 by making the call on the event loop thread. The code was in Bitcoin Core source releases v22 through v29, which did not ship IPC binaries.

What happened

mp#160 added the first tests that break a connection while a server call is running. Within days they failed intermittently in Bitcoin Core CI, on bitcoin#32345 and on Sjors/bitcoin#90. While investigating, ryanofsky ran mptest under ThreadSanitizer; mp#186 ("Fix mptest failures in bitcoin CI") adds a TSan CI job for this. The test passed, but TSan reported:

WARNING: ThreadSanitizer: data race
  Write of size 8 ... by thread T10:
    #0 mp::EventLoopRef::reset(bool) src/mp/proxy.cpp:61
    #1 mp::Connection::~Connection() include/mp/proxy.h:58

against an earlier read by thread T11 in Connection::removeSyncCleanup. No CI failure was attributed to this particular race.

Root cause

The ProxyClientBase constructor in include/mp/proxy-io.h registers a cleanup function that runs when the proxy is destroyed, on whichever thread destroys it. Before the fix it began with

if (m_context.connection) {
    m_context.connection->removeSyncCleanup(disconnect_cb);

and only then entered m_context.loop->sync(...) to release the capability. removeSyncCleanup dereferences Connection::m_loop to lock the event loop's mutex.

In the test, the proxy is the server's ProxyClient<Thread> in request_threads, the server's handle for the client thread that made the call. Worker thread T11 destroys it in PassField (include/mp/type-context.h) when the IPC method returns. The method has just called client_disconnect(), so the event-loop thread T10 is in ~Connection destroying the server connection and resetting m_loop. Nothing orders the worker's null check and removeSyncCleanup call against that destructor, so the worker can read a Connection that is being destroyed. The fix's commit message says this "could potentially cause segfaults if the timing was different".

The unsynchronized call dates from the initial commit, where ~ProxyClientBase already called m_connection->removeSyncCleanup(...) before entering m_loop.sync. It was harmless while connections outlived every proxy using them. That held until mp#160's tests broke connections while a server call was running.

The code

Quoted from d8011c83608e^. The ProxyClientBase constructor (include/mp/proxy-io.h) registers a cleanup function that runs when the proxy is destroyed, on whichever thread destroys it. It unregisters the connection's disconnect callback before switching to the event-loop thread:

m_context.cleanup_fns.emplace_front([this, destroy_connection, disconnect_cb]{
if (m_context.connection) {
    // Remove disconnect callback so it doesn't run and try to access this
    // object after it's already destroyed.
    m_context.connection->removeSyncCleanup(disconnect_cb);
    // ...
    m_context.loop->sync([&]() {

Connection::removeSyncCleanup (src/mp/proxy.cpp) reaches the event loop through the connection's own m_loop reference:

void Connection::removeSyncCleanup(CleanupIt it)
{
    const Lock lock(m_loop->m_mutex);
    m_sync_cleanup_fns.erase(it);
}

In the "disconnecting and blocking during the call" test the proxy is the server's ProxyClient<Thread> for the caller, which the worker thread destroys when the method returns, just after the method has disconnected the client:

  1. Worker: the cleanup reads m_context.connection, still set.
  2. Event loop: ~Connection for that connection runs and resets its m_loop reference.
  3. Worker: removeSyncCleanup reads m_loop from the connection being destroyed. TSan reports this as a data race; with a later step 3, the connection has been freed.

The fix (d8011c83608e) moves the null check and the removeSyncCleanup call into the sync() lambda (comment lines trimmed):

@@ -420,11 +420,7 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
     m_context.cleanup_fns.emplace_front([this, destroy_connection, disconnect_cb]{
-    if (m_context.connection) {
-        // Remove disconnect callback so it doesn't run and try to access this
-        // object after it's already destroyed.
-        m_context.connection->removeSyncCleanup(disconnect_cb);
-
+    {
@@ -433,6 +429,14 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
         m_context.loop->sync([&]() {
+            if (m_context.connection) m_context.connection->removeSyncCleanup(disconnect_cb);
+
             // Release client capability by move-assigning to temporary.

~Connection also runs on the event-loop thread, and its disconnect callback clears m_context.connection, so inside sync() the proxy either sees a live connection and unregisters before ~Connection can start, or sees null and leaves the connection alone.

Fix

The fix commit d8011c83608e ("proxy-io: fix race conditions in ProxyClientBase cleanup handler") moves the removeSyncCleanup call inside the loop->sync() lambda, still guarded by if (m_context.connection), so it runs on the event-loop thread, serialized with the disconnect handler that deletes the Connection (+9/-5 in proxy-io.h). Because ~Connection now runs later relative to the worker, this exposed three further TSan races in the same test, where code run from ~Connection read ProxyClientBase fields without locks; those were fixed separately in the same PR by fe4a188803c6 ("proxy-io: fix race conditions in disconnect callback code"; see its page). ryanofsky later cited this commit on mp#189 ("Races between ~ProxyClient<Thread> and ~Connection destructors") as the pattern for the fix in mp#201, which applied the same rule to ProxyClient<Thread>'s own disconnect callback (its page).

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 ThreadSanitizer (did not exist then)
  • Diagnosis moderate
  • Fix design easy
  • Debugging aids sanitizer output, symbolized stack traces, a reproduction script
  • Rust yes

Would Rust have prevented it? Yes. Capnproto-rust RPC objects are Rc-based and !Send, so a worker thread could not hold or destroy the proxy off the event loop thread at all; with a hand-rolled multithreaded design the compiler would force the Connection behind an Arc, which turns the crash into a lifetime extension that can keep the loop alive. More on Rust

Could it have been prevented?

Probably. The fix is the rule: anything that touches a Connection runs on the event-loop thread, which is the thread that destroys it. The cleanup lambda registered by ProxyClientBase already entered loop->sync() to release the capability; it just did the removeSyncCleanup call first, on whichever thread happened to destroy the proxy. Moving the call inside the sync() block (d8011c83608e, +9/-5) is the whole fix. The unsynchronized call dates from the 2019 initial commit, when connections outlived every proxy and the ordering was true by construction; an assert(loop->isLoopThread()) in Connection::removeSyncCleanup, of the kind mp#201 later added to the same functions, would have failed the first time a proxy was destroyed off the loop, and would have made the 2019 assumption a stated contract instead of an accident.

Could it have been found sooner?

Yes, by ThreadSanitizer on a test that breaks a connection while a server call is in flight. The two halves arrived a week apart in June 2025: the disconnect tests in mp#160, then the library's TSan job in mp#186 itself, which ryanofsky added while investigating the intermittent failures those tests caused in Bitcoin Core CI on bitcoin#32345. The six-year-old race reported on the first sanitized run. Before that nothing could have seen it: Bitcoin Core's TSan job existed from 2024 but ipc_tests never dropped a connection mid-call, and every real client was a spawned subprocess the node stopped only after its proxies were gone.

What made it hard to debug?

TSan gave a symbolized pair of stacks (T10 in ~Connection resetting m_loop, T11 in removeSyncCleanup from the ProxyClient<Thread> destructor in PassField), and a local sanitized mptest loop reproduced it, so no gdb session or CI archaeology was needed. The work was in reading the trace: the commit body spends four paragraphs naming the three threads and explaining why the server's per-request Thread proxy is destroyed by the worker just as the loop thread destroys the connection, and that explanation is what later let the same shape be recognized in mp#189. The fix also shifted the timing of ~Connection and uncovered three further races in the same test, so the first TSan report was masking the next ones; the PR had to be re-run under the sanitizer after each change rather than fixed from one trace.

Lesson

Destructors and cleanup hooks run on whatever thread drops the object; if they touch state owned by an event loop, hop to the loop first and assert the affinity in the callee, so the ordering that holds by construction today is enforced when the lifetimes change.

Classification

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

Field Value
Symptom data-race
Worst effect observed sanitizer-report-only
Seen in mptest
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component proxy-io
Root cause kind lifetime
Fix kind local-fix
Detection tsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released source
Confidence high