dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(opcuaserver): UNS reference nodes repair on backing-tag historian/shape edits

Browse Source 
Adversarial review of 9f68f272 (classifier: non-alarm raw-tag deltas became
surgical) found stale UNS inheritance after a surgical raw edit. Editing a raw
tag's IsHistorized / HistorianTagname / IsArray / ArrayLength left every
referencing UNS node STALE: none of those fields were on UnsReferenceVariable,
so no coincident UnsReferenceDelta fired; the applier updated the raw node in
place; and EnsureVariable early-returns on an existing node. Consequences:
HistoryRead through the UNS NodeId served a different series than through the
raw NodeId (breaking the documented dual-registration invariant), a
de-historized tag kept serving history via UNS, and a scalar->array flip left
the UNS mirror scalar AND writable (bypassing the array read-only write gate).

UnsReferenceVariable now carries Backing{IsHistorized,HistorianTagname,IsArray,
ArrayLength}, copied from the backing tag's TagConfigIntent at compose time, so
record equality makes the reference Changed in the SAME plan. Both deltas are
non-rebuild kinds, so the existing machinery repairs the mirror surgically:
remove-previous in Apply, recreate in MaterialiseUnsReferences. That pass now
reads the four values from the RECORD (the single truth the planner diffed)
instead of a rawByPath back-lookup, preserving historian resolution exactly
(blank override => BackingRawPath, which is the backing tag's NodeId).

The artifact-decode seam needs no change: DeploymentArtifact.BuildRawAndUnsSubtrees
drives AddressSpaceComposer.Compose verbatim, which is what guarantees byte-parity.
The classifier needs no change either (a UnsReferenceDelta never forces Rebuild);
its comment now records why.

Also from the same review: a three-place-contract comment on RawTagPlan's
hand-written Equals naming its two downstream consumers, plus classifier pins for
the .Any(!eligible) quantifier, an ArrayLength-only delta, and an
alarm-content-only delta.
This commit is contained in:
Joseph Doherty
2026-08-15 17:11:03 -04:00
parent 9dbd5e96f7
commit 875ec9fe47
9 changed files with 416 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs
+19 -18
View File
@@ -828,35 +828,36 @@ public sealed class AddressSpaceApplier
/// parented under its equipment folder (the logical <see cref="UnsReferenceVariable.EquipmentId"/> node
/// <see cref="MaterialiseHierarchy"/> already created), THEN an <c>Organizes</c> reference
/// UNS→Raw to its backing raw node. The UNS node inherits DataType / writable / array shape / historian
/// tagname from its backing RAW tag (looked up by <see cref="UnsReferenceVariable.BackingRawPath"/>), so a
/// historized raw tag's UNS reference registers the SAME tagname (both NodeIds → one tagname → HistoryRead
/// works against either). The UNS node binds NO driver — the driver publish path fans values to it (WP3
/// runtime binding). Every sink call passes <see cref="AddressSpaceRealm.Uns"/> / the Organizes edge
/// passes both realms EXPLICITLY. Idempotent.
/// tagname from its backing RAW tag, so a historized raw tag's UNS reference registers the SAME tagname
/// (both NodeIds → one tagname → HistoryRead works against either). The UNS node binds NO driver — the
/// driver publish path fans values to it (WP3 runtime binding). Every sink call passes
/// <see cref="AddressSpaceRealm.Uns"/> / the Organizes edge passes both realms EXPLICITLY. Idempotent.
/// <para><b>The inherited values are read from the <see cref="UnsReferenceVariable"/> RECORD, not
/// re-looked-up from <see cref="AddressSpaceComposition.RawTags"/> by RawPath.</b> The record is the
/// single truth: the composer copies them off the backing tag's <c>TagConfigIntent</c> at compose time
/// (both compose seams, byte-parity), and having them ON the record is what makes a raw-side historian /
/// shape edit produce the coincident <c>UnsReferenceDelta</c> that drives this node's repair
/// (remove-previous in <c>Apply</c>, recreate here). A back-lookup would re-introduce the split the
/// 2026-08-15 review found: a record that says one thing and a materialised node built from another.</para>
/// </summary>
/// <param name="composition">The composition carrying the UNS reference variables + Raw tags (for inherited shape).</param>
/// <param name="composition">The composition carrying the UNS reference variables.</param>
/// <returns>The count of swallowed sink failures in this pass.</returns>
public int MaterialiseUnsReferences(AddressSpaceComposition composition)
{
ArgumentNullException.ThrowIfNull(composition);
if (composition.UnsReferenceVariables.Count == 0) return 0;
// Backing raw tag shape/historian by RawPath — a UNS reference inherits these so its node matches the
// raw node it mirrors (a historized raw tag registers the SAME tagname on the UNS node for HistoryRead).
var rawByPath = new Dictionary<string, RawTagPlan>(StringComparer.Ordinal);
foreach (var t in composition.RawTags) rawByPath[t.NodeId] = t;
var failed = 0;
foreach (var v in composition.UnsReferenceVariables)
{
var backing = rawByPath.GetValueOrDefault(v.BackingRawPath);
var isArray = backing?.IsArray ?? false;
uint? arrayLength = backing?.ArrayLength;
var isArray = v.BackingIsArray;
uint? arrayLength = v.BackingArrayLength;
// A UNS reference to a historized raw tag registers the SAME effective historian tagname (override
// else the backing RawPath) so HistoryRead resolves against the UNS NodeId too. The mux ref stays
// single (keyed by RawPath) — FeedHistorizedRefs emits raw refs only.
string? historianTagname = backing is { IsHistorized: true }
? (string.IsNullOrWhiteSpace(backing.HistorianTagname) ? backing.NodeId : backing.HistorianTagname)
// else the backing RawPath — BackingRawPath IS the backing raw tag's NodeId) so HistoryRead resolves
// against the UNS NodeId too. The mux ref stays single (keyed by RawPath) — FeedHistorizedRefs emits
// raw refs only.
string? historianTagname = v.BackingIsHistorized
? (string.IsNullOrWhiteSpace(v.BackingHistorianTagname) ? v.BackingRawPath : v.BackingHistorianTagname)
: null;
var writable = v.Writable && !isArray;
if (!SafeEnsureVariable(v.NodeId, v.EquipmentId, v.EffectiveName, v.DataType, writable, AddressSpaceRealm.Uns, historianTagname, isArray, arrayLength))
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceChangeClassifier.cs
+13
View File
@@ -74,6 +74,19 @@ public static class AddressSpaceChangeClassifier
// • ChangedUnsReferenceVariables ⇒ surgical remove-previous + recreate-current, applier-owned (a UNS
// reference variable is a mirror node binding no driver, so it carries no notifier wiring of its
// own — the alarm fan-out it participates in is re-derived from the raw tag's referencing set).
// No rebuild is forced for a UNS-reference delta in EITHER direction, and that is load-bearing for
// the repair path below.
// UNS-REFERENCE STALENESS, and why nothing here gates it (2026-08-15 review). Making non-alarm raw-tag
// deltas surgical meant a raw-side IsHistorized / HistorianTagname / IsArray / ArrayLength edit updated
// the raw node in place while every referencing UNS MIRROR node kept the old historian tagname + shape
// (EnsureVariable early-returns on an existing node) — wrong history series through the UNS NodeId, and
// a scalar→array flip left the mirror scalar AND writable. The fix is in the RECORD, not the classifier:
// UnsReferenceVariable now carries the backing tag's Backing{IsHistorized,HistorianTagname,IsArray,
// ArrayLength}, so the same edit makes the reference record unequal and the planner emits a COINCIDENT
// UnsReferenceDelta alongside the RawTagDelta. Both are non-rebuild kinds, so the plan stays
// AttributeOnly and the applier repairs the mirror surgically (remove-previous + materialise-recreate).
// Adding a new inherited field to UnsReferenceVariable therefore needs no change here — but OMITTING one
// silently reopens the staleness.
// Anything a future surgical raw-tag path lets through that DOES touch alarm notifiers must still
// re-wire/unwire them (WireAlarmNotifiers is a reconcile — pass the tag's COMPLETE current referencing
// set — plus an explicit unwire on removal), else a de-referenced equipment keeps receiving the events.
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceComposer.cs
+47 -1
View File
@@ -137,6 +137,17 @@ public sealed record RawContainerNode(
/// referencing equipment. <see cref="Realm"/> is always <see cref="AddressSpaceRealm.Raw"/>.
/// A null <see cref="HistorianTagname"/> means the historian tagname defaults to the RawPath (resolved
/// later, not here).
/// <para><b>ADDING A FIELD IS A FOUR-PLACE CONTRACT — the hand-written equality below does NOT close
/// over new members.</b> Because <see cref="ReferencingEquipmentPaths"/> forces a custom
/// <c>Equals</c>/<c>GetHashCode</c>, the compiler's positional equality is suppressed, so a field added
/// to the ctor and nowhere else silently never diffs (a redeploy that changes only it produces an EMPTY
/// plan). Update, in order: (1) the primary constructor, (2) <see cref="Equals(RawTagPlan)"/>,
/// (3) <see cref="GetHashCode"/>. Then reconsider two downstream consumers: (4a)
/// <c>AddressSpaceChangeClassifier.RawTagDeltaIsSurgicalEligible</c>'s <c>with</c>-equality whitelist —
/// a new field is default-closed (⇒ Rebuild) until it is listed, which is the safe direction but may be
/// the wrong one; and (4b) <see cref="UnsReferenceVariable"/>'s <c>Backing*</c> mirror fields — if the
/// new field is INHERITED by a UNS reference node at materialise time, it must be mirrored there too or
/// referencing UNS nodes go stale on a surgical raw edit (the exact 2026-08-15 review finding).</para>
/// </summary>
public sealed record RawTagPlan(
string TagId,
@@ -200,6 +211,27 @@ public sealed record RawTagPlan(
/// edit keeps the same id while the NodeId/effective-name changes; a backing-tag rename keeps the same id
/// + NodeId while <see cref="BackingRawPath"/> re-points). <see cref="Realm"/> is always
/// <see cref="AddressSpaceRealm.Uns"/>.
/// <para><b><see cref="BackingIsHistorized"/> / <see cref="BackingHistorianTagname"/> /
/// <see cref="BackingIsArray"/> / <see cref="BackingArrayLength"/> mirror the backing raw tag's
/// historian intent + array shape, and they are on THIS record for one reason: to make a raw-side
/// edit produce a COINCIDENT <c>UnsReferenceDelta</c>.</b> The 2026-08-15 surgical-deploys review
/// found that once non-alarm raw-tag deltas became surgical, editing a backing tag's
/// <c>IsHistorized</c> / <c>HistorianTagname</c> / <c>IsArray</c> / <c>ArrayLength</c> left every
/// referencing UNS node STALE: the applier updates the raw node in place, no UNS-reference delta
/// fired (none of those fields were on this record), and <c>EnsureVariable</c> early-returns on an
/// existing node — so HistoryRead via the UNS NodeId served a different series than via the raw
/// NodeId, a de-historized tag kept serving history through UNS, and a scalar→array flip left the
/// UNS mirror scalar AND writable (bypassing the array read-only write gate). Carrying the four
/// fields here makes record equality flag the reference as Changed in the same plan, and the
/// existing machinery repairs it: the applier removes the previous UNS node and
/// <see cref="AddressSpaceApplier.MaterialiseUnsReferences"/> recreates it from these values.
/// <see cref="DataType"/> / <see cref="Writable"/> were already inherited onto the record and are
/// covered by the same mechanism.</para>
/// <para>Equality is the COMPILER-GENERATED positional-record equality: every member is a
/// value/string type (no <c>IReadOnlyList</c> member, unlike <see cref="RawTagPlan"/>), so the four
/// new fields are compared by value with no hand-written <c>Equals</c>/<c>GetHashCode</c> needed. If
/// a collection-typed member is ever added here, custom equality becomes mandatory — see the
/// three-place contract comment on <see cref="RawTagPlan"/>.</para>
/// </summary>
public sealed record UnsReferenceVariable(
string UnsTagReferenceId,
@@ -209,6 +241,10 @@ public sealed record UnsReferenceVariable(
string BackingRawPath,
string DataType,
bool Writable,
bool BackingIsHistorized = false,
string? BackingHistorianTagname = null,
bool BackingIsArray = false,
uint? BackingArrayLength = null,
AddressSpaceRealm Realm = AddressSpaceRealm.Uns);
public sealed record UnsAreaProjection(string UnsAreaId, string DisplayName);
@@ -799,6 +835,12 @@ public static class AddressSpaceComposer
continue; // invalid effective-name segment — dropped (deploy gate rejects invalid names)
}
// The backing tag's historian intent + array shape ride the record so a raw-side edit to any of
// them produces a coincident UnsReferenceDelta that repairs this mirror node (see the record's
// doc-comment; 2026-08-15 surgical-deploys review). Parsed from the SAME TagConfigIntent the raw
// tag plan uses, so the two sides can never disagree.
var intent = TagConfigIntent.Parse(tag.TagConfig);
vars.Add(new UnsReferenceVariable(
UnsTagReferenceId: r.UnsTagReferenceId,
EquipmentId: r.EquipmentId,
@@ -806,7 +848,11 @@ public static class AddressSpaceComposer
EffectiveName: effectiveName,
BackingRawPath: rawPath,
DataType: tag.DataType,
Writable: tag.AccessLevel == TagAccessLevel.ReadWrite));
Writable: tag.AccessLevel == TagAccessLevel.ReadWrite,
BackingIsHistorized: intent.IsHistorized,
BackingHistorianTagname: intent.HistorianTagname,
BackingIsArray: intent.IsArray,
BackingArrayLength: intent.ArrayLength));
}
vars.Sort((x, y) => string.CompareOrdinal(x.NodeId, y.NodeId));