Let an explicit MaxInstancesToLaunch beat DiffEngine_MaxInstances (#852)

DiffEngine_MaxInstances was read ahead of the app domain value, and it
persists per user - the tray writes it to the user environment on every
settings save. So on a machine that had ever saved tray settings,
DiffRunner.MaxInstancesToLaunch silently did nothing, and a test calling
it to keep diff windows shut still had them open.

Read the app domain value first, as DiffRunner.Disabled already does for
an explicit set, leaving the environment as the ambient default for a run
that sets nothing.

Two things behind that, in SetForUser:

- A save that changes nothing writes nothing. Write is handed whatever
  the options form was populated with, which is the value already in
  effect, and unrelated saves come through the same path - the "always
  kill locking processes" prompt persists itself that way and has no
  options form at all. Opening the tray once was enough to leave a user
  scope variable behind on a machine that had never chosen a limit.
- Choosing the default clears the variable rather than persisting it, as
  SetTargetOnLeft does with false, so there is a way back to an unset
  machine through the options form.

Tests wrote the user environment of the machine running them, and could
not 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. EnvironmentHelper.Set is now swappable, and both test module
initializers keep those writes inside the process.

Author: SimonCropp

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