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

fix(opcuaserver): chunked, per-item-isolated WriteValues with typed partial-failure outcome

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-15 12:32:44 -04:00
parent 8ef02506f7
commit 6bf1fb3754
7 changed files with 468 additions and 82 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerBatchWriteTests.cs
+168 -66
View File
@@ -7,9 +7,10 @@ using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
/// <summary>
/// Value-pipeline batching (Task 6) — <see cref="OtOpcUaNodeManager.WriteValues"/> applies a whole
/// batch under ONE acquisition of the manager-wide SDK <c>Lock</c> instead of one per value (the
/// per-value take was the steady-state throughput ceiling on the publish path).
/// Value-pipeline batching (Task 6) — <see cref="OtOpcUaNodeManager.WriteValues"/> applies a batch under
/// one acquisition of the manager-wide SDK <c>Lock</c> per 512-item chunk instead of one per value (the
/// per-value take was the steady-state throughput ceiling on the publish path; the chunk cap keeps the
/// first poll cycle's force-raise-everything batch from stalling every SDK service thread).
/// <para>These tests pin the <b>observable</b> contract rather than the locking: every item lands with
/// its own Value / StatusCode / Timestamp, an item for an undeclared NodeId is lazily created exactly
/// as <see cref="OtOpcUaNodeManager.WriteValue"/> does, and an empty batch does nothing at all.
@@ -35,78 +36,174 @@ public sealed class NodeManagerBatchWriteTests : IDisposable
public async Task WriteValues_lands_every_value()
{
var (host, server) = await BootAsync();
var nm = server.NodeManager!;
foreach (var name in new[] { "a", "b", "c" })
try
{
nm.EnsureVariable($"eq-1/{name}", parentFolderNodeId: null, displayName: name, dataType: "Double",
writable: false, realm: AddressSpaceRealm.Uns);
var nm = server.NodeManager!;
foreach (var name in new[] { "a", "b", "c" })
{
nm.EnsureVariable($"eq-1/{name}", parentFolderNodeId: null, displayName: name, dataType: "Double",
writable: false, realm: AddressSpaceRealm.Uns);
}
var t1 = new DateTime(2026, 8, 15, 10, 0, 0, DateTimeKind.Utc);
var t2 = new DateTime(2026, 8, 15, 10, 0, 1, DateTimeKind.Utc);
var t3 = new DateTime(2026, 8, 15, 10, 0, 2, DateTimeKind.Utc);
nm.WriteValues(new List<ValueWrite>
{
new("eq-1/a", 1.5d, OpcUaStatus.Good, t1, AddressSpaceRealm.Uns),
new("eq-1/b", null, OpcUaStatus.BadNoCommunication, t2, AddressSpaceRealm.Uns),
new("eq-1/c", 3.5d, 0x80740000u /* BadTypeMismatch */, t3, AddressSpaceRealm.Uns),
});
var a = nm.TryGetVariable("eq-1/a");
a.ShouldNotBeNull();
a!.Value.ShouldBe(1.5d);
a.StatusCode.ShouldBe((StatusCode)StatusCodes.Good);
a.Timestamp.ShouldBe(t1);
var b = nm.TryGetVariable("eq-1/b");
b.ShouldNotBeNull();
b!.Value.ShouldBeNull();
b.StatusCode.ShouldBe((StatusCode)StatusCodes.BadNoCommunication);
b.StatusCode.ShouldNotBe((StatusCode)StatusCodes.Bad,
"the batched path must hand the status sub-code to the SDK verbatim (#519)");
b.Timestamp.ShouldBe(t2);
var c = nm.TryGetVariable("eq-1/c");
c.ShouldNotBeNull();
c!.Value.ShouldBe(3.5d);
c.StatusCode.ShouldBe((StatusCode)StatusCodes.BadTypeMismatch);
c.Timestamp.ShouldBe(t3);
}
var t1 = new DateTime(2026, 8, 15, 10, 0, 0, DateTimeKind.Utc);
var t2 = new DateTime(2026, 8, 15, 10, 0, 1, DateTimeKind.Utc);
var t3 = new DateTime(2026, 8, 15, 10, 0, 2, DateTimeKind.Utc);
nm.WriteValues(new List<ValueWrite>
finally
{
new("eq-1/a", 1.5d, OpcUaStatus.Good, t1, AddressSpaceRealm.Uns),
new("eq-1/b", null, OpcUaStatus.BadNoCommunication, t2, AddressSpaceRealm.Uns),
new("eq-1/c", 3.5d, 0x80740000u /* BadTypeMismatch */, t3, AddressSpaceRealm.Uns),
});
var a = nm.TryGetVariable("eq-1/a");
a.ShouldNotBeNull();
a!.Value.ShouldBe(1.5d);
a.StatusCode.ShouldBe((StatusCode)StatusCodes.Good);
a.Timestamp.ShouldBe(t1);
var b = nm.TryGetVariable("eq-1/b");
b.ShouldNotBeNull();
b!.Value.ShouldBeNull();
b.StatusCode.ShouldBe((StatusCode)StatusCodes.BadNoCommunication);
b.StatusCode.ShouldNotBe((StatusCode)StatusCodes.Bad,
"the batched path must hand the status sub-code to the SDK verbatim (#519)");
b.Timestamp.ShouldBe(t2);
var c = nm.TryGetVariable("eq-1/c");
c.ShouldNotBeNull();
c!.Value.ShouldBe(3.5d);
c.StatusCode.ShouldBe((StatusCode)StatusCodes.BadTypeMismatch);
c.Timestamp.ShouldBe(t3);
await host.DisposeAsync();
await host.DisposeAsync();
}
}
/// <summary>A batch item naming a NodeId that was never declared lazily creates the variable, exactly
/// as the single-value <see cref="OtOpcUaNodeManager.WriteValue"/> create-on-first-write path does —
/// and it does so per realm, so a Raw and a UNS item sharing an <c>s=</c> id create two nodes.</summary>
/// and it does so PER REALM. Both items here carry the SAME <c>s=</c> id and differ only by realm, so a
/// key that ignored the realm would create one node and have the second item overwrite the first. The
/// assertion is therefore two nodes with DISTINCT namespace indices, not merely "both reads returned
/// something" (which a realm-blind map satisfies, since the shared key resolves for either lookup).</summary>
[Fact]
public async Task WriteValues_creates_missing_variables()
{
var (host, server) = await BootAsync();
var nm = server.NodeManager!;
nm.TryGetVariable("eq-1/lazy").ShouldBeNull();
nm.TryGetVariable("Plant/dev1/lazy", AddressSpaceRealm.Raw).ShouldBeNull();
var ts = new DateTime(2026, 8, 15, 11, 0, 0, DateTimeKind.Utc);
nm.WriteValues(new List<ValueWrite>
try
{
new("eq-1/lazy", 42, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
new("Plant/dev1/lazy", 7, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw),
});
var nm = server.NodeManager!;
var uns = nm.TryGetVariable("eq-1/lazy");
uns.ShouldNotBeNull();
uns!.Value.ShouldBe(42);
uns.StatusCode.ShouldBe((StatusCode)StatusCodes.Good);
uns.Timestamp.ShouldBe(ts);
const string SharedId = "Shared/lazy/id";
nm.TryGetVariable(SharedId, AddressSpaceRealm.Uns).ShouldBeNull();
nm.TryGetVariable(SharedId, AddressSpaceRealm.Raw).ShouldBeNull();
var raw = nm.TryGetVariable("Plant/dev1/lazy", AddressSpaceRealm.Raw);
raw.ShouldNotBeNull();
raw!.Value.ShouldBe(7);
var ts = new DateTime(2026, 8, 15, 11, 0, 0, DateTimeKind.Utc);
nm.WriteValues(new List<ValueWrite>
{
new(SharedId, 42, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
new(SharedId, 7, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw),
});
await host.DisposeAsync();
var uns = nm.TryGetVariable(SharedId, AddressSpaceRealm.Uns);
uns.ShouldNotBeNull();
uns!.Value.ShouldBe(42);
uns.StatusCode.ShouldBe((StatusCode)StatusCodes.Good);
uns.Timestamp.ShouldBe(ts);
var raw = nm.TryGetVariable(SharedId, AddressSpaceRealm.Raw);
raw.ShouldNotBeNull();
raw!.Value.ShouldBe(7);
// Two DISTINCT nodes, one per namespace — the realm-blind failure mode collapses these to one.
raw.NodeId.NamespaceIndex.ShouldNotBe(uns.NodeId.NamespaceIndex);
raw.NodeId.NamespaceIndex.ShouldBe(nm.NamespaceIndexForRealm(AddressSpaceRealm.Raw));
uns.NodeId.NamespaceIndex.ShouldBe(nm.NamespaceIndexForRealm(AddressSpaceRealm.Uns));
}
finally
{
await host.DisposeAsync();
}
}
/// <summary>A bad item does NOT drop the tail of the batch. Item 2's NodeId is empty; items 1 and 3 must
/// still land, and the aggregate surfaces as a <see cref="ValueWriteBatchException"/> naming the offender
/// and the failed/succeeded split.
/// <para>This is the durability-critical half: the poll engine updates its last-value cache BEFORE
/// raising, so an item dropped here is never re-raised until that tag's value changes again — the node
/// would serve a stale value indefinitely, with nothing in the log naming it.</para></summary>
[Fact]
public async Task A_failing_item_neither_drops_the_tail_nor_hides_which_node_failed()
{
var (host, server) = await BootAsync();
try
{
var nm = server.NodeManager!;
var ts = new DateTime(2026, 8, 15, 12, 0, 0, DateTimeKind.Utc);
var ex = Should.Throw<ValueWriteBatchException>(() => nm.WriteValues(new List<ValueWrite>
{
new("eq-iso/first", 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
new(string.Empty, 2, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
new("eq-iso/third", 3, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
}));
ex.FailedCount.ShouldBe(1);
ex.SucceededCount.ShouldBe(2);
ex.FirstFailedNodeId.ShouldBe(string.Empty);
// The load-bearing half: BOTH good items applied — including the one AFTER the failure.
var first = nm.TryGetVariable("eq-iso/first");
first.ShouldNotBeNull();
first!.Value.ShouldBe(1);
var third = nm.TryGetVariable("eq-iso/third");
third.ShouldNotBeNull("the item after the failing one must not be dropped");
third!.Value.ShouldBe(3);
}
finally
{
await host.DisposeAsync();
}
}
/// <summary>A batch larger than <c>MaxWritesPerLockAcquisition</c> (512) spans several lock acquisitions;
/// chunking must be invisible in the outcome — every item still lands exactly once, with its own value.
/// Chunk boundaries are the natural off-by-one hazard, so the size is deliberately not a multiple of 512.
/// </summary>
[Fact]
public async Task A_batch_spanning_several_chunks_lands_every_item()
{
var (host, server) = await BootAsync();
try
{
var nm = server.NodeManager!;
var ts = new DateTime(2026, 8, 15, 13, 0, 0, DateTimeKind.Utc);
const int Count = 1300; // > 2 chunks of 512, and not a multiple of it
var items = new List<ValueWrite>(Count);
for (var i = 0; i < Count; i++)
{
items.Add(new ValueWrite($"eq-chunk/{i}", i, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns));
}
nm.WriteValues(items);
nm.VariableCount.ShouldBe(Count);
foreach (var probe in new[] { 0, 511, 512, 1023, 1024, Count - 1 })
{
var v = nm.TryGetVariable($"eq-chunk/{probe}");
v.ShouldNotBeNull($"item {probe} (a chunk boundary) must have landed");
v!.Value.ShouldBe(probe);
}
}
finally
{
await host.DisposeAsync();
}
}
/// <summary>An empty batch neither throws nor touches the address space.</summary>
@@ -114,13 +211,18 @@ public sealed class NodeManagerBatchWriteTests : IDisposable
public async Task Empty_batch_is_a_no_op()
{
var (host, server) = await BootAsync();
var nm = server.NodeManager!;
try
{
var nm = server.NodeManager!;
var before = nm.VariableCount;
Should.NotThrow(() => nm.WriteValues([]));
nm.VariableCount.ShouldBe(before);
await host.DisposeAsync();
var before = nm.VariableCount;
Should.NotThrow(() => nm.WriteValues([]));
nm.VariableCount.ShouldBe(before);
}
finally
{
await host.DisposeAsync();
}
}
private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync()
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerPreStartGuardTests.cs
+24 -1
View File
@@ -9,7 +9,8 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
/// <summary>
/// OpcUaServer-004 — a fresh <see cref="OtOpcUaNodeManager"/> whose <c>CreateAddressSpace</c> has NOT
/// yet run (i.e. the server has not started) has a null <c>_root</c>. Every public address-space mutator
/// (<see cref="OtOpcUaNodeManager.WriteValue"/>, <see cref="OtOpcUaNodeManager.WriteAlarmCondition"/>,
/// (<see cref="OtOpcUaNodeManager.WriteValue"/>, <see cref="OtOpcUaNodeManager.WriteValues"/>,
/// <see cref="OtOpcUaNodeManager.WriteAlarmCondition"/>,
/// <see cref="OtOpcUaNodeManager.EnsureFolder"/>, <see cref="OtOpcUaNodeManager.EnsureVariable"/>,
/// <see cref="OtOpcUaNodeManager.MaterialiseAlarmCondition"/>) must now fail with a legible
/// <see cref="InvalidOperationException"/> instead of a bare NRE out of <c>ResolveParentFolder</c> /
@@ -78,6 +79,28 @@ public sealed class NodeManagerPreStartGuardTests : IDisposable
}
}
/// <summary>The batched mutator guards the same way — and it does so BEFORE its empty-batch early-return,
/// so a pre-start caller gets the legible failure regardless of what the batch happens to carry (an empty
/// batch returning quietly would make the guard depend on payload rather than on server state).</summary>
[Fact]
public async Task WriteValues_before_CreateAddressSpace_throws_InvalidOperationException()
{
var (host, nm) = await BuildPreStartNodeManagerAsync();
try
{
Should.Throw<InvalidOperationException>(() => nm.WriteValues(new[]
{
new ValueWrite("eq-1/temp", 1.0, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns),
}));
Should.Throw<InvalidOperationException>(() => nm.WriteValues([]));
}
finally
{
await host.DisposeAsync();
}
}
[Fact]
public async Task MaterialiseAlarmCondition_before_CreateAddressSpace_throws_InvalidOperationException()
{
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs
+77
View File
@@ -36,6 +36,83 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable
await host.DisposeAsync();
}
/// <summary>The sink's <see cref="SdkAddressSpaceSink.WriteValues"/> override forwards the batch to the
/// node manager's own batched entry point — every item lands, with its own value and realm.</summary>
[Fact]
public async Task WriteValues_forwards_the_batch_to_the_node_manager()
{
var (host, server) = await BootAsync();
try
{
var nm = server.NodeManager!;
// Through the INTERFACE on purpose: that is the seam production uses, and it is the only call
// shape under which deleting the sink's override silently degrades to the per-value default loop
// (a concrete-typed call would simply stop compiling, which is not the regression being pinned).
IOpcUaAddressSpaceSink sink = new SdkAddressSpaceSink(nm);
var ts = new DateTime(2026, 8, 15, 9, 0, 0, DateTimeKind.Utc);
sink.WriteValues(new List<ValueWrite>
{
new("Plant/dev1/Speed", 42, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw),
new("eq-1/Speed", 42, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
});
nm.VariableCount.ShouldBe(2);
var raw = nm.TryGetVariable("Plant/dev1/Speed", AddressSpaceRealm.Raw);
raw.ShouldNotBeNull();
raw!.Value.ShouldBe(42);
raw.Timestamp.ShouldBe(ts);
var uns = nm.TryGetVariable("eq-1/Speed", AddressSpaceRealm.Uns);
uns.ShouldNotBeNull();
uns!.Value.ShouldBe(42);
}
finally
{
await host.DisposeAsync();
}
}
/// <summary>Pins that the sink actually OVERRIDES <c>WriteValues</c> rather than inheriting the
/// interface's per-value default loop. The values-landed assertion above cannot tell the two apart — the
/// default loop lands them too — so this uses the one place their behaviour diverges: the manager's
/// batched path isolates per item and reports a <see cref="ValueWriteBatchException"/> aggregate, whereas
/// the default loop lets the first item's <c>ArgumentException</c> escape and never reaches the tail.
/// A test seam (a recording subclass) is not available here — <see cref="SdkAddressSpaceSink"/> is sealed
/// and its <c>WriteValues</c> is not virtual — so the forward is pinned by its observable contract
/// instead, which is the stronger assertion anyway.</summary>
[Fact]
public async Task WriteValues_forward_surfaces_the_node_managers_partial_failure_contract()
{
var (host, server) = await BootAsync();
try
{
var nm = server.NodeManager!;
IOpcUaAddressSpaceSink sink = new SdkAddressSpaceSink(nm);
var ts = new DateTime(2026, 8, 15, 9, 30, 0, DateTimeKind.Utc);
var ex = Should.Throw<ValueWriteBatchException>(() => sink.WriteValues(new List<ValueWrite>
{
new("eq-fwd/first", 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
new(string.Empty, 2, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
new("eq-fwd/third", 3, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns),
}));
ex.FailedCount.ShouldBe(1);
ex.SucceededCount.ShouldBe(2);
// The tail item landed — the default per-value loop would have thrown out on item 2 and skipped it.
var third = nm.TryGetVariable("eq-fwd/third", AddressSpaceRealm.Uns);
third.ShouldNotBeNull();
third!.Value.ShouldBe(3);
}
finally
{
await host.DisposeAsync();
}
}
/// <summary>Verifies that WriteAlarmCondition (un-materialised fallback) creates a dedicated node
/// distinct from value writes.</summary>
[Fact]
tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorBatchTests.cs
+63 -1
View File
@@ -72,6 +72,38 @@ public sealed class OpcUaPublishActorBatchTests : RuntimeActorTestBase
sink.Threw.ShouldBeTrue();
}
/// <summary>A PARTIAL failure is metered honestly: a sink that applies 2 of 3 items and reports the
/// aggregate as a <see cref="ValueWriteBatchException"/> must advance the write tally by the SUCCEEDED
/// count, not by zero (the general catch's behaviour) and not by the whole batch. The invariant the
/// batching work must not break is that the tally counts per ITEM — so a batch that half-failed reports
/// exactly the half that landed.</summary>
[Fact]
public void Partial_failure_still_counts_the_items_that_landed()
{
var sink = new PartialFailureSink();
// TestActorRef so the actor's own WriteCount tally is readable — the counter the handler advances is
// the thing under test, and nothing else observes it.
var actor = ActorOfAsTestActorRef<OpcUaPublishActor>(OpcUaPublishActor.PropsForTests(sink: sink));
var t0 = new DateTime(2026, 8, 15, 10, 0, 0, DateTimeKind.Utc);
actor.Tell(new OpcUaPublishActor.AttributeValueBatchUpdate(new[]
{
new ValueWrite("Plant/Modbus/dev1/Speed", 1, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw),
new ValueWrite("", 2, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw),
new ValueWrite("Plant/Modbus/dev1/Temp", 3, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw),
}));
sink.Threw.ShouldBeTrue();
actor.UnderlyingActor.WriteCount.ShouldBe(2,
"the two items the sink DID apply must still be counted — not 0 (the general catch) and not 3");
// The actor survived the partial failure and keeps pumping.
actor.Tell(new OpcUaPublishActor.AttributeValueUpdate(
"Plant/Modbus/dev1/Other", 4, OpcUaStatus.Good, t0, AddressSpaceRealm.Raw));
sink.SingleWrites.Count.ShouldBe(1);
actor.UnderlyingActor.WriteCount.ShouldBe(3);
}
/// <summary>The pre-existing single-item AttributeValueUpdate path is untouched by the batch addition.</summary>
[Fact]
public void Single_item_message_still_works()
@@ -133,7 +165,8 @@ public sealed class OpcUaPublishActorBatchTests : RuntimeActorTestBase
public void AddReference(string sourceNodeId, AddressSpaceRealm sourceRealm, string targetNodeId, AddressSpaceRealm targetRealm, string referenceType = "Organizes") { }
}
/// <summary>A sink whose batched write throws — drives the handler's catch.</summary>
/// <summary>A sink whose batched write throws a PLAIN exception — drives the handler's GENERAL catch (an
/// unrecognised sink failure, where nothing is known about what landed, so nothing is counted).</summary>
private sealed class ThrowOnWriteValuesSink : RecordingSink
{
private int _threw;
@@ -146,4 +179,33 @@ public sealed class OpcUaPublishActorBatchTests : RuntimeActorTestBase
throw new InvalidOperationException("simulated batched-write fault");
}
}
/// <summary>A sink modelling the real SDK-backed one's PARTIAL failure: it applies every item with a
/// non-empty NodeId, then reports the aggregate as a <see cref="ValueWriteBatchException"/>. Drives the
/// handler's typed catch.</summary>
private sealed class PartialFailureSink : RecordingSink
{
private int _threw;
public bool Threw => Volatile.Read(ref _threw) != 0;
public override void WriteValues(IReadOnlyList<ValueWrite> values)
{
var applied = values.Where(v => !string.IsNullOrEmpty(v.NodeId)).ToArray();
var failedCount = values.Count - applied.Length;
if (failedCount == 0)
{
base.WriteValues(values);
return;
}
base.WriteValues(applied);
Interlocked.Exchange(ref _threw, 1);
throw new ValueWriteBatchException(
firstFailedNodeId: values.First(v => string.IsNullOrEmpty(v.NodeId)).NodeId,
failedCount: failedCount,
succeededCount: applied.Length,
firstFailure: new ArgumentException("empty NodeId"));
}
}
}