fix: survive an unreadable directory and a symlink cycle while scanning [patch] - #130
Conversation
…ng [patch] FileScanner walked the tree with SearchOption.AllDirectories and no EnumerationOptions, which put two failures on one unguarded call. Enumeration is lazy, so an UnauthorizedAccessException raised while descending into a single unreadable subdirectory propagated out of the foreach and out of Program.Main, abandoning the whole scan before any file was hashed, reported or deleted -- however much of the tree was readable. A restricted share, an OS-protected folder or a lost+found anywhere under the scan root was enough. Reparse points were also followed, so a directory symlink pointing at one of its own ancestors was descended into repeatedly. Observed against the old code: it took 23 laps of a/b/loop/b/loop/... before dying on an unhandled ArgumentException out of the AbsoluteFilePath conversion, the path having grown past what the semantic type accepts. Walk one directory at a time instead, materializing each listing inside a guard that reports and skips the directory it could not read, and skip reparse points so a cycle cannot be entered. Listing each directory separately is what keeps a failure local to its own directory rather than ending the walk. The unreadable-directory test stages the refusal with UnixFileMode and measures whether it took effect, following DeletionBlock and its test: a privileged process reads the directory regardless, so the test reports itself inconclusive there rather than passing without observing anything. Fixes #125 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd
CI: the one red check is not this PR's
Everything that actually exercises the code is green: Test on ubuntu-latest, windows-latest and macos-latest, plus CodeQL and Two further points confirming it is environmental:
I have not re-run it: Worth noting for review that the unprivileged CI runners are where Generated by Claude Code |
…escent [patch]
The manual walk carried four catch branches that only execute on an
unprivileged process, so SonarCloud measured 70.8% coverage on new code
against a gate of 80%. An uncoverable branch in a tool that deletes files is
worse than the log line it was there to print, so this trades that reporting
for enumeration options the runtime applies itself:
IgnoreInaccessible = true skips an unreadable directory instead of
ending the walk
AttributesToSkip = ReparsePoint cannot descend into a symlink cycle
Both acceptance criteria still hold, and both tests still cover them --
including the symlink-cycle test, which still fails against the original code
with the same unhandled ArgumentException after 23 laps of a/b/loop/...
Hidden and system files stay in scope. A bare new EnumerationOptions() would
skip them, where the SearchOption overload this replaces did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd
Reworked after the quality gate failed — simpler, and the gate failure was a fair hitSonarCloud failed this PR on 70.8% coverage on new code (gate requires ≥80%), and it was right to. My manual directory walk carried four So I've replaced the 102-line walk with the other option the issue offered — enumeration options the runtime applies itself (75 lines deleted, 30 added): IgnoreInaccessible = true // skips an unreadable directory instead of ending the walk
AttributesToSkip = ReparsePoint // cannot descend into a symlink cycleBoth acceptance criteria still hold, and both tests still cover them. Re-verified the symlink-cycle test against the original code after the rework — it still fails with the same unhandled Two notes for review:
The Generated by Claude Code |
|
2 participants
Fixes #125
The problem
FileScanner.ScanForFileswalked the tree withSearchOption.AllDirectoriesand noEnumerationOptions, putting two distinct failures on one unguarded call.Unreadable subdirectory. Enumeration is lazy, so an
UnauthorizedAccessExceptionraised while descending into a single unreadable subdirectory propagated out of theforeachand out ofProgram.Main, abandoning the whole scan before any file was hashed, reported or deleted — however much of the tree was readable. Worth noting why the old call throws at all:Directory.EnumerateFiles(path, pattern, SearchOption)resolves to the legacy-compatible options, which setIgnoreInaccessible = false. Only theEnumerationOptionsoverloads default it totrue.Symlink cycle. Reparse points were followed, so a directory symlink pointing at one of its own ancestors was descended into repeatedly.
The fix
Three enumeration options, applied by the runtime:
Two notes for review:
new EnumerationOptions()skips them by default, where theSearchOptionoverload this replaces did not.AttributesToSkipis therefore set explicitly toReparsePointalone — a hidden duplicate is still a duplicate.How this got here
The first version of this PR did the walk by hand — one directory at a time, each listing materialized inside a
try/catchthat reported and skipped a directory it could not read. SonarCloud then failed the quality gate at 70.8% coverage on new code, and it was right to: four of thosecatchbranches only execute in an unprivileged process, so some could not be covered on any runner. An uncoverable error branch in a tool whose job is deleting files is worse than the log line it existed to print, so the walk was replaced with the options above — 75 lines deleted, 30 added, and coverage on new code went to 100%.What that trade cost is the per-directory
Skipped …line:EnumerationOptionsexposes no error hook, so an unreadable directory is now passed over silently. If you'd rather have that reporting back, it needs the manual walk plus a coverage exemption for its error branches.Testing
ScanTerminatesOnADirectorySymlinkCycle— stagesa/b/loop -> a. Verified against the unfixed code (before and after the rework), where it fails outright:23 laps around the cycle before the path outgrew what
AbsoluteFilePathaccepts — the issue's "path-length error rather than terminating", as an unhandled exception out of the scan. With the fix, the real file is found exactly once.ScanCarriesOnPastADirectoryItCannotRead— a readable file on either side of the unreadable directory in the walk order, asserting both are still found. The refusal is staged viaUnixFileModein a newDirectoryReadBlockhelper that then measures whether it took effect, following the existingDeletionBlockandACopyThatCannotBeDeletedIsReportedAndTheRunCarriesOn: a privileged process reads the directory regardless, so under root — or on Windows, where a listing is refused only by ACL and not by any file attribute — the test reports itself inconclusive rather than passing without observing anything. It therefore self-reports as skipped in a root container and does its real work on the unprivileged CI runners.Full suite on the branch: 45 passed, 2 skipped, 0 failed — the 2 skipped being the pre-existing privilege-dependent deletion test and the new one above.
CI is green apart from
github-advanced-security, which fails on an account-level Copilot quota unrelated to this diff; see the comment below for the evidence.🤖 Generated with Claude Code
https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd