Inspect segments under the per-segment lock in needReloadSegments()#18901
Open
gortiz wants to merge 1 commit into
Open
Inspect segments under the per-segment lock in needReloadSegments()#18901gortiz wants to merge 1 commit into
gortiz wants to merge 1 commit into
Conversation
needReloadSegments() inspected each segment via ImmutableSegment.isReloadNeeded() -> SegmentPreProcessor.needProcess(), which reads the segment directory, while holding only a reference count and not the per-segment lock that reload / replace / offload acquire. That let the reload-need check race with a concurrent reload of the same segment mutating or releasing its backing resources. Inspect each segment under getSegmentLock(segmentName), re-acquiring it fresh under the lock so a segment already replaced by a reload is not inspected. Segment names are snapshotted first so the lock is not taken while holding segment references. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18901 +/- ##
============================================
- Coverage 64.80% 57.00% -7.80%
+ Complexity 1347 7 -1340
============================================
Files 3393 2607 -786
Lines 211664 152959 -58705
Branches 33305 24879 -8426
============================================
- Hits 137160 87193 -49967
+ Misses 63431 58346 -5085
+ Partials 11073 7420 -3653
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jackie-Jiang
reviewed
Jul 2, 2026
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
Contributor
There was a problem hiding this comment.
Is this necessary? I'd expect needReloadSegments() to be fast. Even if we switch to the per-segment lock, there is still no guarantee segment won't be deleted/replaced just after the check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
BaseTableDataManager.needReloadSegments()inspects each segment viaImmutableSegment.isReloadNeeded()→SegmentPreProcessor.needProcess(), which reads thesegment directory. It did so while holding only a reference count (from
acquireAllSegments()),not the per-segment lock (
getSegmentLock) thatreloadSegment/replaceSegment/offloadSegmentacquire. As a result, the reload-need check can run concurrently with a reloadof the same segment that is mutating or releasing that segment's backing resources.
This change inspects each segment under
getSegmentLock(segmentName), and re-acquires thesegment fresh under the lock so a segment that a reload has already replaced is not inspected.
Segment names are snapshotted first, so the lock is never taken while holding segment references
(avoiding lock/refcount ordering surprises).
Notes
races with a reload / replace / offload of the same segment.
serializes with an in-progress reload of that segment.
Testing