From 87e9474a912848d8e2a983555e1095dfa38982de Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 12:57:42 +1000 Subject: [PATCH] Do not spend an instance slot on a relaunch InnerLaunch killed the tool already showing a pair and then asked MaxInstance whether it was allowed to launch. Once the per process counter was spent the answer was no - so a re-failing test closed its own diff window and opened nothing in its place, reporting TooManyRunningDiffTools with the move sent as processId null. The count is meant to bound how many tools are open at once, and a replacement does not raise that number. KillIfNotMdi now reports whether it actually closed something, and a launch that is replacing skips the check. An MDI tool closes nothing, so it is not replacing and still counts. MaxInstance gains an internal ResetCount, because the counter is process wide and never reset, so a limit means nothing definite in a test otherwise. Worth knowing for anyone else writing one of these: MaxInstance reads DiffEngine_MaxInstances before the app domain value, and that variable persists per user - it is set to 5 on this machine, which is why the test sets it per process rather than calling MaxInstancesToLaunch alone. My first version did call it alone, and silently ran with a limit of 5. --- .../MaxInstanceReplacementTests.cs | 134 ++++++++++++++++++ src/DiffEngine/DiffRunner.cs | 29 +++- src/DiffEngine/MaxInstance.cs | 7 + 3 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 src/DiffEngine.Tests/MaxInstanceReplacementTests.cs diff --git a/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs b/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs new file mode 100644 index 00000000..7f673f0b --- /dev/null +++ b/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs @@ -0,0 +1,134 @@ +#if NET10_0 +/// +/// Relaunching a pair that is already open does not spend an instance slot. +/// +/// InnerLaunch killed the tool already showing the pair and then asked MaxInstance whether it was +/// allowed to launch. Once the per process counter was spent that answer was no - so a re-failing +/// test closed its own diff window and opened nothing in its place, and reported +/// TooManyRunningDiffTools with the move sent as processId null. The number of open tools had gone +/// down, not up. +/// +/// +[NotInParallel] +[RunOn(TUnit.Core.Enums.OS.Windows)] +public class MaxInstanceReplacementTests : + IDisposable +{ + [Test] + public async Task RelaunchingTheSamePairIsNotANewInstance() + { + LimitTo(1); + + ProcessCleanup.Refresh(); + var first = DiffRunner.Launch(temp, target); + await Assert.That(first).IsEqualTo(LaunchResult.StartedNewInstance); + + await WaitForRunning(); + ProcessCleanup.Refresh(); + + // The slot is spent, but this pair is already open - so this is a replacement + var second = DiffRunner.Launch(temp, target); + await Assert.That(second).IsEqualTo(LaunchResult.StartedNewInstance); + } + + /// + /// And a different pair still runs into the limit, which is what the limit is for. + /// + [Test] + public async Task ADifferentPairStillHitsTheLimit() + { + LimitTo(1); + + ProcessCleanup.Refresh(); + await Assert.That(DiffRunner.Launch(temp, target)).IsEqualTo(LaunchResult.StartedNewInstance); + + ProcessCleanup.Refresh(); + await Assert.That(DiffRunner.Launch(otherTemp, otherTarget)).IsEqualTo(LaunchResult.TooManyRunningDiffTools); + } + + /// + /// Through the environment variable, because that is what MaxInstance reads first and this + /// machine may well have one set - DiffEngine_MaxInstances persists per user, so the app + /// domain setting alone silently loses to it. Process scoped, so nothing outlives the run. + /// + static void LimitTo(int value) + { + Environment.SetEnvironmentVariable(variable, value.ToString()); + // Forces MaxInstance to re-read, since it caches the first answer + DiffRunner.MaxInstancesToLaunch(value); + MaxInstance.ResetCount(); + } + + async Task WaitForRunning() + { + var command = tool.BuildCommand(temp, target); + for (var attempt = 0; attempt < 40; attempt++) + { + ProcessCleanup.Refresh(); + if (ProcessCleanup.IsRunning(command)) + { + return; + } + + await Task.Delay(250); + } + } + + public MaxInstanceReplacementTests() + { + Directory.CreateDirectory(directory); + + temp = Write("first.received.zzmax"); + target = Write("first.verified.zzmax"); + otherTemp = Write("second.received.zzmax"); + otherTarget = Write("second.verified.zzmax"); + tool = DiffTools.AddTool( + name: $"MaxProbe{Guid.NewGuid():N}", + autoRefresh: false, + isMdi: false, + supportsText: false, + requiresTarget: true, + useShellExecute: false, + launchArguments: new( + Left: (t, g) => $"\"{t}\" \"{g}\"", + Right: (t, g) => $"\"{g}\" \"{t}\""), + exePath: FakeDiffTool.Exe, + binaryExtensions: [".zzmax"])!; + } + + string Write(string name) + { + var path = Path.Combine(directory, name); + File.WriteAllText(path, name); + return path; + } + + public void Dispose() + { + Environment.SetEnvironmentVariable(variable, original); + DiffRunner.MaxInstancesToLaunch(5); + MaxInstance.ResetCount(); + try + { + DiffRunner.Kill(temp, target); + DiffRunner.Kill(otherTemp, otherTarget); + Directory.Delete(directory, true); + } + catch + { + // Best effort: the fake tool exits on its own anyway + } + } + + // Per test, not static: two tests sharing paths means the second one's first launch finds + // the first one's tool still open and is treated as a replacement + const string variable = "DiffEngine_MaxInstances"; + string? original = Environment.GetEnvironmentVariable(variable); + string directory = Path.Combine(Path.GetTempPath(), $"DiffEngine.MaxInstance.{Guid.NewGuid():N}"); + ResolvedTool tool; + string temp; + string target; + string otherTemp; + string otherTarget; +} +#endif diff --git a/src/DiffEngine/DiffRunner.cs b/src/DiffEngine/DiffRunner.cs index 2aaab7a4..eb492457 100644 --- a/src/DiffEngine/DiffRunner.cs +++ b/src/DiffEngine/DiffRunner.cs @@ -157,6 +157,7 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile, tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); var canKill = !tool.IsMdi; + var replacing = false; if (ProcessCleanup.TryGetProcessInfo(command, out var processCommand)) { if (tool.AutoRefresh) @@ -165,10 +166,14 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile, return LaunchResult.AlreadyRunningAndSupportsRefresh; } - KillIfNotMdi(tool, command); + replacing = KillIfNotMdi(tool, command); } - if (MaxInstance.Reached()) + // A replacement does not raise the number of open tools, so it does not spend a slot. The + // kill above has already happened by this point, so counting it meant a re-failing test + // closed its own window and then declined to open another + if (!replacing && + MaxInstance.Reached()) { DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null); return LaunchResult.TooManyRunningDiffTools; @@ -192,6 +197,7 @@ static async Task InnerLaunchAsync(TryResolveTool tryResolveTool, tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); var canKill = !tool.IsMdi; + var replacing = false; if (ProcessCleanup.TryGetProcessInfo(command, out var processCommand)) { if (tool.AutoRefresh) @@ -200,10 +206,12 @@ static async Task InnerLaunchAsync(TryResolveTool tryResolveTool, return LaunchResult.AlreadyRunningAndSupportsRefresh; } - KillIfNotMdi(tool, command); + replacing = KillIfNotMdi(tool, command); } - if (MaxInstance.Reached()) + // As above: a replacement is not a new instance + if (!replacing && + MaxInstance.Reached()) { await DiffEngineTray.AddMoveAsync(tempFile, targetFile, tool.ExePath, arguments, canKill, null); return LaunchResult.TooManyRunningDiffTools; @@ -295,12 +303,19 @@ Failed to launch diff tool. } } - static void KillIfNotMdi(ResolvedTool tool, string command) + /// + /// 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. + /// + static bool KillIfNotMdi(ResolvedTool tool, string command) { - if (!tool.IsMdi) + if (tool.IsMdi) { - ProcessCleanup.Kill(command); + return false; } + + ProcessCleanup.Kill(command); + return true; } static void GuardFiles(string tempFile, string targetFile) diff --git a/src/DiffEngine/MaxInstance.cs b/src/DiffEngine/MaxInstance.cs index 3a14d6cb..6bd7fb13 100644 --- a/src/DiffEngine/MaxInstance.cs +++ b/src/DiffEngine/MaxInstance.cs @@ -45,6 +45,13 @@ public static void SetForUser(int value) ResetCapturedValue(); } + /// + /// Forgets the instances launched so far. For tests, which need the limit to mean something + /// definite rather than however many launches the rest of the run happened to make. + /// + internal static void ResetCount() => + Interlocked.Exchange(ref launchedInstances, 0); + public static bool Reached() { var count = Interlocked.Increment(ref launchedInstances);