From e2b47638ec9d391fc7e344b77714e084ae3135e4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 12:22:45 -0400 Subject: [PATCH] feat(opcua): surgical mixed add+remove deploys (R2-07 T13) --- ...2-07-surgical-pure-adds-plan.md.tasks.json | 2 +- .../AddressSpaceApplier.cs | 25 ++++--- .../AddressSpaceApplierTests.cs | 65 ++++++++++++++++++- 3 files changed, 79 insertions(+), 13 deletions(-) diff --git a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json index a3db061f..dbfd5a76 100644 --- a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json +++ b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json @@ -113,7 +113,7 @@ { "id": "T13", "subject": "Phase 3: AddRemoveMix routing \u2014 removes-then-adds surgically, same-NodeId remove-then-recreate ordering test (high-risk)", - "status": "pending", + "status": "completed", "blockedBy": [ "T12" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index 61a963a7..a4f72c49 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -132,8 +132,11 @@ public sealed class AddressSpaceApplier // subtree, via the ISurgicalAddressSpaceSink remove members below (each preceded by a terminal // Bad / RemovedConditionState write so in-flight MonitoredItems observe the removal); subscribers // of OTHER nodes are untouched. Any false/throw from a remove ⇒ full-rebuild fallback (ratchet). - // • AddRemoveMix ⇒ full rebuild UNTIL Phase 3 ships (R2-07 T13); mapping it here keeps each phase - // independently shippable with today's behaviour as the floor. + // • AddRemoveMix ⇒ NO full rebuild. Removes-then-adds within one apply: the removed nodes are torn + // down IN PLACE here (the PureRemove pass), then OpcUaPublishActor's idempotent Materialise passes + // create the added nodes and AnnounceAddedNodes announces them — natural remove-then-recreate + // order even when an id is reused across the remove + add sets (the recreated node is a fresh + // NodeState, correct — it is a different tag). Any remove false/throw ⇒ full-rebuild fallback. // • Rebuild ⇒ the default-closed safety valve for any node-affecting change (ChangedEquipment / // ChangedAlarms / non-surgical ChangedEquipmentTags / node-relevant ChangedEquipmentVirtualTags) // — the classifier's rule 2, which also catches any future plan field that makes a changed @@ -141,8 +144,10 @@ public sealed class AddressSpaceApplier // ChangedDrivers is node-inert (routes through DriverHostActor's spawn plan) — the classifier leaves // a driver-only plan AttributeOnly, so it never rebuilds here. var kind = AddressSpaceChangeClassifier.Classify(plan); - var mustRebuild = kind is AddressSpaceChangeKind.Rebuild - or AddressSpaceChangeKind.AddRemoveMix; + var mustRebuild = kind is AddressSpaceChangeKind.Rebuild; + // PureRemove + AddRemoveMix both run the in-place remove pass below (AddRemoveMix's adds are handled + // afterward by the publish actor's Materialise passes + announce). + var hasRemovePass = kind is AddressSpaceChangeKind.PureRemove or AddressSpaceChangeKind.AddRemoveMix; var surgicalTagDeltas = plan.ChangedEquipmentTags.Where(AddressSpaceChangeClassifier.TagDeltaIsSurgicalEligible).ToList(); // UNS Area / Line renames are surgically applicable (in-place DisplayName swap) @@ -158,7 +163,7 @@ public sealed class AddressSpaceApplier rebuildFailed = !SafeRebuild(); rebuilt = true; } - else if (surgicalTagDeltas.Count > 0 || renamedFolders.Count > 0 || kind == AddressSpaceChangeKind.PureRemove) + else if (surgicalTagDeltas.Count > 0 || renamedFolders.Count > 0 || hasRemovePass) { if (_sink is ISurgicalAddressSpaceSink surgical) { @@ -194,10 +199,12 @@ public sealed class AddressSpaceApplier } if (!ok) { allApplied = false; break; } } - // R2-07 Phase 2 — PureRemove scoped teardown (runs after any coincident surgical - // attribute updates / renames succeeded). Terminal Bad / RemovedConditionState writes then - // in-place removes; any false/throw flips allApplied → the rebuild ratchet below. - if (allApplied && kind == AddressSpaceChangeKind.PureRemove) + // R2-07 Phase 2/3 — scoped remove teardown for PureRemove AND AddRemoveMix (runs after any + // coincident surgical attribute updates / renames succeeded). Terminal Bad / + // RemovedConditionState writes then in-place removes; any false/throw flips allApplied → the + // rebuild ratchet below. For AddRemoveMix the adds are materialised afterward by the publish + // actor's passes (remove-then-recreate order). + if (allApplied && hasRemovePass) { allApplied = ApplyPureRemove(surgical, plan, ts, ref failedNodes); } 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 72021ec8..97dfefc5 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs @@ -927,10 +927,11 @@ public sealed class AddressSpaceApplierTests sink.RebuildCalls.ShouldBe(1); } - /// R2-07 T2 phase pin — an add+remove plan still triggers a full rebuild until Phase 3 - /// (AddRemoveMix) ships. Flipped in T13. + /// R2-07 T13 — an add+remove plan is an AddRemoveMix: the applier removes the old node IN PLACE + /// (RemoveVariableNode) and does NOT rebuild; the add is materialised afterward by the publish actor's + /// passes. (Supersedes the T2 phase pin.) [Fact] - public void Add_and_remove_still_rebuild_phase_pin() + public void Add_and_remove_is_mixed_removes_in_place_without_rebuild() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); @@ -949,6 +950,64 @@ public sealed class AddressSpaceApplierTests var outcome = applier.Apply(plan); + outcome.RebuildCalled.ShouldBeFalse(); + sink.RebuildCalls.ShouldBe(0); + // The removed node is torn down in place; the add is left to the publish actor's materialise passes. + sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("var", EquipmentNodeIds.Variable("eq-1", "", "Old"))); + outcome.AddedNodes.ShouldBe(1); + outcome.RemovedNodes.ShouldBe(1); + } + + /// R2-07 T13 — an id REUSED across the remove + add sets in one deploy (a tag removed at the + /// same folder-scoped NodeId another tag is added at): the applier removes the old node in place (the + /// recreate is the publish actor's job, a fresh NodeState). No rebuild. + [Fact] + public void Add_and_remove_reusing_same_node_id_removes_the_old_node_without_rebuild() + { + var sink = new RecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var nodeId = EquipmentNodeIds.Variable("eq-1", "", "Slot"); + var plan = EmptyPlan with + { + // Different TagId, but both resolve to the SAME folder-scoped NodeId (eq-1/Slot). + RemovedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-old", "eq-1", "drv", FolderPath: "", Name: "Slot", DataType: "Float", FullName: "40001", Writable: false, Alarm: null), + }, + AddedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-new", "eq-1", "drv", FolderPath: "", Name: "Slot", DataType: "Int32", FullName: "40002", Writable: false, Alarm: null), + }, + }; + + var outcome = applier.Apply(plan); + + outcome.RebuildCalled.ShouldBeFalse(); + sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("var", nodeId)); + } + + /// R2-07 T13 — a remove failure inside an AddRemoveMix still trips the rebuild ratchet. + [Fact] + public void Add_and_remove_with_remove_returns_false_falls_back_to_rebuild() + { + var sink = new RecordingSink { RemoveReturns = false }; + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var plan = EmptyPlan with + { + AddedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-new", "eq-1", "drv", FolderPath: "", Name: "New", DataType: "Float", FullName: "40009", Writable: false, Alarm: null), + }, + RemovedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-old", "eq-1", "drv", FolderPath: "", Name: "Old", DataType: "Float", FullName: "40001", Writable: false, Alarm: null), + }, + }; + + var outcome = applier.Apply(plan); + outcome.RebuildCalled.ShouldBeTrue(); sink.RebuildCalls.ShouldBe(1); }