IPC stability bugs

Bug 50

Thread-busy test copies capabilities off the event loop thread

Status
Fixed
Symptom
Data race; worst effect seen: sanitizer report only
Where seen
Libmultiprocess tests (mptest)
Trigger
Normal use; no special event is needed
First reported
2025-10-02 (first report)
Fixed
2025-10-07
Fix
mp#222
Shipped to Bitcoin Core
bitcoin#33518, bitcoin#33519

Summary

The mptest test "Make simultaneous IPC callbacks with same request_thread and callback_thread", added in mp#214 (which fixed a crash on simultaneous IPC calls to one thread), copied Thread::Client capabilities on the main test thread. That raced the event loop thread on Cap'n Proto's non-atomic reference counts. Bitcoin Core's TSan CI job reported it, and it blocked the libmultiprocess subtree updates for master and 30.x until fixed. The defect was in test code and did not affect users. Fixed in mp#222 ("test, ci: Fix threadsanitizer errors in mptest").

What happened

Root cause

The test, added by 1238170f68e8, declared std::optional<Thread::Client> callback_thread, request_thread; and copied the capabilities out of g_thread_context.callback_threads and request_threads on the main test thread, under Waiter::m_mutex. Copying a Thread::Client increments a kj::Refcounted count and destroying the copies at the end of the test decrements it, both on the main thread, while the event loop thread changes the same counts as capabilities are exported and released. KJ reference counts are not atomic, and Cap'n Proto objects may only be used on the event loop thread; the Waiter mutex does not protect them.

The test was reviewed without a TSan run that could see it: libmultiprocess's own sanitize job did not instrument Cap'n Proto or libc++, so races inside KJ were invisible there, and the test reached Bitcoin Core's fully instrumented TSan job only through the subtree update.

The code

The test "Make simultaneous IPC callbacks with same request_thread and callback_thread" in test/mp/test/test.cpp, quoted from 73d22ba2e930^. After one call, the main test thread copies the two Thread capabilities that the call set up:

setup.server->m_impl->m_fn = [&] {};
foo->callFnAsync();
ThreadContext& tc{g_thread_context};
std::optional<Thread::Client> callback_thread, request_thread;
{
    Lock lock(tc.waiter->m_mutex);
    callback_thread = tc.callback_threads.at(foo->m_context.connection)->m_client;
    request_thread = tc.request_threads.at(foo->m_context.connection)->m_client;
}

Copying a Thread::Client increments the reference count of the underlying Cap'n Proto object, and destroying the copies at the end of the test decrements it. KJ reference counts are not atomic and are meant to be used only on the event-loop thread. Waiter::m_mutex does not help, because the event loop never takes it to change these counts:

  1. Main thread: callFnAsync() returns once the response arrives.
  2. Event loop: still releasing the capabilities the call exported, it decrements a count in kj::Refcounted::disposeImpl.
  3. Main thread: copying m_client increments the same count in kj::Refcounted::addRefInternal. TSan reports the two writes as a data race; a lost update would leave the count wrong.

The fix (73d22ba2e930) keeps pointers instead of copies and takes them on the event-loop thread:

@@ -321,12 +321,12 @@ KJ_TEST("Make simultaneous IPC callbacks with same request_thread and callback_t
     ThreadContext& tc{g_thread_context};
-    std::optional<Thread::Client> callback_thread, request_thread;
-    {
+    Thread::Client *callback_thread, *request_thread;
+    foo->m_context.loop->sync([&] {
         Lock lock(tc.waiter->m_mutex);
-        callback_thread = tc.callback_threads.at(foo->m_context.connection)->m_client;
-        request_thread = tc.request_threads.at(foo->m_context.connection)->m_client;
-    }
+        callback_thread = &tc.callback_threads.at(foo->m_context.connection)->m_client;
+        request_thread = &tc.request_threads.at(foo->m_context.connection)->m_client;
+    });

Taking a pointer does not touch the reference count, and the test only dereferences the pointers later inside another loop->sync() call, so the counts of these two capabilities are changed only on the event-loop thread.

Fix

The fix commit 73d22ba2e930 keeps plain pointers to the m_client members instead of copies and takes them inside foo->m_context.loop->sync(), so the two thread capabilities are no longer copied or destroyed off the event loop thread (+5/-5 in test/mp/test/test.cpp). The test still copies one capability on the main thread, auto client{foo->m_client};, which the fix leaves alone. mp#222 also makes the libmultiprocess sanitize job use TSan-instrumented Cap'n Proto and libc++, so races of this kind show up in libmultiprocess CI before a subtree update.

@theuni asked whether thread-safety annotations could enforce the loop-thread rule; ryanofsky sketched a LoopObj<T> wrapper for it, which was not implemented.

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 the type system, a thread-affinity assertion
  • Earliest detector ThreadSanitizer (did not exist then)
  • Diagnosis moderate
  • Fix design easy
  • Debugging aids sanitizer output, a reproduction script
  • Rust yes

Would Rust have prevented it? Yes. A non-atomic reference count is Rc, which is neither Send nor Sync, so copying or dropping a capability on the test thread while the event loop owns it is a compile error; capnp-rust clients are Rc-based for exactly this reason. More on Rust

Could it have been prevented?

Yes, by making the rule the compiler's business. Cap'n Proto capabilities carry non-atomic KJ refcounts and may only be copied or destroyed on the event-loop thread; the library follows that rule through EventLoop::sync but nothing enforces it, so a test could copy two Thread::Clients on the main thread under a mutex the loop never takes. theuni asked whether thread-safety annotations could cover this, and ryanofsky sketched a LoopObj<T> wrapper that only dereferences given a loop-thread token; neither was built. Short of a type, an assert(loop->isLoopThread()) in the places the library hands out capabilities would have fired on the first run of the new test. The fix (73d22ba2e930) applies the rule by hand: pointers instead of copies, taken inside loop->sync(), and it leaves one more main-thread copy (auto client{foo->m_client}) untouched, which shows how easy the rule is to miss without help.

Could it have been found sooner?

Yes, by the library's own TSan job on the day the test was written, had that job instrumented Cap'n Proto. It did not: the sanitize job built KJ and libc++ without TSan, so a race inside kj::Refcounted was invisible there, and the test passed review (mp#214) and reached Bitcoin Core's fully instrumented job only via the subtree update, where it failed on the same day and blocked both the master and 30.x updates. mp#222 closed the gap by building Cap'n Proto and libc++ with TSan in the library's CI.

What made it hard to debug?

The trace itself was clear (two writes to one refcount, one from disposeImpl on a worker and one from the main thread's copy), and Sjors and Eunovo reproduced both the failure and the fix within days. The cost was a false attribution: a gnu32 segfault in the same test the next day was blamed on this race for a few hours before a gdb session traced it to an unrelated kj::str problem. A fresh TSan finding in a test makes every other failure of that test look explained; the discipline is to confirm each symptom separately.

Lesson

When a library's objects are bound to one thread, a comment and a convention are not enough for code written by people who did not write the convention; give the rule a type or an assert. And a sanitizer job is only as deep as its instrumentation: races inside an uninstrumented dependency do not exist as far as that job can tell.

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 test-code
Trigger Normal use (ordinary-path)
Phase steady-state
Side n/a
Component test-harness
Root cause kind api-misuse
Fix kind test-only
Detection tsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Confidence high