IPC stability bugs

Bug 4

Cancellation flag is written unlocked because its lock was a temporary

Status
Fixed
Symptom
Data race; worst effect seen: nothing observed
Where seen
Never observed
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2026-02-24 (first report)
Fixed
2026-02-25
Fix
mp#245
Shipped to Bitcoin Core
bitcoin#34422
In released binaries
No
Defect ever on master
Yes

Summary

The request cancellation added in mp#240 lets the server stop work on a call whose client has disconnected. Its cancellation callback took its mutex as an unnamed temporary, so the lock was released immediately and the request_canceled = true write that followed ran unlocked. In theory a worker thread could then miss a cancel and write results into structs Cap'n Proto was about to free. It was found in code review, was never observed in practice, and was fixed in mp#245 five days after mp#240 merged, before it reached Bitcoin Core or any release.

What happened

Never observed. @janb84 was reviewing the subtree update bitcoin#34422, which carried mp#240 into Bitcoin Core, and asked (2026-02-24): "Lock{cancel_mutex}; is an unnamed temporary ... Leaving the server_context.request_canceled = true; without a lock?" ryanofsky agreed it was a theoretical race and opened mp#245 within the hour.

Root cause

In PassField (include/mp/type-context.h), the m_on_cancel callback runs on the event loop thread when a request is cancelled. It is meant to wait on cancel_mutex while the worker thread is reading the request's parameters or writing its results, then mark the request cancelled:

Lock{cancel_mutex};
server_context.request_canceled = true;

Lock{cancel_mutex}; creates and destroys a temporary, so the mutex is released at the semicolon. In an earlier revision of mp#240 request_canceled was a std::atomic<bool> assigned before the lock, and an unnamed lock was enough. The final revision made it a plain bool protected by cancel_mutex but kept the unnamed lock. The worker holds cancel_mutex except while the method body runs: ServerCall::invoke (include/mp/proxy-types.h) unlocks it around the call and retakes it before the results are written. So when a cancel lands while the method runs, Lock{cancel_mutex} only waits until the worker has dropped the lock. The event loop then writes request_canceled with no lock held while the worker, its method returned, retakes the lock and reads the flag. That is a data race that can read a stale false, after which the worker fills in results that Cap'n Proto frees as soon as m_on_cancel returns.

The fix's commit message describes the window as the event loop cancelling "the request before the execution thread runs" and the worker then accessing "deleted parameters". The code is narrower: m_on_cancel is only registered once the worker has started running the request (an earlier cancel is caught by a separate check on the event loop), and what the worker touches afterwards is the results. The commit message calls it "very unlikely to occur in practice".

The code

Quoted from e0f1cd762190^. PassField (include/mp/type-context.h) installs this cancellation callback, which the event-loop thread runs if the request is cancelled while the worker thread is executing it:

cancel_monitor.m_on_cancel = [&server, &server_context, &cancel_mutex, req]() {
    MP_LOG(*server.m_context.loop, Log::Info) << "IPC server request #" << req << " canceled while executing.";
    // ...
    Lock{cancel_mutex};
    server_context.request_canceled = true;
};

On the worker thread, ServerCall::invoke (include/mp/proxy-types.h) releases the worker's cancel_lock while the method runs, then retakes it and checks the flag before the results are written:

if (server_context.cancel_lock) server_context.cancel_lock->m_lock.unlock();
return TryFinally(
    // ...
    [&] {
        if (server_context.cancel_lock) server_context.cancel_lock->m_lock.lock();
        // ...
        if (server_context.request_canceled) throw InterruptException{"canceled"};
    });

Lock{cancel_mutex}; builds a temporary that locks and unlocks on the same line, so request_canceled (a plain bool) is written with no lock held:

  1. Worker: releases cancel_lock and runs the method.
  2. Event loop: the client disconnects; m_on_cancel takes and immediately drops cancel_mutex.
  3. Worker: the method returns; it retakes cancel_lock and reads request_canceled while the event loop is writing it, a data race that can see false.
  4. Event loop: returns from m_on_cancel and Cap'n Proto frees the params and results the worker is about to fill.

The fix names the lock so it is held until the end of the lambda (e0f1cd762190):

@@ -123,7 +123,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
-                            Lock{cancel_mutex};
+                            Lock cancel_lock{cancel_mutex};
                             server_context.request_canceled = true;
                         };

The write now happens under cancel_mutex, so a worker that holds cancel_lock either sees true or has already finished with the structs before the event loop can go on to free them.

Fix

The fix commit e0f1cd762190 names the lock, Lock cancel_lock{cancel_mutex};, so it covers the assignment (+1/-1).

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 field locked by some accessors and not others

  • Prevented by the type system, a review checklist item
  • Earliest detector static analysis (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Rust yes

Would Rust have prevented it? Yes. The flag would live inside the Mutex and could only be written through a live guard, so a lock temporary that dies on the same line has no equivalent: let _ = m.lock() gives nothing to write through, and clippy's let_underscore_lock lint flags the attempt. More on Rust

Could it have been prevented?

Yes, cheaply. Lock{cancel_mutex}; is a temporary that locks and unlocks on one line; the language accepts it silently because the object has a destructor. A [[nodiscard]] on the Lock type (or its constructor, in C++20) turns the line into a compiler warning, and the review comment that found it is the checklist item: an RAII guard that is not named is not held. The deeper cause is a revision slip inside one PR: request_canceled was an std::atomic<bool> assigned before the lock, then became a plain bool guarded by the lock on 2026-02-18, and the unnamed lock survived the change (e0f1cd762190 tells that history). A flag reachable only through an accessor that takes a Lock&, the shape the same file already uses for GuardedRef, would have refused to compile the unlocked write.

Could it have been found sooner?

By clang-tidy's bugprone-unused-raii, which exists to flag exactly this construct; neither project's .clang-tidy enables it, and Bitcoin Core's tidy job does not cover the subtree. It was found five days after mp#240 merged, by janb84 reading the diff of the subtree update rather than the library PR, which is the cost of the two-repository layout: the change had been reviewed once already. TSan is the other candidate, since the unlocked write and the worker's later locked read are unsynchronized, but only when a test cancels a request mid-execution and the worker's wake-up does not itself order the read after the write; the PR's TSan runs did not report it, and no one has reproduced it since.

What made it hard to debug?

Nothing: it was diagnosed from the source line and fixed by naming the variable (+1/-1) within the hour. It belongs in the ledger as the first of three regressions from the cancellation mechanism, all found in review or by sanitizer within three weeks of merging, and as a reminder that a PR whose synchronization design changes between revisions needs its earlier revisions' assumptions re-read, not just the final diff.

Lesson

An RAII lock must be a named variable, and the compiler can enforce that; when a field changes from atomic to lock-protected during review, every existing access to it is a new line to review.

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 none
Seen in never
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component type-context
Root cause kind missing-lock
Fix kind one-line-tweak
Detection review
Needs a peer that is not libmultiprocess C++ yes
Needed a sanitizer to see no
Defect ever on master yes
Released no
Confidence high