fix(opcua): tombstone MonitoredNode2 on every node removal (B2a)

Review 01 L2. The SDK keys its fan-out lists by NodeId and its component cache hands back the CACHED NodeState for a NodeId it already holds, so a rebuild or a surgical remove+re-add left a NEW item on the same NodeId — from any session — bound to the DEAD node: created Good, fed its stale initial value, never again, until every session had deleted its items. Every removal path (RebuildAddressSpace, RemoveVariableNode, RemoveAlarmConditionNode incl. the condition's instance-scoped children, RemoveEquipmentSubtree) now queues BadNodeIdUnknown to the node's data items, drops the MonitoredNodes entry, releases each item's component cache reference and forgets the _liveItems/_eventItems entries. Items the ACL pass stood down (on no fan-out list) are swept by NodeId. A later client delete of an orphaned item is completed by the DeleteMonitoredItem override / the SubscribeToEvents unsubscribe arm instead of answering BadMonitoredItemIdInvalid or NRE-ing in MonitoredNode2.Remove. The revalidation restore re-resolves the node by NodeId and no longer re-attaches to Handle.Node blindly. Timestamps now come from the injected Clock (UtcNow). Tests: StaleMonitoredNodeWireTests (real SDK, three legs: rebuild, surgical remove+re-add, second session; each keeps the old item alive, proves the new item receives a value written after its creation, the old one gets BadNodeIdUnknown, and its delete succeeds — all three fail with the tombstone neutered), NodeManagerStaleMonitoredNodeTests (stood-down item swept on surgical remove and rebuild; sibling removal leaves it restorable). Refs #732 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 06:41:09 -04:00
parent 00300d0f60
commit eaac65286d
3 changed files with 694 additions and 5 deletions
@@ -290,6 +290,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2

    private volatile TimeProvider _clock = TimeProvider.System;

+    /// <summary>The manager's "now" (UTC) — read off <see cref="Clock"/> so a test clock drives every timestamp this
+    /// manager stamps (condition / shelve / revert / revalidation times), not only the HistoryUpdate bound.</summary>
+    private DateTime UtcNow => _clock.GetUtcNow().UtcDateTime;
+
    private volatile IOpcUaNodeWriteGateway _nodeWriteGateway = NullOpcUaNodeWriteGateway.Instance;

    /// <summary>
@@ -3491,6 +3495,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    {
        lock (Lock)
        {
+            // Review 01 L2 — every monitored item on a node this rebuild discards is told and forgotten BEFORE
+            // the nodes go, so a later create on the same NodeId binds to the re-created node (see
+            // TombstoneMonitoredNode). The shared root survives the rebuild and keeps its items.
+            TombstoneAllMonitoredNodes();
+
            foreach (var v in _variables.Values)
            {
                v.Parent?.RemoveChild(v);
@@ -3602,6 +3611,7 @@ 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;
+            TombstoneMonitoredNode(variable.NodeId, UtcNow);
            // 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
@@ -3668,6 +3678,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            // lingering notifier folder is harmless (RemoveEquipmentSubtree demotes it when the whole
            // equipment goes).
            if (!_alarmConditions.TryRemove(key, out var condition)) return false;
+            TombstoneMonitoredSubtree(condition, UtcNow);
            // unwire this condition's extra equipment-folder notifiers bidirectionally BEFORE
            // the condition drops — the folders (equipment, UNS realm) SURVIVE this scoped remove, so without
            // this they would keep a dangling inverse-notifier entry to the removed condition.
@@ -3704,11 +3715,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            bool InScope(RealmKey key) => key.Realm == realm
                && (key.Id == equipmentNodeId || key.Id.StartsWith(prefix, StringComparison.Ordinal));

+            var now = UtcNow;
            // Value variables (+ their historized-tagname registrations).
            foreach (var id in _variables.Keys.Where(InScope).ToList())
            {
                if (_variables.TryRemove(id, out var v))
                {
+                    TombstoneMonitoredNode(v.NodeId, now);
                    // Sever the inverse half of each cross-tree Organizes edge
                    // first, exactly as RemoveVariableNode does — a UNS reference variable in scope points at a
                    // RAW node that survives this UNS-scoped removal and would otherwise keep a dangling edge.
@@ -3725,6 +3738,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            {
                if (_alarmConditions.TryRemove(id, out var c))
                {
+                    TombstoneMonitoredSubtree(c, now);
                    c.Parent?.RemoveChild(c);
                    ForgetPredefinedSubtree(c);
                }
@@ -3765,6 +3779,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            {
                if (_folders.TryRemove(id, out var f))
                {
+                    TombstoneMonitoredNode(f.NodeId, now);
                    f.Parent?.RemoveChild(f);
                    PredefinedNodes?.Remove(f.NodeId);
                }
@@ -4080,8 +4095,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2

        if (unsubscribe)
        {
-            // An item RevalidateMonitoredItems stood down is ALREADY off its notifier's fan-out.
-            // Handing its delete to the SDK would make it remove an item that is not there — at best a
+            // An item RevalidateMonitoredItems stood down is ALREADY off its notifier's fan-out, and so is
+            // one whose notifier a rebuild / surgical remove tombstoned (review 01 L2, TombstoneMonitoredNode).
+            // Handing either delete to the SDK would make it remove an item that is not there — at best a
            // spurious BadNodeIdUnknown on the client's delete, at worst a NullReferenceException inside
            // MonitoredNode2.Remove when the node's event list has gone null. Checked and dropped under the
            // same Lock the revalidation pass re-attaches under, so the two cannot interleave into
@@ -4089,7 +4105,17 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            lock (Lock)
            {
                if (_eventItems.TryRemove(monitoredItem, out var stoodDown) && stoodDown.Suppressed)
+                {
+                    Interlocked.Decrement(ref _suppressedItems);
                    return ServiceResult.Good;
+                }
+
+                if (IsHandleInNamespace(sourceId) is { } h
+                    && (!MonitoredNodes.TryGetValue(h.NodeId, out var current)
+                        || current.EventMonitoredItems?.Contains(monitoredItem) != true))
+                {
+                    return ServiceResult.Good;
+                }
            }

            return base.SubscribeToEvents(context, sourceId, subscriptionId, monitoredItem, unsubscribe);
@@ -4175,10 +4201,186 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    protected override void OnMonitoredItemDeleted(
        ServerSystemContext context, NodeHandle handle, ISampledDataChangeMonitoredItem monitoredItem)
    {
-        _liveItems.TryRemove(monitoredItem, out _);
+        if (_liveItems.TryRemove(monitoredItem, out var tracked) && tracked.Suppressed) Interlocked.Decrement(ref _suppressedItems);
        base.OnMonitoredItemDeleted(context, handle, monitoredItem);
    }

+    // Review 01 L2 — a client deleting an item whose node this manager removed from under it (a rebuild or
+    // a surgical remove tombstoned the node's MonitoredNode2, see TombstoneMonitoredNode) must succeed. The
+    // SDK's own delete looks the node up by NodeId: with the entry gone it answers BadMonitoredItemIdInvalid,
+    // and with a FRESH entry (the same NodeId re-created and re-subscribed) it would try to remove the item
+    // from a fan-out list it is not on — a NullReferenceException when that list has gone null. An item that
+    // is not on the current fan-out for its NodeId has nothing left to detach, so its delete is completed here.
+    /// <inheritdoc />
+    protected override ServiceResult DeleteMonitoredItem(
+        ServerSystemContext context, IMonitoredItem monitoredItem, NodeHandle handle)
+    {
+        lock (Lock)
+        {
+            if (monitoredItem is ISampledDataChangeMonitoredItem dataItem
+                && (!MonitoredNodes.TryGetValue(handle.NodeId, out var current)
+                    || current.DataChangeMonitoredItems?.Contains(dataItem) != true))
+            {
+                OnMonitoredItemDeleted(context, handle, dataItem);
+                return ServiceResult.Good;
+            }
+        }
+
+        return base.DeleteMonitoredItem(context, monitoredItem, handle);
+    }
+
+    /// <summary>How many items the revalidation pass currently holds detached (stood down). Lets the
+    /// tombstone sweep for stood-down items — which are not on any fan-out list — run only when there are any.</summary>
+    private int _suppressedItems;
+
+    /// <summary>
+    ///     Review 01 L2 — forget the <see cref="MonitoredNode2"/> of a node this manager is removing, telling
+    ///     every data-change item on it <c>BadNodeIdUnknown</c> first. MUST be called under <c>Lock</c>, BEFORE
+    ///     the node is detached.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         The SDK keys its fan-out lists by NodeId and never clears them on the node manager's behalf
+    ///         (<c>DeleteMonitoredItem</c> drops an entry only when its item count reaches zero), and its
+    ///         component cache (<c>AddNodeToComponentCache</c>) hands back the CACHED <see cref="NodeState"/>
+    ///         for a NodeId it already holds. So after a rebuild or a surgical remove+re-add, a NEW item on the
+    ///         same NodeId — from any session — was bound to the DEAD node: created <c>Good</c>, never fed. Only
+    ///         once every session had deleted its items did a create bind to the live node.
+    ///     </para>
+    ///     <para>
+    ///         The fix removes the entry and releases each item's component-cache reference (the refcount the
+    ///         SDK took at create), so the next create binds to the re-created node. The items themselves stay in
+    ///         their client's subscription — a node manager cannot remove them — told <c>BadNodeIdUnknown</c>
+    ///         (data items; an event item has no value slot) and dropped from the revalidation maps; their later
+    ///         client delete is completed by <see cref="DeleteMonitoredItem"/> / the <c>SubscribeToEvents</c>
+    ///         unsubscribe arm. Queueing under <c>Lock</c> follows the publish path's own order (<c>Lock</c>, then
+    ///         the item lock) — the inversion the revalidation pass avoids is the other way round.
+    ///     </para>
+    ///     <para>
+    ///         An item the revalidation pass has STOOD DOWN is on no fan-out list, so it is swept from the
+    ///         tracking map by NodeId instead — it still holds a component-cache reference that would otherwise
+    ///         keep the dead node cached.
+    ///     </para>
+    /// </remarks>
+    /// <param name="nodeId">The NodeId being removed.</param>
+    /// <param name="now">The timestamp the terminal <c>BadNodeIdUnknown</c> carries.</param>
+    private void TombstoneMonitoredNode(NodeId nodeId, DateTime now)
+    {
+        if (MonitoredNodes.TryGetValue(nodeId, out var monitored))
+        {
+            MonitoredNodes.Remove(nodeId);
+            if (monitored.DataChangeMonitoredItems is { Count: > 0 } dataItems)
+            {
+                foreach (var item in dataItems.ToArray())
+                {
+                    TellNodeRemoved(item, now);
+                    if (item is ISampledDataChangeMonitoredItem sampled) _liveItems.TryRemove(sampled, out _);
+                    ReleaseComponentCacheReference(item);
+                }
+            }
+
+            if (monitored.EventMonitoredItems is { Count: > 0 } eventItems)
+            {
+                foreach (var item in eventItems.ToArray())
+                {
+                    _eventItems.TryRemove(item, out _);
+                    ReleaseComponentCacheReference(item);
+                }
+            }
+        }
+
+        if (Volatile.Read(ref _suppressedItems) > 0) SweepStoodDownItems(nodeId, now);
+    }
+
+    /// <summary>Tombstones <paramref name="node"/> and every descendant (a condition's instance-scoped
+    /// children are monitorable in their own right). MUST be called under <c>Lock</c>.</summary>
+    private void TombstoneMonitoredSubtree(NodeState node, DateTime now)
+    {
+        TombstoneMonitoredNode(node.NodeId, now);
+        var children = new List<BaseInstanceState>();
+        node.GetChildren(SystemContext, children);
+        foreach (var child in children) TombstoneMonitoredSubtree(child, now);
+    }
+
+    /// <summary>The rebuild form: every monitored node this manager serves except the shared root, which the
+    /// rebuild keeps. MUST be called under <c>Lock</c>.</summary>
+    private void TombstoneAllMonitoredNodes()
+    {
+        var now = UtcNow;
+        var rootId = _root?.NodeId;
+        foreach (var nodeId in MonitoredNodes.Keys.ToList())
+        {
+            if (rootId is not null && nodeId == rootId) continue;
+            TombstoneMonitoredNode(nodeId, now);
+        }
+
+        if (Volatile.Read(ref _suppressedItems) > 0) SweepStoodDownItems(nodeId: null, now);
+    }
+
+    /// <summary>Drops the stood-down items of <paramref name="nodeId"/> (every stood-down item when null —
+    /// the rebuild case) from the revalidation maps, telling the data items <c>BadNodeIdUnknown</c>.</summary>
+    private void SweepStoodDownItems(NodeId? nodeId, DateTime now)
+    {
+        foreach (var (item, tracked) in _liveItems)
+        {
+            if (!tracked.Suppressed || (nodeId is not null && tracked.Handle.NodeId != nodeId)) continue;
+            if (!_liveItems.TryRemove(item, out _)) continue;
+            Interlocked.Decrement(ref _suppressedItems);
+            TellNodeRemoved(item, now);
+            ReleaseComponentCacheReference(item);
+        }
+
+        foreach (var (item, tracked) in _eventItems)
+        {
+            if (!tracked.Suppressed || (nodeId is not null && tracked.Handle.NodeId != nodeId)) continue;
+            if (!_eventItems.TryRemove(item, out _)) continue;
+            Interlocked.Decrement(ref _suppressedItems);
+            ReleaseComponentCacheReference(item);
+        }
+    }
+
+    /// <summary>Queues the terminal <c>BadNodeIdUnknown</c> on a data-change item whose node is going away.</summary>
+    private void TellNodeRemoved(IDataChangeMonitoredItem2 item, DateTime now)
+    {
+        try
+        {
+            item.QueueValue(
+                new DataValue(StatusCodes.BadNodeIdUnknown) { SourceTimestamp = now, ServerTimestamp = now },
+                new ServiceResult(StatusCodes.BadNodeIdUnknown),
+                ignoreFilters: true);
+        }
+        catch (Exception ex)
+        {
+            // A disabled item refuses the value; nothing else here is worth failing a removal for.
+            _logger.LogDebug(ex, "OtOpcUaNodeManager: could not queue the removal status to a monitored item");
+        }
+    }
+
+    /// <summary>Gives back the component-cache reference the SDK took for <paramref name="item"/> at create, so
+    /// the cache stops handing out the removed node for that NodeId.</summary>
+    private void ReleaseComponentCacheReference(IMonitoredItem item)
+    {
+        if (item.ManagerHandle is NodeHandle handle) RemoveNodeFromComponentCache(SystemContext, handle);
+    }
+
+    /// <summary>Test observability: how many NodeIds currently carry a <see cref="MonitoredNode2"/> fan-out.</summary>
+    internal int MonitoredNodeCount
+    {
+        get { lock (Lock) return MonitoredNodes.Count; }
+    }
+
+    /// <summary>Test observability: whether <paramref name="nodeId"/> currently carries a fan-out.</summary>
+    internal bool IsNodeMonitored(NodeId nodeId)
+    {
+        lock (Lock) return MonitoredNodes.ContainsKey(nodeId);
+    }
+
+    /// <summary>Test observability: how many data-change items the revalidation map tracks.</summary>
+    internal int LiveItemCount => _liveItems.Count;
+
+    /// <summary>Test observability: how many direct event items the revalidation map tracks.</summary>
+    internal int LiveEventItemCount => _eventItems.Count;
+
    /// <summary>
    ///     Live data-change monitored items on nodes this manager owns, so a grant revoked by a deployment
    ///     can reach the subscriptions it was authorizing.
@@ -4817,7 +5019,26 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            {
                if (!tracked.Suppressed) continue;

-                lock (Lock) tracked.Handle.MonitoredNode?.Add(item);
+                // Re-resolve the node by NodeId rather than trusting the handle (review 01 L2): the fan-out
+                // the item came off belongs to the NodeState that existed when it was stood down, and a
+                // rebuild or surgical remove in between has tombstoned that node — then the item was already
+                // dropped from this map, so a mismatch here is a race with one; tell the client and let go.
+                bool reattached;
+                lock (Lock)
+                {
+                    reattached = ResolveNodeState(MapKey(nodeId)) is { } liveNode
+                        && ReferenceEquals(liveNode, tracked.Handle.Node);
+                    if (reattached) tracked.Handle.MonitoredNode?.Add(item);
+                    else _liveItems.TryRemove(item, out _);
+                }
+
+                Interlocked.Decrement(ref _suppressedItems);
+                if (!reattached)
+                {
+                    TellNodeRemoved(item, now);
+                    continue;
+                }
+
                tracked.Suppressed = false;
                restored++;
                continue;
@@ -4832,8 +5053,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                new ServiceResult(StatusCodes.BadUserAccessDenied),
                ignoreFilters: true);

-            lock (Lock) tracked.Handle.MonitoredNode?.Remove(item);
+            lock (Lock)
+            {
+                // The fan-out list may have gone null if every other item on the node was deleted meanwhile.
+                if (tracked.Handle.MonitoredNode is { DataChangeMonitoredItems: not null } fanOut) fanOut.Remove(item);
+            }
            tracked.Suppressed = true;
+            Interlocked.Increment(ref _suppressedItems);
            stoodDown++;
        }

@@ -4905,8 +5131,22 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            {
                if (!_eventItems.TryGetValue(item, out var current) || !ReferenceEquals(current, tracked)) continue;

+                // Re-resolve the notifier by NodeId (review 01 L2): re-attaching to the handle's NodeState
+                // after a rebuild would bind the item to a dead node. A notifier that is gone, or has been
+                // re-created, cannot be the one this item was stood down from — drop the item instead.
+                if (permitted
+                    && (ResolveNodeState(MapKey(tracked.Handle.NodeId)) is not { } live || !ReferenceEquals(live, tracked.Handle.Node)))
+                {
+                    _eventItems.TryRemove(item, out _);
+                    Interlocked.Decrement(ref _suppressedItems);
+                    ReleaseComponentCacheReference(item);
+                    continue;
+                }
+
                SubscribeToEvents(SystemContext, tracked.Handle.Node, item, unsubscribe: !permitted);
                tracked.Suppressed = !permitted;
+                if (permitted) Interlocked.Decrement(ref _suppressedItems);
+                else Interlocked.Increment(ref _suppressedItems);
            }

            if (permitted) restored++;