fix(dashboard): clamped CAS retry loop in snapshot hub connection counter + direct counter tests

Decrement was decrement-first with a single non-retried repair CAS. From zero,
two unmatched decrements (SignalR calls OnDisconnectedAsync for a connection
whose OnConnectedAsync faulted) capture -1 and -2; a real Increment then makes
the count -1, and the first decrementer's stale CompareExchange(0, -1) matches
and resets to zero — erasing a live connection, so the idle gate freezes an open
dashboard. The same lost race also made Decrement report 0 when it had not
written 0.

Clamping now happens inside the compare-and-swap: read, clamp, publish, retry on
loss. A lost race re-reads the fresh value instead of repairing a stale one.

The counter moves to its own file per the one-public-type-per-file convention and
gains direct tests: the zero floor under concurrent unmatched decrements, matched
pairs settling at zero, and an interleaved connect/disconnect stress round. The
stress test asserts the observable invariants only — the specific interleaving
cannot be forced through the public API (verified: the previous implementation
passes it), which its remarks now state rather than implying a reproducer. A hub
wiring test is skipped for the EventsHub reason: driving Hub.OnConnectedAsync
needs caller-clients and connection-context fakes, and the overrides are two
lines of delegation to the tested type.

Also documents that the API-key refresh's pre-gate time check races benignly.
This commit is contained in:
Joseph Doherty
2026-08-15 12:32:58 -04:00
parent 44ca7c8623
commit f920b4cbf5
4 changed files with 209 additions and 40 deletions
@@ -294,6 +294,9 @@ public sealed class DashboardSnapshotService : IDashboardSnapshotService
// Inside the refresh window: reuse the cached summaries rather than // Inside the refresh window: reuse the cached summaries rather than
// re-reading the API key table on this tick. Only a *successful* refresh // re-reading the API key table on this tick. Only a *successful* refresh
// moves the timestamp, so a failed read is retried on the next tick. // moves the timestamp, so a failed read is retried on the next tick.
// This check is deliberately outside the refresh gate, so it races
// benignly: if two callers both read a stale timestamp, the zero-timeout
// gate below admits one and the other returns without touching the store.
return; return;
} }
@@ -39,43 +39,3 @@ public sealed class DashboardSnapshotHub(
await base.OnDisconnectedAsync(exception).ConfigureAwait(false); await base.OnDisconnectedAsync(exception).ConfigureAwait(false);
} }
} }
/// <summary>
/// Process-wide count of live <see cref="DashboardSnapshotHub"/> connections.
/// Registered as a singleton and read by <see cref="DashboardSnapshotPublisher"/>
/// to idle-gate the snapshot tick: with no dashboard connected there is nothing
/// to broadcast to, so no snapshot is built.
/// </summary>
public sealed class DashboardSnapshotHubConnectionCounter
{
private int _count;
/// <summary>Gets the number of live snapshot hub connections.</summary>
public int Count => Volatile.Read(ref _count);
/// <summary>Records a new snapshot hub connection.</summary>
/// <returns>The connection count after the increment.</returns>
public int Increment()
{
return Interlocked.Increment(ref _count);
}
/// <summary>
/// Records a snapshot hub disconnection. The count is clamped at zero: SignalR
/// can invoke <c>OnDisconnectedAsync</c> for a connection whose
/// <c>OnConnectedAsync</c> faulted, and a negative count would idle-gate the
/// publisher while viewers are still attached.
/// </summary>
/// <returns>The connection count after the decrement.</returns>
public int Decrement()
{
int updated = Interlocked.Decrement(ref _count);
if (updated >= 0)
{
return updated;
}
Interlocked.CompareExchange(ref _count, 0, updated);
return 0;
}
}
@@ -0,0 +1,52 @@
namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs;
/// <summary>
/// Process-wide count of live <see cref="DashboardSnapshotHub"/> connections.
/// Registered as a singleton and read by <see cref="DashboardSnapshotPublisher"/>
/// to idle-gate the snapshot tick: with no dashboard connected there is nothing
/// to broadcast to, so no snapshot is built.
/// </summary>
public sealed class DashboardSnapshotHubConnectionCounter
{
private int _count;
/// <summary>Gets the number of live snapshot hub connections.</summary>
public int Count => Volatile.Read(ref _count);
/// <summary>Records a new snapshot hub connection.</summary>
/// <returns>The connection count after the increment.</returns>
public int Increment()
{
return Interlocked.Increment(ref _count);
}
/// <summary>
/// Records a snapshot hub disconnection, clamped at zero: SignalR can invoke
/// <c>OnDisconnectedAsync</c> for a connection whose <c>OnConnectedAsync</c>
/// faulted, and a negative count would idle-gate the publisher while viewers
/// are still attached.
/// </summary>
/// <remarks>
/// The clamp is applied inside the compare-and-swap rather than as a repair
/// afterwards. Decrementing first and then correcting a negative result races:
/// two unmatched decrements from zero would both plan a repair, a real
/// connection could increment in between, and the stale repair would then
/// overwrite that live connection's increment — freezing a real viewer's
/// dashboard behind the idle gate. Reading, clamping, and publishing as one
/// atomic step means a lost race simply retries against the fresh value.
/// </remarks>
/// <returns>The connection count after the decrement.</returns>
public int Decrement()
{
int current;
int next;
do
{
current = Volatile.Read(ref _count);
next = current > 0 ? current - 1 : 0;
}
while (Interlocked.CompareExchange(ref _count, next, current) != current);
return next;
}
}
@@ -0,0 +1,154 @@
using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs;
namespace ZB.MOM.WW.MxGateway.Tests.Gateway.Dashboard;
/// <summary>
/// Verifies <see cref="DashboardSnapshotHubConnectionCounter"/>, the seam
/// <see cref="DashboardSnapshotPublisher"/> reads before building a snapshot.
/// An over-count leaves the publisher ticking for nobody; an under-count is
/// worse — it idle-gates a dashboard that is actually open, so the page silently
/// stops updating. The counter is exercised directly rather than through the hub,
/// mirroring the <see cref="EventsHubViewerRegistry"/> precedent: a SignalR
/// <c>Hub</c> instance needs a caller-clients and connection context fake to
/// invoke <c>OnConnectedAsync</c>, and the hub methods themselves are two lines
/// of delegation to this type.
/// </summary>
public sealed class DashboardSnapshotHubConnectionCounterTests
{
/// <summary>A fresh counter reports no viewers, so the publisher starts idle.</summary>
[Fact]
public void Count_WhenNothingConnected_IsZero()
{
DashboardSnapshotHubConnectionCounter counter = new();
Assert.Equal(0, counter.Count);
}
/// <summary>Connect/disconnect pairs move the count and return the post-operation value.</summary>
[Fact]
public void IncrementThenDecrement_TracksLiveConnections()
{
DashboardSnapshotHubConnectionCounter counter = new();
Assert.Equal(1, counter.Increment());
Assert.Equal(2, counter.Increment());
Assert.Equal(2, counter.Count);
Assert.Equal(1, counter.Decrement());
Assert.Equal(0, counter.Decrement());
Assert.Equal(0, counter.Count);
}
/// <summary>
/// SignalR calls <c>OnDisconnectedAsync</c> for a connection whose
/// <c>OnConnectedAsync</c> faulted, so unmatched decrements happen. They must
/// hold the floor at zero rather than driving the count negative.
/// </summary>
[Fact]
public void Decrement_WithoutMatchingIncrement_HoldsAtZero()
{
DashboardSnapshotHubConnectionCounter counter = new();
Assert.Equal(0, counter.Decrement());
Assert.Equal(0, counter.Decrement());
Assert.Equal(0, counter.Count);
// A genuine connection after unmatched disconnects still registers as one.
Assert.Equal(1, counter.Increment());
}
/// <summary>
/// Many concurrent unmatched decrements must not leave the count below zero:
/// a negative floor would swallow the next real connection's increment and
/// keep the publisher idle-gated while a viewer waits. The clamp lives inside
/// the compare-and-swap, so the floor holds however the calls interleave.
/// </summary>
[Fact]
public void Decrement_UnderConcurrencyFromZero_NeverGoesNegative()
{
DashboardSnapshotHubConnectionCounter counter = new();
Parallel.For(0, 256, _ => counter.Decrement());
Assert.Equal(0, counter.Count);
Assert.Equal(1, counter.Increment());
Assert.Equal(1, counter.Count);
}
/// <summary>
/// Stress check on the invariant the idle gate depends on: real connects
/// interleaved with unmatched disconnects leave the count in [0, connects], and a
/// connect after the storm is always visible to the publisher. The failure this
/// guards is the decrement-then-repair race the CAS retry loop replaced — an early
/// decrementer's stale repair either erases a live connection's increment or leaves
/// a negative value behind, and either way an open dashboard freezes behind the
/// gate.
/// </summary>
/// <remarks>
/// This does not deterministically reproduce that race, and it is not claimed to:
/// the bad interleaving needs a specific few-instruction overlap that cannot be
/// forced through the public API, and a merely low count is a legitimate outcome
/// here (a decrement that runs while the count is positive consumes a real
/// connection). Verified by experiment: the previous implementation passes this
/// test. What is asserted are the observable consequences — never negative, the
/// floor holds, a later connect still registers — with the correctness argument
/// resting on the clamped CAS retry loop itself.
/// </remarks>
[Fact]
public void IncrementAndDecrement_InterleavedUnderConcurrency_StayWithinTheRealConnectionCount()
{
const int LiveConnections = 8;
const int UnmatchedDisconnects = 128;
for (int round = 0; round < 50; round++)
{
DashboardSnapshotHubConnectionCounter counter = new();
// A few workers are real connects that must survive; the rest are
// unmatched disconnects hammering the zero floor around them.
Parallel.For(0, LiveConnections + UnmatchedDisconnects, index =>
{
if (index % 16 == 0 && index / 16 < LiveConnections)
{
counter.Increment();
return;
}
counter.Decrement();
});
Assert.InRange(counter.Count, 0, LiveConnections);
// Every unmatched decrement has completed, so the surviving connections
// disconnect cleanly and the counter must land exactly on zero — never
// below it, and a subsequent connect must be visible to the publisher.
int remaining = counter.Count;
for (int i = 0; i < remaining; i++)
{
counter.Decrement();
}
Assert.Equal(0, counter.Count);
Assert.Equal(1, counter.Increment());
}
}
/// <summary>Matched connect/disconnect pairs under concurrency settle back at zero.</summary>
[Fact]
public void IncrementAndDecrement_MatchedPairsUnderConcurrency_SettleAtZero()
{
DashboardSnapshotHubConnectionCounter counter = new();
Parallel.For(0, 64, _ =>
{
for (int pass = 0; pass < 50; pass++)
{
counter.Increment();
counter.Decrement();
}
});
Assert.Equal(0, counter.Count);
}
}