Fix false positive in isAbsPathOverlapping path boundary check#14426
Open
soya111 wants to merge 1 commit intografana:mainfrom
Open
Fix false positive in isAbsPathOverlapping path boundary check#14426soya111 wants to merge 1 commit intografana:mainfrom
soya111 wants to merge 1 commit intografana:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a false positive in isAbsPathOverlapping by making the overlap check respect path segment boundaries (so sibling directories that share a string prefix aren’t treated as overlapping).
Changes:
- Update
isAbsPathOverlappingto require a separator boundary when checking parent/child relationships. - Add regression tests covering “string prefix but not ancestor” cases.
- Add a changelog entry documenting the bugfix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/mimir/mimir.go | Adjusts overlap detection logic to avoid prefix-based false positives. |
| pkg/mimir/mimir_test.go | Adds regression test cases for sibling vs ancestor path relationships. |
| CHANGELOG.md | Records the bugfix in the changelog. |
Comments suppressed due to low confidence (1)
pkg/mimir/mimir.go:647
isAbsPathOverlappingusesfilepath.Split/filepath.Abs(OS-specific separators), but the new boundary check hard-codes"/". On Windows paths use\, so this will fail to detect overlaps (false negatives). Consider appendingstring(filepath.Separator)(orstring(os.PathSeparator)) instead of"/"and, if needed, normalize inputs withfilepath.Cleanto avoid doubled separators.
// We append "/" to ensure we match on path segment boundaries and avoid false positives
// like "/data/tsdb" matching "/data/tsdb-compactor/cache".
return strings.HasPrefix(firstAbsPath, secondAbsPath+"/") || strings.HasPrefix(secondAbsPath, firstAbsPath+"/")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
strings.HasPrefix without a trailing "/" causes false positives when one path is a string prefix of another but not a path ancestor (e.g. "/data/tsdb" vs "/data/tsdb-compactor/cache"). Append "/" before comparing to enforce path segment boundaries.
560d375 to
9dd896c
Compare
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
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.
What this PR does
Fixes a false positive in
isAbsPathOverlappingwherestrings.HasPrefixdoes not respect path segment boundaries.For example,
/data/tsdband/data/tsdb-compactor/cacheare sibling directories that should not overlap, butstrings.HasPrefix("/data/tsdb-compactor/cache", "/data/tsdb")returnstruebecause it operates on raw strings without considering/boundaries.The fix appends
"/"before comparing, so only true parent/child relationships are detected as overlapping.Which issue(s) this PR fixes or relates to
N/A — discovered while configuring component storage paths.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.Note
Low Risk
Small, well-tested change to config sanity checking that only affects startup validation for filesystem paths and reduces false positives.
Overview
Fixes a config validation bug where filesystem path overlap detection could incorrectly flag sibling directories as overlapping when one path was only a string prefix of the other.
isAbsPathOverlappingnow treats/as overlapping with any absolute path and uses aHasPrefix(path, other+"/")boundary check to only detect true parent/child directory relationships; tests were extended with regression cases and the fix is noted inCHANGELOG.md.Written by Cursor Bugbot for commit 9dd896c. This will update automatically on new commits. Configure here.