IPC stability bugs

Bug 79

Node can crash when a client dies while a call's reply is being built

Status
Fixed
Symptom
Data race; worst effect seen: sanitizer report only
Where seen
Bitcoin Core functional tests, libmultiprocess tests (mptest)
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2026-08-19 (first report)
Fixed
2026-08-26
Fix
mp#349
In released binaries
Yes (v30.0-v31.1)
Defect ever on master
Yes

Summary

When an IPC client died in the middle of a call, the server's worker thread could read Cap'n Proto connection state in its first getResults() call while the event-loop thread was tearing that state down. TSan reports it as a data race, and one interleaving dereferences a null pointer, so a client that disconnects mid-call could take down bitcoin-node. It was found by Antithesis running a TSan build; no crash was observed. Released binaries v30.0 through v31.1 contain it. Fixed in mp#349; not yet in Bitcoin Core's subtree.

What happened

Root cause

For methods taking an mp.Context parameter, PassField (include/mp/type-context.h) runs the method on a worker thread, and the generated ServerRet::invoke (include/mp/proxy-types.h) then calls call_context.getResults() on that thread. Cap'n Proto's RpcCallContext::getResults() (capnp/rpc.c++) reads connection state on its first call, to decide whether to allocate the results inside a real outgoing message or a detached one, and caches the response after that.

On an abrupt disconnect, RpcConnectionState::disconnect() runs on the event-loop thread and tears the connection down in two steps: it moves the live connection out of the connection field (nulling the stored pointer), then flips the field to its disconnected state. A worker calling getResults() between the two steps passes the is<Connected>() check and dereferences the null pointer. The other interleavings are harmless.

Nothing in libmultiprocess can order the worker's read against this teardown. The Connection::onDisconnect promise used for cleanup fires only after the teardown, and the request is not cancelled first: with capnp's allowCancellation off (the default), LocalClient::callInternal detaches a fork of the call promise, so the CancelMonitor handshake that tells the worker thread its call was cancelled, added by mp#240 (request cancellation) and mp#249 (disconnect race fixes), never runs on this path. Enabling it would only narrow the window. mp#249's cancel_mutex keeps the params and results alive but does not cover this field.

The worker-thread getResults() call dates from the library's initial commit.

The code

Quoted from 914dc839f279^. For a method with an mp.Context parameter, the method body runs on a worker thread, and the generated ServerRet::invoke (include/mp/proxy-types.h) asks for the results struct there once the method returns:

void invoke(ServerContext& server_context, TypeList<>, Args&&... args) const
{
    auto&& result = Parent::invoke(server_context, TypeList<>(), std::forward<Args>(args)...);
    auto&& results = server_context.call_context.getResults();

Before handing the request to the worker, PassField (include/mp/type-context.h) already runs a block on the event-loop thread, but it does not touch the results:

loop.sync([&] {
    // Detect request being canceled before it executes.
    if (cancel_monitor.m_canceled) {
    // ...
    std::tie(request_thread, inserted) = SetThread(
        GuardedRef{thread_context.waiter->m_mutex, request_threads}, server.m_context.connection,
        [&] { return Accessor::get(call_context.getParams()).getCallbackThread(); });
});

The first getResults() call on a request reads Cap'n Proto's connection state (in RpcCallContext::getResults, capnp/rpc.c++) to decide where to allocate the response; later calls return the cached response. On an abrupt client disconnect, RpcConnectionState::disconnect() rewrites that state on the event-loop thread in two steps, before libmultiprocess's disconnect handler or any cancellation runs:

  1. Worker: the method returns; ServerRet::invoke calls getResults() for the first time.
  2. Event loop: disconnect() moves the live connection out of the state field, leaving a null pointer, then marks the field disconnected.
  3. Worker: reading between those two writes, it sees the state as still connected and dereferences the null pointer, crashing the node. TSan reports the race whichever way the reads and writes interleave.

The fix makes the first getResults() call inside the existing event-loop block (914dc839f279; comment trimmed; the new testing_hook_misc and the test are omitted):

@@ -131,6 +131,18 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
                 [&] { return Accessor::get(call_context.getParams()).getCallbackThread(); });
+            call_context.getResults();
         });

The connection state is now read only on the event-loop thread, the same thread that tears it down, and the worker's later getResults() calls return the cached response without reading it.

Fix

914dc839f279 (co-authored with Claude) adds call_context.getResults(); inside the existing loop.sync() block that runs on the event-loop thread before the request is handed to the worker, so capnp caches the response there and the worker's later calls never touch connection state. The cost is a wasted preallocated results message when the method throws. The commit also adds a generic testing_hook_misc to EventLoop and the test "Calling async IPC method with a remote disconnect while results are built", which reproduces the race under TSan before the fix and is clean after. @xyzconstant's ACK summarises the mechanism.

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: A single-thread object used from another thread

  • Prevented by a thread-affinity assertion, a design rule
  • Earliest detector ThreadSanitizer (did not exist then)
  • Diagnosis moderate
  • Fix design moderate
  • Debugging aids sanitizer output, thread names, a hook to force the timing
  • Rust yes

Would Rust have prevented it? Yes. Capnproto-rust's call context is !Send, so the worker thread could not call getResults() on it; the results would have to be built on the loop thread, or on the worker and sent back, which is the fix's shape at the cost of a different model for blocking method bodies. More on Rust

Could it have been prevented?

Partly. Cap'n Proto's CallContext is a KJ object, and KJ objects are single-threaded by contract; libmultiprocess runs method bodies on worker threads and calls call_context.getResults() there, relying on the observation that the results message, once allocated, is plain memory. The trap is that the first getResults() call is not a plain accessor: it reads RpcConnectionState::connection to decide where to allocate, and that field is rewritten by disconnect() on the loop thread with no notification. A rule that the worker receives only message readers and builders obtained on the loop thread, never the context itself, would have put the allocation in the loop.sync() block from the start, which is what 914dc839f279 does with one line. A thread-affinity assert inside capnp's getResults() would have failed on the first mptest run in 2019, but that check does not exist in KJ, and the library's whole design depends on reading capnp messages off the loop thread, so the rule was not obvious in 06e1045baba0.

Could it have been found sooner?

Yes, by TSan on a test that drops the client between a method body returning and its first getResults(), which is the test the fix adds with a new testing_hook_misc to hold the worker at that point. The TSan job existed from 2025-06 and the disconnect tests from mp#160, mp#240 and mp#249 all disconnect mid-call, but none held the worker in this window, and the window is small: the read must land between two writes in capnp's teardown. Antithesis found it because its coverage instrumentation sleeps inside the callbacks, widening exactly this kind of gap. Even a targeted test is fragile under TSan, since the teardown's own later reads evict the racing write from TSan's shadow memory unless the worker's read comes first.

What made it hard to debug?

The report (mp#348) came with a TSan trace whose b-capnp-loop thread name placed the writer, and an LLM-written analysis that had the mechanism right and the capnp version wrong. ryanofsky's first reaction was that the onDisconnect cancellation from mp#249 should already cover it; the same-day answer needed a read of capnp/rpc.c++ to see that onDisconnect fires only after the connection is destroyed, that allowCancellation off means the request is never cancelled first, and that nothing in the library can order against the teardown. The fix design then hinged on a detail of capnp, that only the first getResults() reads connection state and later calls return a cached response, so pre-calling it on the loop thread is enough; the alternatives on the table were copying every parameter or patching capnp. The commit body records all of this, which is the main reason the record is easy to verify now.

Lesson

When you move work off the thread that owns a library object, treat every method of that object as thread-affine until you have read its implementation, and take from it on the owning thread everything the worker will need; a getter that allocates on first use is a write in disguise.

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 data-race
Worst effect observed sanitizer-report-only
Seen in functional-test, mptest
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component type-context
Root cause kind assumption-violated
Fix kind one-line-tweak
Detection antithesis
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released binary
Confidence high