From d2a6107cdb8d084501da496c6b030305bbbdf379 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 16 Jul 2026 23:31:28 -0400 Subject: [PATCH] test(host): serialize Central-boot fixtures to close env-var race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CentralDbTestEnvironment sets five process-wide environment variables, and Program's AddEnvironmentVariables() reads them at an unpredictable point during host boot. With xUnit collection parallelization on, one fixture's teardown could clear a var mid-boot for a sibling. Since the secrets adoption (G-4) three of those keys are ${secret:...} references that fail closed, turning a previously benign empty value into a SecretNotFoundException that aborts the boot — an intermittent CI failure. Adds a HostBootCollection that serializes every fixture booting a real host while depending on that shared state, folding in the narrower "ActorSystem" collection so its members stay serialized with each other as before. Site-role fixtures stay parallel: they call Configuration.Sources.Clear(), dropping the env-var provider, so they cannot participate in the race. CentralDbTestEnvironment now also fails fast if two instances are ever live at once, making a regression (a fixture added outside the collection) deterministic rather than intermittent — this is what surfaced the disposed-CTS defect fixed in the previous commit. Fixture teardown is try/finally so a throwing host teardown can no longer strand the vars for the rest of the run. Cost: Host.Tests runs ~2m30s -> ~4m10s; the serialized Central-boot classes are most of the assembly's parallelism. Fixes: Gitea #15 --- .../ActorPathTests.cs | 24 +++++++++++------ .../AkkaHostedServiceAuditWiringTests.cs | 15 ++++++++--- .../CentralDbTestEnvironment.cs | 27 +++++++++++++++++++ .../CompositionRootTests.cs | 15 ++++++++--- .../HealthCheckTests.cs | 1 + .../HostBootCollection.cs | 25 +++++++++++++++++ .../HostStartupTests.cs | 1 + .../MetricsEndpointTests.cs | 1 + 8 files changed, 95 insertions(+), 14 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostBootCollection.cs diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs index f96a8f54..5b0b7bcd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs @@ -10,14 +10,11 @@ using ZB.MOM.WW.ScadaBridge.Host.Actors; namespace ZB.MOM.WW.ScadaBridge.Host.Tests; -[CollectionDefinition("ActorSystem")] -public class ActorSystemCollection : ICollectionFixture { } - /// /// Verifies that all expected Central-role actors are created at the correct paths /// when AkkaHostedService starts. /// -[Collection("ActorSystem")] +[Collection(HostBootCollection.Name)] public class CentralActorPathTests : IAsyncLifetime { private WebApplicationFactory? _factory; @@ -93,9 +90,20 @@ public class CentralActorPathTests : IAsyncLifetime public async Task DisposeAsync() { - _factory?.Dispose(); - Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", _previousEnv); - _dbEnv?.Dispose(); + // try/finally: a throwing host teardown must not strand the process-wide + // environment variables. Without it, one failed _factory.Dispose() leaks + // DOTNET_ENVIRONMENT=Central and the CentralDbTestEnvironment vars into + // every later test in the run. + try + { + _factory?.Dispose(); + } + finally + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", _previousEnv); + _dbEnv?.Dispose(); + } + await Task.CompletedTask; } @@ -149,7 +157,7 @@ public class CentralActorPathTests : IAsyncLifetime /// Verifies that all expected Site-role actors are created at the correct paths /// when AkkaHostedService starts. /// -[Collection("ActorSystem")] +[Collection(HostBootCollection.Name)] public class SiteActorPathTests : IAsyncLifetime { private IHost? _host; diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs index 15afc439..d31f27a8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs @@ -86,6 +86,7 @@ public class AkkaHostedServiceAuditWiringHoconTests /// /// Verifies Audit Log (#23) services land in the Central composition root. /// +[Collection(HostBootCollection.Name)] public class CentralAuditWiringTests : IDisposable { private readonly WebApplicationFactory _factory; @@ -156,9 +157,17 @@ public class CentralAuditWiringTests : IDisposable public void Dispose() { - _factory.Dispose(); - Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", _previousEnv); - _dbEnv.Dispose(); + // try/finally: a throwing host teardown must not strand the process-wide + // environment variables for the rest of the run. + try + { + _factory.Dispose(); + } + finally + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", _previousEnv); + _dbEnv.Dispose(); + } } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralDbTestEnvironment.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralDbTestEnvironment.cs index 60940a69..62a091fd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralDbTestEnvironment.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralDbTestEnvironment.cs @@ -26,6 +26,14 @@ namespace ZB.MOM.WW.ScadaBridge.Host.Tests; /// the tokens entirely — so tests that boot the real Program pipeline do /// not need a seeded secrets store. All vars are restored on Dispose so tests /// stay isolated. +/// +/// Every var below is process-wide, and the reader — Program's +/// AddEnvironmentVariables() — runs at an unpredictable point during host boot. +/// Two live instances therefore cannot be allowed to overlap: one's Dispose would +/// restore/clear a var while the other's boot is still reading it, and the fail-closed +/// secrets expander turns that into a SecretNotFoundException. Serialization via +/// is what enforces the invariant; the overlap check in +/// the constructor makes a regression fail deterministically instead of intermittently. /// internal sealed class CentralDbTestEnvironment : IDisposable { @@ -64,8 +72,25 @@ internal sealed class CentralDbTestEnvironment : IDisposable private readonly string? _previousLdapPassword; private readonly string? _previousJwtSigningKey; + /// + /// Number of live instances. Only ever 0 or 1 while every consuming fixture is a + /// member of ; see the overlap check below. + /// + private static int _liveCount; + public CentralDbTestEnvironment() { + if (Interlocked.Increment(ref _liveCount) != 1) + { + Interlocked.Decrement(ref _liveCount); + throw new InvalidOperationException( + $"Two {nameof(CentralDbTestEnvironment)} instances are live at once, so one fixture's " + + "teardown can clear the process-wide environment variables another fixture's host boot " + + $"is reading. Add the offending test class to the \"{HostBootCollection.Name}\" collection " + + $"([Collection({nameof(HostBootCollection)}.Name)]) so xUnit runs it sequentially with the " + + "other host-boot fixtures."); + } + _previousConfig = Environment.GetEnvironmentVariable(ConfigKey); Environment.SetEnvironmentVariable(ConfigKey, ConfigurationDb); @@ -89,5 +114,7 @@ internal sealed class CentralDbTestEnvironment : IDisposable Environment.SetEnvironmentVariable(PepperKey, _previousPepper); Environment.SetEnvironmentVariable(LdapPasswordKey, _previousLdapPassword); Environment.SetEnvironmentVariable(JwtSigningKeyKey, _previousJwtSigningKey); + + Interlocked.Decrement(ref _liveCount); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs index ba54db24..d6ab0f9e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs @@ -80,6 +80,7 @@ internal static class AkkaHostedServiceRemover /// Verifies every expected DI service resolves from the Central composition root. /// Uses WebApplicationFactory to exercise the real Program.cs pipeline. /// +[Collection(HostBootCollection.Name)] public class CentralCompositionRootTests : IDisposable { private readonly WebApplicationFactory _factory; @@ -159,9 +160,17 @@ public class CentralCompositionRootTests : IDisposable public void Dispose() { - _factory.Dispose(); - Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", _previousEnv); - _dbEnv.Dispose(); + // try/finally: a throwing host teardown must not strand the process-wide + // environment variables for the rest of the run. + try + { + _factory.Dispose(); + } + finally + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", _previousEnv); + _dbEnv.Dispose(); + } } // --- Singletons --- diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs index 298d959b..c730038e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs @@ -17,6 +17,7 @@ namespace ZB.MOM.WW.ScadaBridge.Host.Tests; /// active-node) rather than by check-name predicates. These are pure route/tag assertions /// — they require no database, LDAP, or formed Akka cluster. /// +[Collection(HostBootCollection.Name)] public class HealthCheckTests : IDisposable { private readonly List _disposables = new(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostBootCollection.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostBootCollection.cs new file mode 100644 index 00000000..8629bd65 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostBootCollection.cs @@ -0,0 +1,25 @@ +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// Serializes every fixture that boots a real host while depending on process-wide +/// state — the DOTNET_ENVIRONMENT role selector and the environment variables +/// sets. +/// +/// Program's configuration builder calls AddEnvironmentVariables() at an +/// unpredictable point during boot, so a sibling fixture's teardown (which restores or +/// clears those vars) can otherwise be observed mid-boot as a transient null. Since the +/// secrets adoption (G-4) three of those keys are ${secret:...} references that +/// fail closed, which turns that race into a hard SecretNotFoundException. +/// xUnit runs classes in a shared collection sequentially, which removes the overlap. +/// +/// Site-role fixtures are deliberately not members: they call +/// Configuration.Sources.Clear(), dropping the environment-variable provider, so +/// they neither read nor write the shared state and stay free to run in parallel. +/// +/// Supersedes the narrower "ActorSystem" collection, whose members are folded in here. +/// +[CollectionDefinition(HostBootCollection.Name)] +public class HostBootCollection +{ + public const string Name = "HostBoot"; +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs index 2ff6c6d7..206926ad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HostStartupTests.cs @@ -9,6 +9,7 @@ using ZB.MOM.WW.ScadaBridge.Host; namespace ZB.MOM.WW.ScadaBridge.Host.Tests; +[Collection(HostBootCollection.Name)] public class HostStartupTests : IDisposable { private readonly List _disposables = new(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs index f5aeb8ee..563c8800 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/MetricsEndpointTests.cs @@ -13,6 +13,7 @@ namespace ZB.MOM.WW.ScadaBridge.Host.Tests; /// cluster state. This is a pure route assertion — it requires no database, LDAP, or formed /// Akka cluster. The Central-role factory bootstrap mirrors . /// +[Collection(HostBootCollection.Name)] public class MetricsEndpointTests : IDisposable { private readonly List _disposables = new();