From 2b165cd3a42df93e1c4d00da1f2bf9466f56dfda Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:26:33 -0400 Subject: [PATCH] fix(sessions): report a close whose kill fallback succeeded as Closed, not CloseFailed CloseAsync rethrew the graceful-shutdown exception after a successful Kill, so the manager faulted the session, recorded a CloseFailed fault and surfaced CloseFailed to the caller for the ordinary hung-worker case. docs/Sessions.md reserves CloseFailed for both shutdown and kill failing. A successful kill now completes the close as Closed; the shutdown failure rides back on SessionCloseResult.WorkerShutdownFailure and the manager logs it at warning level. Both-fail still raises CloseFailed. --- docs/Sessions.md | 21 +++-- .../Sessions/GatewaySession.cs | 11 ++- .../Sessions/SessionCloseResult.cs | 16 +++- .../Sessions/SessionManager.cs | 12 +++ .../Gateway/Sessions/SessionManagerTests.cs | 76 +++++++++++++++---- 5 files changed, 111 insertions(+), 25 deletions(-) diff --git a/docs/Sessions.md b/docs/Sessions.md index f8798de..101f700 100644 --- a/docs/Sessions.md +++ b/docs/Sessions.md @@ -137,18 +137,19 @@ public sealed record SessionOpenRequest( ### SessionCloseResult -`SessionCloseResult` is the record returned from a successful close. `AlreadyClosed` distinguishes "this call closed the session" from "the session was already closed when we acquired the close lock", which the metrics layer uses to avoid double-counting. +`SessionCloseResult` is the record returned from a successful close. `AlreadyClosed` distinguishes "this call closed the session" from "the session was already closed when we acquired the close lock", which the metrics layer uses to avoid double-counting. `WorkerShutdownFailure` is set when the worker refused a graceful shutdown and was killed instead — still a successful close (see [Close](#close)); the manager logs it and the gRPC reply does not expose it. ```csharp public sealed record SessionCloseResult( string SessionId, SessionState FinalState, - bool AlreadyClosed); + bool AlreadyClosed, + Exception? WorkerShutdownFailure = null); ``` ### SessionCloseStartedException -`SessionCloseStartedException` is `internal` and is only thrown from inside `GatewaySession.CloseAsync` when the close path has already begun mutating worker state and a subsequent step fails. `SessionManager.CloseSessionCoreAsync` catches it, marks the session faulted, increments the close-failed metric, removes the session from the registry, and rethrows it wrapped as `SessionManagerException` with `CloseFailed`. The intermediate type exists so the public API surface only ever exposes `SessionManagerException`. +`SessionCloseStartedException` is `internal` and is only thrown from inside `GatewaySession.CloseAsync` when the close path has already begun mutating worker state and a subsequent step fails — in practice, when the worker's graceful shutdown failed *and* the kill fall-back failed too, so a worker may still be running. `SessionManager.CloseSessionCoreAsync` catches it, marks the session faulted, increments the close-failed metric, removes the session from the registry, and rethrows it wrapped as `SessionManagerException` with `CloseFailed`. The intermediate type exists so the public API surface only ever exposes `SessionManagerException`. ### SessionManagerException and SessionManagerErrorCode @@ -163,7 +164,7 @@ public sealed record SessionCloseResult( | `EventQueueOverflow` | Reserved for the worker event channel overflow path. | | `SessionLimitExceeded` | `MaxSessions` is in use. | | `OpenFailed` | `OpenSessionAsync` failed; the inner exception carries the cause. | -| `CloseFailed` | A close started but did not complete cleanly; the session is removed and faulted. | +| `CloseFailed` | A close started but did not complete cleanly — the worker's graceful shutdown and the kill fall-back both failed (or a kill threw); the session is removed and faulted. A shutdown failure whose kill succeeded is a successful close, not this. | | `PermissionDenied` | The caller may not perform the operation on this session (for example, attaching `StreamEvents` to a session opened by a different API key). | | `GatewayStopping` | The gateway is stopping and refuses new sessions (see [Shutdown Coordination](#shutdown-coordination)). Maps to gRPC `Unavailable`; not recorded as a fault. | @@ -275,6 +276,7 @@ The ReplayGap sentinel is emitted **only** on the `StreamEvents` server stream a `GatewaySession.CloseAsync` is serialized by a per-session `SemaphoreSlim` (`_closeLock`) so only one close runs at a time, but every read/write of `_state` still passes through `_syncRoot` (via `TryBeginClose` and `MarkClosed`). The close path therefore obeys the same lock discipline as `TransitionTo` / `MarkFaulted`: it transitions to `Closing`, asks the worker client to shut down within `ShutdownTimeout`, and on success transitions to `Closed`. `DisposeAsync` waits on `_closeLock` once before disposing the semaphore so an in-flight close's `Release()` cannot race against the dispose. If `WorkerClient.ShutdownAsync` throws, the session falls back to `IWorkerClient.Kill` (forced close): ```csharp +Exception? workerShutdownFailure = null; if (_workerClient is not null) { try @@ -294,14 +296,19 @@ if (_workerClient is not null) new AggregateException(exception, killException)); } - throw; + workerShutdownFailure = exception; } } + +MarkClosed(); +return new SessionCloseResult(SessionId, SessionState.Closed, alreadyClosing, workerShutdownFailure); ``` -If both graceful shutdown and the kill fall-back fail, the original and kill exceptions are bundled into an `AggregateException` and surfaced as `SessionCloseStartedException`. `SessionManager.CloseSessionCoreAsync` then translates that into a `SessionManagerException` with `CloseFailed` and removes the session. +A kill that succeeds completes the close **successfully**: the worker is gone and the session is `Closed`, exactly as if the shutdown had worked, and the caller's `CloseSession` reply says so. The graceful failure is not lost — it travels back as `SessionCloseResult.WorkerShutdownFailure`, and `SessionManager.CloseSessionCoreAsync` logs it at warning level ("did not shut down gracefully and was killed"), which is the operator signal that workers are hanging on shutdown. The kill itself is counted by `WorkerClient.Kill` (`mxgateway.workers.killed`). Nothing is faulted and no `CloseFailed` fault is recorded, because nothing failed from the session's point of view; reporting the common hung-worker case that way had made every such close look like a teardown failure. -`GatewaySession.KillWorker` is the unconditional forced-close path used by shutdown when graceful close itself throws, and also by `SessionManager.KillWorkerAsync` — the explicit kill path that the dashboard's admin Kill button invokes. `KillWorkerAsync` skips `WorkerClient.ShutdownAsync` entirely, so `KillCount` increments while `ShutdownCount` does not; the session is then removed from the registry and the open-session slot is released, identical to the cleanup that follows a successful `CloseSessionAsync`. +Only if both the graceful shutdown and the kill fall-back fail are the original and kill exceptions bundled into an `AggregateException` and surfaced as `SessionCloseStartedException`. `SessionManager.CloseSessionCoreAsync` then translates that into a `SessionManagerException` with `CloseFailed`, faults the session, records the fault metric and removes the session — a worker may still be running, which is what `CloseFailed` is reserved for. + +`SessionManager.KillWorkerAsync` is the explicit kill path that the dashboard's admin Kill button invokes and that `ShutdownAsync` falls back to when a graceful close throws. It goes through `GatewaySession.KillWorkerWithCloseGateAsync`, which skips `WorkerClient.ShutdownAsync` entirely, so `KillCount` increments while `ShutdownCount` does not; the session is then removed from the registry and the open-session slot is released, identical to the cleanup that follows a successful `CloseSessionAsync`. ## Shutdown Coordination diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs index 651ad0e..b227352 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs @@ -1731,6 +1731,7 @@ public sealed class GatewaySession return new SessionCloseResult(SessionId, SessionState.Closed, AlreadyClosed: true); } + Exception? workerShutdownFailure = null; if (_workerClient is not null) { try @@ -1739,6 +1740,12 @@ public sealed class GatewaySession } catch (Exception exception) { + // The kill is the documented fallback for a worker that will not shut down + // (typically a ShutdownTimeout on a hung worker). When it succeeds the + // worker is gone and this close has done its job, so it completes as a + // normal close carrying the shutdown failure for the caller to log; only a + // kill that ALSO fails is a close failure (SessionCloseStartedException → + // CloseFailed), because then a worker may still be running. try { _workerClient.Kill(reason); @@ -1750,12 +1757,12 @@ public sealed class GatewaySession new AggregateException(exception, killException)); } - throw; + workerShutdownFailure = exception; } } MarkClosed(); - return new SessionCloseResult(SessionId, SessionState.Closed, alreadyClosing); + return new SessionCloseResult(SessionId, SessionState.Closed, alreadyClosing, workerShutdownFailure); } catch (Exception exception) when (exception is not SessionCloseStartedException) { diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionCloseResult.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionCloseResult.cs index 6cab07c..e0d5260 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionCloseResult.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionCloseResult.cs @@ -2,7 +2,21 @@ using ZB.MOM.WW.MxGateway.Contracts.Proto; namespace ZB.MOM.WW.MxGateway.Server.Sessions; +/// Outcome of closing a session. +/// Identifier of the closed session. +/// State the session ended in. +/// +/// when the session was already closed (or being closed by another +/// caller) when this close acquired the close lock, so this call did no teardown of its own. +/// +/// +/// The exception the worker's graceful shutdown threw when the close had to fall back to +/// killing the worker; when the worker shut down cleanly. The close is +/// still a success — the worker is gone and the session is closed — so this is diagnostic +/// detail for the caller's log, not an error. +/// public sealed record SessionCloseResult( string SessionId, SessionState FinalState, - bool AlreadyClosed); + bool AlreadyClosed, + Exception? WorkerShutdownFailure = null); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs index a3520af..51fdb02 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs @@ -565,6 +565,18 @@ public sealed class SessionManager : ISessionManager try { SessionCloseResult result = await session.CloseAsync(reason, cancellationToken).ConfigureAwait(false); + if (result.WorkerShutdownFailure is not null) + { + // A clean close from the session's point of view (the worker was killed and the + // session is Closed), but the operator should see that the worker would not stop + // gracefully — a run of these is the signal that workers are hanging on shutdown. + _logger.LogWarning( + result.WorkerShutdownFailure, + "Worker for session {SessionId} did not shut down gracefully and was killed; reason={Reason}.", + session.SessionId, + reason); + } + if (!wasClosed && !result.AlreadyClosed && session.OpenRecorded) { _metrics.SessionClosed(); 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 f1773a9..e84376d 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs @@ -641,10 +641,16 @@ public sealed class SessionManagerTests Assert.Equal(0, metrics.GetSnapshot().OpenSessions); } - /// Verifies that closing a session kills the worker when shutdown fails. + /// + /// A worker that does not shut down gracefully is killed, and that is a successful + /// close: the kill is the documented fallback, the worker is gone and the session is + /// Closed. Reporting it as CloseFailed + Faulted + a fault metric + /// misreported every hung-worker close as a teardown failure. CloseFailed is reserved + /// for both the shutdown and the kill failing. + /// /// A task that represents the asynchronous operation. [Fact] - public async Task CloseSessionAsync_WhenWorkerShutdownFails_KillsWorker() + public async Task CloseSessionAsync_WhenWorkerShutdownFails_KillsWorkerAndReportsClosed() { FakeWorkerClient workerClient = new() { @@ -652,15 +658,24 @@ public sealed class SessionManagerTests WorkerClientErrorCode.ShutdownTimeout, "Worker shutdown timed out."), }; - SessionManager manager = CreateManager(new FakeSessionWorkerClientFactory(workerClient)); + using GatewayMetrics metrics = new(); + SessionManager manager = CreateManager(new FakeSessionWorkerClientFactory(workerClient), metrics: metrics); GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", ownerKeyId: null, CancellationToken.None); - SessionManagerException exception = await Assert.ThrowsAsync( - async () => await manager.CloseSessionAsync(session.SessionId, CancellationToken.None)); + SessionCloseResult result = await manager.CloseSessionAsync(session.SessionId, CancellationToken.None); - Assert.Equal(SessionManagerErrorCode.CloseFailed, exception.ErrorCode); + Assert.Equal(SessionState.Closed, result.FinalState); + Assert.False(result.AlreadyClosed); + Assert.Same(workerClient.ShutdownException, result.WorkerShutdownFailure); + Assert.Equal(SessionState.Closed, session.State); + Assert.Null(session.FinalFault); Assert.Equal(1, workerClient.ShutdownCount); Assert.Equal(1, workerClient.KillCount); + Assert.False(manager.TryGetSession(session.SessionId, out _)); + GatewayMetricsSnapshot snapshot = metrics.GetSnapshot(); + Assert.Equal(0, snapshot.Faults); + Assert.Equal(1, snapshot.SessionsClosed); + Assert.Equal(0, snapshot.OpenSessions); } /// Verifies that when worker shutdown fails, the session is removed and the slot is released. @@ -689,31 +704,59 @@ public sealed class SessionManagerTests CancellationToken.None); metrics.EventReceived(firstSession.SessionId, MxEventFamily.OnDataChange.ToString()); - SessionManagerException exception = await Assert.ThrowsAsync( - async () => await manager.CloseSessionAsync(firstSession.SessionId, CancellationToken.None)); + SessionCloseResult result = await manager.CloseSessionAsync(firstSession.SessionId, CancellationToken.None); GatewaySession secondSession = await manager.OpenSessionAsync( CreateOpenRequest(), "client-2", ownerKeyId: null, CancellationToken.None); - Assert.Equal(SessionManagerErrorCode.CloseFailed, exception.ErrorCode); + Assert.Equal(SessionState.Closed, result.FinalState); Assert.False(manager.TryGetSession(firstSession.SessionId, out _)); Assert.True(manager.TryGetSession(secondSession.SessionId, out _)); Assert.Equal(1, registry.Count); Assert.Equal(1, failingWorkerClient.KillCount); Assert.Equal(1, failingWorkerClient.DisposeCount); GatewayMetricsSnapshot snapshot = metrics.GetSnapshot(); - - // A close-that-failed accounts as SessionClosed (counter += 1) rather - // than SessionRemoved (gauge -= 1, counter unchanged). The session is - // being removed from the registry on this path, so it must show up in - // the closed count. Assert.Equal(1, snapshot.SessionsClosed); Assert.False(snapshot.EventsBySession.ContainsKey(firstSession.SessionId)); Assert.Equal(1, snapshot.OpenSessions); } + /// + /// Only when the graceful shutdown AND the kill fallback both fail is the close a failure: + /// the session is faulted, removed, counted as closed (it is leaving the registry) and the + /// caller gets CloseFailed. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task CloseSessionAsync_WhenShutdownAndKillBothFail_ThrowsCloseFailed() + { + FakeWorkerClient workerClient = new() + { + ShutdownException = new WorkerClientException( + WorkerClientErrorCode.ShutdownTimeout, + "Worker shutdown timed out."), + KillException = new InvalidOperationException("worker kill failed"), + }; + using GatewayMetrics metrics = new(); + SessionManager manager = CreateManager(new FakeSessionWorkerClientFactory(workerClient), metrics: metrics); + GatewaySession session = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", ownerKeyId: null, CancellationToken.None); + + SessionManagerException exception = await Assert.ThrowsAsync( + async () => await manager.CloseSessionAsync(session.SessionId, CancellationToken.None)); + + Assert.Equal(SessionManagerErrorCode.CloseFailed, exception.ErrorCode); + Assert.Equal(SessionState.Faulted, session.State); + Assert.Equal(1, workerClient.ShutdownCount); + Assert.Equal(1, workerClient.KillCount); + Assert.False(manager.TryGetSession(session.SessionId, out _)); + GatewayMetricsSnapshot snapshot = metrics.GetSnapshot(); + Assert.Equal(1, snapshot.Faults); + Assert.Equal(1, snapshot.SessionsClosed); + Assert.Equal(0, snapshot.OpenSessions); + } + /// Verifies that when the second close is canceled, the session is not removed if owned by the first close. /// A task that represents the asynchronous operation. [Fact] @@ -926,7 +969,9 @@ public sealed class SessionManagerTests /// KillWorker + RemoveSessionAsync when CloseSessionCoreAsync throws) /// must still account a successful close: both the open-session gauge must drop to zero AND /// the mxgateway.sessions.closed counter must increment. Without the fix, the - /// graceful-close failure path under-counts the closed counter. + /// graceful-close failure path under-counts the closed counter. Both the shutdown and the + /// in-close kill fail here, because a kill that succeeds is a clean close and never reaches + /// the fallback. /// /// A task that represents the asynchronous operation. [Fact] @@ -935,6 +980,7 @@ public sealed class SessionManagerTests FakeWorkerClient throwingClient = new() { ShutdownException = new InvalidOperationException("worker shutdown failed"), + KillException = new InvalidOperationException("worker kill failed"), }; using GatewayMetrics metrics = new(); SessionManager manager = CreateManager(