From 26e3a1b9a6b2af910c173899ddcc933fabe56b1b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:27:22 -0400 Subject: [PATCH 1/3] =?UTF-8?q?test(siteruntime):=20close=20register=20row?= =?UTF-8?q?=2091=20=E2=80=94=20compile-cache=20asserts=20become=20key-scop?= =?UTF-8?q?ed=20deltas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SiteScriptCompileCacheTests.GetOrAdd_SameCodeAndGlobals_ComputesOnce asserted Assert.Equal(1, SiteScriptCompileCache.Hits) after a Clear() on what is a process-wide static cache. The [Collection("SiteScriptCompileCache")] attribute serializes this class against ScriptCompilationServiceTests and DeploymentWarmThenGateTests, but NOT against the rest of the assembly: every class that compiles a script — Instance/Script actor tests, deployment tests — goes through the same GetOrAdd in parallel and bumps the same global Hits/Count. Failed ~1 run in 4 under a full parallel solution run, green in isolation. Fixed with the register's row-74 pattern (98c94588): assert the DELTA attributable to this test's OWN keys, never an absolute global value. Two habits carry it. 1. Every test now keys its entries on a per-test-instance guid token (the new Code() helper embeds it as a comment), so no other class in the assembly can collide with or serve them. That makes the factory-call counts and Assert.Same identity checks exact and fully owned — and those are the real claims: "computed once" and "the second lookup returned that instance". 2. The process-global counters are only ever asserted as one-sided bounds (>= baseline + n), since foreign traffic can only add. There is no concurrent Clear() to make a delta go negative: all three classes that call Clear() are in the serialized collection. Four assertions changed shape, and two got stronger rather than weaker. GetOrAdd_SameCode_DifferentGlobals_AreSeparateEntries asserted Count == 2 and Hits == 0 — both only true while nothing else was compiling. "No cross-globals hit" is now asserted where it is observable and ours: the second lookup HAD to compute, the two results are not the same instance, and re-getting either serves its own entry rather than the other globals surface's. Overflow_StaysWithinTheBound gained an eviction probe over its own keys — a bound with foreign slack is not proof a sweep ran; a recompute of an entry we inserted is. Overflow_EvictsOldestBatch_… had Assert.Equal(hitsBefore + 1, Hits), the same absolute-delta shape, now a lower bound alongside the sharp Assert.Same. DeploymentWarmThenGateTests carried the identical latent shape one line before its own delta assertion — Assert.Equal(0, Hits) immediately after Clear() — and is converted to a baseline snapshot too. SiteRuntime.Tests: 633/633 green, twice, at full parallelism (43 s, 42 s). --- .../Actors/DeploymentWarmThenGateTests.cs | 11 +- .../Scripts/SiteScriptCompileCacheTests.cs | 118 ++++++++++++++---- 2 files changed, 105 insertions(+), 24 deletions(-) diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs index 58f23c41..7028e886 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs @@ -248,15 +248,20 @@ public class DeploymentWarmThenGateTests : TestKit, IDisposable "BatchTwo", ConfigJson("BatchTwo", sharedCode), "d2", "h2", true); SiteScriptCompileCache.Clear(); - Assert.Equal(0, SiteScriptCompileCache.Hits); + // Baseline rather than Assert.Equal(0, Hits): Hits is a process-global counter and test + // classes outside this collection compile scripts in parallel, so a foreign hit can land + // between the Clear() and the read (deferred-work register row 91). Everything below is + // asserted as a delta over this baseline. + var hitsBefore = SiteScriptCompileCache.Hits; CreateDeploymentManager(); AwaitAssert(() => { // One compile, then repeated hits: the second pre-warm plus both PreStarts. - Assert.True(SiteScriptCompileCache.Hits >= 3, - $"expected the pre-warmed body to be served from cache, saw {SiteScriptCompileCache.Hits} hits"); + Assert.True(SiteScriptCompileCache.Hits - hitsBefore >= 3, + $"expected the pre-warmed body to be served from cache, saw " + + $"{SiteScriptCompileCache.Hits - hitsBefore} hits over baseline {hitsBefore}"); }, TimeSpan.FromSeconds(20)); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs index d25f7e9c..125b415b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs @@ -7,39 +7,98 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Scripts; /// /// Tests for the process-wide . The cache is /// static/process-wide state, so this class shares the "SiteScriptCompileCache" xunit -/// collection with ScriptCompilationServiceTests (no cross-class parallelism) and -/// calls at the top of each test. +/// collection with ScriptCompilationServiceTests and DeploymentWarmThenGateTests +/// (no cross-class parallelism between those three, so no concurrent +/// ). /// +/// +/// +/// Deferred-work register row 91. That collection does NOT stop the rest of the +/// assembly: any test class that compiles a script — Instance/Script actor tests, deployment +/// tests — goes through too, in parallel, and +/// bumps the same process-global / +/// . Absolute assertions such as +/// Assert.Equal(1, Hits) after a Clear() therefore only held while nothing else +/// happened to be compiling; they failed roughly 1 run in 4 of a full parallel solution run and +/// were always green in isolation. +/// +/// +/// The fix follows the register's row-74 pattern: assert the DELTA attributable to this test's +/// OWN keys across its own scoped action, never an absolute global value. Two habits carry it: +/// every test keys its cache entries on a per-test-instance unique token (), +/// so no other class can collide with or serve them; and the global counters are only ever +/// asserted as one-sided bounds (>= baseline + n), since foreign traffic can only add. +/// A collection fixture spanning every script-compiling class in the assembly would fix it too, +/// but would serialise most of the suite for the benefit of five facts. +/// +/// [Collection("SiteScriptCompileCache")] public class SiteScriptCompileCacheTests { + /// + /// Slack allowed on bounds for entries inserted + /// by test classes outside this collection while a bound test runs (register row 91). The + /// exactness of the eviction claims comes from the per-key probes, not from this bound. + /// + private const int ForeignEntrySlack = 512; + + /// Per-test-instance token making every cache key in this test uniquely ours. + private readonly string _uniq = Guid.NewGuid().ToString("N"); + + /// Builds a cache key body that no other test class in the assembly can produce. + private string Code(string body) => $"/*{_uniq}*/ {body}"; + private static ScriptCompilationResult Ok() => ScriptCompilationResult.Succeeded(CSharpScript.Create("1", ScriptOptions.Default, typeof(ScriptGlobals))); [Fact] public void GetOrAdd_SameCodeAndGlobals_ComputesOnce() { - SiteScriptCompileCache.Clear(); + var code = Code("return 1;"); + var hitsBefore = SiteScriptCompileCache.Hits; var factoryCalls = 0; ScriptCompilationResult Factory() { factoryCalls++; return Ok(); } - var r1 = SiteScriptCompileCache.GetOrAdd("return 1;", typeof(ScriptGlobals), Factory); - var r2 = SiteScriptCompileCache.GetOrAdd("return 1;", typeof(ScriptGlobals), Factory); + var r1 = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), Factory); + var r2 = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), Factory); + // Scoped to our own key, so these two are exact and are the real claim: one compile, + // and the second lookup returned that very instance rather than a re-computation. Assert.Equal(1, factoryCalls); Assert.Same(r1, r2); - Assert.Equal(1, SiteScriptCompileCache.Hits); + + // Hits is process-global; a parallel class's compile can only push the delta up, so the + // claim "the second lookup was counted as a hit" is a lower bound, not an equality. + Assert.True(SiteScriptCompileCache.Hits >= hitsBefore + 1, + $"expected the second GetOrAdd to register a hit; hits went {hitsBefore} -> {SiteScriptCompileCache.Hits}"); } [Fact] public void GetOrAdd_SameCode_DifferentGlobals_AreSeparateEntries() { - SiteScriptCompileCache.Clear(); - SiteScriptCompileCache.GetOrAdd("1 > 0", typeof(ScriptGlobals), Ok); - SiteScriptCompileCache.GetOrAdd("1 > 0", typeof(TriggerExpressionGlobals), Ok); + var code = Code("1 > 0"); + var scriptCompiles = 0; + var triggerCompiles = 0; - Assert.Equal(2, SiteScriptCompileCache.Count); - Assert.Equal(0, SiteScriptCompileCache.Hits); // no cross-globals hit + var script = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), + () => { scriptCompiles++; return Ok(); }); + var trigger = SiteScriptCompileCache.GetOrAdd(code, typeof(TriggerExpressionGlobals), + () => { triggerCompiles++; return Ok(); }); + + // "No cross-globals hit" asserted where it is actually observable and ours: the second + // lookup had to compute, and the two globals surfaces hold distinct entries. (The old + // form — Count == 2 and Hits == 0 on the process-global counters — was only true while + // no other class was compiling.) + Assert.Equal(1, scriptCompiles); + Assert.Equal(1, triggerCompiles); + Assert.NotSame(script, trigger); + + // Each entry is independently addressable afterwards: re-getting either must serve the + // matching cached result, never the other globals surface's. + Assert.Same(script, SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), + () => throw new InvalidOperationException("script entry was not cached"))); + Assert.Same(trigger, SiteScriptCompileCache.GetOrAdd(code, typeof(TriggerExpressionGlobals), + () => throw new InvalidOperationException("trigger entry was not cached"))); } [Fact] @@ -47,9 +106,22 @@ public class SiteScriptCompileCacheTests { SiteScriptCompileCache.Clear(); for (var i = 0; i <= SiteScriptCompileCache.MaxEntries; i++) - SiteScriptCompileCache.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return {i};"), typeof(ScriptGlobals), Ok); - Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries); + // MaxEntries + 1 distinct inserts must not all be resident — the sweep ran. The bound + // carries slack for foreign inserts (register row 91); the sharp claim is the eviction + // probe below, which is scoped to keys only this test can produce. + Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries + ForeignEntrySlack, + $"cache grew past its bound: {SiteScriptCompileCache.Count} vs max {SiteScriptCompileCache.MaxEntries}"); + + var recomputed = 0; + for (var i = 0; i < 16; i++) + { + var code = Code($"return {i};"); + SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), () => { recomputed++; return Ok(); }); + } + + Assert.True(recomputed > 0, "no oldest-batch entry was evicted — the overflow sweep did not run"); } // ── WP3.1: approximate LRU replaced the wholesale-Clear overflow cliff ────────── @@ -70,15 +142,15 @@ public class SiteScriptCompileCacheTests // Fill to the bound. Entries 1..N are inserted oldest-first. for (var i = 0; i < SiteScriptCompileCache.MaxEntries; i++) - SiteScriptCompileCache.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return {i};"), typeof(ScriptGlobals), Ok); // Touch entry 0 so it carries the NEWEST access stamp despite being inserted first. - var hot = SiteScriptCompileCache.GetOrAdd("return 0;", typeof(ScriptGlobals), Ok); + var hot = SiteScriptCompileCache.GetOrAdd(Code("return 0;"), typeof(ScriptGlobals), Ok); var hitsAfterTouch = SiteScriptCompileCache.Hits; // Push well past the bound so at least one eviction sweep definitely runs. for (var i = 0; i < 200; i++) - SiteScriptCompileCache.GetOrAdd($"return overflow{i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return overflow{i};"), typeof(ScriptGlobals), Ok); // Nothing was cleared wholesale: the cache is still most of the way full and the hit // counter survived (Clear() would have reset it to 0). @@ -89,16 +161,20 @@ public class SiteScriptCompileCacheTests // The touched entry survived the sweep — a hit, not a recompile. This is the LRU // property: recency, not insertion order, decides what stays. var hitsBefore = SiteScriptCompileCache.Hits; - var again = SiteScriptCompileCache.GetOrAdd("return 0;", typeof(ScriptGlobals), + var again = SiteScriptCompileCache.GetOrAdd(Code("return 0;"), typeof(ScriptGlobals), () => throw new InvalidOperationException("hot entry was evicted — LRU is not keeping recently-used entries")); + // Assert.Same is the sharp claim (our own key, our own instance). The counter check is a + // lower bound: Hits is process-global and a parallel class's hit lands in the same window + // (register row 91 — this was `Assert.Equal(hitsBefore + 1, …)`). Assert.Same(hot, again); - Assert.Equal(hitsBefore + 1, SiteScriptCompileCache.Hits); + Assert.True(SiteScriptCompileCache.Hits >= hitsBefore + 1, + $"expected the surviving hot entry to register a hit; hits went {hitsBefore} -> {SiteScriptCompileCache.Hits}"); // …and untouched old entries genuinely were evicted, so the sweep really ran. var recomputed = 0; for (var i = 1; i <= 16; i++) { - var code = $"return {i};"; + var code = Code($"return {i};"); SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), () => { recomputed++; return Ok(); }); } @@ -119,7 +195,7 @@ public class SiteScriptCompileCacheTests var tasks = Enumerable.Range(0, 8).Select(t => Task.Run(() => { for (var i = 0; i < perTask; i++) - SiteScriptCompileCache.GetOrAdd($"return {t}_{i};", typeof(ScriptGlobals), Ok); + SiteScriptCompileCache.GetOrAdd(Code($"return {t}_{i};"), typeof(ScriptGlobals), Ok); })); await Task.WhenAll(tasks); @@ -131,7 +207,7 @@ public class SiteScriptCompileCacheTests // eviction bug would show up here as unbounded growth, not as a handful of extra rows. // (Other test classes compile into this process-wide cache concurrently, which is the // other reason the bound is asserted with slack rather than exactly.) - Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries + 512, + Assert.True(SiteScriptCompileCache.Count <= SiteScriptCompileCache.MaxEntries + ForeignEntrySlack, $"cache grew past its bound: {SiteScriptCompileCache.Count} vs max {SiteScriptCompileCache.MaxEntries} " + $"after {8 * perTask} distinct inserts"); } From 34a8079d156b5dd4086854b6733a048a1d65d794 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:27:37 -0400 Subject: [PATCH 2/3] =?UTF-8?q?test(auditlog):=20close=20register=20row=20?= =?UTF-8?q?95=20=E2=80=94=20stop=20the=20three=20remaining=20leaked=20fast?= =?UTF-8?q?-tick=20actors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Row 82 (da23f2dc) fixed this leak shape in PartitionPurgeTests and NotifyDispatcherAuditTrailTests; three classes in the same assembly still had it. Each creates an actor with a fast periodic tick (100 ms purge override, ~0 s reconciliation override) against a shared MsSqlMigrationFixture database and never stops it deterministically, relying on TestKit's Dispose -> Shutdown — which is timeout-bounded and does NOT fail when it expires. A survivor keeps mutating the fixture DB while LATER test classes run. All three now use the same Watch(actor) / Sys.Stop(actor) / ExpectTerminated(actor) helper da23f2dc established, applied at EVERY actor-creation site — 11 in AuditLogPurgeActorTests, 10 in SiteAuditReconciliationActorTests, 2 in OutageReconciliationTests. The two real-DB cases are the ones that could corrupt another class: AuditLogPurgeActorTests' EndToEnd_RealPartition_… keeps switching partitions out of the shared DB, and SiteAuditReconciliationActorTests' Tick_Duplicates_NotDoubleInserted holds a live AuditLogRepository over a DbContext the test is about to dispose. The stub-repo cases are cheaper but not free: each burns a DI scope and a thread every 100 ms for the rest of the assembly run. OutageReconciliationTests needed more than the helper. It already called Sys.Stop, but (a) fire-and-forget, so the call returns before the in-flight tick has finished writing to MSSQL, and (b) from the LAST line of the enclosing `await using` block, so any earlier assertion failure skipped it entirely. Both stops move to immediately after the last step that needs the actor alive — before the read-back that counts rows — and the AuditLogIngestActor that BuildCentralPipeline creates is now stopped too (test 2 was discarding its ref outright, leaving it holding a service provider the block then disposed). Where a test's remaining assertions read a collection the actor appends to (client.Calls indexing, Assert.Empty on a switched-boundaries list), the stop is placed BEFORE them so they no longer read a moving target. AuditLog.Tests: 383/383 green (1 m 51 s), 0 skipped. --- .../Central/AuditLogPurgeActorTests.cs | 82 ++++++++++++++++--- .../SiteAuditReconciliationActorTests.cs | 79 +++++++++++++++--- .../Integration/OutageReconciliationTests.cs | 52 ++++++++++-- 3 files changed, 184 insertions(+), 29 deletions(-) diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeActorTests.cs index 6ac9dabc..457cba2f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPurgeActorTests.cs @@ -138,6 +138,38 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture.Instance))); } + /// + /// Stops a purge actor and blocks until it is actually dead. + /// + /// + /// + /// Deferred-work register row 95, the same leak shape row 82 fixed in + /// PartitionPurgeTests / NotifyDispatcherAuditTrailTests. Every actor this + /// class creates ticks every 100 ms + /// () and was only ever stopped + /// implicitly by 's Dispose → Shutdown — which is + /// bounded by a timeout and does NOT fail when that timeout expires. So on a loaded + /// machine an actor kept ticking while LATER test classes ran: the stub-repo tests burn a + /// DI scope and a thread every 100 ms for the rest of the assembly run, and + /// EndToEnd_RealPartition_…'s actor keeps switching partitions out of the shared + /// database that other classes in this assembly seed + /// against. + /// + /// + /// Stopping the actor inside the test that created it removes the dependency on + /// ActorSystem-shutdown timing entirely. + /// + /// + private void StopPurgeActor(IActorRef actor) + { + Watch(actor); + Sys.Stop(actor); + // A stop is processed after the in-flight tick completes, so this also guarantees no + // partition switch is still running against the fixture DB. + ExpectTerminated(actor, TimeSpan.FromSeconds(30)); + Unwatch(actor); + } + private static AuditLogPurgeOptions FastTickOptions(TimeSpan? interval = null) => new() { IntervalHours = 24, @@ -163,7 +195,7 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture= 1 enumerator call, got {repo.ThresholdQueries.Count}"), duration: TimeSpan.FromSeconds(3), interval: TimeSpan.FromMilliseconds(50)); + + StopPurgeActor(actor); } // --------------------------------------------------------------------- @@ -192,7 +226,7 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture 42L, }; - CreateActor(repo, FastTickOptions()); + var actor = CreateActor(repo, FastTickOptions()); AwaitAssert( () => @@ -202,6 +236,8 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture() }; - CreateActor(repo, FastTickOptions()); + var actor = CreateActor(repo, FastTickOptions()); // Wait for at least one tick (visible via the enumerator call) then // assert no switch happened. @@ -226,6 +262,10 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture(TimeSpan.FromSeconds(5)); + StopPurgeActor(actor); Assert.Equal(boundary, msg.MonthBoundary); Assert.Equal(1234L, msg.RowsDeleted); Assert.True(msg.DurationMs >= 0, @@ -269,7 +310,7 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture @@ -281,6 +322,8 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture new AuditLogPurgeActor( + var actor = Sys.ActorOf(Props.Create(() => new AuditLogPurgeActor( sp, Options.Create(purgeOptions), Options.Create(auditOptions), @@ -363,6 +406,11 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture m.MonthBoundary == janBoundary, max: TimeSpan.FromSeconds(30)); + // This is the one actor in the class that mutates the SHARED fixture database, so it + // must not outlive the test and start switching partitions out from under the next + // class's seed rows (register row 95). + StopPurgeActor(actor); + Assert.True(matched.RowsDeleted >= 1, $"Expected RowsDeleted >= 1 for the Jan-2026 partition; got {matched.RowsDeleted}."); @@ -394,7 +442,7 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture new AuditLogPurgeActor( + var actor = Sys.ActorOf(Props.Create(() => new AuditLogPurgeActor( sp, purgeOptionsWrapped, auditOptions, @@ -448,6 +498,8 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture p.Channel == "ApiOutbound"); Assert.Equal(1234, purge.BatchSize); @@ -483,7 +535,7 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture new AuditLogPurgeActor( + var actor = Sys.ActorOf(Props.Create(() => new AuditLogPurgeActor( sp, purgeOptions, auditOptions, @@ -496,6 +548,10 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture new AuditLogPurgeActor( + var actor = Sys.ActorOf(Props.Create(() => new AuditLogPurgeActor( sp, Options.Create(purgeOptions), auditOptions, @@ -542,6 +598,8 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture new AuditLogPurgeActor( + var actor = Sys.ActorOf(Props.Create(() => new AuditLogPurgeActor( sp, Options.Create(FastTickOptions()), Options.Create(new AuditLogOptions()), @@ -588,5 +646,7 @@ public class AuditLogPurgeActorTests : TestKit, IClassFixture= 1, got {snapshot.PurgeFailures}"), duration: TimeSpan.FromSeconds(3), interval: TimeSpan.FromMilliseconds(50)); + + StopPurgeActor(actor); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationActorTests.cs index 411cc2c4..3cd50a0c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/SiteAuditReconciliationActorTests.cs @@ -252,6 +252,37 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture.Instance))); } + /// + /// Stops a reconciliation actor and blocks until it is actually dead. + /// + /// + /// + /// Deferred-work register row 95, the same leak shape row 82 fixed in + /// PartitionPurgeTests / NotifyDispatcherAuditTrailTests. Every actor this + /// class creates ticks on a ~0 s repeat interval () and was + /// only ever stopped implicitly by 's Dispose → + /// Shutdown, which is bounded by a timeout and does NOT fail when it expires. A + /// survivor keeps pulling and INSERTING audit rows: in + /// Tick_Duplicates_NotDoubleInserted it holds a real + /// over the shared + /// database (and over a DbContext the test is about to dispose), so it writes into + /// the DB LATER test classes read from. + /// + /// + /// Stopping the actor inside the test that created it removes the dependency on + /// ActorSystem-shutdown timing entirely. + /// + /// + private void StopReconciliationActor(IActorRef actor) + { + Watch(actor); + Sys.Stop(actor); + // A stop is processed after the in-flight tick completes, so this also guarantees no + // pull or insert is still running against the fixture DB. + ExpectTerminated(actor, TimeSpan.FromSeconds(30)); + Unwatch(actor); + } + /// /// Subscribes to the EventStream and collects every /// publication into a list @@ -278,7 +309,7 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture Assert.True(client.Calls.Count >= 1, $"expected >= 1 pull call, got {client.Calls.Count}"), duration: TimeSpan.FromSeconds(3), interval: TimeSpan.FromMilliseconds(50)); + + StopReconciliationActor(actor); } // --------------------------------------------------------------------- @@ -304,7 +337,7 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture { @@ -313,6 +346,8 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture Assert.Equal(2, repo.InsertCallCount), duration: TimeSpan.FromSeconds(3), interval: TimeSpan.FromMilliseconds(50)); + + StopReconciliationActor(actor); + Assert.Contains(repo.Inserted, e => e.EventId == e1.EventId); Assert.Contains(repo.Inserted, e => e.EventId == e2.EventId); } @@ -371,13 +409,18 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture Assert.True(client.Calls.Count >= 1), duration: TimeSpan.FromSeconds(3)); + // Stop before reading: this actor holds a real repository over the SHARED fixture + // database (and over the DbContext this test is about to dispose), so it must not + // outlive the test (register row 95). + StopReconciliationActor(actor); + // Even though the pull returned 2 events, only 1 fresh row should // exist in MSSQL alongside the pre-existing one — InsertIfNotExistsAsync // is first-write-wins on EventId. @@ -413,7 +456,7 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture Assert.True(client.Calls.Count >= 2, $"need at least 2 pulls, got {client.Calls.Count}"), duration: TimeSpan.FromSeconds(5), interval: TimeSpan.FromMilliseconds(50)); + StopReconciliationActor(actor); + Assert.Equal(t, client.Calls[1].SinceUtc); Assert.Equal(high.EventId.ToString(), client.Calls[1].AfterId); } @@ -475,13 +524,15 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture Assert.True(client.Calls.Count >= 2, $"need at least 2 pulls, got {client.Calls.Count}"), duration: TimeSpan.FromSeconds(5), interval: TimeSpan.FromMilliseconds(50)); + StopReconciliationActor(actor); + Assert.Equal(DateTime.MinValue, client.Calls[1].SinceUtc); Assert.Null(client.Calls[1].AfterId); } @@ -543,7 +594,7 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture { @@ -552,6 +603,8 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture(TimeSpan.FromSeconds(5)); + StopReconciliationActor(actor); Assert.Equal("siteA", msg.SiteId); Assert.True(msg.Stalled); } @@ -606,7 +660,7 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture(TimeSpan.FromSeconds(5)); + StopReconciliationActor(actor); Assert.False(second.Stalled); Assert.Equal("siteA", second.SiteId); } @@ -643,7 +698,7 @@ public class SiteAuditReconciliationActorTests : TestKit, IClassFixture Assert.Contains(client.Calls, c => c.SinceUtc == poisonTime), duration: TimeSpan.FromSeconds(5), interval: TimeSpan.FromMilliseconds(100)); + + StopReconciliationActor(actor); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/OutageReconciliationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/OutageReconciliationTests.cs index 2104c9a1..c02d5840 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/OutageReconciliationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/OutageReconciliationTests.cs @@ -165,6 +165,37 @@ public class OutageReconciliationTests : TestKit, IClassFixture + /// Stops an actor and blocks until it is actually dead. + /// + /// + /// + /// Deferred-work register row 95, the same leak shape row 82 fixed in + /// PartitionPurgeTests / NotifyDispatcherAuditTrailTests. Both tests here + /// already called Sys.Stop, but that is fire-and-forget: it returns before the + /// actor has processed the stop, so an in-flight 100 ms tick could still be pulling and + /// INSERTING into the shared database — and into the + /// the enclosing await using is about to dispose. + /// Worse, the call sat at the very END of the block, so any earlier assertion failure + /// skipped it entirely and left the actor ticking for the rest of the assembly run, + /// stopped only by 's Dispose → Shutdown, which is + /// timeout-bounded and does not fail when it expires. + /// + /// + /// Watch/Stop/ExpectTerminated makes the stop synchronous, and each call site now sits + /// immediately after the last step that needs the actor alive rather than at the end. + /// + /// + private void StopActor(IActorRef actor) + { + Watch(actor); + Sys.Stop(actor); + // A stop is processed after the in-flight tick completes, so this also guarantees no + // pull or insert is still running against the fixture DB. + ExpectTerminated(actor, TimeSpan.FromSeconds(30)); + Unwatch(actor); + } + private static SiteAuditReconciliationOptions FastTickOptions(int batchSize = 256) => new() { ReconciliationIntervalSeconds = 300, @@ -265,6 +296,12 @@ public class OutageReconciliationTests : TestKit, IClassFixture centralIds.Contains(w.EventId)), "every site-written EventId should be present centrally."); - - // Tear the actor down before disposing the harness; the actor's - // PostStop cancels its scheduled timer. - Sys.Stop(reconciliationActor); } } @@ -303,7 +336,7 @@ public class OutageReconciliationTests : TestKit, IClassFixture callsAfterDrain, $"expected additional pull calls after drain to validate idempotency, got {pullClient.CallCount} after {callsAfterDrain}"); + // Both actors have done everything the test needs; stop them HERE, before the + // read-back, so nothing is still writing to the shared fixture DB while the + // duplicate check counts rows and so a failure below cannot leak them + // (register row 95). + StopActor(reconciliationActor); + StopActor(ingest); + // Central count must still be exactly totalEvents — no duplicates // even though the cursor + read-Reconciled-too semantics could // theoretically re-fetch on the second cycle. @@ -347,8 +387,6 @@ public class OutageReconciliationTests : TestKit, IClassFixture r.EventId).Distinct().Count()); - - Sys.Stop(reconciliationActor); } } } From d2d4db492bce8f40dedf696e7755be4017587c9b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:27:52 -0400 Subject: [PATCH 3/3] =?UTF-8?q?test(infra):=20close=20register=20row=2096?= =?UTF-8?q?=20=E2=80=94=20per-suite=20xunit=20parallelism=20caps=20for=20t?= =?UTF-8?q?he=20resource-heavy=20suites?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit maxParallelThreads was unset everywhere and only 3 of ~20 test projects carried an xunit.runner.json at all, so xunit ran Environment.ProcessorCount collections at once (14 on the dev box) in every suite. That is the general load multiplier behind the row-82/91/95 flake family. Policy: per-suite caps, NOT a global one. Pure unit suites stay uncapped for speed; a suite gets a config only when a test CLASS costs a real external or OS-level resource, so ProcessorCount of them at once is a resource question rather than a CPU question. Four suites qualify, all capped at maxParallelThreads: 4. - ConfigurationDatabase.Tests, AuditLog.Tests, SiteCallAudit.Tests — verified as the three that actually take MsSqlMigrationFixture (the latter two by project reference). Every MSSQL-backed test class means a CREATE DATABASE plus the ~100-migration chain against the one shared dev SQL Server. 4 is chosen to MATCH the default slot count of row 82's cross-process sp_getapplock migrate semaphore, so the in-process and cross-process limits agree rather than fighting. - Host.Tests — classes boot real ASP.NET Core hosts and real ActorSystems; Akka.Remote binds real sockets, Kestrel binds real ports, gRPC dials them. Threads are capped, collections are NOT serialized. IntegrationTests (parallelizeAssembly/parallelizeTestCollections false) and CentralUI.PlaywrightTests (maxParallelThreads 1) already carry stricter pre-existing settings and are left alone — the Playwright suite drives a browser against a live rig, where shared UI state, not CPU, is the constraint. Each capped csproj gains ; the SDK does not copy it automatically and without it the file is silently ignored. Verified by moving the copied file out of bin/ to take an uncapped baseline: ConfigurationDatabase.Tests ran at 216 % CPU uncapped vs 145 % capped, so the cap demonstrably takes effect. Cost is close to nil — these suites are latency-bound on an external resource, not CPU-bound (14-25 % utilisation for the two slowest). Standalone, uncapped -> capped: AuditLog 383 tests 1m52s -> 1m51s; Host 520 tests 4m05s -> 4m08s; ConfigurationDatabase 396 tests 10s -> 12s; SiteCallAudit 66 tests 8s -> 8s. All green. docs/test_infra/test_infra.md gains a Test Parallelism Policy section: the two axes (in-process collection threads = this cap; cross-process solution-level dotnet test = unbounded in general, bounded on the DB-migration path by the sp_getapplock semaphore), the per-suite table with reasons, the rules (cap threads rather than serialize; a cap is not a substitute for test isolation; the Content copy is required), and the default for new projects (no config until the project acquires a per-class external resource). test_infra_db.md cross-links it from the semaphore bullet. --- docs/test_infra/test_infra.md | 58 +++++++++++++++++++ docs/test_infra/test_infra_db.md | 5 +- ...B.MOM.WW.ScadaBridge.AuditLog.Tests.csproj | 16 +++++ .../xunit.runner.json | 4 ++ ...aBridge.ConfigurationDatabase.Tests.csproj | 17 ++++++ .../xunit.runner.json | 4 ++ .../ZB.MOM.WW.ScadaBridge.Host.Tests.csproj | 13 +++++ .../xunit.runner.json | 4 ++ ....WW.ScadaBridge.SiteCallAudit.Tests.csproj | 15 +++++ .../xunit.runner.json | 4 ++ 10 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/xunit.runner.json create mode 100644 tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/xunit.runner.json create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Host.Tests/xunit.runner.json create mode 100644 tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/xunit.runner.json diff --git a/docs/test_infra/test_infra.md b/docs/test_infra/test_infra.md index f78828d4..9aad5b4f 100644 --- a/docs/test_infra/test_infra.md +++ b/docs/test_infra/test_infra.md @@ -46,6 +46,64 @@ Each service has a dedicated document with configuration details, verification s - [test_infra_playwright.md](test_infra_playwright.md) — Playwright browser server (Central UI testing) - Traefik LB — see `docker/README.md` and `docker/traefik/` (runs with the cluster, not in `infra/`) +## Test Parallelism Policy + +There are **two independent parallelism axes**, and they are bounded by two different mechanisms. +Conflating them is what made the row-82/91/96 flake family hard to read: a fix on one axis leaves the +other untouched. + +| Axis | What runs at once | Bounded by | +|------|-------------------|-----------| +| **In-process** — xunit collection threads | Test collections *within one test assembly*. Default is `Environment.ProcessorCount` (14 on the current dev box). | `xunit.runner.json` → `maxParallelThreads`, per suite. This section. | +| **Cross-process** — `dotnet test` on the solution | One test *process* per test project, several at a time. xunit cannot see across them. | For the MS SQL migration path only: the machine-wide `sp_getapplock` counting semaphore in `MsSqlMigrationFixture` (4 slots, `SCADABRIDGE_MSSQL_TEST_MIGRATE_SLOTS`) — see [test_infra_db.md](test_infra_db.md). Nothing bounds it in general. | + +### Which suites carry a runner config, and why + +The cap is **per suite, not global**: pure unit suites (Commons, ScriptAnalysis, TemplateEngine, +Transport, …) stay uncapped for speed. A suite gets a config only when a test *class* costs a real +external or OS-level resource, so that `ProcessorCount` of them at once is a resource question rather +than a CPU question. + +| Suite | Setting | Why | +|-------|---------|-----| +| `ConfigurationDatabase.Tests` | `maxParallelThreads: 4` | Owns `MsSqlMigrationFixture`: every MSSQL-backed test **class** does a `CREATE DATABASE` plus the full ~100-migration chain against the one shared dev SQL Server. 4 matches the `sp_getapplock` slot count so the in-process and cross-process limits agree. | +| `AuditLog.Tests` | `maxParallelThreads: 4` | Same fixture (by project reference), plus most classes stand up an Akka TestKit `ActorSystem` with fast-ticking actors. | +| `SiteCallAudit.Tests` | `maxParallelThreads: 4` | Same fixture (by project reference) plus a TestKit `ActorSystem`. | +| `Host.Tests` | `maxParallelThreads: 4` | Classes boot real ASP.NET Core hosts and real Akka `ActorSystem`s — Akka.Remote binds real sockets, Kestrel binds real ports, gRPC channels dial them. | +| `IntegrationTests` | `parallelizeAssembly: false`, `parallelizeTestCollections: false` | Pre-existing. Fully serialized; a thread cap would be redundant. | +| `CentralUI.PlaywrightTests` | `maxParallelThreads: 1` | Pre-existing, and the strictest setting in the repo — the suite drives a browser against a **live rig**, so shared UI state, not CPU, is the constraint. | +| `StoreAndForward.Tests` | `parallelizeTestCollections: true` | Pre-existing; that is the default, so this file is a no-op pin. Uncapped by choice — SQLite files are per-test. | + +### Rules + +- **Cap threads, do not serialize.** `maxParallelThreads` throttles how many collections run at once; + `parallelizeTestCollections: false` runs them one at a time forever. Reach for the second only when a + suite has a genuinely global resource it cannot partition (`IntegrationTests` and the Playwright suite + are the only two, and both predate this policy). +- **A cap is not a substitute for test isolation.** Rows 82/91/95 were all *correctness* bugs — leaked + ticking actors, absolute assertions on process-global counters — that a cap only makes rarer. Fix the + test; the cap is for resource pressure. +- **The json must be copied to the output directory.** The .NET SDK does not do this automatically: + each capped `.csproj` carries + ``. Without it the file + is silently ignored and the suite runs uncapped. +- **Default for a new test project: no runner config.** Add one only when the project acquires a + per-class external resource (an MSSQL fixture, a real host/port, a browser). Record the reason in the + `.csproj` comment next to the `Content` item. + +### Measured cost + +The cap is close to free, because these suites are latency-bound on an external resource rather than +CPU-bound (CPU utilisation stayed at 14–25 % for the two slowest). Measured standalone on a 14-core dev +box, uncapped vs. `maxParallelThreads: 4`: + +| Suite | Tests | Uncapped | Capped at 4 | +|-------|-------|----------|-------------| +| `AuditLog.Tests` | 383 | 1 m 52 s | 1 m 51 s | +| `Host.Tests` | 520 | 4 m 5 s | 4 m 8 s | +| `ConfigurationDatabase.Tests` | 396 | 10 s | 12 s | +| `SiteCallAudit.Tests` | 66 | 8 s | 8 s | + ## Remote Test Infrastructure In addition to the local Docker services, the following remote services are available for testing against real AVEVA System Platform hardware. diff --git a/docs/test_infra/test_infra_db.md b/docs/test_infra/test_infra_db.md index 1cda1c23..249ed0ee 100644 --- a/docs/test_infra/test_infra_db.md +++ b/docs/test_infra/test_infra_db.md @@ -43,7 +43,10 @@ Three behaviours are worth knowing when you look at this server during a test ru solution-level `dotnet test` runs several MSSQL-backed test *processes* at once, so the throttle has to live on the shared server. Without it, dozens of simultaneous fresh-database migrations saturated the container and individual DDL statements blew past EF's default 30-second command timeout, - failing whole test classes at fixture construction (deferred-work register row 82). + failing whole test classes at fixture construction (deferred-work register row 82). The + *in-process* half of the same problem is bounded separately, by a `maxParallelThreads: 4` cap in + each MSSQL-backed suite's `xunit.runner.json` — deliberately the same number, so the two limits + agree. See [test_infra.md → Test Parallelism Policy](test_infra.md#test-parallelism-policy). - **Migration commands use a 300s timeout**, and CREATE/DROP DATABASE use 120s. Under a bounded queue the server is busy but progressing, so the correct response to a slow statement is to wait. - **Strays are swept.** Each test process drops, once at start and best-effort, any diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests.csproj index 337d93b2..492b5526 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests.csproj @@ -46,6 +46,22 @@ + + + + + + + + diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/xunit.runner.json b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/xunit.runner.json new file mode 100644 index 00000000..6366089b --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/xunit.runner.json @@ -0,0 +1,4 @@ +{ + "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json", + "maxParallelThreads": 4 +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ZB.MOM.WW.ScadaBridge.Host.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ZB.MOM.WW.ScadaBridge.Host.Tests.csproj index d6fb9a82..d757589c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ZB.MOM.WW.ScadaBridge.Host.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ZB.MOM.WW.ScadaBridge.Host.Tests.csproj @@ -28,6 +28,19 @@ + + + + + diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/xunit.runner.json b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/xunit.runner.json new file mode 100644 index 00000000..6366089b --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/xunit.runner.json @@ -0,0 +1,4 @@ +{ + "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json", + "maxParallelThreads": 4 +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.csproj index 8e017f90..293f95b5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.csproj @@ -36,6 +36,21 @@ + + + + +