Let an explicit MaxInstancesToLaunch beat DiffEngine_MaxInstances

src/DiffEngine.Tests/MaxInstancePrecedenceTests.cs

@@ -0,0 +1,62 @@
+/// <summary>
+/// DiffEngine_MaxInstances was read ahead of the app domain value, and it persists per user -
+/// DiffEngineTray writes it to the user environment on every options save. So on a machine that
+/// had ever saved Options, <see cref="DiffRunner.MaxInstancesToLaunch" /> silently did nothing,
+/// and a test calling it to keep diff windows shut still had them open.
+/// </summary>
+[NotInParallel]
+public class MaxInstancePrecedenceTests
+{
+    const string variable = "DiffEngine_MaxInstances";
+
+    [Test]
+    public async Task Setting_it_beats_the_environment()
+    {
+        Environment.SetEnvironmentVariable(variable, "10");
+        MaxInstance.ResetAppDomainValue();
+
+        DiffRunner.MaxInstancesToLaunch(0);
+
+        await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(0);
+    }
+
+    /// <summary>
+    /// The environment is still what an app domain that sets nothing gets, which is how the tray
+    /// and DiffEngine_MaxInstances go on working.
+    /// </summary>
+    [Test]
+    public async Task The_environment_is_the_default_when_nothing_sets_it()
+    {
+        Environment.SetEnvironmentVariable(variable, "10");
+        MaxInstance.ResetAppDomainValue();
+
+        await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(10);
+    }
+
+    /// <summary>
+    /// A user scope save is the later explicit set, so it takes the value back off the app domain
+    /// rather than losing to it. This is what the tray does on an options save.
+    /// </summary>
+    [Test]
+    public async Task A_user_set_after_an_app_domain_set_wins()
+    {
+        // Not 3, so the save is a change and SetForUser does not skip it. Whatever this machine
+        // happens to have set is not allowed to decide that.
+        Environment.SetEnvironmentVariable(variable, "10");
+        DiffRunner.MaxInstancesToLaunch(0);
+
+        MaxInstance.SetForUser(3);
+
+        await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(3);
+    }
+
+    string? original = Environment.GetEnvironmentVariable(variable);
+
+    [After(Test)]
+    public void Restore()
+    {
+        Environment.SetEnvironmentVariable(variable, original);
+        MaxInstance.ResetAppDomainValue();
+        MaxInstance.ResetCount();
+    }
+}

src/DiffEngine.Tests/MaxInstanceReplacementTests.cs

@@ -47,14 +47,11 @@ public async Task ADifferentPairStillHitsTheLimit()
     }
 
     /// <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.
+    /// The app domain value beats DiffEngine_MaxInstances, which this machine may well have set -
+    /// it persists per user - so this is all it takes to pin the limit for the test.
     /// </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();
     }
@@ -105,8 +102,7 @@ string Write(string name)
 
     public void Dispose()
     {
-        Environment.SetEnvironmentVariable(variable, original);
-        DiffRunner.MaxInstancesToLaunch(5);
+        MaxInstance.ResetAppDomainValue();
         MaxInstance.ResetCount();
         try
         {
@@ -122,8 +118,6 @@ public void Dispose()
 
     // 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;

src/DiffEngine.Tests/MaxInstanceUserWriteTests.cs

@@ -0,0 +1,108 @@
+/// <summary>
+/// The tray persisted DiffEngine_MaxInstances on every settings save, whether or not the limit was
+/// part of what changed - the tray's SettingsHelper.Write calls SetForUser unconditionally, and
+/// the value it passes is whatever the options form was populated with, which is the value already
+/// in effect. Saves of unrelated settings go through the same path, including the "always kill
+/// locking processes" prompt, which has no options form at all. So opening the tray once was enough
+/// to leave a user scope variable behind on a machine that had never chosen a limit.
+/// </summary>
+[NotInParallel]
+public class MaxInstanceUserWriteTests
+{
+    const string variable = "DiffEngine_MaxInstances";
+
+    /// <summary>
+    /// The case that put the variable on machines that never asked for it: nothing set, so the
+    /// form opens at the default and saves it straight back.
+    /// </summary>
+    [Test]
+    public async Task Saving_the_default_on_a_machine_with_nothing_set_writes_nothing()
+    {
+        Environment.SetEnvironmentVariable(variable, null);
+        MaxInstance.ResetAppDomainValue();
+
+        MaxInstance.SetForUser(MaxInstance.MaxInstancesToLaunch);
+
+        await Assert.That(Environment.GetEnvironmentVariable(variable)).IsNull();
+    }
+
+    [Test]
+    public async Task Saving_the_value_already_set_writes_nothing()
+    {
+        Environment.SetEnvironmentVariable(variable, "10");
+        MaxInstance.ResetAppDomainValue();
+
+        MaxInstance.SetForUser(10);
+
+        await Assert.That(Environment.GetEnvironmentVariable(variable)).IsEqualTo("10");
+        await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(10);
+    }
+
+    /// <summary>
+    /// Choosing the default is a way back to an unset machine, rather than a pin at whatever the
+    /// default happens to be today. Same as SetTargetOnLeft with false.
+    /// </summary>
+    [Test]
+    public async Task Saving_the_default_clears_a_value_that_was_set()
+    {
+        var @default = Default();
+        Environment.SetEnvironmentVariable(variable, "10");
+        MaxInstance.ResetAppDomainValue();
+
+        MaxInstance.SetForUser(@default);
+
+        await Assert.That(Environment.GetEnvironmentVariable(variable)).IsNull();
+        await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(@default);
+    }
+
+    /// <summary>
+    /// Including a value that was pinned at the default explicitly - it says nothing the absent
+    /// variable does not, and leaving it would keep the machine pinned.
+    /// </summary>
+    [Test]
+    public async Task Saving_the_default_over_a_redundant_pin_clears_it()
+    {
+        var @default = Default();
+        Environment.SetEnvironmentVariable(variable, @default.ToString());
+        MaxInstance.ResetAppDomainValue();
+
+        MaxInstance.SetForUser(@default);
+
+        await Assert.That(Environment.GetEnvironmentVariable(variable)).IsNull();
+    }
+
+    /// <summary>
+    /// What the limit is with nothing set, which is what defaultMax is, without reaching into it.
+    /// </summary>
+    static int Default()
+    {
+        Environment.SetEnvironmentVariable(variable, null);
+        MaxInstance.ResetAppDomainValue();
+        return MaxInstance.MaxInstancesToLaunch;
+    }
+
+    /// <summary>
+    /// And a save that does change the limit still lands, which is the point of the setting.
+    /// </summary>
+    [Test]
+    public async Task Saving_a_different_value_writes_it()
+    {
+        Environment.SetEnvironmentVariable(variable, "10");
+        MaxInstance.ResetAppDomainValue();
+
+        MaxInstance.SetForUser(7);
+
+        await Assert.That(Environment.GetEnvironmentVariable(variable)).IsEqualTo("7");
+        await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(7);
+    }
+
+    string? original = Environment.GetEnvironmentVariable(variable);
+
+    [After(Test)]
+    public void Restore()
+    {
+        Environment.SetEnvironmentVariable(variable, original);
+        MaxInstance.ResetAppDomainValue();
+        MaxInstance.ResetCount();
+    }
+}

src/DiffEngine.Tests/ModuleInitializer.cs

@@ -9,9 +9,18 @@ public static void Initialize()
         FileExtensions.AddTextFileConvention(_ => _.EndsWith(".txtConvention".AsSpan()));
         Logging.Enable();
         DiffRunner.Disabled = false;
+        KeepEnvironmentWritesInProcess();
         DetachFromPendingFileSurfaces();
     }
 
+    /// <summary>
+    /// Tests must not write the user environment of the machine running them. The test projects
+    /// run as parallel processes over the one registry key, so a capture in one and a restore in
+    /// the other race, and the value that loses is gone.
+    /// </summary>
+    static void KeepEnvironmentWritesInProcess() =>
+        EnvironmentHelper.Set = EnvironmentHelper.SetProcessOnly;
+
     /// <summary>
     /// Launching sends a real pending move to whatever owns the queue on this machine. On a
     /// developer box that is the tray, started at login, and an accept or discard from it kills the

src/DiffEngine/EnvironmentHelper.cs

@@ -1,8 +1,25 @@
 static class EnvironmentHelper
 {
-    public static void Set(string name, string? value)
+    /// <summary>
+    /// Both scopes, so a setting chosen in the tray outlives the process that chose it.
+    /// <para>
+    /// Swapped for <see cref="SetProcessOnly" /> by the test projects. A test would otherwise
+    /// write the user environment of the machine running it, and cannot put it back reliably: the
+    /// test projects run as parallel processes over the one registry key, so a capture in one and
+    /// a restore in the other race, and the value that loses is gone.
+    /// </para>
+    /// </summary>
+    internal static Action<string, string?> Set = SetUserAndProcess;
+
+    static void SetUserAndProcess(string name, string? value)
     {
         Environment.SetEnvironmentVariable(name, value, EnvironmentVariableTarget.User);
         Environment.SetEnvironmentVariable(name, value, EnvironmentVariableTarget.Process);
     }
-}
+
+    /// <summary>
+    /// Nothing outside the process. For tests.
+    /// </summary>
+    internal static void SetProcessOnly(string name, string? value) =>
+        Environment.SetEnvironmentVariable(name, value, EnvironmentVariableTarget.Process);
+}

src/DiffEngine/MaxInstance.cs

@@ -7,9 +7,20 @@ static class MaxInstance
     static int launchedInstances;
     const int defaultMax = 5;
 
+    /// <summary>
+    /// An explicit set wins over the environment, which is only the ambient default for a run that
+    /// sets nothing.
+    /// <para>
+    /// DiffEngine_MaxInstances persists per user - DiffEngineTray writes it to the user
+    /// environment on every options save - so reading it first meant that on a machine which had
+    /// ever saved Options, <see cref="DiffRunner.MaxInstancesToLaunch" /> silently did nothing. A
+    /// test suppressing diff windows with it still opened them. Same order as
+    /// <see cref="DiffRunner.Disabled" />, where an explicit set also pins the value.
+    /// </para>
+    /// </summary>
     static int GetMaxInstances() =>
-        GetEnvironmentValue() ??
         appDomainMaxInstancesToLaunch ??
+        GetEnvironmentValue() ??
         defaultMax;
 
     static int? GetEnvironmentValue()
@@ -38,10 +49,61 @@ public static void SetForAppDomain(int value)
         ResetCapturedValue();
     }
 
+    /// <summary>
+    /// Persists a user scope value, as <see cref="TargetPosition.SetTargetOnLeft" /> does for
+    /// DiffEngine_TargetOnLeft.
+    /// <para>
+    /// A save that changes nothing writes nothing. The tray calls this for every settings save -
+    /// including saves of unrelated settings, like the "always kill locking processes" prompt -
+    /// and the value it passes is whatever the options form was populated with. So a machine that
+    /// had never chosen a limit still ended up with DiffEngine_MaxInstances in its user
+    /// environment, at the value that was already in effect.
+    /// </para>
+    /// <para>
+    /// And choosing the default clears the variable rather than persisting it, so there is a way
+    /// back to an unset machine through the options form.
+    /// </para>
+    /// </summary>
     public static void SetForUser(int value)
     {
         Guard.AgainstNegative(value, nameof(value));
-        EnvironmentHelper.Set("DiffEngine_MaxInstances", value.ToString());
+
+        // The default needs nothing persisted to mean what it means, so returning to it takes the
+        // variable back out rather than pinning it at what the default happens to be today.
+        string? desired;
+        if (value == defaultMax)
+        {
+            desired = null;
+        }
+        else
+        {
+            desired = value.ToString();
+        }
+
+        // Only when what is persisted would actually change. Read raw rather than through
+        // GetEnvironmentValue, because this is a comparison against the stored text, and because
+        // a setter is no place to throw over an existing unparseable value it is about to
+        // overwrite anyway.
+        if (Environment.GetEnvironmentVariable("DiffEngine_MaxInstances") == desired)
+        {
+            return;
+        }
+
+        EnvironmentHelper.Set("DiffEngine_MaxInstances", desired);
+        // The later explicit set is the one that counts, so an app domain value from earlier in
+        // the process cannot shadow what the user just chose.
+        appDomainMaxInstancesToLaunch = null;
+        ResetCapturedValue();
+    }
+
+    /// <summary>
+    /// Forgets an explicit <see cref="SetForAppDomain" />, so the value is read from the
+    /// environment again. For tests, which is where anything sets it and then wants the ambient
+    /// value back.
+    /// </summary>
+    internal static void ResetAppDomainValue()
+    {
+        appDomainMaxInstancesToLaunch = null;
         ResetCapturedValue();
     }
 
@@ -57,4 +119,4 @@ public static bool Reached()
         var count = Interlocked.Increment(ref launchedInstances);
         return count > MaxInstancesToLaunch;
     }
-}
+}

src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs

@@ -8,17 +8,14 @@ public class DiffRunnerCanKillTest :
 {
     string tempFile = Path.GetTempFileName();
     bool originalDisabled = DiffRunner.Disabled;
-    string? originalMaxInstances = Environment.GetEnvironmentVariable("DiffEngine_MaxInstances");
 
     public DiffRunnerCanKillTest()
     {
         PiperClient.Port = GetFreePort();
         DiffEngine.DiffEngineTray.IsRunning = true;
         DiffRunner.Disabled = false;
         // Force the "too many running" branch so no real process is launched, while a move
-        // payload is still sent to the tray. The env var takes precedence over the app-domain
-        // value, so set it too; MaxInstancesToLaunch resets the cached lookup.
-        Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", "0");
+        // payload is still sent to the tray.
         DiffRunner.MaxInstancesToLaunch(0);
     }
 
@@ -107,8 +104,7 @@ public void Dispose()
     {
         DiffEngine.DiffEngineTray.IsRunning = false;
         DiffRunner.Disabled = originalDisabled;
-        Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", originalMaxInstances);
-        DiffRunner.MaxInstancesToLaunch(5);
+        MaxInstance.ResetAppDomainValue();
         File.Delete(tempFile);
     }
 }

src/DiffEngineTray.Tests/ModuleInitializer.cs

@@ -5,9 +5,18 @@ public static void Initialize()
     {
         VerifyWinForms.Initialize();
         VerifierSettings.UseSsimForPng(PngSsimThreshold);
+        KeepEnvironmentWritesInProcess();
         PointAtAClosedPort();
     }
 
+    /// <summary>
+    /// Tests must not write the user environment of the machine running them. The test projects
+    /// run as parallel processes over the one registry key, so a capture in one and a restore in
+    /// the other race, and the value that loses is gone.
+    /// </summary>
+    static void KeepEnvironmentWritesInProcess() =>
+        EnvironmentHelper.Set = EnvironmentHelper.SetProcessOnly;
+
     /// <summary>
     /// Effectively "the same pixels", rather than Verify's 0.98 default. These screens are mostly
     /// flat background, so 0.98 is far looser than it sounds on them: a whole missing row of text