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);
}
}