proxy-client: fix TSan data race in clientDestroy by ryanofsky · Pull Request #286 · bitcoin-core/libmultiprocess

ryanofsky · 2026-06-03T23:28:54Z

#273 added a new test which exposed a longstanding race condition in clientDestroy logging code, causing the sanitize CI job to now fail with a TSAN error. Fix the race by simplifying the logging code.

Details about the fix are in the commit message. TSAN error looked like

[ TEST ] test.cpp:437: Destroying ProxyClient<> with destroy method after peer disconnect
...
  Write of size 8 at 0x721400005020 by thread T24 (mutexes: write M0):
    #0 std::__1::__function::__func<mp::ProxyClientBase<mp::test::messages::FooCallback, mp::test::FooCallback>::ProxyClientBase(mp::test::messages::FooCallback::Client, mp::Connection*, bool)::{lambda()#1}, void ()>::operator()() /home/runner/work/libmultiprocess/libmultiprocess/include/mp/proxy-io.h:522 (mptest+0x242b20)
    #1 void mp::Unlock<mp::Lock, std::__1::function<void ()>&>(mp::Lock&, std::__1::function<void ()>&) /nix/store/6z6xj37ljxwrwd5w3rs0zyfkrxd48lzl-libcxx-21.1.2-dev/include/c++/v1/__functional/function.h:274 (mptest+0x35582d)
    #2 mp::Connection::~Connection() /home/runner/work/libmultiprocess/libmultiprocess/src/mp/proxy.cpp:150 (mptest+0x34e596)
    ...
    #11 mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const /home/runner/work/libmultiprocess/libmultiprocess/test/mp/test/test.cpp:112 (mptest+0x1504fe)

  Previous read of size 8 at 0x721400005020 by thread T26:
    #0 void mp::clientDestroy<mp::ProxyClient<mp::test::messages::FooCallback> >(mp::ProxyClient<mp::test::messages::FooCallback>&) /home/runner/work/libmultiprocess/libmultiprocess/include/mp/proxy-types.h:647 (mptest+0x342d2a)
     ...
    #21 std::__1::__function::__func<mp::ProxyServerBase<mp::test::messages::FooInterface, mp::test::FooImplementation>::~ProxyServerBase()::{lambda()#1}, void ()>::operator()() /nix/store/6z6xj37ljxwrwd5w3rs0zyfkrxd48lzl-libcxx-21.1.2-dev/include/c++/v1/__functional/function.h:174 (mptest+0x347c76)
     ...
    #23 void* std::__1::__thread_proxy[abi:ne210101]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, mp::EventLoop::startAsyncThread()::$_0> >(void*) /home/runner/work/libmultiprocess/libmultiprocess/src/mp/proxy.cpp:309 (mptest+0x353ef2)

  Thread T24 (tid=10032, running) created by main thread at:
    ...
    #4 mp::test::TestCase437::run() /home/runner/work/libmultiprocess/libmultiprocess/test/mp/test/test.cpp:449 (mptest+0x1459a7)

https://github.com/bitcoin-core/libmultiprocess/actions/runs/26851297744/job/79183796152

clientDestroy() reads m_context.connection to decide whether to use MP_LOG vs KJ_LOG, but Connection::~Connection() can set it to null concurrently from the event loop thread (via the disconnect_cb sync cleanup callback) while the destructor runs on an async cleanup thread, causing a TSan-reported data race. The race is exposed by the test added in commit 90be835 ("test: regression for ~ProxyClient destroy after peer disconnect"). The KJ_LOG fallback was only needed before commit 315ff53 ("refactor: Add ProxyContext EventLoop* member"), when logging required going through connection->m_loop. Since that commit, m_context.loop is a direct EventLoopRef that is always valid regardless of whether m_context.connection is null. The KJ_LOG branch is now dead code, so drop it and the connection check entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

DrahtBot · 2026-06-03T23:29:02Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	w0xlt
Stale ACK	enirox001

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#288 (Create support branch for CI scripts, documentation, and examples by ryanofsky)
#287 (Split repository into master (library source) and support (CI, docs, examples) branches. by ryanofsky)
#269 (proxy: add local connection limit to ListenConnections by enirox001)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

gen.cpp uses IWYU pragma: keep for include which appears to be necessary for newer versions of capnproto (1.4.0 but not 1.1.0)

ryanofsky · 2026-06-04T01:27:18Z

Added 1 commits 25bb3e6 -> 0963634 (pr/drace.1 -> pr/drace.2, compare) to fix IWYU errors https://github.com/bitcoin-core/libmultiprocess/actions/runs/26919582954/job/79416996848?pr=286

Updated 0963634 -> 039e5ac (pr/drace.2 -> pr/drace.3, compare) to fix IWYU errors https://github.com/bitcoin-core/libmultiprocess/actions/runs/26924152551/job/79430685686?pr=286

w0xlt · 2026-06-04T06:11:13Z

Concept ACK,

enirox001

ACK 039e5ac

Built and ran mptest with -fsanitize=thread, all 11 tests pass cleanly.

The fix makes sense to me. Instead of adding synchronization for what constitutes a log line, we drop the branch and unconditionally log via MP_LOG.

ryanofsky · 2026-06-05T13:16:18Z

Thanks for the reviews. I also went back and tried to debug why CI did not catch the TSAN error. It turns out to be because an interaction between #273 and #218. The new test added in #273 does not trigger the TSAN error deterministically. When it is run standalone without #218 there's usually no error, although I did see a TSAN error once running the test maybe 10 times. But when #273 and #218 are combined, the test triggers TSAN failures most of the time. This seems to happen because #218 changes clientDestroy to call CxxTypeName which can change thread timing.

ryanofsky · 2026-06-05T14:13:08Z

Started to merge this but I saw some more IWYU errors locally that seem better to fix. I think the errors don't happen in CI because nixpkgs are older and use an older version of capnproto. I also ran IWYU locally before my last push, but didn't see these errors because I was using a combined branch with #287 with example files removed. Errors look like:

/home/ryan/mp/build-default/example/calculator.capnp.proxy-client.c++ should add these lines:
#include <capnp/common.h>                    // for MessageSize
#include <string>                            // for basic_string

Updated 039e5ac -> 90982f7 (pr/drace.3 -> pr/drace.4, compare) touching up IWYU commit to fix more local IWYU errors

DrahtBot mentioned this pull request Jun 3, 2026

proxy: add local connection limit to ListenConnections #269

Open

mpgen: iwyu changes required by previous commit

90982f7

gen.cpp uses IWYU pragma: keep for include which appears to be necessary for newer versions of capnproto (1.4.0 but not 1.1.0)

ryanofsky force-pushed the pr/drace branch from 0963634 to 039e5ac Compare June 4, 2026 01:38

ryanofsky mentioned this pull request Jun 4, 2026

Split repository into master (library source) and support (CI, docs, examples) branches. #287

Open

enirox001 reviewed Jun 5, 2026

View reviewed changes

ryanofsky force-pushed the pr/drace branch from 039e5ac to 90982f7 Compare June 5, 2026 14:11

DrahtBot mentioned this pull request Jun 5, 2026

Create support branch for CI scripts, documentation, and examples #288

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proxy-client: fix TSan data race in clientDestroy#286

proxy-client: fix TSan data race in clientDestroy#286
ryanofsky wants to merge 2 commits into
bitcoin-core:masterfrom
ryanofsky:pr/drace

ryanofsky commented Jun 3, 2026 •

edited

Loading

Uh oh!

DrahtBot commented Jun 3, 2026 •

edited

Loading

Uh oh!

ryanofsky commented Jun 4, 2026 •

edited

Loading

Uh oh!

w0xlt commented Jun 4, 2026

Uh oh!

enirox001 left a comment •

edited

Loading

Uh oh!

ryanofsky commented Jun 5, 2026

Uh oh!

ryanofsky commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ryanofsky commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ryanofsky commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt commented Jun 4, 2026

Uh oh!

enirox001 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jun 5, 2026

Uh oh!

ryanofsky commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryanofsky commented Jun 3, 2026 •

edited

Loading

DrahtBot commented Jun 3, 2026 •

edited

Loading

ryanofsky commented Jun 4, 2026 •

edited

Loading

enirox001 left a comment •

edited

Loading