Do not report a move that succeeded as failed (#840)

* 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.

* 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.

Author: SimonCropp

src/DiffEngineViewer.Tests/MoveSweepTests.cs

@@ -0,0 +1,94 @@
+/// <summary>
+/// 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.
+/// </summary>
+public class MoveSweepTests :
+    IDisposable
+{
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [Test]
+    [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()
+    {
+        // 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, "target");
+        Directory.CreateDirectory(target);
+        var verified = Path.Combine(target, "sample.verified.txt");
+        File.SetUnixFileMode(locked, UnixFileMode.UserRead | UnixFileMode.UserExecute);
+
+        ViewerActions.Real.MoveFile(temp, verified);
+
+        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]
+    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();
+    }
+
+    /// <summary>
+    /// And one still holding something is left alone.
+    /// </summary>
+    [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()
+    {
+        // 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;
+}

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())
+    /// <summary>
+    /// The directory the received file sat in, if it is now empty.
+    /// <para>
+    /// 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 <see cref="IOException" /> 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.
+    /// </para>
+    /// </summary>
+    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.
         }
     }
 }