feat(opcua): make MaterialiseAlarmCondition idempotent for same-kind re-applies (R2-07 T4a)

archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json

@@ -27,7 +27,7 @@
     {
       "id": "T4a",
       "subject": "Phase 1: idempotent MaterialiseAlarmCondition (skip-if-present-and-same-kind) (high-risk)",
-      "status": "pending",
+      "status": "completed",
       "blockedBy": [
         "T1"
       ]

src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs

@@ -591,6 +591,17 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
 
         lock (Lock)
         {
+            // R2-07 T4a: idempotent skip-if-present-and-same-kind. On a PURE-ADD apply,
+            // MaterialiseScriptedAlarms + the native-alarm materialise re-run over the FULL composition, so
+            // an existing enabled condition is re-materialised with the SAME id + kind. Skip it — KEEP the
+            // existing AlarmConditionState instance alive so every MonitoredItem on that condition node
+            // survives the deploy (the drop-and-recreate below would otherwise kill them). A genuine
+            // re-severity/type change arrives as a ChangedAlarms delta ⇒ full rebuild (the map is cleared
+            // first), so it never reaches this path with a stale-but-present entry; the drop-and-recreate
+            // stays ONLY for the kind-swap (native↔scripted) case, which flips the native flag.
+            if (_alarmConditions.ContainsKey(alarmNodeId) && _nativeAlarmNodeIds.Contains(alarmNodeId) == isNative)
+                return;
+
             // Idempotent: drop any prior node for this id so a re-materialise (e.g. changed
             // type/severity on redeploy) reflects cleanly instead of leaking the old node.
             if (_alarmConditions.TryRemove(alarmNodeId, out var existing))

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

@@ -0,0 +1,132 @@
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
+
+/// <summary>
+///     R2-07 T4a — <see cref="OtOpcUaNodeManager.MaterialiseAlarmCondition"/> must be idempotent for a
+///     same-id + same-kind re-apply: it KEEPS the existing <c>AlarmConditionState</c> instance instead of
+///     dropping-and-recreating it. This is what preserves a client's MonitoredItems on an alarm-condition
+///     node across a PURE-ADD deploy (the MaterialiseScriptedAlarms pass re-runs over the whole composition
+///     every apply, re-touching every existing enabled condition). A genuine kind-swap (native↔scripted)
+///     still recreates so the flip takes effect.
+/// </summary>
+public sealed class NodeManagerAlarmIdempotentMaterialiseTests : IDisposable
+{
+    private static CancellationToken Ct => TestContext.Current.CancellationToken;
+
+    private readonly string _pkiRoot = Path.Combine(
+        Path.GetTempPath(),
+        $"otopcua-alarm-idempotent-{Guid.NewGuid():N}");
+
+    /// <summary>Re-materialising the SAME alarm id + kind returns the SAME instance (reference-equal) — the
+    /// condition node is not torn down, so subscriptions on it survive.</summary>
+    [Trait("Category", "Unit")]
+    [Fact]
+    public async Task Re_materialise_same_id_and_kind_keeps_the_same_instance()
+    {
+        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", 700, isNative: false);
+        var first = nm.TryGetAlarmCondition("alm-1");
+        first.ShouldNotBeNull();
+
+        // Same id + same kind ⇒ skip-if-present: the existing instance is kept.
+        nm.MaterialiseAlarmCondition("alm-1", "eq-1", "HighTemp", "OffNormalAlarm", 700, isNative: false);
+        var second = nm.TryGetAlarmCondition("alm-1");
+
+        second.ShouldBeSameAs(first);
+
+        await host.DisposeAsync();
+    }
+
+    /// <summary>A kind-swap (scripted → native) on the same id still RECREATES the node so the native flag
+    /// flips — the drop-and-recreate is preserved for the kind-swap case.</summary>
+    [Trait("Category", "Unit")]
+    [Fact]
+    public async Task Kind_swap_recreates_and_flips_native_flag()
+    {
+        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", 700, isNative: false);
+        var scripted = nm.TryGetAlarmCondition("alm-1");
+        nm.IsNativeAlarmNode("alm-1").ShouldBeFalse();
+
+        // Same id but the OTHER kind ⇒ recreate (a different instance) and the native flag is now set.
+        nm.MaterialiseAlarmCondition("alm-1", "eq-1", "HighTemp", "OffNormalAlarm", 700, isNative: true);
+        var native = nm.TryGetAlarmCondition("alm-1");
+
+        native.ShouldNotBeSameAs(scripted);
+        nm.IsNativeAlarmNode("alm-1").ShouldBeTrue();
+
+        await host.DisposeAsync();
+    }
+
+    /// <summary>After a full <see cref="OtOpcUaNodeManager.RebuildAddressSpace"/> cleared the maps, the next
+    /// materialise creates a FRESH instance (the skip-if-present guard sees no entry) — so a re-severity
+    /// arriving via the rebuild path is reflected.</summary>
+    [Trait("Category", "Unit")]
+    [Fact]
+    public async Task Materialise_after_rebuild_creates_fresh_instance()
+    {
+        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", 700, isNative: false);
+        var before = nm.TryGetAlarmCondition("alm-1");
+
+        nm.RebuildAddressSpace();
+        nm.TryGetAlarmCondition("alm-1").ShouldBeNull();
+
+        nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1");
+        nm.MaterialiseAlarmCondition("alm-1", "eq-1", "HighTemp", "OffNormalAlarm", 700, isNative: false);
+        var after = nm.TryGetAlarmCondition("alm-1");
+
+        after.ShouldNotBeNull();
+        after.ShouldNotBeSameAs(before);
+
+        await host.DisposeAsync();
+    }
+
+    private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync()
+    {
+        var host = new OpcUaApplicationHost(
+            new OpcUaApplicationHostOptions
+            {
+                ApplicationName = "OtOpcUa.AlarmIdempotentTest",
+                ApplicationUri = $"urn:OtOpcUa.AlarmIdempotentTest:{Guid.NewGuid():N}",
+                OpcUaPort = AllocateFreePort(),
+                PublicHostname = "localhost",
+                PkiStoreRoot = _pkiRoot,
+            },
+            Microsoft.Extensions.Logging.Abstractions.NullLogger<OpcUaApplicationHost>.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;
+    }
+
+    /// <summary>Cleans up the PKI root directory.</summary>
+    public void Dispose()
+    {
+        if (Directory.Exists(_pkiRoot))
+        {
+            try { Directory.Delete(_pkiRoot, recursive: true); }
+            catch { /* best-effort cleanup */ }
+        }
+    }
+}