lmxopcua/archreview/plans/02-scripting-alarms-plan.md

Design + Implementation Plan — 02 Scripting, Virtual Tags, Scripted Alarms, Alarm Historian

Status (2026-07-08): ✅ U2 (Critical) + U3 (High) DONE — branch fix/archreview-crit2-vt-timeout @ 7fd44f0f (real TimedScriptEvaluator timeout + CompiledScriptCache + IScriptCacheOwner apply-boundary clear; VT tests 18/18 + 12/12). Remaining: P1 (memoize ScriptSandbox.Build), U1 (retire dormant Core.VirtualTags engine — after U2/U3 hardened the live path), and the S/C/P batches. See STATUS.md.

• Source report: archreview/02-scripting-alarms.md
• Review commit: 9cad9ed0 · Plan verified against tree at: 9cad9ed0 (master)
• Scope: Core.Scripting(+Abstractions), Core.VirtualTags, Core.ScriptedAlarms, Core.AlarmHistorian, plus the live consumers in Host/Runtime.

Verification summary

Every finding in the report was opened at the cited file:line and checked against the current tree. All findings CONFIRMED — none stale. Line numbers in the report match the current source. Key confirmations:

• U2 — RoslynVirtualTagEvaluator.Evaluate (Host/Engines/RoslynVirtualTagEvaluator.cs:101-108) builds a CancellationTokenSource(_runTimeout) and calls evaluator.RunAsync(context, cts.Token).GetAwaiter().GetResult(). ScriptEvaluator.RunAsync (Core.Scripting/ScriptEvaluator.cs:178-190) runs the compiled delegate synchronously (var result = _func(globals); line 188) and only checks the token at entry (line 182). The CTS cannot interrupt a running script; the catch (OperationCanceledException) branch (line 105-108) is dead. VirtualTagActor.OnDependencyChanged (Runtime/VirtualTags/VirtualTagActor.cs:113) calls _evaluator.Evaluate(...) inline in the actor's message handler → a while(true) script hangs that actor's message loop forever. Registered live in Host/Program.cs:217-221. Confirmed.
• U3 — same file: _cache is a raw ConcurrentDictionary<string, ScriptEvaluator<…>> keyed by expression source (RoslynVirtualTagEvaluator.cs:25-26), populated via GetOrAdd(expression, …Compile) (line 67), only ever cleared in Dispose() (line 132-141). No apply-boundary eviction → cross-publish ALC accretion. GetOrAdd value-factory can double-compile under a race and leak the losing ALC. Confirmed.
• P1 — ScriptEvaluator.Compile calls ScriptSandbox.Build(typeof(TContext)) (line 74) on every compile; Build runs MetadataReference.CreateFromFile for every pinned assembly + every System.*/netstandard TPA path (ScriptSandbox.cs:83-87, EnumerateBclAssemblyPaths 100-130). Immutable per contextType.Assembly, rebuilt every call. Confirmed.
• U1 — new VirtualTagEngine, TimerTriggerScheduler, new VirtualTagSource appear only under tests/Core/…Core.VirtualTags.Tests. No production instantiation. DependencyGraph (Tarjan/Kahn) has no production consumer — repo grep for topo/cycle in the live path returns only unrelated matches (MemoryRecycle, resilience). Live cascade fan-out is DependencyMuxActor (no topo sort; cross-tag writes dropped). Confirmed.
• S1–S6, S8, S10, S11, P2–P6, C1, C3, C7, U4, U5 — all confirmed at cited lines (details in each section below).

---

Priority ordering

Ordered by the report's Priority Recommendations and the OVERALL prioritized action list (item #2 = U2/U3):

1. U2 (Critical) — production VT script timeout ineffective
2. U3 (High) — live path bypasses CompiledScriptCache; ALC accretion
3. P1 (High) — ScriptSandbox.Build rebuilds full BCL ref set every compile
4. U1 (High) — dormant Core.VirtualTags engine stack
5. S2 (High) — no coalescing/backpressure on upstream fan-out
6. S1 (High) — VirtualTagEngine.Load un-gated (resolves with U1)
7. S5 + C7 (Medium) — timed-shelve expiry swallows Unshelved + doc drift
8. S3 / S4 / S6 (Medium) — reload-swap, _alarmsReferencing sync, sink dispose-drain (one hardening pass)
9. U4 / U5 / C1 / C2 / C5 (Medium) — Null Historize, Part 9 conformance doc, dup interface, namespace doc, contract split
10. Lows (batched) — S7, S8, S9, S10, S11, P2–P6, C3, C4, C6, U6, U7

---

1. U2 — CRITICAL — Production virtual-tag script timeout is ineffective

Restatement: RoslynVirtualTagEvaluator runs scripts on the calling (actor) thread with a token that can never interrupt them; a CPU-bound/infinite-loop virtual-tag script hangs the owning VirtualTagActor forever — no timeout, no log, no recovery.

Verification: Confirmed (see summary). The dead code is RoslynVirtualTagEvaluator.cs:105-108. Root cause: ScriptEvaluator.RunAsync is synchronous by design (line 184-189 documents "TimedScriptEvaluator wraps this in Task.Run…"), but the Host adapter never wraps it — it hand-rolls a CancellationTokenSource that a synchronous _func(globals) invocation ignores.

Root cause: The one component that evaluates operator-authored VT scripts in production (RoslynVirtualTagEvaluator) reimplemented timeout handling incorrectly instead of reusing TimedScriptEvaluator, which was built for exactly this and is already used by the live alarm engine (ScriptedAlarmEngine.cs:227) and the dormant VT engine (VirtualTagEngine.cs:117).

Proposed design: Route the adapter's execution through TimedScriptEvaluator<VirtualTagContext, object?>, accepting the documented orphan-thread trade-off (S7) — this is the sanctioned mechanism (Task.Run + WaitAsync) that makes the wall-clock budget real for CPU-bound scripts. Alternatives considered:

• Inline Task.Run + WaitAsync in Evaluate — duplicates TimedScriptEvaluator logic (the exact anti-pattern U1/theme-#2 warns against). Rejected in favor of reuse.
• Out-of-process runner — the real fix for CPU budgeting but a v3 concern (per TimedScriptEvaluator docs); out of scope.

Because TimedScriptEvaluator wraps a single ScriptEvaluator, and the adapter caches evaluators per source, the wrapper is constructed per compiled entry. Combine with U3: the cache value should become the wrapped/timed evaluator (or a small record holding both). Simplest coherent shape — cache the ScriptEvaluator (from CompiledScriptCache, U3) and construct a TimedScriptEvaluator per call (cheap — it's a thin struct-like wrapper over the inner evaluator; the inner compiled delegate is what's expensive and that stays cached).

Implementation steps:

1. In RoslynVirtualTagEvaluator.Evaluate (Host/Engines/RoslynVirtualTagEvaluator.cs), replace the block at lines 99-113:
   • Construct var timed = new TimedScriptEvaluator<VirtualTagContext, object?>(evaluator, _runTimeout);
   • Call var raw = timed.RunAsync(context).GetAwaiter().GetResult(); (no CTS needed — the wall-clock budget lives in TimedScriptEvaluator).
   • Change the catch to catch (ScriptTimeoutException) { return VirtualTagEvalResult.Failure($"script timed out after {_runTimeout.TotalSeconds:F1}s"); } (keep the generic catch (Exception) for user-code faults).
2. Keep _runTimeout default 2 s (existing). Optionally expose per-tag timeout later (out of scope).
3. Note the orphan-thread interaction with S7: a hot looping script now orphans one pool thread per evaluation attempt. Track the S7 circuit-breaker as a fast follow (below) — but U2's fix (stop hanging the actor) is strictly better than today regardless.

Tests (unit + regression):

• Add to the existing tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs:
  • Evaluate_WithInfiniteLoopScript_ReturnsFailureWithinTimeout — expression while(true){} return 0; (or a spin that exceeds the budget); assert result.Success == false, result.Reason contains "timed out", and the call returns within ~timeout + slack (wrap in its own Task.Run + WaitAsync(5s) so a regression fails the test instead of hanging the suite).
  • Evaluate_WithFastScript_StillReturnsValue — regression that the wrapping didn't break the happy path or the passthrough fast-path.
• Optionally an actor-level test in Runtime.Tests proving VirtualTagActor continues processing messages after a timed-out evaluation (message loop not wedged).

Effort: S. Risk/blast-radius: Low-Medium. Touches the single live VT evaluation path; the happy path is unchanged (passthrough fast-path still short-circuits before compile). The behavioral change is that a runaway script now returns Bad instead of hanging — strictly safer. Combine the edit with U3 (same method).

---

2. U3 — HIGH — Live path bypasses CompiledScriptCache: unbounded ALC accretion

Restatement: RoslynVirtualTagEvaluator caches compiled evaluators in a raw ConcurrentDictionary never evicted on republish; every edited-script publish roots a new collectible AssemblyLoadContext forever, and the GetOrAdd factory can double-compile under a race and leak the loser.

Verification: Confirmed (RoslynVirtualTagEvaluator.cs:25-26, 67, 132-141). This is the same leak CompiledScriptCache.Clear() was built to fix (CompiledScriptCache.cs:34-40, 87-118) and that both engines already route through (ScriptedAlarmEngine.cs:66-77, VirtualTagEngine.cs:26-38). The adapter is the lone hold-out.

Root cause: Adapter reimplemented caching with a plain dictionary + value-factory GetOrAdd instead of the purpose-built CompiledScriptCache (Lazy + ExecutionAndPublication single-compile, dispose-on-Clear). No apply-boundary Clear() is called because the adapter doesn't see config-apply — but the host actor does.

Proposed design: Swap the raw dictionary for CompiledScriptCache<VirtualTagContext, object?> and give the adapter a Clear()/OnConfigApply() seam the host actor invokes at each deploy boundary. Two wiring options for the apply-boundary clear:

• (a) Adapter exposes void ClearCompiledScripts(); VirtualTagHostActor calls it when it (re)loads a plan generation. Preferred — mirrors how ScriptedAlarmHostActor drives ScriptedAlarmEngine.LoadAsync at apply, and how the alarm engine's _compileCache.Clear() runs per generation.
• (b) IVirtualTagEvaluator gains an optional Clear member. Broader surface change; the interface has a NullVirtualTagEvaluator and other consumers — only do this if the host actor can't reach the concrete type. Given Host/Program.cs:217-221 registers the concrete RoslynVirtualTagEvaluator as a singleton AND as IVirtualTagEvaluator, the host actor can resolve the concrete singleton; prefer (a) but if the actor only has the interface, add a narrow IScriptCacheOwner { void ClearCompiledScripts(); } the adapter implements and the actor optionally casts to (theme #1: assert the wiring with a test).

Implementation steps:

1. In RoslynVirtualTagEvaluator: replace _cache with private readonly CompiledScriptCache<VirtualTagContext, object?> _cache = new();
2. Replace the GetOrAdd at line 67 with _cache.GetOrCompile(expression) inside the same try/catch (it throws the same CompilationErrorException/ScriptSandboxViolationException).
3. Add public void ClearCompiledScripts() => _cache.Clear();
4. Dispose() → _cache.Dispose() (drops the per-evaluator dispose loop; CompiledScriptCache.Dispose already disposes every materialised ALC).
5. Wire the apply-boundary clear: in Runtime/VirtualTags/VirtualTagHostActor.cs, at the point it receives a new plan generation / rebuilds its child VirtualTagActor set, call ClearCompiledScripts() on the injected evaluator (resolve the concrete type, or via the narrow interface from option (b)). Verify the host actor already has an apply/rebuild message it handles — if it recreates children per generation, that handler is the hook.
6. Confirm CompiledScriptCache.GetOrCompile's Lazy single-compile removes the double-compile race automatically.

Tests:

• Unit (Host.IntegrationTests or a new RoslynVirtualTagEvaluator unit test): compile source A, then call ClearCompiledScripts(), assert the prior evaluator's ALC is reclaimed via WeakReference + GC.Collect() (mirror CompiledScriptCacheTests ALC-reclaim test at tests/Core/…Core.Scripting.Tests/CompiledScriptCacheTests.cs).
• Wiring assertion (theme #1): a VirtualTagHostActor test proving a config-apply/rebuild triggers ClearCompiledScripts (spy evaluator or count cache entries before/after).

Effort: S. Risk: Low. Behavior-preserving except that stale ALCs are now released. Do in the same PR as U2 (same file). Blast radius = VT evaluation + one host-actor hook.

---

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

Restatement: Each compile re-reads 100+ System.*/netstandard DLLs into fresh MetadataReferences; a 200-script publish pays this ~200×, and the AdminUI ScriptAnalysis endpoints pay it per keystroke-cadence request.

Verification: Confirmed (ScriptSandbox.cs:47, 83-87, 100-130; called from ScriptEvaluator.cs:74). The reference list is a pure function of contextType.Assembly (the only per-call input) and the process TPA set — immutable for the process lifetime.

Root cause: No memoization; MetadataReference.CreateFromFile (which loads + parses AssemblyMetadata) runs unconditionally per compile.

Proposed design: Memoize the built SandboxConfig (references + imports) in a static readonly ConcurrentDictionary<Assembly, SandboxConfig> keyed by contextType.Assembly. The pinned OtOpcUa assemblies and the BCL path set are stable per key. MetadataReference instances are immutable and thread-safe to share across compilations (Roslyn is designed for this). Alternative: cache just the BCL List<MetadataReference> (process-global, key-independent) and rebuild only the 4 pinned entries per key — marginal extra win, more code. Prefer caching the whole SandboxConfig per contextType.Assembly (there are only ~3 distinct context assemblies in practice: VirtualTags, ScriptedAlarms, test contexts).

Implementation steps:

1. In ScriptSandbox, add private static readonly ConcurrentDictionary<Assembly, SandboxConfig> _cache = new();
2. Change Build body to return _cache.GetOrAdd(contextType.Assembly, static asm => BuildUncached(asm)); — but note Build currently takes contextType and validates ScriptContext-assignability. Keep the validation in Build (before the cache lookup, since it's cheap and guards the contract), then key the cache on contextType.Assembly. Move the reference-list construction (lines 54-97) into a private BuildUncached(Type contextType) / or key on assembly and pin typeof(DataValueSnapshot).Assembly etc. which are constant.
   • Caveat: the four pinned assemblies at lines 59-70 include contextType.Assembly itself — keying on contextType.Assembly keeps them correct.
3. No API change to callers; SandboxConfig is already an immutable record.

Tests:

• ScriptSandboxTests (tests/Core/…Core.Scripting.Tests/ScriptSandboxTests.cs): assert two Build(sameContextType) calls return the same SandboxConfig reference (memoized), and Build for two different context types returns configs whose reference sets both resolve ctx.GetTag. Assert the existing escape-catalog tests still pass (they exercise the analyzer through Compile, which will now hit the cache).
• Optional micro-benchmark note in the PR: compile-latency before/after (report predicts 10-100×).

Effort: S. Risk: Low. MetadataReference sharing across compilations is a supported Roslyn pattern. The only subtlety is the contextType.Assembly key correctly capturing the pinned-set variation. Blast radius = all script compilation (VT, alarm, ScriptAnalysis) — but purely a perf/allocation change; output is byte-identical.

---

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

Restatement: VirtualTagEngine (568 lines), TimerTriggerScheduler (162), VirtualTagSource (106), DependencyGraph (324) have no production instantiation; the live path (VirtualTagHostActor + DependencyMuxActor + RoslynVirtualTagEvaluator) reimplements the same concerns. ~1,160 engine + ~1,260 test lines maintained for dead code that carries latent bugs (S1, S3, P3).

Verification: Confirmed — new VirtualTagEngine/TimerTriggerScheduler/new VirtualTagSource only in tests/Core/…Core.VirtualTags.Tests. DependencyGraph has no production consumer (grep for Tarjan/topo/cycle in the live path finds only unrelated MemoryRecycle/resilience). RoslynVirtualTagEvaluator's own xmldoc acknowledges the split (lines 20-21).

Root cause: The Akka actor pipeline (F8b) superseded the embedded engine, but the engine was never retired — a "built-but-never-wired" instance (OVERALL theme #1).

Proposed design — RETIRE (report's preferred option (a)). The live actor pipeline is the single sanctioned implementation. Once U2/U3 land, the actor path has the timeout + cache defenses the engine held, so nothing of value is lost. Retire:

• VirtualTagEngine.cs, TimerTriggerScheduler.cs, VirtualTagSource.cs, DependencyGraph.cs (+ DependencyCycleException) and their test suites (VirtualTagEngineTests, TimerTriggerSchedulerTests, VirtualTagSourceTests, DependencyGraphTests).
• Keep in Core.VirtualTags: ITagUpstreamSource (subject of C1 — hoist, don't delete), VirtualTagDefinition, IHistoryWriter/NullHistoryWriter (U4), and anything the composer/actor path references. Verify before deleting each type has no non-test consumer (grep each; VirtualTagContext lives in Abstractions and is consumed live, per C2).

Alternative — sanction the engine as an embedded/non-Akka path and fix S1/S3/P3. Rejected: doubles the maintenance surface with no consumer, and there is no roadmap need for a non-Akka embedded runtime. If one ever appears, the git history preserves the engine.

Retiring the engine subsumes S1, S3, P3 (they are latent bugs in the deleted code) and removes half of C1/C3's duplication pressure.

Implementation steps:

1. Grep each candidate type for non-test references (VirtualTagEngine, TimerTriggerScheduler, VirtualTagSource, DependencyGraph, DependencyCycleException). Confirm zero src/ consumers (done for the four engine types; re-verify at delete time).
2. Delete the four source files + their four test files; remove from any .csproj/slnx include if explicitly listed (they use globbing — likely no edit needed).
3. If ITagUpstreamSource is being hoisted (C1), do that first so the delete doesn't strand the interface.
4. Update docs/VirtualTags.md (referenced by VirtualTagEngine xmldoc) to describe only the live actor pipeline; drop engine-specific sections.
5. Build + run the full Core.VirtualTags.Tests (now trimmed) and the Runtime/Host suites.

Tests: No new tests; deletion. Confirm the remaining Core.VirtualTags.Tests (definition/IHistoryWriter/VirtualTagSource-adjacent) still build. Confirm Runtime.Tests VT-actor coverage is the surviving safety net.

Effort: M (mechanical but touches ~2.4k lines + verification). Risk: Low if the grep confirms no src/ consumers — but it is a large deletion, so land it after U2/U3 (so the live path is provably hardened first) and in its own PR for a clean revert. Blast radius = the dormant subtree only.

---

5. S2 — HIGH — No coalescing/backpressure on upstream-change fan-out

Restatement: Every upstream change spawns a fire-and-forget task queued on the single eval gate; a fast tag (e.g. 100 Hz) referenced by an alarm accumulates tasks without bound — memory growth, staleness, and wasted re-evaluation against the current cache.

Verification: Confirmed. ScriptedAlarmEngine.OnUpstreamChange (ScriptedAlarmEngine.cs:442-450) spawns one ReevaluateAsync per change via TrackBackgroundTask. The _inFlight set tracks but does not bound them. (The VT-engine sibling OnUpstreamChange at VirtualTagEngine.cs:263-272 is retired under U1 — S2 applies to the live alarm engine only after U1.)

Root cause: Per-event task spawning instead of a dirty-set + single pump; cost scales with event rate, not distinct dirty alarms.

Proposed design: Replace per-event ReevaluateAsync spawning with a dirty-set + single-consumer pump inside ScriptedAlarmEngine:

• OnUpstreamChange updates _valueCache[path] (unchanged), then for each referencing alarm id, _dirtyAlarmIds.Add(id) (a ConcurrentDictionary<string, byte> used as a set, or a HashSet under a lock), and signals a single long-lived pump (e.g. _pumpSignal.Release() on a SemaphoreSlim(0), or an AutoResetEvent/Channel<>).
• One pump loop (started in LoadAsync, stopped in Dispose) waits on the signal, drains the current dirty set under _evalGate, and re-evaluates each distinct dirty alarm once against the current cache. New changes during a pass simply re-mark dirty for the next pass — this is the coalescing.
• This makes cost proportional to distinct dirty alarms per drain, not to event rate, and naturally bounds memory (the dirty set is bounded by the alarm count).

Alternatives: bounded channel with drop-oldest (simpler but drops evaluations — acceptable since re-eval reads current cache, but a dirty-set is strictly better because it never loses the fact that an alarm needs re-eval); debounce timer per alarm (more timers, more complexity). Prefer the dirty-set + single pump — it also composes with a future VT need if one arises.

Reuse the existing _inFlight/drain discipline for the pump task (the pump is one tracked task; Dispose already drains _inFlight). Keep the shelving timer as-is.

Implementation steps:

1. Add _dirtyAlarmIds (concurrent set) + a signal primitive + a Task _pumpTask + a CancellationTokenSource _pumpCts to ScriptedAlarmEngine.
2. LoadAsync: after _loaded = true, start the pump loop (once per engine lifetime, not per load — or restart per load; simplest is start-once in the constructor and gate work on _loaded). Guard: the pump must acquire _evalGate and re-check _disposed/_loaded (mirror ReevaluateAsync lines 460-463).
3. OnUpstreamChange: mark dirty + signal instead of TrackBackgroundTask(ReevaluateAsync(...)).
4. Pump body reuses the existing EvaluatePredicateToStateAsync + persist-before-memory + emit-outside-gate pattern (lines 465-489). Snapshot + clear the dirty set under a short lock, then process under _evalGate.
5. Dispose: cancel _pumpCts, signal to unblock, await the pump (fold into the existing _inFlight drain or await _pumpTask explicitly).

Tests: tests/Core/…Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs:

• RapidUpstreamChanges_CoalesceToBoundedEvaluations — push N (e.g. 1000) changes to one referenced tag faster than eval drains (controllable clock / instrumented predicate counter); assert the predicate ran far fewer than N times and the final state reflects the last value.
• FanOut_DoesNotLeakTasks — assert the in-flight/dirty structures return to empty after quiescence.
• Regression: existing change-driven activation/clear tests still pass (a single change still promptly re-evaluates).

Effort: M. Risk: Medium — reworks the alarm engine's hot path and its concurrency model. The _evalGate + persist-before-memory + emit-outside-gate invariants must be preserved exactly. Land standalone with careful review; it is the alarm engine's most-exercised path. Blast radius = scripted-alarm evaluation only.

---

6. S1 — HIGH — VirtualTagEngine.Load mutates shared state with no gate

Restatement: Load clears/rebuilds _tags, _graph, and disposes the compile cache without holding _evalGate, racing in-flight CascadeAsync/EvaluateInternalAsync/timer ticks reading those non-thread-safe collections.

Verification: Confirmed. Load (VirtualTagEngine.cs:78-173) does UnsubscribeFromUpstream(); _tags.Clear(); _graph.Clear(); _compileCache.Clear(); at 84-87 with no gate; EvaluateInternalAsync reads _tags at 293 before taking the gate (295); CascadeAsync walks _graph at 278.

Root cause: The engine never received the alarm engine's gate-held-load + concurrent-collection discipline.

Proposed design: Resolved by U1 (retire the engine). Since S1 is a latent bug in dormant code, deleting VirtualTagEngine eliminates it. No separate fix. If U1 is deferred or the engine is instead sanctioned (option (b)), then port the alarm engine's discipline: acquire _evalGate for the whole Load, make _tags a ConcurrentDictionary, and re-check-after-gate in EvaluateInternalAsync (it already reads _tags before the gate at 293 — move the read inside). Effort in that case: M; risk: Medium.

Recommendation: Fold into U1's deletion. Effort: none (subsumed). Risk: none.

---

7. S5 + C7 — MEDIUM — Timed-shelve expiry via predicate path swallows Unshelved; doc drift

S5 restatement: When ApplyPredicate expires a timed shelve (via MaybeExpireShelving), it returns Activated/Cleared/None and never Unshelved — that emission only comes from ApplyShelvingCheck on the 5 s timer. A predicate re-eval landing between expiry time and the next tick persists the state as unshelved but publishes no Unshelved event, so OPC UA clients / /alerts never learn the shelve ended.

C7 restatement: ApplyPredicate's xmldoc (Part9StateMachine.cs:31-34) promises "branch-stack increment when a new active arrives while prior active is still un-acked" — no such logic exists (re-activation just resets Acked/Confirmed at lines 62-63).

Verification: Both confirmed. ApplyPredicate (Part9StateMachine.cs:39-90): MaybeExpireShelving at 48, expiry produces stateWithShelving but the only emissions returned are Suppressed/Activated/Cleared/None (67, 84, 89). ApplyShelvingCheck (300-316) is the sole Unshelved source. Doc-drift at 31-34 confirmed against the whole file (no branch logic anywhere).

Root cause: TransitionResult carries a single EmissionKind, so a compound "shelve expired AND value transitioned" can only report one. The engine's EvaluatePredicateToStateAsync (ScriptedAlarmEngine.cs:546-552) fires exactly one emission per predicate result.

Proposed design: Make the shelve-expiry emission explicit. Two options:

• (a) Engine-side detection (minimal, preferred). In EvaluatePredicateToStateAsync, before calling ApplyPredicate, note seed.Shelving.Kind; after, if the result's Shelving.Kind changed from Timed to Unshelved (i.e. expiry happened inside ApplyPredicate), append a synthetic Unshelved emission to pendingEmissions before the value-transition emission. Order matters: Unshelved first, then Activated/Cleared. No state-machine signature change. Add an AutoUnshelve audit comment to match ApplyShelvingCheck for consistency (or centralize expiry so both paths append the same comment).
• (b) Compound result. Change ApplyPredicate to return a list of emissions (or a nullable secondary Unshelved). Cleaner semantically but a wider signature change touching all callers/tests. Prefer (a) — smaller blast radius, keeps Part9StateMachine pure-function-per-transition.

Either way, centralize the expiry so MaybeExpireShelving and ApplyShelvingCheck don't drift.

C7 fix: Correct the ApplyPredicate xmldoc (lines 30-34) to describe actual behavior: "activation, clearing, timed-shelve expiry, and shelving suppression; re-activation resets Acked/Confirmed. No condition-branch / previous-instance stack (see U5 conformance note)."

Implementation steps:

1. Part9StateMachine.cs: fix the ApplyPredicate summary (C7). Optionally factor a private ExpireTimedShelvingWithAudit so both entry points append the identical AutoUnshelve comment.
2. ScriptedAlarmEngine.EvaluatePredicateToStateAsync (lines 504-553): capture seed.Shelving.Kind; after ApplyPredicate, detect Timed→Unshelved and prepend an Unshelved ScriptedAlarmEvent (via BuildEmission(state, result.State, EmissionKind.Unshelved)) to pendingEmissions ahead of the value emission.

Tests:

• Part9StateMachineTests.cs: add ApplyPredicate_WhenTimedShelveExpired_StatePersistsUnshelved (state-level assertion — the machine already unshelves; assert Shelving.Kind == Unshelved after expiry).
• ScriptedAlarmEngineTests.cs: PredicateReeval_AfterTimedShelveExpiry_EmitsUnshelvedBeforeTransition — set a timed shelve, advance the controllable clock past UnshelveAtUtc, drive a predicate change via upstream push (not the shelving timer), assert two emissions in order: Unshelved then Activated/Cleared.
• Doc-only assertion not needed for C7.

Effort: S-M. Risk: Low-Medium — touches emission ordering; keep it additive (never suppress the existing value emission). Blast radius = scripted-alarm shelving semantics; OPC UA clients gain a correct Unshelved event.

---

8. S3 / S4 / S6 — MEDIUM — Reload-swap, _alarmsReferencing sync, sink dispose-drain (one hardening pass)

S3 — Failed reload is fail-stop, not fail-back

Restatement: Both engines tear down the prior generation before compiling the new one; a compile OR transient IAlarmStateStore failure leaves the engine inert (alarm engine: zero alarms until next publish; VT engine: serving stale Good values).

Verification: Confirmed. ScriptedAlarmEngine.LoadAsync clears/unsubscribes at 196-209, compiles, throws at 250-255 on any failure — but the store-restore loop (273-281) can also throw (_store.LoadAsync/SaveAsync), and that path is not operator-preventable (unlike compile errors, which publish-time validation catches). VT-engine equivalent is retired under U1.

Design: For the alarm engine, split the two failure classes:

• Compile failures — already aggregated (250-255); publish-time validation makes these unlikely. Keep fail-stop but note the operator-preventable nature. Optionally build the new generation into locals and swap under the gate only on success (compile is side-effect-free; subscriptions + timer are the only external effects). This is the cleaner "build-then-swap" the report recommends, but it is a larger rewrite of LoadAsync.
• Transient store failures in the restore loop (273-281) — wrap each _store.LoadAsync/SaveAsync so a transient failure for one alarm falls back to state.Condition (the Fresh seed already compiled) and logs, rather than aborting the entire load. At minimum, distinguish store-failures from compile-failures and don't let a store hiccup zero out every alarm.

Recommended scope: the targeted fix (don't abort the whole load on a single-alarm store failure) is S; the full build-then-swap is M. Do the targeted fix now; note build-then-swap as optional follow-up.

Implementation: In LoadAsync's restore loop (273-281), try/catch per alarm around _store.LoadAsync/EvaluatePredicateToStateAsync/_store.SaveAsync; on failure, keep the Fresh-seeded AlarmState, log a warning, and continue. The alarm still loads and evaluates live; only its persisted-state restore is skipped.

S4 — _alarmsReferencing read without synchronization

Restatement: _alarmsReferencing is a plain Dictionary (ScriptedAlarmEngine.cs:120-121) mutated under _evalGate in LoadAsync (201, 237-242) but read by OnUpstreamChange (446) from upstream callback threads with no gate; a callback in flight during reload races Clear()/Add().

Verification: Confirmed. _alarms was deliberately made ConcurrentDictionary for exactly this reader class (comment 42-50); _alarmsReferencing was missed. Note: S2's dirty-set rework also reads _alarmsReferencing in OnUpstreamChange — fix S4 together with S2 since they touch the same method.

Design: Publish _alarmsReferencing as an immutable snapshot swapped atomically: keep a private volatile IReadOnlyDictionary<string, IReadOnlyCollection<string>> _alarmsReferencing; LoadAsync builds a fresh Dictionary under the gate and Volatile.Writes it; OnUpstreamChange reads the current reference (a coherent snapshot). Alternative: ConcurrentDictionary — but the value is a mutable HashSet also mutated during build, so the immutable-snapshot swap is cleaner and matches the "rebuilt per load" lifecycle. Prefer the volatile-snapshot swap.

Implementation: Change the field type; build a local Dictionary<string, HashSet<string>> in the LoadAsync compile loop, freeze it (or keep HashSet values but never mutate after publish), and Volatile.Write. OnUpstreamChange reads via Volatile.Read (or just the volatile field).

S6 — SqliteStoreAndForwardSink.Dispose races an in-flight drain

Restatement: Dispose (SqliteStoreAndForwardSink.cs:679-686) sets _disposed, disposes the timer, then immediately disposes _drainGate + the writer without waiting for an in-flight DrainOnceAsync — the drain's finally { _drainGate.Release(); } (line 467) then throws ObjectDisposedException, and the writer can be disposed mid-WriteBatchAsync.

Verification: Confirmed. DrainOnceAsync takes _drainGate.WaitAsync(0) (323) and releases in finally (467); Timer.Dispose() (683) doesn't wait for a running callback (DrainTimerCallback is async void, 199-203).

Design: Mirror the alarm engine's dispose-drain (ScriptedAlarmEngine.cs:751-769). Before disposing _drainGate/writer, acquire the gate to guarantee no drain is mid-flight:

• Dispose: set _disposed, dispose _drainTimer, then _drainGate.Wait() (blocking; sink is disposed from the host shutdown path, no sync-context deadlock) — this waits for any in-flight DrainOnceAsync to release — then dispose the writer, then dispose _drainGate (or don't re-release; since we hold it, just dispose after). Add a _disposed re-check at the top of DrainOnceAsync after acquiring the gate (it may already have one — verify around line 323-330) so a callback that passed the timer can bail.

Implementation: Rewrite Dispose (679-686) to: _disposed = true; _drainTimer?.Dispose(); _drainGate.Wait(); try { if (_writer is IDisposable d) d.Dispose(); } finally { _drainGate.Dispose(); }. Confirm DrainOnceAsync returns early when _disposed after acquiring the gate.

Tests (S3/S4/S6):

• S3: LoadAsync_WhenStoreThrowsForOneAlarm_OtherAlarmsStillLoad (fake IAlarmStateStore throwing for a specific id) — assert the engine loads and evaluates the rest.
• S4: covered indirectly by S2's concurrency test; optionally a targeted Load_ConcurrentWithUpstreamCallback_NoTornRead stress test.
• S6: Dispose_DuringInFlightDrain_DoesNotThrow — a slow fake IAlarmHistorianWriter.WriteBatchAsync (blocks on a gate the test controls), start a drain, call Dispose on another thread, release the writer, assert no ObjectDisposedException surfaces and the writer's dispose ran after the batch completed. Add to SqliteStoreAndForwardSinkTests.cs.

Effort: S3 targeted S / S4 S / S6 S — bundle as one "alarm/sink hardening" PR (M total). Risk: Low-Medium; all three are fault-path hardening with clear tests. Sequence S4 with S2 (same method). Blast radius = alarm engine load + sink shutdown.

---

9. Medium conventions / underdeveloped: U4, U5, C1, C2, C5

U4 — MEDIUM — IHistoryWriter is a permanently-Null surface

Restatement: VirtualTagDefinition.Historize routes to IHistoryWriter.Record, but DI binds only NullHistoryWriter (Runtime/ServiceCollectionExtensions.cs:58, 237); the operator-visible Historize checkbox on virtual tags is a silent no-op.

Verification: Confirmed. services.TryAddSingleton<IHistoryWriter>(NullHistoryWriter.Instance) (line 58); no real impl in tree. Separate ContinuousHistorization path taps the mux directly (CLAUDE.md — and note the OVERALL report says Known Limitation 2 is closed, so continuous historization is now live via FeedHistorizedRefs/UpdateHistorizedRefs).

Design: Two coherent choices; pick per product intent:

• (a) Wire IHistoryWriter to the gateway value-writer — make the checkbox real by binding a gateway-backed IHistoryWriter (the same WriteLiveValues path continuous-historization uses). Larger; overlaps the continuous-historization recorder. Only do this if VT-result historization is a distinct requirement from continuous historization.
• (b) Hide/annotate the checkbox until it does something (preferred interim) — in the AdminUI VirtualTag modal, disable/hide the Historize toggle (or add an inline "not yet wired" note) and add an xmldoc/appsettings note that IHistoryWriter is Null unless a deployment overrides it. Avoids operator confusion at near-zero cost.

Recommendation: (b) now; open a tracked follow-up for (a) if VT-result historization is on the roadmap. Cross-reference the OVERALL note that continuous historization is the live historization path.

Effort: S (option b). Risk: Low. Touches AdminUI VT modal + a doc line.

U5 — MEDIUM — Part 9 surface gaps real but undeclared

Restatement: No condition branches/previous-instances; Severity static per definition; AlarmKind affects only node typing; MessageTemplate has no brace escaping; Retain carried but not consulted. Each a defensible v1 cut, but scattered.

Verification: Confirmed against Part9StateMachine.cs (no branch logic), ScriptedAlarmDefinition.cs (static Severity), MessageTemplate.cs, and the engine (Retain unused).

Design: Documentation, not code. Add an explicit OPC UA Part 9 conformance statement section to docs/ScriptedAlarms.md listing the supported subset and the v1 cuts (no branching/previous-instances, static Severity, kind→node-typing only, no template brace escaping, Retain carried-not-enforced) so client integrators know what to expect. Fold C7's corrected ApplyPredicate doc into this.

Effort: S. Risk: None.

C1 — MEDIUM — ITagUpstreamSource defined twice

Restatement: Identical-shape interface in Core.VirtualTags/ITagUpstreamSource.cs AND at ScriptedAlarmEngine.cs:860-872; two distinct .NET types, invariant enforced only by comment.

Verification: Confirmed — two interface ITagUpstreamSource definitions.

Design: Hoist a single ITagUpstreamSource into Core.Scripting.Abstractions (both projects reference it — verify the reference direction) and retire both duplicates. If Core.VirtualTags is being retired (U1), the alarm engine's copy is the survivor to relocate. Sequence: do C1 as part of/just before U1 so the delete doesn't strand the interface. Update DependencyMuxTagUpstreamSource and any composing bridge to the single type (they currently must adapt each separately).

Implementation: Add ITagUpstreamSource.cs to Core.Scripting.Abstractions; delete the copy from ScriptedAlarmEngine.cs (843-872 also holds ScriptedAlarmEvent — see C3) and the Core.VirtualTags file; fix usings. Confirm Core.ScriptedAlarms and Core.VirtualTags already reference Core.Scripting.Abstractions.

Effort: S-M. Risk: Low-Medium — a shared contract move; ensure all implementors (DependencyMuxTagUpstreamSource) compile against the single type. Blast radius = both engines + the mux bridge.

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

Restatement: ScriptContext/ScriptGlobals/PassthroughScript → ns Core.Scripting; VirtualTagContext → ns Core.VirtualTags; AlarmPredicateContext → ns Core.ScriptedAlarms — all physically in the Abstractions assembly (deliberate: the sandbox pins contextType.Assembly, so concrete contexts must live Roslyn-free). Documented only in a PassthroughScript remark; misleads find-by-namespace.

Verification: Confirmed (rationale at ScriptSandbox.cs:60-70).

Design: Documentation, not a namespace migration (a TypeForwardedTo migration is higher-risk churn for a convention nit). Add a project-level README.md (or an AssemblyInfo.cs doc comment / a _Namespaces.md) in Core.Scripting.Abstractions stating the rule: "this assembly is the sandbox-pinned Roslyn-free closure; concrete script-context types keep their consumer namespaces deliberately so ScriptSandbox.Build(contextType) pins this assembly without dragging Roslyn." Reference ScriptSandbox.cs:60-70.

Effort: S. Risk: None.

C5 — LOW/MEDIUM — Core.AlarmHistorian mixes contract + implementation

Restatement: IAlarmHistorianSink/IAlarmHistorianWriter/status types share the project with SqliteStoreAndForwardSink, so the Runtime adapter + gateway driver drag the Microsoft.Data.Sqlite dependency to get the interfaces.

Verification: Confirmed by project structure.

Design: Optional. Extract the interfaces + status DTOs into a Core.AlarmHistorian.Abstractions (mirror the scripting split) so consumers reference the contract without the SQLite impl. Tolerable at three files — schedule only if the SQLite transitive dependency becomes a problem for the gateway driver. Defer unless bundled with a broader Abstractions cleanup.

Effort: M (new project + reference rewiring). Risk: Low. Recommend: defer (note only).

---

10. Lows (batched)

ID	Restatement	Verification	Action	Effort
S7	Sandbox CPU/mem unbounded; timed-out CPU-bound script leaks a pool thread; hot looping script orphans one thread per upstream change	Confirmed (ScriptSandbox.cs:30-35, TimedScriptEvaluator.cs:17-26). Note U2's fix increases orphan exposure on the VT path	Add a per-script circuit breaker: N consecutive timeouts → suspend evaluation + surface to Admin UI (meter/health). Cheap interim before out-of-process runner. Applies to both RoslynVirtualTagEvaluator (post-U2) and ScriptedAlarmEngine (quarantine the predicate after repeated timeouts — currently holds prior state but keeps re-evaluating, ScriptedAlarmEngine.cs:535-538)	M
S8	ScriptedAlarmEngine.Dispose Task.WhenAll(...).GetAwaiter().GetResult() (ScriptedAlarmEngine.cs:761) — deadlock trap off the actor host	Confirmed	Implement IAsyncDisposable alongside IDisposable; actor host awaits. Low urgency (fine under current host)	S
S9	At-least-once delivery duplicates on crash between WriteBatchAsync and outcome commit (SqliteStoreAndForwardSink.cs:372-441)	Confirmed (correct choice: dupes over loss)	Document in docs/Historian.md that the gateway SendEvent side must tolerate replays	S
S10	VirtualTagSource.SubscribeAsync emits seed Read before registering observer (VirtualTagSource.cs:62-73) → a change between Read and Subscribe is missed	Confirmed	Retired with U1 (VirtualTagSource is dormant). If kept: document the seed-then-subscribe trade-off, or register-first + let idempotent newer-wins consumer dedupe	none (U1)
S11	Capacity eviction drops oldest accepted alarm events (SqliteStoreAndForwardSink.cs:602-636), EvictedCount counter exists	Confirmed	Document the drop-oldest policy as deliberate in docs/AlarmTracking.md; note drop-newest/refuse-enqueue as the compliance alternative	S
P2	Task.Run+WaitAsync per evaluation on the hot path (TimedScriptEvaluator.cs:78-81)	Confirmed; required for the timeout to work	No action now; note inline-run-with-watchdog as the escape hatch if profiling shows it. (U2 adds this hop to the VT path — accepted)	none
P3	VT engine allocates per-eval what the alarm engine pools (VirtualTagEngine.cs:298, 313-317)	Confirmed	Retired with U1	none (U1)
P4	Single global eval gate per engine	Confirmed; correct + simple	No action (per-alarm/sharded gate is the escape hatch)	none
P5	SQLite per-call PRAGMA + COUNT(*) overheads (SqliteStoreAndForwardSink.cs:244-246, 479-484)	Confirmed; capacity fast path already removed the hot cost	No action	none
P6	DependencyGraph well-optimized	Confirmed	Retired with U1	none (U1)
C3	Trailing type defs: ScriptedAlarmEvent + dup ITagUpstreamSource at ScriptedAlarmEngine.cs:843-872; CompilationErrorException/ScriptAssemblyLoadContext at ScriptEvaluator.cs:391-432; DependencyCycleException in DependencyGraph.cs	Confirmed	Move ScriptedAlarmEvent (public cross-project contract) to its own file (do with C1's interface hoist). Others are reasonable co-location — leave	S
C4	Repeated raw OPC UA status-code literals (0x80340000u etc.) across ≥5 files	Confirmed	Add a KnownStatusCodes static in Core.Abstractions (these layers avoid the OPC Foundation package deliberately) and replace the hand-rolled literals	S
C6	Plan-era naming residue ("Phase 7 plan Stream A.4/B/C/…") in xmldocs across many files	Confirmed	Sweep to current doc anchors (docs/ScriptedAlarms.md/docs/ScriptEditor.md) during the next doc pass	S
U6	No TODO/HACK/FIXME markers (gaps in xmldoc remarks)	Confirmed — healthy pattern	No action	none
U7	Broad behavior-focused tests with specific holes: (a) S5 expiry-emission, (b) load-vs-cascade concurrency (S1/S4), (c) sink Dispose-during-drain (S6), (d) ForbiddenTypeAnalyzer no dedicated suite, (e) production RoslynVirtualTagEvaluator timeout (would've caught U2)	Confirmed — no *ForbiddenType* test file exists; RoslynVirtualTagEvaluatorTests.cs exists in Host.IntegrationTests	Holes (a)/(c)/(e) covered by the tests above (S5/S6/U2). Add a dedicated ForbiddenTypeAnalyzerTests suite (analyzer-pass unit tests against CSharpCompilations) so analyzer regressions produce clear failures	S-M

---

Suggested PR sequencing

1. PR-A (Critical, small): U2 + U3 together (same file RoslynVirtualTagEvaluator.cs + one VirtualTagHostActor hook) + their tests. Ship first — closes the OVERALL action item #2 Critical.
2. PR-B (perf, small): P1 sandbox-reference memoization + ScriptSandboxTests.
3. PR-C (cleanup, medium): C1 (hoist ITagUpstreamSource) + C3 (ScriptedAlarmEvent file) → then U1 retire (VirtualTagEngine/TimerTriggerScheduler/VirtualTagSource/DependencyGraph + tests). Subsumes S1, S3-VT, P3, P6, S10. Land after PR-A.
4. PR-D (alarm hardening, medium): S2 (dirty-set pump) + S4 (_alarmsReferencing snapshot, same method) + S6 (sink dispose-drain) + S3 (targeted store-failure fallback) + their tests.
5. PR-E (semantics, small-medium): S5 + C7 (unshelve emission + doc) with state-machine + engine tests.
6. PR-F (docs/nits): U4 (annotate checkbox), U5 (Part 9 conformance doc), C2 (Abstractions README), C4 (KnownStatusCodes), C6 (plan-era doc sweep), S9/S11 (doc policies), U7 (ForbiddenTypeAnalyzerTests). Optional S7 circuit-breaker + S8 IAsyncDisposable as tracked follow-ups.