Close a Windows diff tool started from WSL

claude.md

@@ -829,7 +829,7 @@ the comment there about not caching "nothing staged" asks for.
 ### Key Patterns
 
 - Tool discovery uses wildcard path matching (`WildcardFileFinder`) to find executables in common install locations. A wildcard whose matches are all version-named folders takes the highest version; anything else takes the most recently written
-- Inside WSL, which `BuildServerDetector.IsWsl` reports and `Detected` does not count as a build server, a tool with no copy in the distribution is resolved from its Windows definition and started through WSL's interop (`Wsl/`: `WslInterop`, `WslHost`, `WslPaths`; `OsSettingsResolver.TryFindOnHost`). The host's variables come from one `cmd.exe /u /c set`, its drives from `/proc/mounts` and the name of the distribution's share from one `wslpath -w /`. Paths are translated in process, not by `wslpath` per path, because `ResolvedTool.GetArguments` runs for every failing pair, opened or not. A program on the host's PATH is found from one listing of each host directory on it (`WslHost.TryFindOnPath`), a stat on a Windows drive from inside WSL being most of a millisecond. What this side holds for such a tool is WSL's `/init` standing in for it: `ps` lists it as `/init <exe> <exe> <args>` (`WslInterop.StripProxy`), ending it does not close a windowed tool, so `ResolvedTool.CanKill` is false and `DiffRunner.Kill` leaves it alone, and it ends with the terminal session while the window stays. It is started with all three streams redirected and closed (`WslInterop.Start`), or it would hold the pipe `dotnet test` reads. Not offered: the viewer (the host's loopback is not the distribution's under WSL's default networking), a Windows executable that is a `.cmd`, and Vim and Neovim. `DiffEngine_WslWindowsTools=false` turns it off. None of it can run in CI: `WslHostTests` stand a temp directory in for the drive, and the launch itself was run by hand in an Ubuntu distribution under WSL 2, with Beyond Compare, WinMerge and P4Merge
+- Inside WSL, which `BuildServerDetector.IsWsl` reports and `Detected` does not count as a build server, a tool with no copy in the distribution is resolved from its Windows definition and started through WSL's interop (`Wsl/`: `WslInterop`, `WslHost`, `WslPaths`; `OsSettingsResolver.TryFindOnHost`). The host's variables come from one `cmd.exe /u /c set`, its drives from `/proc/mounts` and the name of the distribution's share from one `wslpath -w /`. Paths are translated in process, not by `wslpath` per path, because `ResolvedTool.GetArguments` runs for every failing pair, opened or not. A program on the host's PATH is found from one listing of each host directory on it (`WslHost.TryFindOnPath`), a stat on a Windows drive from inside WSL being most of a millisecond. What this side holds for such a tool is WSL's `/init` standing in for it: `ps` lists it as `/init <exe> <exe> <args>` (`WslInterop.StripProxy`), ending it does not close a windowed tool, and it ends with the terminal session while the window stays. So `ResolvedTool.CanKill` is false, which is what anyone handed the process id is told, and the library closes the tool itself on the host: `WslInterop.Kill` has Windows PowerShell end the processes of the tool's image whose command line names both files. That is a third of a second, so it is only asked once the stand-in is found by its command, and a tool whose session has gone is neither refreshed nor closed. `WslKillScriptTests` runs the script on Windows, where it can be run. It is started with all three streams redirected and closed (`WslInterop.Start`), or it would hold the pipe `dotnet test` reads. Not offered: the viewer (the host's loopback is not the distribution's under WSL's default networking), a Windows executable that is a `.cmd`, and Vim and Neovim. `DiffEngine_WslWindowsTools=false` turns it off. None of it can run in CI: `WslHostTests` stand a temp directory in for the drive, and the launch itself was run by hand in an Ubuntu distribution under WSL 2, with Beyond Compare, WinMerge and P4Merge
 - Tool order can be customized via `DiffEngine_ToolOrder` environment variable
 - `DisabledChecker` respects `DiffEngine_Disabled` env var
 - `ViewerClient` remembers a port found unowned for ten minutes (`RecheckUnownedAfter`), and the library's telling sends - settle, retire, move, delete, the first inline or diff send - skip the connect while that stands. A refused loopback connection costs two seconds on Windows (firewall stealth mode drops the reset), and a green run settles once per inline verification, which was six minutes for a class of 188 inline tests. Probes (`IsOwned`), the hosts and `InlineQueueClient` always ask and correct the memory; so does `SettleAppliedInline`, being one send per accept. Asking, on Windows, is the operating system's listener table first (`ListenerTable`, shared with `PiperClient`): no listener on the port means nobody to connect to, said without the two seconds, and a listener or a table that cannot be read leaves the connect to answer. So the first telling send of a test process, the launch gate's probe and each of its polls no longer wait to be refused. By port alone, whichever address, since the table is only believed when it says nobody is there. Not for a port that accepted a connection in the last second (`TrustOwnerFor`), because reading the table is reading every connection the machine has, and a run of settles to a live owner would pay more for each than the connect costs. The table is Windows' listeners alone, by `GetExtendedTcpTable` (`ListenerTable.Listeners`), falling back to .NET's list of every connection where that call is not there: 0.32 ms beside 3,000 connections where it was 13. What the table found empty is asked about again after a second (`RecheckUnlistedAfter`), so a tray started after a test process is found; the ten minutes is only for what a connect found. And only a refusal or an unanswered connect is remembered as nobody being there (`NobodyThere`). A viewer launch that failed gives its `MaxInstance` slot back

docs/diff-tool.md

@@ -152,7 +152,7 @@ The [tool order](/docs/diff-tool.order.md) applies as it does anywhere else, so
 
 What differs from a test run on Windows:
 
- * A Windows tool is not closed when its test passes. WSL runs a Windows program through a process of its own, and ending that process does not close the program's window. For the same reason a tool that does not auto refresh is given a second window when the same comparison fails again, where on Windows its window is replaced.
+ * A Windows tool is only found again while the terminal session that started it is open. WSL runs a Windows program through a process of its own, which is how the tool is recognized, and that process ends with the session while the tool's window stays. A tool left open past its session is not closed when its test passes, and gets a second window when the same comparison fails again.
  * Visual Studio Code is started by its `code` launcher, which WSL places on the `PATH`, and opens both files through its WSL extension. A tool that is a script on Windows (`.cmd`) cannot be started from a distribution.
  * Vim and Neovim are only used from the distribution, since they run in the terminal they are started from.
  * [DiffEngineTray](/docs/tray.md) is not supported from inside a distribution.

docs/mdsource/diff-tool.source.md

@@ -139,7 +139,7 @@ The [tool order](/docs/diff-tool.order.md) applies as it does anywhere else, so
 
 What differs from a test run on Windows:
 
- * A Windows tool is not closed when its test passes. WSL runs a Windows program through a process of its own, and ending that process does not close the program's window. For the same reason a tool that does not auto refresh is given a second window when the same comparison fails again, where on Windows its window is replaced.
+ * A Windows tool is only found again while the terminal session that started it is open. WSL runs a Windows program through a process of its own, which is how the tool is recognized, and that process ends with the session while the tool's window stays. A tool left open past its session is not closed when its test passes, and gets a second window when the same comparison fails again.
  * Visual Studio Code is started by its `code` launcher, which WSL places on the `PATH`, and opens both files through its WSL extension. A tool that is a script on Windows (`.cmd`) cannot be started from a distribution.
  * Vim and Neovim are only used from the distribution, since they run in the terminal they are started from.
  * [DiffEngineTray](/docs/tray.md) is not supported from inside a distribution.

src/DiffEngine.Tests/WslInteropTests.cs

@@ -37,6 +37,21 @@ public async Task TheProxyIsTakenOffAWindowsToolsCommand(string listed, string c
     public async Task AnythingElseIsLeftAlone(string listed) =>
         await Assert.That(WslInterop.StripProxy(listed)).IsEqualTo(listed);
 
+    /// <summary>
+    /// A value is never text in the script that ends a tool on the host, so a quote in a path
+    /// cannot end a string early. It is not only the straight quote that PowerShell takes for
+    /// one. <see cref="WslKillScriptTests" /> runs the script.
+    /// </summary>
+    [Test]
+    public async Task NoValueIsWrittenIntoTheKillScriptAsText()
+    {
+        var script = WslInterop.KillScript("to'ol.exe", @"C:\it's\a’b.received.txt", @"C:\it's\a’b.verified.txt");
+
+        await Assert.That(script).DoesNotContain("to'ol");
+        await Assert.That(script).DoesNotContain("it's");
+        await Assert.That(script).DoesNotContain("’");
+    }
+
     /// <summary>
     /// The viewer is spoken to over a loopback port the host does not share with a
     /// distribution, and the two editors run in a terminal a test process does not have. Every

src/DiffEngine.Tests/WslKillScriptTests.cs

@@ -0,0 +1,118 @@
+/// <summary>
+/// The script that ends a Windows tool on behalf of a test process inside WSL, run here by the
+/// PowerShell it is written for.
+/// <para>
+/// Inside WSL the script is all that reaches the host, so it is the part worth running for
+/// real, and Windows is where it runs. A <c>cmd.exe</c> that names two files on its command
+/// line and then waits stands in for a diff tool showing them.
+/// </para>
+/// </summary>
+[NotInParallel]
+[RunOn(TUnit.Core.Enums.OS.Windows)]
+public class WslKillScriptTests
+{
+    [Test]
+    public async Task TheProcessNamingBothFilesIsEnded()
+    {
+        var (temp, target) = Pair();
+        using var tool = StandIn(temp, target);
+        try
+        {
+            var ended = Run(WslInterop.KillScript("cmd.exe", temp, target));
+
+            await Assert.That(ended).IsEqualTo(1);
+            await Assert.That(tool.WaitForExit(5000)).IsTrue();
+        }
+        finally
+        {
+            End(tool);
+        }
+    }
+
+    /// <summary>
+    /// The image and the paths are matched as Windows and its file system match them.
+    /// </summary>
+    [Test]
+    public async Task CaseIsIgnored()
+    {
+        var (temp, target) = Pair();
+        using var tool = StandIn(temp, target);
+        try
+        {
+            var ended = Run(WslInterop.KillScript("CMD.EXE", temp.ToUpperInvariant(), target.ToUpperInvariant()));
+
+            await Assert.That(ended).IsEqualTo(1);
+            await Assert.That(tool.WaitForExit(5000)).IsTrue();
+        }
+        finally
+        {
+            End(tool);
+        }
+    }
+
+    /// <summary>
+    /// A tool can be showing the same received file against another target, and another program
+    /// altogether can have both files on its command line. Neither is the window for this pair.
+    /// </summary>
+    [Test]
+    public async Task AProcessNamingOneFileOrOfAnotherImageIsLeft()
+    {
+        var (temp, target) = Pair();
+        var (_, otherTarget) = Pair();
+        using var tool = StandIn(temp, otherTarget);
+        try
+        {
+            await Assert.That(Run(WslInterop.KillScript("cmd.exe", temp, target))).IsEqualTo(0);
+            await Assert.That(Run(WslInterop.KillScript("notepad.exe", temp, otherTarget))).IsEqualTo(0);
+            await Assert.That(tool.HasExited).IsFalse();
+        }
+        finally
+        {
+            End(tool);
+        }
+    }
+
+    // A quote and a space, which are what a path is most likely to hold that a script minds
+    static (string temp, string target) Pair()
+    {
+        var name = $@"C:\probe\it's a {Guid.NewGuid():N}";
+        return ($"{name}.received.txt", $"{name}.verified.txt");
+    }
+
+    /// <summary>
+    /// A <c>cmd.exe</c> with both paths on its command line, which then waits on an input that
+    /// is never written to.
+    /// </summary>
+    static Process StandIn(string temp, string target) =>
+        Process.Start(
+            new ProcessStartInfo("cmd.exe", $"/k rem \"{temp}\" \"{target}\"")
+            {
+                UseShellExecute = false,
+                CreateNoWindow = true,
+                RedirectStandardInput = true,
+                RedirectStandardOutput = true
+            })!;
+
+    static void End(Process tool)
+    {
+        if (!tool.HasExited)
+        {
+            tool.Kill();
+        }
+    }
+
+    static int Run(string script)
+    {
+        var encoded = Convert.ToBase64String(Encoding.Unicode.GetBytes(script));
+        using var process = Process.Start(
+            new ProcessStartInfo("powershell.exe", $"-NoProfile -NonInteractive -EncodedCommand {encoded}")
+            {
+                UseShellExecute = false,
+                CreateNoWindow = true,
+                RedirectStandardOutput = true
+            })!;
+        var output = process.StandardOutput.ReadToEnd();
+        process.WaitForExit();
+        return int.Parse(output.Trim());
+    }
+}

src/DiffEngine/DiffRunner.cs

@@ -419,7 +419,7 @@ internal static LaunchResult InnerLaunch(
                 return LaunchResult.AlreadyRunningAndSupportsRefresh;
             }
 
-            replacing = KillIfNotMdi(tool, command);
+            replacing = KillIfNotMdi(tool, command, tempFile, targetFile);
         }
 
         // A replacement does not raise the number of open tools, so it does not spend a slot. The
@@ -481,7 +481,7 @@ await PendingFiles.AddDerivedAsync(viewer, tempFile, targetFile, source, Cancel.
                 return LaunchResult.AlreadyRunningAndSupportsRefresh;
             }
 
-            replacing = KillIfNotMdi(tool, command);
+            replacing = KillIfNotMdi(tool, command, tempFile, targetFile);
         }
 
         // As above: a replacement is not a new instance
@@ -637,16 +637,21 @@ Failed to launch diff tool.
     /// <summary>
     /// Closes the tool already showing this pair, and reports whether it did. An MDI tool hosts
     /// every diff in one window, so there is nothing to close and nothing being replaced. A
-    /// Windows tool started from WSL cannot be closed from here, so its window is left and the
-    /// pair gets another beside it.
+    /// Windows tool started from WSL is closed on the host, and where that could not be done
+    /// its window is left and the pair gets another beside it.
     /// </summary>
-    static bool KillIfNotMdi(ResolvedTool tool, string command)
+    static bool KillIfNotMdi(ResolvedTool tool, string command, string tempFile, string targetFile)
     {
-        if (!tool.CanKill)
+        if (tool.IsMdi)
         {
             return false;
         }
 
+        if (tool.IsWindowsProgramInWsl)
+        {
+            return WslInterop.Kill(tool, tempFile, targetFile);
+        }
+
         ProcessCleanup.Kill(command);
         return true;
     }

src/DiffEngine/DiffRunner_Kill.cs

@@ -39,11 +39,11 @@ public static void Kill(string tempFile, string targetFile)
         }
 
         // What could be ended here is WSL's stand-in for the tool, and a windowed tool outlives
-        // that. Ending it would only lose track of a window that is still open, and the next
-        // failing run for this pair would then open a second
+        // that. Ending it would only lose track of a window that is still open, so the tool is
+        // ended where it runs instead
         if (diffTool.IsWindowsProgramInWsl)
         {
-            Logging.Write($"DiffTool is a Windows program run from WSL so not killing. diffTool: {diffTool.ExePath}");
+            WslInterop.Kill(diffTool, tempFile, targetFile);
             return;
         }
 

src/DiffEngine/ResolvedTool.cs

@@ -20,9 +20,11 @@ public string BuildCommand(string tempFile, string targetFile) =>
         WslInterop.IsWindowsProgram(ExePath);
 
     /// <summary>
-    /// Whether the window opened for a pair can be closed by ending the process started for it.
-    /// Not for an MDI tool, whose one window holds every pair, and not for a Windows tool started
-    /// from WSL, where that process is not the tool.
+    /// Whether the window opened for a pair can be closed by ending the process started for it,
+    /// which is what whoever a pending move is handed to is told. Not for an MDI tool, whose one
+    /// window holds every pair, and not for a Windows tool started from WSL, where that process
+    /// is not the tool: this library closes such a tool itself, on the host
+    /// (<see cref="WslInterop.Kill" />), and nobody it hands the process id to can.
     /// </summary>
     internal bool CanKill =>
         !IsMdi &&

src/DiffEngine/Wsl/WslHost.cs

@@ -60,12 +60,7 @@ public bool TryFindOnPath(string name, [NotNullWhen(true)] out string? path) =>
     Dictionary<string, string> ListPrograms(IReadOnlyList<string> directories)
     {
         var programs = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-        string? systemRoot = null;
-        if (variables.TryGetValue("SystemRoot", out var windowsDirectory))
-        {
-            Paths.TryToLinux(windowsDirectory, out systemRoot);
-        }
-
+        var systemRoot = SystemRoot();
         foreach (var directory in directories)
         {
             if (!Paths.IsOnHost(directory) ||
@@ -100,6 +95,40 @@ Dictionary<string, string> ListPrograms(IReadOnlyList<string> directories)
         return programs;
     }
 
+    /// <summary>
+    /// The host's Windows directory as this process sees it, or null when the host did not say
+    /// where it is or its drive is not mounted.
+    /// </summary>
+    string? SystemRoot()
+    {
+        if (variables.TryGetValue("SystemRoot", out var windowsDirectory) &&
+            Paths.TryToLinux(windowsDirectory, out var systemRoot))
+        {
+            return systemRoot;
+        }
+
+        return null;
+    }
+
+    /// <summary>
+    /// Windows PowerShell, which every Windows this can run on has, and which is the one way to
+    /// ask the host about its own processes from here. Looked for where Windows keeps it before
+    /// the PATH, since that is one question rather than one for each directory on it.
+    /// </summary>
+    public bool TryFindPowerShell([NotNullWhen(true)] out string? path)
+    {
+        if (SystemRoot() is { } systemRoot)
+        {
+            path = $"{systemRoot}/System32/WindowsPowerShell/v1.0/powershell.exe";
+            if (File.Exists(path))
+            {
+                return true;
+            }
+        }
+
+        return OsSettingsResolver.TryFindInEnvPath("powershell.exe", out path);
+    }
+
     static bool IsWithin(string directory, string? parent)
     {
         if (parent is null)
@@ -325,7 +354,7 @@ static bool TryFind(string name, string[] candidates, [NotNullWhen(true)] out st
     /// <summary>
     /// What a program printed, or null when it did not start, failed, or did not finish in time.
     /// </summary>
-    static string? Run(string file, string arguments, string directory, Encoding encoding)
+    internal static string? Run(string file, string arguments, string directory, Encoding encoding)
     {
         using var process = new Process
         {