diff --git a/claude.md b/claude.md index 8c5d1bdf..9e8677c1 100644 --- a/claude.md +++ b/claude.md @@ -266,8 +266,10 @@ apart. - Unless that diff tool is the viewer, which is the `Diff` verb and `--diff `. 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. diff --git a/docs/diff-tool.md b/docs/diff-tool.md index 112a4d4c..faeff535 100644 --- a/docs/diff-tool.md +++ b/docs/diff-tool.md @@ -48,7 +48,9 @@ This allows, in most cases, for no manual closing of the tool to be required. + /// 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. + /// + /// 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. + /// + /// + [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(); + } + } + /// /// 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, diff --git a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs index 0abf47ed..f3eae6f7 100644 --- a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs +++ b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs @@ -1,4 +1,4 @@ -/// +/// /// The gate that keeps a parallel run from starting a viewer per failing snapshot. /// /// 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); } + /// + /// 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. + /// + [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); + } + + /// + [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); + } + + /// + /// 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. + /// + [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); + } + + /// + /// The real cap, so the default the call sites rely on is not only ever exercised through a + /// stand-in. + /// + [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(); + } + } + /// /// 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. diff --git a/src/DiffEngine/DiffRunner.cs b/src/DiffEngine/DiffRunner.cs index d2eca433..c415cc62 100644 --- a/src/DiffEngine/DiffRunner.cs +++ b/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); diff --git a/src/DiffEngine/Tray/PendingFiles.cs b/src/DiffEngine/Tray/PendingFiles.cs index 76067c92..028ce7bd 100644 --- a/src/DiffEngine/Tray/PendingFiles.cs +++ b/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. + /// + /// 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. + /// /// static LaunchResult Launched(ViewerLaunchOutcome outcome) => outcome switch { ViewerLaunchOutcome.Launched => LaunchResult.StartedNewInstance, ViewerLaunchOutcome.Taken => LaunchResult.AlreadyRunningAndSupportsRefresh, + ViewerLaunchOutcome.Capped => LaunchResult.TooManyRunningDiffTools, _ => LaunchResult.NoDiffToolFound }; diff --git a/src/DiffEngine/Viewer/ViewerLaunchGate.cs b/src/DiffEngine/Viewer/ViewerLaunchGate.cs index eddbf259..7da68b08 100644 --- a/src/DiffEngine/Viewer/ViewerLaunchGate.cs +++ b/src/DiffEngine/Viewer/ViewerLaunchGate.cs @@ -19,7 +19,13 @@ enum ViewerLaunchOutcome /// /// Nothing could be started, and nobody was there to take it. /// - Failed + Failed, + + /// + /// Nobody was there to take it and had no slot left, so nothing was + /// started. + /// + Capped } /// @@ -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. /// +/// +/// The gate is also where 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. +/// /// 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. /// - public static ViewerLaunchOutcome Launch(Func retry, Func launch, Func? isOwned = null) + /// + /// Whether a slot is available, and spends one when it is. Defaults to . + /// 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. + /// + public static ViewerLaunchOutcome Launch( + Func retry, + Func launch, + Func? isOwned = null, + Func? canLaunch = null) { isOwned ??= () => ViewerClient.IsOwned(); + canLaunch ??= () => !MaxInstance.Reached(); bool owned; gate.Wait(); try @@ -87,6 +111,11 @@ public static ViewerLaunchOutcome Launch(Func retry, Func launch, Fu owned = isOwned(); if (!owned) { + if (!canLaunch()) + { + return ViewerLaunchOutcome.Capped; + } + if (!launch()) { return ViewerLaunchOutcome.Failed; @@ -113,9 +142,11 @@ public static async Task LaunchAsync( Func> retry, Func> launch, Cancel cancel, - Func? isOwned = null) + Func? isOwned = null, + Func? canLaunch = null) { isOwned ??= () => ViewerClient.IsOwned(); + canLaunch ??= () => !MaxInstance.Reached(); bool owned; await gate.WaitAsync(cancel); try @@ -123,6 +154,11 @@ public static async Task LaunchAsync( owned = isOwned(); if (!owned) { + if (!canLaunch()) + { + return ViewerLaunchOutcome.Capped; + } + if (!await launch()) { return ViewerLaunchOutcome.Failed;