From 7cf65a9bb8238259a9d3aff25dff1cc30d478231 Mon Sep 17 00:00:00 2001
From: Simon Cropp <simon.cropp@gmail.com>
Date: Sat, 22 Aug 2026 14:41:22 +1000
Subject: [PATCH] Guard the menu and hotkey delete paths

The wire path deletes through AcceptTracked, which catches, re-tracks so the
delete can be retried, and reports why it failed. The menu and hot key paths
called File.Delete straight.

So a read-only or open verified file threw out of a click handler, onto the UI
thread, where nothing hooks Application.ThreadException. The entry had already
been untracked by then, so the pending delete was lost with the exception. In
AcceptAll it was worse: the first bad delete skipped the remaining deletes,
every pending move and all the snapshots, so "Accept all" quietly stopped at the
first file it could not remove.

All three now go through AcceptTracked, and AcceptAllDeletes drops its Clear -
a failed delete re-tracks itself, and clearing afterwards would throw that away
again. Failures are logged rather than shown, since a read-only file is not
worth an exception dialog.
---
 src/DiffEngineTray.Tests/TrackerDeleteTest.cs | 42 +++++++++++++++++++
 src/DiffEngineTray/Tracker.cs                 | 31 +++++++++-----
 2 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs
index 3d760f86..58e11db9 100644
--- a/src/DiffEngineTray.Tests/TrackerDeleteTest.cs
+++ b/src/DiffEngineTray.Tests/TrackerDeleteTest.cs
@@ -80,6 +80,48 @@ public async Task AcceptSingle_NotEmpty()
         await Assert.That(tracker.TrackingAny).IsTrue();
     }
 
+    /// <summary>
+    /// A delete that cannot be done. The menu and hot key paths called File.Delete straight, so a
+    /// read-only or open verified file threw out of the click handler - onto the UI thread, where
+    /// nothing hooks Application.ThreadException - and the entry had already been untracked, so
+    /// the pending delete went with it.
+    /// </summary>
+    [Test]
+    public async Task AcceptLeavesAnUndeletableFileTracked()
+    {
+        await using var tracker = new RecordingTracker();
+        var tracked = tracker.AddDelete(file1);
+
+        using (File.Open(file1, FileMode.Open, FileAccess.Read, FileShare.None))
+        {
+            tracker.Accept(tracked);
+        }
+
+        await Assert.That(tracker.Deletes).HasSingleItem();
+        await Assert.That(File.Exists(file1)).IsTrue();
+    }
+
+    /// <summary>
+    /// And one bad delete does not stop the rest of Accept all. Unguarded, the throw skipped the
+    /// remaining deletes, every pending move, and the snapshots.
+    /// </summary>
+    [Test]
+    public async Task AcceptAllContinuesPastAnUndeletableFile()
+    {
+        await using var tracker = new RecordingTracker();
+        tracker.AddDelete(file1);
+        tracker.AddDelete(file2);
+
+        using (File.Open(file1, FileMode.Open, FileAccess.Read, FileShare.None))
+        {
+            await tracker.AcceptAll();
+        }
+
+        // The one that could go, went; the one that could not is still pending
+        await Assert.That(File.Exists(file2)).IsFalse();
+        await Assert.That(tracker.Deletes).HasSingleItem();
+    }
+
     public void Dispose()
     {
         File.Delete(file1);
diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs
index 5ab19434..089e3602 100644
--- a/src/DiffEngineTray/Tracker.cs
+++ b/src/DiffEngineTray/Tracker.cs
@@ -404,22 +404,30 @@ public TrackedDelete AddDelete(string file) =>
                 return existing;
             });
 
+    /// <summary>
+    /// Through <see cref="AcceptTracked(TrackedDelete)"/>, which is what the wire path has always
+    /// used: it catches, re-tracks so the delete can be retried, and reports why.
+    /// <para>
+    /// These called File.Delete straight, so a read-only or open verified file threw out of a menu
+    /// click or a hot key - onto the UI thread, where nothing hooks Application.ThreadException -
+    /// and the entry was already untracked by then, so the pending delete was lost with it.
+    /// </para>
+    /// </summary>
     public void Accept(TrackedDelete delete)
     {
-        if (deletes.TryRemove(delete.File, out var removed))
+        var (ok, message) = AcceptTracked(delete);
+        if (!ok &&
+            message != null)
         {
-            File.Delete(removed.File);
+            Log.Error(message);
         }
     }
 
     public void Accept(IEnumerable<TrackedDelete> toAccept)
     {
-        foreach (var delete in toAccept)
+        foreach (var delete in toAccept.ToList())
         {
-            if (deletes.TryRemove(delete.File, out var removed))
-            {
-                File.Delete(removed.File);
-            }
+            Accept(delete);
         }
     }
 
@@ -706,12 +714,13 @@ public Task AcceptAll()
 
     void AcceptAllDeletes()
     {
-        foreach (var delete in deletes.Values)
+        // One at a time, and no Clear afterwards: a delete that fails re-tracks itself, and
+        // clearing would throw that away. Unguarded, the first bad one also took AcceptMoves and
+        // AcceptAllSnapshots with it, so "Accept all" stopped at the first read-only file
+        foreach (var delete in deletes.Values.ToList())
         {
-            File.Delete(delete.File);
+            Accept(delete);
         }
-
-        deletes.Clear();
     }
 
     public ICollection<TrackedDelete> Deletes => deletes.Values;