Never treat one file reached at two paths as a duplicate of itself [patch]

FileDeduplicator.Test/SameFileTests.cs

@@ -0,0 +1,224 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.FileDeduplicator.Test;
+
+using System.Diagnostics;
+
+using ktsu.FileDeduplicator.Verbs;
+using ktsu.Semantics.Paths;
+using ktsu.Semantics.Strings;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Tests that one file reached at two paths -- a hardlink or a bind mount -- is never treated as a
+/// duplicate of itself, and above all is never deleted as one.
+/// </summary>
+/// <remarks>
+/// Through a bind mount, the "other copy" is the keeper's own directory entry, so deleting it
+/// deletes the only copy of the data, and the keeper re-hash cannot catch it. Through a hardlink,
+/// deleting it removes a name from a snapshot while reclaiming nothing.
+/// </remarks>
+[TestClass]
+[DoNotParallelize]
+public sealed class SameFileTests
+{
+	/// <summary>
+	/// Two hardlinks to one file share an identity, and two files with the same content do not.
+	/// </summary>
+	[TestMethod]
+	public void HardlinksShareAnIdentityAndCopiesDoNot()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath original = tree.Write("original.txt", "content");
+		AbsoluteFilePath copy = tree.Write("copy.txt", "content");
+		AbsoluteFilePath link = MakeHardLink(tree, original, "link.txt");
+
+		// Act
+		Assert.IsTrue(FileIdentity.TryGet(original.WeakString, out FileIdentity originalIdentity));
+		Assert.IsTrue(FileIdentity.TryGet(copy.WeakString, out FileIdentity copyIdentity));
+		Assert.IsTrue(FileIdentity.TryGet(link.WeakString, out FileIdentity linkIdentity));
+
+		// Assert
+		Assert.AreEqual(originalIdentity, linkIdentity);
+		Assert.AreNotEqual(originalIdentity, copyIdentity);
+	}
+
+	/// <summary>
+	/// The index read on Unix is the inode number <c>ls -i</c> reports, which pins the offset the
+	/// identity is read from in the runtime's file status structure.
+	/// </summary>
+	[TestMethod]
+	[OSCondition(OperatingSystems.Linux | OperatingSystems.OSX)]
+	public void TheUnixIndexIsTheInodeNumber()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath file = tree.Write("file.txt", "content");
+
+		// Act
+		Assert.IsTrue(FileIdentity.TryGet(file.WeakString, out FileIdentity identity));
+		string listing = RunTool("ls", "-i", file.WeakString);
+
+		// Assert
+		Assert.AreEqual(ulong.Parse(listing.Trim().Split(' ')[0], System.Globalization.CultureInfo.InvariantCulture), identity.Index);
+	}
+
+	/// <summary>
+	/// A missing file has no identity, rather than throwing.
+	/// </summary>
+	[TestMethod]
+	public void AMissingFileHasNoIdentity()
+	{
+		using TempTree tree = new();
+
+		Assert.IsFalse(FileIdentity.TryGet(Path.Combine(tree.Root.WeakString, "absent.txt"), out _));
+	}
+
+	/// <summary>
+	/// A file and its hardlink are one file, so they do not form a duplicate group.
+	/// </summary>
+	[TestMethod]
+	public void AHardlinkIsNotADuplicateOfItsOwnFile()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath original = tree.Write("original.txt", "content");
+		AbsoluteFilePath link = MakeHardLink(tree, original, "nested/link.txt");
+
+		// Act
+		IReadOnlyList<DuplicateGroup> duplicates = Deduplicator.FindDuplicates(
+			Deduplicator.GroupByHash(FileHasher.HashFiles([original, link])));
+
+		// Assert
+		Assert.IsEmpty(duplicates);
+	}
+
+	/// <summary>
+	/// A hardlink in a group with a genuine copy is collapsed into its file, so the group holds the
+	/// file once and the genuine copy once.
+	/// </summary>
+	[TestMethod]
+	public void AHardlinkIsCollapsedIntoItsFileAlongsideAGenuineCopy()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath original = tree.Write("a.txt", "content");
+		AbsoluteFilePath link = MakeHardLink(tree, original, "linked.txt");
+		AbsoluteFilePath copy = tree.Write("copy.txt", "content");
+
+		// Act
+		IReadOnlyList<DuplicateGroup> duplicates = Deduplicator.FindDuplicates(
+			Deduplicator.GroupByHash(FileHasher.HashFiles([original, link, copy])));
+
+		// Assert
+		Assert.HasCount(1, duplicates);
+		CollectionAssert.AreEquivalent(new[] { original, copy }, duplicates[0].Files);
+	}
+
+	/// <summary>
+	/// The deletion loop refuses a path that leads to the keeper's own file, whatever grouping it is
+	/// handed, and reports it instead.
+	/// </summary>
+	[TestMethod]
+	public void DeletionSkipsAPathThatLeadsToTheKeeper()
+	{
+		// Arrange -- a group built by hand, bypassing the collapse in FindDuplicates
+		using TempTree tree = new();
+		AbsoluteFilePath keeper = tree.Write("a.txt", "content");
+		AbsoluteFilePath link = MakeHardLink(tree, keeper, "longer-name.txt");
+		string hash = FileHasher.HashFiles([keeper])[keeper];
+		DuplicateGroup group = new(hash, [keeper, link], "content".Length);
+
+		// Act
+		DeduplicationResult result = Deduplicator.DeleteDuplicates([group]);
+
+		// Assert
+		Assert.AreEqual(0, result.DeletedCount);
+		Assert.AreEqual(0, result.BytesReclaimed);
+		Assert.IsTrue(TempTree.Exists(keeper));
+		Assert.IsTrue(TempTree.Exists(link));
+		Assert.HasCount(1, result.SkippedFiles);
+		Assert.AreEqual(link, result.SkippedFiles[0].Path);
+		Assert.Contains("same file as the copy being kept", result.SkippedFiles[0].Reason);
+	}
+
+	/// <summary>
+	/// The issue's reproduction: one directory visible at two paths through a bind mount. The run
+	/// ends with the file still present, nothing deleted and nothing reported as reclaimed.
+	/// </summary>
+	[TestMethod]
+	public void DeduplicateLeavesAFileReachedThroughABindMountAlone()
+	{
+		// Arrange
+		using TempTree tree = new();
+		AbsoluteFilePath important = tree.Write("real/important.txt", "only copy of important data");
+		string view = Path.Combine(tree.Root.WeakString, "view");
+		_ = Directory.CreateDirectory(view);
+		BindMount(Path.Combine(tree.Root.WeakString, "real"), view);
+
+		try
+		{
+			// Act
+			string output = ConsoleCapture.Normalize(ConsoleCapture.Run(new Deduplicate { PathString = tree.Root.WeakString }, "y"));
+			string dryRun = ConsoleCapture.Normalize(ConsoleCapture.Run(new DryRun { PathString = tree.Root.WeakString }));
+
+			// Assert
+			Assert.IsTrue(TempTree.Exists(important), $"The only copy was deleted. Output was:\n{output}");
+			Assert.AreEqual("only copy of important data", File.ReadAllText(important.WeakString));
+			Assert.DoesNotContain("Deleted:", output);
+			Assert.Contains("No duplicate files found.", output);
+			Assert.Contains("No duplicate files found.", dryRun);
+		}
+		finally
+		{
+			_ = RunTool("umount", view);
+		}
+	}
+
+	private static AbsoluteFilePath MakeHardLink(TempTree tree, AbsoluteFilePath target, string relativePath)
+	{
+		string link = Path.Combine(tree.Root.WeakString, relativePath.Replace('/', Path.DirectorySeparatorChar));
+		_ = Directory.CreateDirectory(Path.GetDirectoryName(link)!);
+
+		_ = OperatingSystem.IsWindows()
+			? RunTool("cmd", "/c", "mklink", "/H", link, target.WeakString)
+			: RunTool("ln", target.WeakString, link);
+
+		Assert.IsTrue(File.Exists(link), $"Could not create a hardlink at {link}.");
+		return link.As<AbsoluteFilePath>();
+	}
+
+	private static void BindMount(string source, string target)
+	{
+		if (!OperatingSystem.IsLinux())
+		{
+			Assert.Inconclusive("Bind mounts are a Linux feature.");
+		}
+
+		using Process mount = Process.Start(new ProcessStartInfo("mount", ["--bind", source, target])
+		{
+			RedirectStandardError = true,
+			RedirectStandardOutput = true,
+		})!;
+		mount.WaitForExit();
+
+		if (mount.ExitCode != 0)
+		{
+			Assert.Inconclusive($"Could not bind-mount {source} (mount needs root): {mount.StandardError.ReadToEnd()}");
+		}
+	}
+
+	private static string RunTool(string fileName, params string[] arguments)
+	{
+		using Process process = Process.Start(new ProcessStartInfo(fileName, arguments)
+		{
+			RedirectStandardOutput = true,
+			RedirectStandardError = true,
+		})!;
+		string output = process.StandardOutput.ReadToEnd();
+		process.WaitForExit();
+		return output;
+	}
+}

FileDeduplicator/Deduplicator.cs

@@ -35,6 +35,11 @@ internal static Dictionary<string, List<AbsoluteFilePath>> GroupByHash(Dictionar
 	/// The hash pass can take minutes on a large tree, and the directories this is pointed at are
 	/// often live, so a file that hashed may be gone by now. A vanished copy is dropped rather than
 	/// allowed to abort the run, and a group it leaves with a single copy is no longer a group.
+	/// <para>
+	/// Paths that lead to one file (see <see cref="FileIdentity"/>) are collapsed to the one
+	/// <see cref="SelectFileToKeep"/> would prefer. They are one copy, not several, so a file the
+	/// scan reached through a bind mount or a hardlink is not a duplicate of itself.
+	/// </para>
 	/// </remarks>
 	/// <param name="hashGroups">The files that hashed, grouped by hash.</param>
 	/// <returns>Every group that still has at least two copies.</returns>
@@ -46,7 +51,7 @@ internal static IReadOnlyList<DuplicateGroup> FindDuplicates(Dictionary<string,
 		{
 			// Each copy is sized exactly once, so the filter and the size it reports cannot disagree
 			// about a file that disappears between two reads.
-			(AbsoluteFilePath File, long Size)[] present = [.. kvp.Value
+			(AbsoluteFilePath File, long Size)[] present = [.. CollapseSameFile(kvp.Value)
 				.Select(file => (File: file, Size: TryGetSize(file, out long size) ? size : (long?)null))
 				.Where(copy => copy.Size.HasValue)
 				.Select(copy => (copy.File, copy.Size!.Value))];
@@ -60,6 +65,24 @@ internal static IReadOnlyList<DuplicateGroup> FindDuplicates(Dictionary<string,
 		return duplicates;
 	}
 
+	/// <summary>
+	/// Keeps one path per file, preferring the path <see cref="SelectFileToKeep"/> would keep.
+	/// </summary>
+	/// <param name="paths">The paths in one hash group.</param>
+	/// <returns>The paths, with every further path to an already-listed file left out.</returns>
+	/// <remarks>
+	/// A path whose identity cannot be read is kept: it is either gone, which the sizing that follows
+	/// drops, or it cannot be inspected, and the deletion guard in <see cref="DeleteDuplicates"/>
+	/// still stands between it and the keeper.
+	/// </remarks>
+	private static IEnumerable<AbsoluteFilePath> CollapseSameFile(IEnumerable<AbsoluteFilePath> paths)
+	{
+		HashSet<FileIdentity> seen = [];
+
+		return OrderByPreference(paths)
+			.Where(path => !FileIdentity.TryGet(path.WeakString, out FileIdentity identity) || seen.Add(identity));
+	}
+
 	/// <summary>
 	/// Sums the sizes of the given files, counting nothing for one that is no longer there.
 	/// </summary>
@@ -95,7 +118,10 @@ private static bool TryGetSize(AbsoluteFilePath file, out long size)
 	}
 
 	internal static AbsoluteFilePath SelectFileToKeep(List<AbsoluteFilePath> duplicates) =>
-		duplicates.OrderBy(f => f.FileName.WeakString.Length).ThenBy(f => f.WeakString, StringComparer.Ordinal).First();
+		OrderByPreference(duplicates).First();
+
+	private static IOrderedEnumerable<AbsoluteFilePath> OrderByPreference(IEnumerable<AbsoluteFilePath> files) =>
+		files.OrderBy(f => f.FileName.WeakString.Length).ThenBy(f => f.WeakString, StringComparer.Ordinal);
 
 	/// <summary>
 	/// Reports whether deduplication may delete copies from a group.
@@ -130,6 +156,8 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList<DuplicateGrou
 				continue;
 			}
 
+			bool keeperIdentified = FileIdentity.TryGet(keeper.WeakString, out FileIdentity keeperIdentity);
+
 			foreach (AbsoluteFilePath file in group.Files.Where(f => f != keeper))
 			{
 				// Re-hash immediately before deleting: a file that changed since the scan is no
@@ -141,36 +169,76 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList<DuplicateGrou
 					continue;
 				}
 
-				try
+				// A path that leads to the keeper's own file is not another copy. Through a bind
+				// mount, deleting it deletes the keeper, and the re-hash above cannot tell, because
+				// the keeper is the file being deleted. FindDuplicates already collapses such paths,
+				// so this is the last guard, against whatever the grouping was handed.
+				if (keeperIdentified && IsSameFile(file, keeperIdentity))
 				{
-					long fileSize = new FileInfo(file.WeakString).Length;
-					File.Delete(file.WeakString);
-					deletedCount++;
-					bytesReclaimed += fileSize;
-					Console.WriteLine($"  Deleted: {file}");
-				}
-				catch (IOException ex)
-				{
-					string error = $"  Error deleting {file}: {ex.Message}";
-					errors.Add(error);
-					Console.WriteLine(error);
+					Skip(file, $"is the same file as the copy being kept ({keeper}), reached through another path", skipped);
+					continue;
 				}
-				// A copy the process is not allowed to remove -- read-only on Windows, or in a
-				// write-protected directory on Unix -- must cost that one file, not the rest of the
-				// run. Letting this escape would abandon every group after it, with no summary and
-				// no report of what was already deleted.
-				catch (UnauthorizedAccessException ex)
+
+				if (TryDelete(file, errors, out long fileSize))
 				{
-					string error = $"  Error deleting {file}: {ex.Message}";
-					errors.Add(error);
-					Console.WriteLine(error);
+					deletedCount++;
+					bytesReclaimed += fileSize;
 				}
 			}
 		}
 
 		return new DeduplicationResult(deletedCount, bytesReclaimed, errors, skipped);
 	}
 
+	/// <summary>
+	/// Deletes one confirmed duplicate, recording a failure instead of letting it end the run.
+	/// </summary>
+	/// <param name="file">The copy to delete.</param>
+	/// <param name="errors">The list to record a failure on.</param>
+	/// <param name="fileSize">The size of the deleted copy, when it was deleted.</param>
+	/// <returns><see langword="true"/> if the copy was deleted.</returns>
+	private static bool TryDelete(AbsoluteFilePath file, List<string> errors, out long fileSize)
+	{
+		try
+		{
+			fileSize = new FileInfo(file.WeakString).Length;
+			File.Delete(file.WeakString);
+			Console.WriteLine($"  Deleted: {file}");
+			return true;
+		}
+		catch (IOException ex)
+		{
+			RecordDeleteError(file, ex, errors);
+		}
+		// A copy the process is not allowed to remove -- read-only on Windows, or in a
+		// write-protected directory on Unix -- must cost that one file, not the rest of the
+		// run. Letting this escape would abandon every group after it, with no summary and
+		// no report of what was already deleted.
+		catch (UnauthorizedAccessException ex)
+		{
+			RecordDeleteError(file, ex, errors);
+		}
+
+		fileSize = 0;
+		return false;
+	}
+
+	private static void RecordDeleteError(AbsoluteFilePath file, Exception ex, List<string> errors)
+	{
+		string error = $"  Error deleting {file}: {ex.Message}";
+		errors.Add(error);
+		Console.WriteLine(error);
+	}
+
+	/// <summary>
+	/// Reports whether a path leads to the file with the given identity.
+	/// </summary>
+	/// <param name="file">The path to check.</param>
+	/// <param name="identity">The identity to compare against.</param>
+	/// <returns><see langword="true"/> only when the path's identity was read and matches.</returns>
+	private static bool IsSameFile(AbsoluteFilePath file, FileIdentity identity) =>
+		FileIdentity.TryGet(file.WeakString, out FileIdentity fileIdentity) && fileIdentity == identity;
+
 	/// <summary>
 	/// Preserves every copy in a group, because the copy that would have been kept no longer
 	/// holds the group's content.