diff --git a/docs/Historian.md b/docs/Historian.md index 97667113..78a29031 100644 --- a/docs/Historian.md +++ b/docs/Historian.md @@ -587,14 +587,20 @@ paging time-based: returns exactly `NumValuesPerNode` values, and the server stores a resume cursor and returns an opaque continuation point (16 bytes). The client hands it back to fetch the next page. - When it does not, the page is the **last** — no continuation point, even when it is exactly full. (#692: a total that - is an exact multiple of the cap used to end with an extra empty `GoodNoData` page.) + is an exact multiple of the cap used to end with an extra empty `GoodNoData` page.) The exception is an oversized tie + cluster (below): its pages are sliced from the over-fetched cluster, its last slice is `min(N, remaining)` values and + still carries a continuation point when the window goes on — so stepping past a cluster at the very end of the data can + yield one empty final page. - `NumValuesPerNode == 0` ("all values, no limit") is never paged; the whole window returns in one shot. - The resume cursor is **tie-safe**: the next page resumes from the last returned sample's SourceTimestamp *inclusive* and drops the boundary samples already emitted, so samples sharing the boundary timestamp are neither duplicated nor skipped. Because it re-reads them, a resumed page asks for - `NumValuesPerNode + skip + 1` rows, so it still carries a full page. -- ⚠️ **#692 (client-visible): every page is full.** A resumed page used to ask for only `NumValuesPerNode` rows, so after + `NumValuesPerNode + skip + 1` rows, so it still carries a full page. On a strict Gateway + (`AllowStreamingBeyondCeiling=false`) that moves the row-ceiling edge by `skip + 1`: a `NumValuesPerNode` just under the + learned ceiling may now cost one learning refusal (#691.3 re-asks at the ceiling). A page clamp to the learned ceiling is + being added separately (#694). +- ⚠️ **#692 (client-visible): every page is full** (outside an oversized tie cluster — see above). A resumed page used to ask for only `NumValuesPerNode` rows, so after the trim it netted `N − skip` values — and at `NumValuesPerNode = 1` **nothing**: the lone re-read boundary sample was trimmed, the empty "full" page was taken for a stalled tie cluster, and a `MaxTieClusterOverfetch + 1` (65,537-row) over-fetch stepped one tick to return an **empty page with a continuation point between every value** (live, AVEVA: @@ -610,7 +616,10 @@ paging time-based: > `ServerHistorian:MaxTieClusterOverfetch` (default **65 536**), then serves the cluster > `NumValuesPerNode` samples at a time across successive pages, advancing the cursor one tick past > the timestamp once the cluster is fully drained. A cursor left inside the cluster is marked so (`WithinTieCluster`), and -> its next page over-fetches the cluster directly instead of first re-reading rows it knows are ties. +> its next page over-fetches the cluster directly instead of first re-reading rows it knows are ties. A drained cluster +> resumes only while the next step is strictly before `EndTime` (truncated to the backend's resolution) — the forward +> window is end-exclusive; resuming AT `EndTime` used to make the next read a point read that returned the samples at +> `EndTime` as data (#692 review). > > **Reverse reads** (`start > end`, newest first) page the same way in the opposite direction: a drained > cluster steps the cursor one tick **down**, the read ends when that would reach the (excluded) `end`, and diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs index 28082027..c16ba97b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs @@ -258,12 +258,11 @@ internal static class HistoryPaging /// The client's per-page cap (NumValuesPerNode); must be > 0 to make progress. /// The single SourceTimestamp every tie in the cluster shares. /// The (inclusive) upper bound of the read window; unchanged across pages. - /// OpcUaServer-003, accepted by design: the backend contract - /// (IHistorianDataSource.ReadRawAsync) treats endUtc as EXCLUSIVE, so the - /// next <= endUtc test here is deliberately the conservative side — at worst one extra - /// empty resume page ([endUtc, endUtc) → GoodNoData, no continuation point), never a - /// dropped or duplicated sample. Tightening to < would be correct only if the live - /// backend's boundary is confirmed exclusive; it is not worth the opposite off-by-one risk. + /// The forward window is end-EXCLUSIVE (Part 11; the IHistorianDataSource.ReadRawAsync contract), so a drained + /// cluster continues only while next is strictly before truncated to + /// . (OpcUaServer-003 had kept next <= endUtc as "the conservative side", + /// but it was not: resuming AT EndTime makes the next read a start == end point read, which returns the samples + /// at EndTime as data — #692 review F1; on a microsecond backend EndTime is truncated first.) /// The index into the cluster the emitted slice starts at (clamped to the count). /// How many ties the emitted slice contains (may be 0 on a stale-skip self-heal). /// The next page's inclusive start: while the @@ -306,7 +305,9 @@ internal static class HistoryPaging else { var next = boundaryT.AddTicks(resolutionTicks); - if (next <= endUtc) { nextStartUtc = next; nextSkip = 0; } + // The forward window is end-EXCLUSIVE, at the backend's resolution: resuming AT EndTime would turn the next read + // into a start == end point read, which returns the samples at EndTime as data (#692 review F1). + if (next.Ticks < endUtc.Ticks - endUtc.Ticks % resolutionTicks) { nextStartUtc = next; nextSkip = 0; } else { nextStartUtc = null; nextSkip = 0; } } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPageFillTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPageFillTests.cs index 36c7cf3e..87ae0cde 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPageFillTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPageFillTests.cs @@ -146,6 +146,68 @@ public sealed class NodeManagerHistoryReadPageFillTests(BoundsServerFixture fx) pages.Select(p => p.Count).ShouldBe([2, 2, 2, 2]); } + /// + /// #692 review F1: an oversized tie cluster one step below EndTime used to resume AT EndTime (the drain check was + /// next <= end, but the forward window is end-exclusive), so the next read was a start == end point + /// read that returned the samples AT EndTime as data. Paged must equal unpaged — neither holds the EndTime samples. + /// The microsecond shape truncates EndTime (8.1 µs ⇒ 8.0 µs) before the same thing happens. + /// + [Theory] + [InlineData(Shape.Native, 1L, 8L)] + [InlineData(Shape.Microsecond, 10L, 81L)] + public async Task A_tie_cluster_just_below_the_end_time_never_resumes_onto_the_end_time(Shape shape, long unit, long endTicks) + { + var series = new List(); + var v = 0; + foreach (var (at, count) in new[] { (5L, 5), (6L, 2), (7L, 3), (8L, 3) }) + for (var i = 0; i < count; i++) + series.Add(Sample(v++, T0.AddTicks(at * unit))); + var source = Source(shape, series); + var (start, end) = (T0.AddSeconds(-3), T0.AddTicks(endTicks)); + + var unpaged = await SinglePageAsync(source, start, end, bounds: false); + unpaged.Count.ShouldBe(10, "the samples at the (exclusive) end time are not data"); + + var pages = await DrainPagesAsync(source, start, end, 1, bounds: false); + Render(pages.SelectMany(p => p)).ShouldBe(Render(unpaged)); + } + + /// + /// #692: a cursor left inside an oversized tie cluster is flagged, and its next page goes straight to the cluster + /// over-fetch — one backend read — instead of first re-reading N + skip + 1 rows it knows are ties. + /// + [Fact] + public async Task A_cursor_inside_a_tie_cluster_over_fetches_without_a_normal_read_first() + { + // a @1s, five ties @2s, b @3s; N = 1. + var series = new List { Sample(0, T0.AddSeconds(1)) }; + for (var i = 1; i <= 5; i++) series.Add(Sample(i, T0.AddSeconds(2))); + series.Add(Sample(6, T0.AddSeconds(3))); + var source = Source(Shape.Native, series); + var (start, end) = (T0, T0.AddSeconds(10)); + + var readsPerPage = new List(); + var all = new List(); + byte[]? cp = null; + do + { + source.ResetCounts(); + var (values, error, next) = await ReadAsync(source, start, end, 1, bounds: false, cp); + error.StatusCode.Code.ShouldBe(StatusCodes.Good); + values.Count.ShouldBe(1); + all.AddRange(values); + readsPerPage.Add(source.Reads); + cp = next; + readsPerPage.Count.ShouldBeLessThan(20); + } + while (cp is not null); + + all.Select(x => (double)x.Value).ShouldBe([0d, 1d, 2d, 3d, 4d, 5d, 6d]); + // Page 1 [a], page 2 [t1]: one read each. Page 3 [t2]: the read stalls on the cluster, then the over-fetch (2). + // Pages 4 [t3], 5 [t4], 6 [t5]: the flagged cursor over-fetches directly (1 each). Page 7 [b]: one read. + readsPerPage.ShouldBe([1, 1, 2, 1, 1, 1, 1]); + } + /// /// #692 (low): a one-value bounded read whose start bound is not found asks whether the read holds anything. When the /// answer is "only the end bound", the bound it just found rides the continuation point — the next page is served