Review follow-ups to 34829ea0.
IMPORTANT - the leak filter had its own flake vector. This test saturates the
thread pool by design, and TimedScriptEvaluator starts its 5s clock at WaitAsync
rather than when the inner Task.Run gets a thread, so a legitimately-enqueued
evaluation can blow the budget while still queued and surface "script timed out
after 5.0s". The reason != exhaustion filter classified that as a raw disposed
escape and hard-failed on it - handing the test back the load-fragility the
reshape removed. Timeouts are now their own bucket, tolerated and rate-bounded
exactly as exhaustions are; a raw disposed escape still fails on first occurrence.
Doc-comment names this second residual vector alongside the exhaustion calibration.
MINOR (scope extension authorised by review) - "evaluator disposed during
evaluation" existed as three unshared literals. Promoted to
ScriptEvalFailureReasons alongside VirtualTagEvalResult in Commons.Engines (the
assembly all three already reference) and referenced from both evaluators and the
test. The timeout prefix joins it, since the test now classifies on it too. String
values are byte-identical - no behaviour change.
Falsified: with a 12ms script budget the race produced real timeouts (3/2695).
Classified as leaks (the pre-review filter) they hard-failed the test 6/6 items;
bucketed as starvation they sit 0.11% under the 10% bound and the test is green.
U2 (Critical): RoslynVirtualTagEvaluator ran scripts with a CancellationToken
that a synchronous Roslyn script can never observe, so one CPU-bound/infinite-
loop virtual-tag script hung the owning VirtualTagActor forever (no timeout, no
recovery). Route evaluation through the existing TimedScriptEvaluator (Task.Run
+ WaitAsync) so the wall-clock budget is real; a runaway script now returns Bad
within the timeout instead of wedging the actor.
U3 (High): the live path cached evaluators in a raw ConcurrentDictionary never
cleared on republish, leaking a collectible AssemblyLoadContext per edited
script forever. Swap in the purpose-built CompiledScriptCache (Lazy single-
compile, dispose-on-Clear) and add an apply-boundary clear: a new narrow
IScriptCacheOwner seam the VirtualTagHostActor calls on each ApplyVirtualTags
generation (mirrors ScriptedAlarmEngine's per-generation clear).
Tests: infinite-loop-fails-within-timeout (bounded by WaitAsync so a regression
fails instead of hanging), happy-path-after-wrapping, ClearCompiledScripts-
empties-cache, and a host-actor wiring guard asserting each apply generation
calls ClearCompiledScripts (theme #1 production-caller assertion). Host.Integration
VT suite 18/18, Runtime VT suite 12/12.
Adds <summary>/<param>/<returns>/<inheritdoc> where missing and removes
project bookkeeping IDs (task/tracking refs) from shipped code comments,
so the docs read cleanly and CommentChecker is quiet except for known
false positives (PLC/protocol terms, event/IEqualityComparer inheritdoc).
Doc/comment-only; no logic changed; solution builds clean.
Adds <summary>, <param>, <typeparam>, and <inheritdoc/> tags to public
members surfaced by commentchecker — resolves 5,847 of 5,869 issues
(99.6%) across three /fixdocs passes.
ScriptedAlarmActor now survives actor restart: PreStart loads from
the configured store + restores in-memory state; every Transition()
fires a fire-and-forget save. ActiveState still re-derives from the
evaluator on first tick (Phase 7 decision #14), but Acked state +
lastAckUser persist verbatim so operators don't re-ack across an
outage.
Three pieces:
- IAlarmActorStateStore seam in Commons.Engines, with the
AlarmActorStateSnapshot record (alarmId / state / lastTransitionUtc
/ lastAckUser) and NullAlarmActorStateStore default.
- EfAlarmActorStateStore in Runtime.ScriptedAlarms — production
adapter over the existing ScriptedAlarmState table in ConfigDb.
Maps the actor's 3-state enum to the table's AckedState column
(Active⇒Unacknowledged, Acknowledged⇒Acknowledged, Inactive⇒
Acknowledged). Concurrency conflicts are logged + dropped — the
next transition writes again.
- ScriptedAlarmActor PreStart load (async, piped back as
StateRestored) + Transition save. New Props overload takes the
store; default is NullAlarmActorStateStore so tests stay quiet.
Tests: Runtime 52 -> 57 (+5):
- Transition writes Active then Acknowledged snapshots with
lastAckUser populated
- PreStart with persisted Active state restores so a subsequent
AcknowledgeAlarm fires (not ignored as it would be from Inactive)
- Empty store boots Inactive (AcknowledgeAlarm correctly ignored)
- EfAlarmActorStateStore Save + Load round-trips via in-memory EF
- Load for unknown alarmId returns null
All 6 v2 test suites green: 157 tests passing.
Closes#112. F9 (#80) remaining residual is predicate binding to
Core.ScriptedAlarms.ScriptedAlarmEngine — split as F9b in tasks JSON.
VirtualTagActor and ScriptedAlarmActor now route through pluggable
evaluator interfaces and fan out to the cluster's live-tail topics
shipped in F15.3:
- IVirtualTagEvaluator + NullVirtualTagEvaluator in Commons.Engines.
VirtualTagActor calls evaluator on every DependencyValueChanged,
dedupes unchanged values, forwards EvaluationResult to its parent,
and publishes ScriptLogEntry Warning to the script-logs DPS topic
whenever the evaluator fails.
- IScriptedAlarmEvaluator + NullScriptedAlarmEvaluator. ScriptedAlarmActor
takes an AlarmConfig (id/name/equipment-path/severity/predicate) and
publishes both an AlarmTransitionEvent (alerts topic) and a
ScriptLogEntry (script-logs topic) at every transition. Manual
ConditionMet/Acknowledge/Cleared still flow through the same
Transition() so callers without engine bindings still drive the
state machine; the legacy single-string Props() overload routes
through a default AlarmConfig.
The Null* defaults keep the actors safe when no engine is bound —
unconfigured nodes never spuriously alarm. Production binding to
Core.VirtualTags.VirtualTagEngine and Core.ScriptedAlarms is the
remaining residual (F8b/F9b — split in tasks JSON).
Tests: Runtime 34 -> 40 (+6):
- VirtualTagActorTests x3 (evaluator drives EvaluationResult,
unchanged-value dedup, failure publishes Warning ScriptLogEntry)
- ScriptedAlarmActorTests x3 (engine threshold drives Activated +
Cleared on alerts topic, manual Acknowledge attribution).
All 6 v2 test suites green: 126 tests passing.
Joseph Doherty