From f8406d348c9afe22738119ab1692909dfac9ca9b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 26 Jun 2026 07:06:43 -0400 Subject: [PATCH] fix(otopcua): report NodeAdded model-change outside the node Lock --- .../OtOpcUaNodeManager.cs | 37 +++++++++++-------- .../NodeManagerModelChangeOnAddTests.cs | 24 ++++++++++++ 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 985a6aab..a358d7d7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1576,31 +1576,38 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// served Equipment address space (Tasks 4/5), an attribute notification alone is invisible to a /// subscribed client — only a model-change event tells it the address space grew. /// - /// Built AND reported under Lock (like ) and wrapped in - /// try/catch so it is tolerant when eventing is disabled / there are no monitored items / the - /// server is shutting down — the same swallow-and-log tolerance as the write-revert path - /// (). The nodes have already been materialised, so a surprise from - /// the event path MUST NOT propagate out of this announcement. + /// The event is built under Lock but reported AFTER the lock is released, mirroring + /// / : + /// Server.ReportEvent re-enters the server's own subscription/event path, so holding the node + /// Lock across it risks a lock-order inversion with a client that has event subscriptions. + /// The report is wrapped in try/catch so it is tolerant when eventing is disabled / there are no + /// monitored items / the server is shutting down — the same swallow-and-log tolerance as the + /// write-revert path (). The nodes have already been materialised, so + /// a surprise from the event path MUST NOT propagate out of this announcement. /// /// /// The folder-scoped node id of the parent under which nodes were added. public void RaiseNodesAddedModelChange(string affectedNodeId) { ArgumentException.ThrowIfNullOrEmpty(affectedNodeId); + GeneralModelChangeEventState e; lock (Lock) { - try - { - Server.ReportEvent(SystemContext, BuildNodesAddedModelChange(affectedNodeId)); - } - catch (Exception ex) - { - // Model-change reporting disabled / no monitored items / server shutting down ⇒ ReportEvent may - // no-op or throw; either way the node add already stands. Log to the SDK trace, don't rethrow. + e = BuildNodesAddedModelChange(affectedNodeId); + } + // Report OUTSIDE Lock — Server.ReportEvent re-enters the server's own subscription/event path; holding + // Lock across it risks a lock-order inversion (mirrors ReportNodeShapeChangedEvent). + try + { + Server.ReportEvent(SystemContext, e); + } + catch (Exception ex) + { + // Model-change reporting disabled / no monitored items / server shutting down ⇒ ReportEvent may + // no-op or throw; either way the node add already stands. Log to the SDK trace, don't rethrow. #pragma warning disable CS0618 // Utils.LogError is [Obsolete] in favour of an ITelemetryContext this manager doesn't carry. - Utils.LogError(ex, "OtOpcUaNodeManager: failed to report GeneralModelChangeEvent(NodeAdded) for {0}", affectedNodeId); + Utils.LogError(ex, "OtOpcUaNodeManager: failed to report GeneralModelChangeEvent(NodeAdded) for {0}", affectedNodeId); #pragma warning restore CS0618 - } } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerModelChangeOnAddTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerModelChangeOnAddTests.cs index 1801794b..7b942b55 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerModelChangeOnAddTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerModelChangeOnAddTests.cs @@ -60,6 +60,30 @@ public sealed class NodeManagerModelChangeOnAddTests : IDisposable await host.DisposeAsync(); } + /// For an affected id that is not (yet) materialised, the built event still announces NodeAdded but + /// its AffectedType falls back to (a valid Part 3 "type not applicable") — the + /// documented fallback of , locked in as an + /// invariant. + [Trait("Category", "Unit")] + [Fact] + public async Task Built_event_for_unknown_id_falls_back_to_null_AffectedType() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + // No EnsureFolder/EnsureVariable for this id — it is not in the node maps. + var e = nm.BuildNodesAddedModelChange("eq-unknown"); + + e.ShouldNotBeNull(); + e.Changes.ShouldNotBeNull(); + var changes = e.Changes.Value; + changes.Length.ShouldBe(1); + changes[0].Verb.ShouldBe((byte)ModelChangeStructureVerbMask.NodeAdded); + changes[0].AffectedType.ShouldBe(NodeId.Null); + + await host.DisposeAsync(); + } + /// Raising the announcement is tolerant: callable before any nodes exist (unknown affected id ⇒ /// AffectedType defaults to null, still a valid Part 3 change) AND after they are materialised, and never /// throws even when the event path reaches no monitored items (same tolerance as the write-revert path).