Skip to content

[fix](be) Serialize bvar TLS agent lifetime updates - #67692

Closed
hello-stephen wants to merge 1 commit into
apache:masterfrom
hello-stephen:fix-bvar-agent-combiner-weakptr-race
Closed

[fix](be) Serialize bvar TLS agent lifetime updates#67692
hello-stephen wants to merge 1 commit into
apache:masterfrom
hello-stephen:fix-bvar-agent-combiner-weakptr-race

Conversation

@hello-stephen

@hello-stephenhello-stephen commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: N/A

Related PRs: #64040, #66977

Problem Summary:

BE UT build 1040681 hit an ASAN heap-use-after-free in TableRpcQpsRegistryTest.ConcurrentRecordAndCleanup. The failure occurs while the cleanup thread destroys a bvar::Adder and another thread tears down its TLS agent.

The bvar lifetime patch stores each agent's combiner in a std::weak_ptr. Agent::~Agent() may call lock() at the same time as AgentCombiner::clear_all_agents() calls reset() on that same weak pointer. Concurrent non-const access to the same weak_ptr object is not safe and can corrupt the shared ownership control block.

This PR:

  • serializes accesses to each agent's weak pointer;
  • uses an atomic attachment flag to keep the reducer update hot path lock-free;
  • repeats the existing concurrent registry test to exercise cross-thread combiner destruction more aggressively.

The failing build was for #67679 at 094f6499cbcb58acfaf99f1d0f942454c871b9eb. That PR only changes zero-argument COUNT aggregate-state files and does not touch this registry or bvar. The unsafe weak-pointer implementation came from #64040; the registry cleanup path that exposed it was introduced by #66977.

Release note

None

Check List (For Author)

  • Test:
    • ClangFormat 16 check
    • Build hygiene check
    • Applied all brpc patches in build order
    • GCC 15 C++17 syntax instantiation of the patched AgentCombiner
    • TeamCity BE UT ASAN: pending this PR's exact-SHA run
  • Behavior changed: No
  • Does this need documentation: No

### What problem does this PR solve?
Issue Number: N/A
Related PR: apache#66977
Problem Summary:
Concurrent destruction of a bvar combiner and TLS agent teardown can access
the same std::weak_ptr at the same time. This violates the weak_ptr concurrency
contract and can corrupt its shared ownership state, which was observed as an
ASAN heap-use-after-free in TableRpcQpsRegistryTest.ConcurrentRecordAndCleanup.
Serialize accesses to each agent's weak_ptr while using an atomic attachment
flag for the reducer update hot path. Amplify the existing concurrent cleanup
test to exercise repeated cross-thread combiner destruction.
### Release note
None
### Check List (For Author)
- Test:
- ClangFormat 16 check
- Build hygiene check
- Applied all brpc patches in build order
- GCC 15 C++17 syntax instantiation of the patched AgentCombiner
- Behavior changed: No
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
ContributorAuthor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@hello-stephen

Copy link
Copy Markdown
ContributorAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
ContributorAuthor

@vchag@bobhan1 Could you please take a look? #64040 introduced the weak-pointer lifetime implementation, and #66977 introduced the concurrent counter cleanup path that exposed this same-object weak_ptr race in BE UT ASAN.

@hello-stephen

Copy link
Copy Markdown
ContributorAuthor

BE UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage63.40% (29891/47144)
Line Coverage48.45% (313963/647965)
Region Coverage43.95% (253199/576156)
Branch Coverage45.57% (118082/259119)

@hello-stephen

Copy link
Copy Markdown
ContributorAuthor

Closing this PR as requested. The diagnosis will be moved to Jira for an owner-led fix because this draft touches the vendored brpc patch and needs upstream/domain-owner review before selecting the repair boundary.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@hello-stephen