fix(communication): cancel-only CTS replacement, deferred channel disposal, single-endpoint debug stream (arch-review 2026-08-19 P3-1 + P3-2 + P3-3)

2026-08-19 14:38:33 -04:00
parent fd4cb30273
commit 412406e7c5
8 changed files with 566 additions and 50 deletions
@@ -164,4 +164,93 @@ public class DebugStreamServiceTests : TestKit
            DebugStreamBridgeActor.ConsumerLivenessCheckInterval = previousCheck;
        }
    }
+
+    // --- arch-review 2026-08-19 P3-3: single-endpoint sites ---
+
+    [Theory]
+    [InlineData("http://localhost:5100", null)]
+    [InlineData(null, "http://localhost:5200")]
+    public async Task StartStreamAsync_SingleEndpointSite_UsesTheSoleAddressForBothNodeSlots(
+        string? nodeA, string? nodeB)
+    {
+        // Debug View used to hard-require BOTH gRPC node addresses and threw before ever
+        // creating the bridge actor, though single-endpoint sites are supported everywhere else
+        // (SiteAlarmLiveCacheService's N9 degeneration, SitePairChannelProvider's null-tolerant
+        // pair). The sole configured address now fills both slots, so the bridge actor's
+        // NodeA↔NodeB flip degenerates to a reconnect against the same node.
+        var instance = new Instance("Site1.Pump01") { Id = 7, SiteId = 3 };
+        var site = new Site("Site One", "site-1")
+        {
+            Id = 3,
+            GrpcNodeAAddress = nodeA,
+            GrpcNodeBAddress = nodeB
+        };
+
+        var instanceRepo = Substitute.For<ITemplateEngineRepository>();
+        instanceRepo.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>()).Returns(instance);
+        var siteRepo = Substitute.For<ISiteRepository>();
+        siteRepo.GetSiteByIdAsync(3, Arg.Any<CancellationToken>()).Returns(site);
+
+        var services = new ServiceCollection();
+        services.AddScoped(_ => instanceRepo);
+        services.AddScoped(_ => siteRepo);
+        using var provider = services.BuildServiceProvider();
+
+        var commProbe = CreateTestProbe();
+        var commService = new CommunicationService(
+            Options.Create(new CommunicationOptions()),
+            NullLogger<CommunicationService>.Instance);
+        commService.SetCommunicationActor(commProbe.Ref);
+
+        using var grpcFactory = new Grpc.MockSiteStreamGrpcClientFactory(
+            new Grpc.MockSiteStreamGrpcClient());
+        using var service = new DebugStreamService(
+            commService, provider, grpcFactory, NullLogger<DebugStreamService>.Instance);
+        service.SetActorSystem(Sys);
+
+        // Act — resolution must get past the address check and start a bridge actor.
+        var startTask = service.StartStreamAsync(instanceId: 7, onEvent: _ => { }, onTerminated: () => { });
+
+        commProbe.ExpectMsg<SiteEnvelope>(TimeSpan.FromSeconds(5));
+        var bridgeActor = commProbe.LastSender;
+
+        // The stream it opened dials the ONE configured address.
+        Assert.Equal(new[] { nodeA ?? nodeB }, grpcFactory.RequestedEndpoints);
+
+        // Unwind the session.
+        bridgeActor.Tell(new DebugStreamTerminated("site-1", "corr"));
+        await Assert.ThrowsAsync<InvalidOperationException>(() => startTask);
+    }
+
+    [Fact]
+    public async Task StartStreamAsync_SiteWithNoGrpcAddresses_StillThrows()
+    {
+        // Degenerating a single endpoint must not degenerate into accepting NONE.
+        var instance = new Instance("Site1.Pump01") { Id = 7, SiteId = 3 };
+        var site = new Site("Site One", "site-1") { Id = 3 };
+
+        var instanceRepo = Substitute.For<ITemplateEngineRepository>();
+        instanceRepo.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>()).Returns(instance);
+        var siteRepo = Substitute.For<ISiteRepository>();
+        siteRepo.GetSiteByIdAsync(3, Arg.Any<CancellationToken>()).Returns(site);
+
+        var services = new ServiceCollection();
+        services.AddScoped(_ => instanceRepo);
+        services.AddScoped(_ => siteRepo);
+        using var provider = services.BuildServiceProvider();
+
+        var commService = new CommunicationService(
+            Options.Create(new CommunicationOptions()),
+            NullLogger<CommunicationService>.Instance);
+        commService.SetCommunicationActor(CreateTestProbe().Ref);
+
+        using var grpcFactory = new SiteStreamGrpcClientFactory(NullLoggerFactory.Instance);
+        using var service = new DebugStreamService(
+            commService, provider, grpcFactory, NullLogger<DebugStreamService>.Instance);
+        service.SetActorSystem(Sys);
+
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
+            service.StartStreamAsync(instanceId: 7, onEvent: _ => { }, onTerminated: () => { }));
+        Assert.Contains("no gRPC node addresses", ex.Message);
+    }
 }
@@ -0,0 +1,105 @@
+using Grpc.Net.Client;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Options;
+using ZB.MOM.WW.ScadaBridge.Communication.Grpc;
+
+namespace ZB.MOM.WW.ScadaBridge.Communication.Tests.Grpc;
+
+/// <summary>
+/// Arch-review 2026-08-19 P3-2 — a channel replaced by the 60 s DB address refresh (or an admin
+/// edit) must not be disposed out from under an in-flight <see cref="SitePairChannelProvider.ExecuteAsync{T}"/>
+/// call. <c>ExecuteAsync</c> reads the channel under the pair gate but runs the RPC outside it, so an
+/// inline dispose aborted whatever was running — a multi-minute <c>DeployArtifactsCommand</c>
+/// included — with a status the cross-node retry deliberately never engages on.
+/// </summary>
+public class SitePairChannelProviderDisposalTests
+{
+    private static SitePairChannelProvider Build() => new(
+        new NoKeyProvider(),
+        Options.Create(new CommunicationOptions()),
+        NullLogger<SitePairChannelProvider>.Instance);
+
+    [Fact]
+    public async Task UpdateSite_WhileCallInFlight_ParksTheOldChannelAndLetsTheCallComplete()
+    {
+        using var provider = Build();
+        provider.UpdateSite("site-1", "http://localhost:15101", nodeBEndpoint: null);
+
+        var entered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        GrpcChannel? inFlightChannel = null;
+
+        // A long-running command (deploy) against the site's current channel.
+        var call = provider.ExecuteAsync("site-1", async (channel, ct) =>
+        {
+            inFlightChannel = channel;
+            entered.SetResult();
+            await release.Task;
+            // A disposed GrpcChannel throws here — this is the assertion that the address
+            // refresh did not pull the channel out from under the call.
+            _ = channel.CreateCallInvoker();
+            return 42;
+        }, CancellationToken.None);
+
+        await entered.Task.WaitAsync(TimeSpan.FromSeconds(5));
+
+        // The DB refresh tick re-points the site at a new address, mid-call.
+        provider.UpdateSite("site-1", "http://localhost:15102", nodeBEndpoint: null);
+
+        // The superseded channel is PARKED, not disposed synchronously.
+        Assert.Equal(1, provider.ParkedChannelCount);
+
+        release.SetResult();
+        Assert.Equal(42, await call.WaitAsync(TimeSpan.FromSeconds(5)));
+
+        // Once the grace window elapses the park drains and the channel really is disposed —
+        // deferred disposal, not a leak.
+        provider.SweepGraveyard(force: true);
+        Assert.Equal(0, provider.ParkedChannelCount);
+        Assert.Throws<ObjectDisposedException>(() => inFlightChannel!.CreateCallInvoker());
+    }
+
+    [Fact]
+    public void RemoveSite_ParksBothChannelsRatherThanDisposingThemInline()
+    {
+        using var provider = Build();
+        provider.UpdateSite("site-1", "http://localhost:15111", "http://localhost:15112");
+
+        provider.RemoveSite("site-1");
+
+        // Same hazard as UpdateSite: an admin deleting a site while a command is in flight.
+        Assert.Equal(2, provider.ParkedChannelCount);
+
+        provider.SweepGraveyard(force: true);
+        Assert.Equal(0, provider.ParkedChannelCount);
+    }
+
+    [Fact]
+    public void UpdateSite_UnchangedEndpoints_ParksNothing()
+    {
+        using var provider = Build();
+        provider.UpdateSite("site-1", "http://localhost:15121", "http://localhost:15122");
+        provider.UpdateSite("site-1", "http://localhost:15121", "http://localhost:15122");
+
+        Assert.Equal(0, provider.ParkedChannelCount);
+    }
+
+    [Fact]
+    public void Dispose_DrainsParkedChannelsRegardlessOfRemainingGrace()
+    {
+        var provider = Build();
+        provider.UpdateSite("site-1", "http://localhost:15131", nodeBEndpoint: null);
+        provider.UpdateSite("site-1", "http://localhost:15132", nodeBEndpoint: null);
+        Assert.Equal(1, provider.ParkedChannelCount);
+
+        provider.Dispose();
+
+        Assert.Equal(0, provider.ParkedChannelCount);
+    }
+
+    private sealed class NoKeyProvider : ISitePskProvider
+    {
+        public ValueTask<string> GetAsync(string siteId, CancellationToken ct) => new("k");
+        public void Invalidate(string siteId) { }
+    }
+}
@@ -66,6 +66,74 @@ public class SiteStreamGrpcClientFactoryDisposeTests
        });
    }

+    /// <summary>
+    /// Test client whose disposal always throws — stands in for a client whose
+    /// <c>ReleaseResources</c> hit an <see cref="ObjectDisposedException"/> on a subscription
+    /// token source (arch-review 2026-08-19 P3-1).
+    /// </summary>
+    private sealed class ThrowingClient : SiteStreamGrpcClient
+    {
+        public override void Dispose() => throw new ObjectDisposedException(nameof(CancellationTokenSource));
+
+        public override ValueTask DisposeAsync() =>
+            throw new ObjectDisposedException(nameof(CancellationTokenSource));
+    }
+
+    /// <summary>Hands out one throwing client followed by tracking clients.</summary>
+    private sealed class OneThrowingClientFactory : SiteStreamGrpcClientFactory
+    {
+        private readonly ConcurrentBag<TrackingClient> _created = new();
+        private int _next;
+
+        public OneThrowingClientFactory() : base(NullLoggerFactory.Instance) { }
+
+        public IReadOnlyCollection<TrackingClient> Created => _created.ToList();
+
+        protected override SiteStreamGrpcClient CreateClient(string siteIdentifier, string grpcEndpoint)
+        {
+            if (Interlocked.Increment(ref _next) == 1)
+            {
+                return new ThrowingClient();
+            }
+
+            var client = new TrackingClient();
+            _created.Add(client);
+            return client;
+        }
+    }
+
+    [Fact]
+    public async Task DisposeAsync_OneClientThrowing_StillDisposesEveryOtherClient()
+    {
+        // The disposal loop used to abort on the first throwing client, leaving every LATER
+        // client — and its gRPC channel — undisposed for the life of the process.
+        var factory = new OneThrowingClientFactory();
+        factory.GetOrCreate("site-a", "http://localhost:5100"); // throwing
+        factory.GetOrCreate("site-b", "http://localhost:5200");
+        factory.GetOrCreate("site-c", "http://localhost:5300");
+
+        await factory.DisposeAsync();
+
+        Assert.Equal(2, factory.Created.Count);
+        Assert.All(factory.Created, c =>
+            Assert.True(c.AsyncDisposeCalled, "a throwing client must not strand its peers undisposed"));
+    }
+
+    [Fact]
+    public void Dispose_OneClientThrowing_StillDisposesEveryOtherClient()
+    {
+        var factory = new OneThrowingClientFactory();
+        factory.GetOrCreate("site-a", "http://localhost:5100"); // throwing
+        factory.GetOrCreate("site-b", "http://localhost:5200");
+        factory.GetOrCreate("site-c", "http://localhost:5300");
+
+        factory.Dispose();
+
+        Assert.Equal(2, factory.Created.Count);
+        Assert.All(factory.Created, c =>
+            Assert.True(c.SyncDisposeCalled, "a throwing client must not strand its peers undisposed"));
+    }
+
    [Fact]
    public void Dispose_DoesNotDeadlock_UnderSingleThreadedSynchronizationContext()
    {
@@ -677,26 +677,91 @@ public class SiteStreamGrpcClientTests
    // --- Communication-003 regression tests ---

    [Fact]
-    public void RegisterSubscription_ReusedCorrelationId_CancelsAndDisposesPriorCts()
+    public void RegisterSubscription_ReusedCorrelationId_CancelsPriorCts_WithoutDisposingIt()
    {
        // Two SubscribeAsync calls briefly sharing a correlation ID (reconnect race).
-        // Inserting the second must cancel + dispose the first so it does not leak.
+        // Inserting the second must CANCEL the first — and must NOT dispose it (arch-review
+        // 2026-08-19 P3-1, re-pinning the earlier cancel-AND-dispose contract): the prior
+        // handler's ConsumeStreamAsync is still running and still reads prior.Token, so
+        // disposing from the replacing path throws ObjectDisposedException inside that loop.
+        // The owning SubscribeAsync/SubscribeSiteAsync disposes it as its loop unwinds.
        var client = SiteStreamGrpcClient.CreateForTesting();

-        var first = new CancellationTokenSource();
-        var second = new CancellationTokenSource();
+        using var first = new CancellationTokenSource();
+        using var second = new CancellationTokenSource();

        client.RegisterSubscription("corr-shared", first);
        client.RegisterSubscription("corr-shared", second);

        Assert.True(first.IsCancellationRequested);
-        // Disposed CTS throws ObjectDisposedException when its token is touched.
-        Assert.Throws<ObjectDisposedException>(() => _ = first.Token);
+        // Still usable by the prior handler that owns it — no ObjectDisposedException.
+        Assert.True(first.Token.IsCancellationRequested);

        // The second (live) CTS must remain intact.
        Assert.False(second.IsCancellationRequested);
    }

+    [Fact]
+    public void RegisterSubscription_ReplacingAnAlreadyDisposedPriorCts_DoesNotThrowIntoTheNewSubscription()
+    {
+        // The prior handler's loop already unwound and disposed its own CTS while a replacement
+        // subscription is registering. The unguarded prior.Cancel() threw ObjectDisposedException
+        // straight out of the NEW SubscribeAsync, which reads it as a fault of the CURRENT
+        // generation and burns a reconnect on a phantom failure.
+        var client = SiteStreamGrpcClient.CreateForTesting();
+
+        var prior = new CancellationTokenSource();
+        client.RegisterSubscription("corr-shared", prior);
+        prior.Dispose();
+
+        using var replacement = new CancellationTokenSource();
+        client.RegisterSubscription("corr-shared", replacement);
+
+        // The replacement owns the slot and is untouched.
+        Assert.False(replacement.IsCancellationRequested);
+        client.Unsubscribe("corr-shared");
+        Assert.True(replacement.IsCancellationRequested);
+    }
+
+    [Fact]
+    public void Unsubscribe_AfterClientDisposal_IsANoOp()
+    {
+        // Disposal cancels every live subscription and clears the map; a late Unsubscribe from
+        // the consuming actor's teardown must not throw (an ObjectDisposedException here used to
+        // escape into the caller's cleanup path).
+        var client = SiteStreamGrpcClient.CreateForTesting();
+        using var cts = new CancellationTokenSource();
+        client.AddSubscriptionForTesting("corr-1", cts);
+
+        client.Dispose();
+        Assert.True(cts.IsCancellationRequested);
+
+        client.Unsubscribe("corr-1");
+        client.Unsubscribe("never-registered");
+    }
+
+    [Fact]
+    public void Dispose_WithAnAlreadyDisposedSubscriptionCts_StillCancelsTheRest()
+    {
+        // ReleaseResources iterated the live CTSes unguarded, so one already-disposed entry
+        // aborted the whole loop — leaving later subscriptions uncancelled AND (via the
+        // exception escaping into SiteStreamGrpcClientFactory's disposal loop) later clients'
+        // channels undisposed.
+        var client = SiteStreamGrpcClient.CreateForTesting();
+
+        var disposedEarly = new CancellationTokenSource();
+        using var live = new CancellationTokenSource();
+        client.AddSubscriptionForTesting("corr-disposed", disposedEarly);
+        client.AddSubscriptionForTesting("corr-live", live);
+        disposedEarly.Dispose();
+
+        // Deterministic regardless of dictionary iteration order: the disposed entry is always
+        // visited, so the unguarded loop always threw out of Dispose().
+        Assert.Null(Record.Exception(client.Dispose));
+
+        Assert.True(live.IsCancellationRequested);
+    }
+
    [Fact]
    public void RemoveSubscription_OnlyRemovesOwnCts_NotAReplacement()
    {