fix(v3-batch4): reassert skips historian re-record + scripted-alarm redeploy recovery (reassert review M1/M2/LOW-3)

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
This commit is contained in:
Joseph Doherty
2026-07-16 14:33:05 -04:00
parent 21eb81c915
commit 51022c3952
3 changed files with 77 additions and 18 deletions
@@ -43,7 +43,12 @@ public sealed class VirtualTagActor : ReceiveActor
/// <see cref="OpcUaQuality.Bad"/> when the script failed/timed out (02/S13) — in which case
/// <paramref name="Value"/> is the last-known value (or null if none). Defaulting to Good keeps
/// every existing construction site source-compatible.</summary>
public sealed record EvaluationResult(string VirtualTagId, object? Value, DateTime TimestampUtc, CorrelationId Correlation, OpcUaQuality Quality = OpcUaQuality.Good);
/// <param name="IsReassert"><c>true</c> when this result is a deploy-time re-assertion of the last
/// value/quality (see <see cref="ReassertValue"/>) 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
/// <c>false</c> so every existing construction site stays source-compatible.</param>
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 <c>BadWaitingForInitialData</c> 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: <c>DriverHostActor</c> enqueues the address-space <c>RebuildAddressSpace</c> (which
/// re-materialises the node) to the single-threaded publish actor BEFORE the <c>ApplyVirtualTags</c> that
/// triggers this re-assert, so the re-publish lands on the freshly-materialised node.</summary>
/// <para>
/// The result is flagged <see cref="EvaluationResult.IsReassert"/> 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).
/// </para>
/// <para>
/// 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 <c>OpcUaPublishActor</c>. On a deploy
/// <c>DriverHostActor</c> enqueues <c>RebuildAddressSpace</c> (the reset) to that actor FIRST, then sends
/// <c>ApplyVirtualTags</c> to the VirtualTag HOST; the host tells this child <c>ReassertValue</c>, and
/// only then does this method Tell the host an <c>EvaluationResult</c> which the host bridges onward to
/// the publish actor. That multi-hop chain guarantees the re-publish arrives AFTER <c>RebuildAddressSpace</c>
/// in the publish actor's FIFO mailbox, so the materialise runs before the write.
/// </para></summary>
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)
@@ -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(
@@ -519,6 +519,41 @@ public sealed class VirtualTagHostActorTests : RuntimeActorTestBase
mux.ExpectNoMsg(TimeSpan.FromMilliseconds(200));
}
/// <summary>
/// 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.
/// </summary>
[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<DependencyMuxActor.RegisterInterest>();
var child = mux.LastSender;
// First real evaluation → Good publish + exactly one historian record.
child.Tell(new VirtualTagActor.DependencyValueChanged("a", 1, DateTime.UtcNow));
publish.ExpectMsg<OpcUaPublishActor.AttributeValueUpdate>();
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<OpcUaPublishActor.AttributeValueUpdate>();
// 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);
}
/// <summary>Evaluator that always returns <c>Ok(value)</c> — 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.</summary>