Fix bthread_join memory visibility with paired release/acquire on version_butex

[chenBright]

What problem does this PR solve?

Issue Number: resolve apache#3274

Problem Summary:

The join/end handshake on version_butex was not correctly synchronized.

• Producer (task_runner, at bthread end): bumped the version with a plain write
  ++*m->version_butex. The surrounding version_lock provides release semantics
  only to threads that also take that lock — join() does not, so there is no release on
  version_butex for the join path.
• Consumer (TaskGroup::join): exited its wait loop via a plain read *m->version_butex,
  with no acquire ordering.

The result is a data race with no happens-before edge, so writes the joined bthread made
before ending were not guaranteed visible after join() returned. On x86 (TSO) the hardware
masked this; on ARM it surfaced.

PR apache#3276 attempted a fix by adding a lone atomic_thread_fence(acquire) after the loop. But
per the C++ memory model an acquire fence only establishes synchronization when a preceding
atomic load reads a value from a matching release operation. Here the load was a plain read and
the producer had no release store, so the fence pairs with nothing — it happens to work on ARM
only because the compiler emits a real dmb ishld, not because the model guarantees it.

What is changed and the side effects?

Changed:

This PR replaces that band-aid with a properly paired release/acquire on version_butex.

Side effects:

• Performance effects:

• Breaking backward compatibility:

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

Pull request overview

This PR addresses a correctness issue in bthread lifecycle synchronization by ensuring bthread_join() has a well-defined happens-before relationship with the joined bthread’s completion, targeting weakly-ordered architectures (e.g., ARM).

Changes:

• Updates the version bump at bthread end to use release semantics and updates TaskGroup::join() to use acquire semantics when observing the version change.
• Removes the prior acquire-fence “band-aid” and replaces it with a paired release/acquire approach.
• Expands inline comments in TaskMeta/TaskGroup to explain the intended synchronization.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/bthread/task_meta.h	Updates comments describing the role of version_lock and the intended memory-visibility guarantees of version_butex.
src/bthread/task_group.cpp	Implements the release-store on version bump and acquire-load in join(), and removes the standalone acquire fence.

Suppressed comments (1)

src/bthread/task_group.cpp:725

• Same issue as the producer side: m->version_butex is backed by butil::atomic<int> inside Butex (see butex_wait()/butex_wake() casting), so loading it via butil::atomic<uint32_t> is undefined behavior.

Use butil::atomic<int> for the acquire load and cast the loaded value to uint32_t for comparison with expected_version.

    auto* version = reinterpret_cast<butil::atomic<uint32_t>*>(m->version_butex);
    while (version->load(butil::memory_order_acquire) == expected_version) {

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/bthread/task_group.cpp:726

• This change adjusts the synchronization contract of bthread_join() on weak-memory architectures. There are existing unit tests exercising join behavior (e.g. test/bthread_unittest.cpp:253-263), but none appear to cover the memory-visibility guarantee this PR is fixing (or catch the original data race under TSAN). Adding a regression test would help prevent future refactors from breaking the release/acquire pairing again.

    // Acquire load pairs with the release store performed when the joined
    // bthread ends (see the version bump above), ensuring all of its memory
    // writes are visible after join() returns. This matches the semantic
    // guarantee provided by pthread_join() across supported architectures.
    auto* version = reinterpret_cast<butil::atomic<uint32_t>*>(m->version_butex);
    while (version->load(butil::memory_order_acquire) == expected_version) {

src/bthread/task_group.cpp:527

• The comment claims this is "the only writer of version_butex at runtime", but there is at least one other writer under BRPC_BTHREAD_TRACER (see src/bthread/bthread.cpp:160-162). This makes the comment misleading and could hide future concurrency bugs.

            // Bump the version with a release store so that it pairs with the
            // acquire load in TaskGroup::join(): all memory writes made by this
            // bthread become visible to the joining thread. Accessing
            // version_butex atomically also avoids a data race with the read in
            // join(). This path is the only writer of version_butex at runtime.

src/bthread/task_meta.h:98

• This comment describes version_butex as being published/observed with release/acquire, but that is only true for some bump sites (TaskGroup::task_runner in this PR). Other code paths still mutate version_butex differently (e.g. src/bthread/bthread.cpp:160-162 under BRPC_BTHREAD_TRACER). Consider narrowing the wording so it accurately reflects where the release/acquire pairing is implemented.

    // [Not Reset] Backed by a butex (internally `butil::atomic<int>`). The version
    // bump at bthread end is published with a release store, and join() observes
    // it with an acquire load so the joined bthread's prior writes are visible
    // after join() returns.