feat(opcua): surgical RemoveVariableNode with NodeDeleted model-change (R2-07 T8)

2026-07-13 12:09:14 -04:00
parent 438004e371
commit 14003ab9b8
3 changed files with 295 additions and 6 deletions
@@ -1664,6 +1664,59 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        return e;
    }

+    /// <summary>Build (but do not report) the Part 3 <c>GeneralModelChangeEvent</c> announcing that the node
+    /// at <paramref name="affectedNodeId"/> was DELETED. Mirrors <see cref="BuildNodesAddedModelChange"/>
+    /// exactly — the only differences are <c>Verb = NodeDeleted</c> and that the affected node has already
+    /// been dropped from the maps, so <c>AffectedType</c> resolves to <see cref="NodeId.Null"/> (a valid Part 3
+    /// "type not applicable"; clients re-browse the parent regardless). <c>internal</c> so a node-manager test
+    /// can assert the populated Changes structure at the nearest deterministic seam.</summary>
+    /// <param name="affectedNodeId">The node id of the deleted node.</param>
+    /// <returns>A populated, unreported <see cref="GeneralModelChangeEventState"/>.</returns>
+    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;
+    }
+
+    /// <summary>Report a pre-built <see cref="GeneralModelChangeEventState"/> OUTSIDE <c>Lock</c> —
+    /// <c>Server.ReportEvent</c> re-enters the server's own subscription/event path, so holding <c>Lock</c>
+    /// across it risks a lock-order inversion (mirrors <see cref="RaiseNodesAddedModelChange"/> /
+    /// <c>ReportNodeShapeChangedEvent</c>). Tolerant: swallow-and-log when eventing is disabled / there are no
+    /// monitored items / the server is shutting down — the node mutation already stands.</summary>
+    /// <param name="e">The pre-built event to report.</param>
+    /// <param name="affectedNodeId">The affected node id (for the diagnostic only).</param>
+    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
+        }
+    }
+
    /// <summary>Resolve the TypeDefinition of a materialised node id from the live folder/variable maps for a
    /// model-change event's <c>AffectedType</c>; <see cref="NodeId.Null"/> when the id is not registered.</summary>
    /// <param name="nodeId">The folder-scoped node id whose TypeDefinition is wanted.</param>
@@ -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.
    /// <inheritdoc cref="ISurgicalAddressSpaceSink.RemoveVariableNode"/>
-    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;
+    }

    /// <inheritdoc cref="ISurgicalAddressSpaceSink.RemoveAlarmConditionNode"/>
    public bool RemoveAlarmConditionNode(string alarmNodeId) => false;