From 3de2cc3ba8c1e9355248519a824fc876f220466b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 17:02:21 -0400 Subject: [PATCH] fix(opcuaclient): alarm index survives namespace reorder; match condition node first; ack with real EventId (#619 review) The alarm index was keyed on ns=N strings resolved once at subscribe time, while an in-place SDK reconnect transfers the subscription without re-running it - a remote namespace reorder silently orphaned every alarm tag. Keys are now nsu= on both sides. Events match the authored nodeId against the event's ConditionId first, then its SourceNode, so several conditions on one source can be authored 1:1. Acknowledge passes the EventId of the event that named the remembered condition (paired, so node and EventId can never come from different conditions) instead of an empty one. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/OpcUaClient.md | 9 +- .../OpcUaClientDriver.cs | 88 +++++++++++++++---- .../OpcUaClientAlarmTests.cs | 68 +++++++++++++- 3 files changed, 142 insertions(+), 23 deletions(-) diff --git a/docs/drivers/OpcUaClient.md b/docs/drivers/OpcUaClient.md index 7ab1a015..9621370a 100644 --- a/docs/drivers/OpcUaClient.md +++ b/docs/drivers/OpcUaClient.md @@ -93,9 +93,12 @@ rather than surfacing as a runtime surprise. > **Native alarms (#619).** An alarm raw tag is `TagConfig` `{ "nodeId": "", "alarm": {...} }`. > The host subscribes with RawPaths and routes on (driver, RawPath), so the driver matches each event's -> `SourceNode` against the tag's authored `nodeId` and reports the **RawPath** as `ConditionId`; the upstream -> condition node (last seen per RawPath) is what `Acknowledge` is called on. `nodeId` must therefore be the -> event's *source*, not the condition node. docker-dev seeds an opc-plc driver (`MAIN-opcua`); the alarm tag is +> event's `ConditionId` (then its `SourceNode`) against the tag's authored `nodeId` and reports the **RawPath** as +> `ConditionId`. Author `nodeId` as the upstream **condition** node for a 1:1 mapping (required when one source carries +> several conditions, e.g. Hi + HiHi); authoring the **source** node folds every condition of that source into one +> tag. `Acknowledge` is called on the condition node of the last event seen for the RawPath **with that event's +> EventId** (Part 9 §5.7.3). Index keys are server-stable (`nsu=`), so a namespace reorder across a reconnect keeps +> matching. docker-dev seeds an opc-plc driver (`MAIN-opcua`); the alarm tag is > created only when `@OpcPlcAlarmSourceNodeId` is set in `docker-dev/seed/seed-clusters.sql`. > This driver does **not** implement `IRediscoverable` — there is no diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index ee833757..7808cdd6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -96,7 +96,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit /// #619: RawPath → the upstream condition NodeId last seen for it. The host acknowledges by /// RawPath (its only identity for the condition) but Part 9 Acknowledge must be called on the CONDITION /// node, which differs from the authored source node. - private readonly System.Collections.Concurrent.ConcurrentDictionary _alarmConditionByRawPath = + private readonly System.Collections.Concurrent.ConcurrentDictionary _alarmConditionByRawPath = new(StringComparer.Ordinal); private long _nextAlarmSubscriptionId; @@ -885,6 +885,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit catch { /* best-effort */ } } _alarmSubscriptions.Clear(); + _alarmConditionByRawPath.Clear(); await ReleaseSessionAsync(cancellationToken).ConfigureAwait(false); _namespaceMap = null; @@ -1643,12 +1644,20 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit // match in O(1) without re-parsing on every event. // #619: under v3 the host passes RawPaths, never upstream node ids. Translate each to the upstream // source id its tag authored (TagConfig.nodeId) so the per-event match compares like with like. + // Keys are server-stable (AlarmIndexKey) so a namespace reorder across an in-place SDK reconnect — + // which transfers this subscription without re-running SubscribeAlarmsAsync — cannot orphan them. var sessionForResolve = Session; + var mapForResolve = _namespaceMap; var sourceFilter = BuildAlarmSourceIndex(sourceNodeIds, raw => { - if (sessionForResolve is null || !TryResolveTagNode(sessionForResolve, raw, out var live)) return null; - return live.ToString(); + if (sessionForResolve is null) return null; + if (!TryResolveTagNode(sessionForResolve, raw, out var live) + && !NamespaceMap.TryResolve(sessionForResolve, raw, out live)) return null; + return AlarmIndexKey(mapForResolve, live); }); + // Forget ack targets for alarm tags no longer subscribed (removed/renamed RawPaths). + foreach (var known in _alarmConditionByRawPath.Keys) + if (!sourceNodeIds.Contains(known, StringComparer.Ordinal)) _alarmConditionByRawPath.TryRemove(known, out _); var subscription = new Subscription(telemetry: null!, new SubscriptionOptions { @@ -1778,18 +1787,30 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit var callRequests = new CallMethodRequestCollection(); foreach (var ack in acknowledgements) { - // ConditionId is an upstream condition-node id captured from the alarm event (not a - // tag RawPath), so it re-binds directly against the session via the static resolve. - // #619: the host acks by RawPath; Acknowledge must hit the upstream CONDITION node last seen for it. - var upstreamCondition = _alarmConditionByRawPath.TryGetValue(ack.ConditionId, out var seen) - ? seen : ack.ConditionId; - if (!NamespaceMap.TryResolve(session, upstreamCondition, out var conditionId)) continue; + // #619: the host acks by RawPath; Acknowledge must hit the upstream CONDITION node. + // The remembered target pairs the condition node with the EventId of the event that named it, + // so the ack can never mix one condition's node with another's EventId. Nothing seen yet ⇒ the + // tag's authored node (correct when the tag was authored on the condition node), else the id + // verbatim (pre-v3 callers that pass an upstream condition id directly). + NodeId conditionId; + byte[] eventId; + if (_alarmConditionByRawPath.TryGetValue(ack.ConditionId, out var seen)) + { + if (!NamespaceMap.TryResolve(session, seen.ConditionRef, out conditionId)) continue; + eventId = seen.EventId; + } + else + { + if (!TryResolveTagNode(session, ack.ConditionId, out conditionId) + && !NamespaceMap.TryResolve(session, ack.ConditionId, out conditionId)) continue; + eventId = Array.Empty(); + } callRequests.Add(new CallMethodRequest { ObjectId = conditionId, MethodId = MethodIds.AcknowledgeableConditionType_Acknowledge, InputArguments = [ - new Variant(Array.Empty()), // EventId — server-side best-effort; empty resolves to 'most recent' + new Variant(eventId), // Part 9 §5.7.3: the EventId being acknowledged (empty only when none seen) new Variant(new LocalizedText(ack.Comment ?? string.Empty)), ], }); @@ -1853,7 +1874,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit { if (args.NotificationValue is not EventFieldList efl) return; var e = ProjectEvent(efl.EventFields, sourceFilter, handle, - (raw, cond) => _alarmConditionByRawPath[raw] = cond); + n => AlarmIndexKey(_namespaceMap, n), (raw, target) => _alarmConditionByRawPath[raw] = target); if (e is not null) OnAlarmEvent?.Invoke(this, e); } @@ -1885,22 +1906,39 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit IReadOnlyList fields, IReadOnlyDictionary sourceIndex, IAlarmSubscriptionHandle handle, - Action rememberCondition) + Func keyOf, + Action rememberCondition) { if (fields.Count <= AlarmFieldConditionId) return null; - var sourceNode = fields[AlarmFieldSourceNode].Value?.ToString() ?? string.Empty; - string reported; - if (sourceIndex.Count == 0) reported = sourceNode; - else if (!sourceIndex.TryGetValue(sourceNode, out reported!)) return null; + // Pass-through (no authored refs) keeps the raw serialized ids; an authored index compares + // server-stable keys (see AlarmIndexKey), matching the CONDITION node first — the only way to tell + // several conditions on one source apart — and then the source node. + var sourceField = fields[AlarmFieldSourceNode].Value; + var conditionField = fields[AlarmFieldConditionId].Value; + string reported, upstreamCondition; + if (sourceIndex.Count == 0) + { + reported = sourceField?.ToString() ?? string.Empty; + upstreamCondition = conditionField?.ToString() ?? string.Empty; + } + else + { + upstreamCondition = conditionField is NodeId cn && !NodeId.IsNull(cn) ? keyOf(cn) : string.Empty; + var sourceKey = sourceField is NodeId sn && !NodeId.IsNull(sn) ? keyOf(sn) : string.Empty; + if (!(upstreamCondition.Length > 0 && sourceIndex.TryGetValue(upstreamCondition, out reported!)) + && !(sourceKey.Length > 0 && sourceIndex.TryGetValue(sourceKey, out reported!))) + return null; + } var eventType = fields[AlarmFieldEventType].Value?.ToString() ?? "BaseEventType"; var message = (fields[AlarmFieldMessage].Value as LocalizedText)?.Text ?? string.Empty; var severity = fields[AlarmFieldSeverity].Value is ushort sev ? sev : (ushort)0; var time = fields[AlarmFieldTime].Value is DateTime t ? t : DateTime.UtcNow; - var upstreamCondition = fields[AlarmFieldConditionId].Value?.ToString() ?? string.Empty; - if (sourceIndex.Count != 0 && upstreamCondition.Length > 0) rememberCondition(reported, upstreamCondition); + if (sourceIndex.Count != 0 && upstreamCondition.Length > 0) + rememberCondition(reported, new AlarmAckTarget( + upstreamCondition, fields[AlarmFieldEventId].Value as byte[] ?? Array.Empty())); return new AlarmEventArgs( SubscriptionHandle: handle, @@ -1912,6 +1950,20 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit SourceTimestampUtc: time); } + /// #619 — what Acknowledge needs for one RawPath: the upstream condition node (server-stable + /// reference) and the EventId of the event that named it. + /// Upstream condition NodeId, rendered by . + /// EventId of the most recent event seen for that condition. + internal readonly record struct AlarmAckTarget(string ConditionRef, byte[] EventId); + + /// #619 — the key both sides of the alarm index use: server-stable (nsu=) when a map is + /// available, so a remote namespace-table reorder across an in-place reconnect cannot orphan it. + /// The current session's namespace map, or null. + /// The NodeId to key. + /// The index key. + internal static string AlarmIndexKey(NamespaceMap? map, NodeId nodeId) => + map?.ToStableReference(nodeId) ?? nodeId.ToString() ?? string.Empty; + /// /// Map an OPC UA BaseEventType.Severity (1..1000) to our coarse-grained /// bucket. Thresholds match the OPC UA A&C Part 9 diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs index 186eee3d..e53a1dcf 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs @@ -113,7 +113,7 @@ public sealed class OpcUaClientAlarmTests var e = OpcUaClientDriver.ProjectEvent( EventFields("ns=3;s=Alarms.Trip1", "ns=3;s=Alarms.Trip1.Cond"), index, new FakeAlarmHandle(), - (raw, cond) => { rememberedRaw = raw; rememberedCond = cond; }); + n => n.ToString(), (raw, target) => { rememberedRaw = raw; rememberedCond = target.ConditionRef; }); e.ShouldNotBeNull(); e.ConditionId.ShouldBe("Plant/opc/plc/Trip1"); @@ -128,10 +128,74 @@ public sealed class OpcUaClientAlarmTests { var index = new Dictionary { ["ns=3;s=Alarms.Trip1"] = "Plant/opc/plc/Trip1" }; OpcUaClientDriver.ProjectEvent( - EventFields("ns=3;s=Other", "ns=3;s=Other.Cond"), index, new FakeAlarmHandle(), (_, _) => { }) + EventFields("ns=3;s=Other", "ns=3;s=Other.Cond"), index, new FakeAlarmHandle(), n => n.ToString(), (_, _) => { }) .ShouldBeNull(); } + /// + /// A remote namespace-table reorder across an in-place SDK reconnect must not orphan the alarm index: + /// it is built once at subscribe time, while events keep arriving on the (transferred) subscription with + /// the NEW namespace indexes. Keys are therefore server-stable (nsu=) on both sides. + /// + [Fact] + public void ProjectEvent_matches_across_a_namespace_reorder() + { + var before = new NamespaceTable(); before.Append("urn:other"); before.Append("urn:plc"); // urn:plc = 2 + var after = new NamespaceTable(); after.Append("urn:new"); after.Append("urn:other"); after.Append("urn:plc"); // urn:plc = 3 + var mapBefore = NamespaceMap.FromTable(before); + var mapAfter = NamespaceMap.FromTable(after); + + var index = OpcUaClientDriver.BuildAlarmSourceIndex( + ["Plant/opc/plc/Trip1"], _ => OpcUaClientDriver.AlarmIndexKey(mapBefore, new NodeId("Alarms.Trip1", 2))); + + var e = OpcUaClientDriver.ProjectEvent( + EventFields("ns=3;s=Alarms.Trip1", "ns=3;s=Alarms.Trip1.Cond"), index, new FakeAlarmHandle(), + n => OpcUaClientDriver.AlarmIndexKey(mapAfter, n), (_, _) => { }); + + e.ShouldNotBeNull(); + e.ConditionId.ShouldBe("Plant/opc/plc/Trip1"); + } + + /// + /// A source with several conditions (Hi + HiHi on one variable) can only be told apart by the CONDITION + /// node, so a tag authored on the condition node matches on the event's ConditionId — 1:1, no collapse. + /// + [Fact] + public void ProjectEvent_matches_a_tag_authored_on_the_condition_node() + { + var index = new Dictionary + { + ["ns=3;s=Tank.HiHi"] = "Plant/opc/plc/HiHi", + ["ns=3;s=Tank.Hi"] = "Plant/opc/plc/Hi", + }; + + var e = OpcUaClientDriver.ProjectEvent( + EventFields("ns=3;s=Tank", "ns=3;s=Tank.HiHi"), index, new FakeAlarmHandle(), n => n.ToString(), (_, _) => { }); + + e.ShouldNotBeNull(); + e.ConditionId.ShouldBe("Plant/opc/plc/HiHi"); + } + + /// + /// The ack target pairs the condition node WITH the EventId of the event that named it, so Acknowledge + /// carries the real EventId (Part 9 §5.7.3) instead of an empty one a strict server rejects + /// BadEventIdUnknown, and the two can never come from different conditions. + /// + [Fact] + public void ProjectEvent_remembers_the_condition_with_its_EventId() + { + var index = new Dictionary { ["ns=3;s=Alarms.Trip1"] = "Plant/opc/plc/Trip1" }; + OpcUaClientDriver.AlarmAckTarget? target = null; + + OpcUaClientDriver.ProjectEvent( + EventFields("ns=3;s=Alarms.Trip1", "ns=3;s=Alarms.Trip1.Cond"), index, new FakeAlarmHandle(), + n => n.ToString(), (_, t) => target = t); + + target.ShouldNotBeNull(); + target.Value.ConditionRef.ShouldBe("ns=3;s=Alarms.Trip1.Cond"); + target.Value.EventId.ShouldBe(new byte[] { 1 }); + } + private sealed class FakeAlarmHandle : IAlarmSubscriptionHandle { /// Gets the diagnostic identifier for this alarm handle.