From b31d7cb03ffd2e2363ffde215d1433ff83811632 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Wed, 10 Jun 2026 19:26:01 -0400
Subject: [PATCH] fix(scripted-alarms): lock CreateVariable +
 RemoveRootNotifier on rebuild (T14 review)

---
 .../OtOpcUaNodeManager.cs                     | 35 +++++++++++++++----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
index 56a47fa2..3d29a556 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
@@ -30,8 +30,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
     private readonly ConcurrentDictionary<string, FolderState> _folders = new(StringComparer.Ordinal);
     private readonly ConcurrentDictionary<string, AlarmConditionState> _alarmConditions = new(StringComparer.Ordinal);
     /// <summary>Folders we have already promoted to event-notifiers + registered as root notifiers,
-    /// so repeated <see cref="MaterialiseAlarmCondition"/> calls don't double-add (idempotent guard).</summary>
-    private readonly HashSet<NodeId> _notifierFolders = new();
+    /// so repeated <see cref="MaterialiseAlarmCondition"/> calls don't double-add (idempotent guard).
+    /// Keyed by NodeId → the actual <see cref="FolderState"/> so <see cref="RebuildAddressSpace"/> can
+    /// pass the folder to <c>RemoveRootNotifier</c> on teardown.</summary>
+    private readonly Dictionary<NodeId, FolderState> _notifierFolders = new();
     private FolderState? _root;
 
     /// <summary>Initializes a new instance of the <see cref="OtOpcUaNodeManager"/> class with the OPC UA server and configuration.</summary>
@@ -69,10 +71,17 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
     public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc)
     {
         ArgumentException.ThrowIfNullOrEmpty(nodeId);
-        var variable = _variables.GetOrAdd(nodeId, CreateVariable);
 
         lock (Lock)
         {
+            // CreateVariable mutates the SDK address space (_root.AddChild + AddPredefinedNode),
+            // so it MUST run under Lock — the SDK's subscription/ConditionRefresh threads take it too.
+            if (!_variables.TryGetValue(nodeId, out var variable))
+            {
+                variable = CreateVariable(nodeId);
+                _variables[nodeId] = variable;
+            }
+
             variable.Value = value;
             variable.StatusCode = StatusFromQuality(quality);
             variable.Timestamp = sourceTimestampUtc;
@@ -117,9 +126,15 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
         }
 
         // Fallback: alarm not materialised as a real condition — keep the legacy bool[2] variable.
-        var variable = _variables.GetOrAdd(alarmNodeId, CreateVariable);
         lock (Lock)
         {
+            // CreateVariable mutates the SDK address space, so it MUST run under Lock (see WriteValue).
+            if (!_variables.TryGetValue(alarmNodeId, out var variable))
+            {
+                variable = CreateVariable(alarmNodeId);
+                _variables[alarmNodeId] = variable;
+            }
+
             variable.Value = new[] { active, acknowledged };
             variable.StatusCode = StatusCodes.Good;
             variable.Timestamp = sourceTimestampUtc;
@@ -230,7 +245,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
     /// alarm condition has a notifier path to the Server object for T16's event propagation.</summary>
     private void EnsureFolderIsEventNotifier(FolderState folder)
     {
-        if (!_notifierFolders.Add(folder.NodeId)) return;
+        if (!_notifierFolders.TryAdd(folder.NodeId, folder)) return;
         folder.EventNotifier = EventNotifiers.SubscribeToEvents;
         AddRootNotifier(folder);
         folder.ClearChangeMasks(SystemContext, includeChildren: false);
@@ -240,7 +255,6 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
     /// <see cref="EventSeverity"/> enum buckets the SDK's <c>SetSeverity</c> expects.</summary>
     private static EventSeverity MapSeverity(int severity) => severity switch
     {
-        <= 0 => EventSeverity.Low,
         < 200 => EventSeverity.Low,
         < 400 => EventSeverity.MediumLow,
         < 600 => EventSeverity.Medium,
@@ -380,6 +394,15 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
             }
             _folders.Clear();
 
+            // Detach the Server↔folder HasNotifier ref for every promoted folder before dropping the
+            // guard, otherwise the rebuild leaks an orphaned root-notifier reference on the Server
+            // object. RemoveRootNotifier just severs that link, so its order relative to the folder
+            // teardown above doesn't matter — but it must run under this same Lock.
+            foreach (var folder in _notifierFolders.Values)
+            {
+                RemoveRootNotifier(folder);
+            }
+
             // Drop the notifier-folder guard so re-materialised alarms re-promote their (rebuilt)
             // equipment folders to event notifiers.
             _notifierFolders.Clear();