fix(historian): #691 review — clamp Gateway at-or-after read, GoodNoData when nothing exists, HonorReturnBounds kill switch

1. The at-or-after bound read on a forward-only backend (Gateway) ends at now + ReverseRawReader.LookAhead instead of DateTime.MaxValue (the gateway only checks start <= end, so a year-9999 end reached AVEVA); a bound beyond the horizon is not found without a read. SqlServer/QuestDB keep MaxValue: they bind it safely and may hold samples further ahead. The Gateway fake now refuses far-future ends, so the missing-end-bound test is no longer vacuous. 3. No data and no bound found (whole read empty) => GoodNoData, no values — the pre-bounds answer (6.5.3.2: success only if one or both bounds exist). A one-value page whose start bound is missing first checks there is data or a found end bound. 4. ServerHistorian:HonorReturnBounds (default true); false ignores ReturnBounds as before #691, including the modified-read Bad_InvalidArgument.
2026-10-12 17:00:40 -04:00
parent ebc6b8d4d9
commit 934909b430
8 changed files with 233 additions and 12 deletions
@@ -317,6 +317,9 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
        // section is absent; NodeManager is non-null here (guarded above).
        _server.NodeManager.MaxTieClusterOverfetch = _serverHistorianOptions.MaxTieClusterOverfetch;

+        // Gitea #691: the ReturnBounds kill switch (default on).
+        _server.NodeManager.HonorReturnBounds = HonorReturnBounds;
+
        // Propagate the S3 HistoryRead concurrency bounds (arch-review 03/S3): bounded per-node fan-out
        // within a batch, the process-wide concurrent-batch limiter, and the per-request deadline. Defaults
        // (4 / 16 / 60 s) survive when the ServerHistorian section is absent.
@@ -347,6 +350,12 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
        _logger.LogInformation("OtOpcUaServerHostedService: SDK started, address-space + ServiceLevel sinks bound");
    }

+    /// <summary>
+    ///     Gitea #691 — the <c>ServerHistorian:HonorReturnBounds</c> kill switch <c>StartAsync</c> applies to the node manager
+    ///     (default <c>true</c> when the section or key is absent). Exposed so the wiring is testable without booting the SDK.
+    /// </summary>
+    internal bool HonorReturnBounds => _serverHistorianOptions.HonorReturnBounds;
+
    /// <summary>
    ///     Gitea #674 — what <c>StartAsync</c> wires onto the node manager for HistoryUpdate: the Primary gate (read from
    ///     the <see cref="IHistoryUpdateRoleView"/> on every call; null when none is registered, i.e. allow), the
@@ -65,6 +65,12 @@ internal static class HistoryBounds
    public static HistoryEndBound For(DateTime startUtc, DateTime endUtc) =>
        new(endUtc, Reverse: startUtc > endUtc, Inclusive: startUtc != endUtc, Open: IsOpen(endUtc));

+    /// <summary>Whether a returned value is a <see cref="NotFound"/> placeholder rather than a stored sample.</summary>
+    /// <param name="value">The value.</param>
+    /// <returns><c>true</c> for a <c>Bad_BoundNotFound</c> placeholder.</returns>
+    public static bool IsPlaceholder(DataValueSnapshot value) =>
+        value.StatusCode == StatusCodes.BadBoundNotFound && value.Value is null;
+
    /// <summary>The pending end bound to store behind a continuation point, remembering the page's last returned time.</summary>
    /// <param name="bound">The pending end bound (null = bounds not requested).</param>
    /// <param name="page">The page being returned.</param>
@@ -90,7 +96,7 @@ internal static class HistoryBounds
    {
        if (IsOpen(startUtc)) return NotFound(startUtc);
        var found = reverse
-            ? await EarliestAtOrAfterAsync(source, tagname, startUtc, ct).ConfigureAwait(false)
+            ? await EarliestAtOrAfterAsync(source, tagname, startUtc, nowUtc, ct).ConfigureAwait(false)
            : await LatestAtOrBeforeAsync(source, tagname, startUtc, maxTieCluster, nowUtc, ct).ConfigureAwait(false);
        if (found is null) return NotFound(startUtc);
        return SameInstant(Ticks(found), startUtc.Ticks, Resolution(source)) ? null : found;
@@ -120,13 +126,13 @@ internal static class HistoryBounds
        if (bound.Reverse)
            found = await LatestAtOrBeforeAsync(source, tagname, bound.AtUtc, maxTieCluster, nowUtc, ct).ConfigureAwait(false);
        else if (bound.Inclusive)
-            found = await EarliestAtOrAfterAsync(source, tagname, bound.AtUtc, ct).ConfigureAwait(false);
+            found = await EarliestAtOrAfterAsync(source, tagname, bound.AtUtc, nowUtc, ct).ConfigureAwait(false);
        else
        {
            var resolution = Resolution(source);
            found = bound.AtUtc.Ticks > DateTime.MaxValue.Ticks - resolution
                ? null
-                : await EarliestAtOrAfterAsync(source, tagname, bound.AtUtc.AddTicks(resolution), ct).ConfigureAwait(false);
+                : await EarliestAtOrAfterAsync(source, tagname, bound.AtUtc.AddTicks(resolution), nowUtc, ct).ConfigureAwait(false);
        }

        return found ?? NotFound(bound.AtUtc);
@@ -172,11 +178,26 @@ internal static class HistoryBounds
    }

    /// <summary>The earliest stored sample at or after <paramref name="atUtc"/>, or null.</summary>
+    /// <remarks>
+    ///     #691 review: on a backend that reads forward only (the Gateway) the read's end is clamped to now +
+    ///     <see cref="ReverseRawReader.LookAhead"/> — the same horizon the walk-back uses — and a time beyond it is "not found"
+    ///     without a read. The HistorianGateway only checks <c>start &lt;= end</c>, so a <c>DateTime.MaxValue</c> end would
+    ///     reach AVEVA as a year-9999 query end (unverified: a rejection, or a fabricated row). The clamp is NOT applied to a
+    ///     backend that reads newest-first natively (SqlServer / QuestDB): they bind <c>MaxValue</c> safely (pinned by the
+    ///     <c>Extreme_bounds</c> conformance test) and can legitimately hold samples further ahead (HistoryUpdate's
+    ///     <c>HistoryUpdateMaxFutureSkew</c> is configurable, and driver-stamped live values carry device time) — clamping
+    ///     there would turn a real end bound into <c>Bad_BoundNotFound</c>.
+    /// </remarks>
    internal static async Task<DataValueSnapshot?> EarliestAtOrAfterAsync(
-        IHistorianDataSource source, string tagname, DateTime atUtc, CancellationToken ct)
+        IHistorianDataSource source, string tagname, DateTime atUtc, DateTime nowUtc, CancellationToken ct)
    {
-        if (atUtc.Ticks >= DateTime.MaxValue.Ticks) return null;
-        var rows = (await source.ReadRawAsync(tagname, atUtc, new DateTime(DateTime.MaxValue.Ticks, DateTimeKind.Utc), 1, ct)
+        var endTicks = DateTime.MaxValue.Ticks;
+        if (!source.SupportsReverseRaw)
+            endTicks = nowUtc.Ticks > DateTime.MaxValue.Ticks - ReverseRawReader.LookAhead.Ticks
+                ? DateTime.MaxValue.Ticks
+                : nowUtc.Ticks + ReverseRawReader.LookAhead.Ticks;
+        if (atUtc.Ticks >= endTicks) return null;
+        var rows = (await source.ReadRawAsync(tagname, atUtc, new DateTime(endTicks, DateTimeKind.Utc), 1, ct)
            .ConfigureAwait(false)).Samples;
        var limit = Align(atUtc.Ticks, Resolution(source));
        foreach (var row in rows)
@@ -544,6 +544,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    /// </summary>
    public int MaxTieClusterOverfetch { get; set; } = 65536;

+    /// <summary>
+    ///     Gitea #691: whether HistoryRead-Raw honours <c>ReturnBounds</c> (Part 11 bounding values, <see cref="HistoryBounds"/>)
+    ///     — <c>ServerHistorian:HonorReturnBounds</c>, default <c>true</c>. <c>false</c> ignores the flag, as before #691,
+    ///     including the modified-read <c>Bad_InvalidArgument</c>.
+    /// </summary>
+    public bool HonorReturnBounds { get; set; } = true;
+
    /// <summary>
    ///     Maximum historized nodes served concurrently within one HistoryRead batch (arch-review 03/S3).
    ///     Mirrors <c>ServerHistorianOptions.HistoryReadBatchParallelism</c>; the Host sets it at
@@ -5056,10 +5063,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            var work = new List<Func<Task>>(nodesToProcess.Count);
            foreach (var handle in nodesToProcess)
            {
-                if (details.IsReadModified && details.ReturnBounds)
+                if (details.IsReadModified && details.ReturnBounds && HonorReturnBounds)
                {
                    // Part 11 §6.5.3.3: returnBounds "shall be set to FALSE" for a modified read, "otherwise the Server returns
-                    // a Bad_InvalidArgument" (#691).
+                    // a Bad_InvalidArgument" (#691). Off with the ServerHistorian:HonorReturnBounds kill switch.
                    errors[handle.Index] = StatusCodes.BadInvalidArgument;
                    results[handle.Index] = new SdkHistoryReadResult { StatusCode = StatusCodes.BadInvalidArgument };
                    continue;
@@ -5079,7 +5086,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                work.Add(() => ServeRawPagedAsync(
                    handle, session, nodesToRead, results, errors,
                    details.StartTime, details.EndTime, details.NumValuesPerNode,
-                    ct, details.ReturnBounds));
+                    ct, details.ReturnBounds && HonorReturnBounds));
            }

            RunBounded(work, HistoryReadBatchParallelism);
@@ -6306,6 +6313,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                    PendingEndBound: HistoryBounds.Carry(endBound!, page)! with { DataExhausted = true }));
            }

+            // #691 review: whether a bounded read whose start bound was NOT found has any data or a found end bound — only
+            // asked when a one-value page would otherwise be the start-bound placeholder alone.
+            async Task<bool> BoundedReadHasAnythingAsync()
+            {
+                var src = HistorianDataSource;
+                IReadOnlyList<DataValueSnapshot> first = startUtc > endUtc && !src.SupportsReverseRaw
+                    ? (await ReverseRawReader.ReadAsync(src, tagname, startUtc, endUtc, 1, MaxTieClusterOverfetch, nowUtc, 0, ct)
+                        .ConfigureAwait(false)).Samples
+                    : (await src.ReadRawAsync(tagname, startUtc, endUtc, 1, ct).ConfigureAwait(false)).Samples;
+                if (first.Any(x => HistoryBounds.InDataWindow(x, endBound!, resolution))) return true;
+                var end = await HistoryBounds.EndBoundAsync(
+                    src, tagname, endBound!, startUtc, MaxTieClusterOverfetch, nowUtc, ct).ConfigureAwait(false);
+                return !HistoryBounds.IsPlaceholder(end);
+            }
+
            // #691: a resumed bounded read whose data is drained (the end bound did not fit on the previous page, or the
            // resume point already sits on the end time) owes only the end bound.
            if (endBound is not null && inboundCp is { Length: > 0 }
@@ -6332,7 +6354,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                startBound = await HistoryBounds.StartBoundAsync(
                        HistorianDataSource, tagname, startUtc, endBound.Reverse, MaxTieClusterOverfetch, nowUtc, ct)
                    .ConfigureAwait(false);
-                if (startBound is not null && numValuesPerNode == 1)
+                if (startBound is not null && numValuesPerNode == 1
+                    && (!HistoryBounds.IsPlaceholder(startBound) || await BoundedReadHasAnythingAsync().ConfigureAwait(false)))
                {
                    // The page is the start bound alone; the data starts at StartTime on the next page.
                    var boundOnlyCp = _historyContinuationStore.Save(session, new HistoryContinuationState(
@@ -6349,6 +6372,19 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                    return;
                }

+                if (startBound is not null && numValuesPerNode == 1)
+                {
+                    // Nothing at all — no data and no bound found (review of #691): the pre-bounds GoodNoData answer.
+                    results[handle.Index] = new SdkHistoryReadResult
+                    {
+                        StatusCode = StatusCodes.GoodNoData,
+                        HistoryData = new ExtensionObject(ToHistoryDataFromSamples([])),
+                        ContinuationPoint = null,
+                    };
+                    errors[handle.Index] = ServiceResult.Good;
+                    return;
+                }
+
                if (startBound is not null && numValuesPerNode > 0) readCap = numValuesPerNode - 1;
            }

@@ -6510,6 +6546,15 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                historyData = ToHistoryDataFromSamples(samples);
            }

+            // #691 review: a whole bounded read with no data and no bound found (one page, every value a placeholder) is the
+            // pre-bounds answer — GoodNoData, no values (§6.5.3.2: bounds make it a success only "if one or both exist").
+            if (endBound is not null && inboundCp is not { Length: > 0 } && outboundCp is null
+                && samples.All(HistoryBounds.IsPlaceholder))
+            {
+                samples = [];
+                historyData = ToHistoryDataFromSamples(samples);
+            }
+
            results[handle.Index] = new SdkHistoryReadResult
            {
                // No samples ⇒ GoodNoData (the node is historized, the window just held no data). With the
@@ -89,6 +89,14 @@ public sealed class ServerHistorianOptions
    /// </summary>
    public int MaxTieClusterOverfetch { get; init; } = 65536;

+    /// <summary>
+    ///     Gitea #691: whether HistoryRead-Raw honours <c>ReadRawModifiedDetails.ReturnBounds</c> (OPC UA Part 11 bounding
+    ///     values). Default <c>true</c>. <c>false</c> is the kill switch back to the pre-#691 answer: the flag is ignored
+    ///     (stored samples only) and a modified read carrying it is not refused <c>Bad_InvalidArgument</c>. Note the .NET
+    ///     SDK's <c>new ReadRawModifiedDetails()</c> defaults <c>ReturnBounds</c> (and <c>IsReadModified</c>) to <c>true</c>.
+    /// </summary>
+    public bool HonorReturnBounds { get; init; } = true;
+
    /// <summary>
    ///     Maximum number of historized nodes served CONCURRENTLY within one HistoryRead batch (arch-review
    ///     03/S3). The SDK's <c>HistoryRead*</c> override surface is synchronous, so each arm block-bridges