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 ee3c6b06..e77928e6 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
@@ -71,7 +71,7 @@
{
"id": "T8",
"subject": "Phase 2: OtOpcUaNodeManager.RemoveVariableNode + BuildNodesRemovedModelChange (NodeDeleted) (high-risk)",
- "status": "pending",
+ "status": "completed",
"blockedBy": [
"T7"
]
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
index 91b4acb4..a09a7e58 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
@@ -1664,6 +1664,59 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
return e;
}
+ /// Build (but do not report) the Part 3 GeneralModelChangeEvent announcing that the node
+ /// at was DELETED. Mirrors
+ /// exactly — the only differences are Verb = NodeDeleted and that the affected node has already
+ /// been dropped from the maps, so AffectedType resolves to (a valid Part 3
+ /// "type not applicable"; clients re-browse the parent regardless). internal so a node-manager test
+ /// can assert the populated Changes structure at the nearest deterministic seam.
+ /// The node id of the deleted node.
+ /// A populated, unreported .
+ internal GeneralModelChangeEventState BuildNodesRemovedModelChange(string affectedNodeId)
+ {
+ var affected = new NodeId(affectedNodeId, NamespaceIndex);
+ var e = new GeneralModelChangeEventState(null);
+ e.Initialize(
+ SystemContext,
+ source: null,
+ severity: EventSeverity.Medium,
+ message: new LocalizedText($"Node deleted: {affected}"));
+ // Part 3 §8.7.4: emitted by the Server object — set SourceNode/SourceName to Server explicitly
+ // (mirrors BuildNodesAddedModelChange).
+ e.SetChildValue(SystemContext, BrowseNames.SourceNode, ObjectIds.Server, false);
+ e.SetChildValue(SystemContext, BrowseNames.SourceName, "Server", false);
+ var change = new ModelChangeStructureDataType
+ {
+ Affected = affected,
+ // The node is already gone from the maps, so its TypeDefinition is not applicable (Null).
+ AffectedType = ResolveAffectedTypeDefinition(affectedNodeId),
+ Verb = (byte)ModelChangeStructureVerbMask.NodeDeleted,
+ };
+ e.SetChildValue(SystemContext, BrowseNames.Changes, new[] { change }, false);
+ return e;
+ }
+
+ /// Report a pre-built OUTSIDE Lock —
+ /// Server.ReportEvent re-enters the server's own subscription/event path, so holding Lock
+ /// across it risks a lock-order inversion (mirrors /
+ /// ReportNodeShapeChangedEvent). Tolerant: swallow-and-log when eventing is disabled / there are no
+ /// monitored items / the server is shutting down — the node mutation already stands.
+ /// The pre-built event to report.
+ /// The affected node id (for the diagnostic only).
+ private void ReportModelChangeOutsideLock(GeneralModelChangeEventState e, string affectedNodeId)
+ {
+ try
+ {
+ Server.ReportEvent(SystemContext, e);
+ }
+ catch (Exception ex)
+ {
+#pragma warning disable CS0618 // Utils.LogError is [Obsolete] in favour of an ITelemetryContext this manager doesn't carry.
+ Utils.LogError(ex, "OtOpcUaNodeManager: failed to report GeneralModelChangeEvent for {0}", affectedNodeId);
+#pragma warning restore CS0618
+ }
+ }
+
/// Resolve the TypeDefinition of a materialised node id from the live folder/variable maps for a
/// model-change event's AffectedType; when the id is not registered.
/// The folder-scoped node id whose TypeDefinition is wanted.
@@ -1746,12 +1799,28 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
}
- // R2-07 Phase 2 (T7 placeholder) — surgical single-node / subtree removal. These return false (the
- // rebuild-fallback contract) so the tree stays shippable at T7 while T8/T9/T10 implement the real
- // in-place teardown (mirroring the per-node loops inside RebuildAddressSpace, scoped to the id/subtree,
- // with a NodeDeleted model-change reported outside Lock). Implemented in T8/T9/T10.
///
- public bool RemoveVariableNode(string variableNodeId) => false;
+ public bool RemoveVariableNode(string variableNodeId)
+ {
+ ArgumentException.ThrowIfNullOrEmpty(variableNodeId);
+ EnsureAddressSpaceCreated();
+
+ GeneralModelChangeEventState e;
+ lock (Lock)
+ {
+ // Unknown id ⇒ the node-manager maps drifted from what the planner believes; return false so the
+ // caller (AddressSpaceApplier) falls back to a full rebuild (resync). Mirrors the per-node
+ // teardown inside RebuildAddressSpace, scoped to this one id.
+ if (!_variables.TryRemove(variableNodeId, out var variable)) return false;
+ variable.Parent?.RemoveChild(variable);
+ PredefinedNodes?.Remove(variable.NodeId);
+ // Drop the historized-tagname registration alongside the variable it maps (Phase C parity).
+ _historizedTagnames.TryRemove(variableNodeId, out _);
+ e = BuildNodesRemovedModelChange(variableNodeId);
+ }
+ ReportModelChangeOutsideLock(e, variableNodeId);
+ return true;
+ }
///
public bool RemoveAlarmConditionNode(string alarmNodeId) => false;
diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs
new file mode 100644
index 00000000..fd225779
--- /dev/null
+++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs
@@ -0,0 +1,220 @@
+using Opc.Ua;
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
+
+///
+/// R2-07 Phase 2 (T8/T9/T10) — surgical IN-PLACE removal on the node manager: a single value variable
+/// (), a single Part 9 alarm condition
+/// (), and an equipment folder + its whole
+/// descendant subtree with notifier demotion ().
+/// Each removal detaches only the scoped node(s), cleans the matching maps, and raises a Part 3
+/// NodeDeleted model-change; an unknown id returns false (the caller falls back to a full rebuild).
+///
+public sealed class NodeManagerSurgicalRemoveTests : IDisposable
+{
+ private static CancellationToken Ct => TestContext.Current.CancellationToken;
+
+ private readonly string _pkiRoot = Path.Combine(
+ Path.GetTempPath(),
+ $"otopcua-surgical-remove-{Guid.NewGuid():N}");
+
+ // ---------- T8: RemoveVariableNode ----------
+
+ /// Ensure a variable then remove it: it disappears from the maps (TryGetVariable null,
+ /// VariableCount decremented), its historized-tagname registration is dropped, and the call returns
+ /// true.
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task RemoveVariableNode_drops_variable_and_historian_registration()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+ nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1");
+ nm.EnsureVariable("eq-1/A", "eq-1", "A", "Float", writable: false, historianTagname: "Hist.A");
+ nm.EnsureVariable("eq-1/B", "eq-1", "B", "Float", writable: false);
+ var countBefore = nm.VariableCount;
+ nm.TryGetHistorizedTagname("eq-1/A", out _).ShouldBeTrue();
+
+ nm.RemoveVariableNode("eq-1/A").ShouldBeTrue();
+
+ nm.TryGetVariable("eq-1/A").ShouldBeNull();
+ nm.VariableCount.ShouldBe(countBefore - 1);
+ nm.TryGetHistorizedTagname("eq-1/A", out _).ShouldBeFalse();
+ // The sibling variable is untouched.
+ nm.TryGetVariable("eq-1/B").ShouldNotBeNull();
+
+ await host.DisposeAsync();
+ }
+
+ /// Removing an unknown variable id returns false (map drift ⇒ caller rebuilds).
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task RemoveVariableNode_unknown_id_returns_false()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+
+ nm.RemoveVariableNode("eq-1/nope").ShouldBeFalse();
+
+ await host.DisposeAsync();
+ }
+
+ /// The built removed-node event announces the deleted node with verb NodeDeleted.
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task Built_removed_event_announces_the_deleted_node_with_NodeDeleted_verb()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+ nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1");
+ nm.EnsureVariable("eq-1/A", "eq-1", "A", "Float", writable: false);
+
+ var e = nm.BuildNodesRemovedModelChange("eq-1/A");
+
+ e.ShouldNotBeNull();
+ e.Changes.ShouldNotBeNull();
+ var changes = e.Changes.Value;
+ changes.Length.ShouldBe(1);
+ changes[0].Verb.ShouldBe((byte)ModelChangeStructureVerbMask.NodeDeleted);
+
+ await host.DisposeAsync();
+ }
+
+ // ---------- T9: RemoveAlarmConditionNode ----------
+
+ /// Materialise a native alarm condition then remove it: it disappears from the condition map and
+ /// the native flag is cleared; unknown id returns false.
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task RemoveAlarmConditionNode_drops_condition_and_native_flag()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+ nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1");
+ nm.MaterialiseAlarmCondition("eq-1/OverTemp", "eq-1", "OverTemp", "OffNormalAlarm", 700, isNative: true);
+ nm.TryGetAlarmCondition("eq-1/OverTemp").ShouldNotBeNull();
+ nm.IsNativeAlarmNode("eq-1/OverTemp").ShouldBeTrue();
+
+ nm.RemoveAlarmConditionNode("eq-1/OverTemp").ShouldBeTrue();
+
+ nm.TryGetAlarmCondition("eq-1/OverTemp").ShouldBeNull();
+ nm.IsNativeAlarmNode("eq-1/OverTemp").ShouldBeFalse();
+
+ nm.RemoveAlarmConditionNode("eq-1/OverTemp").ShouldBeFalse(); // already gone ⇒ false
+
+ await host.DisposeAsync();
+ }
+
+ /// A scripted condition removes cleanly too (no native flag was set).
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task RemoveAlarmConditionNode_removes_scripted_condition()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+ nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1");
+ nm.MaterialiseAlarmCondition("alm-1", "eq-1", "HighTemp", "OffNormalAlarm", 500, isNative: false);
+
+ nm.RemoveAlarmConditionNode("alm-1").ShouldBeTrue();
+ nm.TryGetAlarmCondition("alm-1").ShouldBeNull();
+
+ await host.DisposeAsync();
+ }
+
+ // ---------- T10: RemoveEquipmentSubtree ----------
+
+ /// Remove an equipment folder carrying a sub-folder, variables (one historized), and a condition:
+ /// every descendant disappears from every map, the notifier registration is demoted, and a SIBLING
+ /// equipment is fully intact.
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task RemoveEquipmentSubtree_removes_all_descendants_and_leaves_siblings_intact()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+
+ // Target equipment eq-1 with a sub-folder, two variables (one historized), and a native condition.
+ nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1");
+ nm.EnsureFolder("eq-1/Diag", parentNodeId: "eq-1", displayName: "Diag");
+ nm.EnsureVariable("eq-1/A", "eq-1", "A", "Float", writable: false, historianTagname: "Hist.A");
+ nm.EnsureVariable("eq-1/Diag/T", "eq-1/Diag", "T", "Float", writable: false);
+ nm.MaterialiseAlarmCondition("eq-1/OverTemp", "eq-1", "OverTemp", "OffNormalAlarm", 700, isNative: true);
+
+ // Sibling equipment eq-2 that must survive untouched.
+ nm.EnsureFolder("eq-2", parentNodeId: null, displayName: "Equipment 2");
+ nm.EnsureVariable("eq-2/S", "eq-2", "S", "Float", writable: false);
+
+ nm.RemoveEquipmentSubtree("eq-1").ShouldBeTrue();
+
+ // Every eq-1 descendant is gone from every map.
+ nm.TryGetFolder("eq-1").ShouldBeNull();
+ nm.TryGetFolder("eq-1/Diag").ShouldBeNull();
+ nm.TryGetVariable("eq-1/A").ShouldBeNull();
+ nm.TryGetVariable("eq-1/Diag/T").ShouldBeNull();
+ nm.TryGetAlarmCondition("eq-1/OverTemp").ShouldBeNull();
+ nm.TryGetHistorizedTagname("eq-1/A", out _).ShouldBeFalse();
+ nm.IsNativeAlarmNode("eq-1/OverTemp").ShouldBeFalse();
+
+ // Sibling eq-2 is fully intact.
+ nm.TryGetFolder("eq-2").ShouldNotBeNull();
+ nm.TryGetVariable("eq-2/S").ShouldNotBeNull();
+
+ // Re-materialising an alarm under eq-2 still works (the notifier machinery was not corrupted by the
+ // eq-1 demotion) — proves no orphaned root-notifier ref broke the event path.
+ Should.NotThrow(() => nm.MaterialiseAlarmCondition("eq-2/Alm", "eq-2", "Alm", "OffNormalAlarm", 300, isNative: false));
+
+ await host.DisposeAsync();
+ }
+
+ /// Removing an unknown equipment id returns false (map drift ⇒ caller rebuilds).
+ [Trait("Category", "Unit")]
+ [Fact]
+ public async Task RemoveEquipmentSubtree_unknown_id_returns_false()
+ {
+ var (host, server) = await BootAsync();
+ var nm = server.NodeManager!;
+
+ nm.RemoveEquipmentSubtree("eq-nope").ShouldBeFalse();
+
+ await host.DisposeAsync();
+ }
+
+ private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync()
+ {
+ var host = new OpcUaApplicationHost(
+ new OpcUaApplicationHostOptions
+ {
+ ApplicationName = "OtOpcUa.SurgicalRemoveTest",
+ ApplicationUri = $"urn:OtOpcUa.SurgicalRemoveTest:{Guid.NewGuid():N}",
+ OpcUaPort = AllocateFreePort(),
+ PublicHostname = "localhost",
+ PkiStoreRoot = _pkiRoot,
+ },
+ Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance);
+
+ var server = new OtOpcUaSdkServer();
+ await host.StartAsync(server, Ct);
+ return (host, server);
+ }
+
+ private static int AllocateFreePort()
+ {
+ using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0);
+ listener.Start();
+ var port = ((System.Net.IPEndPoint)listener.LocalEndpoint).Port;
+ listener.Stop();
+ return port;
+ }
+
+ /// Cleans up the PKI root directory.
+ public void Dispose()
+ {
+ if (Directory.Exists(_pkiRoot))
+ {
+ try { Directory.Delete(_pkiRoot, recursive: true); }
+ catch { /* best-effort cleanup */ }
+ }
+ }
+}