From d799c6d3a00850ace4799fe1a46a2738688e7768 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 10 Oct 2026 13:56:25 +1100 Subject: [PATCH] Close a Windows diff tool started from WSL Ending WSL's stand-in for a Windows tool does not close a windowed tool, so the tool was left open when its test passed and a tool that does not refresh got a second window. The tool is now ended on the host: Windows PowerShell ends the processes of its image whose command line names both files. It is only asked once the stand-in has been found by its command. --- claude.md | 2 +- docs/diff-tool.md | 2 +- docs/mdsource/diff-tool.source.md | 2 +- src/DiffEngine.Tests/WslInteropTests.cs | 15 +++ src/DiffEngine.Tests/WslKillScriptTests.cs | 118 +++++++++++++++++++++ src/DiffEngine/DiffRunner.cs | 17 +-- src/DiffEngine/DiffRunner_Kill.cs | 6 +- src/DiffEngine/ResolvedTool.cs | 8 +- src/DiffEngine/Wsl/WslHost.cs | 43 ++++++-- src/DiffEngine/Wsl/WslInterop.cs | 78 +++++++++++++- src/DiffEngine/Wsl/WslPaths.cs | 8 +- 11 files changed, 270 insertions(+), 29 deletions(-) create mode 100644 src/DiffEngine.Tests/WslKillScriptTests.cs diff --git a/claude.md b/claude.md index b3123683..ec959c23 100644 --- a/claude.md +++ b/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 ` (`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 ` (`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 diff --git a/docs/diff-tool.md b/docs/diff-tool.md index 6bd2e956..3114c3a6 100644 --- a/docs/diff-tool.md +++ b/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. diff --git a/docs/mdsource/diff-tool.source.md b/docs/mdsource/diff-tool.source.md index 7b997c21..1700380b 100644 --- a/docs/mdsource/diff-tool.source.md +++ b/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. diff --git a/src/DiffEngine.Tests/WslInteropTests.cs b/src/DiffEngine.Tests/WslInteropTests.cs index e91ab5c5..181436a3 100644 --- a/src/DiffEngine.Tests/WslInteropTests.cs +++ b/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); + /// + /// 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. runs the script. + /// + [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("’"); + } + /// /// 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 diff --git a/src/DiffEngine.Tests/WslKillScriptTests.cs b/src/DiffEngine.Tests/WslKillScriptTests.cs new file mode 100644 index 00000000..d0b5c06e --- /dev/null +++ b/src/DiffEngine.Tests/WslKillScriptTests.cs @@ -0,0 +1,118 @@ +/// +/// The script that ends a Windows tool on behalf of a test process inside WSL, run here by the +/// PowerShell it is written for. +/// +/// 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 cmd.exe that names two files on its command +/// line and then waits stands in for a diff tool showing them. +/// +/// +[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); + } + } + + /// + /// The image and the paths are matched as Windows and its file system match them. + /// + [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); + } + } + + /// + /// 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. + /// + [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"); + } + + /// + /// A cmd.exe with both paths on its command line, which then waits on an input that + /// is never written to. + /// + 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()); + } +} diff --git a/src/DiffEngine/DiffRunner.cs b/src/DiffEngine/DiffRunner.cs index cdb6764a..827ea744 100644 --- a/src/DiffEngine/DiffRunner.cs +++ b/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. /// /// 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. /// - 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; } diff --git a/src/DiffEngine/DiffRunner_Kill.cs b/src/DiffEngine/DiffRunner_Kill.cs index d437cdfd..a66cb03c 100644 --- a/src/DiffEngine/DiffRunner_Kill.cs +++ b/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; } diff --git a/src/DiffEngine/ResolvedTool.cs b/src/DiffEngine/ResolvedTool.cs index 7b172aa6..b441a6a3 100644 --- a/src/DiffEngine/ResolvedTool.cs +++ b/src/DiffEngine/ResolvedTool.cs @@ -20,9 +20,11 @@ public string BuildCommand(string tempFile, string targetFile) => WslInterop.IsWindowsProgram(ExePath); /// - /// 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 + /// (), and nobody it hands the process id to can. /// internal bool CanKill => !IsMdi && diff --git a/src/DiffEngine/Wsl/WslHost.cs b/src/DiffEngine/Wsl/WslHost.cs index 2a99c092..a8823336 100644 --- a/src/DiffEngine/Wsl/WslHost.cs +++ b/src/DiffEngine/Wsl/WslHost.cs @@ -60,12 +60,7 @@ public bool TryFindOnPath(string name, [NotNullWhen(true)] out string? path) => Dictionary ListPrograms(IReadOnlyList directories) { var programs = new Dictionary(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 ListPrograms(IReadOnlyList directories) return programs; } + /// + /// 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. + /// + string? SystemRoot() + { + if (variables.TryGetValue("SystemRoot", out var windowsDirectory) && + Paths.TryToLinux(windowsDirectory, out var systemRoot)) + { + return systemRoot; + } + + return null; + } + + /// + /// 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. + /// + 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 /// /// What a program printed, or null when it did not start, failed, or did not finish in time. /// - 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 { diff --git a/src/DiffEngine/Wsl/WslInterop.cs b/src/DiffEngine/Wsl/WslInterop.cs index e72268e2..86446c0a 100644 --- a/src/DiffEngine/Wsl/WslInterop.cs +++ b/src/DiffEngine/Wsl/WslInterop.cs @@ -14,9 +14,9 @@ namespace DiffEngine; /// that stands in for it, and three things follow. It is listed by ps with WSL's own /// program in front of the tool's command line, which takes off so a /// running tool is still found by the command it was started with. Ending it does not close a -/// windowed tool, so such a tool is never killed from here: it is not closed when its test -/// passes, and one that does not refresh gets a second window rather than a replacement. And it -/// ends with the terminal session that started it, while the window stays. +/// windowed tool, so the tool itself is ended instead, on the host (). And it +/// ends with the terminal session that started it, while the window stays: a tool left open +/// across sessions is no longer found, and so neither refreshed nor closed. /// /// static class WslInterop @@ -117,6 +117,78 @@ public static int Start(string exePath, string arguments) return process.Id; } + /// + /// Closes the Windows tool showing a pair, and reports whether one was closed. + /// + /// The process this side could signal is WSL's stand-in, and a windowed tool outlives that. + /// So the tool is found where it runs, by Windows PowerShell on the host: the processes of + /// the tool's image whose command line names both files, by the paths the tool was handed. + /// By both, because a tool can be showing the same received file against another target, + /// and by image, so nothing else that happens to mention them is ended. + /// + /// + /// That is a process started and a question put to Windows, a third of a second, where + /// ending a process here is nothing. So it is only asked once the stand-in has been found + /// by its command, which is what says a window was opened for this pair and is still there: + /// a passing verification with no window pays for a look through a list, as it does for + /// any other tool. + /// + /// + public static bool Kill(ResolvedTool tool, string tempFile, string targetFile) + { + var command = tool.BuildCommand(tempFile, targetFile); + if (!ProcessCleanup.IsRunning(command)) + { + return false; + } + + if (!Host!.TryFindPowerShell(out var powerShell)) + { + Logging.Write($"Windows PowerShell was not found, so the tool was left open. Command: {command}"); + return false; + } + + var script = KillScript(Path.GetFileName(tool.ExePath), ToWindows(tempFile), ToWindows(targetFile)); + var encoded = Convert.ToBase64String(Encoding.Unicode.GetBytes(script)); + var output = WslHost.Run( + powerShell, + $"-NoProfile -NonInteractive -EncodedCommand {encoded}", + Path.GetDirectoryName(powerShell)!, + Encoding.UTF8); + var closed = int.TryParse(output?.Trim(), out var count) && count > 0; + Logging.Write($"Kill on the Windows host: {command}. Closed: {closed}"); + return closed; + } + + /// + /// What PowerShell is asked to run: end every process of this image whose command line + /// names both paths, and say how many there were. + /// + /// The script is handed over encoded rather than as text, so nothing has to survive two + /// command lines, and each value is encoded again inside it: a path can hold a quote, and + /// PowerShell takes four other characters for one. The comparison ignores case, as the file + /// system the paths are on does. + /// + /// + internal static string KillScript(string image, string tempFile, string targetFile) => + $$""" + function Decode($value) { [Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($value)) } + $image = Decode '{{Encode(image)}}' + $temp = Decode '{{Encode(tempFile)}}' + $target = Decode '{{Encode(targetFile)}}' + $found = @(Get-CimInstance Win32_Process | Where-Object { + $_.Name -eq $image -and + $_.CommandLine -and + $_.CommandLine.IndexOf($temp, [StringComparison]::OrdinalIgnoreCase) -ge 0 -and + $_.CommandLine.IndexOf($target, [StringComparison]::OrdinalIgnoreCase) -ge 0 + }) + $found | ForEach-Object { Stop-Process -Id $_.ProcessId -Force -ErrorAction SilentlyContinue } + $found.Count + """; + + static string Encode(string value) => + Convert.ToBase64String(Encoding.Unicode.GetBytes(value)); + /// /// The command line a Windows tool was started with, from the one ps lists for it. /// diff --git a/src/DiffEngine/Wsl/WslPaths.cs b/src/DiffEngine/Wsl/WslPaths.cs index 1348137b..66ae9481 100644 --- a/src/DiffEngine/Wsl/WslPaths.cs +++ b/src/DiffEngine/Wsl/WslPaths.cs @@ -11,10 +11,10 @@ namespace DiffEngine; /// /// /// Worked out here rather than by running wslpath per path. A command line is built for -/// every failing pair, whether or not a window ends up being opened for it, so a process a -/// path would be a thousand of them for a run with five hundred failures. What is needed is -/// read once instead: the drive mounts from /proc/mounts, and what the host calls the -/// distribution's root. +/// every pair, to open a window for one that fails and to find a window to close for one that +/// passes, so a process a path would be two for every verification in a run. What is needed +/// is read once instead: the drive mounts from /proc/mounts, and what the host calls +/// the distribution's root. /// /// class WslPaths