fix(comms): reconnect on graceful stream completion — kills the 4h silent stream death

2026-08-14 19:57:08 -04:00
parent ee193cd2bb
commit 34a3f4bb69
9 changed files with 507 additions and 45 deletions
@@ -400,6 +400,67 @@ public class DebugStreamBridgeActorTests : TestKit
        Assert.Equal("corr-1", factory.ClientFor(GrpcNodeB).SubscribeCalls[0].CorrelationId);
    }

+    // ── WP1.1: graceful (status OK) stream completion is a reconnect trigger ──
+
+    [Fact]
+    public void On_GracefulStreamCompletion_Reopens_On_The_Same_Node()
+    {
+        // The site caps every stream at GrpcMaxStreamLifetime and then ends the RPC with
+        // OK. Pre-fix the client's read loop just finished, the actor was told nothing, and
+        // the debug session went silently deaf for the rest of its life.
+        var (_, factory) = CreateBridgeWithTrackingFactory();
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).SubscribeCalls.Count == 1,
+            TimeSpan.FromSeconds(3));
+
+        factory.ClientFor(GrpcNodeA).SubscribeCalls[0].OnCompleted();
+
+        // Reopened on the SAME node — a clean close is not a fault, so there is nothing to
+        // fail over from — and the finished stream is released, not left zombie.
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).SubscribeCalls.Count == 2,
+            TimeSpan.FromSeconds(5));
+        Assert.Empty(factory.ClientFor(GrpcNodeB).SubscribeCalls);
+        Assert.Contains("corr-1", factory.ClientFor(GrpcNodeA).UnsubscribedCorrelationIds);
+    }
+
+    [Fact]
+    public void RepeatedGracefulCompletions_DoNotConsume_TheErrorRetryBudget()
+    {
+        // Five completions — two more than MaxRetries. Had completion been routed through
+        // the error path the session would have terminated on the fourth.
+        var ctx = CreateBridgeActor();
+        ctx.CommProbe.ExpectMsg<SiteEnvelope>();
+        AwaitCondition(() => ctx.MockGrpcClient.SubscribeCalls.Count == 1, TimeSpan.FromSeconds(3));
+
+        for (var i = 1; i <= 5; i++)
+        {
+            ctx.MockGrpcClient.SubscribeCalls[i - 1].OnCompleted();
+            var expected = i + 1;
+            AwaitCondition(() => ctx.MockGrpcClient.SubscribeCalls.Count == expected,
+                TimeSpan.FromSeconds(5));
+        }
+
+        Assert.False(ctx.TerminatedFlag[0]);
+    }
+
+    [Fact]
+    public void LateCompletionFromAPreviousStreamGeneration_IsIgnored()
+    {
+        var (_, factory) = CreateBridgeWithTrackingFactory();
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).SubscribeCalls.Count == 1,
+            TimeSpan.FromSeconds(3));
+
+        var firstSub = factory.ClientFor(GrpcNodeA).SubscribeCalls[0];
+        firstSub.OnError(new Exception("NodeA down"));   // gen 1 dies → gen 2 opens on NodeB
+        AwaitCondition(() => factory.ClientFor(GrpcNodeB).SubscribeCalls.Count == 1,
+            TimeSpan.FromSeconds(5));
+
+        // A completion racing out of the dead gen-1 stream must not tear down the live one.
+        firstSub.OnCompleted();
+        Thread.Sleep(300);
+        Assert.DoesNotContain("corr-1", factory.ClientFor(GrpcNodeB).UnsubscribedCorrelationIds);
+        Assert.Single(factory.ClientFor(GrpcNodeB).SubscribeCalls);
+    }
+
    // ── Task 6 (arch review 02, High): teardown/failover unsubscribe is endpoint-safe ──
    // Both paths use TryGet, never GetOrCreate, so cleanup can never open a fresh
    // channel or (with (site,endpoint) keying) touch another session's channel.
@@ -894,9 +955,11 @@ internal class MockSiteStreamGrpcClient : SiteStreamGrpcClient
        string instanceUniqueName,
        Action<object> onEvent,
        Action<Exception> onError,
+        Action onCompleted,
        CancellationToken ct)
    {
-        var subscription = new MockSubscription(correlationId, instanceUniqueName, onEvent, onError, ct);
+        var subscription = new MockSubscription(
+            correlationId, instanceUniqueName, onEvent, onError, onCompleted, ct);
        lock (_lock) { _subscribeCalls.Add(subscription); }

        // Return a task that completes when cancelled (simulates long-running stream)
@@ -916,6 +979,7 @@ internal record MockSubscription(
    string InstanceUniqueName,
    Action<object> OnEvent,
    Action<Exception> OnError,
+    Action OnCompleted,
    CancellationToken CancellationToken);

 /// <summary>
@@ -93,7 +93,8 @@ public class SiteAlarmAggregatorActorTests : TestKit
    }

    private sealed record SiteSub(
-        string CorrelationId, Action<AlarmStateChanged> OnAlarm, Action<Exception> OnError, CancellationToken Ct);
+        string CorrelationId, Action<AlarmStateChanged> OnAlarm, Action<Exception> OnError,
+        Action OnCompleted, CancellationToken Ct);

    private sealed class MockSiteAlarmStreamClient : SiteStreamGrpcClient
    {
@@ -107,9 +108,10 @@ public class SiteAlarmAggregatorActorTests : TestKit
        public MockSiteAlarmStreamClient() : base() { }

        public override Task SubscribeSiteAsync(
-            string correlationId, Action<AlarmStateChanged> onAlarmEvent, Action<Exception> onError, CancellationToken ct)
+            string correlationId, Action<AlarmStateChanged> onAlarmEvent, Action<Exception> onError,
+            Action onCompleted, CancellationToken ct)
        {
-            lock (_lock) { _subs.Add(new SiteSub(correlationId, onAlarmEvent, onError, ct)); }
+            lock (_lock) { _subs.Add(new SiteSub(correlationId, onAlarmEvent, onError, onCompleted, ct)); }
            var tcs = new TaskCompletionSource();
            ct.Register(() => tcs.TrySetResult());
            return tcs.Task; // never completes until cancelled (simulates a live stream)
@@ -445,6 +447,82 @@ public class SiteAlarmAggregatorActorTests : TestKit
        Assert.Equal(2, TotalSubs());
    }

+    // ── WP1.1: graceful (status OK) stream completion is a reconnect trigger ──
+
+    [Fact]
+    public void GracefulStreamCompletion_ReopensOnReconcileTick_OnTheSameNode()
+    {
+        // The site ends every stream with OK at its 4h max lifetime. Pre-fix the client's
+        // read loop just finished, nothing was told to the actor, and the site's alarm feed
+        // stayed silently dead until the central node restarted.
+        var (_, seed, _, factory) = CreateActor(reconcileInterval: TimeSpan.FromMilliseconds(300));
+        AwaitCondition(() => seed.CallCount == 1, TimeSpan.FromSeconds(3));
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).Subs.Count == 1, TimeSpan.FromSeconds(3));
+        seed.CompleteNext();
+
+        factory.ClientFor(GrpcNodeA).Subs[0].OnCompleted();
+
+        // Reopened by the reconcile tick, on the SAME node — a clean close is not a fault,
+        // so there is nothing to fail over from.
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).Subs.Count == 2, TimeSpan.FromSeconds(3));
+        Assert.Empty(factory.ClientFor(GrpcNodeB).Subs);
+        // The finished stream was released, so the site keeps no zombie relay actor.
+        Assert.Contains("corr-1", factory.ClientFor(GrpcNodeA).Unsubscribed);
+    }
+
+    [Fact]
+    public void GracefulStreamCompletion_NeitherSpendsNorRefunds_TheErrorRetryBudget()
+    {
+        // Reconcile is far away; reopens are driven explicitly so the budget can be observed.
+        var (actor, seed, _, factory) = CreateActor(reconcileInterval: TimeSpan.FromMinutes(10));
+        AwaitCondition(() => seed.CallCount == 1, TimeSpan.FromSeconds(3));
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).Subs.Count == 1, TimeSpan.FromSeconds(3));
+        seed.CompleteNext();
+
+        // Spend the whole error budget (MaxRetries = 3), each error flipping the node.
+        factory.ClientFor(GrpcNodeA).Subs[0].OnError(new Exception("1"));
+        AwaitCondition(() => factory.ClientFor(GrpcNodeB).Subs.Count == 1, TimeSpan.FromSeconds(5));
+        factory.ClientFor(GrpcNodeB).Subs[0].OnError(new Exception("2"));
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).Subs.Count == 2, TimeSpan.FromSeconds(5));
+        factory.ClientFor(GrpcNodeA).Subs[1].OnError(new Exception("3"));
+        AwaitCondition(() => factory.ClientFor(GrpcNodeB).Subs.Count == 2, TimeSpan.FromSeconds(5));
+
+        // A graceful completion reopens without flipping the node (budget not spent) …
+        factory.ClientFor(GrpcNodeB).Subs[1].OnCompleted();
+        actor.Tell(new RunReconcile());
+        AwaitCondition(() => factory.ClientFor(GrpcNodeB).Subs.Count == 3, TimeSpan.FromSeconds(5));
+        Assert.Equal(2, factory.ClientFor(GrpcNodeA).Subs.Count);
+
+        int TotalSubs() => factory.ClientFor(GrpcNodeA).Subs.Count + factory.ClientFor(GrpcNodeB).Subs.Count;
+        var before = TotalSubs();
+
+        // … and without refunding it either: the next error is the 4th, so it exceeds
+        // MaxRetries and the stream is given up rather than reconnected.
+        factory.ClientFor(GrpcNodeB).Subs[2].OnError(new Exception("4"));
+        Thread.Sleep(400);
+        Assert.Equal(before, TotalSubs());
+    }
+
+    [Fact]
+    public void LateCompletionFromAPreviousStreamGeneration_IsIgnored()
+    {
+        var (_, seed, _, factory) = CreateActor(reconcileInterval: TimeSpan.FromMinutes(10));
+        AwaitCondition(() => seed.CallCount == 1, TimeSpan.FromSeconds(3));
+        AwaitCondition(() => factory.ClientFor(GrpcNodeA).Subs.Count == 1, TimeSpan.FromSeconds(3));
+        seed.CompleteNext();
+
+        var firstSub = factory.ClientFor(GrpcNodeA).Subs.Single();
+        firstSub.OnError(new Exception("real fault"));  // gen 1 dies → gen 2 opens on NodeB
+        AwaitCondition(() => factory.ClientFor(GrpcNodeB).Subs.Count == 1, TimeSpan.FromSeconds(5));
+
+        // A completion racing out of the dead gen-1 stream must not tear down the live
+        // gen-2 stream (which would go deltaless until the next reconcile tick).
+        firstSub.OnCompleted();
+        Thread.Sleep(300);
+        Assert.DoesNotContain("corr-1", factory.ClientFor(GrpcNodeB).Unsubscribed);
+        Assert.Single(factory.ClientFor(GrpcNodeB).Subs);
+    }
+
    // ── R2 T10: live-delta publish coalescing (N6) ──

    [Fact]
@@ -1,4 +1,5 @@
 using Google.Protobuf.WellKnownTypes;
+using Grpc.Core;
 using ZB.MOM.WW.ScadaBridge.Communication.Grpc;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Streaming;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
@@ -277,7 +278,120 @@ public class SiteStreamGrpcClientTests
        var client = SiteStreamGrpcClient.CreateForTesting();

        await Assert.ThrowsAsync<InvalidOperationException>(() =>
-            client.SubscribeSiteAsync("corr", _ => { }, _ => { }, CancellationToken.None));
+            client.SubscribeSiteAsync("corr", _ => { }, _ => { }, () => { }, CancellationToken.None));
+    }
+
+    // --- WP1.1: graceful (status OK) stream completion is reported, not swallowed ---
+
+    [Fact]
+    public async Task ConsumeStream_ServerEndsStreamWithOk_InvokesOnCompleted_NotOnError()
+    {
+        // The site server caps every stream at GrpcMaxStreamLifetime (4h) and then ends the
+        // RPC with OK. That surfaces here as a read loop that simply runs out of events.
+        var client = SiteStreamGrpcClient.CreateForTesting();
+        var cts = new CancellationTokenSource();
+        var events = new List<SiteStreamEvent>();
+        Exception? error = null;
+        var completed = 0;
+
+        await client.ConsumeStreamAsync(
+            "corr-ok",
+            cts,
+            () => FakeCall(new StubStreamReader(new SiteStreamEvent { CorrelationId = "corr-ok" })),
+            events.Add,
+            ex => error = ex,
+            () => completed++);
+
+        Assert.Single(events);
+        Assert.Null(error);
+        Assert.Equal(1, completed);
+    }
+
+    [Fact]
+    public async Task ConsumeStream_StreamFaults_InvokesOnError_NotOnCompleted()
+    {
+        var client = SiteStreamGrpcClient.CreateForTesting();
+        var cts = new CancellationTokenSource();
+        Exception? error = null;
+        var completed = 0;
+
+        await client.ConsumeStreamAsync(
+            "corr-fault",
+            cts,
+            () => FakeCall(new StubStreamReader(
+                new RpcException(new Status(StatusCode.Unavailable, "site gone")))),
+            _ => { },
+            ex => error = ex,
+            () => completed++);
+
+        Assert.IsType<RpcException>(error);
+        Assert.Equal(0, completed);
+    }
+
+    [Fact]
+    public async Task ConsumeStream_OwnCancellation_InvokesNeitherCallback()
+    {
+        // Our own Unsubscribe/reconnect is a teardown, not a fault and not a graceful end:
+        // the caller has already moved on to a newer stream.
+        var client = SiteStreamGrpcClient.CreateForTesting();
+        var cts = new CancellationTokenSource();
+        await cts.CancelAsync();
+        Exception? error = null;
+        var completed = 0;
+
+        await client.ConsumeStreamAsync(
+            "corr-cancel",
+            cts,
+            () => FakeCall(new StubStreamReader()),
+            _ => { },
+            ex => error = ex,
+            () => completed++);
+
+        Assert.Null(error);
+        Assert.Equal(0, completed);
+    }
+
+    private static AsyncServerStreamingCall<SiteStreamEvent> FakeCall(StubStreamReader reader) =>
+        new(reader,
+            Task.FromResult(new Metadata()),
+            () => Status.DefaultSuccess,
+            () => new Metadata(),
+            () => { });
+
+    /// <summary>
+    /// Server stream stand-in: yields the queued events, then either ends the stream (the
+    /// status-OK completion the site's max-lifetime cap produces) or throws.
+    /// </summary>
+    private sealed class StubStreamReader : IAsyncStreamReader<SiteStreamEvent>
+    {
+        private readonly Queue<SiteStreamEvent> _events;
+        private readonly Exception? _fault;
+
+        public StubStreamReader(params SiteStreamEvent[] events)
+        {
+            _events = new Queue<SiteStreamEvent>(events);
+        }
+
+        public StubStreamReader(Exception fault)
+            : this()
+        {
+            _fault = fault;
+        }
+
+        public SiteStreamEvent Current { get; private set; } = null!;
+
+        public Task<bool> MoveNext(CancellationToken cancellationToken)
+        {
+            cancellationToken.ThrowIfCancellationRequested();
+
+            if (_events.Count > 0)
+            {
+                Current = _events.Dequeue();
+                return Task.FromResult(true);
+            }
+
+            return _fault is null ? Task.FromResult(false) : Task.FromException<bool>(_fault);
+        }
    }

    // --- Communication-003 regression tests ---
@@ -29,7 +29,7 @@ public class SiteAlarmLiveCacheServiceTests : TestKit
        public HangingClient() : base() { }
        public override Task SubscribeSiteAsync(
            string correlationId, Action<Commons.Messages.Streaming.AlarmStateChanged> onAlarmEvent,
-            Action<Exception> onError, CancellationToken ct)
+            Action<Exception> onError, Action onCompleted, CancellationToken ct)
        {
            var tcs = new TaskCompletionSource();
            ct.Register(() => tcs.TrySetResult());