fix: re-verify each file's hash before deleting it [minor]

FileDeduplicator.Test/DeduplicatorTests.cs

@@ -232,6 +232,149 @@ public void ReclaimedBytesCountsOnlyTheDeletedCopies()
 		Assert.AreEqual(200, result.BytesReclaimed);
 	}
 
+	/// <summary>
+	/// Grouping happens before an interactive confirmation prompt of unbounded length, so a file
+	/// can stop being a duplicate before the delete pass runs. Such a file must be preserved and
+	/// reported, never deleted on the strength of the stale hash.
+	/// </summary>
+	[TestMethod]
+	public void AFileThatChangedAfterGroupingIsPreservedAndReported()
+	{
+		// Arrange -- three copies; a.txt is the keeper, bb.txt and ccc.txt are up for deletion
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "shared");
+		AbsoluteFilePath changed = tree.Write("bb.txt", "shared");
+		AbsoluteFilePath stillDuplicate = tree.Write("ccc.txt", "shared");
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(FileHasher.HashFiles([keeper, changed, stillDuplicate]));
+
+		// Arrange -- bb.txt is rewritten during the confirmation pause, as a sync client or an
+		// editor autosave would do
+		_ = tree.Write("bb.txt", "no longer the same content");
+
+		// Act
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates);
+
+		// Assert
+		Assert.IsTrue(TempTree.Exists(changed), "A file that is no longer a duplicate must not be deleted.");
+		Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive.");
+		Assert.IsFalse(TempTree.Exists(stillDuplicate), "A copy that is still identical should still be deleted.");
+		Assert.AreEqual(1, result.DeletedCount);
+		Assert.ContainsSingle(result.SkippedFiles);
+		Assert.AreEqual(changed, result.SkippedFiles[0].Path);
+		Assert.Contains("changed", result.SkippedFiles[0].Reason);
+		Assert.IsEmpty(result.Errors);
+	}
+
+	/// <summary>
+	/// If the copy being kept changed during the pause, deleting the rest would destroy the only
+	/// remaining copies of the grouped content, so the whole group must be left alone.
+	/// </summary>
+	[TestMethod]
+	public void AChangedKeeperPreservesTheWholeGroup()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "shared");
+		AbsoluteFilePath copyOne = tree.Write("bb.txt", "shared");
+		AbsoluteFilePath copyTwo = tree.Write("ccc.txt", "shared");
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(FileHasher.HashFiles([keeper, copyOne, copyTwo]));
+
+		// Arrange -- the keeper is rewritten during the confirmation pause
+		_ = tree.Write("a.txt", "the keeper was overwritten");
+
+		// Act
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates);
+
+		// Assert
+		Assert.AreEqual(0, result.DeletedCount);
+		Assert.AreEqual(0, result.BytesReclaimed);
+		Assert.HasCount(2, result.SkippedFiles);
+		Assert.IsTrue(TempTree.Exists(copyOne), "The grouped content must survive somewhere.");
+		Assert.IsTrue(TempTree.Exists(copyTwo), "The grouped content must survive somewhere.");
+	}
+
+	/// <summary>
+	/// A file that disappears before the delete pass must be reported as skipped rather than
+	/// counted among the deletions, so the reclaimed total stays honest.
+	/// </summary>
+	[TestMethod]
+	public void AFileThatVanishedBeforeDeletionIsReportedNotCounted()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "shared");
+		AbsoluteFilePath vanishing = tree.Write("bb.txt", "shared");
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(FileHasher.HashFiles([keeper, vanishing]));
+
+		// Arrange -- something else removes it during the confirmation pause
+		File.Delete(vanishing.WeakString);
+
+		// Act
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates);
+
+		// Assert
+		Assert.AreEqual(0, result.DeletedCount);
+		Assert.AreEqual(0, result.BytesReclaimed);
+		Assert.ContainsSingle(result.SkippedFiles);
+		Assert.AreEqual(vanishing, result.SkippedFiles[0].Path);
+		Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive.");
+	}
+
+	/// <summary>
+	/// 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
+	/// <see cref="UnauthorizedAccessException"/>, which the delete path does not catch.
+	/// </summary>
+	[TestMethod]
+	public void APathReplacedByADirectoryIsSkippedRatherThanDeleted()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "shared");
+		AbsoluteFilePath replaced = tree.Write("bb.txt", "shared");
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(FileHasher.HashFiles([keeper, replaced]));
+
+		// Arrange -- during the confirmation pause the file becomes a directory of the same name
+		File.Delete(replaced.WeakString);
+		_ = Directory.CreateDirectory(replaced.WeakString);
+
+		// Act
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates);
+
+		// Assert
+		Assert.AreEqual(0, result.DeletedCount);
+		Assert.ContainsSingle(result.SkippedFiles);
+		Assert.AreEqual(replaced, result.SkippedFiles[0].Path);
+		Assert.IsTrue(Directory.Exists(replaced.WeakString), "The directory now at that path must be left alone.");
+		Assert.IsTrue(TempTree.Exists(keeper), "The keeper must survive.");
+	}
+
+	/// <summary>
+	/// The re-verification must not degrade into skipping everything: untouched duplicates are
+	/// still deleted, and nothing is reported as preserved.
+	/// </summary>
+	[TestMethod]
+	public void UntouchedDuplicatesAreDeletedWithNothingSkipped()
+	{
+		// Arrange
+		using TempTree tree = new();
+		Dictionary<AbsoluteFilePath, string> hashes = FileHasher.HashFiles(
+		[
+			tree.Write("a.txt", "shared"),
+			tree.Write("bb.txt", "shared"),
+			tree.Write("ccc.txt", "shared"),
+		]);
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(hashes);
+
+		// Act
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(duplicates);
+
+		// Assert
+		Assert.AreEqual(2, result.DeletedCount);
+		Assert.IsEmpty(result.SkippedFiles);
+		Assert.IsEmpty(result.Errors);
+	}
+
 	/// <summary>
 	/// Deleting nothing must report nothing, rather than throwing on an empty group list.
 	/// </summary>
@@ -245,5 +388,6 @@ public void DeletingAnEmptyGroupListIsANoOp()
 		Assert.AreEqual(0, result.DeletedCount);
 		Assert.AreEqual(0, result.BytesReclaimed);
 		Assert.IsEmpty(result.Errors);
+		Assert.IsEmpty(result.SkippedFiles);
 	}
 }

FileDeduplicator/Deduplicator.cs

@@ -41,15 +41,30 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList<DuplicateGrou
 		int deletedCount = 0;
 		long bytesReclaimed = 0;
 		List<string> errors = [];
+		List<SkippedFile> skipped = [];
 
 		foreach (DuplicateGroup group in duplicateGroups)
 		{
 			AbsoluteFilePath keeper = SelectFileToKeep(group.Files);
 
-			foreach (AbsoluteFilePath file in group.Files)
+			// The grouping was computed before the confirmation prompt, which is an interactive
+			// pause of unbounded length. If the copy being kept no longer holds the group's
+			// content, deleting the others would destroy the only remaining copies of it, so the
+			// whole group is left alone.
+			if (!StillMatchesGroup(keeper, group.Hash, out string? keeperReason))
 			{
-				if (file == keeper)
+				SkipWholeGroup(group, keeper, keeperReason, skipped);
+				continue;
+			}
+
+			foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper))
+			{
+				// Re-hash immediately before deleting: a file that changed since the scan is no
+				// longer a duplicate, and deleting it would be irreversible loss of content that
+				// exists nowhere else.
+				if (!StillMatchesGroup(file, group.Hash, out string? reason))
 				{
+					Skip(file, reason, skipped);
 					continue;
 				}
 
@@ -70,7 +85,63 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList<DuplicateGrou
 			}
 		}
 
-		return new DeduplicationResult(deletedCount, bytesReclaimed, errors);
+		return new DeduplicationResult(deletedCount, bytesReclaimed, errors, skipped);
+	}
+
+	/// <summary>
+	/// Preserves every copy in a group, because the copy that would have been kept no longer
+	/// holds the group's content.
+	/// </summary>
+	/// <param name="group">The group to leave on disk.</param>
+	/// <param name="keeper">The copy that would have been kept.</param>
+	/// <param name="keeperReason">What the keeper did, phrased to follow its name.</param>
+	/// <param name="skipped">The list to record the preserved files on.</param>
+	private static void SkipWholeGroup(DuplicateGroup group, AbsoluteFilePath keeper, string? keeperReason, List<SkippedFile> skipped)
+	{
+		foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper))
+		{
+			Skip(file, $"the copy being kept ({keeper}) {keeperReason}", skipped);
+		}
+	}
+
+	/// <summary>
+	/// Re-reads a file and reports whether it still holds the content its duplicate group was
+	/// formed from.
+	/// </summary>
+	/// <param name="file">The file to re-hash.</param>
+	/// <param name="groupHash">The hash the group was formed from.</param>
+	/// <param name="reason">When the answer is no, why -- phrased to follow the file's name.</param>
+	/// <returns><see langword="true"/> if the file still hashes to <paramref name="groupHash"/>.</returns>
+	private static bool StillMatchesGroup(AbsoluteFilePath file, string groupHash, out string? reason)
+	{
+		try
+		{
+			if (string.Equals(FileHasher.ComputeHash(file), groupHash, StringComparison.Ordinal))
+			{
+				reason = null;
+				return true;
+			}
+
+			reason = "changed since it was scanned, so it is no longer a duplicate";
+			return false;
+		}
+		catch (IOException ex)
+		{
+			reason = $"could not be re-read to confirm it is still a duplicate: {ex.Message}";
+			return false;
+		}
+		catch (UnauthorizedAccessException ex)
+		{
+			reason = $"could not be re-read to confirm it is still a duplicate: {ex.Message}";
+			return false;
+		}
+	}
+
+	private static void Skip(AbsoluteFilePath file, string? reason, List<SkippedFile> skipped)
+	{
+		SkippedFile skip = new(file, reason ?? "could not be confirmed as a duplicate");
+		skipped.Add(skip);
+		Console.WriteLine($"  Skipped: {file} -- {skip.Reason}");
 	}
 }
 
@@ -81,9 +152,24 @@ internal sealed class DuplicateGroup(string hash, List<AbsoluteFilePath> files)
 	internal long FileSize { get; } = new FileInfo(files[0].WeakString).Length;
 }
 
-internal sealed class DeduplicationResult(int deletedCount, long bytesReclaimed, List<string> errors)
+internal sealed class DeduplicationResult(int deletedCount, long bytesReclaimed, List<string> errors, List<SkippedFile> skippedFiles)
 {
 	internal int DeletedCount { get; } = deletedCount;
 	internal long BytesReclaimed { get; } = bytesReclaimed;
 	internal IReadOnlyList<string> Errors { get; } = errors;
+
+	/// <summary>
+	/// Gets the files that were proposed for deletion but left on disk because they could no
+	/// longer be confirmed as duplicates.
+	/// </summary>
+	internal IReadOnlyList<SkippedFile> SkippedFiles { get; } = skippedFiles;
+}
+
+/// <summary>
+/// A file that was preserved instead of deleted, and why.
+/// </summary>
+internal sealed class SkippedFile(AbsoluteFilePath path, string reason)
+{
+	internal AbsoluteFilePath Path { get; } = path;
+	internal string Reason { get; } = reason;
 }

FileDeduplicator/Verbs/Deduplicate.cs

@@ -83,6 +83,16 @@ internal override void Run(Deduplicate options)
 		Console.WriteLine($"Deleted {result.DeletedCount} file(s).");
 		Console.WriteLine($"Reclaimed {FormatBytes(result.BytesReclaimed)} of disk space.");
 
+		if (result.SkippedFiles.Count > 0)
+		{
+			Console.WriteLine($"Preserved {result.SkippedFiles.Count} file(s) that could no longer be confirmed as duplicates:");
+
+			foreach (SkippedFile skipped in result.SkippedFiles)
+			{
+				Console.WriteLine($"  {skipped.Path} -- {skipped.Reason}");
+			}
+		}
+
 		if (result.Errors.Count > 0)
 		{
 			Console.WriteLine($"Encountered {result.Errors.Count} error(s) during deletion.");