Repository navigation
[fix](be) Guard Segment::_footer_pb against concurrent re-assignment - #69811
airborne12 wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed all three changed files at ce706bb and traced Segment loading, column-reader callbacks, page-cache eviction/replacement, metadata accounting, and the test fixture. No actionable new findings. There are no existing inline findings to retain, and no additional user focus was supplied.
- Goal and regression: the two accessors serialize every production access to the same footer weak_ptr, including metadata sizing. The new eviction stress test exercises retrieval and metadata reads; its execution was not verified here.
- Scope and reuse: the mutex, two private helpers, three call-site substitutions, and one existing-fixture test form a focused change consistent with adjacent locking patterns.
- Concurrency: cached Segment instances can be shared among readers. The new per-instance mutex protects weak_ptr promotion and reassignment independently of callers' locks.
- Work inside locks: only weak-pointer operations run under the new mutex; file reads, parsing, protobuf sizing, and page-cache operations remain outside it.
- Lock order: column-reader cache locking can precede the footer mutex, but neither footer accessor acquires another application lock or invokes a callback. No new lock cycle was identified.
- Lifecycle and memory: cache handles pin entries during extraction, and copied shared_ptrs keep returned footers alive across eviction and replacement. Weak ownership adds no cycle; sizeof(Segment) accounting includes the mutex. No new static initialization dependency or payload allocation is introduced.
- Configuration: no new configuration or dynamic-update behavior.
- Compatibility: no storage format, wire contract, persisted layout, dependency, or linker changes. The synchronization primitive is already used by Segment.
- Thrift/protobuf definitions: none changed; field-ID comparisons are inapplicable.
- Parallel paths: metadata sizing and both footer retrieval paths use the helpers. Local and cloud/remote consumers share this Segment implementation; no separate footer access requires the fix.
- Conditions and comments: no production branch changes. The ownership comment explains the race; duplicate concurrent misses may return separate equivalent footer objects safely.
- Test coverage: eight workers attach thread contexts, check status/footer content/metadata sizing, and join before fixture teardown. Existing cache and I/O-context tests remain. Test discovery and private access follow the existing BE-UT configuration, with no new source .cpp inclusion.
- Test results: no expected-result files changed. No builds, tests, sanitizers, or benchmarks were run during this static review; the author's reported runs were not independently reproduced.
- Observability: existing cache-hit/miss diagnostics and parse-error context remain adequate for this synchronization change; no new noisy logging.
- Transactions and persistence: no changes to version visibility, delete bitmaps, EditLog, replay, or failover behavior.
- Data writes and crash atomicity: no persistent-data mutation or new write protocol; footer parsing and error propagation remain unchanged.
- FE/BE variables: none added; no propagation changes required.
- Performance: short constant-time locking and one per-Segment mutex are the added costs; parsing stays outside the mutex. No new redundant scan or copying of footer payloads was found. Performance was not measured.
- Other checks: no new ignored Status, nullable handling, header dependency, explicit-instantiation pairing, or unity-build concern was identified.
The review converged in round 1: both independent reviewers returned NO_NEW_VALUABLE_FINDINGS, all changed code and applicable checkpoints were covered, and both initial risk focuses were dismissed with ownership/lock/test evidence. No unresolved correctness candidate remains. Validation limits: the stress test is scheduling-dependent because active readers can keep a footer alive after erasure; it does not guarantee reload/reassignment overlap on every run. Runtime, sanitizer, and end-to-end remote/variant validation were outside this static-only review.
Review converged after 1 round(s).
yiguolei
left a comment
yiguolei
left a comment
There was a problem hiding this comment.
Yes—C++20 provides std::atomic<std::weak_ptr>, which could replace the explicit mutex here:
std::atomic<std::weak_ptr> _footer_pb;
// Read
auto footer = _footer_pb.load(std::memory_order_acquire).lock();
// Write
_footer_pb.store(std::weak_ptr{footer},
std::memory_order_release);
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Segment::_footer_pb is a std::weak_ptr to the footer owned by the storage page cache. _get_segment_footer() calls lock() on it and, once the cache entry has been evicted, parses the footer again and assigns the weak_ptr; get_metadata_size() calls lock() as well. A Segment is shared by every reader of its rowset (queries, compaction, statistics), so lock() and the assignment run on the same weak_ptr object concurrently. That is a data race: the standard only allows concurrent use of distinct weak_ptr objects, and the control block counts can be corrupted so that a footer still held through a shared_ptr gets destroyed. Every access to _footer_pb now goes through two small accessors that take a mutex. The new test fetches the footer from 8 threads while half of them keep evicting its cache page. On master's segment.cpp, ASAN reported a heap-use-after-free within 20 runs: a thread read the footer it had just obtained (SegmentFooterPB::ByteSizeLong) after another thread had freed it in std::weak_ptr assignment (__weak_count::operator= -> _M_weak_release -> _M_destroy). With the change, 20 of 20 runs passed without an ASAN report. The race was found while investigating BE crashes on large VARIANT tables under heavy compaction; those crashes had a different cause (the host kernel with jemalloc MADV_FREE). ### Release note Fix a data race on the cached segment footer that could free a footer still in use. ### Check List (For Author) - Test: Unit Test (SegmentFooterCacheTest.ConcurrentGetSegmentFooterWithEviction; ASAN heap-use-after-free without the change within 20 runs, 20/20 clean with it; the other SegmentFooterCacheTest cases pass) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… mutex Review suggestion: the C++20 std::atomic<std::weak_ptr<T>> serializes lock() against the re-assignment just as the mutex did, without the two accessors. The macOS build uses libc++, which has no std::atomic<std::weak_ptr<T>> (libc++ 20 rejects it: std::atomic<T> requires a trivially copyable T), and std::atomic_load has no weak_ptr overload. So common/atomic_shared_ptr.h gets an atomic_weak_ptr next to atomic_shared_ptr: the standard type with libstdc++, a weak_ptr behind a mutex with libc++. With libstdc++ the member is 16 bytes like the plain weak_ptr, so Segment no longer grows by the 40-byte mutex that get_metadata_size() counts through sizeof(Segment). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ce706bb to
36327d6
Compare
Thanks, done in 36327d6:
The branch is rebased on the current master, and all six SegmentFooterCacheTest cases pass 20 of 20 repeats under ASAN with no report. |
|
/review |
|
run buildall |
1 similar comment
|
run buildall |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
TPC-H: Total hot run time: 28409 ms |
TPC-DS: Total hot run time: 151262 ms |
ClickBench: Total hot run time: 24.21 s |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
TPC-H: Total hot run time: 28947 ms |
TPC-DS: Total hot run time: 151697 ms |
ClickBench: Total hot run time: 24.22 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Reviewed the complete exact-head diff for PR 69811 (36327d6) across atomic_shared_ptr.h, Segment footer access, and the SegmentFooterCacheTest. No actionable new finding was verified, and there are no existing P0/P1 inline findings to retain.
The change meets the stated goal: C++20 uses atomic weak-pointer load/store, libc++ uses a mutex-backed equivalent, and all production _footer_pb accesses are covered. Segment is explicitly non-copyable; footer cache handles keep erased entries alive until release; parsed SegmentFooterPB values are finalized before publication and consumed read-only. The stress test attaches thread memory context, exercises concurrent eviction and footer reads, checks status/content/metadata, joins workers, and leaves fixture teardown ordered. No configuration, protocol, persistence, transaction, nullable, or FE/BE propagation changes apply.
The review is static-only: no build, test, sanitizer, benchmark, or runtime execution was performed. The stress test is scheduling-dependent and may not force a reload overlap on every run. No additional user focus was supplied. Independent child review workers repeatedly failed before returning due model-capacity errors, so the required independent-subagent convergence condition could not be proven even after the three bounded rounds; the verified main-agent sweep and all candidate dispositions are recorded in the shared ledger.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The three-round cap was reached without independent subagent returns: every replacement child review failed with the same model-capacity error before producing a ledger section. Main-agent coverage completed all four changed files, upstream/downstream cache and lifecycle paths, test paths, and risk dispositions; no unresolved candidate or uncovered changed-file scope remains, but the contract requires converged=false when the independent-subagent condition is unproven.
|
similar PR #67720 |
|
Thanks, I missed #67720. It fixes the same race and also removes the LRU bypass, as suggested there, so I'm closing this one in favor of it. I left two notes on #67720 (#67720 (comment)): the existing |
… test ### What problem does this PR solve? Issue Number: close apache#67719 Related PR: apache#67720 Problem Summary: PR apache#67720 removed Segment::_footer_pb, but be/test/storage/segment/segment_footer_cache_test.cpp still called segment_ptr->_footer_pb.reset() in GetSegmentFooterPropagatesIoContext, so the BE UT build failed to compile. Per review feedback in apache#67720 (apache#67720 (comment)), drop the stale line: the cache erase() right before it already forces the reload through the cache-only path, so the SyncPoint on Segment::_parse_footer::io_ctx still fires. Also port SegmentFooterCacheTest.ConcurrentGetSegmentFooterWithEviction from apache#69811 (thanks to @airborne12 for the original reproducer): 8 threads call _get_segment_footer() on a segment while half of them keep erasing the footer's cache page; every caller must get a valid, identical footer regardless of concurrent evictions. Adapted to this PR's cache-only design: the get_metadata_size() check is dropped because that method no longer reads the footer. ### Release note None ### Check List (For Author) - Test: Unit Test (this change itself is a test; BE UT run deferred to CI) - Behavior changed: No - Does this need documentation: No
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Segment::_footer_pb is a std::weak_ptr to the footer owned by the storage page
cache. _get_segment_footer() calls lock() on it and, once the cache entry has been evicted,
parses the footer again and assigns the weak_ptr; get_metadata_size() calls lock() as well. A
Segment is shared by every reader of its rowset (queries, compaction, statistics), so lock() and
the assignment run on the same weak_ptr object concurrently. That is a data race: the standard
only allows concurrent use of distinct weak_ptr objects, and the control block counts can be
corrupted so that a footer still held through a shared_ptr gets destroyed.
_footer_pb is now an atomic_weak_ptr, loaded with acquire and stored with release.
atomic_weak_ptr is new in common/atomic_shared_ptr.h, next to atomic_shared_ptr. With libstdc++ it
is the C++20 std::atomic<std::weak_ptr>. libc++, which the macOS build uses, has no
std::atomic<std::weak_ptr> (libc++ 20 rejects it), so there it is a weak_ptr guarded by a
mutex, the same way atomic_shared_ptr falls back for shared_ptr. With libstdc++ the member keeps
the 16 bytes of the plain weak_ptr, so Segment does not grow.
The new test fetches the footer from 8 threads while half of them keep evicting its cache page.
On master's segment.cpp, ASAN reported a heap-use-after-free within 20 runs: a thread read the
footer it had just obtained (SegmentFooterPB::ByteSizeLong) after another thread had freed it in
std::weak_ptr assignment (__weak_count::operator= -> _M_weak_release -> _M_destroy). With the
change, rebased on the current master, all six SegmentFooterCacheTest cases passed 20 of 20
repeats without an ASAN report.
The race was found while investigating BE crashes on large VARIANT tables under heavy
compaction; those crashes had a different cause (the host kernel with jemalloc MADV_FREE).
Release note
Fix a data race on the cached segment footer that could free a footer still in use.
Check List (For Author)
heap-use-after-free without the change within 20 runs; with it all SegmentFooterCacheTest
cases pass 20 of 20 repeats under ASAN).
atomic_weak_ptr was also stress-tested on its own (8 threads storing and locking) with
libstdc++ and with libc++ 20 (-DUSE_LIBCPP), each with and without ASAN.
🤖 Generated with Claude Code