From 527ee7d237b06caa55792171283db5426da4e937 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 17:46:07 +1000 Subject: [PATCH 1/2] Do not report a move that succeeded as failed Accepting a tracked move removes the directory the received file sat in when nothing is left in it, which is tidying up after a move that has already happened. Only IOException was caught there, so a directory whose parent will not have it removed threw UnauthorizedAccessException out of the accept - EACCES from rmdir, which .NET reports as that rather than as an IO error. The caller reads a throw as the move having failed, so the entry went back on the queue with an error on it, and the retry then failed with file not found: the temp file had been moved by the attempt that "failed". The enumerate is inside the guard too now, since it can refuse for the same reason. The test for it is Unix only. A read-only directory on Windows raises IOException, which was always caught, so the case cannot be reached there. --- src/DiffEngineViewer.Tests/MoveSweepTests.cs | 77 +++++++++++++++++++ .../SkipOnWindowsAttribute.cs | 9 +++ src/DiffEngineViewer/ViewerActions.cs | 29 +++++-- 3 files changed, 109 insertions(+), 6 deletions(-) create mode 100644 src/DiffEngineViewer.Tests/MoveSweepTests.cs create mode 100644 src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs diff --git a/src/DiffEngineViewer.Tests/MoveSweepTests.cs b/src/DiffEngineViewer.Tests/MoveSweepTests.cs new file mode 100644 index 00000000..3bf88679 --- /dev/null +++ b/src/DiffEngineViewer.Tests/MoveSweepTests.cs @@ -0,0 +1,77 @@ +/// +/// Accepting a tracked move also removes the directory the received file sat in, when nothing is +/// left in it. That is a tidy-up after the fact, and the move it follows has already happened. +/// +public class MoveSweepTests : + IDisposable +{ + /// + /// The caller reads a throw from here as the move having failed, so it re-tracks the entry - + /// and the retry then fails with file not found, the temp file having been moved by the + /// attempt that "failed". Only IOException was caught, and a directory the parent will not + /// let go of raises UnauthorizedAccessException. + /// + [Test] + [SkipOnWindows("A read-only directory raises IOException on Windows, which was always caught. Denying the removal takes a Unix permission.")] + public async Task A_directory_that_cannot_be_removed_does_not_fail_the_move() + { + var received = Path.Combine(root, "received"); + Directory.CreateDirectory(received); + var temp = Path.Combine(received, "sample.received.txt"); + File.WriteAllText(temp, "the snapshot"); + var target = Path.Combine(root, "sample.verified.txt"); + + // Removing "received" is a write to the directory holding it, which this denies. Moving + // the file out of it is a write to "received" itself, which stays allowed + File.SetUnixFileMode(root, UnixFileMode.UserRead | UnixFileMode.UserExecute); + + ViewerActions.Real.MoveFile(temp, target); + + File.SetUnixFileMode(root, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + await Assert.That(File.Exists(target)).IsTrue(); + } + + [Test] + public async Task An_emptied_directory_is_removed() + { + var received = Path.Combine(root, "received"); + Directory.CreateDirectory(received); + var temp = Path.Combine(received, "sample.received.txt"); + File.WriteAllText(temp, "the snapshot"); + var target = Path.Combine(root, "sample.verified.txt"); + + ViewerActions.Real.MoveFile(temp, target); + + await Assert.That(File.Exists(target)).IsTrue(); + await Assert.That(Directory.Exists(received)).IsFalse(); + } + + /// + /// And one still holding something is left alone. + /// + [Test] + public async Task A_directory_with_anything_left_in_it_stays() + { + var received = Path.Combine(root, "received"); + Directory.CreateDirectory(received); + var temp = Path.Combine(received, "sample.received.txt"); + File.WriteAllText(temp, "the snapshot"); + File.WriteAllText(Path.Combine(received, "other.received.txt"), "another"); + var target = Path.Combine(root, "sample.verified.txt"); + + ViewerActions.Real.MoveFile(temp, target); + + await Assert.That(Directory.Exists(received)).IsTrue(); + } + + public MoveSweepTests() + { + root = Path.Combine(Path.GetTempPath(), $"MoveSweepTests_{Guid.NewGuid()}"); + Directory.CreateDirectory(root); + } + + public void Dispose() => + Directory.Delete(root, true); + + readonly string root; +} diff --git a/src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs b/src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs new file mode 100644 index 00000000..87493c56 --- /dev/null +++ b/src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs @@ -0,0 +1,9 @@ +/// +/// Skips a test on Windows, with the reason given at the use site. For the cases whose failure +/// mode is a Unix file permission, which Windows reports as something else or not at all. +/// +public sealed class SkipOnWindowsAttribute(string reason) : SkipAttribute(reason) +{ + public override Task ShouldSkip(TestRegisteredContext context) => + Task.FromResult(OperatingSystem.IsWindows()); +} diff --git a/src/DiffEngineViewer/ViewerActions.cs b/src/DiffEngineViewer/ViewerActions.cs index e0f7c5fc..375bb938 100644 --- a/src/DiffEngineViewer/ViewerActions.cs +++ b/src/DiffEngineViewer/ViewerActions.cs @@ -58,22 +58,39 @@ static void Missing(string temp, string target) => static void Move(string temp, string target) { File.Move(temp, target, true); + Sweep(Path.GetDirectoryName(temp)); + } - var directory = Path.GetDirectoryName(temp); - if (directory is null || - Directory.EnumerateFileSystemEntries(directory).Any()) + /// + /// The directory the received file sat in, if it is now empty. + /// + /// Nothing in here can fail the move. It is already done, and the caller reads a throw as the + /// move having failed: the entry goes back on the queue, and the retry fails with file not + /// found on a temp file that is no longer there. Only was covered, + /// so a directory whose parent will not have it removed - or whose permissions are not this + /// process's to change - reported a move that had succeeded as a failure. + /// + /// + static void Sweep(string? directory) + { + if (directory is null) { return; } try { + if (Directory.EnumerateFileSystemEntries(directory).Any()) + { + return; + } + Directory.Delete(directory); } - catch (IOException) + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) { - // Raced by something writing into it. The move itself succeeded, which is what the - // caller is reporting on. + // Raced by something writing into it, or not ours to remove. } } } From e25c5170788fd82b78c23eb2866f800bbf95c235 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sun, 23 Aug 2026 18:12:45 +1000 Subject: [PATCH 2/2] Deny the tidy-up rather than the move it follows The test made the whole temp directory read only, so the move could not write its target either and failed for the reason the test was meant to rule out. The received file is two deep now and only the directory holding it is locked, so everything the move itself does stays permitted and the only thing denied is the removal after it. TUnit's own RunOn does the platform restriction, which the rest of the suite already uses. --- src/DiffEngineViewer.Tests/MoveSweepTests.cs | 39 +++++++++++++------ .../SkipOnWindowsAttribute.cs | 9 ----- 2 files changed, 28 insertions(+), 20 deletions(-) delete mode 100644 src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs diff --git a/src/DiffEngineViewer.Tests/MoveSweepTests.cs b/src/DiffEngineViewer.Tests/MoveSweepTests.cs index 3bf88679..3634191a 100644 --- a/src/DiffEngineViewer.Tests/MoveSweepTests.cs +++ b/src/DiffEngineViewer.Tests/MoveSweepTests.cs @@ -12,23 +12,27 @@ public class MoveSweepTests : /// let go of raises UnauthorizedAccessException. /// [Test] - [SkipOnWindows("A read-only directory raises IOException on Windows, which was always caught. Denying the removal takes a Unix permission.")] + [RunOn(TUnit.Core.Enums.OS.Linux | TUnit.Core.Enums.OS.MacOs)] public async Task A_directory_that_cannot_be_removed_does_not_fail_the_move() { - var received = Path.Combine(root, "received"); + // The received file is two deep, and it is the middle directory that cannot be removed. + // Everything the move itself touches - taking the file out of "received", and writing it + // into a directory of its own - stays permitted, so the only thing denied is the tidy-up + var locked = Path.Combine(root, "locked"); + var received = Path.Combine(locked, "received"); Directory.CreateDirectory(received); var temp = Path.Combine(received, "sample.received.txt"); File.WriteAllText(temp, "the snapshot"); - var target = Path.Combine(root, "sample.verified.txt"); + var target = Path.Combine(root, "target"); + Directory.CreateDirectory(target); + var verified = Path.Combine(target, "sample.verified.txt"); + File.SetUnixFileMode(locked, UnixFileMode.UserRead | UnixFileMode.UserExecute); - // Removing "received" is a write to the directory holding it, which this denies. Moving - // the file out of it is a write to "received" itself, which stays allowed - File.SetUnixFileMode(root, UnixFileMode.UserRead | UnixFileMode.UserExecute); + ViewerActions.Real.MoveFile(temp, verified); - ViewerActions.Real.MoveFile(temp, target); - - File.SetUnixFileMode(root, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); - await Assert.That(File.Exists(target)).IsTrue(); + await Assert.That(File.Exists(verified)).IsTrue(); + // Still there, which is the point: it could not be removed and that did not matter + await Assert.That(Directory.Exists(received)).IsTrue(); } [Test] @@ -70,8 +74,21 @@ public MoveSweepTests() Directory.CreateDirectory(root); } - public void Dispose() => + public void Dispose() + { + // Whatever the test denied itself, given back, or the tree cannot be removed here either + if (!OperatingSystem.IsWindows()) + { + foreach (var directory in Directory.EnumerateDirectories(root, "*", SearchOption.AllDirectories)) + { + File.SetUnixFileMode( + directory, + UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + } + } + Directory.Delete(root, true); + } readonly string root; } diff --git a/src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs b/src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs deleted file mode 100644 index 87493c56..00000000 --- a/src/DiffEngineViewer.Tests/SkipOnWindowsAttribute.cs +++ /dev/null @@ -1,9 +0,0 @@ -/// -/// Skips a test on Windows, with the reason given at the use site. For the cases whose failure -/// mode is a Unix file permission, which Windows reports as something else or not at all. -/// -public sealed class SkipOnWindowsAttribute(string reason) : SkipAttribute(reason) -{ - public override Task ShouldSkip(TestRegisteredContext context) => - Task.FromResult(OperatingSystem.IsWindows()); -}