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.
This commit is contained in:
@@ -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)
|
||||
{
|
||||
|
||||
@@ -2,7 +2,21 @@ using ZB.MOM.WW.MxGateway.Contracts.Proto;
|
||||
|
||||
namespace ZB.MOM.WW.MxGateway.Server.Sessions;
|
||||
|
||||
/// <summary>Outcome of closing a session.</summary>
|
||||
/// <param name="SessionId">Identifier of the closed session.</param>
|
||||
/// <param name="FinalState">State the session ended in.</param>
|
||||
/// <param name="AlreadyClosed">
|
||||
/// <see langword="true"/> 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.
|
||||
/// </param>
|
||||
/// <param name="WorkerShutdownFailure">
|
||||
/// The exception the worker's graceful shutdown threw when the close had to fall back to
|
||||
/// killing the worker; <see langword="null"/> 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.
|
||||
/// </param>
|
||||
public sealed record SessionCloseResult(
|
||||
string SessionId,
|
||||
SessionState FinalState,
|
||||
bool AlreadyClosed);
|
||||
bool AlreadyClosed,
|
||||
Exception? WorkerShutdownFailure = null);
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -641,10 +641,16 @@ public sealed class SessionManagerTests
|
||||
Assert.Equal(0, metrics.GetSnapshot().OpenSessions);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that closing a session kills the worker when shutdown fails.</summary>
|
||||
/// <summary>
|
||||
/// A worker that does not shut down gracefully is killed, and that is a <em>successful</em>
|
||||
/// close: the kill is the documented fallback, the worker is gone and the session is
|
||||
/// <c>Closed</c>. Reporting it as <c>CloseFailed</c> + <c>Faulted</c> + a fault metric
|
||||
/// misreported every hung-worker close as a teardown failure. <c>CloseFailed</c> is reserved
|
||||
/// for both the shutdown and the kill failing.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[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<SessionManagerException>(
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that when worker shutdown fails, the session is removed and the slot is released.</summary>
|
||||
@@ -689,31 +704,59 @@ public sealed class SessionManagerTests
|
||||
CancellationToken.None);
|
||||
metrics.EventReceived(firstSession.SessionId, MxEventFamily.OnDataChange.ToString());
|
||||
|
||||
SessionManagerException exception = await Assert.ThrowsAsync<SessionManagerException>(
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>CloseFailed</c>.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[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<SessionManagerException>(
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that when the second close is canceled, the session is not removed if owned by the first close.</summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[Fact]
|
||||
@@ -926,7 +969,9 @@ public sealed class SessionManagerTests
|
||||
/// <c>KillWorker</c> + <c>RemoveSessionAsync</c> when <c>CloseSessionCoreAsync</c> throws)
|
||||
/// must still account a successful close: both the open-session gauge must drop to zero AND
|
||||
/// the <c>mxgateway.sessions.closed</c> 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.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[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(
|
||||
|
||||
Reference in New Issue
Block a user