From 059f18bdad90873431e60d7d6aec6b0e52dfc2b6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 14 Jun 2026 19:44:56 -0400 Subject: [PATCH] test(historian): multi-node HistoryRead isolation + single-lookup ServeNode + comment fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix A: add Raw_multi_node_per_node_error_isolation test — two historized variables (eqA/good→A.PV, eqB/bad→B.PV) in one Raw batch; per-tagname fake throws for B.PV, returns a sample for A.PV; asserts errors[0]=Good+sample, errors[1]=Bad, HistoryData[1]=null (no cross-slot leak), no exception escapes. Fix B: collapse double ConcurrentDictionary lookup in ServeNode — TryGetHistorizedTagname now captures `out var tagname` on the guard; the resolved tagname is threaded into the read callback as a second parameter (Func>), removing the redundant ResolveTagname helper (deleted) and the tiny race window between the check and the second lookup. All three call-sites (Raw/Processed/AtTime) updated. Fix C: rewrite the IsReadModified comment at NodeManagerHistoryReadTests.cs:102 — the SDK's ReadRawModifiedDetails.Initialize() sets m_isReadModified=true (generated ctor body in Opc.Ua.DataTypes.cs), so the default IS true; the test must explicitly clear it to false for a plain raw read. Previous comment said the same thing but imprecisely; now cites the SDK mechanism (Initialize() call in the public ctor). --- .../OtOpcUaNodeManager.cs | 44 +++---- .../NodeManagerHistoryReadTests.cs | 110 ++++++++++++++++-- 2 files changed, 122 insertions(+), 32 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 294d22ae..ca57a6f9 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1050,8 +1050,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 continue; } - ServeNode(handle, results, errors, source => source.ReadRawAsync( - ResolveTagname(handle), + ServeNode(handle, results, errors, (source, tagname) => source.ReadRawAsync( + tagname, details.StartTime, details.EndTime, details.NumValuesPerNode, @@ -1088,8 +1088,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 continue; } - ServeNode(handle, results, errors, source => source.ReadProcessedAsync( - ResolveTagname(handle), + ServeNode(handle, results, errors, (source, tagname) => source.ReadProcessedAsync( + tagname, details.StartTime, details.EndTime, // OPC UA ProcessingInterval is a Duration in milliseconds. @@ -1117,8 +1117,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 var timestamps = details.ReqTimes?.ToList() ?? new List(); foreach (var handle in nodesToProcess) { - ServeNode(handle, results, errors, source => source.ReadAtTimeAsync( - ResolveTagname(handle), + ServeNode(handle, results, errors, (source, tagname) => source.ReadAtTimeAsync( + tagname, timestamps, CancellationToken.None)); } @@ -1126,25 +1126,28 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// /// Block-bridge to the historian source for one node handle and project the result onto the - /// service-level results/errors slots. Resolves the node's registered historian tagname first - /// (a node we don't recognise as historized — which shouldn't reach us, since the base only - /// hands us nodes with the HistoryRead access bit — maps to BadHistoryOperationUnsupported). - /// The callback is invoked only AFTER the tagname is confirmed present; - /// it is wrapped in try/catch so a backend throw / timeout becomes a Bad status for THIS node - /// without throwing out of the batch. + /// service-level results/errors slots. Resolves the node's registered historian tagname first — + /// a single lookup; the resolved tagname is passed directly + /// to , removing any risk of a second concurrent lookup on the same key. + /// A node we don't recognise as historized maps to BadHistoryOperationUnsupported + /// (shouldn't normally reach us, since the base only hands us nodes with the HistoryRead access + /// bit, but we guard explicitly). The callback receives the resolved + /// tagname and is wrapped in try/catch so a backend throw / timeout becomes a Bad status for + /// THIS node without throwing out of the batch. /// /// The pre-filtered node handle to serve; handle.Index indexes results/errors. /// The service-level results list to fill at handle.Index. /// The service-level errors list to fill at handle.Index. - /// Invokes the resolved data-source read; only called once the tagname is known. + /// Invokes the resolved data-source read with the resolved tagname; only called + /// once the tagname is confirmed present. private void ServeNode( NodeHandle handle, IList results, IList errors, - Func> read) + Func> read) { var idString = handle.NodeId.Identifier?.ToString(); - if (idString is null || !TryGetHistorizedTagname(idString, out _)) + if (idString is null || !TryGetHistorizedTagname(idString, out var tagname)) { // Not a historized node we own a tagname for — unsupported. (The base pre-seeds this same // status, but set it explicitly so the contract is local + obvious.) @@ -1156,7 +1159,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 { // HistoryRead is NOT invoked under the node-manager Lock (unlike OnWriteValue), so blocking // on the async source here is safe and won't freeze the address space. - var sourceResult = read(HistorianDataSource).GetAwaiter().GetResult(); + var sourceResult = read(HistorianDataSource, tagname!).GetAwaiter().GetResult(); var historyData = ToHistoryData(sourceResult); results[handle.Index] = new SdkHistoryReadResult @@ -1183,15 +1186,6 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } - /// Resolve a handle's registered historian tagname. The caller () - /// has already confirmed the node is historized before invoking the read callback, so this is a - /// guaranteed hit; the null-coalesce is a defensive fallback to the bare NodeId string. - private string ResolveTagname(NodeHandle handle) - { - var idString = handle.NodeId.Identifier?.ToString() ?? string.Empty; - return TryGetHistorizedTagname(idString, out var tagname) ? tagname! : idString; - } - /// /// Map an OPC UA Part 13 standard-aggregate function NodeId to our /// . Returns null for any aggregate we don't serve so diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadTests.cs index 3d050b43..48ee37f5 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadTests.cs @@ -99,7 +99,9 @@ public sealed class NodeManagerHistoryReadTests : IDisposable StartTime = DateTime.UtcNow.AddHours(-1), EndTime = DateTime.UtcNow, NumValuesPerNode = 10, - // ReadRawModifiedDetails defaults IsReadModified=true; a raw (non-modified) read clears it. + // ReadRawModifiedDetails.Initialize() sets m_isReadModified=true (SDK-generated ctor body), + // so the default from new ReadRawModifiedDetails() is TRUE. A plain raw read must explicitly + // clear it to false to avoid hitting the IsReadModified=true early-return in the override. IsReadModified = false, }; @@ -294,6 +296,67 @@ public sealed class NodeManagerHistoryReadTests : IDisposable await host.DisposeAsync(); } + /// + /// Two nodes in a single Raw HistoryRead batch — one backend succeeds, one throws. Proves that + /// per-node error isolation is enforced across handle indices: the good node's slot is + /// Good+samples, the bad node's slot is Bad, and neither status leaks into the other. + /// + [Fact] + public async Task Raw_multi_node_per_node_error_isolation() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + // Materialise two distinct historized variables under separate equipment folders. + nm.EnsureVariable("eqA/good", parentFolderNodeId: null, displayName: "Good", dataType: "Float", + writable: false, historianTagname: "A.PV"); + nm.EnsureVariable("eqB/bad", parentFolderNodeId: null, displayName: "Bad", dataType: "Float", + writable: false, historianTagname: "B.PV"); + + var goodNodeId = nm.TryGetVariable("eqA/good")!.NodeId; + var badNodeId = nm.TryGetVariable("eqB/bad")!.NodeId; + + var src = DateTime.UtcNow.AddSeconds(-5); + var srv = DateTime.UtcNow; + + // Per-tagname fake: A.PV returns one sample; B.PV throws. + var perTagFake = new PerTagnameHistorianDataSource(tagname => + tagname == "A.PV" + ? Task.FromResult(new HistorianRead(new[] { new DataValueSnapshot(1.0f, StatusCodes.Good, src, srv) }, null)) + : throw new InvalidOperationException("backend boom for B.PV")); + nm.HistorianDataSource = perTagFake; + + var details = new ReadRawModifiedDetails + { + StartTime = DateTime.UtcNow.AddHours(-1), + EndTime = DateTime.UtcNow, + NumValuesPerNode = 10, + IsReadModified = false, + }; + + // Single batch listing BOTH nodes in order [goodNodeId, badNodeId]. + // handle.Index maps by position: good=0, bad=1. + var (results, errors) = InvokeHistoryRead(server, nm, details, goodNodeId, badNodeId); + + // Good node (index 0): should have a Good status + one sample. + errors[0].StatusCode.Code.ShouldBe(StatusCodes.Good); + results[0].StatusCode.Code.ShouldBe(StatusCodes.Good); + var data = (HistoryData)ExtensionObject.ToEncodeable(results[0].HistoryData); + data.DataValues.Count.ShouldBe(1); + data.DataValues[0].Value.ShouldBe(1.0f); + + // Bad node (index 1): errors slot must be Bad; results slot must NOT carry any HistoryData + // (no data was projected into it). The base may pre-seed results[i] to a default-constructed + // SdkHistoryReadResult whose StatusCode is 0 (Good) — that's fine for the result field, but the + // distinguishing signal is the error slot AND the absence of HistoryData from the good node. + StatusCode.IsBad(errors[1].StatusCode).ShouldBeTrue(); + errors[1].StatusCode.Code.ShouldBe(StatusCodes.BadHistoryOperationUnsupported); + // HistoryData must be null/empty — the good node's sample MUST NOT have leaked into slot 1. + (results[1].HistoryData == null || ExtensionObject.IsNull(results[1].HistoryData)).ShouldBeTrue(); + + await host.DisposeAsync(); + } + /// A backend that throws ⇒ that node's error is Bad (not GoodNoData) and no exception /// escapes the HistoryRead call. [Fact] @@ -333,16 +396,19 @@ public sealed class NodeManagerHistoryReadTests : IDisposable /// handle-building only needs the NodeId + namespace, not a session. private static (IList Results, IList Errors) InvokeHistoryRead( OtOpcUaSdkServer server, OtOpcUaNodeManager nm, HistoryReadDetails details, NodeId nodeId) + => InvokeHistoryRead(server, nm, details, new[] { nodeId }); + + /// Multi-node overload — issues one HistoryRead batch for all supplied node ids in order. + /// Results and errors are returned in the same order as . + private static (IList Results, IList Errors) InvokeHistoryRead( + OtOpcUaSdkServer server, OtOpcUaNodeManager nm, HistoryReadDetails details, params NodeId[] nodeIds) { var context = new OperationContext( new RequestHeader(), secureChannelContext: null, RequestType.HistoryRead, identity: null); - var nodesToRead = new List - { - new() { NodeId = nodeId }, - }; - var results = new List { null! }; - var errors = new List { null! }; + var nodesToRead = nodeIds.Select(id => new HistoryReadValueId { NodeId = id }).ToList(); + var results = Enumerable.Repeat(null!, nodeIds.Length).ToList(); + var errors = Enumerable.Repeat(null!, nodeIds.Length).ToList(); nm.HistoryRead( context, @@ -425,6 +491,36 @@ public sealed class NodeManagerHistoryReadTests : IDisposable } } + /// A per-tagname historian source that delegates each raw read to a caller-supplied + /// . The func may throw to simulate a backend failure for a + /// specific tagname while returning results for others — enabling per-node isolation tests. + /// Processed / AtTime / Events are not wired (they delegate to the null source). + private sealed class PerTagnameHistorianDataSource(Func> rawHandler) + : IHistorianDataSource + { + public Task ReadRawAsync( + string fullReference, DateTime startUtc, DateTime endUtc, uint maxValuesPerNode, + CancellationToken cancellationToken) => rawHandler(fullReference); + + public Task ReadProcessedAsync( + string fullReference, DateTime startUtc, DateTime endUtc, TimeSpan interval, + HistoryAggregateType aggregate, CancellationToken cancellationToken) => + NullHistorianDataSource.Instance.ReadProcessedAsync(fullReference, startUtc, endUtc, interval, aggregate, cancellationToken); + + public Task ReadAtTimeAsync( + string fullReference, IReadOnlyList timestampsUtc, CancellationToken cancellationToken) => + NullHistorianDataSource.Instance.ReadAtTimeAsync(fullReference, timestampsUtc, cancellationToken); + + public Task ReadEventsAsync( + string? sourceName, DateTime startUtc, DateTime endUtc, int maxEvents, + CancellationToken cancellationToken) => + NullHistorianDataSource.Instance.ReadEventsAsync(sourceName, startUtc, endUtc, maxEvents, cancellationToken); + + public HistorianHealthSnapshot GetHealthSnapshot() => NullHistorianDataSource.Instance.GetHealthSnapshot(); + + public void Dispose() { } + } + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() { var host = new OpcUaApplicationHost(