From f82dac19062f54012270a637bf0ff1a4ef87081e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 18 Aug 2026 09:33:59 -0400 Subject: [PATCH] docs(src): restore substantive rationale prose the fca978d marker sweep deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Dashboard/DashboardSessionAdminService.cs | 11 +++++++++++ .../Dashboard/HubTokenService.cs | 5 +++++ .../Grpc/MxAccessGatewayService.cs | 9 +++++++++ .../Sessions/SessionManager.cs | 14 ++++++++++++++ .../DashboardBrowseAndAlarmModelTests.cs | 3 +++ .../Dashboard/DashboardSnapshotPublisherTests.cs | 6 ++++++ .../Gateway/Sessions/SessionManagerTests.cs | 16 +++++++++++++++- .../Ipc/WorkerPipeSession.cs | 7 +++++++ .../MxAccess/LmxSubtagAlarmSource.cs | 8 ++++++++ .../MxAccess/WnWrapAlarmConsumer.cs | 4 +++- 10 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs index 595530b..b2aecbb 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs @@ -124,6 +124,11 @@ public sealed class DashboardSessionAdminService( } 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( exception, "Dashboard admin {Actor} close failed unexpectedly for session {SessionId}.", @@ -206,6 +211,12 @@ public sealed class DashboardSessionAdminService( } 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( exception, "Dashboard admin {Actor} kill failed unexpectedly for session {SessionId}.", diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs index d9b16b5..d05d09c 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs @@ -115,6 +115,11 @@ public sealed class HubTokenService 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)) { return null; diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index ced3554..4481c1d 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -185,6 +185,15 @@ public sealed class MxAccessGatewayService( } /// + /// + /// Surfaces the public AcknowledgeAlarm RPC. Acknowledgement is + /// session-less: the gateway routes it through the always-on + /// monitor session. An + /// alarm_full_reference that parses as a canonical GUID forwards + /// to AcknowledgeAlarmCommand; a Provider!Group.Tag + /// reference forwards to AcknowledgeAlarmByNameCommand; anything + /// else returns an InvalidRequest diagnostic in the reply. + /// public override async Task AcknowledgeAlarm( AcknowledgeAlarmRequest request, ServerCallContext context) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs index a58aec0..f4cc712 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs @@ -231,6 +231,10 @@ public sealed class SessionManager : ISessionManager session.MarkFaulted(exception.Message); _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(); await RemoveSessionAsync(session).ConfigureAwait(false); throw new SessionManagerException( @@ -393,6 +397,11 @@ public sealed class SessionManager : ISessionManager "Graceful shutdown failed for session {SessionId}; killing worker.", 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) && registeredSession is not null) { @@ -443,6 +452,11 @@ public sealed class SessionManager : ISessionManager session.MarkFaulted(exception.Message); 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(); } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardBrowseAndAlarmModelTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardBrowseAndAlarmModelTests.cs index 4aab95a..a4a6de2 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardBrowseAndAlarmModelTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardBrowseAndAlarmModelTests.cs @@ -171,6 +171,9 @@ public sealed class DashboardBrowseAndAlarmModelTests Assert.True(model.IsDegraded); Assert.Contains("bg-warning", model.BadgeCssClass, StringComparison.Ordinal); 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); } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotPublisherTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotPublisherTests.cs index efa49f4..02607fc 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotPublisherTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotPublisherTests.cs @@ -57,6 +57,12 @@ public sealed class DashboardSnapshotPublisherTests $"Expected at least 2 subscribe calls, got {snapshotService.SubscribeCount}."); 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; Assert.True(gap >= reconnectDelay - TimeSpan.FromMilliseconds(10), $"Expected reconnect gap >= {reconnectDelay.TotalMilliseconds}ms; got {gap.TotalMilliseconds}ms."); 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 55c2cb6..c0405df 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs @@ -686,6 +686,11 @@ public sealed class SessionManagerTests 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); @@ -743,6 +748,9 @@ public sealed class SessionManagerTests /// /// Verifies that killing a worker removes the session from the registry without calling shutdown. + /// Also pins the reason argument propagating through + /// SessionManager.KillWorkerAsync → session.KillWorker(reason) + /// → IWorkerClient.Kill(reason). /// /// A task that represents the asynchronous operation. [Fact] @@ -1419,7 +1427,13 @@ public sealed class SessionManagerTests /// Gets the number of times kill was called on the fake worker client. public int KillCount { get; private set; } - /// Gets the last reason argument observed by . + /// + /// Gets the last reason argument observed by . Pins the + /// reason-string propagation through SessionManager.KillWorkerAsync + /// → session.KillWorker(reason) → IWorkerClient.Kill(reason); + /// without this, the chain could silently drop or substitute the reason + /// argument and existing tests would still pass. + /// public string? LastKillReason { get; private set; } /// Gets the number of times dispose was called on the fake worker client. diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs index 060f841..a92fa0b 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs @@ -142,6 +142,13 @@ public sealed class WorkerPipeSession /// A task that represents the asynchronous operation. 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() ?? throw new InvalidOperationException( "Worker runtime session factory returned null."); diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/LmxSubtagAlarmSource.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/LmxSubtagAlarmSource.cs index 66bad84..2e40d9c 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/LmxSubtagAlarmSource.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/LmxSubtagAlarmSource.cs @@ -108,6 +108,10 @@ public sealed class LmxSubtagAlarmSource : ISubtagAlarmSource public event EventHandler? ValueChanged; /// + /// + /// Idempotent per address: an address already advised is skipped + /// rather than re-registered. + /// public void Advise(IReadOnlyCollection itemAddresses) { if (itemAddresses is null) @@ -140,6 +144,10 @@ public sealed class LmxSubtagAlarmSource : ISubtagAlarmSource } /// + /// + /// Writes with MXAccess user id 0 — always an unsecured Write, never + /// WriteSecured semantics. + /// public void Write(string itemAddress, object? value) { if (itemAddress is null) diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnWrapAlarmConsumer.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnWrapAlarmConsumer.cs index 55bf215..837663d 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnWrapAlarmConsumer.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnWrapAlarmConsumer.cs @@ -198,7 +198,9 @@ public sealed class WnWrapAlarmConsumer : IMxAccessAlarmConsumer // 2026-08-18) this is the only path that lets AlarmAckByName // return rc=0 afterwards. The v2 Initialize/Register/Subscribe // 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 // AcknowledgeByName below and docs/AlarmProbeFindings.md. int init = com.IwwAlarmConsumer_InitializeConsumer(DefaultApplicationName);