docs(src): restore substantive rationale prose the fca978d marker sweep deleted
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Failing after 37s
ci / java (push) Successful in 2m11s
ci / portable (push) Successful in 19m36s

Targeted re-read of the 203-file fca978d sweep (docs(src): add missing
XML docs and strip tracking-ID comments): a mechanical pre-pass narrowed
1,383 deletions to 68 files / 816 residual prose lines, and a judged
review of every one found 17 collateral deletions across 10 files —
rationale prose deleted alongside resolved markers with no equivalent
surviving anywhere in the tree. Restored (markers stay stripped, per the
sweep's intent):

- SessionManager: the three metrics-accounting invariants (kill-path
  gauge decrement safety, shutdown kill-fallback registry guard vs
  double bookkeeping, SessionClosed-not-SessionRemoved on failed close)
- SessionManagerTests: the matching accounting expectation note and the
  reason-string propagation pins (test summary + FakeWorkerClient.LastKillReason)
- MxAccessGatewayService.AcknowledgeAlarm: the routing remarks (GUID vs
  Provider!Group.Tag vs InvalidRequest; session-less via IGatewayAlarmService)
  — inheritdoc resolves to nothing (proto-generated base is undocumented)
- HubTokenService.Validate: why the hollow-token guard exists
  (non-empty AuthenticationType alone satisfies IsAuthenticated)
- DashboardSessionAdminService: why both broad catches exist (keep raw
  teardown exceptions out of Blazor's error boundary), Close + Kill paths
- WorkerPipeSession.RunAsync: why the factory result throws instead of
  NREing (unambiguous failure; finally-block Dispose can't no-op)
- LmxSubtagAlarmSource: Advise idempotency; Write is always unsecured
  (user id 0), never WriteSecured semantics
- WnWrapAlarmConsumer: the v1-prefix path is what WIN-911-style code uses
- DashboardSnapshotPublisherTests: what the 10ms slack absorbs
  (Task.Delay's coarse Windows timer quantum)
- DashboardBrowseAndAlarmModelTests: why the label text is pinned, not
  just the CSS class

Everything else flagged verified benign: inheritdoc replacements resolve
to equal-or-richer interface docs, or the substance survives relocated.
NonWindows slnx 0W/0E; touched gateway test classes 65/65.
This commit is contained in:
Joseph Doherty
2026-08-18 09:33:59 -04:00
parent 4b6f9077bf
commit f82dac1906
10 changed files with 81 additions and 2 deletions
@@ -124,6 +124,11 @@ public sealed class DashboardSessionAdminService(
} }
catch (Exception exception) catch (Exception exception)
{ {
// Any non-SessionManagerException (e.g. an IOException or
// InvalidOperationException from the session DisposeAsync / pipe
// teardown path) would otherwise propagate raw into Blazor's error
// boundary. Convert it to a friendly failure so the Razor pages see
// only DashboardSessionAdminResult.
_logger.LogWarning( _logger.LogWarning(
exception, exception,
"Dashboard admin {Actor} close failed unexpectedly for session {SessionId}.", "Dashboard admin {Actor} close failed unexpectedly for session {SessionId}.",
@@ -206,6 +211,12 @@ public sealed class DashboardSessionAdminService(
} }
catch (Exception exception) catch (Exception exception)
{ {
// Any non-SessionManagerException (e.g. an IOException from worker
// pipe teardown surfacing through session.DisposeAsync, or an
// InvalidOperationException from a corrupted worker handle) would
// otherwise propagate raw into Blazor's error boundary. Convert it
// to a friendly failure so the page renders the ResultMessage
// rather than the circuit error page.
_logger.LogWarning( _logger.LogWarning(
exception, exception,
"Dashboard admin {Actor} kill failed unexpectedly for session {SessionId}.", "Dashboard admin {Actor} kill failed unexpectedly for session {SessionId}.",
@@ -115,6 +115,11 @@ public sealed class HubTokenService
return null; return null;
} }
// Reject a token whose payload carries no caller identity. A
// null/empty Name AND NameIdentifier would otherwise produce a
// principal that satisfies IsAuthenticated and IsInRole checks
// without any associated user, because the AuthenticationType
// (the HubToken scheme) is non-empty.
if (string.IsNullOrEmpty(payload.Name) && string.IsNullOrEmpty(payload.NameIdentifier)) if (string.IsNullOrEmpty(payload.Name) && string.IsNullOrEmpty(payload.NameIdentifier))
{ {
return null; return null;
@@ -185,6 +185,15 @@ public sealed class MxAccessGatewayService(
} }
/// <inheritdoc /> /// <inheritdoc />
/// <remarks>
/// Surfaces the public AcknowledgeAlarm RPC. Acknowledgement is
/// session-less: the gateway routes it through the always-on
/// <see cref="IGatewayAlarmService"/> monitor session. An
/// <c>alarm_full_reference</c> that parses as a canonical GUID forwards
/// to <c>AcknowledgeAlarmCommand</c>; a <c>Provider!Group.Tag</c>
/// reference forwards to <c>AcknowledgeAlarmByNameCommand</c>; anything
/// else returns an <c>InvalidRequest</c> diagnostic in the reply.
/// </remarks>
public override async Task<AcknowledgeAlarmReply> AcknowledgeAlarm( public override async Task<AcknowledgeAlarmReply> AcknowledgeAlarm(
AcknowledgeAlarmRequest request, AcknowledgeAlarmRequest request,
ServerCallContext context) ServerCallContext context)
@@ -231,6 +231,10 @@ public sealed class SessionManager : ISessionManager
session.MarkFaulted(exception.Message); session.MarkFaulted(exception.Message);
_metrics.Fault(SessionManagerErrorCode.CloseFailed.ToString()); _metrics.Fault(SessionManagerErrorCode.CloseFailed.ToString());
// The open-session gauge was incremented in OpenSessionAsync; every
// session reaching KillWorkerAsync had SessionOpened recorded. If the
// kill path throws, decrement the gauge here so mxgateway.sessions.open
// does not leak — mirroring the equivalent guard in OpenSessionAsync.
_metrics.SessionRemoved(); _metrics.SessionRemoved();
await RemoveSessionAsync(session).ConfigureAwait(false); await RemoveSessionAsync(session).ConfigureAwait(false);
throw new SessionManagerException( throw new SessionManagerException(
@@ -393,6 +397,11 @@ public sealed class SessionManager : ISessionManager
"Graceful shutdown failed for session {SessionId}; killing worker.", "Graceful shutdown failed for session {SessionId}; killing worker.",
session.SessionId); session.SessionId);
// Defensive fallback: CloseSessionCoreAsync's inner
// SessionCloseStartedException catch normally removes the session
// and accounts the close. This outer fallback only fires for
// sessions still in the registry — route through KillWorkerAsync
// so the bookkeeping is identical to the dashboard kill path.
if (_registry.TryGet(session.SessionId, out GatewaySession? registeredSession) if (_registry.TryGet(session.SessionId, out GatewaySession? registeredSession)
&& registeredSession is not null) && registeredSession is not null)
{ {
@@ -443,6 +452,11 @@ public sealed class SessionManager : ISessionManager
session.MarkFaulted(exception.Message); session.MarkFaulted(exception.Message);
if (!wasClosed) if (!wasClosed)
{ {
// Account the close as a SessionClosed (decrements the open-session
// gauge AND increments the sessions.closed counter), not just
// SessionRemoved. The session is being removed from the registry
// below; treating this as a half-finished close that only
// decremented the gauge would under-count the closed counter.
_metrics.SessionClosed(); _metrics.SessionClosed();
} }
@@ -171,6 +171,9 @@ public sealed class DashboardBrowseAndAlarmModelTests
Assert.True(model.IsDegraded); Assert.True(model.IsDegraded);
Assert.Contains("bg-warning", model.BadgeCssClass, StringComparison.Ordinal); Assert.Contains("bg-warning", model.BadgeCssClass, StringComparison.Ordinal);
Assert.Equal("x", model.Reason); Assert.Equal("x", model.Reason);
// Pin the amber label text, not just the CSS class — a label swap
// would otherwise pass this test.
Assert.Equal(DashboardAlarmProviderStatus.DegradedLabel, model.Label); Assert.Equal(DashboardAlarmProviderStatus.DegradedLabel, model.Label);
} }
@@ -57,6 +57,12 @@ public sealed class DashboardSnapshotPublisherTests
$"Expected at least 2 subscribe calls, got {snapshotService.SubscribeCount}."); $"Expected at least 2 subscribe calls, got {snapshotService.SubscribeCount}.");
Assert.True(hubContext.SendCount >= 1); Assert.True(hubContext.SendCount >= 1);
// The gap is measured from the moment the first subscribe actually
// threw (inside the fake) to the moment the second subscribe began
// (also inside the fake). This isolates the publisher's
// Task.Delay(reconnectDelay) — no StartAsync / scheduling overhead in
// the baseline. The 10ms slack absorbs Task.Delay's coarse Windows
// timer quantum (~15ms) when the underlying scheduler wakes early.
TimeSpan gap = secondSubscribeAt - firstThrowAt; TimeSpan gap = secondSubscribeAt - firstThrowAt;
Assert.True(gap >= reconnectDelay - TimeSpan.FromMilliseconds(10), Assert.True(gap >= reconnectDelay - TimeSpan.FromMilliseconds(10),
$"Expected reconnect gap >= {reconnectDelay.TotalMilliseconds}ms; got {gap.TotalMilliseconds}ms."); $"Expected reconnect gap >= {reconnectDelay.TotalMilliseconds}ms; got {gap.TotalMilliseconds}ms.");
@@ -686,6 +686,11 @@ public sealed class SessionManagerTests
Assert.Equal(1, failingWorkerClient.KillCount); Assert.Equal(1, failingWorkerClient.KillCount);
Assert.Equal(1, failingWorkerClient.DisposeCount); Assert.Equal(1, failingWorkerClient.DisposeCount);
GatewayMetricsSnapshot snapshot = metrics.GetSnapshot(); 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.Equal(1, snapshot.SessionsClosed);
Assert.False(snapshot.EventsBySession.ContainsKey(firstSession.SessionId)); Assert.False(snapshot.EventsBySession.ContainsKey(firstSession.SessionId));
Assert.Equal(1, snapshot.OpenSessions); Assert.Equal(1, snapshot.OpenSessions);
@@ -743,6 +748,9 @@ public sealed class SessionManagerTests
/// <summary> /// <summary>
/// Verifies that killing a worker removes the session from the registry without calling shutdown. /// Verifies that killing a worker removes the session from the registry without calling shutdown.
/// Also pins the <c>reason</c> argument propagating through
/// <c>SessionManager.KillWorkerAsync</c> → <c>session.KillWorker(reason)</c>
/// → <c>IWorkerClient.Kill(reason)</c>.
/// </summary> /// </summary>
/// <returns>A task that represents the asynchronous operation.</returns> /// <returns>A task that represents the asynchronous operation.</returns>
[Fact] [Fact]
@@ -1419,7 +1427,13 @@ public sealed class SessionManagerTests
/// <summary>Gets the number of times kill was called on the fake worker client.</summary> /// <summary>Gets the number of times kill was called on the fake worker client.</summary>
public int KillCount { get; private set; } public int KillCount { get; private set; }
/// <summary>Gets the last reason argument observed by <see cref="Kill"/>.</summary> /// <summary>
/// Gets the last reason argument observed by <see cref="Kill"/>. Pins the
/// reason-string propagation through <c>SessionManager.KillWorkerAsync</c>
/// → <c>session.KillWorker(reason)</c> → <c>IWorkerClient.Kill(reason)</c>;
/// without this, the chain could silently drop or substitute the reason
/// argument and existing tests would still pass.
/// </summary>
public string? LastKillReason { get; private set; } public string? LastKillReason { get; private set; }
/// <summary>Gets the number of times dispose was called on the fake worker client.</summary> /// <summary>Gets the number of times dispose was called on the fake worker client.</summary>
@@ -142,6 +142,13 @@ public sealed class WorkerPipeSession
/// <returns>A task that represents the asynchronous operation.</returns> /// <returns>A task that represents the asynchronous operation.</returns>
public async Task RunAsync(CancellationToken cancellationToken = default) public async Task RunAsync(CancellationToken cancellationToken = default)
{ {
// The factory delegate itself is null-checked in the constructor, but its
// return value is not — a factory that returned null would NRE on the
// StartAsync lambda below. Throw a diagnostic exception instead so the
// failure is unambiguous (and so the finally block's
// _runtimeSession?.Dispose() can't silently no-op on a torn
// half-initialized session). Mirrors the same pattern
// AlarmCommandHandler.Subscribe uses for its consumerFactory().
_runtimeSession = _runtimeSessionFactory() _runtimeSession = _runtimeSessionFactory()
?? throw new InvalidOperationException( ?? throw new InvalidOperationException(
"Worker runtime session factory returned null."); "Worker runtime session factory returned null.");
@@ -108,6 +108,10 @@ public sealed class LmxSubtagAlarmSource : ISubtagAlarmSource
public event EventHandler<SubtagValueChange>? ValueChanged; public event EventHandler<SubtagValueChange>? ValueChanged;
/// <inheritdoc /> /// <inheritdoc />
/// <remarks>
/// Idempotent per address: an address already advised is skipped
/// rather than re-registered.
/// </remarks>
public void Advise(IReadOnlyCollection<string> itemAddresses) public void Advise(IReadOnlyCollection<string> itemAddresses)
{ {
if (itemAddresses is null) if (itemAddresses is null)
@@ -140,6 +144,10 @@ public sealed class LmxSubtagAlarmSource : ISubtagAlarmSource
} }
/// <inheritdoc /> /// <inheritdoc />
/// <remarks>
/// Writes with MXAccess user id 0 — always an unsecured Write, never
/// WriteSecured semantics.
/// </remarks>
public void Write(string itemAddress, object? value) public void Write(string itemAddress, object? value)
{ {
if (itemAddress is null) if (itemAddress is null)
@@ -198,7 +198,9 @@ public sealed class WnWrapAlarmConsumer : IMxAccessAlarmConsumer
// 2026-08-18) this is the only path that lets AlarmAckByName // 2026-08-18) this is the only path that lets AlarmAckByName
// return rc=0 afterwards. The v2 Initialize/Register/Subscribe // return rc=0 afterwards. The v2 Initialize/Register/Subscribe
// methods on the class succeed (return 0) but acks against that // methods on the class succeed (return 0) but acks against that
// consumer state return -55. Note rc=0 means the call was // consumer state return -55. The v1 prefix path is what
// WIN-911-style code uses against the same wnwrap library.
// Note rc=0 means the call was
// accepted, not that an acknowledgement was applied — see // accepted, not that an acknowledgement was applied — see
// AcknowledgeByName below and docs/AlarmProbeFindings.md. // AcknowledgeByName below and docs/AlarmProbeFindings.md.
int init = com.IwwAlarmConsumer_InitializeConsumer(DefaultApplicationName); int init = com.IwwAlarmConsumer_InitializeConsumer(DefaultApplicationName);