dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(runtime,opcuaserver): review fast-follows — event-filter throw test, loud MapKey fallback, log shape

Browse Source 
Accumulated fast-follows from the Task 1-11 reviews. No behaviour change on the
value path.

- Strengthen OpcUaPublishActorBatchTests.Sink_throw_is_swallowed_and_logged. The
  survival assertion alone did NOT prove the catch exists: Akka's default
  supervision RESTARTS a throwing actor and the restart preserves the mailbox, so
  the follow-up message landed either way and the test passed against a build with
  no catch at all. It now asserts via EventFilter that the exception NEVER reaches
  supervision (Expect(0) on the InvalidOperationException) and that the handler's
  own Warning IS logged, keeping the survival assertion. Everything that must have
  happened goes inside the Expect(0) action — it counts matches at the end of the
  action, so an assertion left outside would race the actor and pass vacuously.
  Verified falsifiable: deleting the catch turns the test red.

- MapKey(NodeId): the non-string identifier branch collapsed to a string.Empty key
  silently. Such a key cannot match anything the manager stored, so the caller sees
  an unexplained miss rather than an error. It now logs a Utils.LogWarning naming
  the seam. Every current call site is provably string-only, so this should never
  fire.

- The two WireAlarmNotifiers Utils.LogInfo sites interpolated the RealmKey struct,
  rendering a raw struct dump. Interpolate .Id and .Realm as separate placeholders
  so log-grep patterns stay consistent with the file's other sites.

- Stale comments: the _eventNotifierSources key is a RealmKey now, not a
  namespace-qualified NodeId string; and three comments still named
  DriverHostActor.ForwardToMux, which Task 8 replaced with ForwardBatchToMux.
This commit is contained in:
Joseph Doherty
2026-08-15 13:10:43 -04:00
parent ec6f8bb0ea
commit 44b99a9939
4 changed files with 49 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+24 -8
View File
@@ -142,11 +142,24 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// <summary>Build the internal map key for an already-resolved SDK <see cref="NodeId"/> (the SDK-facing
/// seams). Equal to <see cref="MapKey(AddressSpaceRealm, string)"/> for the same node. The SDK-facing
/// seams only ever pass nodes this manager minted (string <c>s=</c> identifiers); a non-string
/// identifier falls back to its string form rather than throwing.</summary>
/// identifier falls back to its string form rather than throwing — and says so loudly, because such a
/// key cannot match anything the manager stored, so the lookup would silently miss.</summary>
/// <param name="nodeId">The resolved SDK node id.</param>
/// <returns>The realm-qualified map key.</returns>
private RealmKey MapKey(NodeId nodeId) =>
new(RealmOf(nodeId), nodeId.Identifier as string ?? nodeId.Identifier?.ToString() ?? string.Empty);
private RealmKey MapKey(NodeId nodeId)
{
if (nodeId.Identifier is string s) return new RealmKey(RealmOf(nodeId), s);
// Unreachable by construction — every call site passes a node this manager minted with a string
// `s=` identifier. If it ever fires, the derived key resolves to nothing and the caller sees an
// unexplained miss, so name the seam rather than collapsing quietly.
#pragma warning disable CS0618 // Utils.LogWarning is [Obsolete] in favour of an ITelemetryContext this manager doesn't carry.
Utils.LogWarning(
"OtOpcUaNodeManager.MapKey: non-string NodeId identifier reached the map-key seam (node {0}, identifier type {1}) — the derived key will not resolve",
nodeId, nodeId.Identifier?.GetType().Name ?? "null");
#pragma warning restore CS0618
return new RealmKey(RealmOf(nodeId), nodeId.Identifier?.ToString() ?? string.Empty);
}
/// <summary>Gets the count of variable nodes currently managed.</summary>
public int VariableCount => _variables.Count;
@@ -1262,7 +1275,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// ReportEvent catch below for the same pattern). Utils.LogInfo is [Obsolete] in 1.5.378 in
// favour of an ITelemetryContext this manager doesn't wire — suppress the deprecation.
#pragma warning disable CS0618 // Type or member is obsolete
Utils.LogInfo("OtOpcUaNodeManager.WireAlarmNotifiers: condition {0} not materialised — skipping notifier wiring", alarmKey);
// Interpolate the key's PARTS, not the RealmKey struct — a struct dump would render a shape
// no log-grep pattern in this file matches.
Utils.LogInfo("OtOpcUaNodeManager.WireAlarmNotifiers: condition {0} (realm {1}) not materialised — skipping notifier wiring",
alarmKey.Id, alarmKey.Realm);
#pragma warning restore CS0618
return;
}
@@ -1297,8 +1313,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
{
// Referencing-equipment folder not (yet) materialised — skip this one (logged, never thrown).
#pragma warning disable CS0618 // Type or member is obsolete
Utils.LogInfo("OtOpcUaNodeManager.WireAlarmNotifiers: notifier folder {0} for condition {1} not present — skipped",
folderNodeId, alarmKey);
Utils.LogInfo("OtOpcUaNodeManager.WireAlarmNotifiers: notifier folder {0} for condition {1} (realm {2}) not present — skipped",
folderNodeId, alarmKey.Id, alarmKey.Realm);
#pragma warning restore CS0618
continue;
}
@@ -1941,8 +1957,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// A historian is wired: advertise event history on this notifier and register it as a source.
// The equipment-folder NodeId identifier IS the equipment id IS the ReadEventsAsync sourceName.
folder.EventNotifier = (byte)(folder.EventNotifier | EventNotifiers.HistoryRead);
// Key by the full (namespace-qualified) NodeId string so a Raw and a UNS folder that share the
// same bare id are distinct sources; the VALUE stays the bare id — that is the historian
// Key by the realm-qualified RealmKey built from the NodeId so a Raw and a UNS folder that share
// the same bare id are distinct sources; the VALUE stays the bare id — that is the historian
// ReadEventsAsync sourceName (== the equipment id).
var sourceName = folder.NodeId.Identifier?.ToString() ?? string.Empty;
_eventNotifierSources[MapKey(folder.NodeId)] = sourceName;
src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs
+1 -1
View File
@@ -507,7 +507,7 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers
failedNodes += _applier.MaterialiseScriptedAlarms(composition);
// Equipment-namespace tags get their own pass: ensures each signal's Variable (and any
// FolderPath sub-folder) exists under its already-materialised equipment folder so
// clients can browse them. Live values are pushed by DriverHostActor.ForwardToMux after
// clients can browse them. Live values are pushed by DriverHostActor.ForwardBatchToMux after
// each subscription cycle; variables show BadWaitingForInitialData only until the first
// publish interval fires.
failedNodes += _applier.MaterialiseEquipmentTags(composition);
tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/CalculationDependencyFlowTests.cs
+2 -2
View File
@@ -17,7 +17,7 @@ namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers;
/// <see cref="CalculationDriver"/> path (not merely that <see cref="IDependencyConsumer"/> exists).
/// A source value published to the mux is fed into the calc driver, which computes and publishes a
/// value; and a calc tag reading ANOTHER calc tag's output (calc-of-calc) recomputes when the first
/// tag's output re-enters the mux — the exact re-entry the host's <c>ForwardToMux</c> performs, mirrored
/// tag's output re-enters the mux — the exact re-entry the host's <c>ForwardBatchToMux</c> performs, mirrored
/// here by looping the driver's <see cref="ISubscribable.OnDataChange"/> back into the mux.
/// </summary>
public sealed class CalculationDependencyFlowTests : RuntimeActorTestBase
@@ -58,7 +58,7 @@ public sealed class CalculationDependencyFlowTests : RuntimeActorTestBase
var published = new ConcurrentQueue<DataChangeEventArgs>();
var mux = Sys.ActorOf(DependencyMuxActor.Props(), "dep-mux");
// Loop the driver's outputs back into the mux exactly as DriverHostActor.ForwardToMux does — this
// Loop the driver's outputs back into the mux exactly as DriverHostActor.ForwardBatchToMux does — this
// is what makes calc-of-calc chains work.
driver.OnDataChange += (_, e) =>
{
tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorBatchTests.cs
+22 -8
View File
@@ -51,7 +51,12 @@ public sealed class OpcUaPublishActorBatchTests : RuntimeActorTestBase
}
/// <summary>A sink that throws out of WriteValues is swallowed and logged — the actor survives and keeps
/// processing (proved by a follow-up message landing), rather than restarting and dropping traffic.</summary>
/// processing, rather than restarting and dropping traffic.
/// <para>The survival assertion alone does NOT prove the catch exists: Akka's default supervision
/// RESTARTS a throwing actor and the restart preserves the mailbox, so a follow-up message lands either
/// way. The load-bearing assertions are therefore the two event filters — the exception must never reach
/// supervision (no Error carrying it is ever logged), and the handler's own Warning must be logged so the
/// fault is swallowed but not silent. Deleting the catch turns BOTH red.</para></summary>
[Fact]
public void Sink_throw_is_swallowed_and_logged()
{
@@ -59,16 +64,25 @@ public sealed class OpcUaPublishActorBatchTests : RuntimeActorTestBase
var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink));
var t0 = new DateTime(2026, 8, 15, 10, 0, 0, DateTimeKind.Utc);
actor.Tell(new OpcUaPublishActor.AttributeValueBatchUpdate(new[]
// Expect(0, …) counts matches at the END of the action, so everything that must have happened by
// then goes INSIDE it — otherwise the absence assertion races the actor and passes vacuously.
EventFilter.Exception<InvalidOperationException>().Expect(0, () =>
{
new ValueWrite("Plant/Modbus/dev1/Speed", 1, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw),
}));
// The handler caught it AND said so: its own Warning, not supervision's Error.
EventFilter.Warning(contains: "sink.WriteValues threw").ExpectOne(() =>
actor.Tell(new OpcUaPublishActor.AttributeValueBatchUpdate(new[]
{
new ValueWrite("Plant/Modbus/dev1/Speed", 1, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw),
})));
// Follow-up on the untouched per-item path: it lands only if the actor is still alive and pumping.
actor.Tell(new OpcUaPublishActor.AttributeValueUpdate(
"Plant/Modbus/dev1/Temp", 2, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw));
// Survival: a follow-up on the untouched per-item path lands only if the actor is still pumping.
actor.Tell(new OpcUaPublishActor.AttributeValueUpdate(
"Plant/Modbus/dev1/Temp", 2, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw));
AwaitAssert(() => sink.SingleWrites.Count.ShouldBe(1), PresenceBudget);
});
AwaitAssert(() => sink.SingleWrites.Count.ShouldBe(1), PresenceBudget);
sink.Threw.ShouldBeTrue();
}