fix(localdb): fault live sync session on cap-prune; keepalive + newest-wins sync guard (0.3.2, ScadaBridge arch-review P4-1 + P4-3)

2026-08-19 14:49:31 -04:00
parent dbcbc5bd41
commit f62b017cc7
13 changed files with 574 additions and 21 deletions
@@ -24,8 +24,9 @@ internal sealed class OplogStore(ILocalDb db, ReplicationOptions options, Func<D

    /// <summary>
    /// Reads up to <paramref name="maxBatch"/> oplog entries with <c>seq &gt; afterSeq</c> in ascending
-    /// seq order. Caller contract: check <c>needs_snapshot</c> before trusting incremental reads —
-    /// after cap-pruning, reads silently resume past a gap.
+    /// seq order. Caller contract: this read resumes SILENTLY past a cap-pruned gap, so a caller
+    /// streaming incrementally must first confirm its position is still below the horizon — see
+    /// <see cref="GetMinSeqAsync"/>, which <c>SyncSession</c>'s pump checks on every iteration.
    /// </summary>
    public Task<IReadOnlyList<OplogEntryRecord>> ReadBatchAboveAsync(long afterSeq, int maxBatch, CancellationToken ct = default) =>
        db.QueryAsync(
@@ -148,6 +149,24 @@ internal sealed class OplogStore(ILocalDb db, ReplicationOptions options, Func<D
        return rows[0];
    }

+    /// <summary>
+    /// The oldest oplog seq still present — the delta horizon — or 0 when the oplog is empty.
+    /// </summary>
+    /// <remarks>
+    /// One b-tree seek (<c>seq</c> is the rowid and SQLite optimizes MIN into a seek), which is what
+    /// makes it affordable for the pump to check on every flush interval. Compared against a
+    /// session's sent watermark it answers the only question the pump has: is everything I have not
+    /// yet sent still here? Ack-pruning deletes <c>seq &lt;= last_acked_seq</c> and the peer can only
+    /// ack what it received, so in a healthy session the horizon never climbs past
+    /// <c>sentThruSeq + 1</c>; a higher one means a cap prune ate unsent deltas.
+    /// </remarks>
+    public async Task<long> GetMinSeqAsync(CancellationToken ct = default)
+    {
+        var rows = await db.QueryAsync(
+            "SELECT COALESCE(MIN(seq), 0) FROM __localdb_oplog", static r => r.GetInt64(0), null, ct);
+        return rows[0];
+    }
+
    /// <summary>The highest oplog seq present, or 0 when the oplog is empty.</summary>
    public async Task<long> GetMaxSeqAsync(CancellationToken ct = default)
    {
@@ -356,9 +356,14 @@ internal sealed class SyncSession
        while (true)
        {
            ct.ThrowIfCancellationRequested();
+            // BEFORE the read, because the read itself cannot tell a pruned gap from an idle oplog.
+            ThrowIfPruneGap(await _store.GetMinSeqAsync(ct));
            var batch = await _store.ReadBatchAboveAsync(_sentThruSeq, _options.MaxBatchSize, ct);
            if (batch.Count > 0)
            {
+                // The exact guard on the rows about to go out, for a prune that landed between the
+                // probe and the read. Free (the seq is already in hand) and strictly tighter.
+                ThrowIfPruneGap(batch[0].Seq);
                var index = 0;
                while (index < batch.Count)
                {
@@ -390,6 +395,40 @@ internal sealed class SyncSession
        }
    }

+    /// <summary>
+    /// Faults the session when the oplog horizon has climbed above this session's read position —
+    /// i.e. a cap prune deleted deltas between <see cref="_sentThruSeq"/> and
+    /// <paramref name="oldestAvailableSeq"/>. An empty oplog (0) is not a gap.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <b>Why a live session has to die for this.</b> <c>EnforceCapsAsync</c> sets
+    /// <c>needs_snapshot</c> in the same transaction as the prune, so a RECONNECT heals — the
+    /// handshake serves a full snapshot instead of deltas. Nothing, however, made the live session
+    /// notice: the pump reads <c>seq &gt; _sentThruSeq</c>, which silently resumes above the hole, and
+    /// the peer acks those later seqs as if the pair were converged. On a stable LAN with a slow but
+    /// alive peer that session can run for days, and the pair is durably divergent for all of it —
+    /// the flag that would fix it is only ever read at handshake time. Tearing down is what turns a
+    /// persisted "needs snapshot" into an actual snapshot.
+    /// </para>
+    /// <para>
+    /// <b>Why the horizon, not the flag.</b> <c>needs_snapshot</c> is also set for reasons that do
+    /// not invalidate THIS session's position (and a full snapshot this session just streamed clears
+    /// it only when the peer's <c>SnapshotAck</c> lands, which is after the pump has started).
+    /// Comparing the horizon against the sent watermark asks the precise question instead, so a
+    /// session is faulted when — and only when — rows it still owes the peer are gone.
+    /// </para>
+    /// </remarks>
+    private void ThrowIfPruneGap(long oldestAvailableSeq)
+    {
+        if (oldestAvailableSeq == 0 || oldestAvailableSeq <= _sentThruSeq + 1)
+            return;
+        throw new LocalDbOplogGapException(
+            $"Oplog deltas above the sent watermark {_sentThruSeq} were pruned (oldest surviving seq " +
+            $"{oldestAvailableSeq}): this session can no longer stream incrementally without skipping " +
+            "them. Ending the session so the reconnect handshake serves a snapshot resync.");
+    }
+
    /// <summary>
    /// Serialized bytes one entry contributes to its batch message: the entry's own encoding plus
    /// the repeated field's tag and length prefix, as protobuf itself computes them.
@@ -0,0 +1,23 @@
+namespace ZB.MOM.WW.LocalDb.Replication;
+
+/// <summary>
+/// Thrown by a live sync session when the oplog horizon has moved ABOVE the position that session
+/// is streaming from: a cap prune deleted deltas it had not yet sent, so every later delta it could
+/// ship would land on the far side of a hole the peer will never receive.
+/// </summary>
+/// <remarks>
+/// This is not a transport failure and not a data error — it is the session discovering that its own
+/// incremental position has become unsound. Nothing can repair it in-session (the deleted rows are
+/// gone), so the session tears itself down; the same prune set <c>needs_snapshot</c>, which makes the
+/// reconnect handshake serve a snapshot instead of deltas. Catching this and continuing would
+/// reintroduce exactly the silent divergence it exists to stop.
+/// </remarks>
+public sealed class LocalDbOplogGapException : Exception
+{
+    /// <summary>Creates the exception with a message describing the gap.</summary>
+    public LocalDbOplogGapException(string message) : base(message) { }
+
+    /// <summary>Creates the exception with a message and the underlying cause.</summary>
+    public LocalDbOplogGapException(string message, Exception innerException)
+        : base(message, innerException) { }
+}
@@ -21,8 +21,26 @@ public sealed class LocalDbSyncService : LocalDbSync.LocalDbSyncBase
    private readonly LocalDbMetrics? _metrics;
    private readonly SyncStatus? _status;

-    // 0 = idle, 1 = a stream is active. Interlocked-guarded across concurrent calls (singleton service).
-    private int _active;
+    // The stream currently holding the guard, or null when idle. Guarded by _gate (singleton service).
+    private readonly object _gate = new();
+    private Registration? _active;
+
+    /// <summary>
+    /// How long a displacing call waits for the incumbent it evicted to actually finish before
+    /// giving up and refusing itself. Test seam; the default is generous because eviction only has
+    /// to outrun the operator, not the network.
+    /// </summary>
+    internal TimeSpan TakeoverTimeout { get; set; } = TimeSpan.FromSeconds(10);
+
+    /// <summary>One in-flight <c>Sync</c> handler: its cancellation handle plus a completion signal
+    /// a displacing handler can await, so two sessions never run against the database at once.</summary>
+    private sealed class Registration(CancellationTokenSource cts)
+    {
+        public CancellationTokenSource Cts { get; } = cts;
+
+        public TaskCompletionSource Finished { get; } =
+            new(TaskCreationOptions.RunContinuationsAsynchronously);
+    }

    /// <summary>
    /// Creates the passive sync service. <paramref name="status"/> is taken as the public
@@ -41,21 +59,47 @@ public sealed class LocalDbSyncService : LocalDbSync.LocalDbSyncBase
        _status = status as SyncStatus;
    }

+    /// <summary>
+    /// Accepts one inbound sync stream, evicting any stream already holding the guard.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <b>Newest wins.</b> The guard exists to SERIALIZE sessions — a node syncs with exactly one
+    /// peer, and two sessions against one database would fight over the same watermarks — not to
+    /// fence out an impostor; authentication is the interceptor's job, and an authenticated caller
+    /// on this key IS the peer by definition. Refusing the newcomer therefore protected nothing and
+    /// cost everything in the case that actually happens: the dialling node crashes or is
+    /// partitioned, this handler stays parked forever inside <c>MoveNext</c> on a half-open stream
+    /// that will never produce another byte, and every reconnect from the rebooted peer is rejected
+    /// <c>AlreadyExists</c> — potentially for as long as the pair stays idle, while this side keeps
+    /// reporting itself connected. The newest dialler is the one with a live link, so it takes over.
+    /// </para>
+    /// <para>
+    /// Eviction is cancel-then-await, not cancel-and-go: the incumbent's cancellation unwinds its
+    /// session and reader pump, and the newcomer waits for that to complete before starting its own,
+    /// so the "one session at a time" property the guard was built for still holds. An incumbent
+    /// that cannot be evicted within <see cref="TakeoverTimeout"/> is the one case where the old
+    /// answer is still right — the newcomer refuses itself rather than double-running.
+    /// </para>
+    /// </remarks>
    public override async Task Sync(
        IAsyncStreamReader<SyncMessage> requestStream,
        IServerStreamWriter<SyncMessage> responseStream,
        ServerCallContext context)
    {
-        if (Interlocked.CompareExchange(ref _active, 1, 0) != 0)
-            throw new RpcException(new Status(StatusCode.AlreadyExists, "a sync stream is already active"));
+        // The linked source exists so the reader pump is deterministically torn down on EVERY exit
+        // path — including a session fault for a local (non-transport) reason, where the call token
+        // never fires and the pump would otherwise keep reading a stream whose handler has exited.
+        // It is also the handle a later call cancels to displace this one.
+        using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(context.CancellationToken);
+        var mine = new Registration(linkedCts);

        try
        {
-            // The linked source exists so the reader pump is deterministically torn down on EVERY
-            // exit path — including a session fault for a local (non-transport) reason, where the
-            // call token never fires and the pump would otherwise keep reading a stream whose
-            // handler has exited.
-            using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(context.CancellationToken);
+            // Inside the try so the finally below always releases the guard — including when the
+            // takeover itself is abandoned because THIS caller gave up mid-handover.
+            await TakeOverAsync(mine, context.CancellationToken);
+
            var (duplex, readerTask) = GrpcSyncDuplex.Create(requestStream, responseStream, linkedCts.Token);
            var session = SyncSessionFactory.Create(_db, _options, _loggerFactory, _metrics, _status);
            _status?.SessionStarted();
@@ -81,13 +125,66 @@ public sealed class LocalDbSyncService : LocalDbSync.LocalDbSyncBase
        {
            // Client disconnect / server shutdown: the normal end of a long-lived streaming RPC.
        }
+        catch (OperationCanceledException)
+        {
+            // Displaced by a newer stream: an orderly handover, not a failure of this node.
+            _logger.LogInformation("Sync stream ended: displaced by a newer stream from the peer.");
+        }
        catch (LocalDbSchemaMismatchException ex)
        {
            throw new RpcException(new Status(StatusCode.FailedPrecondition, ex.Message));
        }
        finally
        {
-            Interlocked.Exchange(ref _active, 0);
+            lock (_gate)
+            {
+                if (ReferenceEquals(_active, mine))
+                    _active = null;
+            }
+            mine.Finished.TrySetResult();
+        }
+    }
+
+    /// <summary>
+    /// Claims the single-stream guard for <paramref name="mine"/>, cancelling and awaiting whatever
+    /// held it. Throws <c>AlreadyExists</c> only when the incumbent refuses to end.
+    /// </summary>
+    private async Task TakeOverAsync(Registration mine, CancellationToken ct)
+    {
+        Registration? displaced;
+        lock (_gate)
+        {
+            displaced = _active;
+            _active = mine;
+        }
+        if (displaced is null)
+            return;
+
+        _logger.LogWarning(
+            "A new sync stream arrived while one was already active; displacing the incumbent (newest wins).");
+        try
+        {
+            // The incumbent may already be disposing its own source between its finally and here.
+            displaced.Cts.Cancel();
+        }
+        catch (ObjectDisposedException) { }
+
+        try
+        {
+            await displaced.Finished.Task.WaitAsync(TakeoverTimeout, ct);
+        }
+        catch (TimeoutException)
+        {
+            // Could not evict it: two live sessions over one database is worse than a refused dial,
+            // so fall back to the historical answer — and hand the guard back to the incumbent.
+            lock (_gate)
+            {
+                if (ReferenceEquals(_active, mine))
+                    _active = displaced;
+            }
+            throw new RpcException(new Status(
+                StatusCode.AlreadyExists,
+                "a sync stream is already active and did not yield within the takeover timeout"));
        }
    }
 }
@@ -54,7 +54,7 @@ public sealed class MaintenanceBackgroundService : BackgroundService
                {
                    _snapshotFlagWarned = true;
                    _logger.LogWarning(
-                        "Oplog backlog/age caps exceeded: pruned to the ceiling and flagged needs_snapshot — the peer must snapshot-resync. Not re-logged until a clean tick.");
+                        "Oplog backlog/age caps exceeded: pruned to the ceiling and flagged needs_snapshot — the peer must snapshot-resync. A session streaming from below the new horizon ends itself on its next pump tick so the reconnect serves that snapshot. Not re-logged until a clean tick.");
                }
                else if (!snapshotFlagged)
                {
@@ -64,6 +64,34 @@ public sealed class ReplicationOptions
    /// <summary>Upper bound on the exponential reconnect backoff.</summary>
    public TimeSpan ReconnectBackoffMax { get; set; } = TimeSpan.FromSeconds(60);

+    /// <summary>
+    /// How long the initiator's HTTP/2 connection may sit without a keepalive PING before one is
+    /// sent. Default 60s.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// A sync stream is long-lived and, on a converged pair, almost entirely silent — which is
+    /// exactly the shape a half-open TCP connection hides behind. Without pings, a hard crash or a
+    /// partition of one node leaves the other holding a stream that will never produce another byte
+    /// and never fail either: the initiator sits in a session it believes is healthy, and the
+    /// passive node's handler sits inside its single-stream guard, refusing the reconnect that would
+    /// heal the pair. Pings turn that into a detected failure on both ends.
+    /// </para>
+    /// <para>
+    /// Sent with <c>KeepAlivePingPolicy.Always</c>, so they flow on an idle connection too — an
+    /// active-streams-only policy would not fire in precisely the silent-pair case above. Safe over
+    /// h2c: a PING is an HTTP/2 frame, nothing to do with TLS.
+    /// </para>
+    /// </remarks>
+    public TimeSpan KeepAlivePingDelay { get; set; } = TimeSpan.FromSeconds(60);
+
+    /// <summary>
+    /// How long the initiator waits for a keepalive PING ack before tearing the connection down.
+    /// Default 20s. Detection therefore lands within roughly
+    /// <see cref="KeepAlivePingDelay"/> + this.
+    /// </summary>
+    public TimeSpan KeepAlivePingTimeout { get; set; } = TimeSpan.FromSeconds(20);
+
    /// <summary>How often the maintenance service enforces the oplog caps and prunes expired tombstones.</summary>
    public TimeSpan MaintenanceInterval { get; set; } = TimeSpan.FromSeconds(60);

@@ -40,6 +40,10 @@ public sealed class ReplicationOptionsValidator : IValidateOptions<ReplicationOp
            failures.Add($"LocalDb:Replication:ReconnectBackoffMax must be greater than zero; got {options.ReconnectBackoffMax}.");
        else if (options.ReconnectBackoffMax > ReconnectBackoffCeiling)
            failures.Add($"LocalDb:Replication:ReconnectBackoffMax must be at most {ReconnectBackoffCeiling}; got {options.ReconnectBackoffMax}.");
+        if (options.KeepAlivePingDelay <= TimeSpan.Zero)
+            failures.Add($"LocalDb:Replication:KeepAlivePingDelay must be greater than zero; got {options.KeepAlivePingDelay}.");
+        if (options.KeepAlivePingTimeout <= TimeSpan.Zero)
+            failures.Add($"LocalDb:Replication:KeepAlivePingTimeout must be greater than zero; got {options.KeepAlivePingTimeout}.");
        if (options.MaintenanceInterval <= TimeSpan.Zero)
            failures.Add($"LocalDb:Replication:MaintenanceInterval must be greater than zero; got {options.MaintenanceInterval}.");
        if (options.MaxHlcDriftAhead <= TimeSpan.Zero)
@@ -1,3 +1,4 @@
+using System.Net.Http;
 using Grpc.Core;
 using Grpc.Net.Client;
 using Microsoft.Extensions.Hosting;
@@ -76,7 +77,7 @@ public sealed class SyncBackgroundService : BackgroundService
            {
                var channel = ChannelFactory is { } factory
                    ? factory()
-                    : ownedChannel = GrpcChannel.ForAddress(_options.PeerAddress);
+                    : ownedChannel = DialPeer();

                var client = new LocalDbSync.LocalDbSyncClient(channel);
                var headers = new Metadata();
@@ -152,6 +153,39 @@ public sealed class SyncBackgroundService : BackgroundService
        }
    }

+    /// <summary>
+    /// Dials the peer over a channel that sends HTTP/2 keepalive PINGs, so a half-open link is
+    /// detected instead of being waited on forever.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// The bare <c>GrpcChannel.ForAddress(address)</c> this replaces had NO keepalive of any kind,
+    /// and neither end of a sync stream ever times out on silence by design (a converged pair is
+    /// legitimately quiet). A hard crash or partition of the peer therefore left this node inside a
+    /// session that could never make progress and could never fail — and its own half of that stream
+    /// pinned on the peer, where the single-stream guard turns it into a refusal of the very
+    /// reconnect that heals the pair.
+    /// </para>
+    /// <para>
+    /// <c>DisposeHttpClient</c> hands the handler's lifetime to the channel, which this loop already
+    /// disposes on every exit path.
+    /// </para>
+    /// </remarks>
+    private GrpcChannel DialPeer() =>
+        GrpcChannel.ForAddress(_options.PeerAddress, new GrpcChannelOptions
+        {
+            HttpHandler = new SocketsHttpHandler
+            {
+                KeepAlivePingDelay = _options.KeepAlivePingDelay,
+                KeepAlivePingTimeout = _options.KeepAlivePingTimeout,
+                // Always, not WithActiveRequests: the case being closed is a SILENT stream on a dead
+                // link, and an active-requests policy stops pinging once the peer stops responding.
+                KeepAlivePingPolicy = HttpKeepAlivePingPolicy.Always,
+                EnableMultipleHttp2Connections = true,
+            },
+            DisposeHttpClient = true,
+        });
+
    private static bool IsSchemaMismatch(Exception? ex) =>
        ex is LocalDbSchemaMismatchException
        || (ex is RpcException rpc && rpc.StatusCode == StatusCode.FailedPrecondition);