fix(historian): #692 review — a drained tie cluster never resumes onto the exclusive EndTime

F1 (pre-existing): SliceTieCluster continued a forward read while next <= endUtc, but the
forward window is end-exclusive; an oversized cluster one step below EndTime resumed AT
EndTime, the resume became a start == end point read and returned the EndTime samples as
data (microsecond: EndTime 8.1 us truncated to 8.0). Continue only while next is strictly
before EndTime truncated to the backend resolution.

Tests: A_tie_cluster_just_below_the_end_time_never_resumes_onto_the_end_time (native +
microsecond; both fail on the old rule) and
A_cursor_inside_a_tie_cluster_over_fetches_without_a_normal_read_first (pins the
WithinTieCluster shortcut by per-page read count; fails with the shortcut disabled).
Docs: the oversized-cluster exception to 'every page full / no lookahead => last' (F2),
and the N + skip + 1 strict-ceiling edge note referencing #694 (F4).
This commit is contained in:
Joseph Doherty
2026-10-13 01:02:12 -04:00
parent 3d7eea6280
commit e04e1bf127
3 changed files with 83 additions and 11 deletions
+13 -4
View File
@@ -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
@@ -258,12 +258,11 @@ internal static class HistoryPaging
/// <param name="cap">The client's per-page cap (<c>NumValuesPerNode</c>); must be &gt; 0 to make progress.</param>
/// <param name="boundaryT">The single SourceTimestamp every tie in the cluster shares.</param>
/// <param name="endUtc">The (inclusive) upper bound of the read window; unchanged across pages.
/// OpcUaServer-003, accepted by design: the backend contract
/// (<c>IHistorianDataSource.ReadRawAsync</c>) treats <c>endUtc</c> as EXCLUSIVE, so the
/// <c>next &lt;= endUtc</c> test here is deliberately the conservative side — at worst one extra
/// empty resume page (<c>[endUtc, endUtc)</c> → GoodNoData, no continuation point), never a
/// dropped or duplicated sample. Tightening to <c>&lt;</c> would be correct only if the live
/// backend's boundary is confirmed exclusive; it is not worth the opposite off-by-one risk.</param>
/// The forward window is end-EXCLUSIVE (Part 11; the <c>IHistorianDataSource.ReadRawAsync</c> contract), so a drained
/// cluster continues only while <c>next</c> is strictly before <paramref name="endUtc"/> truncated to
/// <paramref name="resolutionTicks"/>. (OpcUaServer-003 had kept <c>next &lt;= endUtc</c> as "the conservative side",
/// but it was not: resuming AT EndTime makes the next read a <c>start == end</c> point read, which returns the samples
/// at EndTime as data — #692 review F1; on a microsecond backend EndTime is truncated first.)</param>
/// <param name="sliceStart">The index into the cluster the emitted slice starts at (clamped to the count).</param>
/// <param name="sliceCount">How many ties the emitted slice contains (may be <c>0</c> on a stale-skip self-heal).</param>
/// <param name="nextStartUtc">The next page's inclusive start: <paramref name="boundaryT"/> 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; }
}
}
@@ -146,6 +146,68 @@ public sealed class NodeManagerHistoryReadPageFillTests(BoundsServerFixture fx)
pages.Select(p => p.Count).ShouldBe([2, 2, 2, 2]);
}
/// <summary>
/// #692 review F1: an oversized tie cluster one step below EndTime used to resume AT EndTime (the drain check was
/// <c>next &lt;= end</c>, but the forward window is end-exclusive), so the next read was a <c>start == end</c> 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.
/// </summary>
[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<DataValueSnapshot>();
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));
}
/// <summary>
/// #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 <c>N + skip + 1</c> rows it knows are ties.
/// </summary>
[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<DataValueSnapshot> { 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<int>();
var all = new List<DataValue>();
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]);
}
/// <summary>
/// #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