From f2366a3c013d61462a1766c935251f49999e76ee Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 15 Sep 2026 06:26:12 +0000 Subject: [PATCH 1/2] fix: catch UnauthorizedAccessException when deleting a duplicate [patch] File.Delete throws UnauthorizedAccessException -- not IOException -- when the target is read-only or the process lacks permission to unlink it. Nothing in the chain up to Program.Main caught it, so one protected duplicate ended a destructive run partway through: files already deleted stayed deleted, the remaining groups were never processed, and the user saw neither the summary nor any report of what had happened. The refusal is now recorded against that one file and the run carries on, the same way an IOException already did. Fixes ktsu-dev/FileDeduplicator#113 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011LU3aHGPGmAeEEVUGDiTK7 --- FileDeduplicator.Test/DeduplicatorTests.cs | 44 +++++++- FileDeduplicator.Test/DeletionBlock.cs | 113 +++++++++++++++++++++ FileDeduplicator/Deduplicator.cs | 26 ++++- 3 files changed, 179 insertions(+), 4 deletions(-) create mode 100644 FileDeduplicator.Test/DeletionBlock.cs diff --git a/FileDeduplicator.Test/DeduplicatorTests.cs b/FileDeduplicator.Test/DeduplicatorTests.cs index dea0d86..f792498 100644 --- a/FileDeduplicator.Test/DeduplicatorTests.cs +++ b/FileDeduplicator.Test/DeduplicatorTests.cs @@ -323,7 +323,8 @@ public void AFileThatVanishedBeforeDeletionIsReportedNotCounted() /// /// A path whose file was replaced by a directory of the same name cannot be re-read, so it /// must be skipped. Attempting the delete instead would raise - /// , which the delete path does not catch. + /// and report the directory as a failed deletion, + /// which says nothing about why the path was left alone. /// [TestMethod] public void APathReplacedByADirectoryIsSkippedRatherThanDeleted() @@ -375,6 +376,47 @@ public void UntouchedDuplicatesAreDeletedWithNothingSkipped() Assert.IsEmpty(result.Errors); } + /// + /// A copy the process is not allowed to remove must cost that one file. It is reported as an + /// error, the group it belongs to carries on, and so does every group after it -- otherwise a + /// single read-only duplicate aborts a destructive run partway through, with files already + /// deleted and no summary saying which. + /// + [TestMethod] + public void ACopyThatCannotBeDeletedIsReportedAndTheRunCarriesOn() + { + // Arrange -- two groups, one of which holds a copy that cannot be removed + using TempTree tree = new(); + AbsoluteFilePath keeper = tree.Write("a.txt", "shared"); + AbsoluteFilePath undeletable = tree.Write("protected/bbb.txt", "shared"); + AbsoluteFilePath otherKeeper = tree.Write("x.txt", "other content"); + AbsoluteFilePath otherCopy = tree.Write("yy.txt", "other content"); + IReadOnlyList duplicates = Duplicates(FileHasher.HashFiles([keeper, undeletable, otherKeeper, otherCopy])); + Assert.HasCount(2, duplicates, "The two contents should form two separate groups."); + + using DeletionBlock block = new(undeletable); + + if (!block.IsEnforced) + { + Assert.Inconclusive("This process deletes through a write-protected directory, so the refusal under test cannot be staged. Run the tests as an unprivileged user."); + } + + // Act + DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates); + + // Assert -- the refusal is reported, not thrown + Assert.ContainsSingle(result.Errors); + Assert.Contains(undeletable.WeakString, result.Errors[0]); + Assert.IsTrue(TempTree.Exists(undeletable), "A copy that could not be deleted must still be on disk."); + Assert.IsEmpty(result.SkippedFiles, "The file was a genuine duplicate; it failed to delete rather than failing verification."); + + // Assert -- the rest of the work still happened + Assert.AreEqual(1, result.DeletedCount); + Assert.IsFalse(TempTree.Exists(otherCopy), "An unrelated duplicate group must still be deduplicated."); + Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive."); + Assert.IsTrue(TempTree.Exists(otherKeeper), "The keeper must survive."); + } + /// /// Deleting nothing must report nothing, rather than throwing on an empty group list. /// diff --git a/FileDeduplicator.Test/DeletionBlock.cs b/FileDeduplicator.Test/DeletionBlock.cs new file mode 100644 index 0000000..09ee3ea --- /dev/null +++ b/FileDeduplicator.Test/DeletionBlock.cs @@ -0,0 +1,113 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.FileDeduplicator.Test; + +using ktsu.Semantics.Paths; + +/// +/// Makes a file refuse to be deleted for the lifetime of the block, and puts the permissions back +/// on disposal. +/// +/// +/// The two platforms refuse for different reasons. Windows will not unlink a file carrying +/// . Unix ignores that bit when deleting -- unlink is governed +/// by write permission on the containing directory -- so the directory is what gets write-protected +/// there. Both surface as out of +/// , which is the failure the delete path has to survive. +/// +internal sealed class DeletionBlock : IDisposable +{ + private readonly string file; + private readonly string directory; + private readonly string probe; + private readonly UnixFileMode originalDirectoryMode; + + /// + /// Gets whether the block actually holds. A process running as root deletes through + /// write-protected directories regardless, so the staged failure never happens and a test + /// relying on it has nothing to observe. + /// + internal bool IsEnforced { get; } + + /// + /// Blocks deletion of a file, then measures whether the block took effect. + /// + /// The file to protect. + internal DeletionBlock(AbsoluteFilePath target) + { + file = target.WeakString; + directory = Path.GetDirectoryName(file)!; + probe = Path.Combine(directory, $"{Path.GetFileName(file)}.deletion-probe"); + + // Written before the block goes on, because on Unix the block closes the directory to new + // files as well as to deletions. + File.WriteAllText(probe, "probe"); + + originalDirectoryMode = OperatingSystem.IsWindows() ? default : File.GetUnixFileMode(directory); + Block(); + IsEnforced = ProbeRefusesDeletion(); + } + + private void Block() + { + if (OperatingSystem.IsWindows()) + { + File.SetAttributes(file, File.GetAttributes(file) | FileAttributes.ReadOnly); + File.SetAttributes(probe, File.GetAttributes(probe) | FileAttributes.ReadOnly); + } + else + { + File.SetUnixFileMode(directory, originalDirectoryMode & ~(UnixFileMode.UserWrite | UnixFileMode.GroupWrite | UnixFileMode.OtherWrite)); + } + } + + private void Unblock() + { + if (OperatingSystem.IsWindows()) + { + ClearReadOnly(file); + ClearReadOnly(probe); + } + else + { + File.SetUnixFileMode(directory, originalDirectoryMode); + } + } + + private static void ClearReadOnly(string path) + { + if (File.Exists(path)) + { + File.SetAttributes(path, File.GetAttributes(path) & ~FileAttributes.ReadOnly); + } + } + + /// + /// Spends a throwaway file to find out whether this process is actually refused, rather than + /// guessing from the platform and the user id. + /// + /// if deleting the probe was refused. + private bool ProbeRefusesDeletion() + { + try + { + File.Delete(probe); + return false; + } + catch (UnauthorizedAccessException) + { + return true; + } + } + + /// + public void Dispose() + { + Unblock(); + + if (File.Exists(probe)) + { + File.Delete(probe); + } + } +} diff --git a/FileDeduplicator/Deduplicator.cs b/FileDeduplicator/Deduplicator.cs index 3975caa..c504b29 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -78,9 +78,15 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList + /// Records a copy that survived because it could not be removed, so the run can carry on and + /// still account for it at the end. + /// + /// The file that could not be deleted. + /// Why the delete was refused. + /// The list to record the failure on. + private static void RecordDeleteFailure(AbsoluteFilePath file, Exception ex, List errors) + { + string error = $" Error deleting {file}: {ex.Message}"; + errors.Add(error); + Console.WriteLine(error); + } + /// /// Preserves every copy in a group, because the copy that would have been kept no longer /// holds the group's content. From f0e8ad0a638615a5ac58f70bf6535242cd35c3d7 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 15 Sep 2026 06:36:00 +0000 Subject: [PATCH 2/2] refactor: keep the IOException catch body as it was [patch] Folding the two identical catch bodies into a shared helper rewrote the IOException block, which pulled its never-covered lines into SonarCloud's new-code measure and failed the quality gate at 75%. That branch has no test and predates this change, so the tidy-up is not worth dragging it in. The two bodies now repeat, the way the two catch blocks in StillMatchesGroup already do, and the production diff is purely the new UnauthorizedAccessException block -- every line of which the new test executes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011LU3aHGPGmAeEEVUGDiTK7 --- FileDeduplicator/Deduplicator.cs | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/FileDeduplicator/Deduplicator.cs b/FileDeduplicator/Deduplicator.cs index c504b29..e43b444 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -78,7 +78,9 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList - /// Records a copy that survived because it could not be removed, so the run can carry on and - /// still account for it at the end. - /// - /// The file that could not be deleted. - /// Why the delete was refused. - /// The list to record the failure on. - private static void RecordDeleteFailure(AbsoluteFilePath file, Exception ex, List errors) - { - string error = $" Error deleting {file}: {ex.Message}"; - errors.Add(error); - Console.WriteLine(error); - } - /// /// Preserves every copy in a group, because the copy that would have been kept no longer /// holds the group's content.