From 7c61c7b33641af7adf8e0a2ce6b3d62803a2b6c7 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Mon, 20 Jul 2026 11:49:58 +1000 Subject: [PATCH] Retry failed accept moves and keep them pending instead of dropping Accepting a move races the diff tool teardown: a killed tool releases its file handles asynchronously (and job objects reap children a beat after the direct kill), and a tool killed mid-startup can leave an orphan that only takes its locks after the kill. InnerMove did a single move attempt and a single lock query, and treated "no locker found" as success - silently dropping the move while the target was never updated. - Retry the move and lock query for a few seconds; lockers found on any attempt go through the existing prompt/kill flow - Never treat an unexplained failure as success: keep the move pending and show a balloon notification - Skip the scan round instead of surfacing an error dialog when a tracked file is exclusively locked (FileComparer opens with FileShare.Read and HandleScanMove only tolerated FileNotFoundException) - Move the ContainsFiles check inside the try in SafeDeleteDirectory --- src/DiffEngineTray.Tests/RecordingTracker.cs | 5 +- .../TrackerAcceptRetryTest.cs | 64 +++++++++++++ src/DiffEngineTray/FileEx.cs | 14 +-- src/DiffEngineTray/Program.cs | 7 +- src/DiffEngineTray/Tracker.cs | 91 +++++++++++++------ 5 files changed, 143 insertions(+), 38 deletions(-) create mode 100644 src/DiffEngineTray.Tests/TrackerAcceptRetryTest.cs diff --git a/src/DiffEngineTray.Tests/RecordingTracker.cs b/src/DiffEngineTray.Tests/RecordingTracker.cs index 3fac9d23..71696a8a 100644 --- a/src/DiffEngineTray.Tests/RecordingTracker.cs +++ b/src/DiffEngineTray.Tests/RecordingTracker.cs @@ -1,4 +1,4 @@ -class RecordingTracker(LockedFilesResolver? lockedFilesResolver = null) : +class RecordingTracker(LockedFilesResolver? lockedFilesResolver = null, Action? acceptFailed = null) : Tracker( () => { @@ -6,7 +6,8 @@ class RecordingTracker(LockedFilesResolver? lockedFilesResolver = null) : () => { }, - lockedFilesResolver) + lockedFilesResolver, + acceptFailed) { public async Task AssertEmpty() { diff --git a/src/DiffEngineTray.Tests/TrackerAcceptRetryTest.cs b/src/DiffEngineTray.Tests/TrackerAcceptRetryTest.cs new file mode 100644 index 00000000..0e0805a3 --- /dev/null +++ b/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"); +} diff --git a/src/DiffEngineTray/FileEx.cs b/src/DiffEngineTray/FileEx.cs index d0ad1009..452a2bd2 100644 --- a/src/DiffEngineTray/FileEx.cs +++ b/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) diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index f4a01bcc..c4d084d0 100644 --- a/src/DiffEngineTray/Program.cs +++ b/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); diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index a12584b6..01d944bd 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -4,16 +4,18 @@ class Tracker : Action active; Action inactive; LockedFilesResolver? lockedFilesResolver; + Action? acceptFailed; ConcurrentDictionary moves = new(StringComparer.OrdinalIgnoreCase); ConcurrentDictionary 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? 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; }