diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index ef3abb96..eee64688 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -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()) { + 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; } + /// + /// 2026-08-15 review I2 — true when a 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 NodeId (an OPC UA NodeId is immutable, so a moved + /// effective name is a different node), the EquipmentId (the node's PARENT, which + /// UpdateTagAttributes cannot re-hang), and the BackingRawPath (the target of the node's + /// Organizes 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. + /// + /// The UNS reference delta to classify. + /// true when the delta is attribute-only and may be applied in place. + 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); + /// Remove a value-variable node in place, treating a throw as a false (⇒ rebuild fallback). /// true when the node was removed; false when unknown or the sink threw. 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); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 5d7faab8..8c2883d0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -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; } + /// + /// 2026-08-15 review I3 — remove the INVERSE half, held on the far node, of every FORWARD reference + /// carries to another node this manager owns. The exact reverse of the + /// bidirectional wiring in , and the variable-node counterpart of + /// . MUST be called under Lock. + /// 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 AddChild), and the parent's side of that + /// is severed by Parent.RemoveChild — 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. + /// + /// The node being removed, whose outgoing edges' inverse halves must be severed. + private void SeverOutgoingReferences(NodeState node) + { + var references = new List(); + 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); + } + } + /// public bool RemoveAlarmConditionNode(string alarmNodeId, AddressSpaceRealm realm) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs index 6546a158..f0c9a193 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs @@ -496,8 +496,6 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers } var outcome = _applier.Apply(plan); - _lastApplied = composition; - _hasAppliedComposition = true; // Sum swallowed per-node materialise failures across every pass together with Apply's own // removal-pass tally (archreview 01/S-1). A degraded apply used to vanish into per-node @@ -510,6 +508,41 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers failedNodes += _applier.MaterialiseHierarchy(composition); failedNodes += MaterialiseNodes(plan, composition, fullComposition: outcome.RebuildCalled); + // 2026-08-15 surgical-deploys review C1 — record the composition as applied ONLY when the apply was + // clean, or when a rebuild ran. Two reasons this sits HERE (after the materialise passes) rather + // than immediately after Apply: + // + // • A pass that THROWS outright leaves _lastApplied at the previous composition (the outer catch + // below takes over), so the next deploy re-diffs against what is actually materialised. + // • A DEGRADED non-rebuild apply (failedNodes > 0, no rebuild) must not advance either. Before the + // delta materialisation the full passes re-ensured every node on every apply, which silently + // repaired any prior swallowed sink fault — an accidental self-healing mechanism. The delta path + // removed it: a node whose EnsureVariable failed is inside _lastApplied, never appears in a diff + // set again, and EnsureVariable early-returns on the ones that DID land — so the failed node is + // lost until some unrelated change happens to force a rebuild. Holding the composition back is + // the replacement: the next deploy re-diffs the same composition and re-ensures what failed. + // + // What the re-diff costs, stated honestly: it also re-emits the previous apply's REMOVES. A + // re-remove of an already-removed node returns false from the surgical sink, which trips the + // applier's one-way ratchet into a full rebuild — the correct resync for an apply that has already + // been logged at Error as degraded, and a bounded one (it happens once, then the address space is + // known-good again). + // + // A rebuild DOES advance even with failures: it already re-materialised the whole composition, and + // the ratchet is the resync path for anything it could not place. Holding it back would make every + // subsequent deploy re-diff the entire address space as added. + if (outcome.RebuildCalled || failedNodes == 0) + { + _lastApplied = composition; + _hasAppliedComposition = true; + } + else + { + _log.Error( + "OpcUaPublish: degraded non-rebuild apply (correlation={Correlation}, failedNodes={FailedNodes}) — NOT recording this composition as applied, so the next deploy re-diffs it and repairs the missing node(s)", + msg.Correlation, failedNodes); + } + // R2-07 T4b — on a NON-rebuild apply (PureAdd / AttributeOnly), the Materialise passes above // just created exactly the added nodes WITHOUT tearing anything down, so announce them with a // Part 3 GeneralModelChangeEvent(NodeAdded) per affected parent — model-aware clients then @@ -519,10 +552,15 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers // (subscriptions are dead anyway) — the guard skips it. AnnounceAddedNodes is Safe-wrapped // internally, so a faulting announce can never break the deploy. // - // KNOWN, PRE-EXISTING GAP (unchanged by the delta-materialise work above): ComputeAddAnnouncements - // only walks the equipment-namespace add sets, so added RAW containers/tags and added UNS - // reference variables are materialised but never announced. Model-aware clients pick them up on - // their next unprompted browse. + // KNOWN GAP, and NOT a neutral pre-existing one — the delta work made it matter. Before this + // branch, a plan carrying raw adds went through a full RebuildAddressSpace, so every client had to + // re-browse anyway and the missing announcement changed nothing observable. Now those adds are + // materialised surgically while existing subscriptions stay alive, so an unannounced add is + // genuinely INVISIBLE until a client happens to browse again. + // Added UNS reference variables ARE announced (their equipment folder is the parent, which is the + // realm AnnounceAddedNodes hardcodes). Added RAW containers/tags are still not: the announce is + // Uns-only, and a raw parent needs a realm-aware RaiseNodesAddedModelChange call. That is a real + // follow-up, not an accepted limitation. if (!outcome.RebuildCalled) _applier.AnnounceAddedNodes(plan); OtOpcUaTelemetry.OpcUaSinkWrite.Add(1, new KeyValuePair("kind", "rebuild")); @@ -554,13 +592,20 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers /// the caller always runs it in full, because parents must exist before any add. /// /// - /// Why the delta path exists. The passes are idempotent, but idempotent is not free: - /// every EnsureVariable / EnsureFolder / MaterialiseAlarmCondition is a sink call - /// that takes the node manager's lock and re-derives the node's identity before discovering there is - /// nothing to do. Running them over the whole composition made the cost of ANY apply proportional to - /// the size of the served address space rather than to the size of the change: adding one tag to a - /// 50k-node server was ~50k sink calls, on the actor's pinned publish thread, with every client's - /// publish blocked behind it. + /// Why the delta path exists. The passes are idempotent, but idempotent is not free. + /// EnsureFolder / EnsureVariable early-return on a ConcurrentDictionary hit + /// BEFORE taking the node manager's lock, so an already-standing node costs a dictionary probe rather + /// than a lock acquisition — but MaterialiseAlarmCondition DOES take the lock every time, and + /// the probes are still O(address space) per apply plus the composition walk that produces them. + /// Running the passes over the whole composition made the cost of ANY apply proportional to the size + /// of the served address space rather than to the size of the change: adding one tag to a 50k-node + /// server was ~50k sink calls on the actor's pinned publish thread, with every client's publish + /// blocked behind it. The delta path makes it O(change). + /// What the full pass was silently doing, and its replacement. Re-ensuring every node on + /// every apply also REPAIRED any node a previous apply had swallowed a sink fault on — accidental + /// self-healing nobody designed. The caller's degraded-apply no-advance rule (a non-rebuild apply + /// reporting failedNodes > 0 does not record its composition in _lastApplied, so the + /// next deploy re-diffs and re-ensures it) is the deliberate replacement for it. /// Why is the branch key rather than /// the classifier's kind: a surgical apply that hits a false/throw from the surgical sink falls /// back to a full RebuildAddressSpace() INSIDE Apply, which wipes every node. That plan diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs index ad370d03..1251f581 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs @@ -1401,6 +1401,31 @@ public sealed class AddressSpaceApplierTests applier.ComputeAddAnnouncements(noLine).ShouldBe(new[] { "eq-2" }); } + /// + /// 2026-08-15 review I5 — an added UNS reference variable announces its EQUIPMENT folder, which is the + /// parent MaterialiseUnsReferences hangs it under and a UNS-realm node (the realm + /// AnnounceAddedNodes hardcodes). Before the surgical-deploy branch a plan carrying adds went + /// through a full rebuild, so every client re-browsed regardless and the missing announcement was + /// invisible; now the add is surgical and an unannounced node stays unseen until some later browse. + /// Two references under one equipment dedup to a single announcement. + /// + [Fact] + public void ComputeAddAnnouncements_added_uns_reference_announces_its_equipment() + { + var applier = new AddressSpaceApplier(new RecordingSink(), NullLogger.Instance); + + var plan = EmptyPlan with + { + AddedUnsReferenceVariables = new[] + { + UnsRef("filling/line-1/station-1/speed", "speed"), + UnsRef("filling/line-1/station-1/temp", "temp"), + }, + }; + + applier.ComputeAddAnnouncements(plan).ShouldBe(new[] { "eq-1" }); + } + /// A plan with no additions yields no announcements. [Fact] public void ComputeAddAnnouncements_empty_add_plan_yields_none() @@ -1557,12 +1582,17 @@ public sealed class AddressSpaceApplierTests /// 2026-08-15 review — the combined plan a backing-tag HISTORIAN edit produces: a surgical raw-tag /// delta AND the coincident UNS-reference delta (the reference record carries the backing historian /// intent, so it goes unequal too). Both are non-rebuild kinds, so ONE apply must update the raw node - /// in place AND tear the stale UNS mirror down for the materialise pass to recreate — with NO rebuild. - /// Without the coincident delta the UNS mirror kept the OLD historian tagname and HistoryRead through - /// the UNS NodeId served a different series than through the raw NodeId. + /// in place AND repair the stale UNS mirror — with NO rebuild. Without the coincident delta the UNS + /// mirror kept the OLD historian tagname and HistoryRead through the UNS NodeId served a different + /// series than through the raw NodeId. + /// The mirror is repaired IN PLACE (review I2), not by remove+recreate. This delta's + /// IDENTITY is unchanged (same NodeId, same EquipmentId, same BackingRawPath) — only the inherited + /// Backing* attributes moved — so UpdateTagAttributes in the Uns realm says everything that + /// needs saying and the mirror's client MonitoredItems survive. Remove+recreate stays for the deltas + /// that genuinely change identity (a re-point or an effective-name edit). /// [Fact] - public void Backing_historian_edit_updates_raw_in_place_and_removes_the_stale_uns_mirror_without_rebuilding() + public void Backing_historian_edit_updates_both_the_raw_node_and_the_uns_mirror_in_place() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); @@ -1590,21 +1620,26 @@ public sealed class AddressSpaceApplierTests sink.RebuildCalls.ShouldBe(0); // Raw node: updated in place with the NEW tagname, in the Raw realm. - var call = sink.SurgicalCalls.ShouldHaveSingleItem(); - call.NodeId.ShouldBe("Plant/Modbus/dev1/speed"); - call.Historian.ShouldBe("WW.New"); - call.Realm.ShouldBe(AddressSpaceRealm.Raw); + var raw = sink.SurgicalCalls.Single(c => c.Realm == AddressSpaceRealm.Raw); + raw.NodeId.ShouldBe("Plant/Modbus/dev1/speed"); + raw.Historian.ShouldBe("WW.New"); - // UNS mirror: terminal Bad + in-place remove of the PREVIOUS node (the recreate rides the materialise - // pass after Apply returns, which reads the CURRENT record's Backing* values). - sink.ValueWrites.ShouldContain((unsBefore.NodeId, OpcUaStatus.Bad, AddressSpaceRealm.Uns)); - sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("var", unsBefore.NodeId, AddressSpaceRealm.Uns)); + // UNS mirror: the SAME in-place update, in the Uns realm, carrying the new inherited tagname. + var uns = sink.SurgicalCalls.Single(c => c.Realm == AddressSpaceRealm.Uns); + uns.NodeId.ShouldBe(unsAfter.NodeId); + uns.Historian.ShouldBe("WW.New"); + + // …and NOT torn down: no remove, and no terminal Bad on the mirror's subscribers. + sink.RemoveCalls.ShouldBeEmpty(); + sink.ValueWrites.ShouldNotContain((unsBefore.NodeId, OpcUaStatus.Bad, AddressSpaceRealm.Uns)); } /// The same combined-plan repair for a scalar→ARRAY flip on the backing tag: without it the UNS - /// mirror stays scalar AND writable, bypassing the array read-only write gate through the UNS NodeId. + /// mirror stays scalar AND writable, bypassing the array read-only write gate through the UNS NodeId. + /// Identity is unchanged, so the mirror is repaired in place too (review I2) — and the in-place call must + /// carry the array shape AND the forced read-only, exactly as a recreate would have. [Fact] - public void Backing_shape_flip_removes_the_stale_uns_mirror_without_rebuilding() + public void Backing_shape_flip_updates_the_uns_mirror_in_place_without_rebuilding() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); @@ -1624,8 +1659,16 @@ public sealed class AddressSpaceApplierTests outcome.RebuildCalled.ShouldBeFalse(); sink.RebuildCalls.ShouldBe(0); - sink.SurgicalCalls.ShouldHaveSingleItem().IsArray.ShouldBeTrue(); - sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("var", unsBefore.NodeId, AddressSpaceRealm.Uns)); + sink.SurgicalCalls.Single(c => c.Realm == AddressSpaceRealm.Raw).IsArray.ShouldBeTrue(); + + var uns = sink.SurgicalCalls.Single(c => c.Realm == AddressSpaceRealm.Uns); + uns.NodeId.ShouldBe(unsAfter.NodeId); + uns.IsArray.ShouldBeTrue(); + uns.ArrayLength.ShouldBe(8u); + // The reference row is authored Writable, but an array node is forced read-only — the same + // `Writable && !BackingIsArray` rule MaterialiseUnsReferences applies when it creates the node. + uns.Writable.ShouldBeFalse(); + sink.RemoveCalls.ShouldBeEmpty(); } /// The one-way rebuild ratchet holds for the new raw-tag path: a surgical update that reports the diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs index 699915a1..0e5ae264 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs @@ -190,6 +190,51 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable await host.DisposeAsync(); } + /// + /// 2026-08-15 review I3 — RemoveVariableNode must sever the INVERSE half of the cross-tree + /// Organizes edge it holds, not just drop its own node. AddReference wires the edge + /// bidirectionally (forward on the UNS reference variable, inverse on the backing RAW node), and the + /// raw node SURVIVES a scoped remove — so without this the raw node keeps a browsable, unresolvable + /// reference to a node that no longer exists. Rebuilds used to sweep these up; surgical applies made + /// rebuilds rare, so they now accumulate one per re-point for the life of the process. + /// + [Fact] + public async Task RemoveVariableNode_severs_the_inverse_Organizes_edge_on_the_raw_target() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var sink = new SdkAddressSpaceSink(nm); + + sink.EnsureVariable("MAIN/modbus/dev/grp/temp", parentFolderNodeId: null, displayName: "Temp", + dataType: "Float", writable: false, realm: AddressSpaceRealm.Raw); + sink.EnsureVariable("Area/Line/Eq/Temperature", parentFolderNodeId: null, displayName: "Temperature", + dataType: "Float", writable: false, realm: AddressSpaceRealm.Uns); + sink.AddReference("Area/Line/Eq/Temperature", AddressSpaceRealm.Uns, + "MAIN/modbus/dev/grp/temp", AddressSpaceRealm.Raw); + + var unsVar = nm.TryGetVariable("Area/Line/Eq/Temperature", AddressSpaceRealm.Uns); + unsVar.ShouldNotBeNull(); + // Precondition: the raw node really does hold the inverse half. + nm.GetNodeReferences("MAIN/modbus/dev/grp/temp", AddressSpaceRealm.Raw) + .Any(r => r.ReferenceTypeId == ReferenceTypeIds.Organizes && r.IsInverse && + r.TargetId == unsVar.NodeId).ShouldBeTrue(); + + nm.RemoveVariableNode("Area/Line/Eq/Temperature", AddressSpaceRealm.Uns).ShouldBeTrue(); + + // The raw node survives… + nm.TryGetVariable("MAIN/modbus/dev/grp/temp", AddressSpaceRealm.Raw).ShouldNotBeNull(); + // …carrying NO reference back to the node that just went away. + nm.GetNodeReferences("MAIN/modbus/dev/grp/temp", AddressSpaceRealm.Raw) + .Any(r => r.TargetId == unsVar.NodeId).ShouldBeFalse(); + } + finally + { + await host.DisposeAsync(); + } + } + /// Verifies that RebuildAddressSpace clears all registered variables. [Fact] public async Task RebuildAddressSpace_clears_all_registered_variables() diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs index 0adad84c..63a2cd17 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs @@ -609,10 +609,13 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase // --------------------------------------------------------------------------------------------------- // 2026-08-15 surgical-deploys (Task 6) — a NON-rebuild apply materialises the plan's DELTA, not the whole - // composition. The passes are idempotent, but idempotent is not free: every EnsureVariable / EnsureFolder - // is a sink call that takes the node manager's lock, so re-walking the composition made the cost of ANY - // apply proportional to the size of the served address space (a one-tag add on a 50k-node server was - // ~50k sink calls on the publish thread) instead of to the size of the change. + // composition. The passes are idempotent, but idempotent is not free. EnsureVariable / EnsureFolder + // early-return on a ConcurrentDictionary hit BEFORE taking the node manager's lock, so an already-standing + // node costs a dictionary probe, not a lock acquisition — but MaterialiseAlarmCondition DOES take the lock + // every time, and the probes are still O(address space) per apply plus the composition walk producing them. + // So re-walking the composition made the cost of ANY apply proportional to the size of the served address + // space (a one-tag add on a 50k-node server was ~50k sink calls on the publish thread) instead of to the + // size of the change. // --------------------------------------------------------------------------------------------------- /// THE headline test. Deploy three raw tags, then deploy the same three PLUS one more: the @@ -716,11 +719,17 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase /// /// The same repair for the shape the 2026-08-15 review found (commit 875ec9fe): the reference row is /// untouched, but its BACKING raw tag becomes historized. The coincident UnsReferenceDelta must - /// still reach the materialise pass, or the UNS mirror keeps serving no history while the raw node - /// serves it — the exact split the Backing* fields exist to close. + /// still be applied, or the UNS mirror keeps serving no history while the raw node serves it — the + /// exact split the Backing* fields exist to close. + /// Re-pointed by review I2, which changed HOW this shape is repaired: the delta's identity + /// is unchanged (same NodeId, same equipment, same backing RawPath — only the inherited attributes + /// moved), so it is now an IN-PLACE UpdateTagAttributes in the Uns realm rather than a + /// remove+recreate. The client MonitoredItems on the UNS mirror survive an operator historizing the raw + /// tag behind them. The historian-value assertion is unchanged — that is the actual repair being + /// pinned; only the mechanism that delivers it moved. /// [Fact] - public void Changed_uns_reference_is_recreated_when_the_backing_tag_flips_historian() + public void Changed_uns_reference_is_updated_in_place_when_the_backing_tag_flips_historian() { var db = NewInMemoryDbFactory(); var sink = new RecordingSink(); @@ -736,15 +745,18 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase historianTagnameForA: "WW.A"); actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep2))); + // The mirror now carries the backing tag's historian tagname — the repair itself, unchanged by I2. AwaitAssert( - () => sink.VariableDetails.ShouldContain( - v => v.NodeId == "filling/line1/station1/a" && v.HistorianTagname == "WW.A"), + () => sink.UpdateTagDetails.ShouldContain( + u => u.NodeId == "filling/line1/station1/a" && u.HistorianTagname == "WW.A" && + u.Realm == AddressSpaceRealm.Uns), duration: PresenceBudget); + Thread.Sleep(200); // let any (unwanted) remove+recreate land before asserting its absence var second = sink.Calls.Skip(afterFirst).ToList(); sink.RebuildCalls.ShouldBe(0); - second.ShouldContain("RV:filling/line1/station1/a"); // removed… - second.ShouldContain("EV:filling/line1/station1/a"); // …and re-ensured, now carrying the tagname + second.ShouldContain("UT:filling/line1/station1/a"); // repaired in place… + second.ShouldNotContain("RV:filling/line1/station1/a"); // …NOT torn down (subscriptions survive) } /// An AttributeOnly apply (here a raw tag gaining a historian override, with no UNS reference to @@ -775,6 +787,85 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase second.ShouldContain("UT:Plant/Modbus/dev1/A"); // the in-place attribute update DID happen } + /// + /// 2026-08-15 surgical-deploys review C1 — THE headline degraded-apply test. Before the delta + /// materialisation the full passes re-ensured EVERY node on EVERY apply, so a swallowed + /// EnsureVariable fault silently repaired itself on the next deploy. The delta path removed that + /// accidental self-healing: once _lastApplied records a composition, the failed node is never in + /// a diff set again and EnsureVariable's existing-node early-return means it is never re-created + /// — the node is lost until a rebuild happens to fire. So a non-rebuild apply that reports + /// failedNodes > 0 must NOT advance _lastApplied: the next deploy re-diffs the same + /// composition and re-ensures what failed. + /// + [Fact] + public void Degraded_non_rebuild_apply_does_not_advance_last_applied() + { + var db = NewInMemoryDbFactory(); + // The sink faults on exactly one of the two added variables — a PureAdd apply that lands B but + // loses A, with no rebuild anywhere in sight. + var sink = new RecordingSink { FailEnsureVariableFor = "Plant/Modbus/dev1/A" }; + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + var dep1 = SeedRawTagDeployment(db, new[] { "A", "B" }); + + var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, dbFactory: db, applier: applier)); + actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep1))); + AwaitAssert(() => sink.Calls.ShouldContain("EV:Plant/Modbus/dev1/B"), duration: PresenceBudget); + Thread.Sleep(200); + sink.RebuildCalls.ShouldBe(0); // genuinely a non-rebuild (PureAdd) apply + var afterFirst = sink.Calls.Count; + + // The fault clears (a transient sink fault, an SDK hiccup). Re-deploy the SAME composition: because + // the degraded apply did not record it as applied, the planner re-diffs A (and B) as ADDED and the + // materialise pass re-ensures them. With _lastApplied advanced this second apply would diff to an + // EMPTY plan and A would be gone from the served address space forever. + sink.FailEnsureVariableFor = null; + var dep2 = SeedRawTagDeployment(db, new[] { "A", "B" }); + actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep2))); + + AwaitAssert( + () => sink.Calls.Skip(afterFirst).ShouldContain("EV:Plant/Modbus/dev1/A"), + duration: PresenceBudget); + sink.RebuildCalls.ShouldBe(0); + } + + /// + /// The other half of C1's rule: a REBUILD that also reports failed nodes DOES advance + /// _lastApplied. A rebuild already re-materialised the whole composition, and the applier's + /// one-way rebuild ratchet is the resync mechanism for anything it could not place — holding the + /// composition back here would make every subsequent deploy re-diff the entire address space as added. + /// Proven by the third apply of the same composition being an empty-plan no-op. + /// + [Fact] + public void Rebuild_with_failures_still_advances_last_applied() + { + var db = NewInMemoryDbFactory(); + var sink = new RecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + var dep1 = SeedUnsRawDeployment(db, equipmentName: "station1", referencedTag: "a"); + + var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, dbFactory: db, applier: applier)); + actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep1))); + AwaitAssert(() => sink.Calls.ShouldContain("EV:Plant/Modbus/dev1/b"), duration: PresenceBudget); + + // Renaming the equipment is ChangedEquipment ⇒ the classifier's default-closed Rebuild; the sink + // faults on one variable of the full re-materialise that follows. + sink.FailEnsureVariableFor = "Plant/Modbus/dev1/b"; + var dep2 = SeedUnsRawDeployment(db, equipmentName: "station1-renamed", referencedTag: "a"); + actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep2))); + AwaitAssert(() => sink.RebuildCalls.ShouldBe(1), duration: PresenceBudget); + Thread.Sleep(200); + var afterSecond = sink.Calls.Count; + + // Re-deploy the SAME composition. _lastApplied advanced despite the failure, so this diffs to an + // empty plan and adds no sink calls at all. + sink.FailEnsureVariableFor = null; + var dep3 = SeedUnsRawDeployment(db, equipmentName: "station1-renamed", referencedTag: "a"); + actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep3))); + Thread.Sleep(300); + + sink.Calls.Count.ShouldBe(afterSecond); + } + /// Seal a deployment whose raw tree is Plant/Modbus/dev1 carrying one tag per name in /// (RawPath Plant/Modbus/dev1/<name>). There is no UNS topology, so /// the composition is raw-only. names one tag that carries an @@ -894,6 +985,14 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase public ConcurrentQueue<(string Source, string Target)> ReferenceEdges { get; } = new(); /// Gets or sets the count of rebuild address space calls. public int RebuildCalls; + /// Gets or sets the node id whose throws (null ⇒ none). The + /// applier's SafeEnsureVariable swallows the throw into its failed-node tally, which is exactly + /// the degraded-apply shape the C1 no-advance rule exists for. Settable mid-test so a transient fault + /// can clear between deploys. + public volatile string? FailEnsureVariableFor; + /// Gets every surgical in-place tag-attribute update with the attributes the applier resolved + /// for it (the ordered queue only carries node ids). + public ConcurrentQueue<(string NodeId, bool Writable, string? HistorianTagname, AddressSpaceRealm Realm)> UpdateTagDetails { get; } = new(); /// Gets the queue of surgical in-place folder display-name update calls (OpcUaServer-001). public ConcurrentQueue<(string FolderNodeId, string DisplayName)> FolderRenameQueue { get; } = new(); /// Gets the list of recorded surgical folder display-name update calls. @@ -935,6 +1034,8 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase /// The resolved historian tagname (null ⇒ not historized). public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, AddressSpaceRealm realm, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) { + if (string.Equals(FailEnsureVariableFor, variableNodeId, StringComparison.Ordinal)) + throw new InvalidOperationException($"simulated EnsureVariable fault for {variableNodeId}"); Calls.Enqueue($"EV:{variableNodeId}"); VariableDetails.Enqueue((variableNodeId, parentFolderNodeId, writable, historianTagname, realm)); } @@ -950,6 +1051,7 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { Calls.Enqueue($"UT:{variableNodeId}"); + UpdateTagDetails.Enqueue((variableNodeId, writable, historianTagname, realm)); return true; } /// Records a surgical in-place folder display-name update (always succeeds in this recording sink).