From 96cab3f3d700e1aa9be9c8cc358c1cac23e1f16b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 08:28:22 -0400 Subject: [PATCH] fix(site-runtime): compile cache eviction counter+warning, configurable bound, never cache faulted compiles (review 2026-10-21 01/SR-M2) --- .../Scripts/ScriptCompilationService.cs | 27 ++++++-- .../Scripts/SiteScriptCompileCache.cs | 69 +++++++++++++++++-- .../SiteRuntimeOptions.cs | 8 +++ .../SiteRuntimeOptionsValidator.cs | 4 ++ .../Scripts/SiteScriptCompileCacheTests.cs | 44 ++++++++++++ 5 files changed, 144 insertions(+), 8 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs index 85994a36..6fca8970 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptCompilationService.cs @@ -38,8 +38,12 @@ public class ScriptCompilationService /// Initializes a new instance of the ScriptCompilationService class. /// Logger instance. - public ScriptCompilationService(ILogger logger) - : this(logger, new SiteScriptCompileCache()) + /// Site runtime options; supplies the compile-cache bound. Optional (default bound when null). + public ScriptCompilationService( + ILogger logger, + Microsoft.Extensions.Options.IOptions? options = null) + : this(logger, new SiteScriptCompileCache( + options?.Value.ScriptCompileCacheMaxEntries ?? SiteScriptCompileCache.MaxEntries, logger)) { } @@ -228,7 +232,9 @@ public class ScriptCompilationService catch (Exception ex) { _logger.LogError(ex, "Unexpected error compiling script {Script}", name); - return ScriptCompilationResult.Failed([$"Compilation exception: {ex.Message}"]); + // Faulted, not Failed: an unexpected exception says nothing about the code, so the + // cache must not memoise it (a transient fault would otherwise stick until eviction/restart). + return ScriptCompilationResult.Faulted([$"Compilation exception: {ex.Message}"]); } } } @@ -245,8 +251,15 @@ public class ScriptCompilationResult /// List of error messages, empty if compilation succeeded. public IReadOnlyList Errors { get; } - private ScriptCompilationResult(bool success, Script? script, IReadOnlyList errors) + /// + /// False for a result produced by an unexpected exception during compilation; such a result + /// is returned to the caller but never memoised by . + /// + internal bool IsCacheable { get; } + + private ScriptCompilationResult(bool success, Script? script, IReadOnlyList errors, bool cacheable = true) { + IsCacheable = cacheable; IsSuccess = success; CompiledScript = script; Errors = errors; @@ -263,6 +276,12 @@ public class ScriptCompilationResult /// A with set to false and the provided error messages. public static ScriptCompilationResult Failed(IReadOnlyList errors) => new(false, null, errors); + + /// Creates a failure caused by an unexpected compiler fault; not cacheable. + /// List of error messages. + /// A non-cacheable failed result. + internal static ScriptCompilationResult Faulted(IReadOnlyList errors) => + new(false, null, errors, cacheable: false); } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs index 09729720..f64a9444 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/SiteScriptCompileCache.cs @@ -1,6 +1,7 @@ using System.Collections.Concurrent; using System.Security.Cryptography; using System.Text; +using Microsoft.Extensions.Logging; namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; @@ -56,9 +57,15 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; /// internal sealed class SiteScriptCompileCache { - /// Upper bound on cached entries; crossing it evicts the oldest batch. + /// + /// Default upper bound on cached entries; crossing it evicts the oldest batch. The live bound + /// is , configurable via SiteRuntimeOptions.ScriptCompileCacheMaxEntries. + /// internal const int MaxEntries = 1024; + /// Minimum interval between eviction Warning logs (a churning cache evicts constantly). + private static readonly TimeSpan EvictionWarnInterval = TimeSpan.FromMinutes(1); + /// /// Fraction of the cache evicted in one sweep (⅛ = 128 entries at /// ). Batching amortises the O(n) scan across many inserts, so @@ -69,10 +76,38 @@ internal sealed class SiteScriptCompileCache private readonly ConcurrentDictionary _cache = new(); private readonly object _evictionLock = new(); private long _hits; + private long _evictions; + private long _evictionsAtLastWarn; + private long _lastWarnTicks; + private readonly int _capacity; + private readonly ILogger? _logger; /// Monotonic access sequence; the recency stamp written onto entries. private long _accessSequence; + /// Creates a cache bounded at . + public SiteScriptCompileCache() : this(MaxEntries, null) { } + + /// Creates a cache with an explicit bound and an optional logger for eviction warnings. + /// Maximum cached entries (clamped to at least 8). + /// Receives the rate-limited eviction Warning; may be null. + public SiteScriptCompileCache(int capacity, ILogger? logger) + { + _capacity = Math.Max(8, capacity); + _logger = logger; + } + + /// The live entry bound for this cache instance. + public int Capacity => _capacity; + + /// + /// Total entries evicted since construction. Eviction discards only the managed + /// Script<object?>; the assembly Roslyn loaded for it stays in the default load context + /// for the process lifetime, and a later miss for the same body loads a second copy. A rising value + /// therefore means monotonic working-set growth — raise ScriptCompileCacheMaxEntries. + /// + public long Evictions => Interlocked.Read(ref _evictions); + /// Number of cache hits observed by this cache instance. public long Hits => Interlocked.Read(ref _hits); @@ -102,7 +137,12 @@ internal sealed class SiteScriptCompileCache var result = factory(); - if (_cache.Count >= MaxEntries) + // A faulted compile (unexpected exception, e.g. a transient OOM) is NOT a verdict on the + // code, so it must not become sticky: only success and diagnostic failures are cached. + if (!result.IsCacheable) + return result; + + if (_cache.Count >= _capacity) EvictOldestBatch(); _cache[key] = new CacheEntry(result) { LastAccess = NextStamp() }; @@ -123,9 +163,9 @@ internal sealed class SiteScriptCompileCache { lock (_evictionLock) { - if (_cache.Count < MaxEntries) return; // another thread already swept + if (_cache.Count < _capacity) return; // another thread already swept - var batch = Math.Max(1, MaxEntries / EvictionBatchDivisor); + var batch = Math.Max(1, _capacity / EvictionBatchDivisor); // ConcurrentDictionary.ToArray() takes an ATOMIC snapshot. Enumerating (or // LINQ-ing) the dictionary directly does not: LINQ's ToArray picks the @@ -141,9 +181,30 @@ internal sealed class SiteScriptCompileCache foreach (var victim in victims) _cache.TryRemove(victim, out _); + + Interlocked.Add(ref _evictions, victims.Length); + WarnEvicted(); } } + /// Rate-limited eviction Warning (first sweep, then at most one summary per interval). Caller holds the eviction lock. + private void WarnEvicted() + { + if (_logger is null) return; + var now = Environment.TickCount64; + if (_lastWarnTicks != 0 && now - _lastWarnTicks < (long)EvictionWarnInterval.TotalMilliseconds) return; + + var total = Interlocked.Read(ref _evictions); + var sinceLast = total - _evictionsAtLastWarn; + _evictionsAtLastWarn = total; + _lastWarnTicks = now == 0 ? 1 : now; + _logger.LogWarning( + "Script compile cache is at its {Capacity}-entry bound and evicted {Evicted} entries ({Total} total). " + + "Eviction does not unload the Roslyn assembly: each recompile of an evicted body loads another permanent " + + "copy. Raise ScadaBridge:SiteRuntime:ScriptCompileCacheMaxEntries if this repeats.", + _capacity, sinceLast, total); + } + /// /// A cached compile result plus its recency stamp. A class (not a struct) so /// can update recency in place without replacing the dictionary diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptions.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptions.cs index a7b4f2dd..597367cb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptions.cs @@ -62,6 +62,14 @@ public class SiteRuntimeOptions /// public int ScriptExecutionMaxThreadCount { get; set; } = 32; + /// + /// Maximum entries in the site script compile cache (scripts + trigger expressions, keyed on + /// body hash). Crossing it evicts the oldest eighth. Size it above the site's count of + /// DISTINCT script/trigger bodies: eviction never unloads the Roslyn assembly, so a body that + /// is evicted and later recompiled loads a second permanent copy. Default: 1024. + /// + public int ScriptCompileCacheMaxEntries { get; set; } = 1024; + /// /// Maximum number of trigger-expression evaluations allowed to run /// concurrently across the whole process. Trigger expressions are non-blocking by diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptionsValidator.cs index 7773904d..44c1d913 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/SiteRuntimeOptionsValidator.cs @@ -47,6 +47,10 @@ public sealed class SiteRuntimeOptionsValidator : OptionsValidatorBase= 8, + $"ScadaBridge:SiteRuntime:ScriptCompileCacheMaxEntries must be at least 8 " + + $"(was {options.ScriptCompileCacheMaxEntries}); it bounds the compiled-script cache."); + builder.RequireThat(options.TriggerEvalMaxConcurrency > 0, $"ScadaBridge:SiteRuntime:TriggerEvalMaxConcurrency must be greater than 0 " + $"(was {options.TriggerEvalMaxConcurrency}); it gates concurrent trigger-expression evaluations " + 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 0f8f1f8d..f28e568a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/SiteScriptCompileCacheTests.cs @@ -209,4 +209,48 @@ public class SiteScriptCompileCacheTests $"cache grew past its bound: {_cache.Count} vs max {SiteScriptCompileCache.MaxEntries} " + $"(+{tasks} racing-insert slack) after {tasks * perTask} distinct inserts"); } + + /// A faulted compile (unexpected exception) is returned but never cached: the next call recompiles (review 2026-10-21 01/SR-M2 follow-up). + [Fact] + public void GetOrAdd_FaultedResult_IsNotCached_NextCallRecompiles() + { + var calls = 0; + ScriptCompilationResult Factory() => + ++calls == 1 ? ScriptCompilationResult.Faulted(["Compilation exception: boom"]) : Ok(); + + var first = _cache.GetOrAdd("return 7;", typeof(ScriptGlobals), Factory); + Assert.False(first.IsSuccess); + Assert.Equal(0, _cache.Count); + + var second = _cache.GetOrAdd("return 7;", typeof(ScriptGlobals), Factory); + Assert.True(second.IsSuccess); + Assert.Equal(2, calls); + Assert.Equal(1, _cache.Count); + } + + /// A diagnostic failure IS a verdict on the code and stays cached. + [Fact] + public void GetOrAdd_DiagnosticFailure_IsStillCached() + { + var calls = 0; + ScriptCompilationResult Factory() { calls++; return ScriptCompilationResult.Failed(["CS1002"]); } + + _cache.GetOrAdd("bad", typeof(ScriptGlobals), Factory); + _cache.GetOrAdd("bad", typeof(ScriptGlobals), Factory); + + Assert.Equal(1, calls); + } + + /// A configured capacity bounds the cache and eviction is counted (01/SR-M2). + [Fact] + public void Eviction_HonoursConfiguredCapacity_AndCountsEvictions() + { + var cache = new SiteScriptCompileCache(16, logger: null); + for (var i = 0; i < 40; i++) + cache.GetOrAdd("return " + i + ";", typeof(ScriptGlobals), Ok); + + Assert.Equal(16, cache.Capacity); + Assert.True(cache.Count <= 16); + Assert.True(cache.Evictions > 0); + } }