test(siteruntime): close register row 91 — compile-cache asserts become key-scoped deltas

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).
2026-08-17 05:27:22 -04:00
parent 15e5887b52
commit 26e3a1b9a6
2 changed files with 105 additions and 24 deletions
@@ -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));
    }
 }
@@ -7,39 +7,98 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Scripts;
 /// <summary>
 /// Tests for the process-wide <see cref="SiteScriptCompileCache"/>. The cache is
 /// static/process-wide state, so this class shares the "SiteScriptCompileCache" xunit
-/// collection with <c>ScriptCompilationServiceTests</c> (no cross-class parallelism) and
-/// calls <see cref="SiteScriptCompileCache.Clear"/> at the top of each test.
+/// collection with <c>ScriptCompilationServiceTests</c> and <c>DeploymentWarmThenGateTests</c>
+/// (no cross-class parallelism between those three, so no concurrent
+/// <see cref="SiteScriptCompileCache.Clear"/>).
 /// </summary>
+/// <remarks>
+/// <para>
+/// <b>Deferred-work register row 91.</b> 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 <see cref="SiteScriptCompileCache.GetOrAdd"/> too, in parallel, and
+/// bumps the same process-global <see cref="SiteScriptCompileCache.Hits"/> /
+/// <see cref="SiteScriptCompileCache.Count"/>. Absolute assertions such as
+/// <c>Assert.Equal(1, Hits)</c> after a <c>Clear()</c> 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.
+/// </para>
+/// <para>
+/// 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 (<see cref="_uniq"/>),
+/// so no other class can collide with or serve them; and the global counters are only ever
+/// asserted as one-sided bounds (<c>&gt;= baseline + n</c>), 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.
+/// </para>
+/// </remarks>
 [Collection("SiteScriptCompileCache")]
 public class SiteScriptCompileCacheTests
 {
+    /// <summary>
+    /// Slack allowed on <see cref="SiteScriptCompileCache.Count"/> 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.
+    /// </summary>
+    private const int ForeignEntrySlack = 512;
+
+    /// <summary>Per-test-instance token making every cache key in this test uniquely ours.</summary>
+    private readonly string _uniq = Guid.NewGuid().ToString("N");
+
+    /// <summary>Builds a cache key body that no other test class in the assembly can produce.</summary>
+    private string Code(string body) => $"/*{_uniq}*/ {body}";
+
    private static ScriptCompilationResult Ok() =>
        ScriptCompilationResult.Succeeded(CSharpScript.Create<object?>("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");
    }