IPC stability bugs

Bug 65

Unit test hangs on a missed wakeup when three calls share one thread

Status
Fixed
Symptom
Hang; worst effect seen: hang
Where seen
Libmultiprocess tests (mptest)
Trigger
Several calls running at the same time
First reported
2025-12-05 (first report)
Fixed
2026-06-11
Fix
mp#295
Shipped to Bitcoin Core
bitcoin#35661

Summary

mptest occasionally hung forever in its "Make simultaneous IPC calls on single remote thread" case because the test's response handlers notified the Waiter condition variable without holding its mutex, so the main thread could miss the last wakeup. The defect was in test code and did not affect users. It showed up as 2400 s ctest timeouts on slow Bitcoin Core CI runners (Alpine musl, arm32). Fixed in mp#295, which also adds thread-safety annotations so an unlocked notify no longer compiles under clang's analysis.

What happened

Root cause

The test fires three overlapping calls on the same remote thread and waits with tc.waiter->wait(lock, [&]{ return running == 0; }). Each response handler, running on the event-loop thread, did running -= 1; tc.waiter->m_cv.notify_all(); without taking Waiter::m_mutex. The main thread can evaluate the predicate, see running nonzero, and before it blocks in condition_variable::wait, the handler decrements and notifies. That notification is lost, the predicate is never re-evaluated, and the test waits forever. ryanofsky identified this from the backtrace and proposed locking the mutex before the notify.

The window is a few instructions wide, so it needed a slow, oversubscribed runner and many repetitions to show up.

The code

The test "Make simultaneous IPC calls on single remote thread" in test/mp/test/test.cpp, as of fa35501c4f0a^. Response handlers run on the event-loop thread; the main thread waits on the same condition variable at the end:

std::atomic<size_t> running{3};
foo->m_context.loop->sync([&]
{
    for (size_t i = 0; i < running; i++)
    {
        // ...
        foo->m_context.loop->m_task_set->add(request.send().then(
            [&running, &tc, i](auto&& results) {
                assert(results.getResult() == static_cast<int32_t>(100 * (i+1)));
                running -= 1;
                tc.waiter->m_cv.notify_all();
            }));
    }
});
{
    Lock lock(tc.waiter->m_mutex);
    tc.waiter->wait(lock, [&running] { return running == 0; });
}

running is atomic, but the handler neither decrements nor notifies under Waiter::m_mutex, so nothing stops both happening between the main thread's predicate check and its going to sleep:

  1. Main thread: holds m_mutex, evaluates the predicate and sees running == 1.
  2. Event loop: the last response arrives; the handler sets running to 0 and calls notify_all(), which no thread is waiting for yet.
  3. Main thread: blocks in condition_variable::wait. No other notification will come, so the test hangs.

The fix takes the mutex before notifying, and annotates m_cv so clang's thread-safety analysis rejects an unlocked notify (fa35501c4f0a; the matching MP_REQUIRES on Waiter::wait is omitted):

--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -410,7 +410,7 @@ struct Waiter
-    std::condition_variable m_cv;
+    std::condition_variable m_cv MP_GUARDED_BY(m_mutex);
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -510,6 +510,7 @@ KJ_TEST("Make simultaneous IPC calls on single remote thread")
                     running -= 1;
+                    Lock lock(tc.waiter->m_mutex);
                     tc.waiter->m_cv.notify_all();

The main thread holds m_mutex from its predicate check until it is asleep inside wait(), so a handler that must take the mutex before notifying can only notify before the check, which then sees 0, or after the main thread is waiting, which wakes it.

Fix

fa35501c4f0a (maflcko) adds Lock lock(tc.waiter->m_mutex); before the test's notify_all(). It also annotates Waiter::m_cv with MP_GUARDED_BY(m_mutex) and Waiter::wait with MP_REQUIRES(m_mutex) in include/mp/proxy-io.h, as maflcko proposed, so clang's thread-safety analysis rejects an unlocked notify at compile time. The annotations change no runtime behaviour. In his ACK ryanofsky noted that guarding the condition variable forbids the notify-after-unlock pattern, which was accepted because performance does not matter here.

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 Locks, waits and wakeups: A notify sent without the waiter's lock

  • Prevented by lock annotations, a design rule
  • Earliest detector static analysis (did not exist then)
  • Diagnosis moderate
  • Fix design easy
  • Debugging aids symbolized stack traces, a reproduction script, lock annotations
  • Rust no

Would Rust have prevented it? No. Condvar::notify_all takes no guard, so the same unlocked notify compiles and loses the wakeup the same way; capnproto-rust's single-threaded model would have the test await the responses on the loop instead of waking another thread, which sidesteps the bug rather than catching it. More on Rust

Could it have been prevented?

Yes. The rule is the condition-variable one every threading text states: the state the predicate reads is changed, and the notify is issued, while holding the mutex the waiter holds, or the waiter can check the predicate, be overtaken by both the change and the notify, and then sleep on a notification that has already gone by. The test added by mp#214 (1238170f68e8) did running -= 1; tc.waiter->m_cv.notify_all(); from the event-loop thread with no lock, and the rewrite in mp#240 kept it. The Waiter had carried MP_REQUIRES(m_mutex) annotations since mp#201, but on the mutex and wait() only; marking m_cv itself MP_GUARDED_BY(m_mutex), which the fix (fa35501c4f0a) does, turns an unlocked notify_all into a compile error, at the accepted cost of forbidding the notify-after-unlock optimisation. The same misunderstanding had come up in Bitcoin Core before (bitcoin#30967), which is why maflcko argued for the annotation rather than only the lock.

Could it have been found sooner?

By clang's thread-safety analysis from the day the test was written, had the condition variable been annotated; the analysis already ran on the file. At runtime only repetition on a slow machine finds a window a few instructions wide: maflcko's recipe (loop mptest in an Alpine/musl container with every CPU busy) hung it in 5-15 minutes, Bitcoin Core CI hit it perhaps four times in six months on its musl and arm32 jobs, and the libmultiprocess repository's own CI never did. TSan does not report a lost wakeup, since running is atomic and nothing is a data race.

What made it hard to debug?

The first sighting in December 2025 was the second half of an issue opened for an unrelated i686 segfault, came with a backtrace showing the main thread parked in Waiter::wait and every other thread idle, and was closed four months later as "likely fixed" by a subtree update that had nothing to do with it; the February CI timeout had no backtrace at all. It took the June recurrence and maflcko reproducing it with the same recipe for ryanofsky to read the unlocked notify off the idle-threads backtrace, after which the fix was one line. What would have helped is treating "every thread idle, one blocked in a condition-variable wait" as the signature of a lost wakeup rather than of slowness, and not closing a hang with a backtrace on the strength of an unrelated merge.

Lesson

Change the waited-on state and notify the condition variable under the waiter's mutex, and annotate the condition variable as guarded so the compiler enforces it; a hang whose backtrace shows one thread in a condition-variable wait and every other thread idle is a lost wakeup until proven otherwise.

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 mptest
Scope test-code
Trigger Concurrent calls (concurrent-calls)
Phase steady-state
Side n/a
Component test-harness
Root cause kind missing-lock
Fix kind test-only
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Confidence high