fix(historian): QuestDB modified reads keep a landed change whose marker was lost; H2 review NITs (lmxopcua#778)

QuestDB modified-history reads dropped every still-pending trail row that was not the latest at its source time, so V0->V1 (applied marker never written) followed by V1->V2 hid the V0->V1 change for good. A pending row below the latest is now served when the NEXT trail row at that time recorded a different prior value (kind, value, status or the prior's server_ts) - the next change saw what this one wrote; an orphan of a change that never landed leaves the same prior for the next row and stays hidden. QuestDB window functions reject VARCHAR, so the next row's prior comes from a LEFT JOIN on (ts, lead(id)). The updater's comment and marker-failure log no longer claim modified reads "still serve it" unconditionally. Regression tests (QuestDB 10.0.1 throwaway container): marker lost then a replace / delete at the same time; an orphan followed by a marker-lost retry then a later change (orphan still dropped); the random marker-mix fuzz now varies the recorded priors. All four fail on the old filter. NITs: - SqlServerHistorianValueWriter: a SqlException raised while the token is cancelled (SqlClient's "Operation cancelled by user") is rethrown as OperationCanceledException instead of booked as a failed write. Integration test holds a TABLOCKX on Sample and cancels mid-INSERT; it returned false before. - SqlServerHistorianDataUpdater.DeleteRawBatches is an Interlocked counter. - HistoryReadEvents: an event filter whose SDK validation throws (unknown operator) failed with the generic BadUnexpectedError; the node manager now re-throws any failed event-filter validation as BadEventFilterInvalid. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 16:04:44 -05:00
parent c802f0e7ed
commit e790938f94
9 changed files with 245 additions and 37 deletions
@@ -175,7 +175,7 @@ public sealed class QuestDbHistorianDataSource : IHistorianDataSource
    // (ts, modified_id); maxValues + 1 rows are fetched to report HasMore exactly. A row whose status is
    // NULL had no prior value (an insert) and is served null / BadNoData. Because the trail row lands
    // first, a crash between it and the sample write leaves an orphan row describing a change that never
-    // applied (documented).
+    // applied; on a marker-format trail such a row stays `pending` and is reconciled below.
    /// <inheritdoc />
    public async Task<HistoryModifiedReadResult> ReadModifiedAsync(
        string fullReference, DateTime startUtc, DateTime endUtc, uint maxValues, HistoryModifiedCursor? resumeAfter,
@@ -211,20 +211,37 @@ public sealed class QuestDbHistorianDataSource : IHistorianDataSource
                                       "modification_ts, update_type, principal";

                // The applied marker: a trail row still `pending` is served only when the change it describes evidently
-                // landed — it is the LATEST row at its time and the sample there (a tombstone included) was written at or
-                // after the modification: a crash between the change and its marker. Otherwise it is the orphan of a change
-                // that never landed (typically followed by the client's successful retry, which is then the latest row) and
-                // is dropped. The ASOF JOIN reads the tag's samples of the window only (indexed tag, inclusive bounds); the
-                // resume position filters OUTSIDE the window function so it never cuts a time's partition.
+                // landed. Two pieces of evidence, one per position in its time's trail:
+                //  - it is the LATEST row at its time and the sample there (a tombstone included) was written at or after
+                //    the modification — a crash, or a failed marker write, between the change and its marker;
+                //  - it is NOT the latest and the NEXT row at its time recorded a different prior value — the next change
+                //    saw what this one wrote (lmxopcua#778: V0→V1 lands, its marker fails, a later V1→V2 must not hide it).
+                //    A change that never landed left the sample alone, so the next row records the same prior value as
+                //    this one (kind, value columns, status and the prior's server_ts — a rewrite always moves server_ts),
+                //    and the row is the orphan of that failed change (typically followed by the client's successful
+                //    retry) and is dropped.
+                // QuestDB window functions do not take VARCHAR, so the next row's prior is fetched by a LEFT JOIN on
+                // (ts, next modified_id) rather than lead(). The ASOF JOIN reads the tag's samples of the window only
+                // (indexed tag, inclusive bounds); the resume position filters OUTSIDE the window functions so it never
+                // cuts a time's partition.
                var sql = _bootstrap.SupportsTrailMarker
-                    ? $"SELECT {Columns} FROM (SELECT m.ts ts, m.kind kind, m.num_value num_value, m.int_value int_value, " +
+                    ? $"SELECT {Columns} FROM (SELECT x.ts ts, x.kind kind, x.num_value num_value, x.int_value int_value, " +
+                      "x.str_value str_value, x.status status, x.server_ts server_ts, x.modified_id modified_id, " +
+                      "x.modification_ts modification_ts, x.update_type update_type, x.principal principal, " +
+                      "(NOT x.pending OR (x.next_id = NULL AND x.sample_ts = x.ts AND x.sample_server_ts >= x.modification_ts) " +
+                      "OR (x.next_id != NULL AND NOT (n.kind = x.kind AND n.num_value = x.num_value AND n.int_value = x.int_value " +
+                      "AND n.str_value = x.str_value AND n.status = x.status AND n.server_ts = x.server_ts))) served " +
+                      "FROM (SELECT m.ts ts, m.kind kind, m.num_value num_value, m.int_value int_value, " +
                      "m.str_value str_value, m.status status, m.server_ts server_ts, m.modified_id modified_id, " +
                      "m.modification_ts modification_ts, m.update_type update_type, m.principal principal, m.pending pending, " +
-                      "s.ts sample_ts, s.server_ts sample_server_ts, max(m.modified_id) OVER (PARTITION BY m.ts) latest_id " +
+                      "s.ts sample_ts, s.server_ts sample_server_ts, " +
+                      "lead(m.modified_id) OVER (PARTITION BY m.ts ORDER BY m.modified_id) next_id " +
                      $"FROM {_conn.SampleModifiedTable} m ASOF JOIN (SELECT ts, tag, server_ts FROM {_conn.SampleTable} " +
-                      $"WHERE tag = $1 AND ts >= $2 AND ts <= $3) s ON (tag) WHERE m.tag = $1 AND {window.Replace("ts ", "m.ts ")}) " +
-                      "WHERE (NOT pending OR (sample_ts = ts AND sample_server_ts >= modification_ts AND modified_id = latest_id))" +
-                      $"{resume} ORDER BY ts {order}, modified_id {order}"
+                      $"WHERE tag = $1 AND ts >= $2 AND ts <= $3) s ON (tag) WHERE m.tag = $1 AND {window.Replace("ts ", "m.ts ")}) x " +
+                      "LEFT JOIN (SELECT ts, modified_id, kind, num_value, int_value, str_value, status, server_ts " +
+                      $"FROM {_conn.SampleModifiedTable} WHERE tag = $1 AND ts >= $2 AND ts <= $3) n " +
+                      "ON (x.ts = n.ts AND x.next_id = n.modified_id)) " +
+                      $"WHERE served{resume} ORDER BY ts {order}, modified_id {order}"
                    : $"SELECT {Columns} FROM {_conn.SampleModifiedTable} " +
                      $"WHERE tag = $1 AND {window}{resume} ORDER BY ts {order}, modified_id {order}";

@@ -35,9 +35,11 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.QuestDb;
 ///     <para><b>Applied marker.</b> On a trail table with the marker format
 ///     (<see cref="QuestDbSchemaBootstrapper.SupportsTrailMarker"/>) the trail row is written <c>pending</c> and, once the
 ///     sample change has landed, re-upserted with <c>pending = false</c> (same <c>(ts, tag, modified_id)</c> — the table's
-///     dedup key). A modified read drops a pending row unless the sample at its time was written at or after the
-///     modification (the change landed but the marker did not — a crash in between), so an orphan is no longer served as
-///     a change that happened. The trail INSERT is re-sent once (same ids) after a transient failure: a first attempt that
+///     dedup key). A modified read drops a pending row unless it can tell the change landed: as the latest row at its
+///     time, the sample there was written at or after the modification (a crash or a failed marker in between); below a
+///     later row at that time, that next row recorded a different prior value (the next change saw this one's result —
+///     lmxopcua#778). So an orphan is no longer served as a change that happened, and a change whose marker was lost stays
+///     served after later changes at the same time. The trail INSERT is re-sent once (same ids) after a transient failure: a first attempt that
 ///     did apply converges through the dedup key instead of duplicating.</para>
 /// </summary>
 internal sealed class QuestDbHistorianDataUpdater : IHistorianDataUpdater
@@ -361,8 +363,10 @@ internal sealed class QuestDbHistorianDataUpdater : IHistorianDataUpdater

    /// <summary>
    ///     The applied marker: re-upserts the trail rows of a change that has landed with <c>pending = false</c> (same
-    ///     <c>(ts, tag, modified_id)</c> key). A failure here does not fail the update — the change landed, and a modified read
-    ///     still serves a pending row whose sample shows the change (logged as a Warning).
+    ///     <c>(ts, tag, modified_id)</c> key). A failure here does not fail the update — the change landed — and is logged as
+    ///     a Warning. Its rows stay <c>pending</c>; a modified read serves each one only while the evidence holds: as the
+    ///     latest row at its time, when the sample there was written at or after the modification; once a later change at
+    ///     that time exists, when that change's trail row recorded a different prior value (lmxopcua#778).
    /// </summary>
    private async Task MarkAppliedAsync(string tag, IReadOnlyList<TrailRow> rows, CancellationToken ct)
    {
@@ -375,8 +379,9 @@ internal sealed class QuestDbHistorianDataUpdater : IHistorianDataUpdater
        }
        catch (Exception ex) when (ex is not OperationCanceledException)
        {
-            _log.LogWarning(ex, "QuestDB applied marker for {Count} trail row(s) of tag {Tag} was not written; the change landed and " +
-                "modified reads still serve it.", rows.Count, tag);
+            _log.LogWarning(ex, "QuestDB applied marker for {Count} trail row(s) of tag {Tag} was not written; the change landed, but " +
+                "the rows stay pending and modified reads serve them only while the sample at their time, or the next change's " +
+                "recorded prior value, shows the change.", rows.Count, tag);
        }
    }

@@ -126,7 +126,7 @@ internal sealed class SqlServerHistorianDataUpdater : IHistorianDataUpdater
                await tx.CommitAsync(ct).ConfigureAwait(false);
            }
            rows += batch;
-            DeleteRawBatches++;
+            Interlocked.Increment(ref _deleteRawBatches);
            if (batch < DeleteRawBatchSize) break;
        }
        return HistoryUpdatePlanner.DeleteRawResult(tagKnown: true, rows);
@@ -138,8 +138,10 @@ internal sealed class SqlServerHistorianDataUpdater : IHistorianDataUpdater
    /// </summary>
    internal int DeleteRawBatchSize { get; set; } = 4000;

-    /// <summary>DeleteRaw batches run so far (lifetime) — test/diagnostic.</summary>
-    internal int DeleteRawBatches { get; private set; }
+    private int _deleteRawBatches;
+
+    /// <summary>DeleteRaw batches run so far (lifetime) — test/diagnostic. Atomic: concurrent DeleteRaw calls share it.</summary>
+    internal int DeleteRawBatches => Volatile.Read(ref _deleteRawBatches);

    /// <inheritdoc/>
    public async Task<IReadOnlyList<uint>> DeleteAtTimeAsync(string tag, IReadOnlyList<DateTime> timesUtc,
@@ -95,6 +95,12 @@ internal sealed class SqlServerHistorianValueWriter : IHistorianValueWriter
        {
            throw; // the drain pass was cancelled (shutdown) — not a write failure of this tag
        }
+        catch (SqlException ex) when (ct.IsCancellationRequested)
+        {
+            // SqlClient reports a command cancelled mid-flight as a SqlException ("Operation cancelled by user"), not an
+            // OperationCanceledException: still the drain pass's cancellation, never a write failure of this tag.
+            throw new OperationCanceledException("The SQL historian live-value write was cancelled.", ex, ct);
+        }
        catch (Exception ex)
        {
            _log.LogWarning("SQL historian live-value write failed ({Exception}); will retry.", ex.GetType().Name);