fix(list_files): dedupe raced directory entries against seen_dir_paths - #789
Open
thomwebb wants to merge 1 commit into
Open
fix(list_files): dedupe raced directory entries against seen_dir_paths#789thomwebb wants to merge 1 commit into
thomwebb wants to merge 1 commit into
Conversation
PR mpfaffenberger#787 replaced the O(n^2) parent-directory dedup scan with a set, but the set was only populated inside the parent-synthesis branch. A directory can also reach the main append site directly if something on disk swaps a listed path for a directory between rg's enumeration and the os.path.isfile()/isdir() recheck (rg --files lists files only, so this never happens in normal operation). In that TOCTOU race, the main append site could add a directory entry seen_dir_paths didn't know about, so a sibling file synthesizing the same path as a parent (in either order) would duplicate it. The old O(n^2) scan happened to catch this because it checked all of results, not just synthesized parents. Record main-site directory entries in seen_dir_paths too, and skip if one is already there. Covered by two new deterministic tests that mock rg's output to force the race in both orderings; both fail on the pre-fix code with a duplicated directory line and pass after.
thomwebb
force-pushed
the
fix/list-files-toctou-dir-dedup
branch
from
August 17, 2026 22:11
fb1bbbf to
51570bc
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
1 participant
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.
Summary
Tiny non-blocking follow-up to #787, flagged during review of that PR.
_list_filessynthesizes parent-directory entries for a recursive listing and tracks which ones it's already added in aseen_dir_pathsset (#787 replaced an O(n^2) scan with this set). That set is only populated inside the parent-synthesis branch, though — not at the main append site where eachrg --filesentry gets added.rg --filesonly ever lists files, so in normal operation the main append site never seesentry_type == "directory". But the code re-checks each path against the live filesystem (os.path.isfile()/os.path.isdir()) before classifying it, so if something on disk swaps a listed path for a directory between rg's enumeration and that check (TOCTOU), the main site can append a directory entry thatseen_dir_pathsdoesn't know about. If a sibling file also references that same path as a parent — in either order — the directory gets listed twice.The pre-#787 O(n^2) scan happened to avoid this because it checked all of
results, not just synthesized parents, so this is a narrow behavior change introduced by that PR, not a pre-existing bug.Fix
Record directory entries added at the main append site in
seen_dir_pathstoo, and skip appending if the path is already there. Covers both orderings: directory-in-rg-output-before-the-file, and after.Tests
Added
TestListFilesDirectoryToctouRacewith two tests that mocksubprocess.runto force the race deterministically (a real timing-dependent race isn't reliably testable). Verified both fail on the pre-fix code with a duplicated directory line, and pass after the fix.Verification
tests/tools/test_file_operations_coverage.py: 77/77 pass (75 existing + 2 new).tests/tools/: 609/609 pass.ruff check: same 27 pre-existing findings on both touched files, before and after — confirmed by diffing against the pre-fix commit.ruff format --checkclean.Severity is low (worst case is one cosmetic duplicate line, not a crash or data loss) — flagging as a quick correctness follow-up since it was easy to fix and test properly once identified.