Stop deleting a group's copies once the kept copy changes mid-run [patch]

FileDeduplicator.Test/DeduplicatorTests.cs

@@ -321,6 +321,72 @@ public void AChangedKeeperPreservesTheWholeGroup()
 		Assert.IsTrue(TempTree.Exists(copyTwo), "The grouped content must survive somewhere.");
 	}
 
+	/// <summary>
+	/// Deleting the other copies takes time, and the keeper can be rewritten between two
+	/// deletions. Once it no longer holds the group's content, every copy still to be deleted is
+	/// the last one left, so the rest of the group must be kept and reported.
+	/// </summary>
+	[TestMethod]
+	public void AKeeperChangedDuringTheDeleteLoopPreservesTheRemainingCopies()
+	{
+		// Arrange -- a.txt is kept; bb.txt is deleted first, then ccc.txt and dddd.txt
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "shared");
+		AbsoluteFilePath first = tree.Write("bb.txt", "shared");
+		AbsoluteFilePath second = tree.Write("ccc.txt", "shared");
+		AbsoluteFilePath third = tree.Write("dddd.txt", "shared");
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(FileHasher.HashFiles([keeper, first, second, third]));
+
+		// Act -- the keeper is rewritten right after the first deletion, as a sync client would
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(
+			duplicates,
+			deleted =>
+			{
+				if (deleted == first)
+				{
+					_ = tree.Write("a.txt", "the keeper was overwritten mid-run");
+				}
+			});
+
+		// Assert
+		Assert.IsFalse(TempTree.Exists(first), "The copy deleted before the keeper changed is gone.");
+		Assert.IsTrue(TempTree.Exists(second), "A copy after the keeper changed may be the last one left.");
+		Assert.IsTrue(TempTree.Exists(third), "A copy after the keeper changed may be the last one left.");
+		Assert.AreEqual(1, result.DeletedCount);
+		Assert.HasCount(2, result.SkippedFiles);
+		Assert.IsTrue(result.SkippedFiles.All(s => s.Reason.Contains(keeper.WeakString, StringComparison.Ordinal)), "The reason names the keeper.");
+	}
+
+	/// <summary>
+	/// A rewrite that keeps the keeper's size and last write time slips past the cheap check
+	/// between deletions, so the keeper is re-hashed after the group's last deletion and the
+	/// loss is reported rather than passed off as a clean run.
+	/// </summary>
+	[TestMethod]
+	public void AKeeperRewrittenWithoutChangingSizeOrTimeIsReportedAfterTheGroup()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "shared");
+		AbsoluteFilePath copy = tree.Write("bb.txt", "shared");
+		IReadOnlyList<DuplicateGroup> duplicates = Duplicates(FileHasher.HashFiles([keeper, copy]));
+		DateTime keeperWritten = File.GetLastWriteTimeUtc(keeper.WeakString);
+
+		// Act -- same length, timestamp put back
+		DeduplicationResult result = Deduplicator.DeleteDuplicates(
+			duplicates,
+			_ =>
+			{
+				File.WriteAllText(keeper.WeakString, "SHARED");
+				File.SetLastWriteTimeUtc(keeper.WeakString, keeperWritten);
+			});
+
+		// Assert
+		Assert.AreEqual(1, result.DeletedCount);
+		Assert.ContainsSingle(result.Errors);
+		Assert.Contains(keeper.WeakString, result.Errors[0]);
+	}
+
 	/// <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.

FileDeduplicator/Deduplicator.cs

@@ -135,7 +135,16 @@
 	/// <returns><see langword="true"/> unless the group's files are empty.</returns>
 	internal static bool IsDeletable(DuplicateGroup group) => group.FileSize > 0;
 
-	internal static DeduplicationResult DeleteDuplicates(IReadOnlyList<DuplicateGroup> duplicateGroups)
+	/// <summary>
+	/// Deletes every copy in each deletable group except the one <see cref="SelectFileToKeep"/> keeps.
+	/// </summary>
+	/// <param name="duplicateGroups">The groups to deduplicate.</param>
+	/// <param name="afterDelete">
+	/// Called after each copy is deleted. Production code passes nothing; tests use it to change the
+	/// filesystem between two deletions in one group.
+	/// </param>
+	/// <returns>What was deleted, skipped and failed.</returns>
+	internal static DeduplicationResult DeleteDuplicates(IReadOnlyList<DuplicateGroup> duplicateGroups, Action<AbsoluteFilePath>? afterDelete = null)
 	{
 		int deletedCount = 0;
 		long bytesReclaimed = 0;
@@ -158,8 +167,28 @@
 
 			bool keeperIdentified = FileIdentity.TryGet(keeper.WeakString, out FileIdentity keeperIdentity);
 
-			foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper))
+			// Deleting the other copies can take seconds apiece for large files, and the keeper can
+			// be rewritten by a sync client or an editor while that runs. Once it no longer holds the
+			// group's content, every copy still to be deleted is the last one left, so the keeper is
+			// checked cheaply before each deletion and the rest of the group is kept if it moved.
+			KeeperSnapshot? keeperSnapshot = KeeperSnapshot.TryTake(keeper);
+			bool deletedFromGroup = false;
+			List<AbsoluteFilePath> others = [.. group.Files.Where(f => f != keeper)];
+
+			for (int i = 0; i < others.Count; i++)
 			{
+				AbsoluteFilePath file = others[i];
+
+				if (keeperSnapshot is null || !keeperSnapshot.StillMatches(keeper))
+				{
+					foreach (AbsoluteFilePath remaining in others.Skip(i))
+					{
+						Skip(remaining, $"the copy being kept ({keeper}) changed while the group was being deduplicated, so this may be the last copy of the content", skipped);
+					}
+
+					break;
+				}
+
 				// 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.
@@ -183,8 +212,20 @@
 				{
 					deletedCount++;
 					bytesReclaimed += fileSize;
+					deletedFromGroup = true;
+					afterDelete?.Invoke(file);
 				}
 			}
+
+			// The snapshot cannot see a rewrite that keeps the size and lands within the timestamp's
+			// resolution, so the keeper is re-hashed once after the group's last deletion. The
+			// deletions cannot be undone by then, but the user is told the content may be gone.
+			if (deletedFromGroup && !StillMatchesGroup(keeper, group.Hash, out string? finalReason))
+			{
+				string error = $"  Error: the copy kept for this group ({keeper}) {finalReason}; the deleted copies held content it no longer has.";
+				errors.Add(error);
+				Console.WriteLine(error);
+			}
 		}
 
 		return new DeduplicationResult(deletedCount, bytesReclaimed, errors, skipped);
@@ -296,6 +337,65 @@
 	}
 }
 
+/// <summary>
+/// The size, last write time and identity of the copy being kept, taken once its content has been
+/// confirmed, so it can be checked cheaply between deletions without re-hashing it each time.
+/// </summary>
+internal sealed class KeeperSnapshot
+{
+	private readonly long length;
+	private readonly DateTime lastWriteTimeUtc;
+	private readonly FileIdentity? identity;
+
+	private KeeperSnapshot(long length, DateTime lastWriteTimeUtc, FileIdentity? identity)
+	{
+		this.length = length;
+		this.lastWriteTimeUtc = lastWriteTimeUtc;
+		this.identity = identity;
+	}
+
+	/// <summary>
+	/// Records the keeper's current state.
+	/// </summary>
+	/// <param name="keeper">The copy being kept.</param>
+	/// <returns>The snapshot, or <see langword="null"/> if the keeper could not be read.</returns>
+	internal static KeeperSnapshot? TryTake(AbsoluteFilePath keeper) =>
+		TryRead(keeper, out long length, out DateTime lastWrite, out FileIdentity? identity)
+			? new KeeperSnapshot(length, lastWrite, identity)
+			: null;
+
+	/// <summary>
+	/// Reports whether the keeper still looks exactly as it did when the snapshot was taken.
+	/// </summary>
+	/// <param name="keeper">The copy being kept.</param>
+	/// <returns><see langword="true"/> if its size, last write time and identity are unchanged.</returns>
+	internal bool StillMatches(AbsoluteFilePath keeper) =>
+		TryRead(keeper, out long currentLength, out DateTime currentLastWrite, out FileIdentity? currentIdentity)
+		&& currentLength == length
+		&& currentLastWrite == lastWriteTimeUtc
+		&& currentIdentity == identity;
+
+	private static bool TryRead(AbsoluteFilePath keeper, out long length, out DateTime lastWriteTimeUtc, out FileIdentity? identity)
+	{
+		try
+		{
+			FileInfo info = new(keeper.WeakString);
+			length = info.Length;
+			lastWriteTimeUtc = info.LastWriteTimeUtc;
+			identity = FileIdentity.TryGet(keeper.WeakString, out FileIdentity id) ? id : null;
+			return true;
+		}
+		catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
+		{
+			// A keeper that is gone or can no longer be read does not match the snapshot.
+			length = 0;
+			lastWriteTimeUtc = default;
+			identity = null;
+			return false;
+		}
+	}
+}
+
 internal sealed class DuplicateGroup(string hash, List<AbsoluteFilePath> files, long fileSize)
 {
 	internal string Hash { get; } = hash;

README.md

@@ -70,6 +70,8 @@ Version-control metadata is never scanned. The `.git`, `.svn`, `.hg` and `.bzr`
 
 One file reached at two paths is not a duplicate of itself. A hardlink, a bind mount, or a share mounted twice gives one file several paths, and deleting one of them either deletes the copy being kept or frees nothing. Such paths are counted as one file, and deletion never removes a path that leads to the kept file.
 
+The copy being kept is checked again before each deletion. If a sync client or an editor rewrites it while the other copies are being deleted, the rest of the group is kept and reported as skipped, because each of them may now be the last copy of the content. After a group's last deletion the kept copy is re-hashed, and a change the quick check could not see is reported as an error.
+
 For example, given these duplicates:
 - `C:\photos\vacation\IMG_20240101_123456.jpg`
 - `C:\photos\beach.jpg`