From 77325fa9ea57610a3564f232d47e509d6b0558bb Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Wed, 23 Sep 2026 15:20:18 +1000 Subject: [PATCH] Fix the tray items from the review - The piper port is bound synchronously at startup (PiperServer.TryBind), and a bind that fails is logged and shown. It used to fault a task nothing looked at until exit, so the tray ran without its listener, holding the mutex, and then crashed on the way out. - A locking process is killed only while it is the one Restart Manager reported: LockingProcess keeps the start time it gave, and Kill compares it with the process it opens. The kill can follow a dialog that waits on the user, and Windows reuses ids. - "Open diff tool" finds the move again by its received file when clicked, and AddMove takes the process it disposes off the move it was on. A menu built before a re-run's move threw on the UI thread. - The accept loop catches a bare SocketException ConnectionReset, which is what a connection that reset before it was accepted throws, instead of showing the modal "open an issue" box on the loop's thread. Each connection is handled on its own task. - FileComparer opens both files shared with writers and deleters, so a test's rewrite or delete of its received file is not refused during a compare, and a pass that reads less from one file than the other is a difference rather than the end of both. - "Always kill locking processes" applies to accepts arriving over the socket, which never prompt and so never reached the resolver that read it. --- src/DiffEngineTray.Tests/FileComparerTests.cs | 77 ++++++++ .../FileLockKillerTest.cs | 36 +++- src/DiffEngineTray.Tests/PiperTest.cs | 178 ++++++++++++++++++ src/DiffEngineTray.Tests/TrackerMoveTest.cs | 45 +++++ .../TrackerTrackedFilesTest.cs | 41 ++++ src/DiffEngineTray/FileComparer.cs | 21 ++- src/DiffEngineTray/FileLockKiller.cs | 46 ++++- src/DiffEngineTray/LockingProcess.cs | 8 +- src/DiffEngineTray/MenuBuilder.cs | 18 +- src/DiffEngineTray/PiperServer.cs | 59 +++++- src/DiffEngineTray/Program.cs | 14 +- src/DiffEngineTray/Tracker.cs | 15 +- todo.md | 31 --- 13 files changed, 538 insertions(+), 51 deletions(-) diff --git a/src/DiffEngineTray.Tests/FileComparerTests.cs b/src/DiffEngineTray.Tests/FileComparerTests.cs index aede5387..1d7fc946 100644 --- a/src/DiffEngineTray.Tests/FileComparerTests.cs +++ b/src/DiffEngineTray.Tests/FileComparerTests.cs @@ -94,4 +94,81 @@ public async Task Large_files_spanning_multiple_buffers() await Cleanup(first, second, differsInLastChunk); } } + + /// + /// 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. + /// + [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(); + } + + + /// + /// 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. + /// + [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); + } + } } diff --git a/src/DiffEngineTray.Tests/FileLockKillerTest.cs b/src/DiffEngineTray.Tests/FileLockKillerTest.cs index a173e395..af20ac7f 100644 --- a/src/DiffEngineTray.Tests/FileLockKillerTest.cs +++ b/src/DiffEngineTray.Tests/FileLockKillerTest.cs @@ -116,4 +116,38 @@ public async Task MoveSucceedsAfterKillingLockingProcess() File.Delete(tempFile); } } -} \ No newline at end of file + + /// + /// 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. + /// + [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); + } + } +} diff --git a/src/DiffEngineTray.Tests/PiperTest.cs b/src/DiffEngineTray.Tests/PiperTest.cs index 32e17ec2..6d31a2cd 100644 --- a/src/DiffEngineTray.Tests/PiperTest.cs +++ b/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 logs) : TraceListener public override void Write(string? message) { } public override void WriteLine(string? message) => logs.Add(message ?? ""); } + + /// + /// 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. + /// + [Test] + public async Task AClientThatResetsBeforeItIsAcceptedIsNotReportedAsAnError() + { + var previousLogger = Log.Logger; + var events = new ConcurrentQueue(); + 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) 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(); + } + + + /// + /// 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. + /// + [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(); + } + + /// + /// Queues what is posted to it until told to run it, so a test decides when an awaiting loop + /// resumes. + /// + 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 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 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 events) : + ILogEventSink + { + public void Emit(LogEvent logEvent) => + events.Enqueue(logEvent); + } } diff --git a/src/DiffEngineTray.Tests/TrackerMoveTest.cs b/src/DiffEngineTray.Tests/TrackerMoveTest.cs index 583b2a76..f34e1ddd 100644 --- a/src/DiffEngineTray.Tests/TrackerMoveTest.cs +++ b/src/DiffEngineTray.Tests/TrackerMoveTest.cs @@ -105,4 +105,49 @@ public void Dispose() string file1 = Path.GetTempFileName(); string file2 = Path.GetTempFileName(); string file3 = Path.GetTempFileName(); + + /// + /// 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. + /// + [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); + } + } } diff --git a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs index 44fb92ac..e7af352d 100644 --- a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs +++ b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs @@ -230,4 +230,45 @@ public TrackerTrackedFilesTest() temp = Path.Combine(tempDirectory, "Sample.Test.received.txt"); target = Path.Combine(Path.GetTempPath(), $"TrackedFilesTest_{Guid.NewGuid():N}.verified.txt"); } + + /// + /// With "Always kill locking processes" on, the menu accepts a locked move by killing + /// the locker without asking. The same accept arriving from the viewer is refused as locked, + /// because the preference lives inside the resolver and a wire accept never consults it. + /// + [Test] + public async Task AlwaysKillAppliesToAnAcceptArrivingOverTheSocket() + { + var previous = LockedFilesHandler.AlwaysKill; + // The stored preference, which Program loads into this at startup + LockedFilesHandler.AlwaysKill = true; + try + { + await File.WriteAllTextAsync(temp, "new"); + await File.WriteAllTextAsync(target, "old"); + // The resolver Program passes + await using var tracker = new RecordingTracker(LockedFilesHandler.Resolve); + var locker = FileLockUtils.StartFileLockProcess(target); + try + { + tracker.AddMove(temp, target, "theExe", "theArguments", false, null); + + // What OwnedInlineHost does with an accept sent by a viewer displaying this queue + var (ok, message) = ((ITrackedFiles) tracker).Accept(TrackedKeys.ForMove(temp)); + + await Assert.That(message).IsNotEqualTo($"Files for '{Path.GetFileNameWithoutExtension(target)}' are locked. Accept from the tray menu to resolve."); + await Assert.That(ok).IsTrue(); + } + finally + { + FileLockUtils.Cleanup(locker); + } + + await Assert.That(await File.ReadAllTextAsync(target)).IsEqualTo("new"); + } + finally + { + LockedFilesHandler.AlwaysKill = previous; + } + } } diff --git a/src/DiffEngineTray/FileComparer.cs b/src/DiffEngineTray/FileComparer.cs index 03dfa3c3..7d80790f 100644 --- a/src/DiffEngineTray/FileComparer.cs +++ b/src/DiffEngineTray/FileComparer.cs @@ -7,11 +7,17 @@ static bool FilesAreSameSize(string file1, string file2) return first.Length == second.Length; } + /// + /// Shared with writers and deleters. The scan compares for as long as reading both files takes, + /// and read sharing alone failed a test's rewrite or delete of its received file with "being + /// used by another process" for all of it. Which means a file can change under the compare, + /// and is what makes that read as different rather than equal. + /// static FileStream OpenRead(string path) => new(path, FileMode.Open, FileAccess.Read, - FileShare.Read, + FileShare.ReadWrite | FileShare.Delete, bufferSize: 4096, useAsync: true); @@ -36,11 +42,20 @@ static async Task StreamsAreEqual(Stream stream1, Stream stream2) while (true) { var t1 = ReadBuffer(stream1, buffer1); - await ReadBuffer(stream2, buffer2); + var count2 = await ReadBuffer(stream2, buffer2); var count = await t1; - //no need to compare size since only enter on files being same size + // The sizes matched when the compare began, but either file can be rewritten or cut + // short while it runs. A pass that read less from one than the other is two files that + // are no longer the same size, and ending on the shorter as though both had ended took + // a received file truncated mid compare for equal to its verified file - and the scan + // drops an equal pair and kills its diff tool. + if (count != count2) + { + return false; + } + if (count == 0) { return true; diff --git a/src/DiffEngineTray/FileLockKiller.cs b/src/DiffEngineTray/FileLockKiller.cs index 09e5cac6..1ae2cb3b 100644 --- a/src/DiffEngineTray/FileLockKiller.cs +++ b/src/DiffEngineTray/FileLockKiller.cs @@ -96,7 +96,7 @@ public static List GetLockingProcesses(string filePath) continue; } - processes.Add(new(processId, info.strAppName)); + processes.Add(new(processId, info.strAppName, StartTime(info.Process.ProcessStartTime))); } } catch (Exception exception) @@ -112,6 +112,40 @@ public static List GetLockingProcesses(string filePath) return processes; } + static DateTime? StartTime(System.Runtime.InteropServices.ComTypes.FILETIME time) + { + var ticks = ((long) (uint) time.dwHighDateTime << 32) | (uint) time.dwLowDateTime; + if (ticks == 0) + { + return null; + } + + return DateTime.FromFileTimeUtc(ticks); + } + + /// + /// Whether the process that holds the id now is the one Restart Manager reported, by its start + /// time, read through the handle the kill then uses. + /// + static bool IsSame(Process process, LockingProcess locking) + { + if (locking.StartTime is not { } reported) + { + return false; + } + + try + { + // Restart Manager and the process's own creation time are the same FILETIME + return Math.Abs((process.StartTime.ToUniversalTime() - reported).TotalMilliseconds) < 1; + } + catch (Exception exception) + when (exception is InvalidOperationException or Win32Exception or NotSupportedException) + { + return false; + } + } + public static bool Kill(IEnumerable processes) { var killed = false; @@ -123,6 +157,16 @@ public static bool Kill(IEnumerable processes) continue; } + if (!IsSame(process, locking)) + { + Log.Information( + "Not killing PID {ProcessId}: it is no longer the '{ProcessName}' that held the file", + locking.ProcessId, + locking.Name); + process.Dispose(); + continue; + } + Log.Information( "Killing locking process '{ProcessName}' (PID: {ProcessId})", locking.Name, diff --git a/src/DiffEngineTray/LockingProcess.cs b/src/DiffEngineTray/LockingProcess.cs index ee7a28e1..e8e2a0b7 100644 --- a/src/DiffEngineTray/LockingProcess.cs +++ b/src/DiffEngineTray/LockingProcess.cs @@ -1 +1,7 @@ -record LockingProcess(int ProcessId, string Name); \ No newline at end of file +/// +/// When the process started, as Restart Manager reported it, in UTC. What makes the id mean this +/// process rather than whichever one holds the id when it is acted on: the kill can come after a +/// dialog that waits on the user for as long as they like, and Windows reuses ids. Null when it +/// could not be read, which nothing is then killed on the strength of. +/// +record LockingProcess(int ProcessId, string Name, DateTime? StartTime = null); \ No newline at end of file diff --git a/src/DiffEngineTray/MenuBuilder.cs b/src/DiffEngineTray/MenuBuilder.cs index 5a53e23c..a23203e4 100644 --- a/src/DiffEngineTray/MenuBuilder.cs +++ b/src/DiffEngineTray/MenuBuilder.cs @@ -178,7 +178,8 @@ static IEnumerable BuildMovesAndDeletes( yield return BuildMove( move, () => tracker.Accept(move), - () => tracker.Discard(move)); + () => tracker.Discard(move), + () => tracker.FindMove(move.Temp)); } } @@ -268,7 +269,12 @@ static ToolStripDropDownButton BuildDelete(TrackedDelete delete, Action accept) return menu; } - static ToolStripDropDownButton BuildMove(TrackedMove move, Action accept, Action discard) + /// + /// The move as it is when an item is clicked, found again by its received file. A re-run + /// replaces the move while a menu built before it can still be open, and the one captured + /// here then holds a process the replacement has disposed. + /// + static ToolStripDropDownButton BuildMove(TrackedMove move, Action accept, Action discard, Func current) { var tempName = Path.GetFileNameWithoutExtension(move.Temp); var targetName = Path.GetFileNameWithoutExtension(move.Target); @@ -281,7 +287,13 @@ static ToolStripDropDownButton BuildMove(TrackedMove move, Action accept, Action menu.DropDownItems.Add(new MenuButton("Discard", discard)); if (move.Exe != null) { - menu.DropDownItems.Add(new MenuButton("Open diff tool", () => DiffToolLauncher.Launch(move))); + menu.DropDownItems.Add(new MenuButton("Open diff tool", () => + { + if (current() is { Exe: not null } live) + { + DiffToolLauncher.Launch(live); + } + })); } menu.DropDownItems.Add(BuildShowInExplorer(move.Temp)); diff --git a/src/DiffEngineTray/PiperServer.cs b/src/DiffEngineTray/PiperServer.cs index 5ef3fefd..0589a9e1 100644 --- a/src/DiffEngineTray/PiperServer.cs +++ b/src/DiffEngineTray/PiperServer.cs @@ -19,17 +19,55 @@ /// static class PiperServer { + /// + /// Binds and serves, with a bind that fails faulting the task. For the tests; the tray binds + /// with first, so it can say so. + /// public static async Task Start( Action move, Action delete, Cancel cancel = default) { - TcpListener? listener = default; + var listener = new TcpListener(IPAddress.Loopback, PiperClient.Port); + listener.Start(); + await Serve(listener, move, delete, cancel); + } + /// + /// The port, taken now, or null with why not. + /// + /// Synchronous, and before anything is served. A bind that failed inside the serving task + /// faulted a task nothing looked at until the tray exited: it ran for the whole session with + /// no listener, holding the mutex that keeps a second tray from starting, while every move and + /// delete went to whatever did hold 3492 - and then crashed on the way out, logged as having + /// failed at startup. + /// + /// + public static TcpListener? TryBind(out SocketException? error) + { + var listener = new TcpListener(IPAddress.Loopback, PiperClient.Port); try { - listener = new(IPAddress.Loopback, PiperClient.Port); listener.Start(); + error = null; + return listener; + } + catch (SocketException exception) + { + listener.Stop(); + error = exception; + return null; + } + } + + public static async Task Serve( + TcpListener listener, + Action move, + Action delete, + Cancel cancel = default) + { + try + { // Kept from when the accept lived inside the per-connection method: cancelling stops // the listener, which is what brings a pending accept down with it await using var registration = cancel.Register(listener.Stop); @@ -49,7 +87,11 @@ public static async Task Start( // move and delete from every other process for as long as it stayed that way, // with nothing to end the wait. The callbacks are the tracker's concurrent // collections, which the viewer port already writes to off this thread - _ = Handle(client, move, delete, cancel); + // + // Task.Run rather than a bare call, which ran the handler on this loop until + // its first await that did not complete at once - for a payload already + // buffered, all of it, parse and tracker included - with no accept pending. + _ = Task.Run(() => Handle(client, move, delete, cancel), Cancel.None); } catch (TaskCanceledException) { @@ -60,10 +102,13 @@ public static async Task Start( //when task is cancelled socket is disposed break; } - catch (IOException exception) - when (exception.InnerException is SocketException { SocketErrorCode: SocketError.ConnectionReset }) + catch (SocketException exception) + when (exception.SocketErrorCode == SocketError.ConnectionReset) { - //client disconnected abruptly, e.g. test was canceled + // A client that reset while it waited to be accepted - a test run cancelled + // mid send. The accept throws that bare, where a read throws it wrapped in an + // IOException, so the wrapped catch below never matched it here and it went to + // the "open an issue" box, modal, on this loop's thread. } catch (Exception exception) { @@ -78,7 +123,7 @@ public static async Task Start( } finally { - listener?.Stop(); + listener.Stop(); } } diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index f7fb7271..bdb7b951 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -96,7 +96,14 @@ void Warn(string message) => // that is still running, and Task.Dispose throws for one that has not completed - which // would replace whatever actually went wrong with an InvalidOperationException. A task // needs no disposal anyway; cancelling it is what ends it - var task = StartServer(tracker, cancel); + var listener = PiperServer.TryBind(out var bindError); + if (listener is null) + { + Log.Error(bindError, "Could not listen on port {Port}", PiperClient.Port); + Warn($"Could not listen on port {PiperClient.Port}, so moves and deletes from test runs will not reach the tray. {bindError!.Message}"); + } + + var task = listener is null ? Task.CompletedTask : StartServer(listener, tracker, cancel); using var keyRegister = new KeyRegister(icon.Handle()); ReBindKeys(settings, keyRegister, tracker, Warn); @@ -195,8 +202,9 @@ internal record KeyBinding(int Id, HotKey HotKey, Action Action); } } - static Task StartServer(Tracker tracker, Cancel cancel) => - PiperServer.Start( + static Task StartServer(TcpListener listener, Tracker tracker, Cancel cancel) => + PiperServer.Serve( + listener, payload => { tracker.AddMove( diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index b75475cc..46b86d5e 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -204,7 +204,10 @@ public TrackedMove AddMove( } else { + // Taken off the move it is disposed on, so nothing still holding that move - + // a menu built before this update - reaches a disposed process through it existing.Process?.Dispose(); + existing.Process = null; ProcessEx.TryGet(processId.Value, out process); } @@ -711,8 +714,12 @@ static bool CannotEverMove(TrackedMove move) bool ShouldKill(TrackedMove move, LockedFiles locked, AcceptBatch batch) { + // The user's standing answer, read here rather than only by the resolver: an accept that + // arrives from the viewer never prompts, so it never reached the resolver, and was refused + // as locked with "accept from the tray menu" - where the same accept killed without asking. if (move.KillLockingProcess || - batch.KillWithoutPrompt) + batch.KillWithoutPrompt || + LockedFilesHandler.AlwaysKill) { return true; } @@ -885,6 +892,12 @@ void AcceptAllDeletes() public ICollection Moves => moves.Values; + /// + /// The move for a received file as it is now, or null once it has gone. + /// + public TrackedMove? FindMove(string temp) => + moves.GetValueOrDefault(temp); + IReadOnlyList ITrackedFiles.Moves() => moves.Values .Select(_ => new ViewerResponseMove( diff --git a/todo.md b/todo.md index 12c52097..71048f2c 100644 --- a/todo.md +++ b/todo.md @@ -18,37 +18,6 @@ Viewer model - Fix: have each head report string positions from its own layout rather than cells, or put every code point on the grid. -Tray - -- [ ] **A failed bind on 3492 leaves the tray running without its listener** (verified) - - `PiperServer.Start` is async, so a bind failure (`src/DiffEngineTray/PiperServer.cs:31-32`) faults the returned task, which `Program` awaits only after `Application.Run` returns (`Program.cs:99`, `:132`), holding the "DiffEngine" mutex throughout; on exit it is logged as Fatal "Failed at startup" and rethrown. Meanwhile, if another process holds 3492, `PortIsHeld` says yes and every move and delete goes to it; a bind that failed otherwise sends moves to 3493 without exe, arguments or process id. - - Needs the named mutex, which the real tray holds, so not unit tested. - - Fix: bind synchronously, as `ViewerServer.TryBind` does, and warn; or check `IsFaulted` before `Application.Run`. - -- [ ] **The locked-file kill uses a bare PID after an unbounded modal dialog** (verified) - - `FileLockKiller.cs:89-100` drops `RM_UNIQUE_PROCESS.ProcessStartTime`, and `LockingProcess` has nowhere to keep it. After `form.ShowDialog()` (`LockedFilesHandler.cs:12-13`), which has no timeout, `Kill` opens each process by id (`FileLockKiller.cs:121`). Only the dialog path has a long window; PID reuse cannot be forced in a test. - - Fix: keep the start time and compare it with the opened process's through the same handle before killing. - -- [ ] **"Open diff tool" from a menu built before a re-run's move throws on the UI thread** (repro) - - `AddMove`'s update factory disposes `existing.Process` (`src/DiffEngineTray/Tracker.cs:207`) and leaves it on the old move, which a menu that was open across the re-run still holds (`MenuBuilder.cs:284`; the menu is rebuilt on each Opening). Nothing handles `ThreadException`, so the user gets WinForms' unhandled exception dialog. The factory running twice under contention only leaks a handle. - - Test: `OpenDiffToolFromAMenuBuiltBeforeTheMoveWasUpdated`. - - The suggested fix is incomplete: nulling it stops the throw, but "Open diff tool" would then attach the new tool to the orphaned move, which nothing kills. Look the move up by key when clicked, and dispose outside the factory. - -- [ ] **A connection that resets while waiting to be accepted shows the "open an issue" box and stalls the listener** (repro) - - `AcceptTcpClientAsync` throws a bare `SocketException` ConnectionReset (measured on .NET 10), but the accept loop's catch expects an `IOException` wrapping one (`PiperServer.cs:63-67`), so it reaches `ExceptionHandler.Handle` and its modal box, on the accept loop's own thread. `Handle`'s catch (`:126-130`) is right as it is: a reset during the read is the wrapped form. - - Test: `AClientThatResetsBeforeItIsAcceptedIsNotReportedAsAnError`. - - Fix: catch `SocketException` in the accept loop only. The handler running inline until its first real await is true, but only widens the window. Also, `PiperTest.ClientDisconnectsAbruptly` never sends a reset (`TcpClient.Dispose` closes cleanly); `Socket.Close(0)` does. - -- [ ] **`FileComparer` blocks a test's delete of its received file during a compare** (repro) - - Both files are opened with `FileShare.Read` (`src/DiffEngineTray/FileComparer.cs:10-16`) for the whole compare, which since #884 runs once per change of a same-size pair. - - Test: `ATestCanDeleteItsReceivedFileWhileTheScanComparesIt` (a 64 MB pair). - - `count2` cannot give a wrong answer today, because `FileShare.Read` keeps writers out. So widening the sharing alone is wrong: truncating the received file mid compare then made `FilesAreEqual` return true against a 64 MB verified file, and the scan would drop the move and kill its tool. Widen it only together with returning false when `count1 != count2`. - -- [ ] **"Always kill locking processes" is ignored for accepts arriving over the socket** (repro) - - Wire accepts go through `AcceptWithoutPrompting` (`Tracker.cs:1073-1090`), and `ShouldKill` returns false at `:720-724`, before the resolver, the only place that reads the setting (`LockedFilesHandler.cs:7-10`). The viewer is told to accept from the tray menu, where the same accept kills without asking. - - Test: `AlwaysKillAppliesToAnAcceptArrivingOverTheSocket`. - - Fix: read the setting in `ShouldKill` before the `NeverPrompt` branch. `ALockedMoveIsRefusedWithoutPrompting` requires that the resolver, which builds a dialog, is never consulted. - Native (the Linux items were unreachable until #885 made the Linux window draw and read input; these are verdicts on the code as it behaves since)