Re-ran all seven domain reviews at master f6eaa267 (reports rewritten in
place, each with a prior-finding status table): all 4 round-1 Criticals
verified closed; new top findings are the S7 connect-timeout OCE regression
(05/STAB-14), the ResilienceConfig operator-authorable brick (01/S-6), and
a batch of resilience-seam Mediums. 00-OVERALL.md carries the updated
maturity matrix + 12-item action list.
Adds R2-01..R2-12 design/implementation plans (one per action item, house
format + bite-sized TDD task breakdowns + co-located .tasks.json; 193 tasks,
~18-24 dev-days). STATUS.md updated: round-1 topology marked historical
(all merged+pushed), re-review findings table + plan pointers added.
46 KiB
Design + Implementation Plan — R2-03: VT script-failure quality + cache-clear hardening
- Source report:
archreview/02-scripting-alarms.md(2026-07-12 re-review) - Review commit:
f6eaa267· Plan verified against tree at:f6eaa267(master, clean) - Scope: OVERALL prioritized action item #3 — "Publish Bad quality on VT script failure/timeout
(match the dormant engine's BadInternalError); optionally gate the apply-boundary cache clear on
changed scripts" — covering findings 02/S13 (Medium — failure/timeout never degrades node quality),
02/S12 (Medium — apply-boundary
ClearCompiledScriptsraces in-flight child evaluations), and 02/P7 (Low — unconditional per-apply cache flush forces full VT recompilation on every deploy). - Touches:
Runtime/VirtualTags/VirtualTagActor.cs,Runtime/VirtualTags/VirtualTagHostActor.cs,Host/Engines/RoslynVirtualTagEvaluator.cs,Runtime.Tests/VirtualTags/*,Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs. No Core.Scripting / Commons-contract / OPC UA-sink changes except one additive field onVirtualTagActor.EvaluationResult.
Verification summary
Every anchor in the three findings was opened at the cited file:line against the working tree at
f6eaa267. All three findings CONFIRMED. One minor anchor drift (noted below); everything else is
line-exact.
- S13 — Confirmed.
VirtualTagActor.OnDependencyChangedreturns without any publish on!result.Success(Runtime/VirtualTags/VirtualTagActor.cs:123-128) and on evaluator-threw (:115-121) — WarningScriptLogEntry+failmetric only. The host bridge hard-codesOpcUaQuality.Goodinto everyAttributeValueUpdate(Runtime/VirtualTags/VirtualTagHostActor.cs:179-180) and hard-codes0u /* StatusCodes.Good */into the historize snapshot (:187-188).EvaluationResult(VirtualTagActor.cs:23) carries no quality at all, so the child cannot express degradation today. Confirmed downstream that Bad quality is honored end to end once published:OpcUaPublishActor.HandleAttributeUpdate→IOpcUaAddressSpaceSink.WriteValue→OtOpcUaNodeManager.WriteValue(OpcUaServer/OtOpcUaNodeManager.cs:261-281) setsvariable.StatusCode = StatusFromQuality(quality)+ClearChangeMasks, whereStatusFromQuality(:2418-2423) mapsGood→StatusCodes.Good,Uncertain→StatusCodes.Uncertain,_→StatusCodes.Bad. So the only missing plumbing is child → bridge. - S13 anchor drift (minor): the report cites the dormant engine's failure mapping at
VirtualTagEngine.cs:307,328. Atf6eaa267, line 307 is the cold-start guard (BadWaitingForInitialDatawhen inputs aren't ready yet —AreInputsReady); the actual failure→BadInternalError(0x80020000u) mappings are at 327-329 (uncoercible result), 334 (ScriptTimeoutException), and 343 (script threw). The substance of the finding is unaffected — and line 305-311 turns out to be load-bearing for this plan's design (the inputs-ready gate, below). - S12 — Confirmed.
VirtualTagHostActor.OnApplycallsClearCompiledScripts()first thing (VirtualTagHostActor.cs:92), on the host's dispatcher thread, with nothing gating concurrent child evaluations. The race window is inside oneRoslynVirtualTagEvaluator.Evaluatecall: the evaluator is fetched atRoslynVirtualTagEvaluator.cs:69(_cache.GetOrCompile), the host'sClear()value-scoped-removes and disposes it (CompiledScriptCache.cs:113-116→ScriptEvaluator.Dispose→_alc.Unload()), and the child's subsequent run (RoslynVirtualTagEvaluator.cs:108-109, viaTimedScriptEvaluator.RunAsync'sTask.Run) trips the disposed guard (ScriptEvaluator.cs:180) →ObjectDisposedExceptionsurfaces throughGetAwaiter().GetResult()into the genericcatch (Exception)(RoslynVirtualTagEvaluator.cs:116-120) →Failure("script threw: Cannot access a disposed object…")→ dropped update (and, per S13, no quality degradation either). The contrast withScriptedAlarmEngineholds: its per-generation_compileCache.Clear()(ScriptedAlarmEngine.cs:209) runs under_evalGatewith upstream subscriptions already torn down, so no alarm evaluation can hold a disposed evaluator. The secondary wrinkle also confirmed: the clear runs before the removed/changed children are stopped (VirtualTagHostActor.cs:98-118) andContext.Stopis asynchronous, so a doomed child can recompile its stale expression into the fresh cache for one generation (bounded — not a leak). Note a mid-execution clear is safe:AssemblyLoadContext.Unload()is cooperative (the executing delegate keeps the ALC alive); only the guard atScriptEvaluator.cs:180— i.e. dispose landing between fetch and run-start — produces the dropped update. TheTask.Runhop insideTimedScriptEvaluatorwidens exactly that window. - P7 — Confirmed. The clear at
VirtualTagHostActor.cs:92is unconditional perApplyVirtualTags, including a byte-identical redeploy whose plans all diff equal (the no-churn path proven byApplyVirtualTags_does_not_respawn_child_when_plan_unchanged). The host already keeps last generation's plans in_planByVtag(:50, rebuilt at:160-164) — the comparison input for a conditional clear is already sitting in actor state.EquipmentVirtualTagPlanhas element-wise value equality includingExpression(OpcUaServer/AddressSpaceComposer.cs:179-188). The existing wiring guardVirtualTagHostActorTests.ApplyVirtualTags_clears_the_evaluator_compile_cache_each_generationasserts the clear fires on an identical second apply — that test encodes the P7 behavior and must be rewritten, not just kept green (called out in the task breakdown). - Existing-test impact identified:
VirtualTagActorTests.Evaluator_failure_publishes_ScriptLogEntry_warningends withparent.ExpectNoMsg(...)— under the S13 fix a failure now does send the parent a Bad-qualityEvaluationResult, so that assertion inverts (task R2-03-T3 updates it deliberately). - Design constraint verified: the value fan-out's quality vocabulary is the coarse 3-value
OpcUaQuality { Good, Uncertain, Bad }(Commons/OpcUa/IOpcUaAddressSpaceSink.cs:99) — there is no sub-code channel to the node, andAttributeValueUpdate(Runtime/OpcUa/OpcUaPublishActor.cs:42) already carries it.NullVirtualTagEvaluatorreturnsNoChange(Success=true), so the dev/Mac path can never emit Bad. The passthrough fast-path returnsOk(null)for a missing dep (never Failure), so it is unaffected by all three fixes.
Priority ordering
Per the source report's Priority Recommendations (#1 = S13, #2 = S12) and OVERALL action item #3:
- S13 (Medium, report's top item) — failure/timeout → node quality. The stale-Good repro test lands first and must fail on current code.
- S12 (Medium) — close the clear-vs-in-flight-evaluate race (retry-once in the adapter).
- P7 (Low) — gate the apply-boundary clear on an actually-changed script set.
All three are one PR (fix/archreview-r2-03-vt-failure-quality): S13 and S12 touch disjoint files;
P7 touches the same OnApply region S13's bridge work sits next to.
1. S13 — MEDIUM — Script failure/timeout never degrades the OPC UA node's quality: stale value stays Good forever
Restatement: When a VT script evaluation fails (compile error, runtime throw, sandbox violation, or
the U2-era timeout), VirtualTagActor logs + meters and returns without publishing anything, and the
host bridge only ever publishes OpcUaQuality.Good. A previously-working script that starts failing
leaves its node serving the last Good value with its old timestamp indefinitely — OPC UA clients, the
continuous-historization recorder, and ScadaBridge cannot distinguish "healthy but unchanged" from
"script broken for a week". The dormant VirtualTagEngine maps this correctly to BadInternalError;
the live actor path never implemented it.
Verification: Confirmed (see summary). Bad quality is fully plumbed from AttributeValueUpdate
through the sink to the SDK node — the gap is exactly VirtualTagActor (no quality concept) and the
VirtualTagHostActor bridge (hard-coded Good at :179-180, hard-coded 0u historize status at
:187-188).
Root cause: EvaluationResult was designed as a success-only message ("emit to the parent whenever
the value actually changes"); failures were treated purely as an observability event (script-log +
metric). The quality dimension was left to the materialiser's initial BadWaitingForInitialData and
never revisited when the U2 remediation made timeouts a first-class, expected failure mode.
Proposed design:
(a) StatusCode policy — coarse OpcUaQuality.Bad for both failure and timeout. The node will read
StatusCodes.Bad (0x80000000) via the existing StatusFromQuality mapping. This "matches the dormant
engine's BadInternalError" at the level that matters — the severity bit that flips client/consumer
handling — and the failure kind (timeout vs throw vs compile) is already delivered verbatim to
operators as the script-log Reason string on the script-logs topic. Alternatives rejected:
- Widen
OpcUaQualitywithBadInternalError/BadTimeoutmembers — additive on the enum, but it ripples a Commons contract consumed by every sink implementation and driver publish path, for a sub-code no consumer acts on. Contained follow-up if a real client requirement appears. - Carry a raw
uintstatus throughAttributeValueUpdate/IOpcUaAddressSpaceSink.WriteValue— the precise-parity option, but it changes an interface with four implementations and re-runs the F10b/DeferredAddressSpaceSink forwarding trap for zero operator value. Rejected. - Exception: the historize snapshot takes raw
uintstatus already (DataValueSnapshot), so there the bridge records exact dormant-engine parity:0x80020000u /* BadInternalError */on Bad results (the dormant engine historizes its Bad snapshots unconditionally atVirtualTagEngine.cs:346-348; we mirror that).
(b) Publish cadence — once per transition, not per failure. Every publish is a sink write +
ClearChangeMasks → client notification on the pinned OPC UA dispatcher. A failing script on a hot
dependency would otherwise emit one identical Bad write per dependency change (each also paying the P2
pool hop + 2 s worst-case actor block per S7). The actor tracks _lastPublishedBad; it publishes Bad on
the Good→Bad (or initial→Bad) transition only, and on recovery it force-publishes the next success even
if the value equals the pre-failure value — otherwise the value-dedup at VirtualTagActor.cs:138
would leave the node Bad forever after a same-value recovery. (Script-log Warning entries stay
per-failure, unchanged — that's the existing operator diagnostic and its volume is pre-existing.)
(c) Cold-start suppression — an inputs-ready gate, mirroring the dormant engine. The live path has no
AreInputsReady equivalent: a multi-dep script fails on the first dependency arrival (the missing dep's
GetTag(...).Value is null → cast throws) until all deps have landed. Publishing Bad on those would
make every fresh/respawned multi-dep VT flash BadWaitingForInitialData → Bad → Good on every deploy —
operator-visible churn for healthy scripts. Worse, gating on "had a prior success in this child" is
insufficient for S13's headline scenario: an edited-now-broken script respawns its child (plan
change ⇒ stop+respawn at VirtualTagHostActor.cs:109-118), so the fresh child has no prior success and
would never degrade — reintroducing stale-Good exactly where it matters most. The correct gate is the
dormant engine's: suppress Bad publishes until every dependencyRefs entry has been received at least
once (_dependencyRefs.All(_dependencies.ContainsKey)); after that, any failure degrades. A
never-arriving dependency keeps the node at the materialiser's BadWaitingForInitialData (already Bad —
acceptable, and per the report "acceptable" for never-succeeded scripts).
(d) Message/bridge shape — additive Quality on EvaluationResult. Add
OpcUaQuality Quality = OpcUaQuality.Good as a trailing optional parameter on the
EvaluationResult positional record: every existing construction and destructuring site compiles
unchanged, and the message is a local Context.Parent.Tell (no wire serialization concern). The bridge
replaces the hard-coded OpcUaQuality.Good with result.Quality, and maps the historize status
(Good→0u, else 0x80020000u). The dependency mux is untouched — quality flows child→parent→publish
actor; the mux only fans values into children (DependencyValueChanged is unchanged), so no mux
registration, routing, or message contract moves.
On failure the Bad EvaluationResult carries the last-known value (_lastValue if _hasLastValue,
else null) with the triggering msg.TimestampUtc — clients see the stale value explicitly flagged Bad
(the report's "publish the existing value with Bad quality"), rather than the dormant engine's
null-value choice; either is defensible, this one preserves more information for trend displays.
Implementation steps:
Runtime/VirtualTags/VirtualTagActor.cs:using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;public sealed record EvaluationResult(string VirtualTagId, object? Value, DateTime TimestampUtc, CorrelationId Correlation, OpcUaQuality Quality = OpcUaQuality.Good);- Add fields
private bool _lastPublishedBad;and a helperprivate bool AllDependenciesArrived() => _dependencyRefs.All(_dependencies.ContainsKey);(refs list is small; called once per failure — no caching needed). - Extract the failure tail shared by the evaluator-threw catch (
:115-121) and the!Successbranch (:123-128) intoprivate void OnEvaluationFailed(string reason, string level, DateTime timestampUtc): keep the existing metric +PublishLog, thenif (_lastPublishedBad || !AllDependenciesArrived()) return;else_lastPublishedBad = true; Context.Parent.Tell(new EvaluationResult(_virtualTagId, _hasLastValue ? _lastValue : null, timestampUtc, CorrelationId.NewId(), OpcUaQuality.Bad)); - Success path: change the dedup guard at
:138toif (!_lastPublishedBad && _hasLastValue && Equals(_lastValue, result.Value))and set_lastPublishedBad = false;alongside_hasLastValue = true;before the Good publish. TheNoChangeshort-circuit (:132-136) stays above the dedup and does not touch_lastPublishedBad(NoChange = "no fresh value", not a recovery). - Update the class summary: failures now degrade quality once per transition.
Runtime/VirtualTags/VirtualTagHostActor.cs(OnResult,:170-190):_publishActor.Tell(new OpcUaPublishActor.AttributeValueUpdate(nodeId, result.Value, result.Quality, result.TimestampUtc));- Historize:
var status = result.Quality == OpcUaQuality.Good ? 0u : 0x80020000u /* BadInternalError — dormant-engine parity */;and passstatusinto theDataValueSnapshot. - Update the
:179-180comment (no longer hard-coded Good).
- Update
VirtualTagActorTests.Evaluator_failure_publishes_ScriptLogEntry_warning: the trailingparent.ExpectNoMsgbecomesparent.ExpectMsg<EvaluationResult>()withQuality == Bad(the test constructs the actor with emptydependencyRefs, so the inputs-ready gate is vacuously satisfied —Allover an empty list is true).
Tests: (all deterministic; probes only)
- Repro first (must FAIL at
f6eaa267):Runtime.TestsVirtualTagHostActorTests.Failing_script_after_success_degrades_node_quality_to_Bad— host + real child + a scripted fail-after-successIVirtualTagEvaluatorstub; drive the child (captured via the mux probe'sRegisterInterestsender) with twoDependencyValueChangeds; assert the publish probe receives update 1(Good, 42)then update 2 withQuality == OpcUaQuality.Badcarrying the last value. Uses only current types (AttributeValueUpdatealready hasQuality), so it compiles today and times out on the secondExpectMsg— the stale-Good repro. VirtualTagActorTests: failure-after-success emits BadEvaluationResultwith last value; second consecutive failure emits nothing (once-per-transition); recovery to the same value as before the failure does publish (dedup bypass); evaluator-threw path (not justFailure) also degrades; multi-dep cold-start failure with a missing dep publishes no Bad (inputs-ready gate); after all deps arrive, failure publishes Bad.VirtualTagHostActorTests: a Bad-qualityEvaluationResultbridges to anAttributeValueUpdatewithQuality == Bad; a historized plan's Bad result records status0x80020000u(BadInternalError).- Wiring note (theme #1, "unit-green ≠ wired"): the repro test above IS the wiring assertion — it runs
the real host + real child + parent-Tell bridge, not a hand-delivered message. The remaining live wire
(publish actor → sink → SDK node) is pre-existing, already exercised by
OpcUaPublishActortests, and covered by the live-/rungate below.
Effort: S. Risk: Low-Medium. The behavioral deltas are (1) parents now receive Bad results —
only VirtualTagHostActor parents children in production, and it handles the new field natively; (2) a
node that used to freeze Good now goes Bad on persistent failure — the intended, strictly-safer change;
(3) one existing test's expectation inverts (deliberate, task-tracked). Blast radius = VT value path
only; alarms/drivers untouched.
2. S12 — MEDIUM — Apply-boundary ClearCompiledScripts races in-flight child evaluations
Restatement: The host's per-apply Clear() disposes cached evaluators while unchanged children keep
evaluating on other dispatcher threads. A child that fetched its evaluator (GetOrCompile,
RoslynVirtualTagEvaluator.cs:69) before the clear hits the disposed guard on run
(ScriptEvaluator.cs:180) → ObjectDisposedException → generic catch → Failure → silently dropped
update (self-heals only on the tag's next dependency change — a long staleness window for slow tags).
Secondary: a stopping child can recompile its stale script into the fresh cache for one generation.
Verification: Confirmed (see summary). The cache itself is blameless — its value-scoped TryRemove
protects concurrent GetOrCompile re-adds; the gap is clear-vs-use of an already-handed-out
evaluator. The ScriptedAlarmEngine mirror is inexact exactly as the report says (gate-held clear after
subscription teardown, ScriptedAlarmEngine.cs:199-209).
Root cause: IScriptCacheOwner.ClearCompiledScripts was wired at the apply boundary (U3) without a
coordination primitive between the host actor's clear and child actors' fetch→run sequence — the
Task.Run hop inside TimedScriptEvaluator guarantees a real window between fetch and the disposed
check.
Proposed design — catch-ObjectDisposedException, re-fetch, retry once (in the adapter). The
recompile is idempotent (same source → fresh evaluator in the current cache generation), the retry is
entirely local to RoslynVirtualTagEvaluator.Evaluate, and it converts the dropped update into a
correct evaluation against the post-clear generation. If a second clear lands mid-retry (two applies
inside one evaluation — not a realistic cadence), the second ObjectDisposedException falls through to
the existing Failure path — same as today's worst case, now with an S13 Bad-quality publish instead of
silence. Alternatives rejected:
ReaderWriterLockSlim(children read-lock fetch→run; clear write-locks) — blocks the host actor for up to a full in-flight timeout budget (2 s) during every apply, and adds lock traffic to the hot path, to prevent a race the retry heals for free. Rejected.- Per-generation cache instances with deferred dispose of the old generation — disposing the old generation safely still requires knowing when in-flight evaluations finish, i.e. ref-counting — the same coordination problem with more machinery. Rejected.
- Move the clear after the stop/respawn reconciliation — doesn't close the race for unchanged
children (the common case), and the stale-recompile wrinkle survives either way because
Context.Stopis asynchronous. Rejected as a fix; the wrinkle stays accepted + documented (bounded to one generation, released at the next apply-with-changes; P7's gate also means no-op redeploys no longer create the window at all).
Implementation steps (Host/Engines/RoslynVirtualTagEvaluator.cs, the compiled path :66-121):
- Restructure fetch+run into a bounded loop (
for (var attempt = 0; ; attempt++)):- fetch:
evaluator = _cache.GetOrCompile(expression);— keep the existingCompilationErrorException/ScriptSandboxViolationException/ generic catches on the fetch (unchanged behavior). A disposed cache here means the whole evaluator is being shut down —GetOrCompilethrowsObjectDisposedException, which the new handler must NOT retry: check_disposedand returnVirtualTagEvalResult.Failure("evaluator disposed"). - run: existing
TimedScriptEvaluator+GetAwaiter().GetResult()block, adding before the generic catch:catch (ObjectDisposedException) when (!_disposed && attempt == 0) { // S12: the apply-boundary ClearCompiledScripts disposed the evaluator between our // GetOrCompile and the run's disposed-guard. Re-fetch — GetOrCompile recompiles the same // source into the CURRENT cache generation — and retry once. A second disposal mid-retry // (two applies inside one evaluation) falls through to the generic failure path. continue; } catch (ObjectDisposedException)(second hit, or_disposed) →Failure("evaluator disposed during evaluation").
- fetch:
- Update the class doc: the apply-boundary clear is retry-safe for in-flight evaluations; the one-generation stale-recompile wrinkle for stopping children is the documented residual.
Tests (Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs):
Evaluate_racing_ClearCompiledScripts_never_fails_with_disposed— one evaluator; task A loops N (≈300) compiled (non-passthrough) evaluations of the same expression; task B loopsClearCompiledScripts()withThread.Yield()between calls until A finishes; whole testWaitAsync-bounded (30 s). Assert every resultSuccess == true(and specifically noReasoncontaining "disposed"). Determinism note: the pre-fix failure is probabilistic per iteration (the window is fetch →Task.Run-hopped disposed-guard, widened by pool queuing), but at N=300 racing clears it reproduces reliably in practice; post-fix it is deterministic-green by construction (the retry is exhaustive for a single concurrent clear). This is the report's prescribed "concurrent clear-during-evaluate test".Evaluate_after_ClearCompiledScripts_recompiles_and_succeeds— deterministic sequential guard: evaluate, clear, evaluate same source again → Success both times, cache Count back to 1 (uses the existingCompiledCacheCountreflection helper).- Existing
Evaluate_after_dispose_returns_Failurestill green (the_disposedshort-circuit at:51is untouched; the new ODE handler's!_disposedfilter keeps shutdown semantics).
Effort: S (the report's "10-line fix" is accurate; the loop restructure is the only care point). Risk: Low. Happy path unchanged (attempt 0, no exception); passthrough fast-path never enters the loop. Blast radius = one method in the Host adapter.
3. P7 — LOW — Unconditional per-apply cache flush forces full VT recompilation on every deploy
Restatement: OnApply clears the compile cache on every ApplyVirtualTags
(VirtualTagHostActor.cs:92) — including a byte-identical redeploy that stops/spawns nothing. Every VT
script then recompiles on its next evaluation, each paying the still-open P1 full-BCL-reference-set
rebuild (5-20 ms+ per script). Amplifies P1; also each unnecessary clear is an unnecessary S12 race
window.
Verification: Confirmed (see summary). The host already holds last generation's plans in
_planByVtag; EquipmentVirtualTagPlan.Expression participates in value equality. The existing wiring
guard asserts the current (unconditional) behavior on an identical redeploy and must be rewritten.
Root cause: The U3 fix chose the simplest correct lifecycle ("clear per generation", mirroring the alarm engine) without consulting the plan diff the same method computes a few lines later.
Proposed design — clear only when the deployed expression set changed. At the top of OnApply,
compare the desired expression set against the previous generation's
(_planByVtag.Values.Select(p => p.Expression)), and clear only on inequality:
var newExpressions = msg.Plans.Select(p => p.Expression).ToHashSet(StringComparer.Ordinal);
var prevExpressions = _planByVtag.Values.Select(p => p.Expression).ToHashSet(StringComparer.Ordinal);
if (!newExpressions.SetEquals(prevExpressions))
{
(_evaluator as IScriptCacheOwner)?.ClearCompiledScripts();
}
Decisions inside this shape:
- Expression set, not full-plan set: the cache is keyed by source hash only, so a rename / FolderPath move / Historize toggle (which respawns the child) does not need a recompile — comparing full plans would clear on those spuriously. Two vtags sharing one expression are handled naturally (set semantics: the expression survives while any plan still uses it).
- Any-change clears everything (not per-source eviction): editing 1 of 200 scripts still recompiles
200. Accepted per the report ("cheap to leave as-is at current scale; fix alongside P1") — the
headline P7 cost is the no-op redeploy, which this eliminates entirely. Rejected alternative: a
CompiledScriptCache.RetainOnly(IEnumerable<string> sources)API — precise, but new Core.Scripting surface + tests for a Low; note it as the follow-up shape if P1's memoization doesn't land first. - First apply after actor (re)start:
_planByVtagis empty → sets differ → clear fires (harmless: the singleton evaluator's cache may hold a prior incarnation's entries; clearing converges state). - Interaction with S12 (design question d): independent and composing — the gate reduces clear frequency (no-op redeploys stop racing entirely), the retry remains the correctness backstop for genuine changes. The gate must NOT be treated as the S12 fix: a real script edit still clears while unchanged children evaluate.
- Interaction with S13: none (quality path doesn't touch the cache).
Implementation steps:
- Replace
VirtualTagHostActor.cs:88-92(comment + unconditional call) with the guarded block above + a comment explaining the expression-set gate (and that_planByVtagis "the previous generation" at this point — the method rebuilds it at the end). - Rewrite the wiring guard
ApplyVirtualTags_clears_the_evaluator_compile_cache_each_generation→ApplyVirtualTags_clears_the_compile_cache_only_when_the_expression_set_changes: apply plan A (ClearCount 1) → re-apply identical plan A (ClearCount still 1) → apply plan A′ with an editedExpression(ClearCount 2) → re-apply with a vtag removed but its expression still used by another vtag (ClearCount still 2). Keeps the load-bearing "the host actually calls it" wiring assertion while pinning the new gate.
Tests: the rewritten wiring guard above (in Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs,
using the existing CacheOwningStubEvaluator spy). New-behavior red state: the identical-redeploy
step fails on current code (ClearCount is 2, expected 1).
Effort: S. Risk: Low. The only hazard is under-clearing (a stale ALC surviving a deploy whose expression set is unchanged) — by construction those entries are exactly the still-deployed sources, so nothing stale survives. Blast radius = one method + one test.
Live-/run verification (after all tasks green)
Deterministic units + wiring guards above are necessary but not sufficient — unit-green ≠ wired (the F10b lesson). Close with the docker-dev rig (login disabled; do it yourself, no user sign-in):
- Rebuild both
central-1andcentral-2images (:9200round-robins them) and bringdocker-dev/docker-compose.ymlup. - On
/uns(AdminUIhttp://localhost:9200), author a VirtualTag on a rig equipment with a script that succeeds against live deps (e.g.return (double)ctx.GetTag("<dep>").Value * 2;), deploy (POST :9200/api/deployments,X-Api-Key), and confirm via Client.CLI:dotnet run --project src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI -- read -u opc.tcp://localhost:4840 -n "<folder-scoped NodeId>"→ value present, StatusCode Good. - Edit the script to a guaranteed runtime throw (e.g.
int z = 0; return 1 / z;), redeploy, wait one dependency change → Client.CLIreadagain → StatusCode Bad (0x80000000), value = last-known. This exercises the exact S13 headline path (edited-broken script ⇒ respawned child ⇒ inputs-ready gate ⇒ Bad publish) end to end through the SDK node. - Fix the script back, redeploy →
readreturns Good with a fresh value (recovery + dedup-bypass live). - P7 spot-check: redeploy the identical config; central logs show no recompile burst (and the S12 stress guard covers the race in CI).
Task breakdown
Branch: fix/archreview-r2-03-vt-failure-quality off master@f6eaa267. Tasks are ≤5 min each;
TDD-ordered (repro/red test precedes each implementation). Test commands run from the repo root.
R2-03-T1 — S13 stale-Good repro test (RED — must fail on current code)
Classification: test-first repro (deterministic unit, host-level wiring shape)
Estimated implement time: 5 min
Parallelizable with: T7 (different test file); everything else depends on this landing red first.
Files: tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs
- Add a
FailAfterFirstSuccessEvaluator : IVirtualTagEvaluatorprivate stub (call 1 →Ok(42.0), thereafter →Failure("boom"); interlocked counter). - Add test
Failing_script_after_success_degrades_node_quality_to_Bad: host with publish + mux probes and the stub; apply one plan (deps["a"]); capture the child viamux.ExpectMsg<DependencyMuxActor.RegisterInterest>()+mux.LastSender; tell the childDependencyValueChanged("a", 1, ts1)→ publish probe expectsAttributeValueUpdatewithQuality == OpcUaQuality.Good,Value == 42.0; tellDependencyValueChanged("a", 2, ts2)→ expect a secondAttributeValueUpdatewithQuality == OpcUaQuality.BadandValue == 42.0(last-known) andTimestampUtc == ts2. dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagHostActorTests.Failing_script_after_success_degrades_node_quality_to_Bad"→ expected: FAIL (timeout on the second ExpectMsg — the stale-Good bug, verbatim).
Commit: test(runtime): red repro — VT script failure after success leaves node stale-Good (02/S13)
R2-03-T2 — EvaluationResult.Quality + bridge pass-through + historize parity
Classification: implementation (additive message field + bridge)
Estimated implement time: 5 min
Parallelizable with: T7/T8 (disjoint files). Blocked by T1 (repro must be red first).
Files: src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs (record only),
src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs,
tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs
- RED: add bridge tests
Bad_quality_EvaluationResult_is_bridged_as_Bad(tell the host a syntheticEvaluationResult(..., Quality: OpcUaQuality.Bad)→ publish probe'sAttributeValueUpdate.Qualityis Bad) andHistorized_Bad_result_is_recorded_with_BadInternalError_status(PlanH historize:true; Bad result →CapturingHistoryWritersnapshotStatusCode == 0x80020000u). These require the record change to compile — so within this task: addOpcUaQuality Quality = OpcUaQuality.GoodtoEvaluationResult(+using ...Commons.OpcUa;) FIRST, then run the two tests:dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagHostActorTests.Bad_quality_EvaluationResult_is_bridged_as_Bad|FullyQualifiedName~VirtualTagHostActorTests.Historized_Bad_result_is_recorded_with_BadInternalError_status"→ expected: FAIL (bridge still hard-codes Good /0u). - GREEN:
OnResult— passresult.QualityintoAttributeValueUpdate; historize statusresult.Quality == OpcUaQuality.Good ? 0u : 0x80020000u /* BadInternalError — dormant-engine parity */; fix the stale comments at:179-188. Re-run the filter → PASS. Also run the full host-actor class (--filter "FullyQualifiedName~VirtualTagHostActorTests") — all pre-existing tests (Good-path bridging, historize Good0u) must stay green, proving the default keeps byte-parity.
Commit: feat(runtime): EvaluationResult carries OpcUaQuality; VT bridge publishes it + historizes BadInternalError on Bad (02/S13)
R2-03-T3 — VirtualTagActor failure→Bad transition (turns T1 green)
Classification: implementation (actor behavior)
Estimated implement time: 5 min
Parallelizable with: T7/T8. Blocked by T2.
Files: src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs,
tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagActorTests.cs
- Implement per design §1(b)/(c)/(d):
_lastPublishedBadfield,AllDependenciesArrived(),OnEvaluationFailed(...)shared by the throw-catch and!Successbranch (metric + script-log preserved per-failure; Bad publish once per transition, gated on inputs-ready, carrying_hasLastValue ? _lastValue : null+msg.TimestampUtc); success path resets_lastPublishedBadand bypasses the value-dedup when recovering; class-summary xmldoc updated. - Update
Evaluator_failure_publishes_ScriptLogEntry_warning: replace the trailingparent.ExpectNoMsg(...)withparent.ExpectMsg<VirtualTagActor.EvaluationResult>().Quality.ShouldBe(OpcUaQuality.Bad)— the deliberate behavioral inversion (emptydependencyRefs⇒ inputs-ready gate vacuously true). dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagHostActorTests.Failing_script_after_success_degrades_node_quality_to_Bad"→ expected: PASS (T1 repro green).dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagActorTests"→ expected: PASS (incl. the updated failure test).
Commit: fix(runtime): VT script failure/timeout degrades node quality to Bad once per transition (02/S13)
R2-03-T4 — S13 transition-semantics unit guards
Classification: tests (deterministic units pinning the state machine)
Estimated implement time: 5 min
Parallelizable with: T5, T7/T8. Blocked by T3.
Files: tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagActorTests.cs
Add (scriptable stub evaluator with a queue of results):
Second_consecutive_failure_does_not_publish_a_second_Bad— fail, fail → exactly one BadEvaluationResultat the parent (ExpectMsgthenExpectNoMsg).Recovery_to_the_same_value_republishes_Good_after_Bad— Ok(42), fail, Ok(42) → three parent messages; the third isQuality == Good,Value == 42(dedup bypass — this is the guard against the node staying Bad forever after a same-value recovery).Evaluator_throw_also_degrades_quality— evaluator that throws (notFailure) → BadEvaluationResultpublished (the:115-121path).dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagActorTests"→ expected: PASS (all should pass immediately if T3 is correct; any red here is a T3 bug, fix in place).
Commit: test(runtime): pin VT Bad-quality transition semantics — once-per-transition, same-value recovery, throw path (02/S13)
R2-03-T5 — S13 cold-start suppression guard (inputs-ready gate)
Classification: tests (deterministic unit pinning the gate)
Estimated implement time: 5 min
Parallelizable with: T4, T7/T8. Blocked by T3.
Files: tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagActorTests.cs
Cold_start_failure_with_missing_dependency_publishes_no_Bad— actor withdependencyRefs: ["a","b"], always-failing evaluator; sendDependencyValueChanged("a", ...)only →parent.ExpectNoMsg(node stays at the materialiser'sBadWaitingForInitialData; no false Bad flash during deploy/respawn warm-up).Failure_after_all_dependencies_arrived_publishes_Bad— same actor; send"a"then"b"→ the second change (all refs now seen) publishes exactly one BadEvaluationResult.dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagActorTests.Cold_start_failure_with_missing_dependency_publishes_no_Bad|FullyQualifiedName~VirtualTagActorTests.Failure_after_all_dependencies_arrived_publishes_Bad"→ expected: PASS.
Commit: test(runtime): pin VT inputs-ready gate — no Bad flash before all dependencies arrive (02/S13)
R2-03-T6 — S12 sequential clear-then-recompile guard (RED-able)
Classification: test-first (deterministic unit)
Estimated implement time: 3 min
Parallelizable with: T1-T5 (different project/file).
Files: tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs
- Add
Evaluate_after_ClearCompiledScripts_recompiles_and_succeeds: evaluate a compiled expression,ClearCompiledScripts(), evaluate the same source again → bothSuccess,CompiledCacheCount == 1after the second call. (Green on current code — this pins the sequential contract the T8 retry loop must not regress; the restructure moves the fetch inside a loop.) dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~RoslynVirtualTagEvaluatorTests.Evaluate_after_ClearCompiledScripts_recompiles_and_succeeds"→ expected: PASS (pre-fix baseline pin).
Commit: test(host): pin sequential clear-then-recompile contract of RoslynVirtualTagEvaluator (02/S12 baseline)
R2-03-T7 — S12 concurrent clear-vs-evaluate stress guard (RED — fails on current code)
Classification: test-first repro (concurrency stress; probabilistic-per-iteration, reliable at volume)
Estimated implement time: 5 min
Parallelizable with: T1-T6.
Files: tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/RoslynVirtualTagEvaluatorTests.cs
- Add
async Task Evaluate_racing_ClearCompiledScripts_never_fails_with_disposed: one evaluator (generous 5 s timeout so the budget never fires);Task.Runloop A doing 300 sequentialEvaluate("vt-race", "return (int)ctx.GetTag(\"a\").Value + 1;", …)calls collecting results; concurrent loop B callingClearCompiledScripts()+Thread.Yield()until A completes; whole test awaited withWaitAsync(TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken)(regression fails, never hangs — the U2 test's pattern). Assert every resultSuccessshouldBeTrue withresult.Reasonas the message (surfaces "script threw: Cannot access a disposed object" on red). dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~RoslynVirtualTagEvaluatorTests.Evaluate_racing_ClearCompiledScripts_never_fails_with_disposed"→ expected: FAIL on current code (disposed-reason failures observed within 300 racing iterations; the window is GetOrCompile-return → Task.Run-hopped disposed guard). If a run happens to pass, bump iterations — do not weaken the assertion.
Commit: test(host): red repro — apply-boundary cache clear races in-flight VT evaluation into ObjectDisposedException (02/S12)
R2-03-T8 — S12 retry-once on ObjectDisposedException (turns T7 green)
Classification: implementation (adapter hardening)
Estimated implement time: 5 min
Parallelizable with: T1-T5. Blocked by T6 + T7.
Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/Engines/RoslynVirtualTagEvaluator.cs
- Restructure the compiled path per design §2:
for (var attempt = 0; ; attempt++)around fetch (GetOrCompile— existing catch chain preserved) + run (TimedScriptEvaluatorblock), addingcatch (ObjectDisposedException) when (!_disposed && attempt == 0) { continue; }before the generic catch, andcatch (ObjectDisposedException)→Failure("evaluator disposed during evaluation")for the shutdown / double-clear case. Update the class xmldoc (retry-safe vs apply-boundary clear; one-generation stale-recompile residual documented). dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~RoslynVirtualTagEvaluatorTests"→ expected: PASS (T7 stress green; T6 sequential pin green; all pre-existing U2/U3/passthrough tests green — proves the loop restructure didn't disturb timeout, cache, or fast-path semantics).
Commit: fix(host): retry VT evaluation once on ObjectDisposedException from the apply-boundary cache clear (02/S12)
R2-03-T9 — P7 conditional-clear wiring guard (RED — fails on current code)
Classification: test-first (rewrites an existing wiring guard — deliberate behavior change)
Estimated implement time: 5 min
Parallelizable with: T7/T8. Blocked by T3 (same test file churn as T1-T5; serialize after S13 lands).
Files: tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs
- Rewrite
ApplyVirtualTags_clears_the_evaluator_compile_cache_each_generation→ApplyVirtualTags_clears_the_compile_cache_only_when_the_expression_set_changes(keep theCacheOwningStubEvaluatorspy; keep the "the host actually calls it" wiring-assertion role — update the xmldoc to say the clear is now gated, citing 02/P7): apply plan A (ClearCount == 1) → re-apply value-identical plan A (ClearCount == 1) → apply plan A with an editedExpression(ClearCount == 2) → apply two vtags sharing one expression, then remove one of them (expression set unchanged ⇒ count unchanged). dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagHostActorTests.ApplyVirtualTags_clears_the_compile_cache_only_when_the_expression_set_changes"→ expected: FAIL on current code (identical-redeploy step observesClearCount == 2).
Commit: test(runtime): red — apply-boundary compile-cache clear must be gated on a changed expression set (02/P7)
R2-03-T10 — P7 expression-set gate (turns T9 green)
Classification: implementation (host-actor apply path)
Estimated implement time: 3 min
Parallelizable with: — (same method as T2's file; do after T9). Blocked by T9.
Files: src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs
- Replace
OnApply's unconditional clear (:88-92) with the guarded block from design §3 (HashSet.SetEqualsovermsg.Plansvs_planByVtag.Valuesexpressions; ordinal comparer), with a comment covering:_planByVtagis the previous generation at this point; rename/Historize-only changes don't clear (cache is source-keyed); first-apply-after-restart clears by construction; the S12 retry — not this gate — is the in-flight-race fix. dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests --filter "FullyQualifiedName~VirtualTagHostActorTests"→ expected: PASS (T9 guard green + every other host-actor test green).
Commit: perf(runtime): gate the VT apply-boundary compile-cache clear on a changed expression set (02/P7)
R2-03-T11 — Full-suite sweep + STATUS/report bookkeeping
Classification: verification + docs
Estimated implement time: 5 min
Parallelizable with: —. Blocked by T1-T10.
Files: archreview/plans/STATUS.md, archreview/plans/R2-03-vt-failure-quality-plan.md.tasks.json
dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests→ expected: PASS (whole project).dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests --filter "FullyQualifiedName~RoslynVirtualTagEvaluatorTests"→ expected: PASS.dotnet build ZB.MOM.WW.OtOpcUa.slnx→ clean (analyzer OTOPCUA0001 + xmldoc warnings unchanged).- Record S13/S12/P7 as fixed-on-branch in
STATUS.md(Re-review 2026-07-12 section) + tick the tasks.json.
Commit: docs(archreview): record R2-03 (02/S13+S12+P7) fixed on branch; full VT suites green
R2-03-T12 — Live-/run gate on docker-dev (fail→Bad→recover, end to end)
Classification: live verification (docker-dev rig; no user sign-in needed — login disabled) Estimated implement time: 15-20 min (rig rebuild dominates) Parallelizable with: —. Blocked by T11. Files: none (verification only; findings, if any, loop back as new tasks)
Follow the "Live-/run verification" section above: rebuild both central images; author a working
VT → Client.CLI read Good; edit it to a runtime throw → redeploy → after one dependency change,
Client.CLI read shows StatusCode Bad with the last-known value (this is the edited-broken-script
respawn path — the S13 headline — through the real mux/child/bridge/publish-actor/SDK-node chain); fix
→ redeploy → read Good with a fresh value. Spot-check an identical redeploy produces no recompile
burst in central logs (P7). Do NOT merge on unit-green alone — this is the wiring bar.
Commit: none (verification). If the rig exposes a gap, open follow-up tasks before merge.
Task dependency summary
T1 (S13 red repro) ──► T2 (record+bridge) ──► T3 (actor fix, T1 green) ──► T4, T5 (guards)
└────────► T9 (P7 red) ──► T10 (P7 green)
T6 (S12 pin) ─┐
T7 (S12 red) ─┴──► T8 (S12 retry, T7 green)
T4, T5, T8, T10 ──► T11 (sweep + docs) ──► T12 (live /run gate)
Overall effort: Small (≈2-3 h implementation + the rig session), matching the OVERALL table's estimate for action item #3. Overall risk: Low-Medium, concentrated in S13's deliberate behavior change (nodes that used to freeze Good now go Bad — the point of the fix) and the one inverted legacy test expectation, both explicitly task-tracked.