fix(archreview): gateway core P0 remediation (GWC-01/02/03, TST-02, TST-12)

Interlocking changes across the gateway server (shared GatewaySession.cs /
SessionManager.cs), committed together:

- GWC-01 (Critical): alarm monitor now attaches as an internal
  (non-counted) distributor subscriber instead of a second raw drain of the
  single worker event channel; WorkerClient._events -> SingleReader with a
  claimed-once guard so a future dual-consumer regression throws loudly.
- GWC-02 (High): faulted sessions are swept in CloseExpiredLeasesAsync
  (IsFaultedReapable + FaultedReason); new FaultedGraceSeconds (default 0).
- GWC-03 (High): configurable MaxSparseArrayLength (default 1_000_000)
  enforced before allocation.
- TST-02 (High, security): StreamEvents attach now enforces the opening key
  id -> PermissionDenied on owner mismatch.
- TST-12 (Medium): CLAUDE.md retention-defaults sentence corrected.

Verified: NonWindows build clean; targeted tests 135/135 on macOS, plus
WorkerClientTests 18/18 on the Windows host.
This commit is contained in:
Joseph Doherty
2026-07-09 05:51:57 -04:00
parent 31eec41456
commit 20392cf246
30 changed files with 632 additions and 48 deletions
@@ -225,11 +225,14 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic
Task reconcileLoop = ReconcileLoopAsync(session.SessionId, linked.Token);
try
{
await foreach (WorkerEvent workerEvent in _sessionManager
.ReadEventsAsync(session.SessionId, linked.Token)
// Consume mapped MxEvents through the session's single distributor pump (as an
// internal, non-counted subscriber) rather than opening a second raw drain of the
// worker event channel — a second drain would split events with the dashboard
// mirror pump and silently lose Acknowledge/mode-change transitions (GWC-01).
await foreach (MxEvent mxEvent in _sessionManager
.ReadAlarmEventsAsync(session.SessionId, linked.Token)
.ConfigureAwait(false))
{
MxEvent? mxEvent = workerEvent.Event;
if (mxEvent is { BodyCase: MxEvent.BodyOneofCase.OnAlarmTransition }
&& mxEvent.OnAlarmTransition is not null)
{
@@ -27,4 +27,14 @@ public sealed class EventOptions
/// bounds the buffer).
/// </summary>
public double ReplayRetentionSeconds { get; init; } = 300;
/// <summary>
/// Gets the maximum <c>total_length</c> a sparse-array write may declare before the
/// gateway rejects it with <c>InvalidArgument</c>, enforced in
/// <see cref="Sessions.SparseArrayExpander"/> before the full array is materialized.
/// Guards against a single write forcing a multi-GB allocation. The default of
/// 1,000,000 elements is far above any realistic MXAccess array write yet well below
/// the frame-size ceiling.
/// </summary>
public int MaxSparseArrayLength { get; init; } = 1_000_000;
}
@@ -181,6 +181,10 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase<GatewayOption
options.DetachGraceSeconds,
"MxGateway:Sessions:DetachGraceSeconds must be zero or greater (0 disables detach-grace retention).",
builder);
AddIfNegative(
options.FaultedGraceSeconds,
"MxGateway:Sessions:FaultedGraceSeconds must be zero or greater (0 reaps a faulted session on the next sweep).",
builder);
AddIfNegative(
options.WorkerReadyWaitTimeoutMs,
"MxGateway:Sessions:WorkerReadyWaitTimeoutMs must be greater than or equal to zero.",
@@ -214,6 +218,10 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase<GatewayOption
builder.RequireThat(
options.ReplayRetentionSeconds >= 0,
"MxGateway:Events:ReplayRetentionSeconds must be greater than or equal to zero.");
builder.RequireThat(
options.MaxSparseArrayLength >= 1 && options.MaxSparseArrayLength <= Array.MaxLength,
$"MxGateway:Events:MaxSparseArrayLength must be between 1 and {Array.MaxLength}.");
}
private static void ValidateDashboard(DashboardOptions options, ValidationBuilder builder)
@@ -45,6 +45,18 @@ public sealed class SessionOptions
/// </remarks>
public int DetachGraceSeconds { get; init; } = 30;
/// <summary>
/// Gets the grace period, in seconds, that a faulted session is retained before the
/// lease monitor reaps it (killing its worker and freeing the session slot). A faulted
/// session is otherwise permanently unusable — every command fails the readiness check —
/// yet without this sweep it pins a session slot and a live x86 worker until its normal
/// lease expires (up to <see cref="DefaultLeaseSeconds"/>). A value of <c>0</c> (the
/// default) reaps a faulted session on the next sweep cycle, bounding the blast radius; a
/// positive value keeps the faulted session observable via <c>GetSessionStatus</c> for the
/// grace window before it is reclaimed. Must be greater than or equal to zero.
/// </summary>
public int FaultedGraceSeconds { get; init; }
/// <summary>
/// Gets a value indicating whether multiple event subscribers are allowed per session.
/// </summary>
@@ -48,6 +48,7 @@ public sealed class EventStreamService(
/// </remarks>
public async IAsyncEnumerable<MxEvent> StreamEventsAsync(
StreamEventsRequest request,
string? callerKeyId,
[EnumeratorCancellation] CancellationToken cancellationToken)
{
if (!sessionManager.TryGetSession(request.SessionId, out GatewaySession? session) || session is null)
@@ -57,6 +58,19 @@ public sealed class EventStreamService(
$"Session {request.SessionId} was not found.");
}
// Owner-scoped attach (TST-02, security control): a session's event stream may be
// attached or reattached ONLY by the API key that opened the session. The detach-grace
// and fan-out retention windows are on by default, so without this check any event-scoped
// key that learns a session id could attach to another key's retained session and receive
// its replayed and live data. Ordinal comparison; null owner (session opened with no auth)
// matches only a null caller key.
if (!string.Equals(session.OwnerKeyId, callerKeyId, StringComparison.Ordinal))
{
throw new SessionManagerException(
SessionManagerErrorCode.PermissionDenied,
$"Session {request.SessionId} is owned by a different API key; event-stream attach is owner-scoped.");
}
// No `using` here — subscriber.Dispose() is called exactly once in the finally
// block below, which also disposes the reader. A `using` declaration would add a
// second Dispose on the same path and double-decrement the session subscriber count.
@@ -11,9 +11,16 @@ public interface IEventStreamService
/// Streams events for the specified session to the caller.
/// </summary>
/// <param name="request">Request payload.</param>
/// <param name="callerKeyId">
/// The API key id of the calling client, used to enforce that only the key that opened a
/// session may attach or reattach its event stream. <see langword="null"/> when the call
/// is unauthenticated (e.g. auth disabled), which only matches a session opened with no
/// owner key.
/// </param>
/// <param name="cancellationToken">Token to cancel the asynchronous operation.</param>
/// <returns>The events emitted for the requested session.</returns>
IAsyncEnumerable<MxEvent> StreamEventsAsync(
StreamEventsRequest request,
string? callerKeyId,
CancellationToken cancellationToken);
}
@@ -148,7 +148,7 @@ public sealed class MxAccessGatewayService(
{
requestValidator.ValidateStreamEvents(request);
await foreach (MxEvent publicEvent in eventStreamService
.StreamEventsAsync(request, context.CancellationToken)
.StreamEventsAsync(request, identityAccessor.Current?.KeyId, context.CancellationToken)
.WithCancellation(context.CancellationToken)
.ConfigureAwait(false))
{
@@ -931,6 +931,7 @@ public sealed class MxAccessGatewayService(
SessionManagerErrorCode.SessionLimitExceeded => StatusCode.ResourceExhausted,
SessionManagerErrorCode.OpenFailed => StatusCode.Unavailable,
SessionManagerErrorCode.CloseFailed => StatusCode.Unavailable,
SessionManagerErrorCode.PermissionDenied => StatusCode.PermissionDenied,
_ => StatusCode.Unavailable,
};
@@ -23,8 +23,10 @@ public sealed class GatewaySession
private bool _closeStarted;
private int _activeEventSubscriberCount;
private readonly TimeSpan _detachGrace;
private readonly TimeSpan _faultedGrace;
private readonly TimeSpan _workerReadyWaitTimeout;
private DateTimeOffset? _detachedAtUtc;
private DateTimeOffset? _faultedAtUtc;
// True once at least one external subscriber attached SUCCESSFULLY. Detach-grace's
// "last subscriber dropped" stamp (see DetachEventSubscriber) is gated on this so a
// FAILED first attach — which still runs the rollback DetachEventSubscriber from the
@@ -139,6 +141,14 @@ public sealed class GatewaySession
/// When <see langword="null"/> (legacy unit-construction paths that do not exercise Galaxy
/// metadata), addresses pass through unchanged.
/// </param>
/// <param name="faultedGrace">
/// Grace window kept after the session faults before the lease monitor reaps it. When the
/// window is positive the faulted session stays observable via <c>GetSessionStatus</c> for
/// that long before it is reclaimed; <see cref="TimeSpan.Zero"/> (the default) makes the
/// session reapable on the next sweep. The fault timestamp is stamped in
/// <see cref="MarkFaulted"/> using <paramref name="eventStreaming"/>'s clock so the timer
/// is unit-testable.
/// </param>
public GatewaySession(
string sessionId,
string backendName,
@@ -156,7 +166,8 @@ public sealed class GatewaySession
SessionEventStreaming? eventStreaming = null,
TimeSpan detachGrace = default,
TimeSpan workerReadyWaitTimeout = default,
ArrayAddressNormalizer? addressNormalizer = null)
ArrayAddressNormalizer? addressNormalizer = null,
TimeSpan faultedGrace = default)
{
if (string.IsNullOrWhiteSpace(sessionId))
{
@@ -195,6 +206,7 @@ public sealed class GatewaySession
_leaseExpiresAt = openedAt + leaseDuration;
_eventStreaming = eventStreaming ?? SessionEventStreaming.Default;
_detachGrace = detachGrace > TimeSpan.Zero ? detachGrace : TimeSpan.Zero;
_faultedGrace = faultedGrace > TimeSpan.Zero ? faultedGrace : TimeSpan.Zero;
_workerReadyWaitTimeout = workerReadyWaitTimeout > TimeSpan.Zero ? workerReadyWaitTimeout : TimeSpan.Zero;
_addressNormalizer = addressNormalizer;
}
@@ -522,6 +534,33 @@ public sealed class GatewaySession
}
}
/// <summary>
/// Registers a gateway-owned <em>internal</em> (non-counted) distributor subscriber and
/// returns its lease. The lease's <see cref="IEventSubscriberLease.Reader"/> yields the
/// same mapped <see cref="MxEvent"/>s the single distributor pump fans to every
/// subscriber; disposing the lease unregisters it.
/// </summary>
/// <remarks>
/// Used by the central alarm monitor so it consumes events through the one distributor
/// pump instead of opening a second raw drain of the single worker event channel (which
/// would split events between the two readers). Mirrors the dashboard-mirror lease:
/// <c>isInternal: true</c> keeps this subscriber out of the
/// <c>MaxEventSubscribersPerSession</c> accounting and out of the single-subscriber
/// overflow-fault path, so a slow alarm reconcile can never fault the session — it only
/// disconnects this internal subscriber.
/// </remarks>
/// <returns>The internal subscriber's lease; dispose it to unregister.</returns>
public IEventSubscriberLease AttachInternalEventSubscriber()
{
// Same sequence StartDashboardMirror uses: create the distributor (claiming the pump
// start if we are first), register the internal subscriber BEFORE the pump starts so a
// subscriber is always present at pump start, then start the pump if requested.
SessionEventDistributor distributor = EnsureDistributorCreated(out bool startNow);
IEventSubscriberLease lease = distributor.Register(isInternal: true);
StartPumpIfRequested(distributor, startNow);
return lease;
}
private static void StartPumpIfRequested(SessionEventDistributor distributor, bool startNow)
{
if (!startNow)
@@ -724,6 +763,11 @@ public sealed class GatewaySession
_finalFault = reason;
_state = SessionState.Faulted;
// Stamp the fault time once, on the first fault, so the sweeper can apply
// FaultedGraceSeconds. A subsequent MarkFaulted (already-faulted session) keeps the
// original timestamp so the grace window is measured from the first fault.
_faultedAtUtc ??= _eventStreaming.TimeProvider.GetUtcNow();
}
}
@@ -790,6 +834,24 @@ public sealed class GatewaySession
}
}
/// <summary>
/// Determines whether a faulted session is now eligible for reaping by the lease monitor.
/// A faulted session is permanently unusable (every command fails the readiness check),
/// so the sweeper closes it exactly as it closes an expired lease — but no sooner than the
/// configured <c>FaultedGraceSeconds</c> after the fault, so a monitoring client can still
/// observe the fault before the slot is reclaimed. Always returns <see langword="false"/>
/// for a non-faulted session.
/// </summary>
/// <param name="now">Current timestamp for comparison.</param>
/// <returns><see langword="true"/> if the session is faulted and past its fault-grace window; otherwise <see langword="false"/>.</returns>
public bool IsFaultedReapable(DateTimeOffset now)
{
lock (_syncRoot)
{
return IsFaultedReapableCore(now);
}
}
/// <summary>
/// Attaches an event subscriber and returns a lease whose
/// <see cref="IEventSubscriberLease.Reader"/> reads the fanned public
@@ -1053,12 +1115,13 @@ public sealed class GatewaySession
_addressNormalizer?.Normalize(address) ?? address;
// MXAccess writes replace the whole array; expand a sparse value in place so the worker only
// ever receives a whole-array MxValue. No-op for null or non-sparse values.
private static void ExpandValue(MxValue? value)
// ever receives a whole-array MxValue. No-op for null or non-sparse values. The configured
// MxGateway:Events:MaxSparseArrayLength cap is enforced before the full array is allocated.
private void ExpandValue(MxValue? value)
{
if (value is not null)
{
SparseArrayExpander.Expand(value);
SparseArrayExpander.Expand(value, _eventStreaming.EventOptions.MaxSparseArrayLength);
}
}
@@ -1571,7 +1634,7 @@ public sealed class GatewaySession
// Re-verify eligibility atomically. If a subscriber reattached between the sweep's
// eligibility check and this point, neither condition holds and we decline.
bool eligible = IsLeaseExpiredCore(now) || IsDetachGraceExpiredCore(now);
bool eligible = IsLeaseExpiredCore(now) || IsFaultedReapableCore(now) || IsDetachGraceExpiredCore(now);
if (!eligible)
{
alreadyClosing = false;
@@ -1597,6 +1660,12 @@ public sealed class GatewaySession
&& _detachedAtUtc is not null
&& now - _detachedAtUtc.Value >= _detachGrace;
private bool IsFaultedReapableCore(DateTimeOffset now)
=> _state is SessionState.Faulted
&& (_faultedGrace <= TimeSpan.Zero
|| _faultedAtUtc is null
|| now - _faultedAtUtc.Value >= _faultedGrace);
// Final terminal transition; under _syncRoot to keep _state writes single-lock.
// Closed is unconditionally terminal — TransitionTo refuses to overwrite it —
// so we don't need to re-check the precondition here.
@@ -43,6 +43,18 @@ public interface ISessionManager
string sessionId,
CancellationToken cancellationToken);
/// <summary>
/// Reads mapped events for the central alarm monitor by attaching an internal
/// (non-counted) distributor subscriber, so the alarm feed shares the one worker-event
/// pump instead of opening a second raw drain of the single worker event channel.
/// </summary>
/// <param name="sessionId">Identifier of the session.</param>
/// <param name="cancellationToken">Token to cancel the asynchronous operation.</param>
/// <returns>The mapped <see cref="MxEvent"/>s fanned by the session's distributor.</returns>
IAsyncEnumerable<MxEvent> ReadAlarmEventsAsync(
string sessionId,
CancellationToken cancellationToken);
/// <summary>Closes a session and terminates its worker process.</summary>
/// <param name="sessionId">Identifier of the session to close.</param>
/// <param name="cancellationToken">Token to cancel the asynchronous operation.</param>
@@ -1,4 +1,5 @@
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Security.Cryptography;
using Google.Protobuf.WellKnownTypes;
using Microsoft.Extensions.Logging;
@@ -18,6 +19,7 @@ public sealed class SessionManager : ISessionManager
public const string GatewayShutdownReason = "gateway-shutdown";
public const string LeaseExpiredReason = "lease-expired";
public const string DetachGraceExpiredReason = "detach-grace-expired";
public const string FaultedReason = "faulted-reaped";
private readonly ISessionRegistry _registry;
private readonly ISessionWorkerClientFactory _workerClientFactory;
@@ -189,6 +191,22 @@ public sealed class SessionManager : ISessionManager
return session.ReadEventsAsync(cancellationToken);
}
/// <inheritdoc />
public async IAsyncEnumerable<MxEvent> ReadAlarmEventsAsync(
string sessionId,
[EnumeratorCancellation] CancellationToken cancellationToken)
{
GatewaySession session = GetRequiredSession(sessionId);
using IEventSubscriberLease lease = session.AttachInternalEventSubscriber();
await foreach (MxEvent mxEvent in lease.Reader
.ReadAllAsync(cancellationToken)
.ConfigureAwait(false))
{
yield return mxEvent;
}
}
/// <inheritdoc />
public async Task<SessionCloseResult> CloseSessionAsync(
string sessionId,
@@ -259,22 +277,25 @@ public sealed class SessionManager : ISessionManager
int closedCount = 0;
foreach (GatewaySession session in _registry.Snapshot())
{
// A session is swept when its normal lease has expired OR its detach-grace
// retention window has elapsed (last external subscriber dropped and no client
// reconnected within DetachGraceSeconds). The detach-grace close is the same
// teardown as a lease-expiry close; only the reason differs so operators can tell
// a short reconnect-window expiry from a long idle-lease expiry in logs/metrics.
// Lease-expiry takes PRECEDENCE over detach-grace when both conditions fire
// simultaneously (reason will be lease-expired, not detach-grace-expired).
// A session is swept when its normal lease has expired, it has FAULTED (a faulted
// session is permanently unusable yet otherwise pins a session slot and a live x86
// worker until its DefaultLeaseSeconds lease expires), OR its detach-grace retention
// window has elapsed (last external subscriber dropped and no client reconnected
// within DetachGraceSeconds). All three are the same teardown; only the reason differs
// so operators can tell a fault reap from a short reconnect-window expiry from a long
// idle-lease expiry in logs/metrics. Precedence when several fire simultaneously:
// lease-expired, then faulted, then detach-grace.
//
// TOCTOU note: eligibility is re-verified atomically inside TryBeginCloseIfExpired
// under _syncRoot, so a client that reattaches a subscriber between the check above
// and the close call wins the race and the session is left open and usable.
string? reason = session.IsLeaseExpired(now)
? LeaseExpiredReason
: session.IsDetachGraceExpired(now)
? DetachGraceExpiredReason
: null;
: session.IsFaultedReapable(now)
? FaultedReason
: session.IsDetachGraceExpired(now)
? DetachGraceExpiredReason
: null;
if (reason is null)
{
continue;
@@ -457,7 +478,8 @@ public sealed class SessionManager : ISessionManager
eventStreaming,
TimeSpan.FromSeconds(Math.Max(0, _options.Sessions.DetachGraceSeconds)),
TimeSpan.FromMilliseconds(Math.Max(0, _options.Sessions.WorkerReadyWaitTimeoutMs)),
_addressNormalizer);
_addressNormalizer,
TimeSpan.FromSeconds(Math.Max(0, _options.Sessions.FaultedGraceSeconds)));
}
private static string CreateClientCorrelationId(
@@ -10,4 +10,11 @@ public enum SessionManagerErrorCode
SessionLimitExceeded,
OpenFailed,
CloseFailed,
/// <summary>
/// The caller is not permitted to perform the operation on this session — for example,
/// attaching a <c>StreamEvents</c> stream to a session opened by a different API key.
/// Maps to gRPC <c>PermissionDenied</c>.
/// </summary>
PermissionDenied,
}
@@ -33,13 +33,20 @@ internal static class SparseArrayExpander
/// a sparse array this is a no-op, so callers may invoke it unconditionally.
/// </summary>
/// <param name="value">The value to expand in place.</param>
/// <param name="maxSparseArrayLength">
/// The maximum <c>total_length</c> the sparse array may declare before the write is
/// rejected, enforced before the full array is allocated (see
/// <c>MxGateway:Events:MaxSparseArrayLength</c>). Defaults to <see cref="int.MaxValue"/>
/// so the <see cref="Array.MaxLength"/> backstop is the only bound in test/unit-construction
/// paths that do not thread the configured cap.
/// </param>
/// <exception cref="RpcException">
/// <see cref="StatusCode.InvalidArgument"/> when the sparse payload is invalid: zero
/// total length, an index at or beyond the total length, a duplicate index, an
/// unsupported element type, or an element value whose kind does not match the declared
/// element type.
/// total length, a total length exceeding <paramref name="maxSparseArrayLength"/>, an index
/// at or beyond the total length, a duplicate index, an unsupported element type, or an
/// element value whose kind does not match the declared element type.
/// </exception>
public static void Expand(MxValue value)
public static void Expand(MxValue value, int maxSparseArrayLength = int.MaxValue)
{
ArgumentNullException.ThrowIfNull(value);
@@ -62,6 +69,12 @@ internal static class SparseArrayExpander
throw Invalid($"Sparse array element_data_type '{elementType}' is not a supported scalar element type.");
}
if (totalLength > (uint)maxSparseArrayLength)
{
throw Invalid(
$"Sparse array total_length {totalLength} exceeds the configured maximum {maxSparseArrayLength} (MxGateway:Events:MaxSparseArrayLength).");
}
if (totalLength > (uint)Array.MaxLength)
{
throw Invalid(
@@ -32,6 +32,7 @@ public sealed class WorkerClient : IWorkerClient
private DateTimeOffset _lastHeartbeatAt;
private int? _processId;
private int _eventQueueDepth;
private int _eventsReaderClaimed;
private Task? _readLoopTask;
private Task? _writeLoopTask;
private Task? _heartbeatLoopTask;
@@ -70,7 +71,13 @@ public sealed class WorkerClient : IWorkerClient
_events = Channel.CreateBounded<WorkerEvent>(
new BoundedChannelOptions(_options.EventChannelCapacity)
{
SingleReader = false,
// The worker event channel has exactly ONE consumer: the per-session
// SessionEventDistributor pump. The alarm monitor and dashboard mirror both
// attach to the distributor rather than draining this channel directly, so a
// second concurrent reader would silently split events between the two
// enumerators. SingleReader=true asserts that invariant; ReadEventsAsync adds a
// claimed-once guard so a regression fails loudly instead of losing events.
SingleReader = true,
SingleWriter = true,
FullMode = BoundedChannelFullMode.Wait,
AllowSynchronousContinuations = false,
@@ -224,7 +231,24 @@ public sealed class WorkerClient : IWorkerClient
}
/// <inheritdoc />
public async IAsyncEnumerable<WorkerEvent> ReadEventsAsync(
public IAsyncEnumerable<WorkerEvent> ReadEventsAsync(CancellationToken cancellationToken)
{
// The event channel is SingleReader: only one enumerator may ever drain it, otherwise
// the two readers would each receive a random subset of events. Claim the reader at CALL
// time (not lazily on first MoveNext) and fail loudly on a second consumer rather than
// silently splitting the stream (see GWC-01). The distributor pump is the only intended
// caller; the alarm monitor and dashboard mirror attach to the distributor instead.
if (Interlocked.CompareExchange(ref _eventsReaderClaimed, 1, 0) != 0)
{
throw new InvalidOperationException(
"WorkerClient.ReadEventsAsync was already claimed by another consumer. The worker event "
+ "channel is single-reader; attach to the SessionEventDistributor instead of draining it twice.");
}
return ReadEventsCoreAsync(cancellationToken);
}
private async IAsyncEnumerable<WorkerEvent> ReadEventsCoreAsync(
[EnumeratorCancellation] CancellationToken cancellationToken)
{
await foreach (WorkerEvent workerEvent in _events.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false))