fix(otopcua): report NodeAdded model-change outside the node Lock

This commit is contained in:
Joseph Doherty
2026-06-26 07:06:43 -04:00
parent 93f7586590
commit f8406d348c
2 changed files with 46 additions and 15 deletions
@@ -1576,22 +1576,30 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// served Equipment address space (Tasks 4/5), an attribute notification alone is invisible to a /// 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. /// subscribed client — only a model-change event tells it the address space grew.
/// <para> /// <para>
/// Built AND reported under <c>Lock</c> (like <see cref="ReportConditionEvent"/>) and wrapped in /// The event is built under <c>Lock</c> but reported AFTER the lock is released, mirroring
/// try/catch so it is tolerant when eventing is disabled / there are no monitored items / the /// <see cref="ReportNodeShapeChangedEvent"/> / <see cref="RevertOptimisticWriteIfNeeded"/>:
/// server is shutting down — the same swallow-and-log tolerance as the write-revert path /// <c>Server.ReportEvent</c> re-enters the server's own subscription/event path, so holding the node
/// (<see cref="ReportAuditEvent"/>). The nodes have already been materialised, so a surprise from /// <c>Lock</c> across it risks a lock-order inversion with a client that has event subscriptions.
/// the event path MUST NOT propagate out of this announcement. /// 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 (<see cref="ReportAuditEvent"/>). The nodes have already been materialised, so
/// a surprise from the event path MUST NOT propagate out of this announcement.
/// </para> /// </para>
/// </summary> /// </summary>
/// <param name="affectedNodeId">The folder-scoped node id of the parent under which nodes were added.</param> /// <param name="affectedNodeId">The folder-scoped node id of the parent under which nodes were added.</param>
public void RaiseNodesAddedModelChange(string affectedNodeId) public void RaiseNodesAddedModelChange(string affectedNodeId)
{ {
ArgumentException.ThrowIfNullOrEmpty(affectedNodeId); ArgumentException.ThrowIfNullOrEmpty(affectedNodeId);
GeneralModelChangeEventState e;
lock (Lock) lock (Lock)
{ {
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 try
{ {
Server.ReportEvent(SystemContext, BuildNodesAddedModelChange(affectedNodeId)); Server.ReportEvent(SystemContext, e);
} }
catch (Exception ex) catch (Exception ex)
{ {
@@ -1602,7 +1610,6 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
#pragma warning restore CS0618 #pragma warning restore CS0618
} }
} }
}
/// <summary>Build (but do not report) the Part 3 <c>GeneralModelChangeEvent</c> announcing that nodes were /// <summary>Build (but do not report) the Part 3 <c>GeneralModelChangeEvent</c> announcing that nodes were
/// added under <paramref name="affectedNodeId"/>. MIRRORS <see cref="BuildNodeShapeChangedEvent"/> exactly — /// added under <paramref name="affectedNodeId"/>. MIRRORS <see cref="BuildNodeShapeChangedEvent"/> exactly —
@@ -60,6 +60,30 @@ public sealed class NodeManagerModelChangeOnAddTests : IDisposable
await host.DisposeAsync(); await host.DisposeAsync();
} }
/// <summary>For an affected id that is not (yet) materialised, the built event still announces NodeAdded but
/// its AffectedType falls back to <see cref="NodeId.Null"/> (a valid Part 3 "type not applicable") — the
/// documented fallback of <see cref="OtOpcUaNodeManager.BuildNodesAddedModelChange"/>, locked in as an
/// invariant.</summary>
[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();
}
/// <summary>Raising the announcement is tolerant: callable before any nodes exist (unknown affected id ⇒ /// <summary>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 /// 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).</summary> /// throws even when the event path reaches no monitored items (same tolerance as the write-revert path).</summary>