lmxopcua/archreview/02-scripting-alarms.md

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

• Date: 2026-08-19
• Commit: 12b0035a (master, clean tree)
• Updates: the 2026-07-12 re-review at f6eaa267 (which updated the 2026-07-08 original at 9cad9ed0). This is the second update (round 3). ~1274 commits landed since f6eaa267; the delta in this domain is substantial: R2-03 (02/S13 + 02/S12 + 02/P7) merged; the dirty-set coalescing rework (5df793e7 + the drain-latch fix train 445bb855/c8a70143/5ae43023); scripted-condition quality #478 (8c5e2be9) + comms-loss bridging #481 (c81cef39) + status sub-codes #519 (2d20aeb7); the re-assert-after-reload fix #487 (549c6564); the LocalDb consolidation (8a9cb40a/71379816/af545efd — SqliteStoreAndForwardSink → LocalDbStoreAndForwardSink with a Primary-only drain gate) and the LocalDb condition-state store (4f4cdd05, EfAlarmConditionStateStore deleted). Core.Scripting, Core.Scripting.Abstractions, and Core.VirtualTags are byte-identical to f6eaa267 (git diff empty), so prior findings in those projects were re-verified against unchanged code and their 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 (dormant cascade engine)
  • src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms (engine + Part 9 state machine)
  • src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian (LocalDbStoreAndForwardSink, AlarmSfSchema, AlarmConditionStateSchema)
  • Production evaluation path: Host/Engines/RoslynVirtualTagEvaluator, Runtime/VirtualTags/* (VirtualTagActor, VirtualTagHostActor, mux), Runtime/ScriptedAlarms/* (ScriptedAlarmHostActor, DependencyMuxTagUpstreamSource, LocalDbAlarmConditionStateStore)
  • The drain-gate wiring (Runtime/ServiceCollectionExtensions.AddAlarmHistorian, IRedundancyRoleView), Host/Configuration/AlarmSfLegacyMigrator + LocalDbSetup
  • The {{equip}} resolution seam (Commons/Types/EquipmentReferenceMap)
• Method: re-verified every prior finding against current code (never STATUS.md's claims); full adversarial read of the coalescing rework, the LocalDb sink + migrator + condition store, the re-assert paths (#487 alarm-side, VT ReassertValue), the quality plumbing (#478/#481/#519), and the R2-03 fixes with their tests; fresh-eyes pass over everything landed since f6eaa267.

---

Prior-finding status (f6eaa267 → 12b0035a)

ID	One-liner	Status @ 12b0035a
S1	VirtualTagEngine.Load mutates shared state with no gate vs in-flight evals	STILL OPEN — file byte-identical (VirtualTagEngine.cs:23-24, 78+); engine still dormant (U1). Third round carrying this in dead code
S2	No coalescing/backpressure on upstream-change fan-out	FIXED on every live path — alarm engine: dirty-set + single drainer (ScriptedAlarmEngine.cs:189-196, 547-677), pinned by A_storm_of_pushes_coalesces_to_one_drain_pass incl. a falsification control (ScriptedAlarmEngineTests.cs:1439-1500); VT actors additionally get batched mux delivery — one evaluation per burst (VirtualTagActor.cs:201-217, d7edc709). Residual only in the dormant VirtualTagEngine.OnUpstreamChange (folds into U1)
S3	Failed reload is fail-stop, not fail-back (both engines)	STILL OPEN — alarm engine still tears down before compiling (ScriptedAlarmEngine.cs:256-270, throw at 311-316); a transient IAlarmStateStore fault in the restore loop (336-342) still lands "zero alarms loaded". Mitigation only: the host keeps the prior mux interest on a failed reload (ScriptedAlarmHostActor.cs:262-264)
S4	_alarmsReferencing plain Dictionary read from upstream threads without a gate	STILL OPEN — decl ScriptedAlarmEngine.cs:165-166, unguarded read in OnUpstreamChange at 551, gate-held Clear/Add at 261/300-302. The coalescing rework touched the surrounding code and still left this one plain
S5	Timed-shelve expiry via the predicate path swallows the Unshelved emission	STILL OPEN — Part9StateMachine.ApplyPredicate (47-52) still silently expires via MaybeExpireShelving (318-322) and returns only Activated/Cleared/None; Part9StateMachineTests still has no expiry-during-predicate case
S6	Sink Dispose races an in-flight drain	STILL OPEN — carried into the rewrite. LocalDbStoreAndForwardSink.Dispose (698-705) disposes the timer, then _drainGateLock and the writer, without waiting for a running DrainOnceAsync (which holds the gate, 320, and releases at 471 → ObjectDisposedException into the tick's catch; writer disposable under an active WriteBatchAsync). Same shape, new file
S7	Sandbox CPU/memory unbounded; timed-out script leaks a thread; no quarantine	STILL OPEN — no per-script circuit breaker anywhere (grep clean). New corroborating evidence: 837d8a17's commit message documents that the timeout clock starts at WaitAsync, so under pool starvation queued evaluations time out spuriously — the exact starvation the orphaned threads cause (see updated body)
S8	ScriptedAlarmEngine.Dispose blocks sync-over-async	STILL OPEN — Task.WhenAll(...).GetAwaiter().GetResult() now at ScriptedAlarmEngine.cs:1180
S9	At-least-once delivery duplicates on crash window (doc note)	CLOSED — documented: docs/AlarmHistorian.md:236 ("delivery is at-least-once across a failover by design"); the pair double-accept additionally converges via payload-hash row ids (AlarmSfSchema.DeriveId, 60-64)
S10	VirtualTagSource.SubscribeAsync seed-then-subscribe can miss one update	STILL OPEN — file byte-identical; dormant code (U1)
S11	Capacity eviction drops oldest accepted alarm events (policy doc note)	CLOSED — policy documented (docs/AlarmHistorian.md:179, 257; EvictedCount described as the data-loss signal, IAlarmHistorianSink.cs:61-65)
S12	Apply-boundary ClearCompiledScripts races in-flight child evaluations	FIXED (R2-03) — bounded retry-once on ObjectDisposedException re-fetches into the current cache generation (RoslynVirtualTagEvaluator.cs:93-97, 142-153); fetch-time disposal correctly classified as shutdown, never retried (114-119); stress guard in Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs. Residual one-generation stale-ALC window documented in the class header (22-33)
S13	Script failure/timeout never degrades node quality; stale value stays Good forever	FIXED (R2-03 + #519 reshape) — VirtualTagActor.OnEvaluationFailed publishes BadInternalError carrying the last-known value, once per Good→Bad transition, gated on inputs-ready (276-289); recovery bypasses the value dedup (254-262); host bridges StatusCode verbatim to the publish actor and the historian record (VirtualTagHostActor.cs:227-241). Tests: VirtualTagActorTests.cs:85-110+ (degrade, once-per-transition, same-value recovery)
P1	ScriptSandbox.Build rebuilds the full BCL reference set on every compile	STILL OPEN — ScriptSandbox.cs:83-87, 100-130 byte-identical; no caller-level caching added. The P7 gate reduces how often deploys trigger mass recompiles, but each compile still pays the full rebuild
P2	Task.Run + WaitAsync per evaluation on the hot path	STILL OPEN (accepted) — unchanged; VT path still also blocks its dispatcher thread (RoslynVirtualTagEvaluator.cs:134)
P3	Live VT adapter allocates per evaluation what the alarm engine pools	STILL OPEN — fresh read-cache dict + context + TimedScriptEvaluator wrapper per call (RoslynVirtualTagEvaluator.cs:79-91, 133); the alarm engine's AlarmScratch reuse (1231-1255) still has no VT sibling
P4	Single global eval gate per engine	STILL OPEN (accepted) — coalescing makes the serialized gate far cheaper in practice
P5	SQLite per-call overheads	STILL OPEN (accepted) — new impl: GetStatus still runs a synchronous COUNT(*) per call (LocalDbStoreAndForwardSink.cs:527-532); enqueue fast path (253-274) preserved
P6	DependencyGraph well optimized	unchanged — informational
P7	Unconditional per-apply cache flush forces full VT recompilation on every deploy	FIXED (R2-03) — clear gated on a changed deployed expression set (VirtualTagHostActor.cs:94-113, ordinal SetEquals); rename/flag-toggle respawns need no recompile (cache keyed by source)
C1	ITagUpstreamSource defined twice	STILL OPEN — duplicate now at ScriptedAlarmEngine.cs:1301-1317
C2	Abstractions assembly spans three namespaces undocumented	STILL OPEN — project byte-identical
C3	Trailing type definitions in engine files	STILL OPEN — grew: ScriptedAlarmEvent, ScriptedAlarmProjection, and the duplicate interface all trail ScriptedAlarmEngine.cs (1258-1318)
C4	Repeated raw OPC UA status-code literals	LARGELY CLOSED — Commons.OpcUa.OpcUaStatus is now the named authority and the Runtime/Host paths use it (VirtualTagActor.cs:38, 182, 288; DependencyMuxTagUpstreamSource.cs:43); the remaining Core inlines are deliberate (no Commons ref), justified in-code, and pinned by a cross-project test (ScriptedAlarmEngine.cs:58-67). Residual accepted
C5	Core.AlarmHistorian mixes contract and implementation	STILL OPEN — contract + LocalDbStoreAndForwardSink + schemas share the project, which now drags Microsoft.Data.Sqlite and ZB.MOM.WW.LocalDb (csproj:15, 28) into anything wanting just IAlarmHistorianSink
C6	Plan-era "Phase 7 plan Stream …" doc residue	STILL OPEN — 20 hits across the five Core projects (e.g. ScriptedAlarmEngine.cs:9, IAlarmHistorianSink.cs:14, TimedScriptEvaluator.cs)
C7	ApplyPredicate xmldoc claims branch handling that doesn't exist	STILL OPEN — Part9StateMachine.cs:31-33 still promises "branch-stack increment"; no branch logic anywhere in the file
C8	Script timeout budgets hard-coded and inconsistent (VT 2 s vs alarm 250 ms)	STILL OPEN — Host registration passes no runTimeout (Program.cs:324-328 → default 2 s at RoslynVirtualTagEvaluator.cs:58); DriverHostActor builds the engine with no scriptTimeout (786-787 → DefaultTimeout 250 ms, TimedScriptEvaluator.cs:40); still no Scripting:* key for either
U1	Entire Core.VirtualTags engine stack dormant in production	STILL OPEN — third round. new VirtualTagEngine still appears only under tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests; ScriptAnalysisService.cs:350 still documents "the cascade VirtualTagEngine is dormant". No retire/sanction decision. The case for retirement is now stronger still — see body
U2	(fixed @ f6eaa267) production VT timeout	REMAINS FIXED — routing intact (RoslynVirtualTagEvaluator.cs:126-141)
U3	(fixed @ f6eaa267) compile-cache/ALC accretion	REMAINS FIXED — cache + apply-boundary clear intact (RoslynVirtualTagEvaluator.cs:41; VirtualTagHostActor.cs:112); alarm engine gained the same discipline (ScriptedAlarmEngine.cs:122, 270, 1207)
U4	IHistoryWriter is a permanently-Null surface	STILL OPEN — DI still binds only NullHistoryWriter (Runtime/ServiceCollectionExtensions.cs:67-68, 387-388); no real implementation exists; the VT Historize flag routes to _history.Record (VirtualTagHostActor.cs:238-242) and silently no-ops. Note the separate ContinuousHistorization path does NOT cover VT nodes (it keys on raw-path historized refs)
U5	Part 9 surface gaps real but undeclared in one place	MOSTLY OPEN — individual cuts are now documented where they live (docs/ScriptedAlarms.md:135 no brace escaping; :33 Retain semantics; ScriptedAlarmDefinition.cs:20 static severity), but no consolidated conformance statement exists, Retain is still consulted nowhere (only carried: ScriptedAlarmHostActor.cs:700), and the C7 doc-drift stands
U6	No TODO/HACK/stub markers (healthy)	unchanged — informational
U7	Test coverage holes (a)–(f)	PARTIALLY FIXED — closed: (e) production timeout (held), (f) node-quality degradation (VirtualTagActorTests.cs:85-110+); plus strong new coverage: coalescing storm with falsification control, drain-latch stack/handover pins (Sustained_synchronous_drain_failures_hand_over_without_deepening_the_stack), S&F sink 39 tests, condition-store parity round-trips. Still open: (a) S5's expiry-during-predicate emission, (b) load-vs-cascade concurrency (dormant engine), (c) sink Dispose-during-drain (only disposed-then-call is tested, LocalDbStoreAndForwardSinkTests.cs:453, 984-991), (d) dedicated ForbiddenTypeAnalyzer suite (still exercised only through Compile in ScriptSandboxTests)

---

What changed in this domain (round-3 architecture delta)

1. Upstream fan-out is now a dirty set + single drainer (ScriptedAlarmEngine.cs:189-196, 547-677). A push marks referencing alarm ids dirty and starts at most one drainer; a storm collapses into one further pass over the latest cached values (latest-wins by construction — RefillReadCache reads _valueCache). The drain latch (_drainScheduled) is released on every exit path, with a queued (not called) replacement drainer on exceptional escape so sustained failures cannot deepen the stack (626-676) — pinned by a dedicated test. TrackBackgroundTask registers a placeholder before invoking the factory so Dispose's drain can never miss just-started work (1000-1078). This is careful, well-reasoned concurrency code.
2. Scripted-condition quality (#478/#481/#519). Worst-of-input OPC UA status is computed per evaluation (WorstInputStatus, 923-942), bucket transitions emit an out-of-band EmissionKind.QualityChanged (never through the IAlarmSource fan-out — ScriptedAlarmSource.cs:84-87 swallows it; the host routes it to the dedicated AlarmQualityUpdate node path, ScriptedAlarmHostActor.cs:446-454). Cold start is distinguishable from a comms-lost driver because the upstream stamps never-pushed paths BadWaitingForInitialData (DependencyMuxTagUpstreamSource.cs:30-43) and WorstInputStatus skips exactly that code. Sub-codes ride verbatim end-to-end since #519.
3. Re-assert after (re)load (#487) — ScriptedAlarmEngine.GetProjections (a read, deliberately a distinct type from the emission so the historian path cannot consume it) + ScriptedAlarmHostActor.ReassertConditionNodes (302-378) restore non-normal condition nodes after an address-space rebuild without appending historian rows. Mirrors the VT ReassertValue fix (VirtualTagActor.cs:47-52, 179-185; VirtualTagHostActor.cs:192-208), whose historian-skip flag (IsReassert) prevents fabricated samples.
4. Alarm S&F moved to the replicated LocalDb (LocalDbStoreAndForwardSink): payload-hash TEXT primary keys make the pair's boot-window double-accept converge to one row (AlarmSfSchema.DeriveId), acks DELETE (tombstone replicates), and the drain is gated to the Primary only when the queue actually replicates (ServiceCollectionExtensions.cs:167-209 — the gate deliberately scopes to "is my queue shared?" precisely because the fleet-wide election and the pair-local queue disagree; the NotPrimary drain state makes a suspended Secondary diagnosable, IAlarmHistorianSink.cs:83-93). AlarmSfLegacyMigrator is a genuinely careful one-time copy: read-only open, tolerant column intersection, single transaction, rename-after-commit, re-run harmless under payload-hash ids.
5. Part 9 condition state moved to LocalDb (LocalDbAlarmConditionStateStore) — byte-parity port of the deleted EF store (Active deliberately not persisted; re-derived), unconditional PK upsert riding the replication HLC.

---

Findings

Severity scale: Critical / High / Medium / Low. Fixed prior findings live only in the status table. New findings continue the per-category scheme (S14+, C9+).

1. Stability

S1 — HIGH — VirtualTagEngine.Load mutates shared state with no gate against in-flight evaluations (carried, third round)

Unchanged since 9cad9ed0 (VirtualTagEngine.cs:78+ rebuilds _tags (24) and _graph (23) without _evalGate while EvaluateInternalAsync/CascadeAsync/timer ticks read them from arbitrary threads). Still fully mitigated only by dormancy (U1). Two review rounds have recommended retire-or-fix; neither has happened. This finding should be resolved by resolving U1, not by fixing the code.

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

Shape unchanged: LoadAsync tears down the prior generation (timer, subscriptions, _alarms, scratch, compile cache — ScriptedAlarmEngine.cs:256-270) before compiling, and throws with zero alarms loaded on any compile failure (311-316) or store fault during restore (336-342 — not operator-preventable). The host logs and stays inert (ScriptedAlarmHostActor.OnLoadFailed:279-280), keeping only the mux interest alive. Publish-time validation makes the compile leg unlikely; the store-fault leg got worse odds of being transient-safe, not better: the store is now LocalDb (local SQLite), so faults are rarer but a reload no longer has central SQL retry semantics either. Recommendation unchanged: build-then-swap under the gate, or at minimum retry store faults distinctly from compile faults.

S4 — MEDIUM — _alarmsReferencing read without synchronization (carried)

Dictionary<string, HashSet<string>> (ScriptedAlarmEngine.cs:165-166) mutated under the gate in LoadAsync (261, 300-302), read and its HashSet values iterated without the gate in OnUpstreamChange (551, 558) from upstream callback threads. UnsubscribeFromUpstream (259) does not guarantee an already-executing callback finished. The coalescing rework added a beautifully documented _dirtyLock discipline for the new state (189-196) and iterates alarmIds inside it — but the set being iterated is the one LoadAsync can concurrently Clear(). The class's own comment block (76-83) justifies ConcurrentDictionary for _alarms for exactly this reader class; _alarmsReferencing remains the one plain structure on a cross-thread path. Cheap fix: immutable snapshot swapped via Volatile.Write, or ConcurrentDictionary<string, string[]> rebuilt per load.

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

ApplyPredicate (Part9StateMachine.cs:47-52) expires timed shelving silently (MaybeExpireShelving, 318-322): the state record unshelves and persists (via ReevaluateAsync's !ReferenceEquals persist, ScriptedAlarmEngine.cs:697-703), but the only Unshelved emission source remains ApplyShelvingCheck on the 5 s timer — which then finds Kind != Timed (302) and emits nothing. Clients tracking ShelvingState from events never learn the shelve ended. Now slightly more visible in effect than before: an expiry-coincident activation/clear fires un-suppressed (52, 67, 84) with no preceding Unshelved, which is exactly the confusing sequence the prior review predicted. Still no state-machine test for the path (14 ApplyPredicate tests, none crossing expiry). Fix as before: compound result (Unshelved + value transition), plus the missing test.

S6 — MEDIUM — LocalDbStoreAndForwardSink.Dispose races an in-flight drain (carried into the rewrite)

Dispose (LocalDbStoreAndForwardSink.cs:698-705) sets _disposed, disposes the timer (which does not wait for a running callback), then immediately disposes _drainGateLock and the writer. An in-flight DrainOnceAsync holds the gate (320) and may be mid-WriteBatchAsync (373): its finally { _drainGateLock.Release(); } (471) then throws ObjectDisposedException (surfacing as a spurious "drain tick faulted" Error, 175-197), and the writer can be disposed under an active gRPC call. DrainOnceAsync checks _disposed only at entry (319). The engine sibling solved this exact problem with placeholder-tracked drains; the sink still hasn't. U7-(c): still no Dispose-during-drain test (only disposed-then-call, LocalDbStoreAndForwardSinkTests.cs:453, 984-991). Fix: track the in-flight drain task (or Timer.Dispose(WaitHandle) + _drainGateLock.Wait()) before disposing gate/writer.

S7 — MEDIUM — Sandbox CPU/memory unbounded; timed-out scripts leak a thread; no quarantine (carried, sharpened)

Unchanged design (TimedScriptEvaluator.cs:10-26; ScriptSandbox.cs:30-35): every timed-out CPU-bound evaluation orphans a pool thread, on both live paths, with no per-script circuit breaker (repo-wide grep clean). The round-3 delta documents the compounding loop the prior review predicted: 837d8a17's starvation test found that the budget clock starts at WaitAsync, so under pool starvation legitimately queued evaluations time out spuriously — i.e. orphaned threads → starvation → more timeouts → more orphans, with each VT timeout also blocking its actor 2 s. The alarm engine's dirty-set coalescing meaningfully caps the rate (one pass per push cadence, not per queued task), which lowers the practical blast radius versus round 2 — but a hot dependency on a wedged script is still a slow pool-starvation vector with no quarantine and no Admin-UI surface. Recommendation unchanged: N-consecutive-timeouts → suspend + surface. The compile-time sandbox itself remains notably strong.

S8 — LOW — ScriptedAlarmEngine.Dispose blocks sync-over-async (carried)

Task.WhenAll(toAwait).GetAwaiter().GetResult() (ScriptedAlarmEngine.cs:1180). Fine under the actor host; IAsyncDisposable remains the clean fix. The placeholder-registration rework (1024-1078) made the drain correct; it is still synchronous.

S10 — LOW — VirtualTagSource.SubscribeAsync initial-value window (carried)

Unchanged, dormant (fold into U1).

S14 — MEDIUM — NEW — VT path deliberately ignores input quality: a virtual tag computed from Bad/comms-lost inputs publishes Good

#478/#481 taught the scripted-alarm path to derive worst-of-input quality and to hold evaluation on Bad inputs. The VT path never received the sibling treatment, and is now explicitly documented as not having it: DependencyValueChanged.StatusCode "…the virtual-tag engine ignores it" (VirtualTagActor.cs:33-38), and RoslynVirtualTagEvaluator.BuildReadCache stamps every dependency StatusCode: 0u Good regardless of the source's actual status (RoslynVirtualTagEvaluator.cs:165-177). VirtualTagActor.OnDependencyChanged stores the value and evaluates unconditionally (187-191). Consequence: when a driver goes comms-lost (#527 watchdog publishes Bad for its tags), every virtual tag computed from those tags keeps evaluating against the last-known values and keeps publishing fresh Good results with fresh timestamps — the one signature S13 was fixed to avoid (a broken script now degrades the node; a broken input still doesn't). A script author cannot even detect it: ctx.GetTag(path).StatusCode reads the fabricated Good. The dormant engine's AreInputsReady shape and the alarm engine's hold-on-Bad show the intended semantics. Recommendation: propagate msg.StatusCode into _dependencies (value + status pairs), pass real statuses through BuildReadCache, and pick a policy — either hold evaluation on Bad inputs (alarm parity) or publish the computed value with degraded quality. Pair with a test: driver goes Bad ⇒ dependent VT node does not stay/turn Good.

S15 — MEDIUM — NEW — The alarm drain-gate's "is my queue replicated?" predicate is hand-rolled and disagrees with LocalDbSetup.ReplicationIsConfigured

AddAlarmHistorian computes replicated as PeerAddress OR SyncListenPort (Runtime/ServiceCollectionExtensions.cs:183-185), while the predicate that actually decides whether the table is registered/captured for replication is PeerAddress OR ApiKey (Host/Configuration/LocalDbSetup.cs:171-179 — whose remarks explicitly rule that SyncListenPort must not count, because a port without a key rejects every inbound stream, and note the health check deliberately asks a third question). The two predicates answer the same question for the drain gate's purposes — "does a peer hold my rows?" — and diverge on the port-without-key misconfiguration: LocalDbSetup deregisters the table (the queue is local-only, replicating to nobody) while the drain gate treats the queue as shared and suspends the drain whenever ShouldDrainAlarmHistory is false. Result: that node's alarm events accumulate in a queue no node will ever drain, until capacity evictions discard them — the exact "deferring to anyone means they are never delivered by anyone" failure the gate's own comment block warns about (169-172). Mitigations: the health check reports Degraded for port-without-key, and GetStatus shows NotPrimary — but the sink-status diagnosis rule documented at IAlarmHistorianSink.cs:88-92 ("if BOTH nodes report this, no one is draining") is written for a genuinely replicating pair and reads as healthy here (only one node shows NotPrimary). Recommendation: make AddAlarmHistorian call LocalDbSetup.ReplicationIsConfigured(configuration) (move it somewhere both can reference, or duplicate with a parity test) so the gate scopes to the predicate that governs capture. One config-shape test: SyncListenPort-without-ApiKey ⇒ the sink always drains.

S16 — LOW — NEW — ScriptedAlarmEngine.OnUpstreamChange writes the value cache before the disposed check

_valueCache[path] = value (549) runs before the _disposed bail (550), so a late upstream callback mutates a cleared cache after Dispose finished (1199-1202 clears under no gate the callback holds). Harmless today (ConcurrentDictionary, nothing reads it afterwards) — noted so a future Dispose-order change doesn't turn it into a real use-after-clear.

2. Performance

P1 — HIGH — ScriptSandbox.Build rebuilds the full BCL reference set on every compile (carried, third round)

Byte-identical (ScriptSandbox.cs:83-87, 100-130). Every distinct script compile — publish-time VT/alarm compiles and every Admin-UI ScriptAnalysis request — still pays ~100+ MetadataReference.CreateFromFile calls. The P7 gate removed the worst amplifier (no-op redeploys no longer force mass recompiles), and the alarm engine's per-generation _compileCache.Clear() (ScriptedAlarmEngine.cs:270) means every real alarm publish still recompiles every predicate from scratch, each paying the rebuild. Three rounds open; the fix remains a static ConcurrentDictionary<Assembly, SandboxConfig> — an afternoon of work for a 10-100× compile-latency win.

P2 — MEDIUM — Pool hop + dispatcher block per evaluation (carried, accepted)

Unchanged (TimedScriptEvaluator.cs:74-81; RoslynVirtualTagEvaluator.cs:134). The batched mux delivery (one evaluation per burst) has materially reduced how often this cost is paid.

P3 — MEDIUM — Live VT adapter allocates per evaluation what the alarm engine pools (carried)

RoslynVirtualTagEvaluator.Evaluate still builds a fresh read-cache dictionary, VirtualTagContext, per-eval ForContext logger chain, and a fresh TimedScriptEvaluator wrapper per call (79-91, 133); the alarm engine's AlarmScratch (ScriptedAlarmEngine.cs:1231-1255) shows the pattern. On the hot ingress path; worth doing when P1 is touched.

P5 — LOW — Per-call DB overheads in the sink (carried, accepted; refs updated)

GetStatus runs a synchronous COUNT(*) per call (LocalDbStoreAndForwardSink.cs:527-532); RetryDeadLettered likewise synchronous (559-573). Fine at Admin-UI/health cadence; the enqueue capacity fast path (253-274, with the replication-aware periodic resync) is well done.

P6 — LOW — DependencyGraph well optimized (carried) — no action.

3. Conventions

C1 — MEDIUM — ITagUpstreamSource defined twice (carried)

Core.VirtualTags/ITagUpstreamSource.cs + the byte-shaped duplicate at ScriptedAlarmEngine.cs:1301-1317, still enforced only by comment ("intentionally identical shape so Stream G can compose them" — a plan-stream reference that is itself C6 residue). DependencyMuxTagUpstreamSource implements the ScriptedAlarms one; a VT-side upstream would need the other. Hoist into Core.Scripting.Abstractions.

C2 — MEDIUM — Abstractions assembly spans three namespaces undocumented (carried) — unchanged; same fix (project README stating the sandbox-closure rule).

C3 — LOW — Trailing type definitions in engine files (carried, grew)

ScriptedAlarmEvent (1258-1279), ScriptedAlarmProjection (1281-1299), duplicate ITagUpstreamSource (1301-1317) all trail ScriptedAlarmEngine.cs; both records are public cross-project contracts.

C5 — LOW — Core.AlarmHistorian mixes contract and implementation (carried, heavier)

The project now carries Microsoft.Data.Sqlite and the ZB.MOM.WW.LocalDb package (csproj:15, 28); Runtime and the gateway driver still reference the whole project to get IAlarmHistorianSink/IAlarmHistorianWriter. The LocalDb move would have been the natural moment to split contract from impl; it didn't happen.

C6 — LOW — Plan-era naming residue (carried) — 20 remaining "Phase 7 plan / Stream A-G" doc references across the five Core projects (ScriptedAlarmEngine.cs:9, IAlarmHistorianSink.cs:14, 20, TimedScriptEvaluator.cs:5, 39, IAlarmStateStore.cs, etc.). Sweep to current doc anchors (docs/ScriptedAlarms.md, docs/AlarmHistorian.md).

C7 — LOW — ApplyPredicate xmldoc claims branch handling that doesn't exist (carried) — Part9StateMachine.cs:31-33. Two rounds; a one-line comment fix. Pair with U5.

C8 — LOW — Script timeout budgets hard-coded and inconsistent (carried)

VT = ctor-default 2 s (RoslynVirtualTagEvaluator.cs:58; nothing passed at Program.cs:324-328); alarm predicates = 250 ms (TimedScriptEvaluator.cs:40; nothing passed at DriverHostActor.cs:786-787). Both budgets are now load-bearing safety controls (S7/S13/#478) and neither is operator-configurable despite the Scripting:* section existing for log settings.

C9 — LOW — NEW — EquipmentReferenceMap spells the effective-name rule twice, divergently

The class exists so "the effective-name rule … live[s] ONLY here" (EquipmentReferenceMap.cs:13-16), but it is implemented twice with different empty-string semantics: Build treats an empty DisplayNameOverride as "use the tag's name" (string.IsNullOrEmpty, line 58), while EffectiveNames uses ?? (line 88) — an empty-string override yields "", fails the IsNullOrEmpty filter (90), and the name is dropped from the authoring-gate/Monaco set even though Build (and therefore deploy resolution) would resolve it to the tag's name. On that input the editor rejects a {{equip}}/<Name> path publish accepts — inverting the editor⇔publish parity the seam exists to guarantee. Only reachable if authoring persists "" rather than null; make EffectiveNames reuse Build's rule (or extract one private EffectiveNameOf).

C10 — LOW — NEW — Operator/shelving-path emissions canonicalize the worst-input status, losing the sub-code

#519's stated goal is that the status sub-code rides "UNCHANGED" to the condition node (ScriptedAlarmEvent remarks, 1275-1278), and the evaluation path honors that — but the operator-command and shelving-timer paths rebuild the status from the 3-state bucket via LastWorstStatus (ScriptedAlarmEngine.cs:963-972), so an ack while an input is BadNoCommunication projects generic 0x80000000 Bad. Documented in-code as deliberate (the paths don't re-read inputs); cheap improvement: cache the last worst status code (already computed per evaluation at 746) alongside the bucket.

4. Underdeveloped Areas

U1 — HIGH — The dormant Core.VirtualTags engine stack: third round, still no decision

VirtualTagEngine (568 lines) + TimerTriggerScheduler + VirtualTagSource + their ~1,260 test lines remain production-dead (new VirtualTagEngine only in its own tests; ScriptAnalysisService.cs:350). Every argument from round 2 got stronger: the live path now has the timeout, the ALC-safe cache, quality degradation (S13), re-assert recovery, and batched-burst evaluation — the dormant engine's remaining unique features (topo cascade, cycle detection at runtime, timer triggers) are unshipped features, and the deploy-time Tarjan cycle gate lives in the Calculation-driver deploy path, not here. Meanwhile the stack carries S1/S3/S10 as permanently-open findings and (uniquely in this domain) mirrors the actual interface duplication (C1). Recommendation (unchanged, escalating): retire it this round, or write the one-paragraph sanction that makes keeping it a decision instead of a default. Keep DependencyGraph only if something real consumes it.

U4 — MEDIUM — IHistoryWriter is a permanently-Null surface (carried)

Still only NullHistoryWriter (Runtime/ServiceCollectionExtensions.cs:67-68, 387-388; sole implementation in the tree at Core.VirtualTags/IHistoryWriter.cs:24). The VT bridge dutifully calls _history.Record for Historize=true plans — with correct re-assert suppression and #519 status carriage (VirtualTagHostActor.cs:238-242) — into a no-op. The operator-visible Historize checkbox on virtual tags remains a silent lie, and the ContinuousHistorization recorder does not cover VT nodes (it taps raw-path historized refs). Either wire IHistoryWriter to the gateway value-writer or hide/annotate the checkbox.

U5 — MEDIUM — Part 9 conformance statement still missing (carried, narrowed)

Progress: docs/ScriptedAlarms.md now documents the message-template escaping cut (:135) and the ack/shelve surfaces in detail; quality semantics (#478) are documented. Still missing: one consolidated "what subset of Part 9 to expect" statement (no branches/previous-instances; static severity; Retain carried but consulted nowhere — ScriptedAlarmHostActor.cs:700 is its last stop; AlarmKind affects node typing only), and the C7 xmldoc still overpromises. Integrators still have to derive the subset from code.

U7 — LOW — Test coverage: strong and improving, four specific holes remain (carried)

Closed this round: (f) node-quality degradation, plus the coalescing falsification control, drain-latch handover/stack pins, register-before-start pin, sink outcome/poison/corrupt/capacity suite (39 tests), condition-store parity round-trips, projection tests. Still open: (a) S5's expiry-during-predicate emission swallow; (b) load-vs-cascade concurrency (moot if U1 resolves to retirement); (c) sink Dispose-during-drain (S6); (d) a dedicated ForbiddenTypeAnalyzer unit suite (the deny-list is the actual security boundary and is still only tested end-to-end through Compile).

---

Maturity Ratings

Scale: 1 (prototype) → 5 (production-hardened). Previous scores (2026-07-12 @ f6eaa267) in parentheses.

Dimension	Old → New	Justification
Stability	(3.5) → 4	Both remediation-introduced moderates (S12/S13) verifiably fixed with tests that can fail; the S2 fan-out High is closed on every live path with a falsification-controlled test; the drain-latch/background-task lifecycle work is exemplary concurrency engineering; the LocalDb move made alarm history failover-safe with a correctly scoped drain gate. Held below 4.5 by the untouched hardening backlog (S3/S4/S5/S6/S7 — all three rounds old), one new gate-predicate drift (S15), and the VT input-quality gap (S14).
Performance	(3) → 3.5	Dirty-set coalescing + batched mux delivery + retained scratch reuse remove the real hot-path waste; P7 fixed. P1 — the domain's standing High, now three rounds old — is the single reason this isn't 4: every compile everywhere still rebuilds the BCL reference set.
Conventions	(4) → 4	New code's why-comments are the best in the repo (the drain-latch finally block, the drain-gate scoping rationale, the migrator remarks are teaching documents). But the ledger barely moved: C1/C2/C3/C5/C6/C7/C8 all carried a third round, C3/C5 grew, and two small new inconsistencies (C9/C10) appeared. Quality of new code high; debt retirement near zero.
Underdeveloped areas	(3) → 3.5	#487 + #481 + #478 closed real operator-facing gaps; S9/S11 documentation debts paid; U7 keeps shrinking. Held down by the same two anchors as round 2: the dormant 1,100-line engine still awaits a decision (U1 — now the oldest unactioned High in the domain), and Historize on virtual tags is still a silent no-op (U4).

---

Priority Recommendations (ordered)

1. U1 — Decide the dormant VirtualTagEngine stack's fate. Retiring it closes S1, S10, half of S3, U7-(b), and the C1 duplicate in one commit. Three rounds is enough.
2. S14 — Propagate dependency quality into the VT path (or hold evaluation on Bad inputs). S13 fixed "broken script looks Good"; "broken input looks Good" is the same operator lie through the other door.
3. P1 — Statically cache the ScriptSandbox reference set. Third round; every publish and every editor keystroke still pays it; the alarm engine's per-publish cache clear guarantees mass recompiles pay it too.
4. S15 — Unify the drain-gate replicated predicate with LocalDbSetup.ReplicationIsConfigured (+ one config-shape test) before a port-without-key misconfiguration maroons a node's alarm queue.
5. S7 — Per-script quarantine (N consecutive timeouts → suspend + Admin-UI chip). The starvation test (837d8a17) has now empirically documented the timeout↔starvation coupling the breaker would sever.
6. S5 + C7 — Fix the predicate-path shelve-expiry emission and the branch-claim doc drift together, with the missing state-machine test.
7. S3/S4/S6 — One hardening pass: build-then-swap reload, _alarmsReferencing snapshot-swap, sink dispose-drain (with the U7-(c) test).
8. U4 — Wire IHistoryWriter to the gateway value-writer or remove/annotate the VT Historize checkbox.