feat(opcua): surgical mixed add+remove deploys (R2-07 T13)

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"
       ]

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
+        //   • AddRemoveMix ⇒ NO full rebuild. Removes-then-adds within one apply: the removed nodes are torn
-        //     independently shippable with today's behaviour as the floor.
+        //     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
+        var mustRebuild = kind is AddressSpaceChangeKind.Rebuild;
-            or AddressSpaceChangeKind.AddRemoveMix;
+        // 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
+                // R2-07 Phase 2/3 — scoped remove teardown for PureRemove AND AddRemoveMix (runs after any
-                // attribute updates / renames succeeded). Terminal Bad / RemovedConditionState writes then
+                // coincident surgical attribute updates / renames succeeded). Terminal Bad /
-                // in-place removes; any false/throw flips allApplied → the rebuild ratchet below.
+                // RemovedConditionState writes then in-place removes; any false/throw flips allApplied → the
-                if (allApplied && kind == AddressSpaceChangeKind.PureRemove)
+                // 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);
                 }

tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs

@@ -927,10 +927,11 @@ public sealed class AddressSpaceApplierTests
         sink.RebuildCalls.ShouldBe(1);
     }
 
-    /// <summary>R2-07 T2 phase pin — an add+remove plan still triggers a full rebuild until Phase 3
+    /// <summary>R2-07 T13 — an add+remove plan is an AddRemoveMix: the applier removes the old node IN PLACE
-    /// (AddRemoveMix) ships. Flipped in T13.</summary>
+    /// (RemoveVariableNode) and does NOT rebuild; the add is materialised afterward by the publish actor's
+    /// passes. (Supersedes the T2 phase pin.)</summary>
     [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<AddressSpaceApplier>.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);
+    }
+
+    /// <summary>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.</summary>
+    [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<AddressSpaceApplier>.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));
+    }
+
+    /// <summary>R2-07 T13 — a remove failure inside an AddRemoveMix still trips the rebuild ratchet.</summary>
+    [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<AddressSpaceApplier>.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);
     }