Charge MaxInstance for a viewer that has to be started (#855)

The viewer was exempt from the cap on the grounds that it queues rather
than opening a window per pair. That is true of the second pair and every
one after it, and false of the first: with nothing owning the queue,
AddDiff starts a process. So MaxInstancesToLaunch(0) - "no window opens" -
opened one, and a run with no tray put a viewer on screen per staged
snapshot with nothing available to stop it.

The check goes in ViewerLaunchGate, which is the only place that knows
which of the two is happening, and after the ownership probe. A caller
that asked would charge all twenty of a parallel run's callers for the one
window between them, and the nineteen that find the owner would be capped
out of forwarding their pairs to it.

Capped is its own outcome so AddDiff can report TooManyRunningDiffTools
rather than folding it in with a tool that could not be found.

Author: SimonCropp

claude.md

@@ -266,8 +266,10 @@ apart.
 - Unless that diff tool is the viewer, which is the `Diff` verb and `--diff <received> <target>`.
   Then the premise above is false — there is no window for the pair yet — so it is tracked exactly
   as a move and a window is raised over the entry, and `DiffRunner` skips the whole process per
-  pair path: nothing to find already showing it, no window to replace, no `MaxInstance` slot to
-  spend, and no process for the tray to kill on accept. `DiffRunner.Kill` sends `Settle` for the
+  pair path: nothing to find already showing it, no window to replace, and no process for the
+  tray to kill on accept. `MaxInstance` still applies, but charged by `ViewerLaunchGate` rather
+  than by `DiffRunner`, and only on a viewer that has to be started: handing a pair to one already
+  on screen opens no window and spends nothing, so the caller cannot be the one to ask. `DiffRunner.Kill` sends `Settle` for the
   move key rather than killing anything, since the row is drawn in a window shared with every other
   pending pair. That is what makes ten failing image snapshots one window instead of ten, and it is
   only available to the viewer because no other tool can be told to drop one pair.

docs/diff-tool.md

@@ -48,7 +48,9 @@ This allows, in most cases, for no manual closing of the tool to be required.<!-
 
 By default a maximum of 5 tool instances will be launched. This prevents a change that breaks many tests from causing too much load on a machine.
 
-This value can be changed using an environment variable or by explicitly specifying the value by code. When both are used, the environment variable value will be used.
+This value can be changed using an environment variable or by explicitly specifying the value by code. When both are used, the value set in code wins; the environment variable is the ambient default for a run that sets nothing.
+
+The count includes [DiffEngineViewer](/docs/viewer.md), but only when a viewer has to be started. Handing a pair to one already on screen opens no window and so spends nothing.
 
 
 ### Using an environment variable

docs/mdsource/diff-tool.source.md

@@ -41,7 +41,9 @@ include: diffToolCleanup
 
 By default a maximum of 5 tool instances will be launched. This prevents a change that breaks many tests from causing too much load on a machine.
 
-This value can be changed using an environment variable or by explicitly specifying the value by code. When both are used, the environment variable value will be used.
+This value can be changed using an environment variable or by explicitly specifying the value by code. When both are used, the value set in code wins; the environment variable is the ambient default for a run that sets nothing.
+
+The count includes [DiffEngineViewer](/docs/viewer.md), but only when a viewer has to be started. Handing a pair to one already on screen opens no window and so spends nothing.
 
 
 ### Using an environment variable

src/DiffEngine.Tests/PendingFilesDiffTests.cs

@@ -70,6 +70,37 @@ await Assert.That(owner.Heard).IsEquivalentTo(
         ]);
     }
 
+    /// <summary>
+    /// With nothing owning the queue this route starts a viewer, and MaxInstancesToLaunch(0) says
+    /// no window opens. It used to be exempt on the grounds that the viewer queues rather than
+    /// opening one per pair - true of every pair after the first, and not of the first, which
+    /// starts a process.
+    /// <para>
+    /// This is the arrangement a test suite that has to leave diff on runs in: no tray, no owner,
+    /// and the cap at zero. Before, every staged snapshot in such a run put a viewer on the
+    /// screen, and nothing in DiffEngine could be set to stop it.
+    /// </para>
+    /// </summary>
+    [Test]
+    public async Task WithNoOwnerAndNoSlotNothingIsStarted()
+    {
+        using var absent = new NoOwner();
+
+        DiffRunner.MaxInstancesToLaunch(0);
+        MaxInstance.ResetCount();
+        try
+        {
+            var result = await PendingFiles.AddDiffAsync(Viewer(), Temp, Target, Cancel.None);
+
+            await Assert.That(result).IsEqualTo(LaunchResult.TooManyRunningDiffTools);
+        }
+        finally
+        {
+            MaxInstance.ResetAppDomainValue();
+            MaxInstance.ResetCount();
+        }
+    }
+
     /// <summary>
     /// The other end: the pair's test started passing, so the row it took goes. A settle rather
     /// than a kill, because there is no process of its own to kill, and rather than a discard,

src/DiffEngine.Tests/ViewerLaunchGateTests.cs

@@ -1,4 +1,4 @@
-/// <summary>
+/// <summary>
 /// The gate that keeps a parallel run from starting a viewer per failing snapshot.
 /// <para>
 /// The ownership probe is supplied rather than the real one, so what is asserted is the gate's own
@@ -24,7 +24,8 @@ public async Task ManyCallersAtOnceLaunchOnce()
                 .Select(_ => Task.Run(() => ViewerLaunchGate.Launch(
                     retry: () => true,
                     launch: viewer.Start,
-                    isOwned: viewer.IsUp))));
+                    isOwned: viewer.IsUp,
+                    canLaunch: () => true))));
 
         await Assert.That(viewer.Starts).IsEqualTo(1);
         await Assert.That(outcomes.Count(_ => _ == ViewerLaunchOutcome.Launched)).IsEqualTo(1);
@@ -46,7 +47,8 @@ public async Task TheGateIsHeldUntilTheLaunchedViewerAnswers()
                 .Select(_ => Task.Run(() => ViewerLaunchGate.Launch(
                     retry: () => true,
                     launch: viewer.Start,
-                    isOwned: viewer.IsUp))));
+                    isOwned: viewer.IsUp,
+                    canLaunch: () => true))));
 
         await Assert.That(viewer.Starts).IsEqualTo(1);
         await Assert.That(outcomes.Count(_ => _ == ViewerLaunchOutcome.Taken)).IsEqualTo(9);
@@ -74,7 +76,8 @@ public async Task AViewerThatNeverAnswersDoesNotHoldTheGateForever()
                             Interlocked.Increment(ref starts);
                             return true;
                         },
-                        isOwned: () => false))));
+                        isOwned: () => false,
+                        canLaunch: () => true))));
 
             await Assert.That(starts).IsEqualTo(3);
             await Assert.That(outcomes.All(_ => _ == ViewerLaunchOutcome.Launched)).IsTrue();
@@ -96,7 +99,8 @@ public async Task ALaunchThatCouldNotStartIsReportedRatherThanWaitedOn()
             var outcome = ViewerLaunchGate.Launch(
                 retry: () => true,
                 launch: () => false,
-                isOwned: () => false);
+                isOwned: () => false,
+                canLaunch: () => true);
 
             await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Failed);
         }
@@ -129,6 +133,98 @@ public async Task ARefusingOwnerIsNotLaunchedOver()
         await Assert.That(launches).IsEqualTo(0);
     }
 
+    /// <summary>
+    /// MaxInstancesToLaunch(0) means no window opens, and the viewer is a window. It used to be
+    /// exempt on the grounds that it queues rather than opening one per pair, which is true of the
+    /// second pair and every one after, and not of the first: that one starts a process.
+    /// </summary>
+    [Test]
+    public async Task NoSlotMeansNoViewerIsStarted()
+    {
+        var viewer = new FakeViewer();
+
+        var outcome = ViewerLaunchGate.Launch(
+            retry: () => true,
+            launch: viewer.Start,
+            isOwned: () => false,
+            canLaunch: () => false);
+
+        await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Capped);
+        await Assert.That(viewer.Starts).IsEqualTo(0);
+    }
+
+    /// <inheritdoc cref="NoSlotMeansNoViewerIsStarted" />
+    [Test]
+    public async Task NoSlotMeansNoViewerIsStartedAsync()
+    {
+        var viewer = new FakeViewer();
+
+        var outcome = await ViewerLaunchGate.LaunchAsync(
+            retry: () => Task.FromResult(true),
+            launch: () => Task.FromResult(viewer.Start()),
+            Cancel.None,
+            isOwned: () => false,
+            canLaunch: () => false);
+
+        await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Capped);
+        await Assert.That(viewer.Starts).IsEqualTo(0);
+    }
+
+    /// <summary>
+    /// A slot is spent on a window, not on a pair. So the cap is asked only once the ownership
+    /// probe has said there is no window - otherwise the nineteen callers that find the one their
+    /// sibling started would each be charged for it, and a run of twenty failing snapshots would
+    /// exhaust any cap and strand its pairs.
+    /// </summary>
+    [Test]
+    public async Task ForwardingToARunningViewerSpendsNoSlot()
+    {
+        var viewer = new FakeViewer();
+        var asked = 0;
+
+        var outcome = ViewerLaunchGate.Launch(
+            retry: () => true,
+            launch: viewer.Start,
+            isOwned: () => true,
+            canLaunch: () =>
+            {
+                asked++;
+                return true;
+            });
+
+        await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Taken);
+        await Assert.That(viewer.Starts).IsEqualTo(0);
+        await Assert.That(asked).IsEqualTo(0);
+    }
+
+    /// <summary>
+    /// The real cap, so the default the call sites rely on is not only ever exercised through a
+    /// stand-in.
+    /// </summary>
+    [Test]
+    public async Task TheDefaultSlotCheckReadsMaxInstance()
+    {
+        var viewer = new FakeViewer();
+        try
+        {
+            DiffRunner.MaxInstancesToLaunch(0);
+            MaxInstance.ResetCount();
+
+            var outcome = ViewerLaunchGate.Launch(
+                retry: () => true,
+                launch: viewer.Start,
+                isOwned: () => false);
+
+            await Assert.That(outcome).IsEqualTo(ViewerLaunchOutcome.Capped);
+            await Assert.That(viewer.Starts).IsEqualTo(0);
+        }
+        finally
+        {
+            MaxInstance.ResetAppDomainValue();
+            MaxInstance.ResetCount();
+        }
+    }
+
     /// <summary>
     /// The real probe, against a real bound port, so the default the call sites rely on is not
     /// only ever exercised through a stand-in.

src/DiffEngine/DiffRunner.cs

@@ -216,8 +216,11 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile,
         }
 
         // The viewer queues rather than opening a window per pair, so none of the process
-        // bookkeeping below applies to it: there is no instance showing this pair to find, no
-        // window to replace, and no slot to spend on a window that already exists.
+        // bookkeeping below applies to it: there is no instance showing this pair to find, and no
+        // window to replace. The cap still does, but only on a viewer that has to be started -
+        // handing a pair to one already on screen opens nothing. ViewerLaunchGate is the only
+        // place that knows which of the two is happening, so it charges MaxInstance rather than
+        // this method.
         if (PendingFiles.IsViewer(tool))
         {
             return PendingFiles.AddDiff(tool, tempFile, targetFile);

src/DiffEngine/Tray/PendingFiles.cs

@@ -130,12 +130,17 @@ public static LaunchResult AddDiff(ResolvedTool tool, string tempFile, string ta
     /// A launch that turned out not to be one is not reported as one. Twenty pairs failing at once
     /// put twenty callers on the gate and one viewer on the screen, and calling that twenty new
     /// instances is how the count stopped meaning anything.
+    /// <para>
+    /// A capped one reports what every other tool's does, rather than being folded in with a tool
+    /// that could not be found: the pair has a tool and the cap is why no window opened.
+    /// </para>
     /// </summary>
     static LaunchResult Launched(ViewerLaunchOutcome outcome) =>
         outcome switch
         {
             ViewerLaunchOutcome.Launched => LaunchResult.StartedNewInstance,
             ViewerLaunchOutcome.Taken => LaunchResult.AlreadyRunningAndSupportsRefresh,
+            ViewerLaunchOutcome.Capped => LaunchResult.TooManyRunningDiffTools,
             _ => LaunchResult.NoDiffToolFound
         };
 

src/DiffEngine/Viewer/ViewerLaunchGate.cs

@@ -19,7 +19,13 @@ enum ViewerLaunchOutcome
     /// <summary>
     /// Nothing could be started, and nobody was there to take it.
     /// </summary>
-    Failed
+    Failed,
+
+    /// <summary>
+    /// Nobody was there to take it and <see cref="MaxInstance" /> had no slot left, so nothing was
+    /// started.
+    /// </summary>
+    Capped
 }
 
 /// <summary>
@@ -51,6 +57,14 @@ enum ViewerLaunchOutcome
 /// cross process wait on the failing path of every run to save a handful of starts in the rarer
 /// arrangement.
 /// </para>
+/// <para>
+/// The gate is also where <see cref="MaxInstance" /> is charged for a viewer, because it is the one
+/// place that knows whether a window is about to be opened. Handing a pair to a viewer that is
+/// already up is not a new instance and spends nothing, which is why the caller cannot ask: it
+/// would charge all twenty of the callers above for the one window between them. Asked after the
+/// ownership probe, so the nineteen that find an owner still forward their work when the cap is
+/// long since reached.
+/// </para>
 /// </summary>
 static class ViewerLaunchGate
 {
@@ -75,9 +89,19 @@ static class ViewerLaunchGate
     /// twenty concurrent connects to a port nothing is listening on and read the answer back out
     /// of the operating system.
     /// </param>
-    public static ViewerLaunchOutcome Launch(Func<bool> retry, Func<bool> launch, Func<bool>? isOwned = null)
+    /// <param name="canLaunch">
+    /// Whether a slot is available, and spends one when it is. Defaults to <see cref="MaxInstance" />.
+    /// Supplied by the tests that are about the gate rather than about the cap, since the count it
+    /// reads is shared with every other launch the process has made.
+    /// </param>
+    public static ViewerLaunchOutcome Launch(
+        Func<bool> retry,
+        Func<bool> launch,
+        Func<bool>? isOwned = null,
+        Func<bool>? canLaunch = null)
     {
         isOwned ??= () => ViewerClient.IsOwned();
+        canLaunch ??= () => !MaxInstance.Reached();
         bool owned;
         gate.Wait();
         try
@@ -87,6 +111,11 @@ public static ViewerLaunchOutcome Launch(Func<bool> retry, Func<bool> launch, Fu
             owned = isOwned();
             if (!owned)
             {
+                if (!canLaunch())
+                {
+                    return ViewerLaunchOutcome.Capped;
+                }
+
                 if (!launch())
                 {
                     return ViewerLaunchOutcome.Failed;
@@ -113,16 +142,23 @@ public static async Task<ViewerLaunchOutcome> LaunchAsync(
         Func<Task<bool>> retry,
         Func<Task<bool>> launch,
         Cancel cancel,
-        Func<bool>? isOwned = null)
+        Func<bool>? isOwned = null,
+        Func<bool>? canLaunch = null)
     {
         isOwned ??= () => ViewerClient.IsOwned();
+        canLaunch ??= () => !MaxInstance.Reached();
         bool owned;
         await gate.WaitAsync(cancel);
         try
         {
             owned = isOwned();
             if (!owned)
             {
+                if (!canLaunch())
+                {
+                    return ViewerLaunchOutcome.Capped;
+                }
+
                 if (!await launch())
                 {
                     return ViewerLaunchOutcome.Failed;