From 0f22b4274433da4a6158dd8dfc098f9f918026a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 09:24:48 +0000 Subject: [PATCH 1/2] Stop deleting a group's copies once the kept copy changes mid-run [patch] DeleteDuplicates re-hashed the kept copy once per group, before the delete loop. The loop re-hashes and deletes each other copy, which takes seconds apiece for large files, and nothing looked at the kept copy again. A sync client or editor rewriting it after the first deletion left the remaining copies to be deleted anyway, and the original content existed nowhere. The kept copy's size, last write time and file identity are now recorded after the pre-loop re-hash and compared before each deletion. Once they differ, every copy still to go is kept and reported as skipped with a reason naming the kept copy. After a group's last deletion the kept copy is re-hashed once more, and a rewrite the quick check could not see (same size, timestamp put back) is reported as an error. DeleteDuplicates takes an optional afterDelete callback, which the tests use to rewrite the kept copy between two deletions. Fixes ktsu-dev/FileDeduplicator#201 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW5MMN2e4Pk8UWLnvp9 --- FileDeduplicator.Test/DeduplicatorTests.cs | 66 +++++++++++++ FileDeduplicator/Deduplicator.cs | 107 ++++++++++++++++++++- README.md | 2 + 3 files changed, 173 insertions(+), 2 deletions(-) diff --git a/FileDeduplicator.Test/DeduplicatorTests.cs b/FileDeduplicator.Test/DeduplicatorTests.cs index a8e93b9..8888eb9 100644 --- a/FileDeduplicator.Test/DeduplicatorTests.cs +++ b/FileDeduplicator.Test/DeduplicatorTests.cs @@ -321,6 +321,72 @@ public void AChangedKeeperPreservesTheWholeGroup() Assert.IsTrue(TempTree.Exists(copyTwo), "The grouped content must survive somewhere."); } + /// + /// 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. + /// + [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 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."); + } + + /// + /// 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. + /// + [TestMethod] + public void AKeeperRewrittenWithoutChangingSizeOrTimeIsReportedAfterTheGroup() + { + // Arrange + using TempTree tree = new(); + AbsoluteFilePath keeper = tree.Write("a.txt", "shared"); + AbsoluteFilePath copy = tree.Write("bb.txt", "shared"); + IReadOnlyList 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]); + } + /// /// 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. diff --git a/FileDeduplicator/Deduplicator.cs b/FileDeduplicator/Deduplicator.cs index 3af2feb..ebb0b60 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -135,7 +135,16 @@ private static IOrderedEnumerable OrderByPreference(IEnumerabl /// unless the group's files are empty. internal static bool IsDeletable(DuplicateGroup group) => group.FileSize > 0; - internal static DeduplicationResult DeleteDuplicates(IReadOnlyList duplicateGroups) + /// + /// Deletes every copy in each deletable group except the one keeps. + /// + /// The groups to deduplicate. + /// + /// Called after each copy is deleted. Production code passes nothing; tests use it to change the + /// filesystem between two deletions in one group. + /// + /// What was deleted, skipped and failed. + internal static DeduplicationResult DeleteDuplicates(IReadOnlyList duplicateGroups, Action? afterDelete = null) { int deletedCount = 0; long bytesReclaimed = 0; @@ -158,8 +167,28 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList 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 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 @@ internal static DeduplicationResult DeleteDuplicates(IReadOnlyList +/// 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. +/// +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; + } + + /// + /// Records the keeper's current state. + /// + /// The copy being kept. + /// The snapshot, or if the keeper could not be read. + internal static KeeperSnapshot? TryTake(AbsoluteFilePath keeper) => + TryRead(keeper, out long length, out DateTime lastWrite, out FileIdentity? identity) + ? new KeeperSnapshot(length, lastWrite, identity) + : null; + + /// + /// Reports whether the keeper still looks exactly as it did when the snapshot was taken. + /// + /// The copy being kept. + /// if its size, last write time and identity are unchanged. + 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 (IOException) + { + } + catch (UnauthorizedAccessException) + { + } + + length = 0; + lastWriteTimeUtc = default; + identity = null; + return false; + } +} + internal sealed class DuplicateGroup(string hash, List files, long fileSize) { internal string Hash { get; } = hash; diff --git a/README.md b/README.md index 55861e6..657182e 100644 --- a/README.md +++ b/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` From 3ef70f075a5259e05727cdf9370449542635b1a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 09:28:10 +0000 Subject: [PATCH 2/2] Fold KeeperSnapshot.TryRead's empty catches into one filtered catch [patch] Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW5MMN2e4Pk8UWLnvp9 --- FileDeduplicator/Deduplicator.cs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/FileDeduplicator/Deduplicator.cs b/FileDeduplicator/Deduplicator.cs index ebb0b60..b01dc19 100644 --- a/FileDeduplicator/Deduplicator.cs +++ b/FileDeduplicator/Deduplicator.cs @@ -385,17 +385,14 @@ private static bool TryRead(AbsoluteFilePath keeper, out long length, out DateTi identity = FileIdentity.TryGet(keeper.WeakString, out FileIdentity id) ? id : null; return true; } - catch (IOException) - { - } - catch (UnauthorizedAccessException) + 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; } - - length = 0; - lastWriteTimeUtc = default; - identity = null; - return false; } }