fix(communication): debug-stream teardown/failover unsubscribes via TryGet, never creates or disposes shared channels

This commit is contained in:
Joseph Doherty
2026-07-08 17:43:40 -04:00
parent 80013639ef
commit e75cd6f3d8
2 changed files with 76 additions and 5 deletions
@@ -466,8 +466,11 @@ public class DebugStreamBridgeActor : ReceiveActor, IWithTimers
// stops the StreamRelayActor for this correlation ID, rather than leaving a
// zombie relay actor until TCP RST / keepalive eventually detects the loss.
var previousEndpoint = _useNodeA ? _grpcNodeAAddress : _grpcNodeBAddress;
var previousClient = _grpcFactory.GetOrCreate(_siteIdentifier, previousEndpoint);
previousClient.Unsubscribe(_correlationId);
// TryGet, not GetOrCreate: unsubscribing a failed stream must never open a
// fresh channel (and, with (site,endpoint) keying, must never touch another
// session's healthy channel). Absent client => the channel is already gone
// and the site-side relay will be reaped by keepalive/session-lifetime.
_grpcFactory.TryGet(_siteIdentifier, previousEndpoint)?.Unsubscribe(_correlationId);
// Flip to the other node
_useNodeA = !_useNodeA;
@@ -489,9 +492,10 @@ public class DebugStreamBridgeActor : ReceiveActor, IWithTimers
_grpcCts?.Dispose();
_grpcCts = null;
var client = _grpcFactory.GetOrCreate(_siteIdentifier,
_useNodeA ? _grpcNodeAAddress : _grpcNodeBAddress);
client.Unsubscribe(_correlationId);
// TryGet, not GetOrCreate: teardown must never open a fresh channel just to
// unsubscribe. Absent client => nothing to cancel.
var endpoint = _useNodeA ? _grpcNodeAAddress : _grpcNodeBAddress;
_grpcFactory.TryGet(_siteIdentifier, endpoint)?.Unsubscribe(_correlationId);
}
private void SendUnsubscribe()
@@ -400,6 +400,60 @@ public class DebugStreamBridgeActorTests : TestKit
Assert.Equal("corr-1", factory.ClientFor(GrpcNodeB).SubscribeCalls[0].CorrelationId);
}
// ── 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.
private (IActorRef Actor, EndpointTrackingGrpcClientFactory Factory) CreateBridgeWithTrackingFactory()
{
var commProbe = CreateTestProbe();
var factory = new EndpointTrackingGrpcClientFactory();
var props = Props.Create(typeof(DebugStreamBridgeActor),
SiteId, InstanceName, "corr-1", commProbe.Ref,
(Action<object>)(_ => { }), (Action)(() => { }),
factory, GrpcNodeA, GrpcNodeB);
var actor = Sys.ActorOf(props);
commProbe.ExpectMsg<SiteEnvelope>(); // PreStart subscribe handshake
return (actor, factory);
}
[Fact]
public void CleanupGrpc_UnsubscribesViaTryGet_WithoutOpeningAChannel()
{
var (actor, factory) = CreateBridgeWithTrackingFactory();
AwaitCondition(() => factory.ClientFor(GrpcNodeA).SubscribeCalls.Count == 1,
TimeSpan.FromSeconds(3));
var createdBefore = factory.CreatedCount; // node-a only
actor.Tell(new StopDebugStream());
AwaitCondition(() => factory.ClientFor(GrpcNodeA).UnsubscribedCorrelationIds.Contains("corr-1"),
TimeSpan.FromSeconds(3));
// Pre-fix CleanupGrpc called GetOrCreate → could open a fresh channel here.
Assert.Equal(createdBefore, factory.CreatedCount);
}
[Fact]
public void SessionFailover_UnsubscribesFailedEndpointViaTryGet_OpensOnlyTheReconnectChannel()
{
var (_, factory) = CreateBridgeWithTrackingFactory();
AwaitCondition(() => factory.ClientFor(GrpcNodeA).SubscribeCalls.Count == 1,
TimeSpan.FromSeconds(3));
// Fail NodeA: the bridge unsubscribes the failed NodeA stream via TryGet
// (if that path had used GetOrCreate on the post-flip cache it could have
// disposed/created the wrong client), then reconnects to NodeB.
factory.ClientFor(GrpcNodeA).SubscribeCalls[0].OnError(new Exception("NodeA down"));
AwaitCondition(() => factory.ClientFor(GrpcNodeA).UnsubscribedCorrelationIds.Contains("corr-1"),
TimeSpan.FromSeconds(5));
AwaitCondition(() => factory.ClientFor(GrpcNodeB).SubscribeCalls.Count == 1,
TimeSpan.FromSeconds(5));
// Only the two real node endpoints were ever opened — the unsubscribe
// itself created nothing.
Assert.Equal(2, factory.CreatedCount);
}
// ---------------------------------------------------------------------
// M2.18 (#26) — stream-first + replay/dedup
// ---------------------------------------------------------------------
@@ -883,6 +937,12 @@ internal class MockSiteStreamGrpcClientFactory : SiteStreamGrpcClientFactory
RequestedEndpoints.Add(grpcEndpoint);
return _mockClient;
}
// Mirrors the real factory's TryGet: returns the client only for an endpoint
// already opened via GetOrCreate, never creating one. Lets teardown/failover
// unsubscribe paths (now TryGet-based, Task 6) resolve the mock client.
public override SiteStreamGrpcClient? TryGet(string siteIdentifier, string grpcEndpoint)
=> RequestedEndpoints.Contains(grpcEndpoint) ? _mockClient : null;
}
/// <summary>
@@ -902,6 +962,13 @@ internal class EndpointTrackingGrpcClientFactory : SiteStreamGrpcClientFactory
public MockSiteStreamGrpcClient ClientFor(string endpoint) =>
_byEndpoint.GetOrAdd(endpoint, _ => new MockSiteStreamGrpcClient());
/// <summary>Number of distinct endpoint clients created so far (channels opened).</summary>
public int CreatedCount => _byEndpoint.Count;
public override SiteStreamGrpcClient GetOrCreate(string siteIdentifier, string grpcEndpoint)
=> ClientFor(grpcEndpoint);
// TryGet never creates: returns the client only for an already-opened endpoint.
public override SiteStreamGrpcClient? TryGet(string siteIdentifier, string grpcEndpoint)
=> _byEndpoint.TryGetValue(grpcEndpoint, out var client) ? client : null;
}