fix(sessions): exception-total shutdown body with non-cancellable kill fallback
This commit is contained in:
Joseph Doherty
+23
-12
@@ -209,9 +209,15 @@ Sessions open with `MxGateway:Sessions:DefaultLeaseSeconds` (default 1800) added
|
||||
|
||||
#### Teardown parallelism
|
||||
|
||||
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` (4). 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 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.
|
||||
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.
|
||||
|
||||
A close that throws no longer abandons the rest of the selected set: the sweep captures the first failure, finishes the remaining closes, then rethrows it so `SessionLeaseMonitorHostedService` still logs the sweep failure as before. `ShutdownAsync` drains sessions with the same bounded fan-out, keeping its existing per-session catch → `KillWorkerAsync` fallback.
|
||||
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.
|
||||
|
||||
`ShutdownAsync` drains sessions with the same bounded fan-out and the same per-session catch → `KillWorkerAsync` fallback, with two rules that keep a stop deadline from turning into leaked workers. First, its body is **exception-total** — nothing escapes it — because `Parallel.ForEachAsync` cancels the token handed to the sibling bodies as soon as one body throws, which would abort in-flight graceful shutdowns *and* make their kill fallback fail instantly on the freshly cancelled token. Second, the drain loop is deliberately **not** bound to the caller's `CancellationToken` and the kill fallback runs on `CancellationToken.None`: a cancelled `ParallelOptions` token stops dispatching the remaining sessions entirely, whereas the sequential drain this replaced let every remaining session fail its graceful close fast and still kill its worker. The token is passed to the graceful close instead, so a host stop deadline turns the drain into a kill sweep rather than into a leak. This matters because nothing reattaches to a leaked worker — a restarted gateway terminates orphans (see [Design Decisions](DesignDecisions.md)).
|
||||
|
||||
**Stranded-`Closing` bound.** A sweep pass that is cancelled after selection leaves its unclosed selections in `Closing` with close already started. `IsFaultedReapableCore` requires `state == Faulted`, so a session selected under `FaultedReason` and stranded this way is not re-selected as faulted; it is swept only when its normal lease expires (up to `MxGateway:Sessions:DefaultLeaseSeconds`, default 1800 s), since `IsLeaseExpiredCore` and `IsDetachGraceExpiredCore` are state-agnostic. This bound is documented rather than closed with a re-selection clause: the sweep's only caller cancels on the host's `stoppingToken`, so the very next thing that runs is `ShutdownAsync`, which drains (or kills) the whole registry — and any worker that still survives that is terminated as an orphan on the next gateway start. Adding a "`Closing` and close-started" re-selection clause would also have to distinguish an abandoned close from one that is merely still in flight, which would weaken the single invariant that makes the parallel close phase safe.
|
||||
|
||||
#### Detach-grace retention
|
||||
|
||||
@@ -282,21 +288,17 @@ If both graceful shutdown and the kill fall-back fail, the original and kill exc
|
||||
|
||||
## Shutdown Coordination
|
||||
|
||||
`SessionShutdownHostedService.StopAsync` calls `SessionManager.ShutdownAsync`, which closes every registered session with `GatewayShutdownReason`. Sessions are drained with the same bounded fan-out the lease sweep uses (`MaxParallelSessionCloses`), because a one-at-a-time drain of a full registry at a worst-case worker shutdown timeout each outruns any host stop-timeout and leaves the tail to the orphan killer. Each iteration catches its own exceptions, calls `KillWorkerAsync`, and removes the session so that one stuck worker cannot block the rest of the host:
|
||||
`SessionShutdownHostedService.StopAsync` calls `SessionManager.ShutdownAsync`, which closes every registered session with `GatewayShutdownReason`. Sessions are drained with the same bounded fan-out the lease sweep uses (`MaxParallelSessionCloses`), because a one-at-a-time drain of a full registry at a worst-case worker shutdown timeout each outruns any host stop-timeout and leaves the tail to the orphan killer. Each iteration catches its own exceptions — *every* exception, including from the fallback — calls `KillWorkerAsync` on an uncancellable token, and removes the session, so that neither one stuck worker nor one failing teardown can block or abort the rest of the host's drain:
|
||||
|
||||
```csharp
|
||||
await Parallel.ForEachAsync(
|
||||
_registry.Snapshot(),
|
||||
new ParallelOptions
|
||||
{
|
||||
MaxDegreeOfParallelism = MaxParallelSessionCloses,
|
||||
CancellationToken = cancellationToken,
|
||||
},
|
||||
async (session, closeToken) =>
|
||||
new ParallelOptions { MaxDegreeOfParallelism = MaxParallelSessionCloses },
|
||||
async (session, _) =>
|
||||
{
|
||||
try
|
||||
{
|
||||
await CloseSessionCoreAsync(session, GatewayShutdownReason, closeToken).ConfigureAwait(false);
|
||||
await CloseSessionCoreAsync(session, GatewayShutdownReason, cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
catch (Exception exception)
|
||||
{
|
||||
@@ -304,9 +306,18 @@ await Parallel.ForEachAsync(
|
||||
exception,
|
||||
"Graceful shutdown failed for session {SessionId}; killing worker.",
|
||||
session.SessionId);
|
||||
if (_registry.TryGet(session.SessionId, out _))
|
||||
if (_registry.TryGet(session.SessionId, out GatewaySession? registeredSession)
|
||||
&& registeredSession is not null)
|
||||
{
|
||||
await KillWorkerAsync(session.SessionId, GatewayShutdownReason, closeToken).ConfigureAwait(false);
|
||||
try
|
||||
{
|
||||
// Not the caller's token: the kill is the last-resort orphan preventer.
|
||||
await KillWorkerAsync(session.SessionId, GatewayShutdownReason, CancellationToken.None).ConfigureAwait(false);
|
||||
}
|
||||
catch (Exception killException)
|
||||
{
|
||||
_logger.LogWarning(killException, "Worker kill fallback failed for session {SessionId}.", session.SessionId);
|
||||
}
|
||||
}
|
||||
}
|
||||
}).ConfigureAwait(false);
|
||||
|
||||
@@ -68,6 +68,12 @@ public interface ISessionManager
|
||||
/// <param name="now">The current time to evaluate expiration against.</param>
|
||||
/// <param name="cancellationToken">Token to cancel the asynchronous operation.</param>
|
||||
/// <returns>The number of sessions closed.</returns>
|
||||
/// <remarks>
|
||||
/// A close that fails does not abandon the rest of the pass: every session selected by this
|
||||
/// sweep is attempted, and the first failure is then rethrown so the caller still observes
|
||||
/// (and logs) that the sweep failed. Which failure surfaces is nondeterministic when several
|
||||
/// closes fail in the same pass, because the closes run concurrently.
|
||||
/// </remarks>
|
||||
Task<int> CloseExpiredLeasesAsync(
|
||||
DateTimeOffset now,
|
||||
CancellationToken cancellationToken);
|
||||
|
||||
@@ -356,18 +356,28 @@ public sealed class SessionManager : ISessionManager
|
||||
// tail to the orphan killer. Per-session exclusivity comes from GatewaySession.CloseAsync's
|
||||
// own close gate, and each iteration touches only its own session plus thread-safe
|
||||
// registry/metrics state.
|
||||
//
|
||||
// The body must be exception-TOTAL. Parallel.ForEachAsync cancels the token it hands the
|
||||
// sibling bodies as soon as one body throws, so a single escaping exception would abort up
|
||||
// to MaxParallelSessionCloses - 1 in-flight graceful shutdowns AND make their kill fallback
|
||||
// throw immediately on the freshly cancelled token — sessions neither closed nor killed,
|
||||
// i.e. leaked x86 workers that nothing reattaches to (a gateway restart terminates orphans
|
||||
// rather than adopting them).
|
||||
//
|
||||
// For the same reason the loop itself is NOT bound to cancellationToken: a cancelled
|
||||
// ParallelOptions token stops dispatching the remaining sessions entirely, whereas the
|
||||
// sequential drain this replaced let every remaining session fail its graceful close fast
|
||||
// and still kill its worker. The token is passed to the graceful close instead, which
|
||||
// preserves that behavior — a host stop deadline turns the drain into a kill sweep rather
|
||||
// than into a leak.
|
||||
await Parallel.ForEachAsync(
|
||||
_registry.Snapshot(),
|
||||
new ParallelOptions
|
||||
{
|
||||
MaxDegreeOfParallelism = MaxParallelSessionCloses,
|
||||
CancellationToken = cancellationToken,
|
||||
},
|
||||
async (session, closeToken) =>
|
||||
new ParallelOptions { MaxDegreeOfParallelism = MaxParallelSessionCloses },
|
||||
async (session, _) =>
|
||||
{
|
||||
try
|
||||
{
|
||||
await CloseSessionCoreAsync(session, GatewayShutdownReason, closeToken).ConfigureAwait(false);
|
||||
await CloseSessionCoreAsync(session, GatewayShutdownReason, cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
catch (Exception exception)
|
||||
{
|
||||
@@ -376,13 +386,19 @@ public sealed class SessionManager : ISessionManager
|
||||
"Graceful shutdown failed for session {SessionId}; killing worker.",
|
||||
session.SessionId);
|
||||
|
||||
if (_registry.TryGet(session.SessionId, out _))
|
||||
if (_registry.TryGet(session.SessionId, out GatewaySession? registeredSession)
|
||||
&& registeredSession is not null)
|
||||
{
|
||||
try
|
||||
{
|
||||
await KillWorkerAsync(session.SessionId, GatewayShutdownReason, closeToken).ConfigureAwait(false);
|
||||
// Deliberately NOT the caller's token: the kill is the last-resort orphan
|
||||
// preventer, so it must still run when the host stop deadline (or a
|
||||
// sibling body's failure) has already cancelled the drain. It is a
|
||||
// synchronous Kill plus registry/dispose bookkeeping, not a wait on
|
||||
// the worker, so it cannot extend the drain meaningfully.
|
||||
await KillWorkerAsync(session.SessionId, GatewayShutdownReason, CancellationToken.None).ConfigureAwait(false);
|
||||
}
|
||||
catch (SessionManagerException killException)
|
||||
catch (Exception killException)
|
||||
{
|
||||
_logger.LogWarning(
|
||||
killException,
|
||||
|
||||
@@ -1171,6 +1171,33 @@ public sealed class SessionManagerTests
|
||||
Assert.False(manager.TryGetSession(failingSession.SessionId, out _));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A drain whose token is already cancelled (the host stop deadline elapsed) must still
|
||||
/// kill every worker rather than skip the teardown: an unkilled worker is a leaked x86
|
||||
/// process, and a restarted gateway terminates orphans instead of reattaching to them. This
|
||||
/// pins both halves of the fix — the parallel loop is not bound to the caller's token, and
|
||||
/// the kill fallback does not run on it.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[Fact]
|
||||
public async Task ShutdownAsync_WhenCancelledBeforeDraining_StillKillsEveryWorker()
|
||||
{
|
||||
FakeWorkerClient firstClient = new();
|
||||
FakeWorkerClient secondClient = new();
|
||||
SessionManager manager = CreateManager(new QueueingSessionWorkerClientFactory(firstClient, secondClient));
|
||||
GatewaySession firstSession = await manager.OpenSessionAsync(CreateOpenRequest(), "client-1", ownerKeyId: null, CancellationToken.None);
|
||||
GatewaySession secondSession = await manager.OpenSessionAsync(CreateOpenRequest(), "client-2", ownerKeyId: null, CancellationToken.None);
|
||||
using CancellationTokenSource cancellation = new();
|
||||
await cancellation.CancelAsync();
|
||||
|
||||
await manager.ShutdownAsync(cancellation.Token);
|
||||
|
||||
Assert.Equal(1, firstClient.KillCount);
|
||||
Assert.Equal(1, secondClient.KillCount);
|
||||
Assert.False(manager.TryGetSession(firstSession.SessionId, out _));
|
||||
Assert.False(manager.TryGetSession(secondSession.SessionId, out _));
|
||||
}
|
||||
|
||||
/// <summary>Verifies that shutdown closes all registered sessions.</summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[Fact]
|
||||
@@ -1510,10 +1537,12 @@ public sealed class SessionManagerTests
|
||||
{
|
||||
await _reached.Task.WaitAsync(RendezvousTimeout, cancellationToken);
|
||||
}
|
||||
catch (TimeoutException)
|
||||
catch (Exception exception) when (exception is TimeoutException or OperationCanceledException)
|
||||
{
|
||||
// Sequential teardown: the expected overlap never happens, so let the shutdown
|
||||
// finish and let MaxObservedConcurrency report the (failing) truth.
|
||||
// finish and let MaxObservedConcurrency report the (failing) truth. Cancellation is
|
||||
// swallowed for the same reason — a cancelled rendezvous must not turn into a
|
||||
// second, misleading failure on top of the concurrency assertion.
|
||||
}
|
||||
finally
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user