fix(communication): P3-4 open-attempt deadline + P3-5 duplicate-subscribe cap order
P3-4 — SiteAlarmAggregatorActor pinned _openInFlight forever on a stream open that produced none of its three peer-driven outcomes. SiteStreamGrpcClient gives up on response headers after ConnectedHeaderTimeout (10s) and then treats the first received EVENT as its fallback connected signal, so an alarm-only site that stays quiet supplies neither — leaving _streamDown=true AND _openInFlight= true, which makes OnReconcileTick's reopen guard (_streamDown && !_openInFlight) skip every tick until the site's 4h max stream lifetime ends the call. Fix: bound one open attempt with a generation-fenced single-shot deadline (GrpcAlarmStreamOpenTimedOut, ctor-injected, production default 30s — above the 10s header timeout, below the 60s reconcile interval). Firing it releases the flag and nothing else: no retry-budget spend, no node flip, no liveness republish, and the abandoned subscription is left running so a merely-slow connect still comes up normally. Self-healing rather than reachability analysis. The deadline is cancelled on connect / error / graceful completion. P3-5 — SiteStreamGrpcServer cancelled the incumbent stream on a duplicate correlation id and only THEN checked the concurrency cap. Checking Count >= max after the removal is right at exactly the cap but wrong above it, and above it is reachable because the cap check is documented check-then-act: two subscribes that both clear it before either registers overshoot by one. In that window a routine reconnect killed a healthy stream AND was refused with ResourceExhausted, leaving the correlation with ZERO streams until its next retry. Fix: claim the removal, consult the cap only for a genuinely NEW correlation id, then cancel. A replacement is net-neutral on the count so it never consults the cap; a refused new subscriber cancelled nothing (TryRemove found nothing), so the incumbent survives. Newest-wins is unchanged — a fresh authenticated subscribe still displaces a dead/wedged incumbent. Tests (both shown failing against the unfixed code): - OpenAttemptThatNeverResolves_ReleasesTheInFlightGuard_SoTheTickCanReopen — fails (reopen never happens) with the deadline arming disabled. - OpenDeadline_DoesNotFire_OnAStreamThatConnects_NorFlipNodeOrBurnBudget — inertness lock on every healthy path. - AtCapOvershoot_DuplicateReconnect_IsNotRefused_AndLeavesExactlyOneLiveStream — fails with ResourceExhausted against the old ordering; the overshoot is driven deterministically by parking both openers on the ServerCallContext token read, which sits between the cap check and the _activeStreams insert. - AtCap_AGenuinelyNewSubscriber_IsStillRefused_AndTheIncumbentSurvives. The P3-4 test's doc comment carries the honesty note: the aggregator-side starvation reproduces exactly, but how often the >10s header delay that gets you there occurs in production is NOT demonstrated — the site flushes headers explicitly as soon as its relay is attached, so the budget has to be consumed by connect + handshake + server scheduling instead.
This commit is contained in:
Joseph Doherty
@@ -8,6 +8,7 @@ using Microsoft.Extensions.Logging.Abstractions;
|
||||
using NSubstitute;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Observability;
|
||||
using ZB.MOM.WW.ScadaBridge.Communication.Grpc;
|
||||
using GrpcStatus = Grpc.Core.Status;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Communication.Tests.Grpc;
|
||||
|
||||
@@ -794,6 +795,155 @@ public class SiteStreamGrpcServerTests : TestKit
|
||||
}
|
||||
}
|
||||
|
||||
// ── P3-5: the concurrency cap must never refuse a REPLACEMENT ───────────────
|
||||
|
||||
/// <summary>
|
||||
/// A <see cref="ServerCallContext"/> whose <see cref="ServerCallContext.CancellationToken"/>
|
||||
/// read runs a caller-supplied hook. That read is the first statement after the concurrency
|
||||
/// cap check and before the <c>_activeStreams</c> insert in
|
||||
/// <c>SiteStreamGrpcServer.RunSubscriptionStreamAsync</c>, so parking there is how a test
|
||||
/// deterministically drives two subscribes into the cap's documented check-then-act window.
|
||||
/// </summary>
|
||||
private sealed class GatedServerCallContext : ServerCallContext
|
||||
{
|
||||
private readonly CancellationToken _token;
|
||||
private readonly Action _onCancellationTokenRead;
|
||||
private readonly Metadata _trailers = new();
|
||||
private readonly Dictionary<object, object> _userState = new();
|
||||
|
||||
public GatedServerCallContext(CancellationToken token, Action onCancellationTokenRead)
|
||||
{
|
||||
_token = token;
|
||||
_onCancellationTokenRead = onCancellationTokenRead;
|
||||
}
|
||||
|
||||
protected override string MethodCore => "/sitestream.SiteStreamService/SubscribeInstance";
|
||||
protected override string HostCore => "localhost";
|
||||
protected override string PeerCore => "ipv4:127.0.0.1:0";
|
||||
protected override DateTime DeadlineCore => DateTime.MaxValue;
|
||||
protected override Metadata RequestHeadersCore => Metadata.Empty;
|
||||
protected override Metadata ResponseTrailersCore => _trailers;
|
||||
protected override GrpcStatus StatusCore { get; set; }
|
||||
protected override WriteOptions? WriteOptionsCore { get; set; }
|
||||
protected override AuthContext AuthContextCore =>
|
||||
new(null, new Dictionary<string, List<AuthProperty>>());
|
||||
protected override IDictionary<object, object> UserStateCore => _userState;
|
||||
|
||||
protected override CancellationToken CancellationTokenCore
|
||||
{
|
||||
get
|
||||
{
|
||||
_onCancellationTokenRead();
|
||||
return _token;
|
||||
}
|
||||
}
|
||||
|
||||
protected override ContextPropagationToken CreatePropagationTokenCore(
|
||||
ContextPropagationOptions? options) => throw new NotSupportedException();
|
||||
|
||||
protected override Task WriteResponseHeadersAsyncCore(Metadata responseHeaders) =>
|
||||
Task.CompletedTask;
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AtCapOvershoot_DuplicateReconnect_IsNotRefused_AndLeavesExactlyOneLiveStream()
|
||||
{
|
||||
// P3-5. The duplicate-replacement path cancels the incumbent; the cap check then had
|
||||
// to decide whether the REPLACEMENT may take its place. Checking `Count >= max` after
|
||||
// the removal is right at exactly the cap but wrong above it — and above it is
|
||||
// reachable, because the cap check is documented check-then-act: two subscribes that
|
||||
// both clear it before either registers overshoot by one. In that state a routine
|
||||
// reconnect killed a healthy stream AND was refused with ResourceExhausted, leaving
|
||||
// the correlation with ZERO streams until its next retry.
|
||||
//
|
||||
// The overshoot is produced DETERMINISTICALLY here (no sleeps, no racing threads left
|
||||
// to luck): both openers park on the CancellationToken read, which sits between the
|
||||
// cap check and the _activeStreams insert, so both are past the cap with nothing
|
||||
// registered. Releasing the gate then lands two entries under a cap of one.
|
||||
var server = CreateServer(maxStreams: 1);
|
||||
server.SetReady(Sys);
|
||||
|
||||
using var gate = new ManualResetEventSlim(false);
|
||||
using var parked = new CountdownEvent(2);
|
||||
void Park()
|
||||
{
|
||||
parked.Signal();
|
||||
Assert.True(gate.Wait(TimeSpan.FromSeconds(15)), "gate was never released");
|
||||
}
|
||||
|
||||
using var ctsA = new CancellationTokenSource();
|
||||
using var ctsB = new CancellationTokenSource();
|
||||
var streamA = Task.Run(() => server.SubscribeInstance(
|
||||
MakeRequest("corr-a"),
|
||||
Substitute.For<IServerStreamWriter<SiteStreamEvent>>(),
|
||||
new GatedServerCallContext(ctsA.Token, Park)));
|
||||
var streamB = Task.Run(() => server.SubscribeInstance(
|
||||
MakeRequest("corr-b"),
|
||||
Substitute.For<IServerStreamWriter<SiteStreamEvent>>(),
|
||||
new GatedServerCallContext(ctsB.Token, Park)));
|
||||
|
||||
Assert.True(parked.Wait(TimeSpan.FromSeconds(15)), "both openers should reach the token read");
|
||||
gate.Set();
|
||||
|
||||
// Both registered: the cap of 1 is overshot to 2, exactly as its comment allows.
|
||||
await WaitForConditionAsync(() => server.ActiveStreamCount == 2);
|
||||
|
||||
// Now "corr-a" reconnects. This is a REPLACEMENT — net-neutral on the stream count —
|
||||
// so it must be accepted. Pre-fix it threw ResourceExhausted *after* cancelling the
|
||||
// incumbent, so "corr-a" was left with no stream at all.
|
||||
using var ctsA2 = new CancellationTokenSource();
|
||||
var streamA2 = Task.Run(() => server.SubscribeInstance(
|
||||
MakeRequest("corr-a"),
|
||||
Substitute.For<IServerStreamWriter<SiteStreamEvent>>(),
|
||||
CreateMockContext(ctsA2.Token)));
|
||||
|
||||
// The incumbent unwinds through its normal cancellation path...
|
||||
await streamA;
|
||||
Assert.False(streamA2.IsFaulted,
|
||||
"the replacement must not be refused: " + streamA2.Exception?.GetBaseException().Message);
|
||||
|
||||
// ...and exactly one stream is live for "corr-a" (plus the untouched "corr-b").
|
||||
await WaitForConditionAsync(() => server.ActiveStreamCount == 2);
|
||||
Assert.False(streamA2.IsCompleted, "the replacement stream should still be running");
|
||||
|
||||
ctsA2.Cancel();
|
||||
ctsB.Cancel();
|
||||
await streamA2;
|
||||
await streamB;
|
||||
await WaitForConditionAsync(() => server.ActiveStreamCount == 0);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AtCap_AGenuinelyNewSubscriber_IsStillRefused_AndTheIncumbentSurvives()
|
||||
{
|
||||
// The other half of P3-5: excluding replacements from the cap must NOT weaken the cap
|
||||
// for a new correlation id, and a refusal must leave the existing streams untouched
|
||||
// (TryRemove found nothing, so nothing was cancelled before the throw).
|
||||
var server = CreateServer(maxStreams: 1);
|
||||
server.SetReady(Sys);
|
||||
|
||||
using var cts1 = new CancellationTokenSource();
|
||||
var stream1 = Task.Run(() => server.SubscribeInstance(
|
||||
MakeRequest("corr-keep"),
|
||||
Substitute.For<IServerStreamWriter<SiteStreamEvent>>(),
|
||||
CreateMockContext(cts1.Token)));
|
||||
|
||||
await WaitForConditionAsync(() => server.ActiveStreamCount == 1);
|
||||
|
||||
var ex = await Assert.ThrowsAsync<RpcException>(() => server.SubscribeInstance(
|
||||
MakeRequest("corr-new"),
|
||||
Substitute.For<IServerStreamWriter<SiteStreamEvent>>(),
|
||||
CreateMockContext()));
|
||||
Assert.Equal(StatusCode.ResourceExhausted, ex.StatusCode);
|
||||
|
||||
// The incumbent is untouched by the refusal.
|
||||
Assert.Equal(1, server.ActiveStreamCount);
|
||||
Assert.False(stream1.IsCompleted, "a refused NEW subscriber must not disturb the incumbent");
|
||||
|
||||
cts1.Cancel();
|
||||
await stream1;
|
||||
}
|
||||
|
||||
/// <summary>Total events carried across all captured frames (unpacking batch frames).</summary>
|
||||
private static int CountEvents(List<SiteStreamEvent> capture)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user