refactor(sessions): derive subscriber mode from session config; close Task 8 review nits

Remove the per-call allowMultipleSubscribers param from AttachEventSubscriber and
derive the mode internally from _eventStreaming.AllowMultipleEventSubscribers — the
same source SessionEventDistributor uses for singleSubscriberMode — so the two can
never structurally diverge. The maxSubscribers cap param is kept because
MaxEventSubscribersPerSession lives in SessionOptions, which the session does not hold
directly (only EventOptions flows through SessionEventStreaming).

Other nits:
- SubscriberCount XML doc clarifies it includes internal subscribers and differs from
  GatewaySession.ActiveEventSubscriberCount (external/gRPC only).
- SingleSubscriberMode_LoneExternalOverflow test: add Assert.Equal(1, observedSet) guard
  before the value assertion so the test cannot pass vacuously if the handler never fired.
- GatewayOptionsValidator.ValidateSessions: add explanatory code comment documenting why
  !AllowMultipleEventSubscribers && MaxEventSubscribersPerSession > 1 is NOT rejected as
  a hard error (the default config ships with this combination; the cap is simply unused
  in single-subscriber mode, not a behavior bug).
- GatewaySession.DetachEventSubscriber: add Debug.Assert before the clamp so a genuine
  double-decrement surfaces in debug builds.
This commit is contained in:
Joseph Doherty
2026-06-15 15:53:27 -04:00
parent ac42783e36
commit 281e00b300
8 changed files with 62 additions and 43 deletions
@@ -181,6 +181,15 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase<GatewayOption
options.MaxEventSubscribersPerSession, options.MaxEventSubscribersPerSession,
"MxGateway:Sessions:MaxEventSubscribersPerSession must be greater than zero.", "MxGateway:Sessions:MaxEventSubscribersPerSession must be greater than zero.",
builder); builder);
// NOTE: We intentionally do NOT reject !AllowMultipleEventSubscribers &&
// MaxEventSubscribersPerSession > 1 as a hard validation error here. The default
// SessionOptions ships with AllowMultipleEventSubscribers=false and
// MaxEventSubscribersPerSession=8; making those defaults a validation failure would
// break every deployment that has not explicitly set the cap. The cap is simply
// ignored in single-subscriber mode (AttachEventSubscriber derives effectiveCap=1),
// so the only practical consequence of the apparent inconsistency is a dead config
// knob, not incorrect behavior.
} }
private static void ValidateEvents(EventOptions options, ValidationBuilder builder) private static void ValidateEvents(EventOptions options, ValidationBuilder builder)
@@ -68,8 +68,10 @@ public sealed class EventStreamService(
// No `using` here — subscriber.Dispose() is called exactly once in the finally // No `using` here — subscriber.Dispose() is called exactly once in the finally
// block below, which also disposes the reader. A `using` declaration would add a // block below, which also disposes the reader. A `using` declaration would add a
// second Dispose on the same path and double-decrement the session subscriber count. // second Dispose on the same path and double-decrement the session subscriber count.
// The subscriber mode (single vs. multi) is derived inside AttachEventSubscriber from
// the session's own SessionEventStreaming.AllowMultipleEventSubscribers field — the
// same source the distributor uses — so the two cannot diverge.
IEventSubscriberLease subscriber = session.AttachEventSubscriber( IEventSubscriberLease subscriber = session.AttachEventSubscriber(
options.Value.Sessions.AllowMultipleEventSubscribers,
options.Value.Sessions.MaxEventSubscribersPerSession); options.Value.Sessions.MaxEventSubscribersPerSession);
int streamQueueDepth = 0; int streamQueueDepth = 0;
@@ -1,3 +1,4 @@
using System.Diagnostics;
using System.Runtime.CompilerServices; using System.Runtime.CompilerServices;
using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging;
using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.MxGateway.Contracts.Proto;
@@ -684,31 +685,31 @@ public sealed class GatewaySession
/// <see cref="MxEvent"/>s for this subscriber. The returned lease, when disposed, /// <see cref="MxEvent"/>s for this subscriber. The returned lease, when disposed,
/// unregisters the distributor subscriber AND decrements the active-subscriber count. /// unregisters the distributor subscriber AND decrements the active-subscriber count.
/// </summary> /// </summary>
/// <param name="allowMultipleSubscribers">
/// When <see langword="false"/>, single-subscriber mode: a second concurrent EXTERNAL
/// subscriber is rejected with <see cref="SessionManagerErrorCode.EventSubscriberAlreadyActive"/>.
/// When <see langword="true"/>, multi-subscriber mode: up to
/// <paramref name="maxSubscribers"/> concurrent EXTERNAL subscribers are allowed; the
/// next attach is rejected with
/// <see cref="SessionManagerErrorCode.EventSubscriberLimitReached"/>.
/// </param>
/// <param name="maxSubscribers"> /// <param name="maxSubscribers">
/// Maximum concurrent external subscribers in multi-subscriber mode /// Maximum concurrent external subscribers in multi-subscriber mode
/// (<c>MxGateway:Sessions:MaxEventSubscribersPerSession</c>). Ignored when /// (<c>MxGateway:Sessions:MaxEventSubscribersPerSession</c>). Ignored when the
/// <paramref name="allowMultipleSubscribers"/> is <see langword="false"/> (the effective /// session is in single-subscriber mode (<c>AllowMultipleEventSubscribers == false</c>);
/// cap is then 1). The gateway-owned internal dashboard subscriber is registered /// the effective cap is then 1. The gateway-owned internal dashboard subscriber is
/// directly on the distributor and is NOT counted here, so it never consumes cap budget. /// registered directly on the distributor and is NOT counted here, so it never
/// consumes cap budget.
/// </param> /// </param>
/// <remarks> /// <remarks>
/// The count-check-and-increment runs atomically under <c>_syncRoot</c>, so two /// The subscriber mode is derived internally from
/// concurrent attaches racing toward the cap can never both succeed past it. On /// <see cref="SessionEventStreaming.AllowMultipleEventSubscribers"/> — the same source
/// distributor-register failure the count is rolled back (see the catch below). /// the <see cref="SessionEventDistributor"/> uses to gate its FailFast decision — so
/// the cap-enforcement mode and the distributor's <c>singleSubscriberMode</c> field
/// cannot diverge. The count-check-and-increment runs atomically under
/// <c>_syncRoot</c>, so two concurrent attaches racing toward the cap can never both
/// succeed past it. On distributor-register failure the count is rolled back (see the
/// catch below).
/// </remarks> /// </remarks>
public IEventSubscriberLease AttachEventSubscriber(bool allowMultipleSubscribers, int maxSubscribers) public IEventSubscriberLease AttachEventSubscriber(int maxSubscribers)
{ {
// Effective cap: 1 in single-subscriber mode, otherwise the configured maximum // Derive the mode from the same source the distributor uses so the two can never
// (clamped to at least 1 so a misconfigured non-positive value can never deadlock // diverge. Effective cap: 1 in single-subscriber mode, otherwise the configured
// attaches in multi-subscriber mode). // maximum (clamped to at least 1 so a misconfigured non-positive value can never
// deadlock attaches in multi-subscriber mode).
bool allowMultipleSubscribers = _eventStreaming.AllowMultipleEventSubscribers;
int effectiveCap = allowMultipleSubscribers ? Math.Max(1, maxSubscribers) : 1; int effectiveCap = allowMultipleSubscribers ? Math.Max(1, maxSubscribers) : 1;
lock (_syncRoot) lock (_syncRoot)
@@ -1493,6 +1494,10 @@ public sealed class GatewaySession
{ {
lock (_syncRoot) lock (_syncRoot)
{ {
// Assert in debug so a genuine double-decrement (a logic error) surfaces
// loudly; the clamp below keeps release builds safe if it somehow fires.
Debug.Assert(_activeEventSubscriberCount > 0,
"DetachEventSubscriber called with _activeEventSubscriberCount already at 0 — possible double-dispose.");
if (_activeEventSubscriberCount > 0) if (_activeEventSubscriberCount > 0)
{ {
_activeEventSubscriberCount--; _activeEventSubscriberCount--;
@@ -236,7 +236,11 @@ public sealed class SessionEventDistributor : IAsyncDisposable
} }
/// <summary> /// <summary>
/// Gets the count of currently-registered subscribers. /// Gets the count of currently-registered subscribers. This count INCLUDES internal
/// subscribers (e.g. the gateway-owned dashboard mirror registered via
/// <c>Register(isInternal: true)</c>), and therefore differs from
/// <see cref="GatewaySession.ActiveEventSubscriberCount"/>, which tracks only external
/// (gRPC) subscribers and excludes the internal dashboard subscriber.
/// </summary> /// </summary>
public int SubscriberCount => _subscribers.Count; public int SubscriberCount => _subscribers.Count;
@@ -151,7 +151,7 @@ public sealed class GatewaySessionDashboardMirrorTests
session.MarkReady(); session.MarkReady();
Assert.Equal(0, session.ActiveEventSubscriberCount); Assert.Equal(0, session.ActiveEventSubscriberCount);
using IEventSubscriberLease lease = session.AttachEventSubscriber(allowMultipleSubscribers: false, maxSubscribers: 1); using IEventSubscriberLease lease = session.AttachEventSubscriber(maxSubscribers: 1);
Assert.Equal(1, session.ActiveEventSubscriberCount); Assert.Equal(1, session.ActiveEventSubscriberCount);
} }
@@ -168,8 +168,8 @@ public sealed class GatewaySessionTests
/// completion and a client cancellation both fire at the same time — must /// completion and a client cancellation both fire at the same time — must
/// decrement <c>_activeEventSubscriberCount</c> exactly once, never to 1. /// decrement <c>_activeEventSubscriberCount</c> exactly once, never to 1.
/// A negative count permanently blocks future subscribers because /// A negative count permanently blocks future subscribers because
/// <c>AttachEventSubscriber(allowMultipleSubscribers:false)</c> gates on /// <c>AttachEventSubscriber</c> gates on <c>_activeEventSubscriberCount >= effectiveCap</c>.
/// <c>_activeEventSubscriberCount > 0</c>. After both racing disposes finish, /// After both racing disposes finish,
/// the count must be exactly 0 and a subsequent single-subscriber attach must /// the count must be exactly 0 and a subsequent single-subscriber attach must
/// succeed. /// succeed.
/// </summary> /// </summary>
@@ -186,8 +186,7 @@ public sealed class GatewaySessionTests
for (int i = 0; i < Iterations; i++) for (int i = 0; i < Iterations; i++)
{ {
// Attach one subscriber; this increments _activeEventSubscriberCount to 1. // Attach one subscriber; this increments _activeEventSubscriberCount to 1.
IEventSubscriberLease lease = session.AttachEventSubscriber( IEventSubscriberLease lease = session.AttachEventSubscriber(maxSubscribers: 1);
allowMultipleSubscribers: false, maxSubscribers: 1);
// Race Concurrency threads all calling Dispose() on the same lease. // Race Concurrency threads all calling Dispose() on the same lease.
// Only one must actually run DetachEventSubscriber. // Only one must actually run DetachEventSubscriber.
@@ -212,8 +211,7 @@ public sealed class GatewaySessionTests
// Observable contract: a fresh single subscriber must now be attachable // Observable contract: a fresh single subscriber must now be attachable
// (i.e., the guard _activeEventSubscriberCount > 0 is false). // (i.e., the guard _activeEventSubscriberCount > 0 is false).
IEventSubscriberLease next = session.AttachEventSubscriber( IEventSubscriberLease next = session.AttachEventSubscriber(maxSubscribers: 1);
allowMultipleSubscribers: false, maxSubscribers: 1);
next.Dispose(); next.Dispose();
Assert.Equal(0, session.ActiveEventSubscriberCount); Assert.Equal(0, session.ActiveEventSubscriberCount);
} }
@@ -233,11 +231,10 @@ public sealed class GatewaySessionTests
FakeWorkerClient workerClient = new(); FakeWorkerClient workerClient = new();
GatewaySession session = CreateReadySessionWithEventStreaming(workerClient); GatewaySession session = CreateReadySessionWithEventStreaming(workerClient);
using IEventSubscriberLease first = session.AttachEventSubscriber( using IEventSubscriberLease first = session.AttachEventSubscriber(maxSubscribers: 8);
allowMultipleSubscribers: false, maxSubscribers: 8);
SessionManagerException exception = Assert.Throws<SessionManagerException>( SessionManagerException exception = Assert.Throws<SessionManagerException>(
() => session.AttachEventSubscriber(allowMultipleSubscribers: false, maxSubscribers: 8)); () => session.AttachEventSubscriber(maxSubscribers: 8));
Assert.Equal(SessionManagerErrorCode.EventSubscriberAlreadyActive, exception.ErrorCode); Assert.Equal(SessionManagerErrorCode.EventSubscriberAlreadyActive, exception.ErrorCode);
Assert.Equal(1, session.ActiveEventSubscriberCount); Assert.Equal(1, session.ActiveEventSubscriberCount);
@@ -262,13 +259,13 @@ public sealed class GatewaySessionTests
List<IEventSubscriberLease> leases = []; List<IEventSubscriberLease> leases = [];
for (int i = 0; i < Cap; i++) for (int i = 0; i < Cap; i++)
{ {
leases.Add(session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap)); leases.Add(session.AttachEventSubscriber(maxSubscribers: Cap));
} }
Assert.Equal(Cap, session.ActiveEventSubscriberCount); Assert.Equal(Cap, session.ActiveEventSubscriberCount);
SessionManagerException exception = Assert.Throws<SessionManagerException>( SessionManagerException exception = Assert.Throws<SessionManagerException>(
() => session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap)); () => session.AttachEventSubscriber(maxSubscribers: Cap));
Assert.Equal(SessionManagerErrorCode.EventSubscriberLimitReached, exception.ErrorCode); Assert.Equal(SessionManagerErrorCode.EventSubscriberLimitReached, exception.ErrorCode);
Assert.Equal(Cap, session.ActiveEventSubscriberCount); Assert.Equal(Cap, session.ActiveEventSubscriberCount);
@@ -304,14 +301,14 @@ public sealed class GatewaySessionTests
List<IEventSubscriberLease> leases = []; List<IEventSubscriberLease> leases = [];
for (int i = 0; i < Cap; i++) for (int i = 0; i < Cap; i++)
{ {
leases.Add(session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap)); leases.Add(session.AttachEventSubscriber(maxSubscribers: Cap));
} }
Assert.Equal(Cap, session.ActiveEventSubscriberCount); Assert.Equal(Cap, session.ActiveEventSubscriberCount);
// The (cap+1)-th still fails: the dashboard mirror did not eat a slot. // The (cap+1)-th still fails: the dashboard mirror did not eat a slot.
SessionManagerException exception = Assert.Throws<SessionManagerException>( SessionManagerException exception = Assert.Throws<SessionManagerException>(
() => session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap)); () => session.AttachEventSubscriber(maxSubscribers: Cap));
Assert.Equal(SessionManagerErrorCode.EventSubscriberLimitReached, exception.ErrorCode); Assert.Equal(SessionManagerErrorCode.EventSubscriberLimitReached, exception.ErrorCode);
foreach (IEventSubscriberLease lease in leases) foreach (IEventSubscriberLease lease in leases)
@@ -356,8 +353,7 @@ public sealed class GatewaySessionTests
await gate.WaitAsync(testTimeout); await gate.WaitAsync(testTimeout);
try try
{ {
IEventSubscriberLease lease = session.AttachEventSubscriber( IEventSubscriberLease lease = session.AttachEventSubscriber(maxSubscribers: Cap);
allowMultipleSubscribers: true, maxSubscribers: Cap);
leases[index] = lease; leases[index] = lease;
Interlocked.Increment(ref successCount); Interlocked.Increment(ref successCount);
} }
@@ -411,13 +407,13 @@ public sealed class GatewaySessionTests
workerClient, workerClient,
allowMultipleEventSubscribers: true); allowMultipleEventSubscribers: true);
IEventSubscriberLease a = session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap); IEventSubscriberLease a = session.AttachEventSubscriber(maxSubscribers: Cap);
IEventSubscriberLease b = session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap); IEventSubscriberLease b = session.AttachEventSubscriber(maxSubscribers: Cap);
Assert.Equal(Cap, session.ActiveEventSubscriberCount); Assert.Equal(Cap, session.ActiveEventSubscriberCount);
// At cap: next attach is rejected. // At cap: next attach is rejected.
Assert.Throws<SessionManagerException>( Assert.Throws<SessionManagerException>(
() => session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap)); () => session.AttachEventSubscriber(maxSubscribers: Cap));
// Dispose one — and dispose it twice. The second dispose must not double-free. // Dispose one — and dispose it twice. The second dispose must not double-free.
a.Dispose(); a.Dispose();
@@ -425,10 +421,10 @@ public sealed class GatewaySessionTests
Assert.Equal(1, session.ActiveEventSubscriberCount); Assert.Equal(1, session.ActiveEventSubscriberCount);
// Exactly one slot is free, so exactly one fresh attach succeeds. // Exactly one slot is free, so exactly one fresh attach succeeds.
using IEventSubscriberLease c = session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap); using IEventSubscriberLease c = session.AttachEventSubscriber(maxSubscribers: Cap);
Assert.Equal(Cap, session.ActiveEventSubscriberCount); Assert.Equal(Cap, session.ActiveEventSubscriberCount);
Assert.Throws<SessionManagerException>( Assert.Throws<SessionManagerException>(
() => session.AttachEventSubscriber(allowMultipleSubscribers: true, maxSubscribers: Cap)); () => session.AttachEventSubscriber(maxSubscribers: Cap));
b.Dispose(); b.Dispose();
@@ -565,6 +565,9 @@ public sealed class SessionEventDistributorTests
} }
}); });
// Guard: ensure the handler actually fired before asserting its observed value.
// Without this the test could pass vacuously if the overflow never triggered.
Assert.Equal(1, Volatile.Read(ref observedSet));
Assert.True(Volatile.Read(ref observedValue), Assert.True(Volatile.Read(ref observedValue),
"isOnlySubscriber must be true for a lone external subscriber in single-subscriber mode."); "isOnlySubscriber must be true for a lone external subscriber in single-subscriber mode.");
} }
@@ -743,7 +743,7 @@ public sealed class SessionManagerTests
GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", ownerKeyId: null, CancellationToken.None); GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", ownerKeyId: null, CancellationToken.None);
DateTimeOffset now = DateTimeOffset.UtcNow; DateTimeOffset now = DateTimeOffset.UtcNow;
session.ExtendLease(now.AddSeconds(-1)); session.ExtendLease(now.AddSeconds(-1));
using IDisposable eventSubscriber = session.AttachEventSubscriber(allowMultipleSubscribers: false, maxSubscribers: 1); using IDisposable eventSubscriber = session.AttachEventSubscriber(maxSubscribers: 1);
int closedCount = await manager.CloseExpiredLeasesAsync(now, CancellationToken.None); int closedCount = await manager.CloseExpiredLeasesAsync(now, CancellationToken.None);