diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs index 49dc7be..25b2e42 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotService.cs @@ -294,6 +294,9 @@ public sealed class DashboardSnapshotService : IDashboardSnapshotService // Inside the refresh window: reuse the cached summaries rather than // 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. + // 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; } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHub.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHub.cs index 3cc8d9f..4f340be 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHub.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHub.cs @@ -39,43 +39,3 @@ public sealed class DashboardSnapshotHub( await base.OnDisconnectedAsync(exception).ConfigureAwait(false); } } - -/// -/// Process-wide count of live connections. -/// Registered as a singleton and read by -/// to idle-gate the snapshot tick: with no dashboard connected there is nothing -/// to broadcast to, so no snapshot is built. -/// -public sealed class DashboardSnapshotHubConnectionCounter -{ - private int _count; - - /// Gets the number of live snapshot hub connections. - public int Count => Volatile.Read(ref _count); - - /// Records a new snapshot hub connection. - /// The connection count after the increment. - public int Increment() - { - return Interlocked.Increment(ref _count); - } - - /// - /// Records a snapshot hub disconnection. The count is clamped at zero: SignalR - /// can invoke OnDisconnectedAsync for a connection whose - /// OnConnectedAsync faulted, and a negative count would idle-gate the - /// publisher while viewers are still attached. - /// - /// The connection count after the decrement. - public int Decrement() - { - int updated = Interlocked.Decrement(ref _count); - if (updated >= 0) - { - return updated; - } - - Interlocked.CompareExchange(ref _count, 0, updated); - return 0; - } -} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHubConnectionCounter.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHubConnectionCounter.cs new file mode 100644 index 0000000..4695565 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardSnapshotHubConnectionCounter.cs @@ -0,0 +1,52 @@ +namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; + +/// +/// Process-wide count of live connections. +/// Registered as a singleton and read by +/// to idle-gate the snapshot tick: with no dashboard connected there is nothing +/// to broadcast to, so no snapshot is built. +/// +public sealed class DashboardSnapshotHubConnectionCounter +{ + private int _count; + + /// Gets the number of live snapshot hub connections. + public int Count => Volatile.Read(ref _count); + + /// Records a new snapshot hub connection. + /// The connection count after the increment. + public int Increment() + { + return Interlocked.Increment(ref _count); + } + + /// + /// Records a snapshot hub disconnection, clamped at zero: SignalR can invoke + /// OnDisconnectedAsync for a connection whose OnConnectedAsync + /// faulted, and a negative count would idle-gate the publisher while viewers + /// are still attached. + /// + /// + /// 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. + /// + /// The connection count after the decrement. + 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; + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotHubConnectionCounterTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotHubConnectionCounterTests.cs new file mode 100644 index 0000000..6c50716 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotHubConnectionCounterTests.cs @@ -0,0 +1,154 @@ +using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; + +namespace ZB.MOM.WW.MxGateway.Tests.Gateway.Dashboard; + +/// +/// Verifies , the seam +/// 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 precedent: a SignalR +/// Hub instance needs a caller-clients and connection context fake to +/// invoke OnConnectedAsync, and the hub methods themselves are two lines +/// of delegation to this type. +/// +public sealed class DashboardSnapshotHubConnectionCounterTests +{ + /// A fresh counter reports no viewers, so the publisher starts idle. + [Fact] + public void Count_WhenNothingConnected_IsZero() + { + DashboardSnapshotHubConnectionCounter counter = new(); + + Assert.Equal(0, counter.Count); + } + + /// Connect/disconnect pairs move the count and return the post-operation value. + [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); + } + + /// + /// SignalR calls OnDisconnectedAsync for a connection whose + /// OnConnectedAsync faulted, so unmatched decrements happen. They must + /// hold the floor at zero rather than driving the count negative. + /// + [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()); + } + + /// + /// 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. + /// + [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); + } + + /// + /// 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. + /// + /// + /// 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. + /// + [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()); + } + } + + /// Matched connect/disconnect pairs under concurrency settle back at zero. + [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); + } +}