diff --git a/docs/Sessions.md b/docs/Sessions.md index 5d52e87..2354434 100644 --- a/docs/Sessions.md +++ b/docs/Sessions.md @@ -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. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs index 57393b4..651ad0e 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs @@ -1822,6 +1822,15 @@ public sealed class GatewaySession /// committing to Closing, so a reattach that wins the race leaves the session /// in Ready and usable. /// + /// + /// This is a reservation, 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 _closeStarted untouched. The sweep's own + /// that follows is the first close to run, and must report + /// AlreadyClosed: false so the manager accounts SessionClosed for it; + /// setting _closeStarted here made that close look like a second, concurrent one + /// and every sweep-driven close went unaccounted. + /// /// 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; } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs index 8e6c915..5fe409c 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs @@ -1095,6 +1095,59 @@ public sealed class SessionManagerTests Assert.Equal(0, activeClient.ShutdownCount); } + /// + /// Every sweep-driven close — lease expiry, faulted reap and detach-grace expiry — must + /// account SessionClosed exactly once. The sweep begins the close itself + /// (TryBeginCloseIfExpired) 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. + /// + /// Which sweep condition selects the session. + /// A task that represents the asynchronous operation. + [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); + } + /// Verifies that an expired-lease sweep leaves a session with an active event subscriber open. /// A task that represents the asynchronous operation. [Fact] @@ -1436,6 +1489,17 @@ public sealed class SessionManagerTests }; } + /// + /// Moves a session's lease expiry by back-dating its last client activity, which is the only + /// production path that moves the lease (_leaseExpiresAt = activityAt + LeaseDuration). + /// + /// Session whose lease to move. + /// Wanted lease expiry. + private static void SetLeaseExpiry(GatewaySession session, DateTimeOffset leaseExpiresAt) + { + session.TouchClientActivity(leaseExpiresAt - session.LeaseDuration); + } + private static SessionOpenRequest CreateOpenRequest() { return new SessionOpenRequest(