diff --git a/AGENTS.md b/AGENTS.md index b7030316b9ee..20e545cbdc12 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,6 +66,34 @@ 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\-p\`. 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 on standard error at the end of the run. Passing runs delete theirs, and + anything older than a day is cleared by the next run. +- **If the folder cannot be deleted, the run says so** — again on standard error, naming one file + that is still open and the reason the OS gave, without failing the run. That normally means a + test finished without disposing something; worth chasing, because a leaked handle can make + later runs behave oddly. +- Note that standard error is the only channel `dotnet test` shows at its default verbosity — + `Console.Out`, `TestContext.Out` and `TestContext.Progress` are all swallowed. Use + `Console.Error` for anything a developer must see. +- Every temp path is longer by `BloomTests\-p\`. 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 diff --git a/src/BloomTests/PretendRequestInfo.cs b/src/BloomTests/PretendRequestInfo.cs index 9891e1db462f..90090b8d776a 100644 --- a/src/BloomTests/PretendRequestInfo.cs +++ b/src/BloomTests/PretendRequestInfo.cs @@ -5,9 +5,11 @@ using System.Collections.Specialized; using System.IO; using System.Text; +using Bloom; +using Bloom.Api; using SIL.IO; -namespace Bloom.Api +namespace BloomTests { public class PretendRequestInfo : IRequestInfo { diff --git a/src/BloomTests/TestTempDirectory.cs b/src/BloomTests/TestTempDirectory.cs new file mode 100644 index 000000000000..92cfdf5d1171 --- /dev/null +++ b/src/BloomTests/TestTempDirectory.cs @@ -0,0 +1,235 @@ +using System; +using System.IO; +using BloomTemp; +using NUnit.Framework; +using NUnit.Framework.Interfaces; + +namespace BloomTests +{ + /// + /// 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. + /// + /// + /// An NUnit [SetUpFixture] applies to the namespace it is declared in and the namespaces + /// beneath it, and the redirect has to be in place before *any* fixture runs. So if you ever + /// add a test fixture outside BloomTests, either bring it inside or this will not cover it. + /// + [SetUpFixture] + public class TestTempDirectory + { + /// Everything this assembly writes to temp goes under here, a folder per run. + private const string kContainerName = "BloomTests"; + + /// How long a leftover run folder must sit untouched before another run clears it. + private static readonly TimeSpan kStaleAfter = TimeSpan.FromDays(1); + + private static string _runFolder; + + /// + /// 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. + /// + internal static string MachineTempFolder { get; private set; } + + /// The folder this run's temporary files live in. + internal static string RunFolder => _runFolder; + + /// + /// Points this process's temp directory at a folder of our own, before any fixture runs, and + /// takes the opportunity to clear out folders left by runs that died. NUnit calls this once. + /// + [OneTimeSetUp] + public void RedirectTempToAFolderOfOurOwn() + { + MachineTempFolder = Path.GetTempPath(); + var container = Path.Combine(MachineTempFolder, kContainerName); + _runFolder = Path.Combine(container, KeyForThisRun()); + StartFolderEmpty(_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); + } + + /// + /// Deletes this run's temp folder — and with it everything the run put in temp, since every + /// temp path the process computed descends from it. Kept instead of deleted when tests + /// failed, so their files can be examined. NUnit calls this once, at the end of the run. + /// + [OneTimeTearDown] + public void RemoveOurTempFolderUnlessSomethingFailed() + { + // Point temp back at the machine's own folder before we go. Anything that runs after + // this -- NUnit's own shutdown, a background thread that outlives the tests -- would + // otherwise compute temp paths inside a directory we are about to delete, and fail + // confusingly at the very end of an otherwise good run. + Environment.SetEnvironmentVariable("TMP", MachineTempFolder); + Environment.SetEnvironmentVariable("TEMP", MachineTempFolder); + + // 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) + { + // Standard error, because it is the only channel `dotnet test` shows at its default + // verbosity; Console.Out, TestContext.Out and TestContext.Progress are all swallowed. + Console.Error.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); + + // But it should not be *silent* silent. If something is still holding a file, that is + // worth knowing: it usually means a test finished without disposing something, which is + // a small bug of its own and can make later runs behave oddly. + var whatIsLeft = DescribeWhyFolderCouldNotBeDeleted(_runFolder); + if (whatIsLeft != null) + { + Console.Error.WriteLine( + $"WARNING: could not delete this test run's temp folder, {_runFolder}. " + + "Something in the run probably did not release a file it opened. " + + whatIsLeft + ); + } + } + + /// + /// Describes what is still sitting in a folder we tried and failed to delete, naming one item + /// that will not open and the reason the operating system gave for it. Returns null when the + /// folder did in fact go, so the caller can use it as "is there anything to complain about?". + /// + internal static string DescribeWhyFolderCouldNotBeDeleted(string folder) + { + if (!Directory.Exists(folder)) + return null; + + string[] files; + try + { + files = Directory.GetFiles(folder, "*", SearchOption.AllDirectories); + } + catch (Exception e) + { + return $"Its contents could not even be listed: {e.Message}"; + } + + // Whatever refuses to open exclusively is almost always what is holding the folder, so + // report the first such file with whatever the OS said about it. Deliberately not + // claiming the file is "in use": the commonest cause is another handle on it, but a + // read-only file or a permissions problem lands here too, and the OS message is what + // tells the two apart. + foreach (var file in files) + { + try + { + using (File.Open(file, FileMode.Open, FileAccess.ReadWrite, FileShare.None)) { } + } + catch (Exception e) + { + return $"{files.Length} file(s) remain; this one could not be opened, which is usually what stops the delete: {file} -- {e.Message}"; + } + } + + if (files.Length > 0) + { + return $"{files.Length} file(s) remain, though each can be opened now, so whatever held them may have just let go. First of them: {files[0]}"; + } + + return "It contains no files, so something may be holding the folder itself -- a process " + + "using it as its current directory, for instance."; + } + + /// + /// Makes sure the run folder exists and is empty. + /// + /// Emptying matters because the name can repeat. A run that crashes leaves its folder behind + /// (and a failing run leaves it deliberately), those folders survive for a day, and Windows + /// recycles process ids — so this exact path may already exist, holding the remains of a run + /// that is long gone. Inheriting those would produce exactly the confusing, stale-file + /// failures this class exists to prevent. + /// + /// Deleting it is safe: a process id is unique among *running* processes, and this one is + /// ours, so nothing alive can be using the folder. + /// + internal static void StartFolderEmpty(string folder) + { + if (Directory.Exists(folder)) + TemporaryFolder.DeleteFolderThatMayBeInUseAndIfNotFailSilently(folder); + Directory.CreateDirectory(folder); + } + + /// + /// A name for this run's folder: unique among live runs, but not unique forever, since it is + /// built from a process id and those get recycled — see . Still + /// recognizable, so you can tell which terminal a leftover folder came from. + /// + private static string KeyForThisRun() + { + // build/agent-dotnet.sh gives each terminal its own build tree at output/agent/ 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}"; + } + + /// + /// 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. + /// + 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. + } + } + } +} diff --git a/src/BloomTests/TestTempDirectoryTests.cs b/src/BloomTests/TestTempDirectoryTests.cs new file mode 100644 index 000000000000..35885492f353 --- /dev/null +++ b/src/BloomTests/TestTempDirectoryTests.cs @@ -0,0 +1,196 @@ +using System.IO; +using BloomTemp; +using NUnit.Framework; + +namespace BloomTests +{ + /// + /// Checks that 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). + /// + public class TestTempDirectoryTests + { + /// + /// The redirect happened at all: this process's temp directory is our own run folder + /// rather than the machine-wide one that other runs also write to. + /// + [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." + ); + } + + /// + /// Path.GetTempPath() always ends in a directory separator and the paths we compare it + /// with do not, so strip it before comparing. + /// + private static string Normalize(string path) + { + return Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); + } + + /// + /// A run folder's name is built from a process id, and Windows recycles those, so a run + /// can be handed the path of a crashed earlier run whose files are still sitting there. + /// Starting from whatever that run left behind would cause exactly the stale-file + /// confusion this class exists to prevent, so the folder must be emptied, not just made. + /// + [Test] + public void StartFolderEmpty_FolderAlreadyHasFilesInIt_ClearsThem() + { + using (var parent = new TemporaryFolder("StartFolderEmptyTests")) + { + var reusedPath = Path.Combine(parent.FolderPath, "a7920d49-p12345"); + Directory.CreateDirectory(Path.Combine(reusedPath, "leftover subfolder")); + File.WriteAllText( + Path.Combine(reusedPath, "leftover subfolder", "stale.txt"), + "from a run that died" + ); + File.WriteAllText(Path.Combine(reusedPath, "also-stale.txt"), "likewise"); + + // Sanity check: the setup really did leave something behind to be cleared. + Assert.That( + Directory.GetFiles(reusedPath, "*", SearchOption.AllDirectories).Length, + Is.EqualTo(2), + "Setup sanity check: two stale files should be sitting in the reused folder." + ); + + TestTempDirectory.StartFolderEmpty(reusedPath); + + Assert.That( + Directory.Exists(reusedPath), + Is.True, + "The folder should still be there, ready to be used." + ); + Assert.That( + Directory.GetFileSystemEntries(reusedPath), + Is.Empty, + "Nothing from the previous run should have survived into this one." + ); + } + } + + /// + /// The same call is what creates the folder in the ordinary case, where nothing is there. + /// + [Test] + public void StartFolderEmpty_FolderDoesNotExist_CreatesIt() + { + using (var parent = new TemporaryFolder("StartFolderEmptyCreates")) + { + var path = Path.Combine(parent.FolderPath, "brand-new"); + Assert.That(Directory.Exists(path), Is.False, "Setup sanity check."); + + TestTempDirectory.StartFolderEmpty(path); + + Assert.That(Directory.Exists(path), Is.True); + } + } + + /// + /// When a test leaves a file open, the end-of-run warning has to say which file and why, + /// otherwise "could not delete the temp folder" gives whoever reads it nowhere to start. + /// + [Test] + public void DescribeWhyFolderCouldNotBeDeleted_FileStillOpen_NamesThatFileAndTheReason() + { + using (var folder = new TemporaryFolder("DescribeWhyFolderCouldNotBeDeleted")) + { + var lockedPath = Path.Combine(folder.FolderPath, "someone-left-me-open.txt"); + File.WriteAllText(lockedPath, "contents"); + var innocentPath = Path.Combine(folder.FolderPath, "closed-properly.txt"); + File.WriteAllText(innocentPath, "contents"); + + // Sanity check: with nothing holding either file, no file should be singled out. + Assert.That( + TestTempDirectory.DescribeWhyFolderCouldNotBeDeleted(folder.FolderPath), + Does.Not.Contain("could not be opened"), + "Setup sanity check: neither file is open yet, so none should be blamed." + ); + + using (File.Open(lockedPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + var message = TestTempDirectory.DescribeWhyFolderCouldNotBeDeleted( + folder.FolderPath + ); + + Assert.That(message, Is.Not.Null); + Assert.That( + message, + Does.Contain("someone-left-me-open.txt"), + "The warning should name the file that is actually held." + ); + Assert.That( + message, + Does.Not.Contain("closed-properly.txt"), + "It should point at the locked file, not just the first file it happened to find." + ); + Assert.That( + message, + Does.Contain("being used by another process").IgnoreCase, + "It should pass on the reason the operating system gave." + ); + } + } + } + + /// + /// The same call is how the teardown decides whether there is anything to complain about + /// at all, so a folder that really did go must produce nothing. + /// + [Test] + public void DescribeWhyFolderCouldNotBeDeleted_FolderIsGone_SaysNothing() + { + string path; + using (var folder = new TemporaryFolder("DescribeWhyFolderIsGone")) + { + path = folder.FolderPath; + Assert.That(Directory.Exists(path), Is.True, "Setup sanity check."); + } + + Assert.That(Directory.Exists(path), Is.False, "Setup sanity check: it was disposed."); + Assert.That(TestTempDirectory.DescribeWhyFolderCouldNotBeDeleted(path), Is.Null); + } + + /// + /// The redirect reaches the code that matters: an ordinary fixed-name TemporaryFolder, + /// written exactly as the ~180 existing calls are, is created inside our run folder. + /// + [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." + ); + } + } + } +}