Add the 7 per-domain design+implementation plans (archreview/plans/) with an index, produced from the 2026-07-08 architecture review. Fix two confirmed doc drifts the review flagged (theme #5): - CLAUDE.md KNOWN LIMITATION 2: the continuous-historization historized-ref feed IS wired (AddressSpaceApplier.FeedHistorizedRefs -> UpdateHistorizedRefs -> recorder); rewrite to reflect that value-capture is code-complete and only the live end-to-end + restart-convergence verification remains. - CLAUDE.md ScriptAnalysis gating: endpoints use Roles=Administrator,Designer via RequireAuthorization, not the FleetAdmin policy.
34 KiB
Architecture Review 02 — Scripting, Virtual Tags, Scripted Alarms, Alarm Historian
- Date: 2026-07-08
- Commit:
9cad9ed0 - 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)- Test projects under
tests/Core/for coverage assessment
- Method: full read of the load-bearing files (ScriptEvaluator, ScriptSandbox, ForbiddenTypeAnalyzer, CompiledScriptCache, TimedScriptEvaluator, VirtualTagEngine, ScriptedAlarmEngine, Part9StateMachine, SqliteStoreAndForwardSink) plus their support types; production-consumer tracing into
Runtime/Hostto distinguish live vs dormant code; TODO/stub grep; test-breadth vs source-surface comparison.
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:
- Parse the wrapper source with
#line 1remapping so diagnostics point at operator source. - Wrapper-injection guard (
EnforceSingleRunMember, ScriptEvaluator.cs:222) — rejects brace-balanced injections that declare sibling members/types (diagnostics LMX001/LMX002). - Roslyn compile against the
ScriptSandboxreference set (pinned OtOpcUa assemblies +System.*/netstandard TPA subset) — the allow-list is explicitly not the security boundary (type forwarding makes it porous). ForbiddenTypeAnalyzer— the real gate. Two semantic passes: (1) member/call surface viaGetSymbolInfoon creations/invocations/member-access/identifiers; (2) everyTypeSyntaxviaGetTypeInfo, 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).- Emit to an in-memory PE, load into a per-script collectible
AssemblyLoadContext, bind a staticFunc<ScriptGlobals<TContext>,TResult>delegate.Dispose()unloads the ALC.
Around the core evaluator sit three wrappers: CompiledScriptCache (SHA-256-source-keyed, Lazy single-compile, disposes ALCs on Clear() at publish-replace), 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 (Core.VirtualTags) — two implementations, one live
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.
However, production does not run this engine. The live path is VirtualTagHostActor → per-tag VirtualTagActor → IVirtualTagEvaluator bound to Host/Engines/RoslynVirtualTagEvaluator (single-tag adapter; fan-out owned by DependencyMuxActor). Grep confirms VirtualTagEngine, TimerTriggerScheduler, VirtualTagSource, and DependencyGraph have no production instantiation — they are exercised only by their unit tests (see U1). Only VirtualTagContext, IHistoryWriter/NullHistoryWriter, and PassthroughScript cross into the live path.
Scripted-alarm flow (Core.ScriptedAlarms) — live end to end
ScriptedAlarmHostActor (Runtime) owns a long-lived ScriptedAlarmEngine and calls LoadAsync per config apply (generation-tagged to discard stale completions). Inside the engine:
LoadAsync(under_evalGate) tears down the prior generation (shelving timer, subscriptions, alarms, scratch, compile-cache ALCs), compiles every predicate viaCompiledScriptCache<AlarmPredicateContext,bool>, builds an inverse index tag-path → alarm ids (alarms are DAG leaves; no topo sort), seeds the value cache, restores persistedAlarmConditionStatefromIAlarmStateStore(EF-backed in prod, in-memory for dev/tests), re-derivesActiveStatefrom the live predicate (startup recovery), then subscribes upstream and starts the 5 s shelving-check timer.- Upstream changes (
OnUpstreamChange, fed byDependencyMuxTagUpstreamSource) update the value cache and spawn tracked fire-and-forgetReevaluateAsynctasks that serialize on_evalGate, run the predicate throughTimedScriptEvaluatoragainst a reused per-alarmAlarmScratch(read-cache dictionary + context, refilled in place — the hot path allocates nothing), and apply the result throughPart9StateMachine.ApplyPredicate. Part9StateMachineis a pure-function transition table over the immutableAlarmConditionStaterecord (Enabled/Active/Acked/Confirmed/Shelving + auditCommentsasImmutableList). Operator verbs (Acknowledge/Confirm/OneShotShelve/TimedShelve/Unshelve/Enable/Disable/AddComment) come in via the engine'sApplyAsync(persist-before-update-in-memory; emissions built under the gate, fired after release to avoid re-entrancy deadlock).- Emissions (
ScriptedAlarmEvent) fan out throughOnEvent→ScriptedAlarmSource(theIAlarmSourceadapter with equipment-path-prefix subscription filters) into the Part 9 condition nodes //alerts/ historian adapter, carrying the per-alarmHistorizeToAvevaopt-out.MessageTemplateresolves{TagPath}tokens at emission with a stricter-than-predicate quality bar (Good only; else{?}).
Alarm-history write path (Core.AlarmHistorian)
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).
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
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 — under change-triggered evaluation a hot looping script orphans a thread per upstream change until the pool starves, since the engine keeps re-evaluating (alarm engine holds prior state on timeout, ScriptedAlarmEngine.cs:535-538, but does not quarantine the predicate).
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) is notably strong — including the type-forwarding rationale, var-inference coverage via GetTypeInfo, and the Unsafe/Activator/ThreadPool type-granular entries.
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.
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.
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. For the alarm engine this happens once per referencing alarm per upstream change. The hop is required for the timeout to work at all (see U2 for what happens without it), but 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. Inconsistent — moot while the VT engine is dormant (U1), but whichever way U1 resolves, the two should match.
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.
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). 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); the doc references now point at plans that have been 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.
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 finds new VirtualTagEngine only in tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests. The live virtual-tag runtime is Runtime/VirtualTags/VirtualTagHostActor + DependencyMuxActor + Host/Engines/RoslynVirtualTagEvaluator (single-tag adapter, IVirtualTagEvaluator seam), which re-implements read-cache building, cross-tag-write handling (drops them — the actor mux owns fan-out), timeout, and compile caching independently. ~1,160 lines of engine + ~1,260 lines of tests are being maintained, reviewed, and (per S1/S3) carrying latent bugs for a component nothing runs. The RoslynVirtualTagEvaluator doc comment even acknowledges the split ("Cycle detection + cascade ordering live in VirtualTagEngine; this adapter stays single-tag scoped").
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. The current state — two divergent implementations where the tested one is dormant and the live one bypasses the safety wrappers (U2/U3) — is the worst of both.
U2 — CRITICAL (cross-cutting; defect sits in Host, root cause is a dormant in-scope component) — Production script timeout is ineffective: TimedScriptEvaluator is bypassed on the live path
RoslynVirtualTagEvaluator.Evaluate runs evaluator.RunAsync(context, cts.Token).GetAwaiter().GetResult() with a 2 s CancellationTokenSource (Host/Engines/RoslynVirtualTagEvaluator.cs:102-106). But ScriptEvaluator.RunAsync executes the compiled delegate synchronously on the calling thread and only checks the token before invoking it (ScriptEvaluator.cs:178-190 — "TimedScriptEvaluator wraps this in Task.Run so a long-running script still honours its wall-clock budget"). The token can therefore never interrupt a running script: the catch (OperationCanceledException) timeout branch in the Host adapter is dead code, and a CPU-bound or infinite-loop virtual-tag script hangs the owning VirtualTagActor's message loop indefinitely — no timeout, no log, no recovery. The in-scope TimedScriptEvaluator exists precisely to prevent this and is used by the (live) alarm engine and the (dormant) VT engine — but not by the one component that evaluates operator-authored virtual-tag scripts in production.
Recommendation: wrap the adapter's evaluator in TimedScriptEvaluator (accepting the documented orphan-thread trade-off), or at minimum Task.Run + WaitAsync inside Evaluate. Add a regression test with a while(true) script. This should be fixed before any other finding in this report.
U3 — HIGH (same file) — Live path also bypasses CompiledScriptCache: unbounded ALC accretion across republishes
RoslynVirtualTagEvaluator caches compiled evaluators in a raw ConcurrentDictionary<string, ScriptEvaluator<…>> keyed by source (RoslynVirtualTagEvaluator.cs:25-26, 69), never evicted until process dispose. Every edited-script republish adds a new entry while the old source's evaluator — and its collectible AssemblyLoadContext — stays rooted forever. This is exactly the "per-publish recompile accretion" leak that CompiledScriptCache.Clear() was built to fix (its own doc, CompiledScriptCache.cs:34-40, and the engine comments at ScriptedAlarmEngine.cs:66-77 describe the production leak from skipping it). Additionally, GetOrAdd with a value factory can compile twice under a race and silently leak the losing ALC (the cache solved this with Lazy + ExecutionAndPublication).
Recommendation: switch the adapter to CompiledScriptCache<VirtualTagContext, object?> and clear it on config apply (the host actor already sees apply boundaries).
U4 — MEDIUM — IHistoryWriter is a permanently-Null surface
VirtualTagDefinition.Historize routes to IHistoryWriter.Record, but DI binds only NullHistoryWriter (Runtime/ServiceCollectionExtensions.cs:57-58, 236-237 — "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 and is itself gated + ref-set-empty per CLAUDE.md Known Limitation 2). 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 — 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. Holes worth filling: (a) S5's expiry-during-predicate emission swallow — no Part9StateMachineTests case; (b) no concurrency test for load-vs-cascade (would expose S1/S4); (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 — fine functionally, but analyzer-pass regressions produce opaque failures); (e) nothing tests the production RoslynVirtualTagEvaluator timeout (which would have caught U2 — it lives in Host, whose test project should own it).
Maturity Ratings
Scale: 1 (prototype) → 5 (production-hardened). Rating reflects the scoped code as it stands, including how it is (or isn't) consumed.
| Dimension | Rating | Justification |
|---|---|---|
| Stability | 3.5 | Alarm engine + sink show genuinely careful engineering (gate discipline, emit-outside-gate, dispose drains, persist-before-memory, poison-row caps); dragged down by fail-stop reloads, unbounded change fan-out, the VT engine's un-gated Load, and the shelve-expiry emission swallow. |
| Performance | 3 | Right structures where it counts (compile cache, alarm scratch reuse, graph rank cache, SQLite capacity fast path), but every compile rebuilds the full BCL reference set (P1), every evaluation pays a pool hop, and one global gate serializes each engine. |
| Conventions | 4 | Strongly consistent with repo norms (Serilog, Null-object defaults, xUnit/Shouldly, exemplary "why"-comments); duplicated ITagUpstreamSource, the three-namespace Abstractions assembly, and literal status codes are the debt. |
| Underdeveloped areas | 2 | A complete, tested virtual-tag engine nobody runs, while the live path bypasses the scope's two key defenses (timeout → hung actor risk, ALC cache → leak); Historize is a Null no-op; Part 9 gaps undeclared. The split between "what is built and tested" and "what actually runs" is the defining problem of this subsystem. |
Priority Recommendations (ordered)
- U2 — Wrap the production virtual-tag evaluation in
TimedScriptEvaluator(or equivalent). A single bad script can currently hang aVirtualTagActorforever with no timeout. - U3 — Use
CompiledScriptCache(with apply-boundaryClear) inRoslynVirtualTagEvaluatorto stop cross-publish ALC accretion. - P1 — Statically cache the
ScriptSandboxreference set; large publish-time and script-editor latency win for one small change. - U1 — Decide the fate of the dormant
VirtualTagEnginestack (retire preferred); until then S1/S3 fixes are latent-bug maintenance on dead code. - S2 — Add dirty-set coalescing to
ScriptedAlarmEngineupstream fan-out before a fast tag meets a popular alarm in production. - S5 + C7 — Fix the timed-shelve expiry emission and the state-machine doc drift together, with a state-machine test.
- S3/S4/S6 — Schedule the reload-swap,
_alarmsReferencingsynchronization, and sink dispose-drain as one hardening pass.