diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index c266f06a..ad22f311 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -128,7 +128,9 @@ flowchart TD > **The Roslyn `ScriptOptions` are process-static, and must stay that way**: the reference set backing every site-side compile is built once per process, never per compile. `ScriptOptions.WithReferences(Assembly[])` resolves each assembly through `MetadataReference.CreateFromFile`, which does **not** cache — each call mints a fresh `MetadataReference` owning an `AssemblyMetadata` → `PEReader` → `NativeHeapMemoryBlock`, an unmanaged copy of the assembly metadata that nothing disposes. Building the options per compile leaks native memory permanently: no GC reclaims it, and it is invisible to gcdump and to the managed allocation counters, so the node's working set grows without the GC heap growing. This was a live defect (a Site node at 2,885 MB working set with 150 MB of live GC heap, ~2,469 MB of it on the default process heap across ~6,700 undisposed `AssemblyMetadata` instances). Note this is orthogonal to the compile cache above — the cache dedupes *identical* script bodies, so it bounds nothing when the bodies differ. (It no longer clears wholesale on overflow: WP3.1 replaced that latency cliff with approximate-LRU batch eviction, see **Compile cache eviction** below.) Pinned by a reference-equality regression test rather than by watching memory. -> **Compile cache eviction is approximate LRU, not a wholesale clear (WP3.1)**: `SiteScriptCompileCache` is bounded at 1024 entries — deliberately smaller than the TemplateEngine verdict cache's 4096, because entries pin compiled assemblies rather than verdict strings. It used to `Clear()` **everything** on overflow. Instance scripts *and* trigger expressions share this cache, so on a site large enough to cross the bound every overflow discarded up to 1023 live compiled scripts and the next deploy or Instance-Actor start paid a full recompile storm **on actor threads**. Overflow now evicts only the oldest ⅛ (128 entries) by last-access stamp: one 1024-element scan instead of 1023 future recompiles, and hot entries survive. Recency is an `Interlocked` access sequence rather than a clock — deterministic for tests and immune to clock steps; hits update it lock-free, and only the (rare) eviction sweep takes a lock, double-checked so concurrent inserts do not stampede it. The sweep snapshots through `ConcurrentDictionary.ToArray()` and **not** LINQ over the dictionary: LINQ's `ToArray` picks the `ICollection.CopyTo` fast path, which throws when a concurrent insert lands between its Count read and its copy, and inserts are deliberately lock-free. +> **The compile cache is owned by the `ScriptCompilationService` singleton, not static (register row 100)**: `SiteScriptCompileCache` is an ordinary instance held by `ScriptCompilationService`, and "one Roslyn compile per script body per process" (N4) is exactly the statement that `AddSiteRuntime` registers that service as a **DI singleton** — the deploy gate (via `DeployCompileValidator`), every Instance Actor's `PreStart`, and `SharedScriptLibrary` all reach the one cache through the one service the Deployment Manager threads to them. The singleton lifetime is therefore load-bearing rather than incidental, and is pinned by a registration test (`SiteRuntimeServiceRegistrationTests`); downgrading it to Scoped/Transient would hand every consumer an empty cache and silently reintroduce WP3.1's recompile storms with no behavioural test failing. The class was previously `static`, which made its entry table and its `Hits`/`Count` counters one process-global blob every test class in the assembly wrote into; the `Clear()` method that existed only so tests could reset that blob is **removed** — it never had a production caller, and constructing a cache is the honest reset. + +> **Compile cache eviction is approximate LRU, not a wholesale clear (WP3.1)**: `SiteScriptCompileCache` is bounded at 1024 entries — deliberately smaller than the TemplateEngine verdict cache's 4096, because entries pin compiled assemblies rather than verdict strings. It used to clear **everything** on overflow. Instance scripts *and* trigger expressions share this cache, so on a site large enough to cross the bound every overflow discarded up to 1023 live compiled scripts and the next deploy or Instance-Actor start paid a full recompile storm **on actor threads**. Overflow now evicts only the oldest ⅛ (128 entries) by last-access stamp: one 1024-element scan instead of 1023 future recompiles, and hot entries survive. Recency is an `Interlocked` access sequence rather than a clock — deterministic for tests and immune to clock steps; hits update it lock-free, and only the (rare) eviction sweep takes a lock, double-checked so concurrent inserts do not stampede it. The sweep snapshots through `ConcurrentDictionary.ToArray()` and **not** LINQ over the dictionary: LINQ's `ToArray` picks the `ICollection.CopyTo` fast path, which throws when a concurrent insert lands between its Count read and its copy, and inserts are deliberately lock-free. > **Static options were only half the fix — the compile must also use the shared caching metadata resolver**: the static `ScriptOptions` carry only the *direct* API-surface references (5 assemblies here), and each `script.Compile()` still binds their **transitive closure** (~105 assemblies on a site node). Every transitively-referenced assembly is resolved through the options' `MetadataReferenceResolver`, and Roslyn's default one has no cross-compilation cache — it calls `MetadataReference.CreateFromFile` afresh on **every compile**, minting the same undisposed `AssemblyMetadata` → `PEReader` → `NativeHeapMemoryBlock` triple per assembly per compiled script, pinned for the process lifetime by the compile cache. That per-compile *re-resolution* — not the per-compile options construction — was the dominant term (measured live: 21 site scripts holding 6,640 metadata objects, counts bit-identical before and after the static-options fix). `ScriptCompilationService` therefore also attaches the process-wide `CachingScriptMetadataResolver.Instance` (owned by the Script Analysis component, see `Component-ScriptAnalysis.md`) via `ScriptOptions.WithMetadataResolver`, so each distinct assembly is materialized once per process. The decorator returns exactly what the undecorated resolver returned — only object identity is de-duplicated — so compile results and the trust gate are unchanged. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index be83b8ca..8282e4cf 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -677,7 +677,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { // Verdict discarded: startup does not gate on compile failures (a failing // script is logged by the Instance Actor and leaves the rest running). - // This call exists only to populate SiteScriptCompileCache. + // This call exists only to populate the compile cache owned by the + // shared ScriptCompilationService (SiteScriptCompileCache). validator.Validate(configJson); } catch @@ -798,8 +799,8 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers } /// - /// Compiles the deployment's scripts off the actor thread purely to populate the - /// process-wide , then pipes + /// Compiles the deployment's scripts off the actor thread purely to populate the compile + /// cache owned by the shared ScriptCompilationService singleton, then pipes /// back so the authoritative gate can run on the actor /// thread against a warm cache. The warm is best-effort: its verdict is discarded and any /// exception swallowed, because re-derives the @@ -910,9 +911,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // already been warmed off-thread (StartDeployCompileWarm), so this call is // pure cache hits and no longer holds the singleton — while the per-instance // warm guard keeps this instance's own command ordering intact. Redeploys and - // multi-instance deploys of unchanged scripts hit the process-wide compile - // cache (SiteScriptCompileCache) too, and the Instance Actor's PreStart reuses - // the gate's compile (N4). + // multi-instance deploys of unchanged scripts hit the compile cache + // (SiteScriptCompileCache, owned by the ScriptCompilationService singleton this + // actor shares with every Instance Actor) too, and the Instance Actor's PreStart + // reuses the gate's compile (N4). var errors = _deployCompileValidator.Validate(command.FlattenedConfigurationJson); if (errors.Count > 0) { diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs index 196a1eec..3e432233 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs @@ -13,12 +13,13 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Deployment; /// do not compile on the site (design spec: site-side compile failure rejects the /// deployment; no partial state is applied). /// -/// Because it holds no actor state and only calls the (stateless) -/// , it is pure and thread-safe; the Deployment -/// Manager nevertheless invokes it synchronously on the singleton's actor thread -/// as a pure prefix step, preserving mailbox-FIFO deploy ordering (see +/// Because it holds no actor state and only calls +/// (whose only state is a thread-safe compile cache), it is pure and thread-safe; the +/// Deployment Manager nevertheless invokes it synchronously on the singleton's actor +/// thread as a pure prefix step, preserving mailbox-FIFO deploy ordering (see /// ). Compiles of unchanged -/// script bodies are deduped by the process-wide compile cache. +/// script bodies are deduped by that cache, which is shared across the whole site because +/// the compilation service is a DI singleton. /// public sealed class DeployCompileValidator { diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs index 1d66655c..d9a7e73d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs @@ -14,22 +14,48 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; /// execution-path compile of the script against / /// . /// -/// Compile results — success AND failure — are memoised process-wide in +/// Compile results — success AND failure — are memoised in this service's /// (keyed on code hash + globals type), so an /// unchanged script body compiles once per process lifetime and the deploy gate, /// the Instance Actor's PreStart recompile, and SharedScriptLibrary all reuse one /// compiled Script<object?>. Error text is name-free by construction, so a /// cached failure renders correctly for every caller. +/// +/// The cache is owned per service instance, not static (register row 100). +/// "Once per process" is a property of this service being registered as a DI +/// singleton (, pinned by +/// SiteRuntimeServiceRegistrationTests) — the whole site reaches the cache through +/// that one instance, which the Deployment Manager threads into its +/// and into every Instance Actor. Making the +/// cache an instance field rather than static state means a test (or a second host in one +/// process) that constructs its own service gets its own cache instead of mutating everyone +/// else's; that shared-static cross-talk is what row 100 closed. /// public class ScriptCompilationService { private readonly ILogger _logger; + private readonly SiteScriptCompileCache _compileCache; /// Initializes a new instance of the ScriptCompilationService class. /// Logger instance. public ScriptCompilationService(ILogger logger) + : this(logger, new SiteScriptCompileCache()) + { + } + + /// + /// Test seam: constructs the service over a caller-supplied compile cache, so a test can + /// observe / + /// for exactly the compiles it caused. Production always uses the public constructor. + /// + /// Logger instance. + /// The compile cache this service memoises into. + internal ScriptCompilationService( + ILogger logger, + SiteScriptCompileCache compileCache) { _logger = logger; + _compileCache = compileCache; } /// @@ -136,16 +162,16 @@ public class ScriptCompilationService /// /// Shared compilation funnel for every site-side compile (the deploy gate, the /// Instance Actor's PreStart recompile, and SharedScriptLibrary). Results — success - /// AND failure — are memoised process-wide in , + /// AND failure — are memoised in this service's , /// keyed on code hash + globals type: an unchanged script body compiles once per - /// process lifetime and the compiled Script<object?> is shared across all - /// call sites (N4). Cached error text is name-free by construction (Roslyn diagnostics / - /// the compile-exception message never embed the caller's script name), so a cached - /// failure renders correctly for every caller. + /// process lifetime (this service is a DI singleton) and the compiled + /// Script<object?> is shared across all call sites (N4). Cached error text is + /// name-free by construction (Roslyn diagnostics / the compile-exception message never + /// embed the caller's script name), so a cached failure renders correctly for every caller. /// private ScriptCompilationResult CompileCore(string name, string code, Type globalsType) { - var result = SiteScriptCompileCache.GetOrAdd(code, globalsType, + var result = _compileCache.GetOrAdd(code, globalsType, () => CompileUncached(name, code, globalsType)); if (!result.IsSuccess) _logger.LogDebug( diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs index 768a6e9d..728879b9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs @@ -5,13 +5,25 @@ using System.Text; namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; /// -/// Process-wide cache of site-side script s, keyed on +/// Cache of site-side script s, keyed on /// the globals type plus a SHA-256 hash of the script code. Unlike the TemplateEngine's /// ScriptCompileVerdictCache (which caches central-validation verdicts), this /// caches the full compiled result — the immutable Roslyn Script<object?> itself — /// so the synchronous deploy gate and the InstanceActor.PreStart recompile share one /// Roslyn compile per script body per process lifetime (N4). /// +/// Owned by , not static (register row 100). +/// This used to be a static class, which made its Hits/Count counters and +/// its entry table one process-global blob that every test class in the assembly shared. The +/// process-wide N4 sharing property is unchanged in production — it now comes from +/// ScriptCompilationService being a DI singleton (ServiceCollectionExtensions, +/// pinned by SiteRuntimeServiceRegistrationTests), so exactly one cache exists per host and +/// the deploy gate, the Instance Actor's PreStart recompile and SharedScriptLibrary all reach it +/// through that one service. What changed is that a test constructing its own +/// ScriptCompilationService now gets its own cache instead of writing into everyone else's. +/// The old Clear() escape hatch is deliberately GONE: it had no production caller — it +/// existed only so tests could reset the global — and a fresh instance is the honest reset. +/// /// Soundness rests on three invariants: /// /// A compile result is a pure function of (code, globals type, compile-time-static @@ -30,7 +42,7 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; /// /// /// WP3.1 — approximate LRU replaced the overflow cliff. The cache used to -/// Clear() wholesale on overflow. Instance scripts AND trigger expressions share this +/// clear itself wholesale on overflow. Instance scripts AND trigger expressions share this /// cache, so on a site large enough to cross 1024 entries every overflow discarded up to 1023 /// live compiled scripts and the next deploy or Instance-Actor start paid a full recompile /// storm — on actor threads. Now an insert at the bound evicts only the oldest @@ -42,7 +54,7 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; /// only lock is the (rare) eviction sweep, double-checked so concurrent inserts do not /// stampede it. /// -internal static class SiteScriptCompileCache +internal sealed class SiteScriptCompileCache { /// Upper bound on cached entries; crossing it evicts the oldest batch. internal const int MaxEntries = 1024; @@ -54,18 +66,18 @@ internal static class SiteScriptCompileCache /// private const int EvictionBatchDivisor = 8; - private static readonly ConcurrentDictionary Cache = new(); - private static readonly object EvictionLock = new(); - private static long _hits; + private readonly ConcurrentDictionary _cache = new(); + private readonly object _evictionLock = new(); + private long _hits; /// Monotonic access sequence; the recency stamp written onto entries. - private static long _accessSequence; + private long _accessSequence; - /// Number of cache hits observed since the last . - public static long Hits => Interlocked.Read(ref _hits); + /// Number of cache hits observed by this cache instance. + public long Hits => Interlocked.Read(ref _hits); /// Current number of cached compile results. - public static int Count => Cache.Count; + public int Count => _cache.Count; /// /// Returns the cached compile result for against @@ -77,11 +89,11 @@ internal static class SiteScriptCompileCache /// The Roslyn globals surface the script compiles against; part of the key so identical source under different globals stays distinct. /// Computes the compile result on a cache miss. /// The cached or newly computed compile result. - public static ScriptCompilationResult GetOrAdd(string code, Type globalsType, Func factory) + public ScriptCompilationResult GetOrAdd(string code, Type globalsType, Func factory) { var key = globalsType.FullName + ":" + Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(code))); - if (Cache.TryGetValue(key, out var entry)) + if (_cache.TryGetValue(key, out var entry)) { Interlocked.Increment(ref _hits); Touch(entry); @@ -90,35 +102,28 @@ internal static class SiteScriptCompileCache var result = factory(); - if (Cache.Count >= MaxEntries) + if (_cache.Count >= MaxEntries) EvictOldestBatch(); - Cache[key] = new CacheEntry(result) { LastAccess = NextStamp() }; + _cache[key] = new CacheEntry(result) { LastAccess = NextStamp() }; return result; } - /// Clears all cached compile results and resets the hit counter. - public static void Clear() - { - Cache.Clear(); - Interlocked.Exchange(ref _hits, 0); - } - /// Refreshes an entry's recency stamp. Lock-free — a lost race only costs accuracy, never correctness. - private static void Touch(CacheEntry entry) => Volatile.Write(ref entry.LastAccess, NextStamp()); + private void Touch(CacheEntry entry) => Volatile.Write(ref entry.LastAccess, NextStamp()); - private static long NextStamp() => Interlocked.Increment(ref _accessSequence); + private long NextStamp() => Interlocked.Increment(ref _accessSequence); /// /// Evicts the oldest / entries - /// by recency stamp. Double-checked under so several concurrent + /// by recency stamp. Double-checked under so several concurrent /// inserts crossing the bound together perform ONE sweep rather than one each. /// - private static void EvictOldestBatch() + private void EvictOldestBatch() { - lock (EvictionLock) + lock (_evictionLock) { - if (Cache.Count < MaxEntries) return; // another thread already swept + if (_cache.Count < MaxEntries) return; // another thread already swept var batch = Math.Max(1, MaxEntries / EvictionBatchDivisor); @@ -128,14 +133,14 @@ internal static class SiteScriptCompileCache // copies, and throws ArgumentException when a concurrent insert lands between // the two. The eviction lock only excludes other EVICTORS — inserts run // lock-free by design — so the snapshot must be the thread-safe one. - var victims = Cache.ToArray() + var victims = _cache.ToArray() .OrderBy(kvp => Volatile.Read(ref kvp.Value.LastAccess)) .Take(batch) .Select(kvp => kvp.Key) .ToArray(); foreach (var victim in victims) - Cache.TryRemove(victim, out _); + _cache.TryRemove(victim, out _); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs index e94af366..033e228c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs @@ -32,7 +32,14 @@ public static class ServiceCollectionExtensions services.AddHostedService(); - // Script compilation service + // Script compilation service. + // + // SINGLETON IS LOAD-BEARING (register row 100): this service owns the + // SiteScriptCompileCache, so "one Roslyn compile per script body per process" + // (N4) is exactly the statement that there is one of these per host. Downgrading + // it to Scoped/Transient would silently give every consumer its own cache and + // reintroduce the recompile storms WP3.1 removed — no test would fail on + // behaviour, only the registration pin in SiteRuntimeServiceRegistrationTests. services.AddSingleton(); // Shared script library 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 7028e886..2a3ed54c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentWarmThenGateTests.cs @@ -29,13 +29,17 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors; /// answer its deployer instead of leaving it to Ask-timeout, and commands for DIFFERENT /// instances must not block each other. /// -/// Shares the SiteScriptCompileCache collection because the batch pre-warm test -/// asserts on that process-wide cache's hit counter. +/// The batch pre-warm test asserts on the compile cache's hit counter. As of register +/// row 100 that cache is owned by the ScriptCompilationService instance this fixture +/// builds — the one it threads into the Deployment Manager and, through it, into every Instance +/// Actor — so the counter is private to the test. The old +/// [Collection("SiteScriptCompileCache")] serialisation against the two +/// Scripts/ test classes is therefore gone: nothing here is shared any more. /// -[Collection("SiteScriptCompileCache")] public class DeploymentWarmThenGateTests : TestKit, IDisposable { private readonly SiteStorageService _storage; + private readonly SiteScriptCompileCache _compileCache; private readonly ScriptCompilationService _compilationService; private readonly SharedScriptLibrary _sharedScriptLibrary; private readonly TestLocalDb _localDb; @@ -45,8 +49,9 @@ public class DeploymentWarmThenGateTests : TestKit, IDisposable _localDb = TestLocalDb.CreateTemp("dm-warm-gate-test"); _storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); _storage.InitializeAsync().GetAwaiter().GetResult(); + _compileCache = new SiteScriptCompileCache(); _compilationService = new ScriptCompilationService( - NullLogger.Instance); + NullLogger.Instance, _compileCache); _sharedScriptLibrary = new SharedScriptLibrary( _compilationService, NullLogger.Instance); } @@ -247,21 +252,23 @@ public class DeploymentWarmThenGateTests : TestKit, IDisposable await _storage.StoreDeployedConfigAsync( "BatchTwo", ConfigJson("BatchTwo", sharedCode), "d2", "h2", true); - SiteScriptCompileCache.Clear(); - // 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; + // No Clear() and no baseline subtraction: this cache is the fixture's own and nothing + // has compiled into it yet (register rows 91/100 — the counter used to be a + // process-global that other test classes bumped in parallel, forcing a delta-over- + // baseline reading). Only the Deployment Manager below writes to it. + Assert.Equal(0, _compileCache.Hits); CreateDeploymentManager(); AwaitAssert(() => { // One compile, then repeated hits: the second pre-warm plus both PreStarts. - Assert.True(SiteScriptCompileCache.Hits - hitsBefore >= 3, - $"expected the pre-warmed body to be served from cache, saw " + - $"{SiteScriptCompileCache.Hits - hitsBefore} hits over baseline {hitsBefore}"); + Assert.True(_compileCache.Hits >= 3, + $"expected the pre-warmed body to be served from cache, saw {_compileCache.Hits} hits"); }, TimeSpan.FromSeconds(20)); + + // …and exactly one entry for the one shared body: the hits above were the SAME compiled + // script being reused, not several bodies each compiled once. + Assert.Equal(1, _compileCache.Count); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptCompilationServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptCompilationServiceTests.cs index 5b8dff9d..fd2a70d7 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptCompilationServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptCompilationServiceTests.cs @@ -15,15 +15,20 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Scripts; /// System.Diagnostics.Process is blocked under System.Diagnostics — /// Stopwatch stays allowed. The real execution-path compile against /// ScriptGlobals / TriggerExpressionGlobals is unchanged. +/// +/// Register row 100: the compile cache is owned by the service instance, not +/// static, so this class's service — and therefore its cache — is private to each test. The +/// [Collection("SiteScriptCompileCache")] serialisation and the Clear() calls that +/// used to open the cache-sensitive tests are gone; Hits is now exact. /// -[Collection("SiteScriptCompileCache")] public class ScriptCompilationServiceTests { + private readonly SiteScriptCompileCache _cache = new(); private readonly ScriptCompilationService _service; public ScriptCompilationServiceTests() { - _service = new ScriptCompilationService(NullLogger.Instance); + _service = new ScriptCompilationService(NullLogger.Instance, _cache); } [Fact] @@ -38,28 +43,27 @@ public class ScriptCompilationServiceTests [Fact] public void Compile_SameCodeTwice_SharesOneRoslynCompile() { - SiteScriptCompileCache.Clear(); var r1 = _service.Compile("deploy-gate-copy", "return 1 + 1;"); var r2 = _service.Compile("prestart-copy", "return 1 + 1;"); Assert.True(r1.IsSuccess); Assert.Same(r1.CompiledScript, r2.CompiledScript); // one compile, shared Script (N4) — the definitive proof - // Hits is a process-global counter; other test classes in this assembly compile scripts - // concurrently (they are not in this serialized collection), so the exact post-Clear count - // is not deterministic under a full-assembly parallel run. The shared-Script assertion - // above is the real proof of cache reuse; here we only require the second lookup registered - // a hit (>= 1) rather than pinning an exact global count. - Assert.True(SiteScriptCompileCache.Hits >= 1); + // The cache belongs to this service instance (register row 100), so the hit count is + // exactly the one the second Compile caused — it used to be a >= 1 lower bound on a + // process-global counter other test classes were bumping in parallel. + Assert.Equal(1, _cache.Hits); + Assert.Equal(1, _cache.Count); } [Fact] public void Compile_ScriptAndTriggerExpression_DoNotCrossContaminate() { - SiteScriptCompileCache.Clear(); var script = _service.Compile("s", "1 > 0"); var trigger = _service.CompileTriggerExpression("t", "1 > 0"); Assert.NotSame(script.CompiledScript, trigger.CompiledScript); // different globals surfaces + Assert.Equal(2, _cache.Count); // two entries, no cross-globals hit + Assert.Equal(0, _cache.Hits); } [Fact] @@ -231,7 +235,6 @@ public class ScriptCompilationServiceTests [Fact] public void Compile_DistinctScripts_ShareOneMetadataReferenceSet_SoNativeMemoryDoesNotGrow() { - SiteScriptCompileCache.Clear(); var first = _service.Compile("first", "return 1 + 1;"); var second = _service.Compile("second", "return 2 + 2;"); @@ -255,7 +258,6 @@ public class ScriptCompilationServiceTests [Fact] public void Compile_UsesProcessWideCachingMetadataResolver() { - SiteScriptCompileCache.Clear(); var result = _service.Compile("resolver-pin", "return 41 + 1;"); Assert.True(result.IsSuccess); 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 125b415b..7b5af38f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs @@ -5,48 +5,34 @@ using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; 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 and DeploymentWarmThenGateTests -/// (no cross-class parallelism between those three, so no concurrent -/// ). +/// Tests for . /// /// /// -/// 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. +/// Deferred-work register row 100 — the cache is no longer static. Every test here builds +/// its OWN , so and +/// count this test's compiles and nothing else. That +/// retires three workarounds this file used to carry: /// +/// +/// the [Collection("SiteScriptCompileCache")] serialisation with +/// ScriptCompilationServiceTests and DeploymentWarmThenGateTests — those three +/// classes shared a mutable global and had to be kept off each other; they no longer touch any +/// shared state and run fully in parallel again; +/// the per-test GUID key token (register row 91) — no other class can reach this cache at +/// all, so keys need not be globally unique; +/// the one-sided counter bounds and the ForeignEntrySlack = 512 allowance on +/// Count — foreign inserts are impossible now, so every assertion below is exact. +/// /// -/// 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. +/// The old escape hatch, Clear(), is gone from the type: it had no production caller and +/// existed only to reset the global between tests. Constructing a cache is the reset. /// /// -[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}"; + /// The cache under test. Private to this test instance — xunit builds one per test. + private readonly SiteScriptCompileCache _cache = new(); private static ScriptCompilationResult Ok() => ScriptCompilationResult.Succeeded(CSharpScript.Create("1", ScriptOptions.Default, typeof(ScriptGlobals))); @@ -54,72 +40,94 @@ public class SiteScriptCompileCacheTests [Fact] public void GetOrAdd_SameCodeAndGlobals_ComputesOnce() { - var code = Code("return 1;"); - var hitsBefore = SiteScriptCompileCache.Hits; var factoryCalls = 0; ScriptCompilationResult Factory() { factoryCalls++; return Ok(); } - var r1 = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), Factory); - var r2 = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), Factory); + var r1 = _cache.GetOrAdd("return 1;", typeof(ScriptGlobals), Factory); + var r2 = _cache.GetOrAdd("return 1;", 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); - - // 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}"); + Assert.Equal(1, _cache.Hits); + Assert.Equal(1, _cache.Count); } [Fact] public void GetOrAdd_SameCode_DifferentGlobals_AreSeparateEntries() { - var code = Code("1 > 0"); var scriptCompiles = 0; var triggerCompiles = 0; - var script = SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), + var script = _cache.GetOrAdd("1 > 0", typeof(ScriptGlobals), () => { scriptCompiles++; return Ok(); }); - var trigger = SiteScriptCompileCache.GetOrAdd(code, typeof(TriggerExpressionGlobals), + var trigger = _cache.GetOrAdd("1 > 0", 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), + // Two distinct entries and no cross-globals hit — exact, because this cache is ours. + Assert.Equal(2, _cache.Count); + Assert.Equal(0, _cache.Hits); + + // Each entry stays independently addressable afterwards. + Assert.Same(script, _cache.GetOrAdd("1 > 0", typeof(ScriptGlobals), () => throw new InvalidOperationException("script entry was not cached"))); - Assert.Same(trigger, SiteScriptCompileCache.GetOrAdd(code, typeof(TriggerExpressionGlobals), + Assert.Same(trigger, _cache.GetOrAdd("1 > 0", typeof(TriggerExpressionGlobals), () => throw new InvalidOperationException("trigger entry was not cached"))); } + /// + /// Register row 100: two caches are genuinely independent stores. This is the property the + /// whole change exists for — one test class's cache activity can no longer evict, serve, or + /// be counted by another's. It is the direct replacement for the old + /// Clear()-under-a-serialized-collection arrangement. + /// + [Fact] + public void SeparateCaches_DoNotShareEntriesOrCounters() + { + var other = new SiteScriptCompileCache(); + + var mine = _cache.GetOrAdd("return 1;", typeof(ScriptGlobals), Ok); + _cache.GetOrAdd("return 1;", typeof(ScriptGlobals), Ok); // a hit on OUR cache + + // The other cache has neither the entry nor the hit. + Assert.Equal(0, other.Count); + Assert.Equal(0, other.Hits); + + // Same key, other store: it must compute its own value, and get a distinct instance. + var theirCompiles = 0; + var theirs = other.GetOrAdd("return 1;", typeof(ScriptGlobals), + () => { theirCompiles++; return Ok(); }); + Assert.Equal(1, theirCompiles); + Assert.NotSame(mine, theirs); + + // …and filling the other cache past its bound — the closest thing left to the old + // wholesale Clear() — leaves ours completely untouched. + for (var i = 0; i <= SiteScriptCompileCache.MaxEntries; i++) + other.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); + + Assert.Equal(1, _cache.Count); + Assert.Equal(1, _cache.Hits); + Assert.Same(mine, _cache.GetOrAdd("return 1;", typeof(ScriptGlobals), + () => throw new InvalidOperationException("our entry was evicted by another cache"))); + } + [Fact] public void Overflow_StaysWithinTheBound() { - SiteScriptCompileCache.Clear(); for (var i = 0; i <= SiteScriptCompileCache.MaxEntries; i++) - SiteScriptCompileCache.GetOrAdd(Code($"return {i};"), typeof(ScriptGlobals), Ok); + _cache.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); - // 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}"); + // MaxEntries + 1 distinct inserts, single-threaded: the sweep ran and the ceiling holds + // exactly (no foreign inserts can land here any more — register row 100). + Assert.True(_cache.Count <= SiteScriptCompileCache.MaxEntries, + $"cache grew past its bound: {_cache.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(); }); - } + _cache.GetOrAdd($"return {i};", typeof(ScriptGlobals), () => { recomputed++; return Ok(); }); Assert.True(recomputed > 0, "no oldest-batch entry was evicted — the overflow sweep did not run"); } @@ -134,50 +142,37 @@ public class SiteScriptCompileCacheTests [Fact] public void Overflow_EvictsOldestBatch_AndKeepsRecentlyTouchedEntries() { - // Note: the cache is process-wide static and other test classes compile into it - // concurrently, so the assertions below are phrased as properties of the eviction - // policy (what survives, what does not, nothing is wiped) rather than as exact - // counts, which no test can own here. - SiteScriptCompileCache.Clear(); - // Fill to the bound. Entries 1..N are inserted oldest-first. for (var i = 0; i < SiteScriptCompileCache.MaxEntries; i++) - SiteScriptCompileCache.GetOrAdd(Code($"return {i};"), typeof(ScriptGlobals), Ok); + _cache.GetOrAdd($"return {i};", typeof(ScriptGlobals), Ok); // Touch entry 0 so it carries the NEWEST access stamp despite being inserted first. - var hot = SiteScriptCompileCache.GetOrAdd(Code("return 0;"), typeof(ScriptGlobals), Ok); - var hitsAfterTouch = SiteScriptCompileCache.Hits; + var hot = _cache.GetOrAdd("return 0;", typeof(ScriptGlobals), Ok); + var hitsAfterTouch = _cache.Hits; + Assert.Equal(1, hitsAfterTouch); // exactly one hit so far: the touch // Push well past the bound so at least one eviction sweep definitely runs. for (var i = 0; i < 200; i++) - SiteScriptCompileCache.GetOrAdd(Code($"return overflow{i};"), typeof(ScriptGlobals), Ok); + _cache.GetOrAdd($"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). - Assert.True(SiteScriptCompileCache.Count > SiteScriptCompileCache.MaxEntries / 2, - $"cache collapsed to {SiteScriptCompileCache.Count} entries — this looks like a wholesale clear"); - Assert.True(SiteScriptCompileCache.Hits >= hitsAfterTouch); + // counter survived (a wholesale clear used to reset it to 0). + Assert.True(_cache.Count > SiteScriptCompileCache.MaxEntries / 2, + $"cache collapsed to {_cache.Count} entries — this looks like a wholesale clear"); + Assert.Equal(hitsAfterTouch, _cache.Hits); // 200 distinct inserts, none of them hits // 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(Code("return 0;"), typeof(ScriptGlobals), + var again = _cache.GetOrAdd("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.True(SiteScriptCompileCache.Hits >= hitsBefore + 1, - $"expected the surviving hot entry to register a hit; hits went {hitsBefore} -> {SiteScriptCompileCache.Hits}"); + Assert.Equal(hitsAfterTouch + 1, _cache.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 = Code($"return {i};"); - SiteScriptCompileCache.GetOrAdd(code, typeof(ScriptGlobals), - () => { recomputed++; return Ok(); }); - } + _cache.GetOrAdd($"return {i};", typeof(ScriptGlobals), () => { recomputed++; return Ok(); }); + Assert.True(recomputed > 0, "no old entry was evicted — the overflow sweep did not run"); } @@ -189,26 +184,25 @@ public class SiteScriptCompileCacheTests [Fact] public async Task ConcurrentGetOrAddStorm_KeepsTheBound() { - SiteScriptCompileCache.Clear(); - const int perTask = 400; - var tasks = Enumerable.Range(0, 8).Select(t => Task.Run(() => + const int tasks = 8; + + await Task.WhenAll(Enumerable.Range(0, tasks).Select(t => Task.Run(() => { for (var i = 0; i < perTask; i++) - SiteScriptCompileCache.GetOrAdd(Code($"return {t}_{i};"), typeof(ScriptGlobals), Ok); - })); - - await Task.WhenAll(tasks); + _cache.GetOrAdd($"return {t}_{i};", typeof(ScriptGlobals), Ok); + }))); // A small transient overshoot is by design: threads that lose the double-checked // eviction race still insert their own entry afterwards, so the ceiling is "bounded", - // not "never exceeded by one". The property under test is that 3200 distinct inserts - // do not accumulate — before WP3.1 this was a wholesale Clear(), and a per-entry - // 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 + ForeignEntrySlack, - $"cache grew past its bound: {SiteScriptCompileCache.Count} vs max {SiteScriptCompileCache.MaxEntries} " + - $"after {8 * perTask} distinct inserts"); + // not "never exceeded by one". The slack is bounded by the number of racing threads + // (register row 100 — it used to be 512, to absorb inserts from other test classes + // sharing the process-global cache; nothing foreign can reach this instance). + // The property under test is that 3200 distinct inserts do not accumulate — before + // WP3.1 this was a wholesale clear, and a per-entry eviction bug would show up here as + // unbounded growth, not as a handful of extra rows. + Assert.True(_cache.Count <= SiteScriptCompileCache.MaxEntries + tasks, + $"cache grew past its bound: {_cache.Count} vs max {SiteScriptCompileCache.MaxEntries} " + + $"(+{tasks} racing-insert slack) after {tasks * perTask} distinct inserts"); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/SiteRuntimeServiceRegistrationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/SiteRuntimeServiceRegistrationTests.cs new file mode 100644 index 00000000..14987c5c --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/SiteRuntimeServiceRegistrationTests.cs @@ -0,0 +1,36 @@ +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.SiteRuntime; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests; + +/// +/// Pins the DI lifetimes that carry a behavioural contract rather than a convention. +/// +public class SiteRuntimeServiceRegistrationTests +{ + /// + /// Register row 100. ScriptCompilationService owns the + /// SiteScriptCompileCache, so "one Roslyn compile per script body per process" + /// (N4 / WP3.1's warm-then-gate deploy path) is precisely the statement that DI hands + /// out ONE of these per host. The cache stopped being static when row 100 removed + /// the process-global state tests were clobbering; this registration is what replaced + /// the static as the guarantee, so it is pinned here. + /// + /// Downgrading this to Scoped or Transient would give every consumer — the deploy + /// gate, each Instance Actor's PreStart, SharedScriptLibrary — its own empty cache and + /// silently reintroduce the recompile storms WP3.1 removed, with no functional test + /// failing. This assertion is the only thing that would notice. + /// + [Fact] + public void AddSiteRuntime_RegistersScriptCompilationServiceAsSingleton_SoOneCompileCacheExistsPerProcess() + { + var services = new ServiceCollection(); + + services.AddSiteRuntime(); + + var descriptor = Assert.Single( + services, d => d.ServiceType == typeof(ScriptCompilationService)); + Assert.Equal(ServiceLifetime.Singleton, descriptor.Lifetime); + } +}