Doris-Breakwater commented on issue #67719: URL: https://github.com/apache/doris/issues/67719#issuecomment-5599245017
## Initial triage **Verdict:** this report identifies a credible, code-confirmed concurrency-safety defect, but the attribution of the observed crash family to the stated daemon-thread access path is not yet confirmed. I recommend treating the synchronization fix as valid while requesting the incident evidence below before declaring the production root cause proven. ### Verified facts - The issue is currently open with no labels, assignee, milestone, or linked PR. - At the reported commit `13a108d04b`, `Segment::_footer_pb` is a plain `std::weak_ptr<SegmentFooterPB>`. `Segment::get_metadata_size()` and `Segment::_get_segment_footer()` call `lock()` on that same member, while `_get_segment_footer()` also assigns to it, without a mutex protecting the member. - The same pattern is still present in upstream `master` at `2e89f3a81cbf0effe06491a383109ab36ec72923` and `branch-4.1` at `73fea8a58956d020231bdc053029107e8451f37c` (the branch tips checked during this triage). - Concurrent operations on the same non-atomic `std::weak_ptr` object require synchronization. Therefore, if two threads reach these accesses on the same `Segment`, the code has undefined behavior. - Such same-object concurrency is mechanically possible: `SegmentLoader` caches and returns shared `Segment` instances, and not all post-open `_get_segment_footer()` callers share one mutex. `ColumnReaderCache` serializes its own footer-callback calls with `_cache_mutex`, but `Segment::traverse_column_meta_pbs()` calls `_get_segment_footer()` directly. Thus the standalone TSAN example demonstrates the relevant language-level defect, and the Doris code contains reachable access paths that should not rely on an undocumented single-thread invariant. ### Important discrepancy / evidence gap The daemon call chain stated in the issue does not exist in the referenced upstream source. At `13a108d04b` (and at both branch tips above), `MemoryProfile::refresh_memory_overview_profile()` calls `MetadataAdder<Segment>::get_all_segments_estimate_size()`, which only reads `g_segment_estimate_mem_bytes`; it does not enumerate `Segment` objects or call `Segment::get_metadata_size()`. Consequently, the following should be separated: - **Confirmed:** `_footer_pb` lacks the synchronization required for concurrent access. - **Plausible but not yet proven:** this race caused the reported `ByteSizeLong`, destructor, and unrelated SIGSEGV crashes. - **Currently contradicted by upstream source:** `memory_maintenance_thread -> refresh_memory_overview_profile -> get_all_segments_estimate_size -> Segment::get_metadata_size` at the stated commit. The overlapping destructor symptom in #64826 is worth cross-referencing, but it is not enough to establish that the two issues have the same root cause. ### Information requested 1. Please attach at least one complete, symbolized `be.out`/core backtrace for the `Segment::get_metadata_size()` crash and one for the `SegmentFooterPB` destructor crash, including all frames above the shown functions, thread names, timestamps, and binary build ID. 2. Because this is a local build, please provide the source diff from `13a108d04b` for at least `metadata_adder.h`, `memory_profile.cpp`, `segment.h`, and `segment.cpp` (or confirm that these files are byte-identical to upstream). This is necessary to reconcile the daemon call-chain discrepancy. 3. If available, attach a TSAN report from a Doris test/build that names the actual `Segment::_footer_pb` accesses. The standalone reproducer proves the C++ pattern but not the production execution pair. 4. For the canary result, please include observation duration/load and the pre-fix recurrence baseline; absence of crashes is supportive but not independently conclusive for an intermittent failure. ### Recommended next step The proposed dedicated `mutable std::mutex` is an appropriate minimal fix for C++17. The PR should protect **every** access to the `_footer_pb` member: both `lock()` sites and the rebind. Copy the resulting `shared_ptr` while holding the mutex, then perform `ByteSizeLong()`, footer parsing, page-cache lookup/insert, and I/O outside the critical section. This removes the undefined behavior without holding a segment-local lock across expensive work; duplicate cache-miss work remains possible, as it is today. Please add or run a concurrency-focused test under TSAN if the project test setup supports it, plus the relevant BE unit tests. A normal non-TSAN stress test alone may not reliably detect this race. Once a PR is opened, link it here and target both `master` and `branch-4.1` according to the project backport policy. Breakwater-GitHub-Analysis-Slot: slot_b3ebbbe1cd9e -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
