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 8d8e6167..17b97de4 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 @@ -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" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 23c08e95..1a480a9f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/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)) diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmIdempotentMaterialiseTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmIdempotentMaterialiseTests.cs new file mode 100644 index 00000000..c34a3151 --- /dev/null +++ b/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; + +/// +/// R2-07 T4a — must be idempotent for a +/// same-id + same-kind re-apply: it KEEPS the existing AlarmConditionState 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. +/// +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}"); + + /// 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. + [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(); + } + + /// 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. + [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(); + } + + /// After a full 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. + [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.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 */ } + } + } +}