Fix the tray items from the review

src/DiffEngineTray.Tests/FileComparerTests.cs

@@ -94,4 +94,81 @@ public async Task Large_files_spanning_multiple_buffers()
             await Cleanup(first, second, differsInLastChunk);
         }
     }
+
+    /// <summary>
+    /// The scan opens both files with FileShare.Read only, so for as long as a comparison
+    /// runs, a test deleting its received file - what a passing re-run does - gets a sharing
+    /// violation.
+    /// </summary>
+    [Test]
+    public async Task ATestCanDeleteItsReceivedFileWhileTheScanComparesIt()
+    {
+        // Same size and large, so the comparison reads both through and is still reading when the
+        // delete arrives
+        var content = new byte[64 * 1024 * 1024];
+        var temp = TempFile("");
+        var target = TempFile("");
+        await File.WriteAllBytesAsync(temp, content);
+        await File.WriteAllBytesAsync(target, content);
+
+        var comparing = FileComparer.FilesAreEqual(temp, target);
+
+        Exception? refused = null;
+        try
+        {
+            File.Delete(temp);
+        }
+        catch (IOException exception)
+        {
+            refused = exception;
+        }
+
+        // Both files are opened before FilesAreEqual first yields, and closed only as it completes,
+        // so not being complete here means the delete arrived while the scan held them
+        var overlapped = !comparing.IsCompleted;
+        try
+        {
+            await comparing;
+        }
+        catch (IOException)
+        {
+        }
+
+        await Cleanup(temp, target);
+        await Assert.That(overlapped).IsTrue();
+        await Assert.That(refused).IsNull();
+    }
+
+
+    /// <summary>
+    /// With writers let in, a file can be cut short while it is compared. The shorter read is two
+    /// files that differ, not two that both ended.
+    /// </summary>
+    [Test]
+    public async Task A_file_cut_short_mid_compare_is_not_equal()
+    {
+        var content = new byte[64 * 1024 * 1024];
+        var received = TempFile("");
+        var verified = TempFile("");
+        await File.WriteAllBytesAsync(received, content);
+        await File.WriteAllBytesAsync(verified, content);
+        try
+        {
+            var comparing = FileComparer.FilesAreEqual(received, verified);
+            await using (var cut = new FileStream(received, FileMode.Open, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete))
+            {
+                cut.SetLength(8 * 1024 * 1024);
+            }
+
+            var overlapped = !comparing.IsCompleted;
+            var equal = await comparing;
+
+            await Assert.That(overlapped).IsTrue();
+            await Assert.That(equal).IsFalse();
+        }
+        finally
+        {
+            await Cleanup(received, verified);
+        }
+    }
 }

src/DiffEngineTray.Tests/FileLockKillerTest.cs

@@ -116,4 +116,38 @@ public async Task MoveSucceedsAfterKillingLockingProcess()
             File.Delete(tempFile);
         }
     }
-}
+
+    /// <summary>
+    /// The kill can come after a dialog that waits on the user, and Windows reuses process ids, so a
+    /// process is killed only while it is still the one Restart Manager reported: same id, same
+    /// start time.
+    /// </summary>
+    [Test]
+    public async Task Kill_LeavesAProcessThatIsNoLongerTheOneReported()
+    {
+        var file = Path.Combine(Path.GetTempPath(), $"FileLockKillerTest_{Guid.NewGuid()}.txt");
+        await File.WriteAllTextAsync(file, "content");
+        var lockProcess = FileLockUtils.StartFileLockProcess(file);
+        try
+        {
+            var reported = FileLockKiller.GetLockingProcesses(file).Single(_ => _.ProcessId == lockProcess.Id);
+            await Assert.That(reported.StartTime).IsNotNull();
+
+            // The same id, started at another time: what a reused id looks like
+            var reused = reported with
+            {
+                StartTime = reported.StartTime!.Value.AddSeconds(-30)
+            };
+
+            await Assert.That(FileLockKiller.Kill([reused])).IsFalse();
+            await Assert.That(lockProcess.HasExited).IsFalse();
+
+            await Assert.That(FileLockKiller.Kill([reported])).IsTrue();
+        }
+        finally
+        {
+            FileLockUtils.Cleanup(lockProcess);
+            File.Delete(file);
+        }
+    }
+}

src/DiffEngineTray.Tests/PiperTest.cs

@@ -1,3 +1,9 @@
+using System.Collections.Concurrent;
+using System.Reflection;
+using Serilog;
+using Serilog.Core;
+using Serilog.Events;
+
 public class PiperTest :
     IDisposable
 {
@@ -210,4 +216,176 @@ class LogCapture(List<string> logs) : TraceListener
         public override void Write(string? message) { }
         public override void WriteLine(string? message) => logs.Add(message ?? "");
     }
+
+    /// <summary>
+    /// A connection that resets while it waits in the backlog - a test process cancelled
+    /// mid send, arriving while the loop is between one accept and the next - surfaces from the
+    /// accept as a bare SocketException. The loop's reset catch is written for an IOException
+    /// wrapping one, which is what a read throws, so it never matches, and the reset is reported as
+    /// "Failed to receive payload" with the open-an-issue box, on the accept loop's own thread.
+    /// </summary>
+    [Test]
+    public async Task AClientThatResetsBeforeItIsAcceptedIsNotReportedAsAnError()
+    {
+        var previousLogger = Log.Logger;
+        var events = new ConcurrentQueue<LogEvent>();
+        Log.Logger = new LoggerConfiguration()
+            .WriteTo.Sink(new Capture(events))
+            .CreateLogger();
+        // ExceptionHandler follows the log with a modal "open an issue?" box, which here would wait
+        // for a click forever. It asks once per message, so this one is marked as already asked.
+        var asked = (ConcurrentBag<string>) typeof(IssueLauncher)
+            .GetField("recorded", BindingFlags.NonPublic | BindingFlags.Static)!
+            .GetValue(null)!;
+        asked.Add("Failed to receive payload");
+
+        using var cancel = new CancelSource();
+        var held = new HeldContext();
+        var second = new TcpClient();
+        try
+        {
+            Task serving;
+            var previousContext = SynchronizationContext.Current;
+            SynchronizationContext.SetSynchronizationContext(held);
+            try
+            {
+                serving = PiperServer.Start(_ => { }, _ => { }, cancel.Token);
+            }
+            finally
+            {
+                SynchronizationContext.SetSynchronizationContext(previousContext);
+            }
+
+            // Accepting this one completes the loop's pending accept. The loop resuming is held, so
+            // for now nothing is accepting: the gap between one accept and the next, held open
+            using (var first = new TcpClient())
+            {
+                await first.ConnectAsync(IPAddress.Loopback, PiperClient.Port);
+            }
+
+            await Assert.That(await held.WaitForPending(TimeSpan.FromSeconds(5))).IsTrue();
+
+            // Connects into the backlog during that gap, and resets there
+            await second.ConnectAsync(IPAddress.Loopback, PiperClient.Port);
+            second.Client.Close(0);
+            await Task.Delay(200);
+
+            // The loop goes on: it handles the first client, then accepts again
+            held.RunFor(TimeSpan.FromSeconds(1));
+
+            await cancel.CancelAsync();
+            held.RunUntil(serving, TimeSpan.FromSeconds(5));
+            await Assert.That(serving.IsCompleted).IsTrue();
+        }
+        finally
+        {
+            second.Dispose();
+            Log.Logger = previousLogger;
+        }
+
+        var errors = events
+            .Where(_ => _.Level >= LogEventLevel.Error)
+            .Select(_ => $"{_.MessageTemplate.Text}: {_.Exception?.GetType().Name} {(_.Exception as SocketException)?.SocketErrorCode}")
+            .ToList();
+        await Assert.That(errors).IsEmpty();
+    }
+
+
+    /// <summary>
+    /// The tray binds the port itself, before serving, so a port something else holds is reported
+    /// at startup rather than faulting a task nobody looks at until exit.
+    /// </summary>
+    [Test]
+    public async Task ABindThatFailsSaysWhy()
+    {
+        var holder = new TcpListener(IPAddress.Loopback, PiperClient.Port);
+        holder.Start();
+        try
+        {
+            var listener = PiperServer.TryBind(out var error);
+
+            await Assert.That(listener).IsNull();
+            await Assert.That(error!.SocketErrorCode).IsEqualTo(SocketError.AddressAlreadyInUse);
+        }
+        finally
+        {
+            holder.Stop();
+        }
+
+        var bound = PiperServer.TryBind(out var none);
+        bound!.Stop();
+        await Assert.That(none).IsNull();
+    }
+
+    /// <summary>
+    /// Queues what is posted to it until told to run it, so a test decides when an awaiting loop
+    /// resumes.
+    /// </summary>
+    sealed class HeldContext :
+        SynchronizationContext
+    {
+        readonly BlockingCollection<(SendOrPostCallback callback, object? state)> queue = [];
+
+        public override void Post(SendOrPostCallback d, object? state) =>
+            queue.Add((d, state));
+
+        public override void Send(SendOrPostCallback d, object? state) =>
+            d(state);
+
+        public async Task<bool> WaitForPending(TimeSpan timeout)
+        {
+            var deadline = DateTime.UtcNow + timeout;
+            while (DateTime.UtcNow < deadline)
+            {
+                if (queue.Count > 0)
+                {
+                    return true;
+                }
+
+                await Task.Delay(10);
+            }
+
+            return false;
+        }
+
+        public void RunFor(TimeSpan duration) =>
+            Run(() => false, duration);
+
+        public void RunUntil(Task task, TimeSpan timeout) =>
+            Run(() => task.IsCompleted, timeout);
+
+        void Run(Func<bool> done, TimeSpan duration)
+        {
+            var previous = Current;
+            SetSynchronizationContext(this);
+            try
+            {
+                var deadline = DateTime.UtcNow + duration;
+                while (!done())
+                {
+                    var remaining = deadline - DateTime.UtcNow;
+                    if (remaining <= TimeSpan.Zero)
+                    {
+                        return;
+                    }
+
+                    if (queue.TryTake(out var item, TimeSpan.FromMilliseconds(Math.Min(50, remaining.TotalMilliseconds))))
+                    {
+                        item.callback(item.state);
+                    }
+                }
+            }
+            finally
+            {
+                SetSynchronizationContext(previous);
+            }
+        }
+    }
+
+    sealed class Capture(ConcurrentQueue<LogEvent> events) :
+        ILogEventSink
+    {
+        public void Emit(LogEvent logEvent) =>
+            events.Enqueue(logEvent);
+    }
 }

src/DiffEngineTray.Tests/TrackerMoveTest.cs

@@ -105,4 +105,49 @@ public void Dispose()
     string file1 = Path.GetTempFileName();
     string file2 = Path.GetTempFileName();
     string file3 = Path.GetTempFileName();
+
+    /// <summary>
+    /// DiffRunner resends a move for the same received file on every re-run, with a process
+    /// id each time. The update factory disposes the Process of the move it replaces and leaves it
+    /// there, so a menu opened before that re-run holds a move whose process is disposed, and its
+    /// "Open diff tool" item throws on the UI thread.
+    /// </summary>
+    [Test]
+    public async Task OpenDiffToolFromAMenuBuiltBeforeTheMoveWasUpdated()
+    {
+        await using var tracker = new RecordingTracker();
+        var temp = file1;
+        var target = file2;
+        var toolLock = file3;
+        // Stands in for an auto refresh diff tool that is still open, which is what DiffRunner
+        // resends the id of
+        var tool = FileLockUtils.StartFileLockProcess(toolLock);
+        try
+        {
+            // Nothing by this name exists, so if the launcher gets past the process it starts nothing
+            var exe = Path.Combine(Path.GetTempPath(), $"ReviewReproNoSuchTool_{Guid.NewGuid()}.exe");
+
+            // What the menu captured when it opened
+            var shown = tracker.AddMove(temp, target, exe, "theArguments", false, tool.Id);
+            // The re-run's move, landing while that menu is open
+            tracker.AddMove(temp, target, exe, "theArguments", false, tool.Id);
+
+            Exception? thrown = null;
+            try
+            {
+                // The "Open diff tool" item's click handler
+                DiffToolLauncher.Launch(shown);
+            }
+            catch (Exception exception)
+            {
+                thrown = exception;
+            }
+
+            await Assert.That(thrown).IsNull();
+        }
+        finally
+        {
+            FileLockUtils.Cleanup(tool);
+        }
+    }
 }