fix(vtags): real script timeout + ALC-safe compile cache (arch-review 02/U2,U3)

U2 (Critical): RoslynVirtualTagEvaluator ran scripts with a CancellationToken that a synchronous Roslyn script can never observe, so one CPU-bound/infinite- loop virtual-tag script hung the owning VirtualTagActor forever (no timeout, no recovery). Route evaluation through the existing TimedScriptEvaluator (Task.Run + WaitAsync) so the wall-clock budget is real; a runaway script now returns Bad within the timeout instead of wedging the actor. U3 (High): the live path cached evaluators in a raw ConcurrentDictionary never cleared on republish, leaking a collectible AssemblyLoadContext per edited script forever. Swap in the purpose-built CompiledScriptCache (Lazy single- compile, dispose-on-Clear) and add an apply-boundary clear: a new narrow IScriptCacheOwner seam the VirtualTagHostActor calls on each ApplyVirtualTags generation (mirrors ScriptedAlarmEngine's per-generation clear). Tests: infinite-loop-fails-within-timeout (bounded by WaitAsync so a regression fails instead of hanging), happy-path-after-wrapping, ClearCompiledScripts- empties-cache, and a host-actor wiring guard asserting each apply generation calls ClearCompiledScripts (theme #1 production-caller assertion). Host.Integration VT suite 18/18, Runtime VT suite 12/12.
2026-07-08 16:21:47 -04:00
parent 9cad9ed0fc
commit 7fd44f0fa7
5 changed files with 149 additions and 17 deletions
@@ -0,0 +1,20 @@
+namespace ZB.MOM.WW.OtOpcUa.Commons.Engines;
+
+/// <summary>
+///     Optional capability an <see cref="IVirtualTagEvaluator"/> may implement when it caches
+///     compiled scripts and needs an apply-boundary drop. The production
+///     <c>RoslynVirtualTagEvaluator</c> caches one collectible
+///     <see cref="System.Runtime.Loader.AssemblyLoadContext"/> per compiled expression; without a
+///     drop on each config-apply generation those ALCs accrete forever across script edits (every
+///     republish roots a new one). <c>VirtualTagHostActor</c> casts its injected evaluator to this
+///     interface and calls <see cref="ClearCompiledScripts"/> at each apply so stale ALCs are
+///     reclaimed — mirroring how <c>ScriptedAlarmEngine</c> clears its compile cache per generation.
+///     Evaluators with no cache (e.g. <see cref="NullVirtualTagEvaluator"/>) simply don't implement
+///     it, and the host actor's null-conditional cast makes the call a no-op.
+/// </summary>
+public interface IScriptCacheOwner
+{
+    /// <summary>Drops every cached compiled script, disposing each so its collectible
+    /// AssemblyLoadContext unloads. Called at each config-apply generation boundary.</summary>
+    void ClearCompiledScripts();
+}
@@ -1,4 +1,3 @@
-using System.Collections.Concurrent;
 using Microsoft.Extensions.Logging;
 using ZB.MOM.WW.OtOpcUa.Commons.Engines;
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
@@ -20,10 +19,13 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Engines;
 /// Cycle detection + cascade ordering live in <see cref="VirtualTagEngine"/>; this adapter
 /// stays single-tag scoped to keep <see cref="VirtualTagActor"/>'s message loop simple.
 /// </summary>
-public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IDisposable
+public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IScriptCacheOwner, IDisposable
 {
-    private readonly ConcurrentDictionary<string, ScriptEvaluator<VirtualTagContext, object?>> _cache
-        = new(StringComparer.Ordinal);
+    // Purpose-built source-hash-keyed compile cache: Lazy single-compile (no double-compile race) and
+    // Clear()/Dispose() that dispose each evaluator's collectible AssemblyLoadContext. The apply-boundary
+    // Clear() (driven by VirtualTagHostActor via IScriptCacheOwner) is what stops ALCs accreting across
+    // script edits — the raw ConcurrentDictionary this replaced never released them.
+    private readonly CompiledScriptCache<VirtualTagContext, object?> _cache = new();
    private readonly ILogger<RoslynVirtualTagEvaluator> _logger;
    private readonly SerilogLogger _scriptRoot;
    private readonly TimeSpan _runTimeout;
@@ -64,7 +66,7 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IDisposabl
        ScriptEvaluator<VirtualTagContext, object?> evaluator;
        try
        {
-            evaluator = _cache.GetOrAdd(expression, ScriptEvaluator<VirtualTagContext, object?>.Compile);
+            evaluator = _cache.GetOrCompile(expression);
        }
        catch (CompilationErrorException ex)
        {
@@ -98,11 +100,16 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IDisposabl

        try
        {
-            using var cts = new CancellationTokenSource(_runTimeout);
-            var raw = evaluator.RunAsync(context, cts.Token).GetAwaiter().GetResult();
+            // Route through TimedScriptEvaluator (Task.Run + WaitAsync): a raw CancellationToken can't
+            // interrupt a CPU-bound/infinite-loop script (Roslyn scripts don't poll it), so the previous
+            // CTS-only path let one runaway script hang this actor forever. WaitAsync returns control when
+            // the wall-clock budget fires regardless of whether the inner task completes (the orphaned
+            // thread is the documented, accepted trade-off — TimedScriptEvaluator remarks).
+            var timed = new TimedScriptEvaluator<VirtualTagContext, object?>(evaluator, _runTimeout);
+            var raw = timed.RunAsync(context).GetAwaiter().GetResult();
            return VirtualTagEvalResult.Ok(raw);
        }
-        catch (OperationCanceledException)
+        catch (ScriptTimeoutException)
        {
            return VirtualTagEvalResult.Failure($"script timed out after {_runTimeout.TotalSeconds:F1}s");
        }
@@ -113,6 +120,9 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IDisposabl
        }
    }

+    /// <inheritdoc />
+    public void ClearCompiledScripts() => _cache.Clear();
+
    private static IReadOnlyDictionary<string, DataValueSnapshot> BuildReadCache(
        IReadOnlyDictionary<string, object?> deps)
    {
@@ -133,10 +143,7 @@ public sealed class RoslynVirtualTagEvaluator : IVirtualTagEvaluator, IDisposabl
    {
        if (_disposed) return;
        _disposed = true;
-        foreach (var ev in _cache.Values)
-        {
-            try { ev.Dispose(); } catch { /* best-effort */ }
-        }
-        _cache.Clear();
+        // CompiledScriptCache.Dispose drains + disposes every materialised evaluator's ALC.
+        _cache.Dispose();
    }
 }
@@ -85,6 +85,12 @@ public sealed class VirtualTagHostActor : ReceiveActor

    private void OnApply(ApplyVirtualTags msg)
    {
+        // Apply-boundary compile-cache drop: each deploy generation may carry edited script sources, and
+        // the production evaluator roots one collectible AssemblyLoadContext per compiled expression. Drop
+        // them here (mirroring ScriptedAlarmEngine's per-generation clear) so stale ALCs are reclaimed
+        // instead of accreting across edits. No-op for evaluators without a cache (NullVirtualTagEvaluator).
+        (_evaluator as IScriptCacheOwner)?.ClearCompiledScripts();
+
        var desired = new HashSet<string>(msg.Plans.Select(p => p.VirtualTagId), StringComparer.Ordinal);

        // Stop + forget children whose vtagId is no longer desired. Stopping the child triggers its
@@ -251,7 +251,7 @@ public sealed class RoslynVirtualTagEvaluatorTests
    }

    /// <summary>Decisive proof the fast-path skips Roslyn: the compiled-script cache (which every
-    /// Roslyn evaluation populates via <c>GetOrAdd</c>) stays EMPTY after a mirror evaluation, then
+    /// Roslyn evaluation populates via <c>GetOrCompile</c>) stays EMPTY after a mirror evaluation, then
    /// grows to one entry once a genuine (non-mirror) expression forces compilation.</summary>
    [Fact]
    public void Passthrough_does_not_populate_the_compiled_script_cache()
@@ -299,13 +299,74 @@ public sealed class RoslynVirtualTagEvaluatorTests
        entry.VirtualTagId.ShouldBe("vt-log");
    }

-    /// <summary>Reads the count of the private compiled-script cache via reflection.</summary>
+    // ── U2 — production timeout must be REAL: a CPU-bound/infinite-loop script must fail within the
+    //    wall-clock budget, never hang the owning actor. The pre-fix CTS-only path could not interrupt
+    //    a synchronous Roslyn script, so this class of script wedged the VirtualTagActor forever. ──
+
+    /// <summary>An infinite-loop script returns Failure ("timed out") within the timeout budget instead
+    /// of hanging. The whole call is bounded by <c>WaitAsync</c> so a regression FAILS the test (with a
+    /// TimeoutException) rather than wedging the suite.</summary>
+    [Fact]
+    public async Task Evaluate_infinite_loop_script_fails_within_timeout_and_does_not_hang()
+    {
+        using var sut = new RoslynVirtualTagEvaluator(
+            NullLogger<RoslynVirtualTagEvaluator>.Instance, NoOpScriptRoot(), TimeSpan.FromMilliseconds(200));
+
+        var evalTask = Task.Run(
+            () => sut.Evaluate("vt-loop", "while(true){}return 0;", new Dictionary<string, object?>()),
+            TestContext.Current.CancellationToken);
+
+        var result = await evalTask.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken);
+
+        result.Success.ShouldBeFalse();
+        result.Reason.ShouldNotBeNull();
+        result.Reason.ShouldContain("timed out");
+    }
+
+    /// <summary>Regression: routing through <see cref="TimedScriptEvaluator{TContext, TResult}"/> did not
+    /// break the happy path — a fast compiled script still returns its value well within the budget.</summary>
+    [Fact]
+    public void Evaluate_fast_script_still_returns_value_after_timeout_wrapping()
+    {
+        using var sut = new RoslynVirtualTagEvaluator(
+            NullLogger<RoslynVirtualTagEvaluator>.Instance, NoOpScriptRoot(), TimeSpan.FromMilliseconds(200));
+
+        var result = sut.Evaluate("vt-fast", "return (int)ctx.GetTag(\"a\").Value + 1;",
+            new Dictionary<string, object?> { ["a"] = 41 });
+
+        result.Success.ShouldBeTrue(result.Reason);
+        result.Value.ShouldBe(42);
+    }
+
+    // ── U3 — the apply-boundary clear (IScriptCacheOwner) drops compiled entries so their collectible
+    //    AssemblyLoadContexts are reclaimed. (The ALC-unload-on-Clear proof lives in CompiledScriptCacheTests;
+    //    here we assert the adapter's ClearCompiledScripts actually empties its cache.) ──
+
+    /// <summary>Verifies <see cref="RoslynVirtualTagEvaluator.ClearCompiledScripts"/> drops every cached
+    /// compile — the hook the host actor calls per deploy generation to release stale ALCs.</summary>
+    [Fact]
+    public void ClearCompiledScripts_empties_the_compile_cache()
+    {
+        using var sut = new RoslynVirtualTagEvaluator(NullLogger<RoslynVirtualTagEvaluator>.Instance, NoOpScriptRoot());
+
+        sut.Evaluate("vt-a", "return (int)ctx.GetTag(\"a\").Value + 1;", new Dictionary<string, object?> { ["a"] = 1 });
+        CompiledCacheCount(sut).ShouldBe(1);
+
+        sut.ClearCompiledScripts();
+
+        CompiledCacheCount(sut).ShouldBe(0);
+    }
+
+    /// <summary>Reads the count of the private compiled-script cache via reflection. The cache is a
+    /// <c>CompiledScriptCache&lt;,&gt;</c> which exposes a <c>Count</c> property (not <c>ICollection</c>).</summary>
    private static int CompiledCacheCount(RoslynVirtualTagEvaluator sut)
    {
        var field = typeof(RoslynVirtualTagEvaluator)
            .GetField("_cache", BindingFlags.Instance | BindingFlags.NonPublic);
        field.ShouldNotBeNull();
-        var cache = (ICollection)field.GetValue(sut)!;
-        return cache.Count;
+        var cache = field.GetValue(sut)!;
+        var countProp = cache.GetType().GetProperty("Count");
+        countProp.ShouldNotBeNull();
+        return (int)countProp.GetValue(cache)!;
    }
 }
@@ -345,6 +345,26 @@ public sealed class VirtualTagHostActorTests : RuntimeActorTestBase
        mux.LastSender.ShouldNotBe(firstChild);
    }

+    /// <summary>Wiring guard (arch-review 02/U3, theme #1): every ApplyVirtualTags calls
+    /// <see cref="IScriptCacheOwner.ClearCompiledScripts"/> on the evaluator at the apply boundary.
+    /// This is the load-bearing wire — the production evaluator roots one collectible ALC per compiled
+    /// script, and without this per-generation clear they accrete across deploys. A pure unit test of
+    /// the evaluator can't catch a missing call here; this asserts the host actor actually makes it.</summary>
+    [Fact]
+    public void ApplyVirtualTags_clears_the_evaluator_compile_cache_each_generation()
+    {
+        var publish = CreateTestProbe();
+        var evaluator = new CacheOwningStubEvaluator();
+        var host = Sys.ActorOf(VirtualTagHostActor.Props(publish.Ref, mux: null, evaluator));
+
+        host.Tell(new VirtualTagHostActor.ApplyVirtualTags(new[] { Plan("vt-1", "eq-1", "speed-rpm") }));
+        AwaitAssert(() => evaluator.ClearCount.ShouldBe(1));
+
+        // A second apply generation clears again — proves it's per-generation, not one-shot.
+        host.Tell(new VirtualTagHostActor.ApplyVirtualTags(new[] { Plan("vt-1", "eq-1", "speed-rpm") }));
+        AwaitAssert(() => evaluator.ClearCount.ShouldBe(2));
+    }
+
    /// <summary>Capturing <see cref="IHistoryWriter"/>: records every Record call so H5c tests can
    /// assert the host historizes (or does not) and with what path + snapshot.</summary>
    private sealed class CapturingHistoryWriter : IHistoryWriter
@@ -371,4 +391,22 @@ public sealed class VirtualTagHostActorTests : RuntimeActorTestBase
        public VirtualTagEvalResult Evaluate(string id, string expr, IReadOnlyDictionary<string, object?> deps)
            => VirtualTagEvalResult.NoChange;
    }
+
+    /// <summary>Spy evaluator that also owns a script cache: records how many times the host actor
+    /// calls <see cref="IScriptCacheOwner.ClearCompiledScripts"/> so the apply-boundary wiring can be
+    /// asserted. Counter is interlocked/volatile — written on the actor thread, read on the test thread.</summary>
+    private sealed class CacheOwningStubEvaluator : IVirtualTagEvaluator, IScriptCacheOwner
+    {
+        private int _clearCount;
+
+        /// <summary>Number of ClearCompiledScripts calls observed.</summary>
+        public int ClearCount => Volatile.Read(ref _clearCount);
+
+        /// <inheritdoc/>
+        public VirtualTagEvalResult Evaluate(string id, string expr, IReadOnlyDictionary<string, object?> deps)
+            => VirtualTagEvalResult.NoChange;
+
+        /// <inheritdoc/>
+        public void ClearCompiledScripts() => Interlocked.Increment(ref _clearCount);
+    }
 }