Do not spend an instance slot on a relaunch

src/DiffEngine.Tests/MaxInstanceReplacementTests.cs

@@ -0,0 +1,134 @@
+#if NET10_0
+/// <summary>
+/// Relaunching a pair that is already open does not spend an instance slot.
+/// <para>
+/// 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.
+/// </para>
+/// </summary>
+[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);
+    }
+
+    /// <summary>
+    /// And a different pair still runs into the limit, which is what the limit is for.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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

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<LaunchResult> 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<LaunchResult> 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)
+    /// <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.
+    /// </summary>
+    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)

src/DiffEngine/MaxInstance.cs

@@ -45,6 +45,13 @@ public static void SetForUser(int value)
         ResetCapturedValue();
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    internal static void ResetCount() =>
+        Interlocked.Exchange(ref launchedInstances, 0);
+
     public static bool Reached()
     {
         var count = Interlocked.Increment(ref launchedInstances);