From 56ed12cd44cc75c1d6f3d7071ef31c03183792af Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 21 Aug 2026 21:07:58 +1000 Subject: [PATCH] Read the process list fresh in Kill and TryGetProcessInfo ProcessCleanup filled its list once, in the static constructor, and Refresh had no other caller in the library. So every question about what is running was answered from a snapshot taken the first time anything touched DiffEngine. Inside one process that means a tool launched after first use is invisible for the rest of that process: - Launch then Kill for the same pair logs "No matching commands" and leaves the tool open. - A relaunch never sees the running instance, so a second window opens and another MaxInstance slot goes with it. - An AutoRefresh tool is relaunched instead of being reused and refreshed. - A PID from the startup snapshot may belong to something else entirely by the time Kill terminates it. Refresh at the top of Kill and TryGetProcessInfo. It costs about 14ms on Windows now that CandidateExeNames restricts the command line reads to resolved diff tool images, against a call that is about to start or kill a process. DiffRunnerTests could not have caught this: every test polls through WaitForRunning, which calls Refresh itself, and FakeDiffTool exits on its own after five seconds, so a kill that matched nothing still looked like a kill that worked. The new test refreshes once while the tool is not running, launches, and then asks only the cached query - inside the five seconds, so a pass means it saw the process rather than outlived it. --- src/DiffEngine.Tests/DiffRunnerTests.cs | 47 ++++++++++++++++++++++++ src/DiffEngine/Process/ProcessCleanup.cs | 18 +++++++++ 2 files changed, 65 insertions(+) diff --git a/src/DiffEngine.Tests/DiffRunnerTests.cs b/src/DiffEngine.Tests/DiffRunnerTests.cs index ed9acacf..b2bbe2ad 100644 --- a/src/DiffEngine.Tests/DiffRunnerTests.cs +++ b/src/DiffEngine.Tests/DiffRunnerTests.cs @@ -181,6 +181,53 @@ public async Task LaunchAndKill() await Assert.That(ProcessCleanup.IsRunning(command)).IsFalse(); } + /// + /// A tool launched after the process list was filled has to be visible without anything + /// asking for a refresh first. + /// + /// The list was filled once, by the static constructor, and nothing in the library ever + /// refreshed it. So within one process a tool launched after first use was invisible for the + /// rest of that process: Kill logged "No matching commands" and left it open, a relaunch + /// never saw the running instance and opened a second window while spending another + /// MaxInstance slot, and an AutoRefresh tool was relaunched rather than reused. Every test + /// here polls through WaitForRunning, which calls Refresh itself, so none of them could see + /// it - and FakeDiffTool exits on its own after five seconds, so even a kill that found + /// nothing looked like a kill that worked. + /// + /// + [Test] + public async Task AToolLaunchedAfterTheListWasFilledIsVisible() + { + await WaitForRunning(false); + // The stale snapshot: taken while the tool is not running + ProcessCleanup.Refresh(); + await Assert.That(ProcessCleanup.IsRunning(command)).IsFalse(); + + var result = await DiffRunner.LaunchAsync(file1, file2); + await Assert.That(result).IsEqualTo(LaunchResult.StartedNewInstance); + + // Nothing calls Refresh from here. Bounded well inside FakeDiffTool's five second life, + // so a pass means the query saw it rather than that the process outlived the poll + await Assert.That(await PollCached()).IsTrue(); + + DiffRunner.Kill(file1, file2); + await WaitForRunning(false); + } + + async Task PollCached() + { + for (var attempt = 0; attempt < 12; attempt++) + { + if (ProcessCleanup.IsRunning(command)) + { + return true; + } + + await Task.Delay(250); + } + + return false; + } [Test] public async Task LaunchAndKillAsync() { diff --git a/src/DiffEngine/Process/ProcessCleanup.cs b/src/DiffEngine/Process/ProcessCleanup.cs index f94324cc..9c61c691 100644 --- a/src/DiffEngine/Process/ProcessCleanup.cs +++ b/src/DiffEngine/Process/ProcessCleanup.cs @@ -27,6 +27,11 @@ static ProcessCleanup() Refresh(); } + /// + /// The processes as of the last . A snapshot, so callers that need to + /// know what is running now go through or , both of + /// which take their own. + /// public static IReadOnlyCollection Commands => commands; [MemberNotNull(nameof(commands))] @@ -60,6 +65,14 @@ public static void Kill(string command) command = TrimCommand(command); } + // The list was filled once by the static constructor and nothing in the library refreshed + // it, so this matched against whatever was running the first time anything touched + // DiffEngine. In one process a Launch followed by a Kill for the same pair logged "No + // matching commands" and left the tool open. It also keeps the PID as fresh as this can + // make it: a process that has since exited may have had its id reused, and terminating + // from a stale snapshot kills whatever holds it now + Refresh(); + var matchingCommands = Commands .Where(_ => _.Command == command).ToList(); Logging.Write($"Kill: {command}. Matching count: {matchingCommands.Count}"); @@ -91,6 +104,11 @@ public static bool TryGetProcessInfo(string command, out ProcessCommand process) command = TrimCommand(command); } + // As for Kill: the question is what is running now. Against the startup snapshot a tool + // launched later was never seen again, so an AutoRefresh tool opened a second window + // instead of being reused and every relaunch spent another MaxInstance slot + Refresh(); + process = commands.FirstOrDefault(_ => _.Command == command); return !process.Equals(default(ProcessCommand)); }