Retry failed accept moves and keep them pending instead of dropping

src/DiffEngineTray.Tests/RecordingTracker.cs

@@ -1,12 +1,13 @@
-class RecordingTracker(LockedFilesResolver? lockedFilesResolver = null) :
+class RecordingTracker(LockedFilesResolver? lockedFilesResolver = null, Action<TrackedMove>? acceptFailed = null) :
     Tracker(
         () =>
         {
         },
         () =>
         {
         },
-        lockedFilesResolver)
+        lockedFilesResolver,
+        acceptFailed)
 {
     public async Task AssertEmpty()
     {

src/DiffEngineTray.Tests/TrackerAcceptRetryTest.cs

@@ -0,0 +1,64 @@
+// Covers the accept flow when the move fails without Restart Manager seeing a
+// locker: a lock held by the current process is invisible to FindLockedFiles,
+// which mimics a diff tool child process that is mid-death (handles releasing)
+// or mid-startup (locks not taken yet).
+public class TrackerAcceptRetryTest :
+    IDisposable
+{
+    [Test]
+    public async Task TransientLock_RetriesUntilAccepted()
+    {
+        await using var tracker = new RecordingTracker();
+        var tracked = tracker.AddMove(temp, target, "theExe", "theArguments", false, null);
+
+        // Hold the target exclusively (invisible to Restart Manager since the
+        // lock lives in the current process), then release it while Accept is
+        // still retrying
+        var stream = File.Open(target, FileMode.Open, FileAccess.ReadWrite, FileShare.None);
+        var release = Task.Run(async () =>
+        {
+            await Task.Delay(1000);
+            stream.Dispose();
+        });
+
+        tracker.Accept(tracked);
+        await release;
+
+        await tracker.AssertEmpty();
+        await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("new");
+    }
+
+    [Test]
+    public async Task UnresolvedLock_KeepsMovePendingAndNotifies()
+    {
+        TrackedMove? failed = null;
+        await using var tracker = new RecordingTracker(acceptFailed: move => failed = move);
+        var tracked = tracker.AddMove(temp, target, "theExe", "theArguments", false, null);
+
+        using (File.Open(target, FileMode.Open, FileAccess.ReadWrite, FileShare.None))
+        {
+            tracker.Accept(tracked);
+        }
+
+        await Assert.That(tracker.Moves).HasSingleItem();
+        await Assert.That(failed).IsNotNull();
+        await Assert.That(failed!.Target).IsEqualTo(target);
+        await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("old");
+    }
+
+    static string CreateFile(string content)
+    {
+        var path = Path.Combine(Path.GetTempPath(), $"TrackerAcceptRetryTest_{Guid.NewGuid()}.txt");
+        File.WriteAllText(path, content);
+        return path;
+    }
+
+    public void Dispose()
+    {
+        File.Delete(temp);
+        File.Delete(target);
+    }
+
+    string temp = CreateFile("new");
+    string target = CreateFile("old");
+}

src/DiffEngineTray/FileEx.cs

@@ -37,15 +37,15 @@ public static void SafeDeleteDirectory(string path)
             return;
         }
 
-        // Leave the directory if it still holds files (a running test may be using them).
-        // Otherwise it is safe to remove the whole tree, including any empty sub-directories.
-        if (ContainsFiles(path))
-        {
-            return;
-        }
-
         try
         {
+            // Leave the directory if it still holds files (a running test may be using them).
+            // Otherwise it is safe to remove the whole tree, including any empty sub-directories.
+            if (ContainsFiles(path))
+            {
+                return;
+            }
+
             Directory.Delete(path, true);
         }
         catch (IOException exception)

src/DiffEngineTray/Program.cs

@@ -51,7 +51,12 @@ static async Task Inner()
         await using var tracker = new Tracker(
             active: () => icon.Icon = Images.Active,
             inactive: () => icon.Icon = Images.Default,
-            lockedFilesResolver: LockedFilesHandler.Resolve);
+            lockedFilesResolver: LockedFilesHandler.Resolve,
+            acceptFailed: move => icon.ShowBalloonTip(
+                10000,
+                "DiffEngineTray",
+                $"Could not accept '{move.Name}': the file move keeps failing. The move is still pending, so accept can be retried.",
+                ToolTipIcon.Warning));
 
         using var task = StartServer(tracker, cancel);
 

src/DiffEngineTray/Tracker.cs

@@ -4,16 +4,18 @@ class Tracker :
     Action active;
     Action inactive;
     LockedFilesResolver? lockedFilesResolver;
+    Action<TrackedMove>? acceptFailed;
     ConcurrentDictionary<string, TrackedMove> moves = new(StringComparer.OrdinalIgnoreCase);
     ConcurrentDictionary<string, TrackedDelete> deletes = new(StringComparer.OrdinalIgnoreCase);
     AsyncTimer timer;
     int lastScanCount;
 
-    public Tracker(Action active, Action inactive, LockedFilesResolver? lockedFilesResolver = null)
+    public Tracker(Action active, Action inactive, LockedFilesResolver? lockedFilesResolver = null, Action<TrackedMove>? acceptFailed = null)
     {
         this.active = active;
         this.inactive = inactive;
         this.lockedFilesResolver = lockedFilesResolver;
+        this.acceptFailed = acceptFailed;
         timer = new(
             ScanFiles,
             TimeSpan.FromSeconds(2),
@@ -69,8 +71,10 @@ void RemoveAndKill(TrackedMove tacked)
                 return;
             }
         }
-        catch (FileNotFoundException)
+        catch (IOException)
         {
+            // File is missing, or locked by a diff tool or a running test.
+            // Skip this scan round
             return;
         }
 
@@ -277,44 +281,75 @@ public void Discard(TrackedMove move)
         }
     }
 
+    const int acceptAttempts = 8;
+    static readonly TimeSpan acceptRetryDelay = TimeSpan.FromMilliseconds(400);
+
     // Returns false when the move should be kept pending
     bool InnerMove(TrackedMove move, AcceptBatch batch)
     {
         KillProcesses(move);
 
-        if (FileEx.SafeMove(move.Temp, move.Target))
-        {
-            DeleteTempDirectory(move);
-            return true;
-        }
+        // A single move attempt and a single lock query are both racy:
+        // * A killed diff tool releases its file handles asynchronously, and Job
+        //   Objects reap child processes (eg diffword's WINWORD) a beat after the
+        //   direct kill, so the first move attempt can fail while the locks are
+        //   already on their way out.
+        // * A diff tool killed mid-startup can leave an orphaned child that only
+        //   opens (and locks) the files after the kill, so a lock query can find
+        //   nothing even though the move keeps failing.
+        // So retry both for a few seconds before giving up, and never treat an
+        // unexplained failure as success.
+        var killApproved = false;
+        for (var attempt = 0; attempt < acceptAttempts; attempt++)
+        {
+            if (attempt > 0)
+            {
+                Thread.Sleep(acceptRetryDelay);
+            }
 
-        var locked = FindLockedFiles(move);
-        if (locked == null)
-        {
-            // Not caused by a file lock. Drop the move since it is likely a
-            // running test is reading or writing to the files, and the result
-            // will re-add the tracked item
-            return true;
-        }
+            if (!File.Exists(move.Temp))
+            {
+                // Nothing left to move. Drop the move since it is likely a
+                // running test deleted or is re-writing the file, and the result
+                // will re-add the tracked item
+                return true;
+            }
+
+            if (FileEx.SafeMove(move.Temp, move.Target))
+            {
+                DeleteTempDirectory(move);
+                return true;
+            }
 
-        Log.Information(
-            "Files for `{Name}` are locked by {Processes}",
-            move.Name,
-            locked.ProcessNames);
+            var locked = FindLockedFiles(move);
+            if (locked == null)
+            {
+                // No lock visible (yet). The holder may be mid-death or mid-startup
+                continue;
+            }
 
-        if (!ShouldKill(move, locked, batch))
-        {
-            return false;
-        }
+            Log.Information(
+                "Files for `{Name}` are locked by {Processes}",
+                move.Name,
+                locked.ProcessNames);
 
-        FileLockKiller.Kill(locked.Processes);
+            if (!killApproved &&
+                !ShouldKill(move, locked, batch))
+            {
+                // The user chose to keep the locking processes. Keep the move
+                // pending without further retries
+                return false;
+            }
 
-        if (FileEx.SafeMove(move.Temp, move.Target))
-        {
-            DeleteTempDirectory(move);
-            return true;
+            // Remember the approval so re-surfacing lockers dont re-prompt
+            killApproved = true;
+            FileLockKiller.Kill(locked.Processes);
+            // Killed processes release their handles asynchronously; the next
+            // attempt re-tries the move
         }
 
+        Log.Warning("Could not accept `{Name}`: the move keeps failing. Kept pending", move.Name);
+        acceptFailed?.Invoke(move);
         return false;
     }