Give each test run its own temp directory (BL-16664)

AGENTS.md

@@ -66,6 +66,27 @@ multiple terminals can build/test at once. See `Directory.Build.props` for how i
 - The first build in a fresh terminal is a full (cold) build into that terminal's private
   tree; subsequent builds there are incremental. `output/` is gitignored.
 
+### Temp folders are isolated per test run too
+
+The build tree is not the only thing two concurrent runs would otherwise share. Our tests name
+their scratch folders after themselves (`new TemporaryFolder("SomeFixtureTests")`), which are
+machine-global paths, and `TemporaryFolder` **deletes** an existing folder of that name before
+creating it — so one run's setup would delete another run's in-flight folder.
+
+`src/BloomTests/TestTempDirectory.cs` prevents that: before any fixture runs, it points this
+process's temp directory at `%TEMP%\BloomTests\<key>-p<pid>\`. You therefore do **not** need to
+invent unique folder names in tests — keep naming a temp folder after your fixture, and it is
+already scoped to the run. It also means production code writing to temp while under test is
+isolated as well.
+
+Two consequences worth knowing:
+
+- **After a failing run the folder is kept**, so you can look at what the failing test wrote; the
+  path is printed at the end of the run. Passing runs delete theirs, and anything older than a day
+  is cleared by the next run.
+- Every temp path is longer by `BloomTests\<key>-p<pid>\`. Deeply-nested temp paths in tests are
+  that much closer to `MAX_PATH`.
+
 ## Building / testing the front-end (web UI) while Bloom is running
 
 The developer usually launches Bloom with `./go.sh`, which starts a **Vite dev server** and

src/BloomTests/TestTempDirectory.cs

@@ -0,0 +1,137 @@
+using System;
+using System.IO;
+using BloomTemp;
+using NUnit.Framework;
+using NUnit.Framework.Interfaces;
+
+/// <summary>
+/// Gives this test process a temporary directory of its own, so that two test runs on the same
+/// machine cannot tread on each other's scratch folders.
+///
+/// Our tests name their temp folders after themselves — there are around 180 calls of the form
+/// `new TemporaryFolder("SomeFixtureName")` — and those names resolve to machine-global paths.
+/// That was harmless when one person ran the suite at a time. It is not harmless now that agents
+/// work in several worktrees at once, because TemporaryFolder's constructor *deletes* any
+/// existing folder of the name before creating it (see TemporaryFolder in
+/// src/BloomExe/TempFiles.cs). So one run's setup would quietly delete another run's in-flight
+/// folder, and the victim would fail somewhere unrelated, naming a folder it had never heard of.
+/// See BL-16664, and BL-16661 for the same shape of failure from a different cause.
+///
+/// Rather than rename 180 call sites — which would still leave Bloom's own production code
+/// writing to the shared temp directory while under test — we move the whole process's idea of
+/// where "temp" is. Every existing call site then keeps its familiar name, but the name is scoped
+/// to this run.
+/// </summary>
+/// <remarks>
+/// This deliberately sits in the global namespace. An NUnit [SetUpFixture] applies to the
+/// namespace it is declared in and the namespaces beneath it; nearly all our tests are under
+/// BloomTests, but not quite all (there is a fixture in the Bloom.Api namespace), and the
+/// redirect has to be in place before *any* fixture runs. The global namespace covers the whole
+/// assembly and is ordered ahead of the BloomTests one.
+/// </remarks>
+[SetUpFixture]
+public class TestTempDirectory
+{
+    /// <summary>Everything this assembly writes to temp goes under here, a folder per run.</summary>
+    private const string kContainerName = "BloomTests";
+
+    /// <summary>How long a leftover run folder must sit untouched before another run clears it.</summary>
+    private static readonly TimeSpan kStaleAfter = TimeSpan.FromDays(1);
+
+    private static string _runFolder;
+
+    /// <summary>
+    /// The machine-wide temp directory, as it was before we redirected. Kept so the tests for
+    /// this class can check that we really did move somewhere else.
+    /// </summary>
+    internal static string MachineTempFolder { get; private set; }
+
+    /// <summary>The folder this run's temporary files live in.</summary>
+    internal static string RunFolder => _runFolder;
+
+    [OneTimeSetUp]
+    public void RedirectTempToAFolderOfOurOwn()
+    {
+        MachineTempFolder = Path.GetTempPath();
+        var container = Path.Combine(MachineTempFolder, kContainerName);
+        _runFolder = Path.Combine(container, KeyForThisRun());
+        Directory.CreateDirectory(_runFolder);
+
+        // Path.GetTempPath() is defined in terms of these, so from this point on every temp path
+        // the process computes — ours and Bloom's own — lands inside _runFolder.
+        Environment.SetEnvironmentVariable("TMP", _runFolder);
+        Environment.SetEnvironmentVariable("TEMP", _runFolder);
+
+        RemoveFoldersLeftByRunsThatDiedBeforeCleaningUp(container);
+    }
+
+    [OneTimeTearDown]
+    public void RemoveOurTempFolderUnlessSomethingFailed()
+    {
+        // When tests failed, leave the folder alone. What a failing test wrote is often the
+        // evidence you need, and this suite's nastiest bugs have been about temp folders
+        // appearing and disappearing — deleting the scene of the crime would be perverse.
+        // Whatever we leave behind is cleared by a later run once it goes stale.
+        if (TestContext.CurrentContext.Result.Outcome.Status == TestStatus.Failed)
+        {
+            TestContext.Progress.WriteLine(
+                $"Tests failed, so their temporary files have been left in {_runFolder}"
+            );
+            return;
+        }
+
+        // Failing silently is deliberate: a file some test left open must not turn a green run
+        // red at the very last moment.
+        TemporaryFolder.DeleteFolderThatMayBeInUseAndIfNotFailSilently(_runFolder);
+    }
+
+    /// <summary>
+    /// A name for this run's folder: unique, so no two runs can ever share one, but still
+    /// recognizable, so you can tell which terminal a leftover folder came from.
+    /// </summary>
+    private static string KeyForThisRun()
+    {
+        // build/agent-dotnet.sh gives each terminal its own build tree at output/agent/<key> and
+        // passes the path down in this variable, which the test host inherits. Naming ourselves
+        // after the same key means a temp folder can be matched to the build tree beside it.
+        // It is truncated because every temp path in the run grows by whatever we choose here,
+        // and the key is normally a 36-character session id.
+        var key = "";
+        var buildDir = Environment.GetEnvironmentVariable("BLOOM_AGENT_BUILD_DIR");
+        if (!string.IsNullOrEmpty(buildDir))
+        {
+            var name = new DirectoryInfo(buildDir.TrimEnd('/', '\\')).Name;
+            key = name.Length > 8 ? name.Substring(0, 8) : name;
+        }
+
+        // The process id is what actually guarantees uniqueness. The key above is only a label,
+        // and two terminals could in principle share its first eight characters.
+        return string.IsNullOrEmpty(key)
+            ? $"p{Environment.ProcessId}"
+            : $"{key}-p{Environment.ProcessId}";
+    }
+
+    /// <summary>
+    /// Clear out run folders old enough that nothing can still be using them. Runs that crash,
+    /// or that fail and so deliberately keep their files, would otherwise accumulate forever.
+    /// </summary>
+    private static void RemoveFoldersLeftByRunsThatDiedBeforeCleaningUp(string container)
+    {
+        try
+        {
+            foreach (var folder in Directory.GetDirectories(container))
+            {
+                if (folder == _runFolder)
+                    continue;
+                if (DateTime.UtcNow - Directory.GetLastWriteTimeUtc(folder) < kStaleAfter)
+                    continue;
+                TemporaryFolder.DeleteFolderThatMayBeInUseAndIfNotFailSilently(folder);
+            }
+        }
+        catch (Exception)
+        {
+            // Housekeeping only. If we cannot read the container — another run is busy in it,
+            // a permission oddity — that is no reason to stop the test run before it starts.
+        }
+    }
+}

src/BloomTests/TestTempDirectoryTests.cs

@@ -0,0 +1,65 @@
+using System.IO;
+using BloomTemp;
+using NUnit.Framework;
+
+namespace BloomTests
+{
+    /// <summary>
+    /// Checks that <see cref="TestTempDirectory"/> really did move this process's temp directory
+    /// before any fixture ran. If these fail, tests are once again writing to machine-global
+    /// paths and two concurrent runs can delete each other's folders (BL-16664).
+    /// </summary>
+    public class TestTempDirectoryTests
+    {
+        [Test]
+        public void TempPath_IsARunFolderOfOurOwn_NotTheMachineTempFolder()
+        {
+            // Sanity check: the fixture recorded where temp used to be, so there is something to
+            // compare against.
+            Assert.That(
+                TestTempDirectory.MachineTempFolder,
+                Is.Not.Null.And.Not.Empty,
+                "Setup sanity check: TestTempDirectory should have recorded the original temp folder."
+            );
+
+            var current = Normalize(Path.GetTempPath());
+
+            Assert.That(
+                current,
+                Is.Not.EqualTo(Normalize(TestTempDirectory.MachineTempFolder)),
+                "Tests must not write straight into the machine's temp folder, where another test run could delete what they make."
+            );
+            Assert.That(
+                current,
+                Is.EqualTo(Normalize(TestTempDirectory.RunFolder)),
+                "Path.GetTempPath() should now return this run's own folder."
+            );
+        }
+
+        /// <summary>
+        /// Path.GetTempPath() always ends in a directory separator and the paths we compare it
+        /// with do not, so strip it before comparing.
+        /// </summary>
+        private static string Normalize(string path)
+        {
+            return Path.TrimEndingDirectorySeparator(Path.GetFullPath(path));
+        }
+
+        [Test]
+        public void TemporaryFolderWithAFixedName_LandsInsideOurRunFolder()
+        {
+            // This is the point of the whole exercise: the ~180 existing calls that name a temp
+            // folder after their fixture are left exactly as they are, and are scoped to this run
+            // anyway. So use a fixed name here too, just as they do.
+            using (var folder = new TemporaryFolder("TestTempDirectoryTests_FixedName"))
+            {
+                Assert.That(Directory.Exists(folder.FolderPath), Is.True);
+                Assert.That(
+                    Path.GetFullPath(Path.GetDirectoryName(folder.FolderPath)),
+                    Is.EqualTo(Path.GetFullPath(TestTempDirectory.RunFolder)),
+                    "A plainly-named TemporaryFolder should be created inside this run's folder."
+                );
+            }
+        }
+    }
+}