From 7783fadae1b8bb54a58c73a9933ea36eabc71c22 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 12:51:03 -0500 Subject: [PATCH] =?UTF-8?q?fix(opcua):=20#855=20review=203=20=E2=80=94=20p?= =?UTF-8?q?lain=20refresh=20skips=20removed=20conditions;=20per-hop=20fail?= =?UTF-8?q?ure=20isolation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Both plain ConditionRefresh paths (Server object and folder) skip a condition a deploy removed or replaced between the off-Lock walk and the queueing (the SDK base did not either; only the cut path checked). - A failure delivering at one notifier hop is logged and the walk continues to the remaining hops; a failure deciding the Server-object items withholds only them. - Test: a condition removed during a plain refresh (folder + Server object). Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- .../OtOpcUaNodeManager.cs | 31 ++++++++++++- .../FolderConditionRefreshWireTests.cs | 43 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 74104d3d6..f6af07d1a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1651,7 +1651,19 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 } // Re-checked: a stall may have outlived the condition (the liveness check above ran before it). - if (IsLiveCondition(condition)) DeliverToNotifierItems(monitored, e); + if (IsLiveCondition(condition)) + { + // #855 review 3: one hop's failure must not cost the hops after it (before #855 this hop was a channel + // write, which cannot throw; the SDK isolated failures per item). + try + { + DeliverToNotifierItems(monitored, e); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "OtOpcUaNodeManager: failed to deliver a condition event at notifier {NodeId}", node.NodeId); + } + } } else { @@ -5815,7 +5827,18 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 // through its OLD equipment folder (#808). if (!IsLiveCondition(node)) return; - var scopes = AclEnforced ? ConditionVisibilityScopes(node) : null; + List? scopes; + try + { + scopes = AclEnforced ? ConditionVisibilityScopes(node) : null; + } + catch (Exception ex) + { + // #855 review 3: withheld from the Server-object items (fail closed), but the walk goes on to the folders. + _logger.LogWarning(ex, "OtOpcUaNodeManager: failed to decide a condition event for Server-object items ({NodeId})", node.NodeId); + return; + } + var reportingSession = (context as ISessionSystemContext)?.SessionId; var deferred = e is ConditionEventSnapshot { Deferred: true } d ? d : null; @@ -6595,6 +6618,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 foreach (var (e, _, scopes) in replay) { + // #855 review 3: never replay a condition a deploy removed or replaced since the walk collected it. + if (ConditionOf(e) is AlarmConditionState walked && !IsLiveCondition(walked)) continue; if (scopes is not null && !IsConditionVisibleTo(item, scopes)) continue; if (ServiceResult.IsBad(ValidateEventRolePermissions(item, e))) continue; item.QueueEvent(e); @@ -6878,6 +6903,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 DirectRefreshProbe?.Invoke(false, Monitor.IsEntered(Lock)); foreach (var e in events) { + // #855 review 3: never replay a condition a deploy removed or replaced since the walk collected it. + if (ConditionOf(e) is AlarmConditionState walked && !IsLiveCondition(walked)) continue; if (!IsDirectRefreshEventAllowed(item, e, enforced)) continue; item.QueueEvent(e); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderConditionRefreshWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderConditionRefreshWireTests.cs index 59287d809..b6deba5e2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderConditionRefreshWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderConditionRefreshWireTests.cs @@ -489,6 +489,49 @@ public sealed class FolderConditionRefreshWireTests after.ShouldBeEmpty("and none of its deferred events follows"); } + /// + /// #855 review 3: the plain refresh path (nothing deferred — the only path production takes) collects live + /// conditions off Lock and queues them afterwards; a condition removed in between must not be replayed. + /// + /// Refresh a Server-object item instead of a folder item. + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task A_condition_removed_during_a_plain_refresh_is_not_replayed(bool serverObject) + { + var ct = TestContext.Current.CancellationToken; + await using var rig = await ServerEventRig.StartAsync("Refresh855RemovePlain", encrypted: false, ct); + rig.KeyRoles = []; + rig.MaterialiseNative(); + rig.Enforce(false, [ServerEventRig.Key()], KeyEquipmentGrant(EqA)); + + using var session = await rig.ConnectAsync(anonymous: false, ct); + var events = new RecordedEvents(); + var subscription = await RecordedEvents.SubscribeAsync(session, events, ct); + var item = RecordedEvents.AddEventItem(subscription, serverObject ? ObjectIds.Server : ServerEventRig.UnsNodeId(session, EqA)); + await subscription.ApplyChangesAsync(ct); + + rig.Project(native: true, Active("seed")); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "seed")); + + var removed = 0; + void Remove(bool cut, bool _) + { + if (!cut && Interlocked.Exchange(ref removed, 1) == 0) + rig.Nm.RemoveAlarmConditionNode(ServerEventRig.RawAlarmPath, AddressSpaceRealm.Raw).ShouldBeTrue(); + } + + if (serverObject) rig.Nm.ServerObjectRefreshProbe = Remove; + else rig.Nm.DirectRefreshProbe = Remove; + await session.CallAsync(ObjectTypeIds.ConditionType, MethodIds.ConditionType_ConditionRefresh, ct, subscription.Id); + await RecordedEvents.WaitUntilAsync(() => events.HasType(item, ObjectTypeIds.RefreshEndEventType)); + rig.Nm.ServerObjectRefreshProbe = null; + rig.Nm.DirectRefreshProbe = null; + + Volatile.Read(ref removed).ShouldBe(1, "the plain path ran"); + Window(events.Of(item)).Replayed.ShouldBeEmpty("the condition was removed before the replay was queued"); + } + /// /// #855 review: a delivery that fails inline (here: a hop that throws) is over, not suspended — it must not be booked /// as a deferral episode (which moved the episode a concurrent plain refresh checks, and cleared cuts under the