From 99d3f60ed40999840211fae3f08c4f25ae2949ca Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 4 Sep 2026 11:05:23 +1000 Subject: [PATCH] Show a window when the focus outruns its move A failing pair whose diff tool is the viewer goes over as two messages on two connections: the move to the tray's piper port, then a focus to whoever owns the queue, naming the key that move was tracked under. Nothing orders them. PiperClient is fire and forget - it reports that the bytes went out, not that the tray read them - and the tray takes that connection on a task of its own, reads it to the end, and walks up from the target looking for a solution before the entry is visible to anything asking. The focus is in flight through all of it. ViewerMessageHandler.Focus refuses a key it cannot find and raises nothing, and AddDiff discarded that refusal and reported AlreadyRunningAndSupportsRefresh regardless. Losing the race was a run where the pair reached the tray menu and no window ever opened, and a second run - where the same key was still tracked from the first - where one did. Which is how it was reported: the diff tool does not show, then on the next run it does. Intermittent, and likeliest on the first move for a path, where the solution walk is uncached and a run with many failing snapshots has the thread pool busiest. A refused focus now falls through to ViewerVerb.Diff, which tracks and raises in a single message to a single process, so there is no order left to get wrong. Its tracking is keyed on the received file like the piper move's, so whichever lands second updates the one entry. That also covers the arrangement where a viewer owns the queue while a tray runs and the focus can never find the key, because that viewer does not know the tray's files: the pair is tracked on both sides there rather than shown by neither. DiffRunnerViewerFocusRaceTest holds the race open rather than trying to win it. A real PiperServer takes the move and never gives it to the tracker, which is what the real one looks like for as long as it is still reading. A move the tray has already tracked is pinned too, since it must stay a plain focus: a Diff would replace the entry and lose the exe and arguments the piper send carries. DiffRunnerViewerMoveTest gains a FakeViewer to answer its focus, having pointed at a dead port before. --- src/DiffEngine/Tray/PendingFiles.cs | 29 ++- .../DiffRunnerViewerFocusRaceTest.cs | 188 ++++++++++++++++++ .../DiffRunnerViewerMoveTest.cs | 15 +- 3 files changed, 220 insertions(+), 12 deletions(-) create mode 100644 src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 6241272b..62dd8cdd 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/src/DiffEngine/Tray/PendingFiles.cs @@ -96,9 +96,24 @@ await ViewerLaunchGate.LaunchAsync( /// /// The window is a when the tray took the move, because the /// tray tracks it and the queue owner - normally that same tray - only has to raise something - /// over it. In the arrangement where a viewer owns the queue while a tray runs, that viewer - /// does not know the tray's files, so the focus finds nothing and the pair stays what it was - /// before any of this: an entry in the tray menu. + /// over it. + /// + /// + /// A refused focus falls through to rather than being discarded, + /// because the two sends are two connections and nothing orders them. The piper send is fire + /// and forget: it reports that the bytes went out, not that the move was tracked, and the tray + /// reads that connection on a task of its own - through a solution directory walk, on the first + /// move for a path - while the focus is already asking about a key that has not landed. Focus + /// refuses an unknown key and raises nothing, so losing that race was a first run where the + /// pair reached the tray menu and no window ever opened, and a second run where the same key + /// was still tracked and one did. Diff tracks and raises in a single message to a single + /// process, so there is no order left to get wrong, and its tracking is keyed on the received + /// file like the piper move's, so whichever lands second updates the one entry. + /// + /// + /// It is also the answer in the arrangement where a viewer owns the queue while a tray runs: + /// that viewer does not know the tray's files, so the focus can never find the key. The pair is + /// tracked on both sides there rather than shown by neither. /// /// public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string targetFile) @@ -107,9 +122,9 @@ public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string ta // the tray works out the same two values for itself when a move arrives without them. var (arguments, canKill) = RelaunchFor(tool, tempFile, targetFile); if (TrayAvailable && - PiperClient.SendMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null)) + PiperClient.SendMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null) && + ViewerClient.TrySend(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)))) { - ViewerClient.TrySend(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile))); return LaunchResult.AlreadyRunningAndSupportsRefresh; } @@ -159,9 +174,9 @@ public static async Task AddDiffAsync(ResolvedTool tool, string te { var (arguments, canKill) = RelaunchFor(tool, tempFile, targetFile); if (TrayAvailable && - await PiperClient.SendMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, null, cancel)) + await PiperClient.SendMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, null, cancel) && + await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)), cancel)) { - await ViewerClient.TrySendAsync(new(ViewerVerb.Focus, TrackedKeys.ForMove(tempFile)), cancel); return LaunchResult.AlreadyRunningAndSupportsRefresh; } diff --git a/src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs b/src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs new file mode 100644 index 00000000..fe4baa6f --- /dev/null +++ b/src/DiffEngineTray.Tests/DiffRunnerViewerFocusRaceTest.cs @@ -0,0 +1,188 @@ +#pragma warning disable CS0618 // DiffEngineTray is obsolete; the test drives it directly to enable the send path. + +/// +/// A failing pair whose diff tool is the viewer, sent while the tray has taken its move but not yet +/// tracked it. +/// +/// The route sends two messages on two connections: the move to the piper port, then a focus to +/// whoever owns the queue, naming the key that move was tracked under. Nothing orders them. The +/// piper send is fire and forget - it reports that the bytes went out, not that the tray read them +/// - and the tray takes that connection on a task of its own, reads it to the end, and walks up +/// from the target looking for a solution before the entry is visible to anything asking. The focus +/// is already in flight through all of it. +/// +/// +/// Focus refuses a key it cannot find and raises nothing, so losing that race left the pair in the +/// tray menu with no window ever opened - and a second run, where the key was still tracked from +/// the first, opening one. Which is the report this is written from: the diff tool does not show, +/// then on the next run it does. +/// +/// +/// The race is held open rather than raced for: a real takes the move +/// and never gives it to the tracker, which is what the real one looks like for as long as it is +/// still reading. What has to happen anyway is the window, so the assertions are on the owner - a +/// real over a real socket - rather than on which verb got it there. +/// +/// +public class DiffRunnerViewerFocusRaceTest +{ + [Test] + public async Task A_sync_launch_raises_a_window_for_a_move_the_tray_has_not_tracked() => + await AssertRaisesAWindow(_ => Task.FromResult(DiffRunner.Launch(Viewer(), _.Temp, _.Target))); + + [Test] + public async Task An_async_launch_raises_a_window_for_a_move_the_tray_has_not_tracked() => + await AssertRaisesAWindow(_ => DiffRunner.LaunchAsync(Viewer(), _.Temp, _.Target)); + + static async Task AssertRaisesAWindow(Func> launch) + { + await using var fixture = new Fixture(); + + var result = await launch(fixture); + + // The pair has a surface, which is all the caller is ever told. + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + + // The move did go to the tray. It is still in flight there, which is the whole premise: + // the focus that followed it could not find the key, exactly as it cannot while the real + // tray is still reading that connection. + var move = await fixture.PiperMove(); + await Assert.That(move.Temp).IsEqualTo(fixture.Temp); + await Assert.That(move.Target).IsEqualTo(fixture.Target); + + // So the pair went over again as a Diff, which tracks and raises in the one message. + await Assert.That(fixture.Tracks(TrackedKeys.ForMove(fixture.Temp))).IsTrue(); + await Assert.That(fixture.Launches).IsEqualTo(1); + } + + /// + /// An owner that answers the focus for a key it holds still takes the early return, so the + /// fall through is the refusal and not something every pair now pays for. + /// + [Test] + public async Task A_move_the_tray_has_tracked_is_a_focus_and_nothing_more() + { + await using var fixture = new Fixture(); + fixture.Track(); + + var result = DiffRunner.Launch(Viewer(), fixture.Temp, fixture.Target); + + await Assert.That(result).IsEqualTo(LaunchResult.AlreadyRunningAndSupportsRefresh); + await Assert.That(fixture.Launches).IsEqualTo(1); + // Focus raised the window over the entry that was already there, so nothing re-tracked it: + // a Diff would have replaced the move, losing the exe and arguments the piper send carries. + await Assert.That(fixture.TrackedExe()).IsEqualTo(Exe); + } + + sealed class Fixture : + IAsyncDisposable + { + readonly string directory = Path.Combine(Path.GetTempPath(), $"Viewer Focus {Guid.NewGuid():N}"); + readonly OwnedInlineHost host; + readonly RecordingTracker tracker; + readonly FakeLauncher launcher = new(); + readonly CancelSource piperCancel = new(); + readonly Task piper; + readonly TaskCompletionSource move = new(); + readonly bool originalDisabled = DiffRunner.Disabled; + readonly int originalPiperPort = PiperClient.Port; + readonly string? originalViewerPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + + public Fixture() + { + Directory.CreateDirectory(directory); + Temp = Path.Combine(directory, "Sample.Test.received.txt"); + Target = Path.Combine(directory, "Sample.Test.verified.txt"); + File.WriteAllText(Temp, "received"); + + // A real tray listener that takes the move and stops there, which is what the real one + // is doing for as long as it is reading the connection and walking for a solution. + PiperClient.Port = FreePort(); + piper = PiperServer.Start(_ => move.TrySetResult(_), _ => { }, piperCancel.Token); + + host = OwnedInlineHost.TryOwn(_ => { }, launcher, 0) ?? + throw new("Could not bind an ephemeral port."); + // Before the tracker, which builds a RemoteInlineHost of its own and would otherwise + // ask the port the module initializer left pointing at nothing. + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, host.Port.ToString()); + tracker = new(); + host.TrackedFiles = tracker; + host.Start(); + + DiffEngine.DiffEngineTray.IsRunning = true; + DiffRunner.Disabled = false; + } + + public string Temp { get; } + + public string Target { get; } + + public int Launches => launcher.Launches; + + /// + /// The move landing before the focus does, which is the other side of the race and the one + /// that always worked. + /// + public void Track() => + tracker.AddMove(Temp, Target, Exe, "--diff", false, null); + + public bool Tracks(string key) => + ((ITrackedFiles) tracker).Has(key); + + public string? TrackedExe() => + tracker.Moves.Single().Exe; + + public async Task PiperMove() => + await move.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + static int FreePort() + { + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + try + { + return ((IPEndPoint) probe.LocalEndpoint).Port; + } + finally + { + probe.Stop(); + } + } + + public async ValueTask DisposeAsync() + { + DiffEngine.DiffEngineTray.IsRunning = false; + DiffRunner.Disabled = originalDisabled; + PiperClient.Port = originalPiperPort; + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, originalViewerPort); + + await piperCancel.CancelAsync(); + await piper; + piperCancel.Dispose(); + await host.DisposeAsync(); + await tracker.DisposeAsync(); + Directory.Delete(directory, true); + } + } + + /// + /// Guarded as existing and never started: the pair is the viewer's, so nothing here reaches a + /// process launch. + /// + static readonly string Exe = Environment.ProcessPath!; + + static ResolvedTool Viewer() => + new( + name: DiffTool.DiffEngineViewer.ToString(), + tool: DiffTool.DiffEngineViewer, + exePath: Exe, + launchArguments: new( + Left: (temp, target) => $"\"{target}\" \"{temp}\"", + Right: (temp, target) => $"\"{temp}\" \"{target}\""), + isMdi: false, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: false, + supportsText: true, + useShellExecute: false); +} diff --git a/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs b/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs index bceafc9c..eb71de3c 100644 --- a/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs +++ b/src/DiffEngineTray.Tests/DiffRunnerViewerMoveTest.cs @@ -149,7 +149,15 @@ static int GetFreePort() readonly string temp; readonly string target; readonly bool originalDisabled = DiffRunner.Disabled; - readonly string? originalViewerPort = Environment.GetEnvironmentVariable("DiffEngine_ViewerPort"); + + /// + /// An owner that answers the focus the route sends once the tray has taken the move, and that + /// publishes its own port so a live viewer on this machine is not raised. A port with nothing + /// on it would leave the focus unanswered, which is the fall through to + /// that is about; + /// this one is about what rides the move. + /// + readonly FakeViewer owner = new(); public DiffRunnerViewerMoveTest() { @@ -158,9 +166,6 @@ public DiffRunnerViewerMoveTest() target = Path.Combine(directory, "Sample.Test.verified.txt"); File.WriteAllText(temp, "received"); PiperClient.Port = GetFreePort(); - // The route sends a focus to whoever owns the queue after the tray has taken the move. - // Pointed at a free port so a live viewer on the machine running these is not raised. - Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", GetFreePort().ToString()); DiffEngine.DiffEngineTray.IsRunning = true; DiffRunner.Disabled = false; } @@ -169,7 +174,7 @@ public void Dispose() { DiffEngine.DiffEngineTray.IsRunning = false; DiffRunner.Disabled = originalDisabled; - Environment.SetEnvironmentVariable("DiffEngine_ViewerPort", originalViewerPort); + owner.Dispose(); Directory.Delete(directory, true); } }