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

fix(opcuaserver): degraded applies re-diff instead of losing nodes; in-place UNS mirror updates; sever inverse edges

Browse Source 
C1 — a non-rebuild apply reporting failedNodes > 0 no longer advances _lastApplied.
Pre-delta-materialisation the full passes re-ensured every node on every apply, which
silently repaired any prior swallowed sink fault. The delta path removed that accidental
self-healing: a failed node sat inside _lastApplied, never appeared in a diff set again,
and EnsureVariable early-returns on the ones that landed — so it was lost until an
unrelated change forced a rebuild. The advance also moved to AFTER the materialise
passes, so a throwing pass leaves _lastApplied at the previous composition too. A
rebuild still advances even with failures (the ratchet is its resync path).

I2 — a UnsReferenceDelta whose identity is unchanged (same NodeId, EquipmentId and
BackingRawPath) is now applied in place via UpdateTagAttributes(Uns) instead of
remove+recreate, so the mirror's MonitoredItems survive an operator historizing or
reshaping the raw tag behind them. Remove+recreate stays for identity-changing deltas
(re-point, effective-name edit).

I3 — RemoveVariableNode severs the inverse half of every outgoing Organizes edge, the
variable-node counterpart of UnwireAlarmNotifiers. Rebuilds used to sweep these up;
surgical applies made rebuilds rare, so they accumulated on the raw node for the life of
the process.

I5 — added UNS reference variables are announced (their equipment folder is the parent,
already the Uns realm). Raw adds still are not; the gap comment now says so honestly
rather than calling it neutral and pre-existing.

I4 + M1 — corrected two comments that no longer described the code: the "re-point →
rebuild" claim in ApplyPureRemove, and the "N lock acquisitions" rationale for the delta
passes (Ensure* early-return on a dictionary probe before the lock).
This commit is contained in:
Joseph Doherty
2026-08-15 17:53:26 -04:00
parent 0dbbb5ac6b
commit d787502d54
6 changed files with 395 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs
+81 -17
View File
@@ -244,25 +244,59 @@ public sealed class AddressSpaceApplier
}
if (!ok) { allApplied = false; break; }
}
// v3 UNS reference variables — REMOVE the previous node (terminal Bad first, so an in-flight
// MonitoredItem observes the removal), and let the CURRENT node be (re)created by the
// materialise pass that runs after Apply returns.
// v3 UNS reference variables — TWO shapes, split on whether the delta moves the node's
// IDENTITY (2026-08-15 review I2).
//
// WHY remove+recreate rather than an in-place update: a UnsReferenceDelta is either a re-point
// (BackingRawPath moved — the node's Organizes edge must move with it, and the surgical sink
// surface has no RemoveReference) or a display-name-override edit (the effective name is part of
// the UNS NodeId, so the identity itself changes — an OPC UA NodeId is immutable). Remove+recreate
// covers both shapes with one mechanism. The cost is that ONE node's monitored items, against
// today's cost of a full rebuild killing every subscription server-wide.
// (a) IDENTITY UNCHANGED (same NodeId, same EquipmentId, same BackingRawPath): only the
// inherited Backing* / DataType / Writable attributes moved — the 875ec9fe repair shape,
// produced whenever a raw tag's historian or array shape is edited under a reference that
// was not itself touched. Nothing about the node's identity or its Organizes edge changes,
// so an in-place UpdateTagAttributes in the Uns realm says everything that needs saying
// AND the mirror's client MonitoredItems survive. Remove+recreate here would kill the
// subscriptions of every client watching a UNS signal because someone historized the raw
// tag behind it.
//
// WHERE the recreate happens: the publish actor's materialise passes, which run after Apply
// returns. On a NON-rebuild apply that pass is now narrowed to the plan's added references
// PLUS the CURRENT side of every delta removed here (surgical-deploys Task 6) — dropping the
// currents from that subset would leave a re-pointed / renamed reference removed and never
// recreated. Either way the ordering holds (remove here, recreate strictly after), and the
// recreated node inherits its shape / historian tagname from the UnsReferenceVariable record.
// (b) IDENTITY CHANGED — a re-point (BackingRawPath moved: the node's Organizes edge must move
// with it, and the surgical sink surface has no RemoveReference) or an effective-name edit
// (the effective name is part of the UNS NodeId, and an OPC UA NodeId is immutable). Remove
// the PREVIOUS node here (terminal Bad first, so an in-flight MonitoredItem observes the
// removal) and let the CURRENT node be created by the materialise pass after Apply returns.
// The cost is that ONE node's monitored items, against today's cost of a full rebuild
// killing every subscription server-wide.
//
// BOTH shapes leave the delta's Current in the publish actor's materialise subset, and that is
// deliberate: for (b) the subset IS the recreate (dropping it would leave the reference removed
// and never brought back), and for (a) it is a cheap idempotent no-op — EnsureVariable
// early-returns on the already-present node and AddReference is idempotent on the unchanged
// edge. Making the subset shape-aware would buy nothing and add a way for the two sides to
// disagree about which references were repaired.
foreach (var d in allApplied ? unsRefDeltas : Enumerable.Empty<AddressSpacePlan.UnsReferenceDelta>())
{
if (IsIdentityPreservingUnsRefDelta(d))
{
// Resolve writable + historian EXACTLY as MaterialiseUnsReferences does when it creates
// the node: arrays are forced read-only, and a blank override falls back to the backing
// RawPath (which IS the backing raw tag's NodeId).
var writable = d.Current.Writable && !d.Current.BackingIsArray;
var historian = d.Current.BackingIsHistorized
? (string.IsNullOrWhiteSpace(d.Current.BackingHistorianTagname) ? d.Current.BackingRawPath : d.Current.BackingHistorianTagname)
: null;
bool updated;
try
{
updated = surgical.UpdateTagAttributes(
d.Current.NodeId, writable, historian, d.Current.DataType,
d.Current.BackingIsArray, d.Current.BackingArrayLength, AddressSpaceRealm.Uns);
}
catch (Exception ex)
{
_logger.LogError(ex, "AddressSpaceApplier: surgical UpdateTagAttributes threw for UNS reference {Node}", d.Current.NodeId);
updated = false;
}
if (!updated) { allApplied = false; break; }
continue;
}
if (!SafeWriteValue(d.Previous.NodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Uns)) failedNodes++;
if (!SafeRemoveVariable(surgical, d.Previous.NodeId, AddressSpaceRealm.Uns)) { allApplied = false; break; }
}
@@ -371,8 +405,15 @@ public sealed class AddressSpaceApplier
}
// v3 Batch 4 — removed UNS reference variables (Uns realm, plain value nodes): terminal Bad then
// in-place remove. A backing-raw rename is a re-point (Changed, → rebuild), NOT a removal, so a UNS
// ref only reaches here on a genuine dereference (the equipment dropped the reference).
// in-place remove.
//
// WHAT REACHES THIS LOOP (the "→ rebuild" this comment used to claim is long dead — re-points are
// surgical now): only genuine DEREFERENCES, i.e. the equipment dropped the reference row, so the node
// is gone for good. A CHANGED reference never lands in RemovedUnsReferenceVariables at all — the
// planner reports it as a UnsReferenceDelta, which Apply routes above either to an in-place
// UpdateTagAttributes (identity unchanged) or to its own remove-the-previous + recreate-the-current
// pair (identity changed). That second shape does remove a node, but it removes it from Apply's delta
// loop, not from here.
foreach (var v in plan.RemovedUnsReferenceVariables)
{
SafeWriteValue(v.NodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Uns);
@@ -404,6 +445,22 @@ public sealed class AddressSpaceApplier
return true;
}
/// <summary>
/// 2026-08-15 review I2 — true when a <see cref="AddressSpacePlan.UnsReferenceDelta"/> can be applied
/// as an IN-PLACE attribute update rather than a remove+recreate: the node's identity is untouched.
/// Three fields make up that identity — the <c>NodeId</c> (an OPC UA NodeId is immutable, so a moved
/// effective name is a different node), the <c>EquipmentId</c> (the node's PARENT, which
/// <c>UpdateTagAttributes</c> cannot re-hang), and the <c>BackingRawPath</c> (the target of the node's
/// <c>Organizes</c> edge, which the surgical sink surface has no way to move — there is no
/// RemoveReference). Everything else on the record is a plain attribute the in-place update carries.
/// </summary>
/// <param name="d">The UNS reference delta to classify.</param>
/// <returns><c>true</c> when the delta is attribute-only and may be applied in place.</returns>
private static bool IsIdentityPreservingUnsRefDelta(AddressSpacePlan.UnsReferenceDelta d) =>
string.Equals(d.Previous.NodeId, d.Current.NodeId, StringComparison.Ordinal) &&
string.Equals(d.Previous.EquipmentId, d.Current.EquipmentId, StringComparison.Ordinal) &&
string.Equals(d.Previous.BackingRawPath, d.Current.BackingRawPath, StringComparison.Ordinal);
/// <summary>Remove a value-variable node in place, treating a throw as a false (⇒ rebuild fallback).</summary>
/// <returns><c>true</c> when the node was removed; <c>false</c> when unknown or the sink threw.</returns>
private bool SafeRemoveVariable(ISurgicalAddressSpaceSink surgical, string nodeId, AddressSpaceRealm realm)
@@ -463,6 +520,13 @@ public sealed class AddressSpaceApplier
foreach (var t in plan.AddedEquipmentTags) ids.Add(MaterialiseParent(t.EquipmentId, t.FolderPath));
foreach (var v in plan.AddedEquipmentVirtualTags) ids.Add(MaterialiseParent(v.EquipmentId, v.FolderPath));
foreach (var a in plan.AddedAlarms) ids.Add(a.EquipmentId);
// v3 Batch 4 / review I5 — an added UNS reference variable is materialised directly under its
// equipment folder (MaterialiseUnsReferences passes EquipmentId as the parent), and the equipment
// folder is a UNS-realm node — the realm AnnounceAddedNodes hardcodes. So it announces correctly with
// no realm plumbing. (Added RAW containers/tags are still NOT announced: their parents are Raw-realm
// nodes, which needs a realm-aware RaiseNodesAddedModelChange call — see the gap note in
// OpcUaPublishActor.HandleRebuild.)
foreach (var v in plan.AddedUnsReferenceVariables) ids.Add(v.EquipmentId);
foreach (var eq in plan.AddedEquipment)
ids.Add(string.IsNullOrWhiteSpace(eq.UnsLineId) ? eq.EquipmentId : eq.UnsLineId);
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+39
View File
@@ -2618,6 +2618,18 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// caller (AddressSpaceApplier) falls back to a full rebuild (resync). Mirrors the per-node
// teardown inside RebuildAddressSpace, scoped to this one id.
if (!_variables.TryRemove(key, out var variable)) return false;
// 2026-08-15 review I3: sever the INVERSE half of every cross-tree Organizes edge this node holds
// BEFORE detaching it — the same hazard (and the same fix shape) as UnwireAlarmNotifiers on a
// condition removal. AddReference wires the edge bidirectionally: forward on the UNS reference
// variable, inverse (OrganizedBy) on the backing RAW node. The raw node SURVIVES this scoped
// remove, so without this it keeps a dangling inverse reference to a node that no longer exists —
// browsable, unresolvable, and permanent.
//
// Why it matters more now than it used to: a full RebuildAddressSpace clears every node and every
// reference with it, so these edges were swept up on the next structural deploy. Surgical applies
// made rebuilds rare, so the dangling edges now ACCUMULATE on the raw node for the life of the
// process — one per re-point or effective-name edit of any reference that ever pointed at it.
SeverOutgoingReferences(variable);
variable.Parent?.RemoveChild(variable);
PredefinedNodes?.Remove(variable.NodeId);
// Drop the historized-tagname registration alongside the variable it maps (Phase C parity).
@@ -2628,6 +2640,33 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
return true;
}
/// <summary>
/// 2026-08-15 review I3 — remove the INVERSE half, held on the far node, of every FORWARD reference
/// <paramref name="node"/> carries to another node this manager owns. The exact reverse of the
/// bidirectional wiring in <see cref="AddReference"/>, and the variable-node counterpart of
/// <see cref="UnwireAlarmNotifiers"/>. MUST be called under <c>Lock</c>.
/// <para>Only FORWARD references are walked. A node's link to its own parent manifests as an INVERSE
/// reference (the parent owns the forward half through <c>AddChild</c>), and the parent's side of that
/// is severed by <c>Parent.RemoveChild</c> — walking inverse references too would try to unwire the
/// parent link a second time. A forward reference whose target is not one of this manager's nodes is
/// skipped: there is nothing here to mutate, and the target's own owner is responsible for it.</para>
/// </summary>
/// <param name="node">The node being removed, whose outgoing edges' inverse halves must be severed.</param>
private void SeverOutgoingReferences(NodeState node)
{
var references = new List<IReference>();
node.GetReferences(SystemContext, references);
foreach (var r in references)
{
if (r.IsInverse || r.TargetId is null || r.TargetId.IsAbsolute) continue;
var targetNodeId = (NodeId)r.TargetId;
if (NodeId.IsNull(targetNodeId)) continue;
if (ResolveNodeState(MapKey(targetNodeId)) is not { } target) continue;
if (!target.RemoveReference(r.ReferenceTypeId, isInverse: true, node.NodeId)) continue;
target.ClearChangeMasks(SystemContext, includeChildren: false);
}
}
/// <inheritdoc cref="ISurgicalAddressSpaceSink.RemoveAlarmConditionNode"/>
public bool RemoveAlarmConditionNode(string alarmNodeId, AddressSpaceRealm realm)
{