lmxopcua/archreview/02-scripting-alarms.md

Architecture Review 02 — Scripting, Virtual Tags, Scripted Alarms, Alarm Historian

• Date: 2026-07-12
• Commit: f6eaa267 (master, clean tree)
• Updates: the 2026-07-08 review at 9cad9ed0. All arch-review remediation branches have since merged to master; the delta touching this domain is exactly one commit — 7fd44f0f (Critical 2a/2b: real VT script timeout + ALC-safe compile cache). The five in-scope Core projects are byte-identical to 9cad9ed0 (git diff empty; only test-csproj NoWarn OTOPCUA0001 additions), so every prior Core-internal finding was re-verified against unchanged code and its line refs remain valid.
• Scope:
  • src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting + Core.Scripting.Abstractions (Roslyn compile/eval engine)
  • src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags (virtual-tag runtime)
  • src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms (scripted-alarm engine + Part 9 state machine)
  • src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian (SQLite store-and-forward sink)
  • Production evaluation path: Host/Engines/RoslynVirtualTagEvaluator, Commons/Engines/IScriptCacheOwner, Runtime/VirtualTags/VirtualTagHostActor + VirtualTagActor (the surface the Critical-2 remediation changed)
  • Test projects under tests/Core/ + the new Host.IntegrationTests/Runtime.Tests coverage
• Method: verified every prior finding against the actual code (not STATUS.md claims); full adversarial read of the remediation commit (RoslynVirtualTagEvaluator, IScriptCacheOwner, VirtualTagHostActor.OnApply, the new tests) plus the cache/evaluator internals it now leans on (CompiledScriptCache.Clear concurrency, ScriptEvaluator.Dispose/RunAsync disposed-guard, TimedScriptEvaluator semantics); fresh-eyes pass over the changed surface.

---

Prior-finding status (9cad9ed0 → f6eaa267)

ID	One-liner	Status @ f6eaa267
S1	VirtualTagEngine.Load mutates shared state with no gate vs in-flight evals	STILL OPEN — VirtualTagEngine.cs unchanged (empty diff); engine still dormant (see U1)
S2	No coalescing/backpressure on upstream-change fan-out (both engines)	STILL OPEN — ScriptedAlarmEngine.cs:442-450 / VirtualTagEngine.cs:263-272 unchanged
S3	Failed reload is fail-stop, not fail-back (both engines)	STILL OPEN — unchanged
S4	_alarmsReferencing plain Dictionary read from upstream threads without a gate	STILL OPEN — ScriptedAlarmEngine.cs:446 unchanged
S5	Timed-shelve expiry via the predicate path swallows the Unshelved emission	STILL OPEN — Part9StateMachine.cs unchanged; the missing test is still missing
S6	SqliteStoreAndForwardSink.Dispose races an in-flight drain	STILL OPEN — unchanged
S7	Sandbox CPU/memory unbounded; timed-out script leaks a thread; no quarantine	STILL OPEN — blast radius grew: the U2 fix routes production VT evaluation through the same abandon-on-timeout design, so a wedged VT script now orphans one pool thread and blocks its actor ~2 s per dependency change (see updated S7 body)
S8	ScriptedAlarmEngine.Dispose blocks sync-over-async	STILL OPEN — ScriptedAlarmEngine.cs:761 unchanged
S9	At-least-once delivery duplicates on crash window (doc note)	STILL OPEN — no docs/Historian.md note added
S10	VirtualTagSource.SubscribeAsync seed-then-subscribe can miss one update	STILL OPEN — unchanged (dormant code)
S11	Capacity eviction drops oldest accepted alarm events (policy doc note)	STILL OPEN — no docs/AlarmTracking.md policy note
P1	ScriptSandbox.Build rebuilds the full BCL reference set on every compile	STILL OPEN — ScriptSandbox.cs:83-87 unchanged; now amplified by the per-apply cache clear (see P7)
P2	Task.Run + WaitAsync per evaluation on the hot path	STILL OPEN (accepted) — now also the production VT cost, by design of the U2 fix
P3	VT engine allocates per evaluation what the alarm engine pools	STILL OPEN — moot while U1 unresolved
P4	Single global eval gate per engine	STILL OPEN (accepted)
P5	SQLite per-call overheads	STILL OPEN (accepted)
P6	DependencyGraph well optimized (no action)	unchanged — informational
C1	ITagUpstreamSource defined twice	STILL OPEN — duplicate still at ScriptedAlarmEngine.cs:860-872
C2	Abstractions assembly spans three namespaces undocumented	STILL OPEN
C3	Trailing type definitions in engine files	STILL OPEN
C4	Repeated raw OPC UA status-code literals	STILL OPEN
C5	Core.AlarmHistorian mixes contract and implementation	STILL OPEN
C6	Plan-era "Phase 7 plan Stream …" doc residue	STILL OPEN — TimedScriptEvaluator.cs:5,22,39 etc. unchanged; the 3b5ef439 fixdocs sweep touched only S7/TwinCAT/Resilience files
C7	ApplyPredicate xmldoc claims branch handling that doesn't exist	STILL OPEN — Part9StateMachine.cs:31-34 unchanged
U1	Entire Core.VirtualTags engine stack dormant in production	STILL OPEN — new VirtualTagEngine still appears only in its own tests; ScriptAnalysisService.cs:293 even documents "the cascade VirtualTagEngine is dormant". No retire/sanction decision made
U2	CRITICAL — production script timeout ineffective (TimedScriptEvaluator bypassed)	FIXED — 7fd44f0f: RoslynVirtualTagEvaluator.Evaluate routes through TimedScriptEvaluator (RoslynVirtualTagEvaluator.cs:101-115); the orphan-thread trade-off is explicitly acknowledged in the comment; regression test Evaluate_infinite_loop_script_fails_within_timeout_and_does_not_hang is itself WaitAsync-bounded so a regression fails instead of hanging the suite. Caveats: on the node, a timeout is a dropped update, not Bad quality (new S13), and the runaway thread still leaks with no quarantine (S7)
U3	HIGH — live path bypasses CompiledScriptCache: unbounded ALC accretion	FIXED — 7fd44f0f: raw ConcurrentDictionary → CompiledScriptCache<VirtualTagContext, object?> (RoslynVirtualTagEvaluator.cs:28); new IScriptCacheOwner seam (Commons/Engines/IScriptCacheOwner.cs); apply-boundary clear at VirtualTagHostActor.cs:92 on every ApplyVirtualTags (the only republish path — VT config only reaches children via this message); wiring-guard test asserts the clear per generation. Caveat: the clear races in-flight child evaluations (new S12)
U4	IHistoryWriter is a permanently-Null surface	STILL OPEN — Runtime/ServiceCollectionExtensions.cs:58,242 still binds only NullHistoryWriter
U5	Part 9 surface gaps real but undeclared in one place	STILL OPEN — no conformance statement added to docs/ScriptedAlarms.md
U6	No TODO/HACK/stub markers (healthy)	unchanged — informational
U7	Test coverage broad with specific holes (a)–(e)	PARTIALLY FIXED — hole (e) closed: RoslynVirtualTagEvaluatorTests now covers the production timeout (infinite-loop + happy-path-after-wrapping + ClearCompiledScripts), and VirtualTagHostActorTests guards the apply-boundary clear wiring. Holes (a) shelve-expiry emission, (b) load-vs-cascade concurrency, (c) sink dispose-during-drain, (d) dedicated ForbiddenTypeAnalyzer suite — all still open

---

Architecture Overview

Script compile/eval pipeline (Core.Scripting)

User scripts are C# statement bodies ending in return …;. ScriptEvaluator<TContext,TResult>.Compile (Core.Scripting/ScriptEvaluator.cs) synthesizes a wrapper class (CompiledScript.Run(globals)), then runs a five-step gate:

1. Parse the wrapper source with #line 1 remapping so diagnostics point at operator source.
2. Wrapper-injection guard (EnforceSingleRunMember, ScriptEvaluator.cs:222) — rejects brace-balanced injections that declare sibling members/types (diagnostics LMX001/LMX002).
3. Roslyn compile against the ScriptSandbox reference set (pinned OtOpcUa assemblies + System.*/netstandard TPA subset) — the allow-list is explicitly not the security boundary (type forwarding makes it porous).
4. ForbiddenTypeAnalyzer — the real gate. Two semantic passes: (1) member/call surface via GetSymbolInfo on creations/invocations/member-access/identifiers; (2) every TypeSyntax via GetTypeInfo, recursing generic args + array element types. Deny-list = namespace prefixes (System.IO, System.Net, System.Diagnostics, System.Reflection, System.Threading.Tasks, System.Runtime.InteropServices, System.Runtime.Loader, Microsoft.Win32) plus type-granular names for dangerous residents of allowed namespaces (Environment, AppDomain, GC, Activator, Thread, ThreadPool, Timer, Unsafe).
5. Emit to an in-memory PE, load into a per-script collectible AssemblyLoadContext, bind a static Func<ScriptGlobals<TContext>,TResult> delegate. Dispose() unloads the ALC.

Around the core evaluator sit three wrappers: CompiledScriptCache (SHA-256-source-keyed, Lazy single-compile, value-scoped TryRemove on Clear() so a concurrent re-add survives), TimedScriptEvaluator (Task.Run + WaitAsync wall-clock budget, default 250 ms, wraps timeout as ScriptTimeoutException; documents the known orphan-thread leak for CPU-bound scripts), and the script-log pipeline (ScriptRootLogger → rolling scripts-*.log + ScriptLogCompanionSink error mirror to the main log + ScriptLogTopicSink → DPS script-logs topic via IScriptLogPublisher).

Core.Scripting.Abstractions holds the Roslyn-free closure the sandbox pins: ScriptContext (the ctx API — GetTag/SetVirtualTag/Now/Logger/Deadband), ScriptGlobals<T>, PassthroughScript (regex fast-path classifier for the return ctx.GetTag("X").Value; mirror shape), and — notably — the concrete VirtualTagContext and AlarmPredicateContext, which live in this assembly but declare Core.VirtualTags / Core.ScriptedAlarms namespaces (see C2).

DependencyExtractor statically harvests ctx.GetTag("literal") / ctx.SetVirtualTag("literal", …) call sites from the AST, rejecting dynamic paths at publish so the change-trigger subscription graph is knowable up front.

Virtual-tag runtime — two implementations, one live (now with the safety wrappers on the live one)

The in-scope VirtualTagEngine is a self-contained multi-tag engine: Load() compiles all scripts through a CompiledScriptCache, validates data types and SetVirtualTag targets, builds a DependencyGraph (iterative Tarjan cycle detection + Kahn topo sort + cached rank), subscribes to upstream driver tags via ITagUpstreamSource, and serializes all evaluations under a single SemaphoreSlim gate. Change events cascade dependents in topological order; TimerTriggerScheduler adds interval-grouped timer triggers with in-flight tick skipping; VirtualTagSource adapts the engine to the driver-agnostic IReadable/ISubscribable surface.

Production still does not run this engine (U1 unchanged). The live path is VirtualTagHostActor → per-tag VirtualTagActor → IVirtualTagEvaluator bound to Host/Engines/RoslynVirtualTagEvaluator (single-tag adapter; fan-out owned by DependencyMuxActor). What changed at 7fd44f0f: the adapter now (a) routes every compiled evaluation through TimedScriptEvaluator (default 2 s budget; ScriptTimeoutException → Failure("script timed out …")), so a CPU-bound/infinite-loop script can no longer wedge the owning VirtualTagActor's message loop; and (b) caches compiles in a CompiledScriptCache and implements the new IScriptCacheOwner capability, which VirtualTagHostActor.OnApply invokes at the top of every ApplyVirtualTags generation — stale collectible ALCs are dropped per deploy instead of accreting forever. The PassthroughScript mirror fast-path still bypasses Roslyn (and therefore both wrappers) — correctly, since it executes no user code.

Scripted-alarm flow (Core.ScriptedAlarms) — live end to end (unchanged)

ScriptedAlarmHostActor (Runtime) owns a long-lived ScriptedAlarmEngine and calls LoadAsync per config apply (generation-tagged to discard stale completions). Inside the engine:

1. LoadAsync (under _evalGate) tears down the prior generation (shelving timer, subscriptions, alarms, scratch, compile-cache ALCs), compiles every predicate via CompiledScriptCache<AlarmPredicateContext,bool>, builds an inverse index tag-path → alarm ids (alarms are DAG leaves; no topo sort), seeds the value cache, restores persisted AlarmConditionState from IAlarmStateStore (EF-backed in prod, in-memory for dev/tests), re-derives ActiveState from the live predicate (startup recovery), then subscribes upstream and starts the 5 s shelving-check timer.
2. Upstream changes (OnUpstreamChange, fed by DependencyMuxTagUpstreamSource) update the value cache and spawn tracked fire-and-forget ReevaluateAsync tasks that serialize on _evalGate, run the predicate through TimedScriptEvaluator against a reused per-alarm AlarmScratch (read-cache dictionary + context, refilled in place — the hot path allocates nothing), and apply the result through Part9StateMachine.ApplyPredicate.
3. Part9StateMachine is a pure-function transition table over the immutable AlarmConditionState record (Enabled/Active/Acked/Confirmed/Shelving + audit Comments as ImmutableList). Operator verbs (Acknowledge/Confirm/OneShotShelve/TimedShelve/Unshelve/Enable/Disable/AddComment) come in via the engine's ApplyAsync (persist-before-update-in-memory; emissions built under the gate, fired after release to avoid re-entrancy deadlock).
4. Emissions (ScriptedAlarmEvent) fan out through OnEvent → ScriptedAlarmSource (the IAlarmSource adapter with equipment-path-prefix subscription filters) into the Part 9 condition nodes / /alerts / historian adapter, carrying the per-alarm HistorizeToAveva opt-out. MessageTemplate resolves {TagPath} tokens at emission with a stricter-than-predicate quality bar (Good only; else {?}).

Alarm-history write path (Core.AlarmHistorian) — unchanged

IAlarmHistorianSink is the fire-and-forget ingestion contract; production is SqliteStoreAndForwardSink: WAL-mode SQLite Queue table, EnqueueAsync commits a row per event (capacity fast path consults an Interlocked cached counter, falling back to COUNT(*) + oldest-row eviction at the wall, with periodic 10k-enqueue resync). A self-rescheduling one-shot timer drains batches (default 100) through IAlarmHistorianWriter (the HistorianGateway SendEvent gRPC client), applying per-event outcomes: Ack → delete, PermanentFail → dead-letter, RetryPlease → attempt-bump with a max-attempts (10) poison cap; corrupt payloads dead-letter immediately so they can't stall the queue head. Backoff ladder 1→2→5→15→60 s applied to the timer due-time; dead letters retained 30 days with operator RetryDeadLettered(); GetStatus() surfaces depth/dead-letter/last-error/evictions to the Admin UI and health checks.

---

Findings

Severity scale: Critical (production correctness/availability today) / High (real risk under realistic conditions) / Medium (defect or debt worth scheduling) / Low (note).

Fixed prior findings (U2, U3, U7-e) live only in the status table above. Still-open findings keep their IDs; new findings continue the per-category scheme (S12+, P7+, C8+).

1. Stability

S1 — HIGH — VirtualTagEngine.Load mutates shared state with no gate against in-flight evaluations

VirtualTagEngine.Load (Core.VirtualTags/VirtualTagEngine.cs:78-173) clears and rebuilds the plain Dictionary _tags (line 24), the DependencyGraph, and disposes the compile cache without acquiring _evalGate, while three concurrent paths read that state from arbitrary threads: EvaluateInternalAsync reads _tags before taking the gate (line 293), CascadeAsync walks _graph (line 278) whose internals are plain dictionaries plus a nullable _cachedRank, and timer ticks call EvaluateOneAsync. A Load during an in-flight cascade is a torn-read on non-thread-safe collections. The sibling ScriptedAlarmEngine solved exactly this (gate-held LoadAsync, ConcurrentDictionary with an explanatory comment at ScriptedAlarmEngine.cs:42-50); the VT engine never received the same treatment. Mitigating factor: the engine is dormant in production (U1) — but it is fully unit-tested and presented as production-ready, so anyone wiring it up inherits the race. Recommendation: either retire the engine (preferred — see U1) or port the alarm engine's discipline: gate-held load, ConcurrentDictionary for _tags, and re-check-after-gate in the eval path.

S2 — HIGH — No coalescing/backpressure on upstream-change fan-out (both engines)

Every upstream change spawns a fire-and-forget task that queues on the single eval gate: ScriptedAlarmEngine.OnUpstreamChange (ScriptedAlarmEngine.cs:442-450) spawns one ReevaluateAsync per change event, and VirtualTagEngine.OnUpstreamChange (VirtualTagEngine.cs:263-272) spawns one CascadeAsync per change (untracked). When a referenced tag changes faster than the serialized evaluations drain (a 100 Hz analog referenced by one alarm is enough), tasks accumulate without bound — memory growth plus an ever-staler processing lag, and each queued task re-evaluates against the current cache so the extra work is pure waste. The alarm engine tracks these tasks for dispose-drain (_inFlight) but does not bound them. Recommendation: replace per-event task spawning with a dirty-set + single pump: mark the alarm/tag dirty, and have one loop drain the dirty set under the gate. This makes cost proportional to distinct dirty alarms, not event rate.

S3 — MEDIUM — Failed reload is fail-stop, not fail-back (both engines)

Both engines tear down the previous generation before compiling the new one. On a compile failure, VirtualTagEngine.Load has already unsubscribed, cleared _tags/_graph, and cleared the compile cache (VirtualTagEngine.cs:84-88) before throwing (line 148) — leaving _loaded == true from the prior load, an empty tag set, and a stale _valueCache that Read/VirtualTagSource keep serving as Good values that will never update. ScriptedAlarmEngine.LoadAsync is the same shape (ScriptedAlarmEngine.cs:196-209, throw at 250-255): after a failed apply, zero alarms are loaded — no predicates evaluate until the next successful publish. The host actor explicitly accepts this ("log it and stay inert", ScriptedAlarmHostActor.cs:188, 245), and publish-time validation upstream makes a bad definition reaching the engine unlikely — but a transient IAlarmStateStore.LoadAsync/SaveAsync failure during the state-restore loop (ScriptedAlarmEngine.cs:273-281) also lands here, and that is not operator-preventable. Recommendation: build the new generation into local structures and swap under the gate only on success (compile is already side-effect-free; the subscriptions and timer are the only external effects). At minimum, distinguish store-failures from compile-failures and retry the former.

S4 — MEDIUM — _alarmsReferencing read without synchronization

ScriptedAlarmEngine._alarmsReferencing is a plain Dictionary (ScriptedAlarmEngine.cs:120-121) mutated inside gate-held LoadAsync (lines 201, 237-242) but read by OnUpstreamChange (line 446) from upstream callback threads with no gate. LoadAsync disposes the old subscriptions first, but IDisposable.Dispose on a subscription does not guarantee an already-executing callback has finished — a callback in flight during reload can race Clear()/Add() on a non-thread-safe dictionary. The class comment (lines 42-50) carefully justifies ConcurrentDictionary for _alarms for exactly this class of reader; _alarmsReferencing was missed. Recommendation: make it a ConcurrentDictionary or swap-in an immutable snapshot (Dictionary rebuilt per load, published via Volatile.Write to a field).

S5 — MEDIUM — Timed-shelve expiry via the predicate path swallows the Unshelved emission

Part9StateMachine.ApplyPredicate silently expires timed shelving (Part9StateMachine.cs:47-49 via MaybeExpireShelving, lines 318-322) and returns EmissionKind.None (or Activated/Cleared for the value transition) — the Unshelved emission only ever comes from ApplyShelvingCheck on the 5 s timer. If a predicate re-evaluation lands between expiry time and the next timer tick, the state record is persisted as unshelved but no Unshelved event is ever published — OPC UA clients and the /alerts page tracking ShelvingState from events never learn the shelve ended (the next Activated/Cleared arrives un-suppressed, which is at least confusing for an operator who believes the alarm is still shelved). Recommendation: have ApplyPredicate return a compound result (or the engine detect Shelving kind change between seed and result) so the expiry emits Unshelved before the value-transition emission. Add the missing state-machine test (Part9StateMachineTests.cs does not cover this path).

S6 — MEDIUM — SqliteStoreAndForwardSink.Dispose races an in-flight drain

Dispose (SqliteStoreAndForwardSink.cs:679-686) sets _disposed, disposes the timer, then immediately disposes _drainGate and the writer. Timer.Dispose() (no WaitHandle overload used) does not wait for a running callback, so an in-flight DrainOnceAsync can still be holding the gate and mid-WriteBatchAsync: the finally { _drainGate.Release(); } (line 467) then throws ObjectDisposedException (caught by the timer callback's catch and logged as a drain fault), and the writer can be disposed under an active call. Not a crash — but shutdown noise and an undefined writer contract. Recommendation: mirror the alarm engine's dispose-drain: _drainGate.Wait() (or an in-flight task handle) before disposing the gate/writer, or use Timer.Dispose(WaitHandle).

S7 — MEDIUM — Accepted sandbox limits: CPU/memory unbounded, timed-out scripts leak a thread — blast radius now includes the live VT path

Documented and deliberate: the sandbox cannot bound allocation or CPU (ScriptSandbox.cs:30-35), and a timed-out CPU-bound script leaves its thread-pool thread spinning forever (TimedScriptEvaluator.cs:17-26). One orphan per bad-script evaluation attempt — and since 7fd44f0f this is no longer confined to the alarm engine: production virtual-tag evaluation now runs through the same abandon-on-timeout design (RoslynVirtualTagEvaluator.cs:101-115), so a hot looping VT script referenced by a fast-changing dependency orphans one pool thread and blocks its VirtualTagActor dispatcher thread for the full 2 s budget per dependency change, with no quarantine — the actor keeps re-evaluating on every change until the pool starves. (The alarm engine holds prior state on timeout, ScriptedAlarmEngine.cs:535-538, but likewise does not quarantine the predicate.) That trade-off was the right call for U2 — a bounded-loss timeout strictly beats a wedged actor — but the compensating control the prior review recommended still does not exist. Recommendation: add a per-script circuit breaker (N consecutive timeouts → suspend evaluation + surface to Admin UI) as a cheap interim before any out-of-process runner. The three-layer compile-time sandbox itself (allow-list refs → wrapper-injection guard → semantic deny-list with the full TypeSyntax pass) remains notably strong.

S8 — LOW — ScriptedAlarmEngine.Dispose blocks sync-over-async

Task.WhenAll(toAwait).GetAwaiter().GetResult() (ScriptedAlarmEngine.cs:761) — fine under the actor host (no sync context) but a deadlock trap if ever disposed from a context-bound thread. Consider IAsyncDisposable.

S9 — LOW — At-least-once delivery duplicates on crash window

A crash between a successful WriteBatchAsync and the outcome transaction commit (SqliteStoreAndForwardSink.cs:372-441) re-delivers the batch. Correct choice for alarm history (dupes over loss) — worth one line in docs/Historian.md so the gateway side knows to tolerate replays.

S10 — LOW — VirtualTagSource.SubscribeAsync initial-value window can miss one update

Emitting the seed read before registering the observer (VirtualTagSource.cs:63-75) guarantees ordering but means a change landing between the Read and the Subscribe is missed until the next change — the inverse trade-off of the race the comment describes. Register first, then emit the seed, and let the (idempotent, newer-wins) consumer handle a possible out-of-order pair — or document that seed-then-subscribe can serve a stale value on slow-changing tags.

S11 — LOW — Capacity eviction drops the oldest accepted alarm events

Documented behavior with an EvictedCount operator counter (SqliteStoreAndForwardSink.cs:602-636). Note only: for a compliance-oriented alarm trail, dropping newest (or refusing enqueue) is sometimes preferred over silently losing the oldest; the current choice is defensible but should be a deliberate, documented policy in docs/AlarmTracking.md.

S12 — MEDIUM — NEW (in the U3 remediation) — Apply-boundary ClearCompiledScripts races in-flight child evaluations

VirtualTagHostActor.OnApply calls ClearCompiledScripts() first thing (VirtualTagHostActor.cs:92), on the host actor's thread — while unchanged child VirtualTagActors keep evaluating concurrently on other dispatcher threads (nothing gates them; a DependencyValueChanged can land at any time). The comment says this "mirrors ScriptedAlarmEngine's per-generation clear", but the mirror is inexact: the alarm engine clears under _evalGate with the old subscriptions already torn down, so no evaluation can hold a disposed evaluator; the VT host has no gate spanning child evaluations. Concrete race: a child's Evaluate obtains the evaluator from the cache (RoslynVirtualTagEvaluator.cs:69), the host's Clear() then value-scoped-removes and disposes it (CompiledScriptCache.cs:113-116 → ScriptEvaluator.Dispose sets _disposed + _alc.Unload()), and the child's subsequent timed.RunAsync hits the disposed guard (ScriptEvaluator.cs:180) — ObjectDisposedException → caught by the adapter's generic catch (RoslynVirtualTagEvaluator.cs:116-120) → Failure("script threw: …") → that evaluation's update is silently dropped (and per S13, quality is not degraded). A secondary wrinkle: the clear runs before the removed/changed children are stopped (lines 98-118), and Context.Stop is asynchronous — a doomed child can process one queued change after the clear and recompile its stale expression into the fresh cache, keeping its ALC alive until the next apply (bounded, one generation — not a leak, but it undercuts the "released in one sweep" intent). The cache itself is blameless: its value-scoped TryRemove correctly protects concurrent GetOrCompile re-adds; the gap is the missing coordination between clear and in-flight use of already-handed-out evaluators. Consequence: a transient dropped evaluation per unlucky deploy (self-heals on the tag's next dependency change — which for a slow-changing tag can be a long staleness window), plus one-generation stale-ALC retention. Real but bounded — Medium. Recommendation: cheapest robust fix is in the adapter: catch ObjectDisposedException around RunAsync, re-fetch via GetOrCompile, and retry once (the recompile is idempotent). Alternatively move the clear to after the stop/respawn reconciliation and have Evaluate re-validate, or route child evaluations through a shared ReaderWriterLockSlim the clear takes as writer. Add a concurrent clear-during-evaluate test either way.

S13 — MEDIUM — NEW — Script failure/timeout never degrades the OPC UA node's quality: stale value stays Good forever

VirtualTagActor.OnDependencyChanged handles a failed evaluation (including the new timeout Failure) by publishing a Warning script-log entry + a fail metric and returning without any value/quality publish (Runtime/VirtualTags/VirtualTagActor.cs:123-128; same for the evaluator-threw path at 115-121). The node therefore keeps its previous state: BadWaitingForInitialData if the script never succeeded (acceptable), but the last Good value with its old timestamp, indefinitely, if a previously-working script starts failing or timing out. Data-plane consumers (OPC UA clients, the historian recorder, downstream ScadaBridge) have no way to distinguish "healthy but unchanged" from "script broken for a week" — only the script-log page and the eval metric know. Note the remediation commit message's claim that a runaway script "returns Bad within the timeout" is true of the evaluator's return value but not of the node. The dormant VirtualTagEngine gets this right — it maps script failure to BadInternalError quality (VirtualTagEngine.cs:307,328), and ScriptEvaluator's own remarks (lines 40-43) promise that mapping; the live actor path never implemented it. Recommendation: on !result.Success, publish the existing value with Bad quality (e.g. BadInternalError / Uncertain per policy) to the publish actor — the plumbing (OpcUaPublishActor.AttributeValueUpdate carries OpcUaQuality) already exists at the host-actor bridge (VirtualTagHostActor.cs:179-180, which currently hard-codes Good). Requires EvaluationResult to carry success/quality. Pair with a regression test: succeed once, then fail — node quality must degrade.

2. Performance

P1 — HIGH — ScriptSandbox.Build rebuilds the full BCL reference set on every compile

Every ScriptEvaluator.Compile calls ScriptSandbox.Build(typeof(TContext)) (ScriptEvaluator.cs:74), which calls MetadataReference.CreateFromFile for every System.*/netstandard DLL in the trusted-platform-assemblies list (ScriptSandbox.cs:83-87, 100-130) — on the order of 100+ files, each a fresh PortableExecutableReference whose AssemblyMetadata Roslyn must load and parse per compilation. CompiledScriptCache de-dupes per source, but a publish with 200 distinct scripts pays this ~200 times, and the Admin UI ScriptAnalysis endpoints (which reuse this compiler context per keystroke-ish cadence) pay it continuously. The references are immutable per contextType for the process lifetime. Now amplified by the (correct) U3 apply-boundary clear: every deploy generation recompiles every VT script from scratch (see P7), each paying this cost. Recommendation: cache SandboxConfig (or at least the MetadataReference list) in a static ConcurrentDictionary<Assembly, …> keyed by contextType.Assembly. This is likely a 10-100× compile-latency win and directly reduces publish time and script-editor diagnostic latency.

P2 — MEDIUM — Task.Run + WaitAsync per evaluation on the hot path

TimedScriptEvaluator.RunAsync (TimedScriptEvaluator.cs:78-81) pushes every predicate/tag evaluation to the pool and registers a timeout. This now applies to the production VT path too (by design of the U2 fix): each VirtualTagActor evaluation pays a pool hop plus a synchronous block of its dispatcher thread (RoslynVirtualTagEvaluator.cs:109 GetAwaiter().GetResult()). The hop is required for the timeout to work at all, and the common case (sub-millisecond script) pays scheduling + WaitAsync machinery every time. Recommendation: acceptable at current scale; if profiling ever shows it, an inline-run-with-watchdog design (run synchronously, watchdog flags overruns for quarantine rather than aborting) trades hard timeout for zero hop.

P3 — MEDIUM — VT engine allocates per evaluation what the alarm engine pools

VirtualTagEngine.EvaluateInternalAsync builds a fresh read-cache Dictionary and a fresh VirtualTagContext per evaluation (VirtualTagEngine.cs:298, 313-317); the alarm engine got the AlarmScratch reuse optimization (ScriptedAlarmEngine.cs:52-63, 798-835) for exactly this pattern. The live RoslynVirtualTagEvaluator shares the allocate-per-evaluation shape (BuildReadCache + fresh VirtualTagContext + fresh TimedScriptEvaluator wrapper per call, RoslynVirtualTagEvaluator.cs:87-99,108). Inconsistent with the alarm engine — moot for the dormant engine (U1), but the live adapter is on the hot path.

P4 — LOW — Single global eval gate per engine

All evaluations serialize on one SemaphoreSlim (VirtualTagEngine.cs:44; ScriptedAlarmEngine.cs:124). Correct and simple; it caps throughput at single-threaded script execution per engine. Fine for operator-authored low-thousands scale (the compile cache doc states the same bound); a per-alarm or sharded gate is the escape hatch if scale demands it. Not worth changing now.

P5 — LOW — SQLite per-call overheads

EnqueueAsync opens a (pooled) connection and re-runs two PRAGMAs per event (SqliteStoreAndForwardSink.cs:244-246); GetStatus runs a synchronous COUNT(*) per call (line 479-484). Both fine at alarm-event rates; the capacity fast path (cached counter, probe only near the wall — lines 271-290) already removed the real hot-path cost, and the drain-order index (IX_Queue_Drain) is right.

P6 — LOW — DependencyGraph is well optimized

Cached topological rank invalidated on mutation, shared empty set for misses, iterative Tarjan/Kahn (DependencyGraph.cs:34-44, 141-149). The per-cascade List/HashSet/Stack allocations in TransitiveDependentsInOrder are minor. No action.

P7 — LOW — NEW — Unconditional per-apply cache flush forces full VT recompilation on every deploy, including no-op redeploys

VirtualTagHostActor.OnApply clears the compile cache on every ApplyVirtualTags (VirtualTagHostActor.cs:92) — even an identical redeploy whose plans diff equal and whose children are all left untouched. Every VT script then recompiles on its next evaluation, each paying the full P1 reference-set rebuild (5-20 ms+ per script today). The host actor already computes exactly which plans changed (the _planByVtag diff at lines 109-118); the clear could be conditional on any-change, or the cache could evict by retained-source set. The alarm engine has the same unconditional per-generation shape but its LoadAsync only runs on actual config applies. Cheap to leave as-is at current scale; fix alongside P1.

3. Conventions

C1 — MEDIUM — ITagUpstreamSource is defined twice

Core.VirtualTags/ITagUpstreamSource.cs and a second, byte-for-byte-shaped interface at the bottom of ScriptedAlarmEngine.cs:860-872 ("intentionally identical shape so Stream G can compose them behind one driver bridge"). Two distinct .NET types means DependencyMuxTagUpstreamSource and any composing bridge must implement/adapt each separately, and the "identical shape" invariant is enforced only by comment. Recommendation: hoist a single ITagUpstreamSource into Core.Scripting.Abstractions (both projects already reference it) and retire the duplicate.

C2 — MEDIUM — Core.Scripting.Abstractions declares types across three namespaces

ScriptContext/ScriptGlobals/PassthroughScript claim namespace Core.Scripting, VirtualTagContext claims Core.VirtualTags, AlarmPredicateContext claims Core.ScriptedAlarms — all physically in the Abstractions assembly. The reason is sound (the sandbox pins contextType.Assembly, so concrete contexts must live in the Roslyn-free assembly to keep Roslyn out of the script compilation closure — ScriptSandbox.cs:60-70), but it is documented only in a PassthroughScript remark and violates the repo's namespace-follows-assembly norm; find-by-namespace navigation actively misleads here. Recommendation: add a project-level README or AssemblyInfo doc comment stating the rule ("this assembly is the sandbox-pinned closure; types keep their consumer namespaces deliberately"), or bite the bullet and align namespaces with a TypeForwardedTo migration.

C3 — LOW — Trailing type definitions in engine files

ScriptedAlarmEvent and the duplicate ITagUpstreamSource live at the bottom of ScriptedAlarmEngine.cs (843-872); CompilationErrorException and ScriptAssemblyLoadContext at the bottom of ScriptEvaluator.cs (391-432); DependencyCycleException in DependencyGraph.cs. Mostly reasonable co-location, but ScriptedAlarmEvent is a public cross-project contract and deserves its own file.

C4 — LOW — Repeated raw OPC UA status-code literals

0x80340000u /* BadNodeIdUnknown */, 0x80020000u /* BadInternalError */, 0x80320000u /* BadWaitingForInitialData */, 0x80000000u severity mask are hand-rolled with comments in at least five files (VirtualTagEngine.cs:215-218, 307, 328; VirtualTagContext.cs; AlarmPredicateContext.cs; ScriptedAlarmEngine.cs:584-587; and the remediation added another — RoslynVirtualTagEvaluator.BuildReadCache hard-codes StatusCode: 0u Good, plus VirtualTagHostActor.cs:188's 0u /* StatusCodes.Good */). A small KnownStatusCodes static in Core.Abstractions would end the copy-paste drift risk (these layers deliberately avoid the OPC Foundation package, so the constants can't come from there).

C5 — LOW — Core.AlarmHistorian mixes contract and implementation

IAlarmHistorianSink/IAlarmHistorianWriter/status types share the project with SqliteStoreAndForwardSink, while scripting got a dedicated Abstractions assembly. Tolerable at three files, but the Runtime adapter and the gateway driver both reference the whole project (dragging the Microsoft.Data.Sqlite dependency) to get the interfaces.

C6 — LOW — Plan-era naming residue

XML docs throughout reference "Phase 7 plan Stream A.4/B/C/E/F/G" (TimedScriptEvaluator.cs:5, 39; ScriptSandbox.cs; IAlarmStateStore.cs:13; many more). The repo already purged the meaningless Phase7 type prefixes (master 40e8a23e), and the 3b5ef439 fixdocs sweep stripped arch-review IDs elsewhere — but these plan-stream references survived both passes and now point at plans superseded by docs/ScriptedAlarms.md/docs/ScriptEditor.md. Sweep them to current doc anchors during the next doc pass.

C7 — LOW — Doc-drift: ApplyPredicate claims branch handling that doesn't exist

Part9StateMachine.ApplyPredicate's summary promises "branch-stack increment when a new active arrives while prior active is still un-acked" (Part9StateMachine.cs:31-34), but no branch/previous-instance logic exists anywhere in the file — re-activation simply resets Acked/Confirmed. See U5; at minimum fix the comment.

C8 — LOW — NEW — VT script timeout is hard-coded and inconsistent with the alarm budget

Production registration passes no runTimeout (Host/Program.cs:217-221), so the live VT budget is the ctor default 2 s (RoslynVirtualTagEvaluator.cs:45) while alarm predicates default to TimedScriptEvaluator.DefaultTimeout 250 ms (ScriptedAlarmEngine.cs:160). Neither is operator-configurable despite the Scripting:* config section already existing for log settings (Program.cs:227-229) and TimedScriptEvaluator's doc claiming "configurable per tag so deployments with slower backends can widen it". A Scripting:EvaluationTimeoutMs (or per-engine keys) would make the budgets deliberate instead of incidental — and matters more now that the timeout is load-bearing (S7/S13).

4. Underdeveloped Areas

U1 — HIGH — The entire Core.VirtualTags engine stack is dormant in production

VirtualTagEngine (568 lines), TimerTriggerScheduler (162), VirtualTagSource (106), and DependencyGraph (324) have no production instantiation — repo-wide grep at f6eaa267 still finds new VirtualTagEngine only in tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests, and AdminUI/ScriptAnalysis/ScriptAnalysisService.cs:293 documents the fact in passing ("the cascade VirtualTagEngine is dormant"). The live virtual-tag runtime is Runtime/VirtualTags/VirtualTagHostActor + DependencyMuxActor + Host/Engines/RoslynVirtualTagEvaluator. The prior review's sharpest edge is now blunted — since 7fd44f0f the live adapter no longer bypasses the scope's safety wrappers (U2/U3 fixed), so the dormant engine's remaining unique value (topo-ordered cascade, cycle detection, timer triggers, IReadable adaptation) is squarely "unshipped feature", not "unshipped defense". That strengthens the case for retiring it: ~1,160 lines of engine + ~1,260 lines of tests still being maintained and carrying latent bugs (S1/S3/S10) for a component nothing runs. Recommendation: decide explicitly. Either (a) retire VirtualTagEngine/TimerTriggerScheduler/VirtualTagSource (keep DependencyGraph only if the publish-time cycle check uses it — verify) and let the actor pipeline be the one implementation, or (b) document the engine as the sanctioned embedded/non-Akka path and fix S1-S3. Option (a) is now clearly preferable.

U4 — MEDIUM — IHistoryWriter is a permanently-Null surface

VirtualTagDefinition.Historize routes to IHistoryWriter.Record, but DI binds only NullHistoryWriter (Runtime/ServiceCollectionExtensions.cs:57-58, 241-242 — "durable AVEVA sink is infra-gated; a deployment binding a real IHistoryWriter overrides"). No real implementation exists in the tree; the operator-visible Historize checkbox on virtual tags is a silent no-op (the separate ContinuousHistorization path taps the mux directly). Either wire IHistoryWriter to the gateway value-writer or hide/annotate the checkbox until it does something.

U5 — MEDIUM — Part 9 surface gaps are real but undeclared in one place

No condition branches / previous-instances (re-activation while un-acked just resets Acked — and the xmldoc overpromises, C7); Severity is static per definition ("not currently computed by the predicate", ScriptedAlarmDefinition.cs); AlarmKind affects only node typing; MessageTemplate has no brace escaping (documented as "reach for a feature request", MessageTemplate.cs:16-19); Retain is carried on the definition but not consulted anywhere in this scope. Each is a defensible v1 cut; collect them in docs/ScriptedAlarms.md as an explicit conformance statement so OPC UA client integrators know what subset to expect.

U6 — LOW — No TODO/HACK/stub markers

Grep found zero TODO|HACK|FIXME|NotImplemented across all five projects — unfinished edges are instead documented in XML remarks (a healthier pattern, though it makes gaps greppable only by reading).

U7 — LOW (was MEDIUM-leaning) — Test coverage: broad and behavior-focused, with specific holes

~6,690 test lines against ~5,760 source lines, all four impl projects have dedicated suites: SqliteStoreAndForwardSinkTests (888 — outcomes, poison cap, corrupt payloads, capacity fast path), ScriptedAlarmEngineTests (1,270), ScriptSandboxTests (614 — the escape catalog), CompiledScriptCacheTests (409 — including ALC-reclaim via WeakReference), VirtualTagEngineTests (800), state machine, extractor, graph, loggers, sources. Closed since the prior review: hole (e) — the production RoslynVirtualTagEvaluator timeout is now covered (Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs: WaitAsync-bounded infinite-loop test, happy-path-after-wrapping, ClearCompiledScripts emptying), and the apply-boundary clear has a production-caller wiring guard (Runtime.Tests/VirtualTags/VirtualTagHostActorTests.ApplyVirtualTags_clears_the_evaluator_compile_cache_each_generation — two generations, proving per-generation not one-shot). Remaining holes: (a) S5's expiry-during-predicate emission swallow — no Part9StateMachineTests case; (b) no concurrency test for load-vs-cascade (would expose S1/S4) — and now also none for clear-vs-in-flight-evaluate (would expose S12); (c) no sink Dispose-during-drain test (S6); (d) ForbiddenTypeAnalyzer has no dedicated unit suite (exercised only through end-to-end Compile in sandbox tests); (f — new) nothing asserts node quality degrades on persistent script failure (would expose S13).

---

Maturity Ratings

Scale: 1 (prototype) → 5 (production-hardened). Rating reflects the scoped code as it stands, including how it is (or isn't) consumed. Old rating = 2026-07-08 @ 9cad9ed0.

Dimension	Old → New	Justification
Stability	3.5 → 3.5	The worst live-path availability risk (a hung VirtualTagActor) is gone and the fix carries real, hang-proof regression tests — but every Core-engine stability finding (S1-S11) is untouched, and the remediation itself introduced two moderates: the apply-boundary clear races in-flight evaluations (S12), and failure/timeout never reaches node quality so stale values stay Good (S13). Net: same grade, different failure modes — bounded-loss instead of wedged.
Performance	3 → 3	P1 (per-compile BCL reference-set rebuild) still unfixed and now amplified by the unconditional per-apply cache flush (P7); the hot path additionally pays the (accepted) pool hop + actor-thread block. Right structures elsewhere unchanged.
Conventions	4 → 4	The remediation code follows repo norms well (why-comments, narrow capability seam mirroring the repo's optional-interface pattern, Null-object-safe cast). C1-C7 all still open; C8 (hard-coded, inconsistent timeout budgets) is new but minor.
Underdeveloped areas	2 → 3	The dimension's defining problem — "the live path bypasses the scope's two key defenses" — is fixed and guard-tested (U2/U3), and the production timeout finally has tests (U7-e). Still holding it below 4: the dormant 1,160-line engine remains unretired with its latent bugs (U1), Historize is still a silent Null no-op (U4), and the Part 9 conformance statement still doesn't exist (U5).

---

Priority Recommendations (ordered)

1. S13 — Propagate script failure/timeout to node quality (the AttributeValueUpdate plumbing already carries OpcUaQuality; the bridge hard-codes Good). Without it, the new timeout converts a visible hang into an invisible stale-Good value — arguably a worse operator experience for a persistent fault.
2. S12 — Close the clear-vs-in-flight-evaluate race in RoslynVirtualTagEvaluator (catch-ObjectDisposedException-recompile-retry is a 10-line fix), with a concurrency test.
3. P1 (+P7) — Statically cache the ScriptSandbox reference set; the per-apply cache flush makes every deploy pay the full rebuild for every VT script, so this is now a deploy-latency issue, not just editor latency.
4. U1 — Retire the dormant VirtualTagEngine stack. With U2/U3 fixed, it no longer holds any defense the live path lacks; keeping it means maintaining S1/S3/S10 in dead code.
5. S7 — Per-script circuit breaker (N consecutive timeouts → quarantine + Admin UI surface): the timeout now protects both engines, but each timed-out attempt still orphans a thread and (VT path) blocks an actor 2 s — a hot dependency on a wedged script is a slow pool-starvation vector.
6. S2 — Add dirty-set coalescing to ScriptedAlarmEngine upstream fan-out before a fast tag meets a popular alarm in production.
7. S5 + C7 — Fix the timed-shelve expiry emission and the state-machine doc drift together, with a state-machine test.
8. S3/S4/S6 — Schedule the reload-swap, _alarmsReferencing synchronization, and sink dispose-drain as one hardening pass.