fix(sessions): account SessionClosed for sweep-driven closes

TryBeginCloseIfExpired set _closeStarted when it reserved a session, so the
sweep's own CloseAsync reported AlreadyClosed: true and CloseSessionCoreAsync
skipped SessionClosed: lease-expired, faulted-reaped and detach-grace-expired
closes never left the sessions.open gauge and under-counted sessions.closed.
The reservation now writes Closing only; the first CloseAsync to take the
close lock does the accounting exactly once.
This commit is contained in:
Joseph Doherty
2026-10-22 06:19:20 -04:00
parent 87f23d9712
commit 09666cc719
3 changed files with 75 additions and 1 deletions
+2
View File
@@ -222,6 +222,8 @@ Sessions open with `MxGateway:Sessions:DefaultLeaseSeconds` (default 1800) added
A sweep pass is two phases. *Selection* stays a single sequential pass over the snapshot, because that is what gives the precedence rule (lease-expiry, then faulted, then detach-grace) and the `TryBeginCloseIfExpired` TOCTOU re-check their meaning. *Closing* then runs over the already-selected set with `Parallel.ForEachAsync` at `MaxParallelSessionCloses`, a compile-time constant of `4` in `SessionManager`. Each close is bounded by `MxGateway:Worker:ShutdownTimeoutSeconds` (default 10), so a one-at-a-time sweep lets a few hung workers serialize reaping and starve session slots for the rest. Parallel closing is safe because `TryBeginCloseIfExpired` already flipped each selected session to `Closing` under its own lock — that idempotent begin-close is the per-session exclusivity invariant, so no two teardowns can ever run against one session. The degree is a fixed constant rather than an option, and bounded rather than unlimited, because every concurrent close is one x86 worker process being shut down or killed; the fan-out exists to hide a few hung workers, not to tear the whole registry down at once.
`TryBeginCloseIfExpired` is a *reservation*, not the close: it writes `Closing` but leaves the session's close-started flag alone, so the `CloseAsync` the sweep issues next is the first close to run and reports `AlreadyClosed: false`. That is what lets `CloseSessionCoreAsync` account `SessionClosed` for lease-expired, faulted-reaped and detach-grace-expired closes exactly once; a reservation that also set the flag made every sweep-driven close look like a second concurrent close, and swept sessions never left the `sessions.open` gauge. A client close that races the reservation still counts once: whichever `CloseAsync` takes the close lock first does the accounting, and the other finds the session `Closed`.
Splitting the phases moves the TOCTOU re-check earlier, and that is an accepted trade rather than an unchanged behavior: selection now flips **every** chosen session to `Closing` up front, before any teardown runs, whereas the sequential sweep re-checked session *N* only after sessions *1..N-1* had finished closing. A client that re-attaches a subscriber while the close phase is running therefore loses a race it could previously win — the eligibility snapshot is taken at one instant for the whole pass. Expiry evaluation itself is unaffected, because `now` is a parameter and is not re-read per session.
A close that throws no longer abandons the rest of the selected set: the sweep attempts every selected session, captures the first failure, and rethrows it once the pass is done so `SessionLeaseMonitorHostedService` still logs the sweep failure as before. Because the closes run concurrently, *which* failure surfaces when several fail in one pass is nondeterministic; the log line is the diagnostic, not the identity of the exception.
@@ -1822,6 +1822,15 @@ public sealed class GatewaySession
/// committing to <c>Closing</c>, so a reattach that wins the race leaves the session
/// in <c>Ready</c> and usable.
/// </para>
/// <para>
/// This is a <em>reservation</em>, not the close itself: it flips the state so no
/// reattach can revive the session and no other sweep pass re-selects it, but it
/// deliberately leaves <c>_closeStarted</c> untouched. The sweep's own
/// <see cref="CloseAsync"/> that follows is the first close to run, and must report
/// <c>AlreadyClosed: false</c> so the manager accounts <c>SessionClosed</c> for it;
/// setting <c>_closeStarted</c> here made that close look like a second, concurrent one
/// and every sweep-driven close went unaccounted.
/// </para>
/// </remarks>
internal bool TryBeginCloseIfExpired(DateTimeOffset now, out bool alreadyClosing)
{
@@ -1843,7 +1852,6 @@ public sealed class GatewaySession
}
alreadyClosing = _closeStarted;
_closeStarted = true;
_state = SessionState.Closing;
return true;
}
@@ -1095,6 +1095,59 @@ public sealed class SessionManagerTests
Assert.Equal(0, activeClient.ShutdownCount);
}
/// <summary>
/// Every sweep-driven close — lease expiry, faulted reap and detach-grace expiry — must
/// account <c>SessionClosed</c> exactly once. The sweep begins the close itself
/// (<c>TryBeginCloseIfExpired</c>) before delegating to the shared close path, and that
/// reservation must not make the delegated close look like a second, already-running close
/// whose accounting is skipped: the open-session gauge would then never drop for swept
/// sessions and the closed counter would under-count them.
/// </summary>
/// <param name="sweepKind">Which sweep condition selects the session.</param>
/// <returns>A task that represents the asynchronous operation.</returns>
[Theory]
[InlineData(SessionManager.LeaseExpiredReason)]
[InlineData(SessionManager.FaultedReason)]
[InlineData(SessionManager.DetachGraceExpiredReason)]
public async Task CloseExpiredLeasesAsync_SweptClose_AccountsSessionClosedExactlyOnce(string sweepKind)
{
FakeWorkerClient workerClient = new();
FakeTimeProvider clock = new(DateTimeOffset.UtcNow);
using GatewayMetrics metrics = new();
SessionManager manager = CreateManager(
new FakeSessionWorkerClientFactory(workerClient),
metrics: metrics,
options: CreateOptions(detachGraceSeconds: 30),
timeProvider: clock);
GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", ownerKeyId: null, CancellationToken.None);
Assert.Equal(1, metrics.GetSnapshot().OpenSessions);
switch (sweepKind)
{
case SessionManager.LeaseExpiredReason:
SetLeaseExpiry(session, clock.GetUtcNow().AddSeconds(-1));
break;
case SessionManager.FaultedReason:
session.MarkFaulted("test-fault");
break;
case SessionManager.DetachGraceExpiredReason:
session.AttachEventSubscriber(maxSubscribers: 1).Dispose();
clock.Advance(TimeSpan.FromSeconds(31));
break;
default:
throw new ArgumentOutOfRangeException(nameof(sweepKind), sweepKind, "Unknown sweep kind.");
}
int closedCount = await manager.CloseExpiredLeasesAsync(clock.GetUtcNow(), CancellationToken.None);
Assert.Equal(1, closedCount);
Assert.Equal(SessionState.Closed, session.State);
Assert.False(manager.TryGetSession(session.SessionId, out _));
GatewayMetricsSnapshot snapshot = metrics.GetSnapshot();
Assert.Equal(1, snapshot.SessionsClosed);
Assert.Equal(0, snapshot.OpenSessions);
}
/// <summary>Verifies that an expired-lease sweep leaves a session with an active event subscriber open.</summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
@@ -1436,6 +1489,17 @@ public sealed class SessionManagerTests
};
}
/// <summary>
/// Moves a session's lease expiry by back-dating its last client activity, which is the only
/// production path that moves the lease (<c>_leaseExpiresAt = activityAt + LeaseDuration</c>).
/// </summary>
/// <param name="session">Session whose lease to move.</param>
/// <param name="leaseExpiresAt">Wanted lease expiry.</param>
private static void SetLeaseExpiry(GatewaySession session, DateTimeOffset leaseExpiresAt)
{
session.TouchClientActivity(leaseExpiresAt - session.LeaseDuration);
}
private static SessionOpenRequest CreateOpenRequest()
{
return new SessionOpenRequest(