From 51022c3952aa8e51dbdf2601869389368aaf0138 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 16 Jul 2026 14:33:05 -0400 Subject: [PATCH] fix(v3-batch4): reassert skips historian re-record + scripted-alarm redeploy recovery (reassert review M1/M2/LOW-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1 (MEDIUM): the VirtualTag re-assert re-published a stale last value with a fresh deploy-time timestamp, and VirtualTagHostActor.OnResult recorded it to the IHistoryWriter for Historize=true plans — every deploy would append an artificial historian sample (BadInternalError if the last state was Bad) that never corresponded to a real evaluation. Inert today (NullHistoryWriter) but a data-quality bug once a VT history sink binds. Fix: EvaluationResult carries an IsReassert flag (default false), set true in VirtualTagActor.OnReassertValue; OnResult still PUBLISHES the AttributeValueUpdate (to repair the reset node) but SKIPS _history.Record when IsReassert. Regression test VirtualTagHostActorTests.Reassert_of_a_historized_vtag_publishes_but_does_not_re_record_to_the_historian (fails before — count=2 — passes after: count stays 1). LOW-3: corrected the ordering comments in VirtualTagActor.OnReassertValue and VirtualTagHostActor.OnApply. ApplyVirtualTags goes to the VirtualTag HOST, not the publish actor; the ordering holds because the re-assert reaches the publish actor via a multi-hop chain (host -> child ReassertValue -> child -> parent EvaluationResult -> OnResult -> publish actor) and thus lands AFTER RebuildAddressSpace in the shared publish actor's FIFO mailbox. M2 (CONFIRMED REAL — reported as scoped follow-up, not fixed here): the same redeploy-reset race is latent for Part 9 scripted-alarm condition nodes. A full-rebuild deploy clears + re-materialises them fresh (OtOpcUaNodeManager.RebuildAddressSpace clears _alarmConditions @2160; MaterialiseAlarmCondition recreates normal state), but the engine reload does NOT re-emit an unchanged-active condition: ScriptedAlarmEngine.LoadAsync -> EvaluatePredicateToStateAsync (@546-552) computes ApplyPredicate(seed, true) where seed is the persisted state from the DB-backed EfAlarmConditionStateStore, yielding EmissionKind.None, which ScriptedAlarmHostActor.OnEngineEmission (@292) filters. Net: an active alarm with static dependencies under-reports until its next real transition on a full-rebuild deploy. The fix is larger/riskier than the VT case (touches the Core engine emission contract and must publish ONLY the OPC UA node state, NOT the alerts topic, to avoid duplicate AVEVA history rows — the alarm analogue of M1), so per review guidance it is deferred as a scoped follow-up rather than forced into this commit. Claude-Session: https://claude.ai/code/session_01LVneM3eh1UtJxEisFXgmox --- .../VirtualTags/VirtualTagActor.cs | 26 +++++++++++--- .../VirtualTags/VirtualTagHostActor.cs | 34 +++++++++++------- .../VirtualTags/VirtualTagHostActorTests.cs | 35 +++++++++++++++++++ 3 files changed, 77 insertions(+), 18 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs index 6e29f09e..b9d6e513 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs @@ -43,7 +43,12 @@ public sealed class VirtualTagActor : ReceiveActor /// when the script failed/timed out (02/S13) — in which case /// is the last-known value (or null if none). Defaulting to Good keeps /// every existing construction site source-compatible. - public sealed record EvaluationResult(string VirtualTagId, object? Value, DateTime TimestampUtc, CorrelationId Correlation, OpcUaQuality Quality = OpcUaQuality.Good); + /// true when this result is a deploy-time re-assertion of the last + /// value/quality (see ) rather than a fresh evaluation. The host still + /// publishes it (to repair the reset OPC UA node) but must NOT re-record it to the historian — it is a + /// stale value stamped with a fresh deploy-time timestamp, never a real evaluation sample. Defaults to + /// false so every existing construction site stays source-compatible. + public sealed record EvaluationResult(string VirtualTagId, object? Value, DateTime TimestampUtc, CorrelationId Correlation, OpcUaQuality Quality = OpcUaQuality.Good, bool IsReassert = false); private readonly string _virtualTagId; private readonly string _scriptId; @@ -134,15 +139,26 @@ public sealed class VirtualTagActor : ReceiveActor /// deploy that re-materialised our published UNS node to BadWaitingForInitialData recovers even when /// the recomputed value is unchanged (permanent Bad otherwise, with a static dependency). No-op until the /// child has emitted at least once — the first dependency arrival publishes the initial value normally. - /// Ordering is safe: DriverHostActor enqueues the address-space RebuildAddressSpace (which - /// re-materialises the node) to the single-threaded publish actor BEFORE the ApplyVirtualTags that - /// triggers this re-assert, so the re-publish lands on the freshly-materialised node. + /// + /// The result is flagged so the host publishes it (repairing + /// the reset node) but does NOT re-record it to the historian — it is a stale value stamped with a + /// fresh deploy-time timestamp, not a real evaluation sample (M1). + /// + /// + /// Ordering (why the re-publish lands on the freshly-materialised node, not the pre-reset one): the + /// reset and this re-publish converge on the SAME single-threaded OpcUaPublishActor. On a deploy + /// DriverHostActor enqueues RebuildAddressSpace (the reset) to that actor FIRST, then sends + /// ApplyVirtualTags to the VirtualTag HOST; the host tells this child ReassertValue, and + /// only then does this method Tell the host an EvaluationResult which the host bridges onward to + /// the publish actor. That multi-hop chain guarantees the re-publish arrives AFTER RebuildAddressSpace + /// in the publish actor's FIFO mailbox, so the materialise runs before the write. + /// private void OnReassertValue() { if (!_hasLastValue && !_lastPublishedBad) return; var quality = _lastPublishedBad ? OpcUaQuality.Bad : OpcUaQuality.Good; Context.Parent.Tell(new EvaluationResult( - _virtualTagId, _lastValue, DateTime.UtcNow, CorrelationId.NewId(), quality)); + _virtualTagId, _lastValue, DateTime.UtcNow, CorrelationId.NewId(), quality, IsReassert: true)); } private void OnDependencyChanged(DependencyValueChanged msg) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs index 7326b74d..50592064 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs @@ -182,13 +182,17 @@ public sealed class VirtualTagHostActor : ReceiveActor } // Re-assert last values on surviving (not just-spawned) children. Every apply coincides with a - // deploy, and DriverHostActor enqueues the address-space RebuildAddressSpace — which re-materialises - // each VirtualTag's UNS node to BadWaitingForInitialData — to the single-threaded publish actor - // BEFORE this ApplyVirtualTags. A surviving child keeps its value-dedup state, so an unchanged - // recompute is suppressed and its freshly-reset node would stay Bad forever (permanent with a static - // dependency). Telling each survivor to ReassertValue re-publishes its last state through the same - // publish actor AFTER the materialise, so the node recovers. Fresh children are skipped (nothing to - // re-assert; their first dependency arrival publishes normally). + // deploy, and on a deploy DriverHostActor enqueues RebuildAddressSpace — which re-materialises each + // VirtualTag's UNS node to BadWaitingForInitialData — to the single-threaded OpcUaPublishActor FIRST, + // then sends ApplyVirtualTags to THIS host. A surviving child keeps its value-dedup state, so an + // unchanged recompute is suppressed and its freshly-reset node would stay Bad forever (permanent with + // a static dependency). Telling each survivor to ReassertValue makes it re-publish its last state; that + // re-publish reaches the publish actor via a multi-hop chain (host → child ReassertValue → child → + // parent EvaluationResult → OnResult → publish actor), so its AttributeValueUpdate lands AFTER + // RebuildAddressSpace in the publish actor's FIFO mailbox — the materialise runs before the write, and + // the node recovers. (The ordering does NOT come from ApplyVirtualTags itself reaching the publish + // actor — it doesn't; it goes to this host.) Fresh children are skipped (nothing to re-assert; their + // first dependency arrival publishes normally). foreach (var (vtagId, child) in _children) { if (newlySpawned.Contains(vtagId)) continue; @@ -215,12 +219,16 @@ public sealed class VirtualTagHostActor : ReceiveActor _publishActor.Tell(new OpcUaPublishActor.AttributeValueUpdate( nodeId, result.Value, result.Quality, result.TimestampUtc, AddressSpaceRealm.Uns)); - // Historize iff the plan opted in. Reuses _planByVtag (kept in lock-step with _children), so - // no parallel map. The historian path key is the SAME folder-scoped NodeId we just published - // to. For a computed value source == server, so both timestamps are the evaluation time. Bad - // results record BadInternalError (0x80020000u) — the dormant VirtualTagEngine's Bad-snapshot - // status — so the historian trend can distinguish a degraded sample from a Good one. - if (_planByVtag.TryGetValue(result.VirtualTagId, out var plan) && plan.Historize) + // Historize iff the plan opted in — but NEVER for a re-assert (M1). A re-assert re-publishes a + // stale last value stamped with a fresh deploy-time timestamp purely to repair the reset OPC UA + // node; recording it would append an artificial historian sample that never corresponded to a real + // evaluation (and, if the last state was Bad, a fabricated BadInternalError point). Reuses + // _planByVtag (kept in lock-step with _children), so no parallel map. The historian path key is the + // SAME folder-scoped NodeId we just published to. For a computed value source == server, so both + // timestamps are the evaluation time. Bad results record BadInternalError (0x80020000u) — the + // dormant VirtualTagEngine's Bad-snapshot status — so the historian trend can distinguish a degraded + // sample from a Good one. + if (!result.IsReassert && _planByVtag.TryGetValue(result.VirtualTagId, out var plan) && plan.Historize) { var status = result.Quality == OpcUaQuality.Good ? 0u : 0x80020000u /* BadInternalError — dormant-engine parity */; _history.Record(nodeId, new DataValueSnapshot( diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs index cf86cf86..d74f7276 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/VirtualTags/VirtualTagHostActorTests.cs @@ -519,6 +519,41 @@ public sealed class VirtualTagHostActorTests : RuntimeActorTestBase mux.ExpectNoMsg(TimeSpan.FromMilliseconds(200)); } + /// + /// M1 (reassert review): a re-assert must PUBLISH (to repair the reset OPC UA node) but must NOT + /// re-record to the historian. Otherwise every deploy appends an artificial historian sample carrying a + /// stale value with a fresh deploy-time timestamp — a data-quality bug the moment a VT history sink is + /// bound. Historize=true survivor: first real evaluation records exactly once; the redeploy's re-assert + /// publishes a second time but the historian count stays 1. + /// + [Fact] + public void Reassert_of_a_historized_vtag_publishes_but_does_not_re_record_to_the_historian() + { + var publish = CreateTestProbe(); + var mux = CreateTestProbe(); + var writer = new CapturingHistoryWriter(); + var host = Sys.ActorOf(VirtualTagHostActor.Props(publish.Ref, mux.Ref, new ConstOkEvaluator(7.0), writer)); + + var plan = new[] { PlanH("vt-1", "eq-1", "speed-rpm", historize: true) }; + host.Tell(new VirtualTagHostActor.ApplyVirtualTags(plan)); + + mux.ExpectMsg(); + var child = mux.LastSender; + + // First real evaluation → Good publish + exactly one historian record. + child.Tell(new VirtualTagActor.DependencyValueChanged("a", 1, DateTime.UtcNow)); + publish.ExpectMsg(); + AwaitAssert(() => writer.Calls.Count.ShouldBe(1)); + + // Redeploy (unchanged plan) → the survivor re-asserts: a SECOND publish repairs the reset node. + host.Tell(new VirtualTagHostActor.ApplyVirtualTags(plan)); + publish.ExpectMsg(); + + // The publish landing means the reassert's OnResult turn is complete (Record runs on the same turn, + // right after the Tell) — so a second Record would already be visible. It must NOT be: still 1. + writer.Calls.Count.ShouldBe(1); + } + /// Evaluator that always returns Ok(value) — a static-output VirtualTag whose recompute /// is dedup-suppressed after the first publish, so the re-assert path is the only way its reset node /// recovers.