From 8d51380ff1d685c95b67707142a54ffe9c23f737 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 9 Oct 2026 14:20:00 -0400 Subject: [PATCH] fix(opcua): reverse HistoryRead-Raw paging no longer stops after the first page A reverse read (start > end) with a small NumValuesPerNode resumed through the tie-cluster path, which drained the cluster and stepped FORWARD to T + 1 tick - outside a reverse window - so the read ended with GoodNoData after one sample. SliceTieCluster now steps down to T - 1 tick for a reverse read (terminal at the excluded end), and the node manager slices the over-fetched cluster in the read's own direction so the skip count is not applied from the wrong end. Forward paging is unchanged. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../HistoryPaging.cs | 17 +++- .../OtOpcUaNodeManager.cs | 13 ++- .../HistoryPagingTests.cs | 31 +++++++ .../NodeManagerHistoryReadPagingTests.cs | 80 ++++++++++++++++++- 4 files changed, 134 insertions(+), 7 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs index 61008a9d..d1ffae37 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs @@ -205,14 +205,29 @@ internal static class HistoryPaging /// remains, or null when the window is exhausted (terminal — no continuation point). /// The next page's boundary skip count: the running emitted-tie total while the /// cluster drains, else 0 after advancing past it. + /// + /// true for a reverse read (Part 11: start > end, samples in descending time, window + /// (end, start]). A drained cluster then steps DOWN to T − 1 tick, which is lossless for the + /// same reason T + 1 tick is going forward, and the read continues only while that is still strictly + /// after (the reverse window excludes end; a resume at exactly end + /// would turn into a start == end point read and return the excluded sample). Forward is unchanged. + /// public static void SliceTieCluster( int clusterCount, int skip, uint cap, DateTime boundaryT, DateTime endUtc, - out int sliceStart, out int sliceCount, out DateTime? nextStartUtc, out int nextSkip) + out int sliceStart, out int sliceCount, out DateTime? nextStartUtc, out int nextSkip, + bool reverse = false) { sliceStart = Math.Min(skip, clusterCount); sliceCount = Math.Min((int)cap, clusterCount - sliceStart); var emitted = sliceStart + sliceCount; if (emitted < clusterCount) { nextStartUtc = boundaryT; nextSkip = emitted; } + else if (reverse) + { + // DateTime.MinValue cannot step down; nothing can lie below it anyway. + var more = boundaryT > DateTime.MinValue && boundaryT.AddTicks(-1) > endUtc; + nextStartUtc = more ? boundaryT.AddTicks(-1) : null; + nextSkip = 0; + } else { var next = boundaryT.AddTicks(1); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index f6ab9359..107303bd 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -5898,10 +5898,16 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // Compute in uint so +1 can never wrap: Math.Max(1,…) also guards against a zero/negative // config value slipping through (Validate() warns but does not block startup). var overfetchCap = (uint)Math.Max(1, MaxTieClusterOverfetch) + 1u; - var cluster = (await HistorianDataSource + IReadOnlyList cluster = (await HistorianDataSource .ReadRawAsync(tagname, startUtc, startUtc, overfetchCap, ct) .ConfigureAwait(false)).Samples; + // A start == end point read returns the ties in the backend's FORWARD order, but a reverse read + // emitted them (and counted boundarySkip) in descending order — the mirror of forward. Slice the + // cluster in the read's own direction, or the skip would index from the wrong end and re-emit ties. + var reverse = startUtc > endUtc; + if (reverse) cluster = cluster.Reverse().ToList(); + // Absurd burst: more ties than we're willing to buffer in memory. Preserve today's loud-fail // for that node rather than over-fetch an unbounded cluster; the operator's remedy is a // larger ServerHistorian:MaxTieClusterOverfetch (or NumValuesPerNode). @@ -5916,9 +5922,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 return; } + // Direction comes from the window itself (Part 11: start > end is a reverse read). A resumed reverse + // read keeps start > end, because a reverse resume point is always strictly after endUtc. HistoryPaging.SliceTieCluster( cluster.Count, boundarySkip, numValuesPerNode, startUtc, endUtc, - out var sliceStart, out var sliceCount, out var nextStartUtc, out var nextSkip); + out var sliceStart, out var sliceCount, out var nextStartUtc, out var nextSkip, + reverse: reverse); var slice = new List(sliceCount); for (var i = sliceStart; i < sliceStart + sliceCount; i++) slice.Add(cluster[i]); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/HistoryPagingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/HistoryPagingTests.cs index 3efcc088..b97d859a 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/HistoryPagingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/HistoryPagingTests.cs @@ -218,6 +218,37 @@ public sealed class HistoryPagingTests nextSkip.ShouldBe(0); } + [Fact] + public void SliceTieCluster_reverse_drain_steps_down_one_tick_while_the_window_remains() + { + // Reverse read (start > end): a drained cluster at T resumes at T-1tick, not T+1tick (which lies outside + // a reverse window and used to end the read after its first page). + var t = new DateTime(2026, 1, 1, 0, 0, 5, DateTimeKind.Utc); + + HistoryPaging.SliceTieCluster( + clusterCount: 1, skip: 1, cap: 1, boundaryT: t, endUtc: t.AddHours(-1), + out _, out var sliceCount, out var nextStartUtc, out var nextSkip, reverse: true); + + sliceCount.ShouldBe(0); + nextStartUtc.ShouldBe(t.AddTicks(-1)); + nextSkip.ShouldBe(0); + } + + [Fact] + public void SliceTieCluster_reverse_drain_terminates_when_the_next_tick_is_the_excluded_end() + { + // The reverse window is (end, start]; resuming at exactly end would become a point read of the excluded + // sample, so T-1tick == end is terminal. + var t = new DateTime(2026, 1, 1, 0, 0, 5, DateTimeKind.Utc); + + HistoryPaging.SliceTieCluster( + clusterCount: 2, skip: 0, cap: 5, boundaryT: t, endUtc: t.AddTicks(-1), + out _, out var sliceCount, out var nextStartUtc, out _, reverse: true); + + sliceCount.ShouldBe(2); + nextStartUtc.ShouldBeNull(); + } + [Fact] public void SliceTieCluster_self_heals_when_skip_exceeds_cluster() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs index 93324ec0..b9388b55 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs @@ -312,6 +312,70 @@ public sealed class NodeManagerHistoryReadPagingTests : IDisposable await host.DisposeAsync(); } + /// + /// A reverse read (start > end) with NumValuesPerNode = 1 over three distinct-timestamp samples must + /// page through every sample, newest first. Before the fix the second page's resume read re-returned the + /// boundary sample, the trim emptied it, the tie-cluster path drained the one-sample "cluster" and stepped + /// FORWARD to T + 1 tick — past a reverse window's end — so paging stopped after the first sample. + /// + [Fact] + public async Task Raw_reverse_read_with_one_value_per_page_returns_every_sample_descending() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.HistoryContinuationStore = new InMemoryHistoryContinuationStore(); + nm.HistorianDataSource = new SeriesHistorianDataSource(MakeSeries(count: 3, stepSeconds: 1)); + + nm.EnsureVariable("eq-1/rev", parentFolderNodeId: null, displayName: "Rev", dataType: "Double", + writable: false, historianTagname: "WW.Rev", realm: AddressSpaceRealm.Uns); + var nodeId = nm.TryGetVariable("eq-1/rev")!.NodeId; + + var collected = await DrainAsync(nm, nodeId, start: Epoch.AddHours(1), end: Epoch.AddSeconds(-1), max: 1); + + collected.ShouldBe([2d, 1d, 0d]); + await host.DisposeAsync(); + } + + /// The reverse mirror of : a 5-way tie + /// cluster between distinct samples, cap 2, read newest-first — every value once, in descending order. + [Fact] + public async Task Raw_reverse_read_pages_through_an_oversized_tie_cluster() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.HistoryContinuationStore = new InMemoryHistoryContinuationStore(); + nm.HistorianDataSource = new SeriesHistorianDataSource( + MakeSeriesWithTieCluster(distinctBefore: 3, tieCount: 5, distinctAfter: 2)); + + nm.EnsureVariable("eq-1/revburst", parentFolderNodeId: null, displayName: "RevBurst", dataType: "Double", + writable: false, historianTagname: "WW.RevBurst", realm: AddressSpaceRealm.Uns); + var nodeId = nm.TryGetVariable("eq-1/revburst")!.NodeId; + + var collected = await DrainAsync(nm, nodeId, start: Epoch.AddHours(1), end: Epoch.AddSeconds(-1), max: 2); + + collected.ShouldBe(Enumerable.Range(0, 10).Reverse().Select(i => (double)i)); + await host.DisposeAsync(); + } + + /// Pages a Raw read to completion and returns the union of the pages in arrival order. + private static Task> DrainAsync(OtOpcUaNodeManager nm, NodeId nodeId, DateTime start, DateTime end, uint max) + { + var collected = new List(); + byte[]? cp = null; + var pageCount = 0; + do + { + var (values, error, nextCp) = ReadRaw(nm, nodeId, start, end, max, inboundCp: cp); + error.StatusCode.Code.ShouldBe(StatusCodes.Good); + collected.AddRange(values); + cp = nextCp; + pageCount++; + pageCount.ShouldBeLessThan(40, "paging must terminate, not loop"); + } + while (cp is not null); + return Task.FromResult(collected); + } + // --- helpers -------------------------------------------------------------------------------- /// Issue a single-node Raw HistoryRead and return that node's decoded sample values, error, @@ -420,10 +484,18 @@ public sealed class NodeManagerHistoryReadPagingTests : IDisposable CancellationToken cancellationToken) { Interlocked.Increment(ref _readCount); - var window = series - .Where(s => (s.SourceTimestampUtc ?? DateTime.MinValue) >= startUtc - && (s.SourceTimestampUtc ?? DateTime.MinValue) <= endUtc) - .ToList(); + // Reverse (Part 11, start > end): (end, start] in DESCENDING time — the SQL backend's contract. + // Ties keep series order reversed, so a reverse page of a cluster is deterministic. + var window = startUtc > endUtc + ? series + .Where(s => (s.SourceTimestampUtc ?? DateTime.MinValue) > endUtc + && (s.SourceTimestampUtc ?? DateTime.MinValue) <= startUtc) + .Reverse() + .ToList() + : series + .Where(s => (s.SourceTimestampUtc ?? DateTime.MinValue) >= startUtc + && (s.SourceTimestampUtc ?? DateTime.MinValue) <= endUtc) + .ToList(); // maxValuesPerNode == 0 means "no limit". var page = maxValuesPerNode == 0 ? window : window.Take((int)maxValuesPerNode).ToList(); return Task.FromResult(new HistorianRead(page, null));