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 `VirtualTagActor`s 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.