diff --git a/docs/plans/2026-08-18-followups-and-tickets.md b/docs/plans/2026-08-18-followups-and-tickets.md index dda175c..6a7bbb1 100644 --- a/docs/plans/2026-08-18-followups-and-tickets.md +++ b/docs/plans/2026-08-18-followups-and-tickets.md @@ -511,12 +511,26 @@ Notable findings made along the way: other's child nodes (MSB4166) — `-m:1` avoids it; ssh can transiently refuse with "Too many authentication failures" under agent concurrency (back off and retry). -Follow-ups recorded, not started (deliberately small): +Follow-ups recorded, not started (deliberately small) — *first two closed 2026-08-18 on +`feat/sweep-reread-tag-summary`*: - The `fca978d` sweep re-read (above) — the one genuinely ticket-worthy item. + *Closed in `f82dac1`: a mechanical pre-pass narrowed the sweep's 1,383 deletions to + 68 files / 816 residual prose lines; a judged review of every one found 17 collateral + deletions across 10 files (rationale prose with no surviving equivalent — SessionManager + metrics invariants, HubTokenService hollow-token guard, AcknowledgeAlarm routing + remarks, LmxSubtagAlarmSource unsecured-write/idempotency notes, WnWrapAlarmConsumer's + WIN-911 precedent, DashboardSessionAdminService error-boundary catches, + WorkerPipeSession factory-null throw, three test-design rationales) — all restored, + markers left stripped. Everything else flagged verified benign (`` + resolves to equal-or-richer interface docs, or the substance survives relocated). + Verified: NonWindows slnx 0W/0E, touched gateway test classes 65/65; windev worker + x86 build 0W/0E, worker suite 524/11/0.* - `DashboardGroupTagMapping`'s class-level `` still describes the lookup as "full DN first, leading-RDN fallback" — accurate mechanics, but it could point at the short-name-keys consequence the inline comment now records. + *Closed in `4b6f907`: the summary now states full-DN keys can never match and points + at the lookup comment in `MapGroupsToTags`.* - wwtools `mxa read` human-readable formatter throws `RuntimeBinderException` on a failed read (`ReadCommand.cs:137`); `--llm-json` works. Different repo, noted here so it isn't lost. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs index 84be574..7e3b750 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs @@ -6,6 +6,9 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard; /// Sibling of and deliberately follows /// the same group-matching rules (full DN first, leading-RDN fallback, /// case-insensitive) so operators write one kind of group key for both maps. +/// Because the shared LDAP provider delivers groups already stripped to short +/// RDN names, full-DN GroupToTag keys can never match — use short group +/// names as keys (see the lookup comment in ). /// Tags gate dashboard event VISIBILITY only; they are never a data-access /// constraint. /// 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.KillWorkerAsyncsession.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);