diff --git a/clients/go/generate-proto.ps1 b/clients/go/generate-proto.ps1 index 85489b0..938f480 100644 --- a/clients/go/generate-proto.ps1 +++ b/clients/go/generate-proto.ps1 @@ -6,7 +6,10 @@ $ErrorActionPreference = 'Stop' # those header stamps, so a regeneration on an off-pin machine would churn the tree and make # check-codegen Check 4 false-fail (or mask real drift under churn). Assert the exact versions # so a regen is deterministic. protoc itself is warn-only (source_code_info is normalized out of -# the committed bindings), matching publish-client-proto-inputs.ps1. +# the committed bindings), matching publish-client-proto-inputs.ps1. On Windows a plugin reports +# its argv[0] name, so the banner carries a trailing `.exe` (e.g. "protoc-gen-go.exe v1.36.11"); +# Get-NormalizedToolVersion strips that suffix before the pin compare so Check 4 runs the same on +# Windows as everywhere else. $PinnedProtocGenGoVersion = 'protoc-gen-go v1.36.11' $PinnedProtocGenGoGrpcVersion = 'protoc-gen-go-grpc 1.6.2' $PinnedProtocVersion = 'libprotoc 34.1' @@ -16,6 +19,14 @@ $protoRoot = Join-Path $repoRoot 'src\ZB.MOM.WW.MxGateway.Contracts\Protos' $outputRoot = Join-Path $PSScriptRoot 'internal\generated' $modulePath = 'gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated' +function Get-NormalizedToolVersion { + # On Windows a plugin reports its argv[0] name, so the banner carries an `.exe` + # suffix ("protoc-gen-go.exe v1.36.11"). Strip it so the pin compare is + # host-independent; the version part must still match exactly. + param([string]$RawBanner) + return ($RawBanner -replace '\.exe(?=\s)', '') +} + function Resolve-Tool { # Resolve a codegen tool from PATH first (portable), then the documented Windows fallbacks, # instead of the previous hard-coded per-machine paths. See docs/ToolchainLinks.md. @@ -51,17 +62,17 @@ $protocGenGoGrpc = Resolve-Tool -Names @('protoc-gen-go-grpc', 'protoc-gen-go-gr # Assert the pinned plugin versions before generating so Check 4 cannot false-fail (or mask drift) # on an off-pin machine. protoc is warn-only. -$protocGenGoVersion = (& $protocGenGo --version 2>&1 | Out-String).Trim() +$protocGenGoVersion = Get-NormalizedToolVersion (& $protocGenGo --version 2>&1 | Out-String).Trim() if ($protocGenGoVersion -ne $PinnedProtocGenGoVersion) { throw "protoc-gen-go reports '$protocGenGoVersion', but regeneration is pinned to '$PinnedProtocGenGoVersion'. " + "Install the pin: go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11" } -$protocGenGoGrpcVersion = (& $protocGenGoGrpc --version 2>&1 | Out-String).Trim() +$protocGenGoGrpcVersion = Get-NormalizedToolVersion (& $protocGenGoGrpc --version 2>&1 | Out-String).Trim() if ($protocGenGoGrpcVersion -ne $PinnedProtocGenGoGrpcVersion) { throw "protoc-gen-go-grpc reports '$protocGenGoGrpcVersion', but regeneration is pinned to '$PinnedProtocGenGoGrpcVersion'. " + "Install the pin: go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2" } -$protocVersion = (& $protoc --version 2>&1 | Out-String).Trim() +$protocVersion = Get-NormalizedToolVersion (& $protoc --version 2>&1 | Out-String).Trim() if ($protocVersion -ne $PinnedProtocVersion) { Write-Warning "protoc reports '$protocVersion', pin is '$PinnedProtocVersion'. Descriptor comments are normalized out of the committed Go bindings, so patch drift is tolerated; keep CI on the pin." } diff --git a/clients/java/zb-mom-ww-mxgateway-client/build.gradle b/clients/java/zb-mom-ww-mxgateway-client/build.gradle index 4fe4351..e13861f 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/build.gradle +++ b/clients/java/zb-mom-ww-mxgateway-client/build.gradle @@ -72,16 +72,18 @@ tasks.register('checkGeneratedClean') { group = 'verification' description = 'Fails if the committed generated Java tree differs from a fresh regeneration.' dependsOn 'generateProto' + def generatedDir = 'clients/java/src/main/generated' + def repoRoot = rootProject.projectDir.parentFile.parentFile + // Project.exec was removed in Gradle 9; ProviderFactory.exec runs the git-status probe + // lazily (captured at configuration time, evaluated in doLast) and works on both the + // installed Gradle and Gradle 9. + def gitStatus = providers.exec { + workingDir(repoRoot) + commandLine('git', 'status', '--porcelain', '--', generatedDir) + ignoreExitValue = true + } doLast { - def generatedDir = 'clients/java/src/main/generated' - def stdout = new ByteArrayOutputStream() - def result = exec { - workingDir = rootProject.projectDir.parentFile.parentFile - commandLine 'git', 'status', '--porcelain', '--', generatedDir - standardOutput = stdout - ignoreExitValue = true - } - def dirty = stdout.toString().trim() + def dirty = gitStatus.standardOutput.asText.get().trim() if (!dirty.isEmpty()) { throw new GradleException( "Generated Java is stale:\n${dirty}\n" + diff --git a/clients/python/generate-proto.ps1 b/clients/python/generate-proto.ps1 index a98eb29..863a21c 100644 --- a/clients/python/generate-proto.ps1 +++ b/clients/python/generate-proto.ps1 @@ -36,7 +36,11 @@ function Resolve-Python { function Assert-GrpcioToolsVersion { param([string]$Python) - $version = (& $Python -c 'import grpc_tools; from importlib.metadata import version; print(version("grpcio-tools"))').Trim() + # Windows PowerShell 5.1 strips embedded double quotes when passing an argument to a native + # exe (pwsh 7 does not), so a Python literal quoted with " " here would arrive at Python as + # print(version(grpcio-tools)) -> NameError. Use single quotes for the Python string literal + # inside this double-quoted PowerShell string so the quoting survives on both hosts. + $version = (& $Python -c "import grpc_tools; from importlib.metadata import version; print(version('grpcio-tools'))").Trim() if ($version -ne $PinnedGrpcioToolsVersion) { throw "grpcio-tools $version is installed, but regeneration is pinned to $PinnedGrpcioToolsVersion. " + "Install the pin (python -m pip install 'grpcio-tools==$PinnedGrpcioToolsVersion') before regenerating, " + diff --git a/code-reviews/Server/findings.md b/code-reviews/Server/findings.md index 72b66f8..a9202c2 100644 --- a/code-reviews/Server/findings.md +++ b/code-reviews/Server/findings.md @@ -259,7 +259,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Add a startup hosted service that finds and kills stale worker processes (by executable path / a well-known argument or environment marker) before the server accepts sessions, or update the design docs if reattachment/cleanup is deliberately deferred. -**Resolution:** Resolved 2026-05-18. Confirmed against source: no code path enumerated or killed leftover workers. Added `IRunningProcessInspector` / `SystemRunningProcessInspector` (a testable seam over `Process.GetProcessesByName`/`Kill`), `OrphanWorkerTerminator` (kills processes matched by the configured worker executable path, or by image name when the x64 gateway cannot introspect the x86 worker's `MainModule`, skipping the current process and tolerating per-process kill failures), and `OrphanWorkerCleanupHostedService` (best-effort `IHostedService`). The hosted service is registered in `AddWorkerProcessLauncher` ahead of `AddGatewaySessions` so cleanup runs before the server accepts sessions. `gateway.md` updated to describe the implemented behavior. Regression tests: `OrphanWorkerTerminatorTests` (`KillsWorkerProcessesMatchingConfiguredExecutablePath`, `KillsImageNameMatchWhenExecutablePathUnreadable`, `DoesNotKillUnrelatedProcessSharingImageName`, `DoesNotKillCurrentProcess`, `ContinuesWhenOneKillThrows`). +**Resolution:** Resolved 2026-05-18. Confirmed against source: no code path enumerated or killed leftover workers. Added `IRunningProcessInspector` / `SystemRunningProcessInspector` (a testable seam over `Process.GetProcessesByName`/`Kill`), `OrphanWorkerTerminator` (kills processes matched by the configured worker executable path, or by image name when the x64 gateway cannot introspect the x86 worker's `MainModule`, skipping the current process and tolerating per-process kill failures), and `OrphanWorkerCleanupHostedService` (best-effort `IHostedService`). The hosted service is registered in `AddWorkerProcessLauncher` ahead of `AddGatewaySessions` so cleanup runs before the server accepts sessions. `gateway.md` updated to describe the implemented behavior. Regression tests: `OrphanWorkerTerminatorTests` (`KillsWorkerProcessesMatchingConfiguredExecutablePath`, `KillsImageNameMatchWhenExecutablePathUnreadable`, `DoesNotKillUnrelatedProcessSharingImageName`, `DoesNotKillCurrentProcess`, `ContinuesWhenOneKillThrows`). Re-verified present 2026-08-18 (feat/followups-tickets) — doc sub-claim only; the `gateway.md` orphan-cleanup prose (`OrphanWorkerCleanupHostedService` running `OrphanWorkerTerminator` once on startup, before the server accepts sessions) is still there. ### Server-003 @@ -364,7 +364,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Set `Pooling`, a non-zero `DefaultTimeout`/`busy_timeout`, and enable WAL (`PRAGMA journal_mode=WAL`) once at startup so concurrent readers/writers degrade gracefully. -**Resolution:** Resolved 2026-05-18. Confirmed against source: the connection string set only `DataSource` and `Mode`. `AuthSqliteConnectionFactory.CreateConnection` now also sets `Pooling = true` and a non-zero `DefaultTimeout`. A new `OpenConnectionAsync(CancellationToken)` opens the connection and applies `PRAGMA journal_mode=WAL` and `PRAGMA busy_timeout` (5 s); WAL is a persistent database-level setting so re-applying it per connection is a cheap no-op, while `busy_timeout` is per-connection state. All nine auth-store call sites (`SqliteApiKeyAdminStore`, `SqliteApiKeyAuditStore`, `SqliteApiKeyStore`, `SqliteAuthStoreMigrator`) were switched from `CreateConnection()` + `OpenAsync()` to `OpenConnectionAsync()`. `docs/Authentication.md` updated to describe the WAL/busy-timeout behavior. Regression test: `SqliteAuthStoreTests.OpenConnectionAsync_EnablesWalJournalModeAndBusyTimeout`. +**Resolution:** Resolved 2026-05-18. Confirmed against source: the connection string set only `DataSource` and `Mode`. `AuthSqliteConnectionFactory.CreateConnection` now also sets `Pooling = true` and a non-zero `DefaultTimeout`. A new `OpenConnectionAsync(CancellationToken)` opens the connection and applies `PRAGMA journal_mode=WAL` and `PRAGMA busy_timeout` (5 s); WAL is a persistent database-level setting so re-applying it per connection is a cheap no-op, while `busy_timeout` is per-connection state. All nine auth-store call sites (`SqliteApiKeyAdminStore`, `SqliteApiKeyAuditStore`, `SqliteApiKeyStore`, `SqliteAuthStoreMigrator`) were switched from `CreateConnection()` + `OpenAsync()` to `OpenConnectionAsync()`. `docs/Authentication.md` updated to describe the WAL/busy-timeout behavior. Regression test: `SqliteAuthStoreTests.OpenConnectionAsync_EnablesWalJournalModeAndBusyTimeout`. Regressed or never applied; re-fixed 2026-08-18 in `docs/Authentication.md` (feat/followups-tickets) — the doc sub-claim did not survive: the Storage section was later rewritten to delegate schema/connection-factory detail to `ZB.MOM.WW.Auth.ApiKeys` and the WAL/busy-timeout paragraph went with it, leaving no prose for the behavior anywhere. The behavior itself is intact (the library's `AuthSqliteConnectionFactory.OpenConnectionAsync` still issues both pragmas — confirmed against the 0.2.1 package), so the fix is prose-only: a paragraph restating the pooled-connection + `journal_mode=WAL` + 5 s `busy_timeout` contract and why it is load-bearing for the per-request last-used stamp and the per-denial audit append. ### Server-010 @@ -379,7 +379,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Either hide/disable the Rotate action for revoked keys in `ApiKeysPage.razor`, require an explicit confirmation, or have `RotateAsync` preserve `revoked_utc` and add a separate explicit "reactivate" operation. -**Resolution:** Resolved 2026-05-18. Confirmed against source: `ApiKeysPage.razor` rendered the Rotate button unconditionally while Revoke was already gated on `key.RevokedUtc is null`. Took the lowest-risk recommended option — the dashboard now renders the Rotate (and Revoke) actions only for keys whose status is `Active`; a revoked key shows a "No actions" placeholder, so an operator cannot un-revoke a deliberately disabled key as a side effect of a rotation. `RotateAsync`'s store-level behavior is unchanged (rotation by `key_id` still clears `revoked_utc`, which the CLI relies on); `docs/Authentication.md` updated to document both the store behavior and the dashboard restriction. No automated test added: the change is pure conditional Razor rendering and the test project has no bUnit component-rendering harness; the underlying `DashboardApiKeyManagementService` is already unit-tested. +**Resolution:** Resolved 2026-05-18. Confirmed against source: `ApiKeysPage.razor` rendered the Rotate button unconditionally while Revoke was already gated on `key.RevokedUtc is null`. Took the lowest-risk recommended option — the dashboard now renders the Rotate (and Revoke) actions only for keys whose status is `Active`; a revoked key shows a "No actions" placeholder, so an operator cannot un-revoke a deliberately disabled key as a side effect of a rotation. `RotateAsync`'s store-level behavior is unchanged (rotation by `key_id` still clears `revoked_utc`, which the CLI relies on); `docs/Authentication.md` updated to document both the store behavior and the dashboard restriction. No automated test added: the change is pure conditional Razor rendering and the test project has no bUnit component-rendering harness; the underlying `DashboardApiKeyManagementService` is already unit-tested. Re-verified present 2026-08-18 (feat/followups-tickets) — `ApiKeysPage.razor` still gates both Rotate and Revoke on `key.RevokedUtc is null`, with the "Rotate clears revoked_utc" comment intact. ### Server-011 @@ -394,7 +394,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Add the needed `using` directives, drop the inline fully-qualified names, and convert to a primary constructor for consistency. -**Resolution:** Resolved 2026-05-18. Confirmed against source. Converted `WorkerAlarmRpcDispatcher` to a primary constructor with the standard `?? throw new ArgumentNullException(...)` field-initializer guard; dropped the inline `System.Guid` / `System.ArgumentNullException` qualifications (using implicit `using System;`); removed redundant `using System.Collections.Generic;` / `System.Threading` / `System.Threading.Tasks;` directives (covered by `ImplicitUsings`); replaced the two `if (... is null) throw new System.ArgumentNullException(...)` checks with `ArgumentNullException.ThrowIfNull`. The stale class-level ``/`` ("Replaces NotWiredAlarmRpcDispatcher once ... wired in", "partially wired", "returns an Unimplemented diagnostic") were corrected to describe the actual GUID-vs-`Provider!Group.Tag` handling — overlapping with Server-014. No behavior change, so no new test; existing `WorkerAlarmRpcDispatcherTests` continue to pass and the project builds warning-free under `TreatWarningsAsErrors`. +**Resolution:** Resolved 2026-05-18. Confirmed against source. Converted `WorkerAlarmRpcDispatcher` to a primary constructor with the standard `?? throw new ArgumentNullException(...)` field-initializer guard; dropped the inline `System.Guid` / `System.ArgumentNullException` qualifications (using implicit `using System;`); removed redundant `using System.Collections.Generic;` / `System.Threading` / `System.Threading.Tasks;` directives (covered by `ImplicitUsings`); replaced the two `if (... is null) throw new System.ArgumentNullException(...)` checks with `ArgumentNullException.ThrowIfNull`. The stale class-level ``/`` ("Replaces NotWiredAlarmRpcDispatcher once ... wired in", "partially wired", "returns an Unimplemented diagnostic") were corrected to describe the actual GUID-vs-`Provider!Group.Tag` handling — overlapping with Server-014. No behavior change, so no new test; existing `WorkerAlarmRpcDispatcherTests` continue to pass and the project builds warning-free under `TreatWarningsAsErrors`. Re-verified 2026-08-18 (feat/followups-tickets): not regressed but no longer verifiable in place — `WorkerAlarmRpcDispatcher.cs` was **deleted** (not renamed) by the `dc9c0c9` project-rename/alarm-rework commit, which replaced the whole `IAlarmRpcDispatcher` trio with `GatewayAlarmMonitor` / `IGatewayAlarmService`. A repo-wide grep for the prose this finding removed ("not yet wired", "PR A.6/A.7", "worker-pending", "dev-rig") returns nothing in Server source, so the correction is moot rather than lost. Nothing to re-fix. ### Server-012 @@ -409,7 +409,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Update CLAUDE.md's scope list and the `apikey` example to the canonical `*:*` scope strings, per CLAUDE.md's own rule that docs change with the code. -**Resolution:** Resolved 2026-05-18. Confirmed against `GatewayScopes` (`session:open`, `session:close`, `invoke:read`, `invoke:write`, `invoke:secure`, `events:read`, `metadata:read`, `admin`). CLAUDE.md's Build/Test/Run `apikey create` example and the Authentication-section scope list were both updated to the canonical `*:*` strings. (Note: since finding Server-004 was resolved, the old example would now be actively rejected at create time rather than silently creating an unusable key, making the doc correction load-bearing.) Pure documentation change; no test. +**Resolution:** Resolved 2026-05-18. Confirmed against `GatewayScopes` (`session:open`, `session:close`, `invoke:read`, `invoke:write`, `invoke:secure`, `events:read`, `metadata:read`, `admin`). CLAUDE.md's Build/Test/Run `apikey create` example and the Authentication-section scope list were both updated to the canonical `*:*` strings. (Note: since finding Server-004 was resolved, the old example would now be actively rejected at create time rather than silently creating an unusable key, making the doc correction load-bearing.) Pure documentation change; no test. Regression re-fixed 2026-08-18 on feat/followup-closeout (a5f843c, f2a422b); scope lists re-verified present. (Both CLAUDE.md sites confirmed on feat/followups-tickets: the Build/Test/Run `apikey create-key` example carries the full canonical `session:open,session:close,invoke:read,invoke:write,invoke:secure,events:read,metadata:read,admin` set, and the Authentication-section list names the same eight strings.) ### Server-013 @@ -439,7 +439,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Update the `AcknowledgeAlarm`/`QueryActiveAlarms` remarks to reflect that `WorkerAlarmRpcDispatcher` is the wired default, and describe its actual GUID-vs-`Provider!Group.Tag` handling. -**Resolution:** Resolved 2026-05-18. Confirmed against source: `SessionServiceCollectionExtensions` registers `WorkerAlarmRpcDispatcher` as `IAlarmRpcDispatcher`, so the "not yet wired" / "empty stream until PR A.2" / "PR A.6/A.7 follow-up" prose in the `AcknowledgeAlarm` and `QueryActiveAlarms` `` and inline comments was stale. Rewrote both `` blocks and both inline comments to state that DI binds the production `WorkerAlarmRpcDispatcher`, that it routes over the worker pipe IPC, and that `AcknowledgeAlarm` handles a canonical-GUID reference (→ `AcknowledgeAlarmCommand`) and a `Provider!Group.Tag` reference (→ `AcknowledgeAlarmByNameCommand`), with `NotWiredAlarmRpcDispatcher` being only the null fallback. The matching stale `WorkerAlarmRpcDispatcher` class-level XML doc was corrected as part of Server-011. Pure documentation/comment change; no test. +**Resolution:** Resolved 2026-05-18. Confirmed against source: `SessionServiceCollectionExtensions` registers `WorkerAlarmRpcDispatcher` as `IAlarmRpcDispatcher`, so the "not yet wired" / "empty stream until PR A.2" / "PR A.6/A.7 follow-up" prose in the `AcknowledgeAlarm` and `QueryActiveAlarms` `` and inline comments was stale. Rewrote both `` blocks and both inline comments to state that DI binds the production `WorkerAlarmRpcDispatcher`, that it routes over the worker pipe IPC, and that `AcknowledgeAlarm` handles a canonical-GUID reference (→ `AcknowledgeAlarmCommand`) and a `Provider!Group.Tag` reference (→ `AcknowledgeAlarmByNameCommand`), with `NotWiredAlarmRpcDispatcher` being only the null fallback. The matching stale `WorkerAlarmRpcDispatcher` class-level XML doc was corrected as part of Server-011. Pure documentation/comment change; no test. Re-verified 2026-08-18 (feat/followups-tickets): not regressed but superseded — `MxAccessGatewayService.AcknowledgeAlarm` / `QueryActiveAlarms` still exist but were rewritten onto `IGatewayAlarmService` (the `IAlarmRpcDispatcher` seam is gone), and both now carry a bare `` rather than the rewritten ``. The stale prose this finding removed is absent repo-wide, so there is nothing to re-fix; the rewritten remarks describing a now-deleted dispatcher would be wrong if restored. ### Server-015 @@ -454,7 +454,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Make `CloseAsync` mutate `_state` through the existing `TransitionTo(...)` helper (or acquire `_syncRoot` around the reads/writes) so all `_state` access uses the same lock. Either extend `TransitionTo` to accept the `Closing` and `Closed` transitions (it already handles `Faulted`/`Closed` precedence) or refactor `CloseAsync` to call a private `TrySetClosing()` / `MarkClosed()` that locks `_syncRoot`. Add a regression test that forces a `TransitionTo(Ready)` after `CloseAsync` has set `Closing` and asserts the session does not flip back to `Ready`. -**Resolution:** 2026-05-20 — Unified the close path on `_syncRoot`. `GatewaySession.CloseAsync` (`src/MxGateway.Server/Sessions/GatewaySession.cs`) now mutates `_state` only through two private `_syncRoot`-locked helpers — `TryBeginClose` (writes `Closing`, returns the prior `_closeStarted`) and `MarkClosed` (writes `Closed`) — so every `_state` read/write in the session uses the same lock; `_closeLock` keeps its role of serializing concurrent close attempts. `TransitionTo` was tightened to refuse a transition out of `Closing` to anything other than `Closed`/`Faulted` so a late lifecycle callback cannot walk a closing session back to `Ready`. `docs/Sessions.md` updated to describe the unified lock discipline and the extended terminal precedence. Regression tests in `src/MxGateway.Tests/Gateway/Sessions/GatewaySessionTests.cs`: `TransitionTo_AfterCloseStarted_DoesNotOverwriteClosing` (the named scenario — `BlockingShutdownWorkerClient` parks the close inside `worker.ShutdownAsync` so the test can call `TransitionTo(Ready)` between the `Closing` and `Closed` writes and assert the state stays `Closing`) and `MarkFaulted_AfterCloseCompletes_DoesNotResurrectSession`. +**Resolution:** 2026-05-20 — Unified the close path on `_syncRoot`. `GatewaySession.CloseAsync` (`src/MxGateway.Server/Sessions/GatewaySession.cs`) now mutates `_state` only through two private `_syncRoot`-locked helpers — `TryBeginClose` (writes `Closing`, returns the prior `_closeStarted`) and `MarkClosed` (writes `Closed`) — so every `_state` read/write in the session uses the same lock; `_closeLock` keeps its role of serializing concurrent close attempts. `TransitionTo` was tightened to refuse a transition out of `Closing` to anything other than `Closed`/`Faulted` so a late lifecycle callback cannot walk a closing session back to `Ready`. `docs/Sessions.md` updated to describe the unified lock discipline and the extended terminal precedence. Re-verified present 2026-08-18 (feat/followups-tickets) — doc sub-claim only; `docs/Sessions.md` still states that both close-related writes go through `_syncRoot` via `TryBeginClose`/`MarkClosed` and that `_closeLock` only serializes concurrent close attempts. Regression tests in `src/MxGateway.Tests/Gateway/Sessions/GatewaySessionTests.cs`: `TransitionTo_AfterCloseStarted_DoesNotOverwriteClosing` (the named scenario — `BlockingShutdownWorkerClient` parks the close inside `worker.ShutdownAsync` so the test can call `TransitionTo(Ready)` between the `Closing` and `Closed` writes and assert the state stays `Closing`) and `MarkFaulted_AfterCloseCompletes_DoesNotResurrectSession`. ### Server-016 @@ -484,7 +484,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Add explicit arms to `ResolveRequiredScope`: map `AcknowledgeAlarmRequest` to `GatewayScopes.InvokeWrite` (parity with other write actions; ack changes alarm state) and `QueryActiveAlarmsRequest` to `GatewayScopes.MetadataRead` or `GatewayScopes.InvokeRead`. Update `docs/Authorization.md` to list both. Extend `GatewayGrpcScopeResolverTests` with the new mappings and an assertion that every request type defined by `mxaccess_gateway.proto` is named in the resolver (the test can enumerate the assembly's request types so a future RPC cannot quietly add itself only via the admin fallback). -**Resolution:** 2026-05-20 — Added explicit `AcknowledgeAlarmRequest => GatewayScopes.InvokeWrite` and `QueryActiveAlarmsRequest => GatewayScopes.EventsRead` arms to `GatewayGrpcScopeResolver.ResolveRequiredScope` (`src/MxGateway.Server/Security/Authorization/GatewayGrpcScopeResolver.cs:21-22`). `InvokeWrite` matches the existing `MxCommandKind.Write*` mapping because ack mutates alarm state; `EventsRead` matches `StreamEventsRequest` and `MxCommandKind.DrainEvents` because querying active alarms reads the same alarm/event surface. Extended `GatewayGrpcScopeResolverTests` with two new `InlineData` rows covering both request types (`src/MxGateway.Tests/Security/Authorization/GatewayGrpcScopeResolverTests.cs:16-17`) and added four interceptor-level cases in `GatewayGrpcAuthorizationInterceptorTests` (`UnaryServerHandler_AcknowledgeAlarmMissingScope_ReturnsPermissionDenied`, `UnaryServerHandler_AcknowledgeAlarmWithScope_RunsHandler`, `ServerStreamingServerHandler_QueryActiveAlarmsMissingScope_ReturnsPermissionDenied`, `ServerStreamingServerHandler_QueryActiveAlarmsWithScope_RunsHandler`) proving each new RPC denies callers lacking the chosen scope and runs the handler when the scope is held. Updated `docs/Authorization.md` (resolver snippet and Scope Catalog table) to list both RPCs against their scopes. `dotnet test ... --filter FullyQualifiedName~GatewayGrpcAuthorizationInterceptorTests` → 14 passed, 0 failed; resolver tests 28 passed, 0 failed. +**Resolution:** 2026-05-20 — Added explicit `AcknowledgeAlarmRequest => GatewayScopes.InvokeWrite` and `QueryActiveAlarmsRequest => GatewayScopes.EventsRead` arms to `GatewayGrpcScopeResolver.ResolveRequiredScope` (`src/MxGateway.Server/Security/Authorization/GatewayGrpcScopeResolver.cs:21-22`). `InvokeWrite` matches the existing `MxCommandKind.Write*` mapping because ack mutates alarm state; `EventsRead` matches `StreamEventsRequest` and `MxCommandKind.DrainEvents` because querying active alarms reads the same alarm/event surface. Extended `GatewayGrpcScopeResolverTests` with two new `InlineData` rows covering both request types (`src/MxGateway.Tests/Security/Authorization/GatewayGrpcScopeResolverTests.cs:16-17`) and added four interceptor-level cases in `GatewayGrpcAuthorizationInterceptorTests` (`UnaryServerHandler_AcknowledgeAlarmMissingScope_ReturnsPermissionDenied`, `UnaryServerHandler_AcknowledgeAlarmWithScope_RunsHandler`, `ServerStreamingServerHandler_QueryActiveAlarmsMissingScope_ReturnsPermissionDenied`, `ServerStreamingServerHandler_QueryActiveAlarmsWithScope_RunsHandler`) proving each new RPC denies callers lacking the chosen scope and runs the handler when the scope is held. Updated `docs/Authorization.md` (resolver snippet and Scope Catalog table) to list both RPCs against their scopes. Re-verified present 2026-08-18 (feat/followups-tickets) — doc sub-claim only; the snippet still shows `AcknowledgeAlarmRequest => GatewayScopes.InvokeWrite` / `QueryActiveAlarmsRequest => GatewayScopes.EventsRead` and both RPCs still appear in the Scope Catalog rows. `dotnet test ... --filter FullyQualifiedName~GatewayGrpcAuthorizationInterceptorTests` → 14 passed, 0 failed; resolver tests 28 passed, 0 failed. ### Server-018 @@ -559,7 +559,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Rewrite the `IAlarmRpcDispatcher` `` block to match the language now used on `WorkerAlarmRpcDispatcher` and on the gRPC service: DI binds `WorkerAlarmRpcDispatcher` by default; `NotWiredAlarmRpcDispatcher` is only the null fallback for tests/DI omission. Drop the "PR A.6 / A.7" prefix from the `` — the interface is now the public alarm-RPC seam. -**Resolution:** 2026-05-20 — Rewrote `IAlarmRpcDispatcher`'s `` and `` (`src/MxGateway.Server/Sessions/IAlarmRpcDispatcher.cs`) to match the language now used on `WorkerAlarmRpcDispatcher` and on `MxAccessGatewayService.AcknowledgeAlarm` / `QueryActiveAlarms`: dropped the stale "PR A.6 / A.7" prefix from the summary, and replaced the "this PR ships a not-yet-wired default that returns a clear worker-pending diagnostic" clause with the correct statement that DI binds the production `WorkerAlarmRpcDispatcher` by default and `NotWiredAlarmRpcDispatcher` is only the null fallback for DI omission / standalone tests. Pure documentation change; no test. +**Resolution:** 2026-05-20 — Rewrote `IAlarmRpcDispatcher`'s `` and `` (`src/MxGateway.Server/Sessions/IAlarmRpcDispatcher.cs`) to match the language now used on `WorkerAlarmRpcDispatcher` and on `MxAccessGatewayService.AcknowledgeAlarm` / `QueryActiveAlarms`: dropped the stale "PR A.6 / A.7" prefix from the summary, and replaced the "this PR ships a not-yet-wired default that returns a clear worker-pending diagnostic" clause with the correct statement that DI binds the production `WorkerAlarmRpcDispatcher` by default and `NotWiredAlarmRpcDispatcher` is only the null fallback for DI omission / standalone tests. Pure documentation change; no test. Re-verified 2026-08-18 (feat/followups-tickets): not regressed but no longer verifiable in place — `IAlarmRpcDispatcher.cs` was deleted with the rest of the dispatcher trio in `dc9c0c9`. No stale "PR A.6/A.7" or "not-yet-wired" prose survives anywhere in Server source. Nothing to re-fix. ### Server-023 @@ -574,7 +574,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Replace the `` and `` on `NotWiredAlarmRpcDispatcher` with text that matches the language now used on the interface and `WorkerAlarmRpcDispatcher` — "null fallback `IAlarmRpcDispatcher` used when no dispatcher is registered (DI omission / standalone tests); production wires `WorkerAlarmRpcDispatcher`." Either drop the `AcknowledgeAsync` diagnostic string's dev-rig framing entirely or shorten it to "alarm dispatcher is not registered." `#pragma warning disable CS1998` on `QueryActiveAlarmsAsync` is correct here (empty stream is intentional for the null fallback) and should stay. -**Resolution:** 2026-05-20 — Rewrote `NotWiredAlarmRpcDispatcher` summary/remarks as the null-fallback dispatcher and shortened the `AcknowledgeAsync` diagnostic to "Alarm dispatcher is not registered."; updated the two tests that asserted the old "worker"-prefixed diagnostic. +**Resolution:** 2026-05-20 — Rewrote `NotWiredAlarmRpcDispatcher` summary/remarks as the null-fallback dispatcher and shortened the `AcknowledgeAsync` diagnostic to "Alarm dispatcher is not registered."; updated the two tests that asserted the old "worker"-prefixed diagnostic. Re-verified 2026-08-18 (feat/followups-tickets): not regressed but no longer verifiable in place — `NotWiredAlarmRpcDispatcher.cs` and its tests were deleted with the dispatcher trio in `dc9c0c9`. Nothing to re-fix. ### Server-024 @@ -637,7 +637,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Update the `ResolveCommandScope` snippet to include the four bulk-write arms. Update the Constraint Enforcement prose to enumerate the bulk read/write commands that are actually filtered, and reference the per-entry index-ordered merge that `BulkConstraintPlan.MergeDeniedInto` performs. Adding `ReadBulk` to the `InvokeRead` row of the Scope Catalog would also be useful — the table currently lists `Register`/`AddItem`/`Advise` against `InvokeRead` but not `ReadBulk`. -**Resolution:** 2026-05-20 — Updated the `ResolveCommandScope` snippet in `docs/Authorization.md` to enumerate the four bulk-write arms (`WriteBulk`/`Write2Bulk` against `InvokeWrite`, `WriteSecuredBulk`/`WriteSecured2Bulk` against `InvokeSecure`); expanded the Constraint Enforcement prose to list `ReadBulk` and all four bulk-write commands and to call out `BulkConstraintPlan.MergeDeniedInto`'s index-ordered merge; added `ReadBulk` to the `InvokeRead` row of the Scope Catalog. +**Resolution:** 2026-05-20 — Updated the `ResolveCommandScope` snippet in `docs/Authorization.md` to enumerate the four bulk-write arms (`WriteBulk`/`Write2Bulk` against `InvokeWrite`, `WriteSecuredBulk`/`WriteSecured2Bulk` against `InvokeSecure`); expanded the Constraint Enforcement prose to list `ReadBulk` and all four bulk-write commands and to call out `BulkConstraintPlan.MergeDeniedInto`'s index-ordered merge; added `ReadBulk` to the `InvokeRead` row of the Scope Catalog. Re-verified present 2026-08-18 (feat/followups-tickets) — all three corrections still stand in `docs/Authorization.md`: the snippet carries the four bulk-write arms, the Constraint Enforcement prose names `ReadBulk` plus the four bulk-write commands and `BulkConstraintPlan.MergeDeniedInto`, and `ReadBulk` is in the `InvokeRead` catalog row. ### Server-028 @@ -670,7 +670,7 @@ closed. New findings filed against this pass: Server-051..053. **Recommendation:** Either (a) extend the advertised list with `bulk-read-command` and `bulk-write-commands` (`WriteBulk` / `Write2Bulk` / `WriteSecuredBulk` / `WriteSecured2Bulk` collectively), or (b) document in `gateway.md` and `docs/Contracts.md` that `Capabilities` is informational only and not the contract version. Option (a) is the simplest forward-compatible fix and keeps the capability token shape clients are already familiar with. -**Resolution:** 2026-05-20 — Extended the `OpenSession` capabilities list with `bulk-read-commands` and `bulk-write-commands` alongside the existing `bulk-subscribe-commands` token, so clients that gate on capability strings have an explicit signal for the bulk-read and bulk-write families. +**Resolution:** 2026-05-20 — Extended the `OpenSession` capabilities list with `bulk-read-commands` and `bulk-write-commands` alongside the existing `bulk-subscribe-commands` token, so clients that gate on capability strings have an explicit signal for the bulk-read and bulk-write families. Re-verified present 2026-08-18 (feat/followups-tickets) — `MxAccessGatewayService.OpenSession` still adds all three tokens. ### Server-030 @@ -839,7 +839,7 @@ Add a regression test that advises N items without an active `StreamEvents` cons **Recommendation:** Before the EventsHub is exercised by Admin-only sessions or session-scoped Viewer roles, gate `SubscribeSession` on a session-access check — either via a per-session role check in the hub method itself, or by storing a per-user allowed-session-id set in the connection's `Context.Items` at connect time and rejecting subscribes outside that set. The current dashboard surfaces only a per-page Session Details view that the page can prove it's authorized for, but as soon as a Viewer role exists the gap matters. -**Resolution:** 2026-05-24 — Documented the v1 acceptance per the prompt's "practical fix for v1" direction. Added a detailed `` block to `EventsHub.SubscribeSession` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHub.cs`) stating that (a) in v1 the hub-level `HubClientsPolicy` only requires one of the dashboard roles (Admin or Viewer) and both may subscribe to any session id, (b) this is acceptable today because the dashboard's per-session views show non-secret session metadata any authenticated user can already see and value logging is gated by the same redaction policy, and (c) the per-session ACL that gates the gRPC `StreamEvents` RPC is intentionally not yet mirrored here. Added an explicit `TODO(per-session-acl)` describing the future enforcement seam — once a role/scope is introduced that scopes a Viewer to a specific session or tenant, add a session-access check at this method (inline on `Context.User` claims/`Context.Items`, or via a dedicated authorization policy applied to the hub method). No code-behavior change in this pass; the per-session ACL data model design is out of scope for the resolution window. No new regression test (the change is documentation-only). +**Resolution:** 2026-05-24 — Documented the v1 acceptance per the prompt's "practical fix for v1" direction. Added a detailed `` block to `EventsHub.SubscribeSession` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHub.cs`) stating that (a) in v1 the hub-level `HubClientsPolicy` only requires one of the dashboard roles (Admin or Viewer) and both may subscribe to any session id, (b) this is acceptable today because the dashboard's per-session views show non-secret session metadata any authenticated user can already see and value logging is gated by the same redaction policy, and (c) the per-session ACL that gates the gRPC `StreamEvents` RPC is intentionally not yet mirrored here. Added an explicit `TODO(per-session-acl)` describing the future enforcement seam — once a role/scope is introduced that scopes a Viewer to a specific session or tenant, add a session-access check at this method (inline on `Context.User` claims/`Context.Items`, or via a dedicated authorization policy applied to the hub method). No code-behavior change in this pass; the per-session ACL data model design is out of scope for the resolution window. No new regression test (the change is documentation-only). Re-verified 2026-08-18 (feat/followups-tickets): the documented v1 gap was subsequently **closed**, not regressed — `EventsHub` now takes `IDashboardSessionAcl` and `SubscribeSession` throws `HubException("Not authorized for this session.")` when `CanViewSession` denies (SEC-25 / TST-15). The `TODO(per-session-acl)` is correctly gone and the `` block now describes the shipped tag-scoped gate (admin bypass first, tag intersection, `Dashboard:UntaggedSessionVisibility`, phantom-id denial for non-Admins). Nothing to re-fix; the finding's own recommendation is what shipped. ### Server-039 @@ -869,7 +869,7 @@ Add a regression test that advises N items without an active `StreamEvents` cons **Recommendation:** Add a one-line comment above the loop explaining the precedence: full DN/CN literal first, leading-RDN fallback second. Mention the case-insensitive map comparer (`OrdinalIgnoreCase`) so the next reader doesn't ask why `"GwAdmin"` matches `"gwadmin"`. -**Resolution:** 2026-05-24 — Added a precedence comment block above the lookup in `MapGroupsToRoles` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardAuthenticator.cs:156-163`) explaining that the full literal group string is tried first and the leading-RDN value (e.g. `GwAdmin` extracted from `ou=GwAdmin,ou=groups,...`) is the fallback, and back-referencing `DashboardOptions.GroupToRole` as the source of the `OrdinalIgnoreCase` comparer so a maintainer sees why `"GwAdmin"` matches `"gwadmin"`. No code change — existing `DashboardAuthenticatorTests.MapGroupsToRoles_ResolvesByShortNameAndDistinguishedName` already pins both the full-match and RDN-fallback paths and the case-insensitive lookup; pure documentation-only resolution, no new test. +**Resolution:** 2026-05-24 — Added a precedence comment block above the lookup in `MapGroupsToRoles` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardAuthenticator.cs:156-163`) explaining that the full literal group string is tried first and the leading-RDN value (e.g. `GwAdmin` extracted from `ou=GwAdmin,ou=groups,...`) is the fallback, and back-referencing `DashboardOptions.GroupToRole` as the source of the `OrdinalIgnoreCase` comparer so a maintainer sees why `"GwAdmin"` matches `"gwadmin"`. No code change — existing `DashboardAuthenticatorTests.MapGroupsToRoles_ResolvesByShortNameAndDistinguishedName` already pins both the full-match and RDN-fallback paths and the case-insensitive lookup; pure documentation-only resolution, no new test. Regressed or never applied; re-fixed 2026-08-18 in `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupRoleMapping.cs` (feat/followups-tickets). Chain of events: the lookup moved out of `DashboardAuthenticator` into the shared `DashboardGroupRoleMapping` helper in `792e3f9` (the `IGroupRoleMapper` seam) **with the comment intact**, then `fca978d` ("docs(src): add missing XML docs and strip tracking-ID comments") deleted the entire block rather than just the `(Server-040)` marker inside it. That also took out a second, substantive paragraph added later — the note that the shared `ZB.MOM.WW.Auth.Ldap` provider already strips groups to short RDN names, making the RDN fallback a no-op on the live login path and making a full-DN `GroupToRole` **key** unsupported. Both paragraphs are restored, minus the tracking IDs (that part of the sweep's intent is respected). ### Server-041 @@ -914,7 +914,7 @@ Add a regression test that advises N items without an active `StreamEvents` cons **Recommendation:** Add a `` block to `HubTokenService` noting "Registered as a singleton in `AddGatewayDashboard`; the underlying `ITimeLimitedDataProtector` is thread-safe and shared across hub-token issuance and validation." Optionally add a comment near the DI registration explaining the lifetime contract. -**Resolution:** 2026-05-24 — Added a `` block to `HubTokenService` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs`) documenting that the service is registered as a singleton in `DashboardServiceCollectionExtensions.AddGatewayDashboard` and is shared by two consumer scopes — `DashboardHubConnectionFactory` (scoped, per-circuit; calls `Issue` from the cookie-authenticated dashboard) and `HubTokenAuthenticationHandler` (transient, per-request; calls `Validate` from the SignalR negotiate / connection path). Notes that the underlying `ITimeLimitedDataProtector` is thread-safe so concurrent mint/validate from any number of callers is safe, and explicitly asks future maintainers to preserve the singleton lifetime to keep the protector instance stable. Pure documentation change; no test. +**Resolution:** 2026-05-24 — Added a `` block to `HubTokenService` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs`) documenting that the service is registered as a singleton in `DashboardServiceCollectionExtensions.AddGatewayDashboard` and is shared by two consumer scopes — `DashboardHubConnectionFactory` (scoped, per-circuit; calls `Issue` from the cookie-authenticated dashboard) and `HubTokenAuthenticationHandler` (transient, per-request; calls `Validate` from the SignalR negotiate / connection path). Notes that the underlying `ITimeLimitedDataProtector` is thread-safe so concurrent mint/validate from any number of callers is safe, and explicitly asks future maintainers to preserve the singleton lifetime to keep the protector instance stable. Pure documentation change; no test. Re-verified present 2026-08-18 (feat/followups-tickets) — the `` block still names the `AddGatewayDashboard` singleton registration, the two consumer scopes, the thread-safe `ITimeLimitedDataProtector`, and the preserve-the-lifetime request. One consumer was renamed since (the issuing side is now the `/hubs/token` endpoint rather than `DashboardHubConnectionFactory`) and the doc tracked that change correctly. ## Re-review 2026-05-24 (commit 42b0037) @@ -983,7 +983,7 @@ The user-visible difference: rotating/revoking/deleting a key vs closing/killing **Recommendation:** Align `ApiKeysPage.ConfirmPendingAsync` with the sessions pages: hold `PendingAction`, set `IsBusy = true`, run the action, then clear `PendingAction` in the `finally`. The current ApiKeysPage shape was inherited from before the dialog existed (when the confirmation was a `confirm()` JS call); the dialog component change can flatten the difference now. As a smaller alternative, document the divergence on the component's XML doc — but the shared component should ideally be used consistently. -**Resolution:** 2026-05-24 — Took the recommended alignment. `ApiKeysPage.ConfirmPendingAsync` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor`) now holds `PendingAction` for the duration of the awaited action (so the shared `ConfirmDialog` renders its `IsBusy` in-flight state on the dialog itself, matching the sessions pages) and clears it in `finally` regardless of outcome. The action is captured up front so a clear in `finally` works even when the action throws. `RunManagementActionAsync` continues to drive `IsBusy = true` inside its own `try/finally`, so the dialog now correctly disables Confirm/Cancel while the awaited service call runs. Pure UX-consistency change; no new automated test (no bUnit harness in the test project — same precedent as Server-010). +**Resolution:** 2026-05-24 — Took the recommended alignment. `ApiKeysPage.ConfirmPendingAsync` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor`) now holds `PendingAction` for the duration of the awaited action (so the shared `ConfirmDialog` renders its `IsBusy` in-flight state on the dialog itself, matching the sessions pages) and clears it in `finally` regardless of outcome. The action is captured up front so a clear in `finally` works even when the action throws. `RunManagementActionAsync` continues to drive `IsBusy = true` inside its own `try/finally`, so the dialog now correctly disables Confirm/Cancel while the awaited service call runs. Pure UX-consistency change; no new automated test (no bUnit harness in the test project — same precedent as Server-010). Re-verified present 2026-08-18 (feat/followups-tickets) — `ApiKeysPage.ConfirmPendingAsync` still captures the action up front, awaits inside `try`, and clears `PendingAction` in `finally`; the explanatory comment survives. ### Server-048 @@ -1017,7 +1017,7 @@ The user-visible difference: rotating/revoking/deleting a key vs closing/killing **Recommendation:** Add `` blocks to `IDashboardSessionAdminService.CanManage` (states the Admin-role gate), `CloseSessionAsync` and `KillWorkerAsync` (state that missing sessions return `DashboardSessionAdminResult.Fail(...)` rather than throwing, and that the audit log captures actor + remote IP). Add `` and `` for the request/response shape. The same sweep can pick up the longstanding gap on `IDashboardApiKeyManagementService` if the team wants — but the new file is the load-bearing one. -**Resolution:** 2026-05-24 — Added `` + `` blocks to every member of `IDashboardSessionAdminService` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAdminService.cs`): an interface-level `` describing the Admin-role gate, audit log shape, and `DashboardSessionAdminResult.Fail` semantics; per-member docs on `CanManage`, `CloseSessionAsync`, and `KillWorkerAsync` calling out the missing-session-returns-Fail contract and the `dashboard-admin-kill` reason constant that reaches the worker-kill audit log and `mxgateway.workers.killed` counter tag. `DashboardSessionAdminService` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs`) picked up a class-level `` + `` describing the per-page audit-log seam, plus `` on each public method. Pure documentation change; no test (the behavioral contracts the docs describe are already exercised by the existing `DashboardSessionAdminServiceTests` cases). +**Resolution:** 2026-05-24 — Added `` + `` blocks to every member of `IDashboardSessionAdminService` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAdminService.cs`): an interface-level `` describing the Admin-role gate, audit log shape, and `DashboardSessionAdminResult.Fail` semantics; per-member docs on `CanManage`, `CloseSessionAsync`, and `KillWorkerAsync` calling out the missing-session-returns-Fail contract and the `dashboard-admin-kill` reason constant that reaches the worker-kill audit log and `mxgateway.workers.killed` counter tag. `DashboardSessionAdminService` (`src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs`) picked up a class-level `` + `` describing the per-page audit-log seam, plus `` on each public method. Pure documentation change; no test (the behavioral contracts the docs describe are already exercised by the existing `DashboardSessionAdminServiceTests` cases). Re-verified present 2026-08-18 (feat/followups-tickets) — the interface-level `` and all three per-member blocks are intact, including the `dashboard-admin-kill` reason constant and the missing-session-returns-`Fail` contract. ### Server-050 @@ -1078,7 +1078,7 @@ catch (Exception ex) { _logger.LogWarning(ex, "...continuing with configuration- **Recommendation:** For (1), align the `IAlarmWatchListResolver` doc with whatever Server-051 settles on. For (2), either restrict the exclude to GR-discovered rows (apply `RemoveAll` before appending the `IncludeAttributes` entries) or update the option XML doc and `GatewayConfiguration.md` to say excludes are applied to the merged GR-plus-include list and therefore also suppress matching explicit includes. -**Resolution:** Resolved 2026-06-15. (1) No longer over-promises: the Server-051 fix makes the implementation propagate `OperationCanceledException`, so the `IAlarmWatchListResolver.ResolveAsync` `` doc is now accurate and was left unchanged. (2) Kept the "excludes win" code behaviour (excludes applied to the merged GR-plus-include list) and corrected the prose to match: `AlarmDiscoveryOptions.ExcludeAttributes` XML doc and `docs/GatewayConfiguration.md:247` now state the exclude runs after the GR rows and explicit `IncludeAttributes` are combined, so an exclude matching an explicit include suppresses it too. The "excludes win" precedence is pinned by `AlarmWatchListResolverTests.ResolveAsync_ExcludeAlsoSuppressesMatchingExplicitInclude`. +**Resolution:** Resolved 2026-06-15. (1) No longer over-promises: the Server-051 fix makes the implementation propagate `OperationCanceledException`, so the `IAlarmWatchListResolver.ResolveAsync` `` doc is now accurate and was left unchanged. (2) Kept the "excludes win" code behaviour (excludes applied to the merged GR-plus-include list) and corrected the prose to match: `AlarmDiscoveryOptions.ExcludeAttributes` XML doc and `docs/GatewayConfiguration.md:247` now state the exclude runs after the GR rows and explicit `IncludeAttributes` are combined, so an exclude matching an explicit include suppresses it too. The "excludes win" precedence is pinned by `AlarmWatchListResolverTests.ResolveAsync_ExcludeAlsoSuppressesMatchingExplicitInclude`. Re-verified present 2026-08-18 (feat/followups-tickets) — both doc corrections stand: `IAlarmWatchListResolver.ResolveAsync`'s `` still documents cancellation propagation (and the implementation still honors it), and the "excludes win over explicit includes" wording is present on `AlarmDiscoveryOptions.ExcludeAttributes` (now in `Configuration/AlarmFallbackOptions.cs`) and in the `docs/GatewayConfiguration.md` option table. ### Server-053 @@ -1113,7 +1113,7 @@ Additionally, `GatewayAlarmMonitor.ApplyProviderModeChangeAsync` increments the **Recommendation:** Update both `DesignDecisions.md` sections and the revisit list to describe the shipped behavior (gated by `AllowMultipleEventSubscribers`, `DetachGraceSeconds`, replay options), and amend the CLAUDE.md convention bullet. -**Resolution:** 2026-06-16: updated `docs/DesignDecisions.md` (Session Reconnect section rewritten to describe the shipped detach-grace + replay-on-reconnect behavior with config references; Event Subscribers section rewritten to describe the config-gated multi-subscriber fan-out, mode-dependent `FailFast` semantics, and internal vs external subscriber distinction; Later Revisit Items list removes the two shipped items and records them as shipped with config cross-references) and the `CLAUDE.md` conventions bullet to describe the shipped config-gated multi-subscriber + reconnect-replay behavior while preserving the one-worker-per-session invariant. +**Resolution:** 2026-06-16: updated `docs/DesignDecisions.md` (Session Reconnect section rewritten to describe the shipped detach-grace + replay-on-reconnect behavior with config references; Event Subscribers section rewritten to describe the config-gated multi-subscriber fan-out, mode-dependent `FailFast` semantics, and internal vs external subscriber distinction; Later Revisit Items list removes the two shipped items and records them as shipped with config cross-references) and the `CLAUDE.md` conventions bullet to describe the shipped config-gated multi-subscriber + reconnect-replay behavior while preserving the one-worker-per-session invariant. Re-verified present 2026-08-18 (feat/followups-tickets) — `docs/DesignDecisions.md` still records the "no reconnectable sessions" constraint as superseded and documents `DetachGraceSeconds` / `AllowMultipleEventSubscribers` / `ReplayBufferCapacity`, the Later Revisit Items list still cross-references them as shipped, and the `CLAUDE.md` bullet still carries the config-gated wording. ### Server-055 @@ -1158,7 +1158,7 @@ Additionally, `GatewayAlarmMonitor.ApplyProviderModeChangeAsync` increments the **Recommendation:** Either (a) extend `NormalizeOutboundCommand` and the `MapCommand` tracking path to normalize each `AddItemBulk.TagAddresses` entry (and `AddBufferedItem.ItemDefinition`) the same `IsArray`-gated way, keeping the constraint check, the worker bind, and the stored `SessionItemRegistration.TagAddress` consistent; or (b) if bulk-add normalization is intentionally out of scope for this feature, state that explicitly in `gateway.md` and the client READMEs (alongside the existing `ReadBulk` carve-out) so clients know bulk-added array handles must carry the `[]` suffix themselves to be writable. -**Resolution:** 2026-06-18 — Took option (a). Root cause confirmed: `NormalizeOutboundCommand` had no `AddItemBulk`/`AddBufferedItem` case, so the worker bound bare (non-write-capable) array handles for those paths while single-add was already fixed. Added `AddItemBulk` (normalizes each `TagAddresses` entry in place) and `AddBufferedItem` (normalizes `ItemDefinition`) cases to `NormalizeOutboundCommand`; added the matching `AddBufferedItem` normalization to the `TrackCommandReply`/`MapCommand` tracking path (its registration keys off the command's `ItemDefinition`). `AddItemBulk` tracking needs no change — the worker echoes the already-suffixed address back in each `SubscribeResult.TagAddress`, which `TrackBulkItems` stores. Authz is unchanged and consistent: `FilterTagBulkAsync` checks the bare address through `ConstraintEnforcer.ResolveTarget`'s `[]` fallback, mirroring single-add. Updated `gateway.md` and all five client READMEs (dotnet/go/python/rust/java) so the add-family normalization no longer reads as AddItem-only; the `ReadBulk` carve-out stays. Regression tests: `GatewayArrayWriteWiringTests.AddItemBulk_BareArrayAddress_NormalizedOnWireAndInRegistration`, `.AddBufferedItem_BareArrayAddress_NormalizedOnWireAndInRegistration`. +**Resolution:** 2026-06-18 — Took option (a). Root cause confirmed: `NormalizeOutboundCommand` had no `AddItemBulk`/`AddBufferedItem` case, so the worker bound bare (non-write-capable) array handles for those paths while single-add was already fixed. Added `AddItemBulk` (normalizes each `TagAddresses` entry in place) and `AddBufferedItem` (normalizes `ItemDefinition`) cases to `NormalizeOutboundCommand`; added the matching `AddBufferedItem` normalization to the `TrackCommandReply`/`MapCommand` tracking path (its registration keys off the command's `ItemDefinition`). `AddItemBulk` tracking needs no change — the worker echoes the already-suffixed address back in each `SubscribeResult.TagAddress`, which `TrackBulkItems` stores. Authz is unchanged and consistent: `FilterTagBulkAsync` checks the bare address through `ConstraintEnforcer.ResolveTarget`'s `[]` fallback, mirroring single-add. Updated `gateway.md` and all five client READMEs (dotnet/go/python/rust/java) so the add-family normalization no longer reads as AddItem-only; the `ReadBulk` carve-out stays. Regression tests: `GatewayArrayWriteWiringTests.AddItemBulk_BareArrayAddress_NormalizedOnWireAndInRegistration`, `.AddBufferedItem_BareArrayAddress_NormalizedOnWireAndInRegistration`. Re-verified present 2026-08-18 (feat/followups-tickets) — doc sub-claim only; `gateway.md` still describes normalization across the whole add family (`AddItem`/`AddItem2`/`AddItemBulk`/`AddBufferedItem`) and all five client READMEs still name the bulk and buffered members (Python in its snake_case form: `add_item_bulk` / `add_buffered_item`). ### Server-058 diff --git a/docs/AlarmProbeFindings.md b/docs/AlarmProbeFindings.md index 91afcb9..f47f7e8 100644 --- a/docs/AlarmProbeFindings.md +++ b/docs/AlarmProbeFindings.md @@ -24,13 +24,13 @@ the blocker rather than rediscovering it. |---|---| | GUID stable across polls and `ALM → RTN` | Answered — yes (2026-05-01 capture in `AlarmClientDiscovery.md`, reconfirmed 2026-08-18) | | GUID stable across clear-then-re-raise | Answered 2026-08-18 — **no**, a re-raise mints a new GUID | -| GUID stable across `UNACK → ACK` | **Open** — the rig cannot be driven into an acknowledged state at all | +| GUID stable across `UNACK → ACK` | **Open** — the rig cannot be driven into an acknowledged state; 2026-08-18 (third attempt) identifies the reason as the test attribute's `MxSecurityOperate` classification, which no non-interactive ack surface on the rig can satisfy | | `COUNT` = total active vs records-in-reply under a capped fetch | Answered 2026-08-18 — **records in the reply** | The 2026-08-17 run below is kept because it is the record of the wrong-verb blocker. The 2026-08-18 run cleared that blocker with `AuthenticateUser` + `WriteSecured` and answered two of the three questions; the acknowledge leg is now blocked on something narrower and -different, described in "Second attempt". +different, described in "Second attempt" and diagnosed in "Third attempt". ## First attempt (2026-08-17): plain `Write` @@ -258,15 +258,164 @@ MXAccess-side entry point and the wnwrap-side entry point is inert. requirement that the wnwrap consumer, which passes an operator *name* string and no authenticated identity, cannot meet. If so, ack over wnwrap is not merely untested here but unavailable by configuration, and the gateway's `AcknowledgeByName` path needs the - same treatment on any customer galaxy configured that way. + same treatment on any customer galaxy configured that way. (Answered by the Third attempt + below — enforced by the alarm attribute's `MxSecurityOperate` security classification, an + engine-level write-security setting, not a separate `alarmmgr`-side ack policy.) - If neither lands, the acknowledge leg stays assumed. It is worth restating that this is a documentation gap, not a correctness one: a re-minted GUID on acknowledge would produce a spurious Clear plus a spurious Raise, which is the same shape the now-observed re-raise behaviour produces and which `ComputeTransitions` already handles as two instances. +## Third attempt (2026-08-18): why the acknowledge is refused + +The second attempt left two candidate explanations for `AlarmAckByName` returning `rc=0` +and changing nothing: the rig enforces an acknowledgement security requirement the wnwrap +consumer cannot meet, or wnwrap's ack is simply broken here. This attempt was read-only — +no writes, no alarms raised, no configuration touched — and settles the first question: +the requirement is **enforced**, and the alarm attribute's security classification is what +enforces it. + +### Method + +Read-only inspection of the `ZB` Galaxy Repository over `sqlcmd -S localhost -d ZB -E`, +plus the already-built `mxa` CLI (`C:\Users\dohertj2\Desktop\wwtools\mxaccesscli\src\MxAccess.Cli\bin\x86\Release\net48\mxa.exe`) +for runtime reads. Nothing was written and nothing was installed. The `lmxopcua\gr` schema +notes referenced elsewhere in this repo are **not present on this box** (`Test-Path` is +`False`), so the schema was located by querying `sys.tables` / `sys.columns` directly. + +### The test attribute is classified `MxSecurityOperate` + +UDA security classification lives in `dynamic_attribute.security_classification`, keyed by +`gobject_id`. For the `$TestMachine` template that is `1055`: + +``` +SELECT gobject_id, tag_name, hierarchical_name FROM gobject WHERE gobject_id=1055; +1055|$TestMachine|$TestMachine +``` + +``` +attribute_name | security_classification | mx_attribute_category +ProtectedValue | 2 | 10 +ProtectedValue1 | 3 | 10 +TestAlarm001 | 1 | 10 +TestAlarm002 | 1 | 10 +TestAlarm003 | 1 | 10 +TestChangingInt | 1 | 10 +``` + +The enum is pinned by two independent sources rather than assumed. `ProtectedValue` and +`ProtectedValue1` are the mxaccess analysis project's documented *secured-write* and +*verified-write* fixtures (`C:\Users\dohertj2\Desktop\mxaccess\docs\galaxy-test-fixtures.md`), +and `docs/NMX-COM-Contracts.md` in the same project records "Galaxy security classification +(`2` for `SecuredWrite`, `3` for VerifiedWrite)" — so `2` and `3` land on exactly the two +attributes that are supposed to carry them. `galaxy-test-fixtures.md` also records the +provisioning verb used for every UDA in that inventory: `--security MxSecurityOperate`. +`TestAlarm001` therefore reads `1` = **`MxSecurityOperate`**. + +That is the missing piece from 2026-08-17. `Operate` is not free access: it requires an +authenticated galaxy identity holding Operate permission on the object's security group. +An unauthenticated `Write` is refused with `SecurityError` `1008`, and the same write after +`AuthenticateUser` succeeds — which is precisely the pair of results both prior attempts +recorded. Galaxy security is live on this rig, not disabled. + +`wwAlarmConsumerClass.AlarmAckByName` carries an operator *name* string and a comment. It +carries no authenticated user id, no credential, and no token — there is no parameter on +either the 6-arg or the 8-arg overload that could convey one. A consumer calling it cannot +satisfy an `Operate` classification, and `rc=0` followed by no state change is what an ack +dropped downstream of an accepted call looks like. + +The one step this stops short of is a direct experiment: the rig's security configuration +is out of scope for a read-only probe, so "an `Operate` alarm cannot be acknowledged +without an authenticated identity" is inferred from the classification plus the observed +`1008`/`rc=0` pattern rather than observed by relaxing the classification and watching the +ack start working. The human step below is exactly that experiment. + +### There is no writeable `.Ack` attribute — the second attempt targeted the only one there is + +A plausible reading of the second attempt was that it wrote the wrong sub-attribute: +ArchestrA alarm extensions are commonly described as exposing a writeable `.Ack` alongside +the read-only `.Acked`. On this galaxy they do not. Every `Ack`-named attribute the alarm +primitives define: + +``` +attribute_name | security_classification | mx_attribute_category +Acked | -1 | 2 +AckMsg | 0 | 6 +Bad.Acked | -1 | 2 +Bad.AckMsg | 0 | 6 +TimeAlarmAcked | -1 | 2 +AlarmAckCnt | -1 | 2 +AlarmAckErrorsCnt | -1 | 2 +AlarmMostUrgentAcked | -1 | 2 +AlarmUnAckedCnt | -1 | 2 +``` + +No `Ack`. `Acked` carries `security_classification = -1` — no classification at all, which +is what a non-writeable attribute carries, as distinct from the `1` on the writeable +`TestAlarm001`. That is the configuration-side counterpart of the `detail=1007` +operational refusal the second attempt got when it wrote `.Acked`: not a permission +refusal, but an attribute that has no write path to refuse. The second attempt had already +found the only MXAccess-side candidate, and it is read-only by definition. + +### No non-interactive acknowledge surface is installed + +The `wwtools` collection on the box (`aalogcli`, `aot`, `graccesscli`, `grdb`, `histdb`, +`mbproxy`, `mxaccesscli`, `secrets`) is the most likely home for a scriptable ack. There +isn't one. `mxa --help` lists `diag`, `info`, `read`, `read-batch`, `subscribe`, +`subscribe-batch`, `write`, `write-batch` — a tag data-plane only, with no alarm surface. +`graccesscli` is a Galaxy Repository configuration tool (`object uda add`, `instance +deploy`), which acts at configure/deploy time and not on live alarm state. Reading the +galaxy's own authentication mode at runtime is also unavailable: `Galaxy.AuthenticationMode` +does not resolve over MXAccess (`Category=4 Detail=6`), and the value is not in +`dynamic_attribute` — only the attribute *definition* names `AuthenticationMode` and +`_AuthenticationModeEnum` exist in `attribute_definition`. + +So every remaining acknowledge surface on this rig is interactive: the IDE's alarm client, +InTouch, or an ArchestrA graphic bound to the alarm. Driving those is out of scope. + +### Status of the acknowledge leg + +**Unavailable by configuration, and the GUID question stays assumed.** The two are separate +statements and both matter: + +- The wnwrap ack path is unavailable on this rig as configured, for an identified reason + rather than an unknown one. This is a real finding for the gateway: `AcknowledgeByName` + will behave the same way — accepted, inert — on any customer galaxy whose alarmed + attributes carry a non-free-access security classification (inferred from the mechanism — + no `AlarmAckByName` overload can carry a credential — not confirmed by relaxing the + classification and re-testing; see "What a human would need to do"). It is worth noting in + the alarm client's documentation that a silent `rc=0` is not proof of acknowledgement. +- Whether wnwrap re-mints the record GUID on `UNACK_ALM → ACK_ALM` is still unobserved, and + after three attempts it stays assumed. As the second attempt already noted, this remains + a documentation gap rather than a correctness one: a re-minted GUID produces a spurious + Clear plus a spurious Raise, the same shape the observed re-raise behaviour produces, and + `ComputeTransitions` already handles that correctly as two instances. + +#### What a human would need to do + +Either of these answers it; the second is cheaper and also confirms or refutes the +classification hypothesis above, which the read-only probe could only infer. + +1. Acknowledge `TestMachine_001.TestAlarm001` from an interactive System Platform client + (IDE alarm client, InTouch, or an ArchestrA graphic) while a wnwrap probe polls + `GetXmlCurrentAlarms2` against `\\DESKTOP-6JL3KKO\Galaxy!TestArea`, and record whether + `STATE` reaches `ACK_ALM` and whether `GUID` survives the transition. Raise the alarm + first with `AuthenticateUser` + `WriteSecured` as the second attempt did. +2. Reclassify `TestAlarm001` on the `$TestMachine` template to free access and redeploy — + `graccesscli object uda ... --security MxSecurityFreeAccess` against `$TestMachine`, + then `instance deploy TestMachine_001` — and re-run the second attempt's ack probe + unchanged. If `AlarmAckByName` then moves `STATE` to `ACK_ALM`, the classification is + confirmed as the blocker and the GUID question is answered in the same run. Restore the + classification to `MxSecurityOperate` afterwards, since the secured-write fixtures in + `WorkerLiveMxAccessSmokeTests` depend on the alarm UDAs being secured. + ### Rig state left behind -The three `TestMachine_00{1,2,3}.TestAlarm001` UDAs are back to `false` and their +The third attempt changed nothing. `TestMachine_001.TestAlarm001` and its `.Acked` subtag +both read `false` at the end of the session, matching the state the second attempt left. + +The second attempt's inventory, unchanged since: the three +`TestMachine_00{1,2,3}.TestAlarm001` UDAs are back to `false` and their `.InAlarm` subtags read `false`, but each leaves a `UNACK_RTN` record in the wnwrap snapshot, since nothing can acknowledge them away. `SnapshotActiveAlarms` counts only `UNACK_ALM` and `ACK_ALM` as active, so these are inert for the gateway; they will clear on diff --git a/docs/Authentication.md b/docs/Authentication.md index 02c1206..bdae026 100644 --- a/docs/Authentication.md +++ b/docs/Authentication.md @@ -175,6 +175,17 @@ optional serialized constraints, and the `created_utc`, `last_used_utc`, belong to `ZB.MOM.WW.Auth.ApiKeys`, this document does not restate their column readers or SQL; consult the library for that detail. +One connection-level behavior is worth stating here because it is what keeps the +store usable under concurrency rather than an implementation detail of the schema: +the library's `AuthSqliteConnectionFactory.OpenConnectionAsync` opens pooled +connections with a non-zero command timeout and applies `PRAGMA journal_mode=WAL` +and `PRAGMA busy_timeout` (5 s). Last-used stamping runs on every authenticated +request and the audit store appends on every denial, so without WAL and a busy +timeout those concurrent writers would surface `SQLITE_BUSY` as a hard failure on +the request path instead of degrading gracefully. WAL is a persistent +database-level setting, so re-applying it per connection is a cheap no-op; +`busy_timeout` is per-connection state and must be set each time. + ### Audit trail The library emits its own API-key audit entries (from the admin verbs — create, @@ -260,9 +271,9 @@ Examples: ```bash mxgateway apikey init-db -mxgateway apikey create-key --key-id ops.alice --display-name "Alice (ops)" --scopes invoke:read,invoke:write -mxgateway apikey create-key --key-id area1.reader --display-name "Area 1 reader" --scopes invoke:read,metadata:read --read-subtree "Area1/*" --browse-subtree "Area1/*" -mxgateway apikey create-key --key-id ops.temp --display-name "Temp contractor" --scopes invoke:read --expires 90d +mxgateway apikey create-key --key-id ops.alice --display-name "Alice (ops)" --scopes session:open,invoke:read,invoke:write +mxgateway apikey create-key --key-id area1.reader --display-name "Area 1 reader" --scopes session:open,invoke:read,metadata:read --read-subtree "Area1/*" --browse-subtree "Area1/*" +mxgateway apikey create-key --key-id ops.temp --display-name "Temp contractor" --scopes session:open,invoke:read --expires 90d mxgateway apikey create-key --key-id team-a.svc --display-name "Team A service" --scopes session:open,invoke:read --dashboard-tags team-a mxgateway apikey create-key --key-id ops.audit --display-name "Audit window" --scopes metadata:read --expires 2027-01-01T00:00:00Z mxgateway apikey list-keys --json diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index 4ce63b8..5970259 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -193,8 +193,8 @@ events (a "gap") and must re-snapshot; whatever is still retained is replayed. | `MxGateway:Dashboard:RecentFaultLimit` | `100` | Maximum number of fault summaries projected into each dashboard snapshot. | | `MxGateway:Dashboard:RecentSessionLimit` | `200` | Maximum number of session summaries projected into each dashboard snapshot. | | `MxGateway:Dashboard:ShowTagValues` | `false` | Controls whether tag values reach the dashboard at all. `false` (the default) suppresses them on all three seams that carry one: (1) the **events hub mirror** — `DashboardEventBroadcaster` blanks `MxEvent.value` plus the alarm body's `current_value`/`limit_value` from a deep-cloned copy before it reaches any hub subscriber (see `docs/GatewayDashboardDesign.md`'s `EventsHub` row); (2) the **alarms hub** — `AlarmsHubPublisher` clears `current_value`/`limit_value` from a deep-cloned copy of each `AlarmFeedMessage`, on both value-bearing payload arms (`transition` and `active_alarm`), before broadcasting to `/hubs/alarms`; (3) the **`/browse` live-value panel** — `DashboardLiveDataService` substitutes the literal `[redacted]` for the value text of each **successfully read** tag; a failed read keeps its `-` placeholder (there was no value to suppress, and the row's error is left untouched, so the two cannot contradict each other). Both hub redactions clone: the source `MxEvent` is shared with the gRPC event stream and the replay ring, and the source `AlarmFeedMessage` fans out to gRPC `StreamAlarms` subscribers, so neither is mutated in place and **no gRPC client is affected by this flag** — it is a dashboard-display control only. Everything that is not the value survives on every seam: tag reference, alarm reference/severity/state/operator, data type, quality, status, and timestamps still render, so the dashboard stays diagnostic without disclosing process values. This is one of two independent layers: `IDashboardSessionAcl` decides *which* sessions a caller may subscribe to at all (see `GroupToTag` / `UntaggedSessionVisibility` below), while this flag decides what a permitted subscriber sees. Setting it `true` exposes tag values to everyone the ACL admits — every Administrator, plus each Viewer holding a matching tag — and, because the alarms hub is session-less, exposes alarm values to every dashboard client that can reach `/hubs/alarms`. | -| `MxGateway:Dashboard:GroupToRole` | _(empty)_ | LDAP group → dashboard role mapping. Keys are LDAP group names (short CN or full DN — leading-RDN match). Values must be `Administrator` (read/write, API-key CRUD) or `Viewer` (read-only) — matched ordinally by the startup validator, so the spelling is exact and `Admin` is rejected. A user whose LDAP groups don't intersect this map cannot sign in; with no mapping at all, only the loopback bypass admits anyone. | -| `MxGateway:Dashboard:GroupToTag` | _(empty)_ | LDAP group → dashboard visibility tags. Keys follow the same convention as `GroupToRole` (short CN or full DN — leading-RDN match, case-insensitive); values are tag lists. A dashboard user's granted tag set is the union over the groups they belong to; an unmapped group contributes nothing. **Visibility only:** tags scope which sessions' event streams a Viewer may observe on the dashboard — they never grant or deny data access, which stays with the API key's scopes and constraints. Independent of `GroupToRole`: a group may appear in either map, both, or neither. Empty (the default) means Viewers hold no tags, so under the default `UntaggedSessionVisibility` they observe no session's events. | +| `MxGateway:Dashboard:GroupToRole` | _(empty)_ | LDAP group → dashboard role mapping. Keys must be **short** LDAP group names — the CN / first-RDN value, e.g. `GwAdmin`, not `ou=GwAdmin,ou=groups,dc=zb,dc=local` — matched case-insensitively. The shared `ZB.MOM.WW.Auth.Ldap` provider delivers a user's groups already stripped to short names, so a full-DN key can never match and the group silently maps to nothing. (The mapper does try the full literal string before falling back to the leading-RDN value, but on the live login path both branches see the same short name; the fallback only matters to non-library callers of the `IGroupRoleMapper` seam.) Values must be `Administrator` (read/write, API-key CRUD) or `Viewer` (read-only) — matched ordinally by the startup validator, so the spelling is exact and `Admin` is rejected. A user whose LDAP groups don't intersect this map cannot sign in; with no mapping at all, only the loopback bypass admits anyone. | +| `MxGateway:Dashboard:GroupToTag` | _(empty)_ | LDAP group → dashboard visibility tags. Keys follow the same convention as `GroupToRole` — short LDAP group names (CN / first-RDN value), matched case-insensitively; a full-DN key never matches the pre-stripped groups the LDAP provider returns. Values are tag lists. A dashboard user's granted tag set is the union over the groups they belong to; an unmapped group contributes nothing. **Visibility only:** tags scope which sessions' event streams a Viewer may observe on the dashboard — they never grant or deny data access, which stays with the API key's scopes and constraints. Independent of `GroupToRole`: a group may appear in either map, both, or neither. Empty (the default) means Viewers hold no tags, so under the default `UntaggedSessionVisibility` they observe no session's events. | | `MxGateway:Dashboard:UntaggedSessionVisibility` | `AdminOnly` | Who may observe a session that carries no tags (its owning API key declared none). `AdminOnly` (default, fail-closed) restricts untagged sessions to dashboard Administrators. `AllViewers` shows them to every Viewer — opt-in for a single-tenant deployment that wants the pre-tag behaviour. Administrators always see every session regardless of tags. | | `MxGateway:Dashboard:DisableLogin` | `false` | Dev/test only. When `true`, replaces the cookie authentication handler with `DashboardAutoLoginAuthenticationHandler`, which auto-authenticates every dashboard request — including requests from remote browsers, not just loopback — as `AutoLoginUser` holding both `Administrator` and `Viewer` roles. No login form, LDAP bind, or cookie is involved. A loud one-time startup warning is logged. Differs from `AllowAnonymousLocalhost`: `DisableLogin` mints a real authenticated principal (so role-gated write affordances appear), whereas `AllowAnonymousLocalhost` satisfies the authorization requirement on loopback only without minting a principal (write affordances stay hidden). Never enable in production. **Production hard-stop (SEC-04):** when the host runs in the `Production` environment and `DisableLogin` is `true`, startup validation fails and the process aborts — the flag is only accepted outside Production, where the one-time startup warning still fires. | | `MxGateway:Dashboard:AutoLoginUser` | `(null)` | Username stamped on the synthetic principal when `DisableLogin` is `true`. Default `(null)` — a null or blank value falls back to `multi-role`. Has no effect when `DisableLogin` is `false`. | diff --git a/docs/GatewayDashboardDesign.md b/docs/GatewayDashboardDesign.md index f69b8c0..abbc3f8 100644 --- a/docs/GatewayDashboardDesign.md +++ b/docs/GatewayDashboardDesign.md @@ -186,7 +186,7 @@ hubs stay for the audience that genuinely needs a wire. |---|---|---| | every page deriving from `DashboardPageBase` | `IDashboardSnapshotFeed.WatchAsync` | `DashboardSnapshotFeed` (singleton) multicasting one `IDashboardSnapshotService.WatchSnapshotsAsync` enumeration | | `SessionDetailsPage` | `IDashboardSessionEventSubscriber.Subscribe(sessionId)` | `DashboardEventBroadcaster` — the same singleton the session mirror publishes to, registered behind both interfaces | -| `AlarmsPage` | `IGatewayAlarmService.StreamAsync` | the central alarm monitor, **provider status only**; the alarm rows still come from the 3 s `QueryAlarmsAsync` poll | +| `AlarmsPage` | `IGatewayAlarmService.StreamAsync` | the central alarm monitor, **gateway-status frames only** — `provider_status` for the badge and `snapshot_status` for the truncated-snapshot banner; the alarm rows still come from the 3 s `QueryAlarmsAsync` poll | The snapshot feed multicasts rather than handing each page its own enumeration: `WatchSnapshotsAsync` is not multicast on its own — each enumeration owns a timer @@ -245,8 +245,9 @@ cancelling, a detach-driven exit leaves the pill to the incoming subscription; w the pill therefore reports is the case it exists for — the channel completing under a page that is still watching. -`AlarmsPage` owns two loops of its own (the 3 s alarm poll and the provider-status -badge) and bounds their drain at 5 seconds on dispose, for the same reason +`AlarmsPage` owns two loops of its own (the 3 s alarm poll and the status feed that +drives the provider badge and the truncated-snapshot banner) and bounds their drain +at 5 seconds on dispose, for the same reason `DashboardPageBase` bounds its watch drain: both loops render through the renderer's dispatcher, and disposal can run on it. The two are drained concurrently, so the bound on disposal is 5 seconds in total rather than per loop — a wedged dispatcher @@ -285,11 +286,18 @@ Both seams consume the same producing services, so they share these cadences: - alarm publisher emits on each transition observed by the central monitor; - event publisher emits per event fanned by the session's `SessionEventDistributor` to its internal dashboard-mirror subscriber (independent of any gRPC `StreamEvents`); -- the alarms page's provider-status badge resubscribes one second after its +- the alarms page's status feed resubscribes one second after its `IGatewayAlarmService.StreamAsync` enumeration ends — the monitor completes a - subscriber's stream when it falls behind and again when it restarts, both - recoverable by resubscribing — and holds its last value in between. The page's - alarm rows are independent of that stream and refresh on the 3 s poll. + subscriber's stream only when that subscriber has fallen behind, which resubscribing + recovers (a monitor restart keeps the channel and pushes cleared status frames through + it) — and the badge and banner hold their last values in between. That feed carries both gateway-status frames: `provider_status` drives the + badge, and `snapshot_status` drives the truncated-snapshot banner, so the caveat + appears on the monitor's verdict change rather than up to three seconds later. Every + subscriber is primed with a `snapshot_status` frame at open, so a page attaching + mid-truncation needs no priming logic of its own. The page's alarm rows are + independent of that stream and refresh on the 3 s poll, which also re-asserts the + truncation verdict as its reconcile baseline — both sources read the same monitor + verdict, and neither is synthesized page-side. ### Idle gating and snapshot cost @@ -540,8 +548,11 @@ alarm-history store, so the page reflects only the live active set. The page is read-only; it does not acknowledge alarms. A provider-status badge tracks the central monitor's health from `IGatewayAlarmService.StreamAsync` in process — the alarm service is already a multi-subscriber fan-out, so the badge needs no SignalR -client, no loopback socket, and no hub token — while the alarm rows themselves -still come from the three-second poll. If `MxGateway:Alarms:Enabled` is +client, no loopback socket, and no hub token — and the same subscription carries the +`snapshot_status` frame behind the truncated-snapshot banner ("Alarm snapshot may be +incomplete"), so a capped provider fetch is caveated the moment the monitor decides +it. The alarm rows themselves still come from the three-second poll, which also +re-asserts the truncation verdict as the reconcile baseline. If `MxGateway:Alarms:Enabled` is false the central monitor never starts, and the page says so instead of showing an empty list with no explanation. @@ -679,8 +690,10 @@ Implemented behavior: - a static `/login` HTML form posts username/password to the gateway; - `DashboardAuthenticator` binds against `MxGateway:Ldap` (service-account bind, user search, candidate bind) using `Novell.Directory.Ldap.NETStandard`; -- the user's `memberOf` (or short CN) is matched against - `MxGateway:Dashboard:GroupToRole`; the resolved role(s) are emitted as +- the user's groups arrive from the LDAP provider already stripped to short + names (the CN / first-RDN value of each `memberOf` entry) and are matched + against the short-name keys of `MxGateway:Dashboard:GroupToRole` — a full-DN + key there never matches; the resolved role(s) are emitted as `ClaimTypes.Role` claims, alongside the per-group `mxgateway:ldap_group` claims; - a successful login signs in the `MxGateway.Dashboard` cookie scheme diff --git a/docs/Grpc.md b/docs/Grpc.md index 6d7794e..e548e91 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -92,6 +92,8 @@ An accepted gRPC command payload can still be too large for the worker pipe: the `AcknowledgeAlarm` is a unary, **session-less** RPC that acknowledges a single alarm. The handler validates `alarm_full_reference` inline (it does not run through `MxAccessGrpcRequestValidator`) and delegates to `IGatewayAlarmService.AcknowledgeAsync`. The always-on `GatewayAlarmMonitor` routes the ack over its own gateway-managed worker session — clients no longer open a session to acknowledge an alarm. A reference that parses as a canonical GUID forwards to `AcknowledgeAlarmCommand`; a `Provider!Group.Tag` reference forwards to `AcknowledgeAlarmByNameCommand`. +An `OK` response means the alarm provider accepted the acknowledgement, not that it applied it: the gateway forwards the ack and reports what the provider returned, in keeping with the parity rule. On galaxies whose alarmed attributes carry a non-free-access security classification the by-name ack is accepted and inert — the underlying `AlarmAckByName` conveys an operator name rather than an authenticated identity, observed on the probe rig with the mechanism inferred rather than confirmed — so a client that needs to confirm an acknowledgement should watch for the resulting transition on `StreamAlarms` instead of treating `OK` as proof. See [Alarm Probe Findings](./AlarmProbeFindings.md). + ### `StreamAlarms` `StreamAlarms` is a server-streaming, **session-less** RPC that attaches to the gateway's central alarm feed. The handler delegates to `IGatewayAlarmService.StreamAsync`. The stream opens with a `provider_status` and a `snapshot_status` `AlarmFeedMessage` (the current provider mode and snapshot-completeness verdict), then one `AlarmFeedMessage` carrying an `active_alarm` per currently-active alarm (the ConditionRefresh snapshot), then a single `snapshot_complete`, then a `transition` for every subsequent raise / acknowledge / clear — interleaved with a further `provider_status` on each failover/failback and a further `snapshot_status` on each change of the truncation verdict. It is served by the always-on `GatewayAlarmMonitor`, which owns a single gateway-managed worker session and fans out to every attached client — clients no longer open a session of their own. `alarm_filter_prefix`, when set, scopes the stream to a sub-tree. diff --git a/docs/ToolchainLinks.md b/docs/ToolchainLinks.md index d5859f8..e4a0ac7 100644 --- a/docs/ToolchainLinks.md +++ b/docs/ToolchainLinks.md @@ -69,8 +69,8 @@ a .NET Framework or COM interop build needs classic Visual Studio MSBuild. | Tool | Version | Path | | --- | --- | --- | | Go | 1.26.2 windows/amd64 | `C:\Program Files\Go\bin\go.exe` | -| protoc-gen-go | latest installed by `go install` | `C:\Users\dohertj2\go\bin\protoc-gen-go.exe` | -| protoc-gen-go-grpc | latest installed by `go install` | `C:\Users\dohertj2\go\bin\protoc-gen-go-grpc.exe` | +| protoc-gen-go | v1.36.11 (pinned) | `C:\Users\dohertj2\go\bin\protoc-gen-go.exe` | +| protoc-gen-go-grpc | 1.6.2 (pinned) | `C:\Users\dohertj2\go\bin\protoc-gen-go-grpc.exe` | Environment: @@ -80,11 +80,14 @@ GOPATH=C:\Users\dohertj2\go Go plugin bin=C:\Users\dohertj2\go\bin ``` -Installed plugin commands: +Installed plugin commands. Install the pinned versions, not `@latest`: the committed Go +bindings stamp the plugin version in their headers, so `clients/go/generate-proto.ps1` +throws when a plugin reports anything else. That script is the source of truth for the +pin — change it there first, then update this table. ```powershell -go install google.golang.org/protobuf/cmd/protoc-gen-go@latest -go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest +go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11 +go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2 ``` ## Rust diff --git a/docs/plans/2026-08-17-followup-closeout.md b/docs/plans/2026-08-17-followup-closeout.md index 4847a40..4cc0cbe 100644 --- a/docs/plans/2026-08-17-followup-closeout.md +++ b/docs/plans/2026-08-17-followup-closeout.md @@ -480,22 +480,40 @@ design; they need only a cheap tip re-build there. rig shows the new GUID replaces the record. The prose that implied coexistence was corrected rather than the code. -Follow-ups recorded, not started: +Follow-ups recorded, not started — **all closed 2026-08-18 on `feat/followups-tickets`** +(plan `docs/plans/2026-08-18-followups-and-tickets.md`; per-bullet closing commits below): - `WorkerPipeSessionTests.RunAsync_LongInFlightCommandThatKeepsPumping_DoesNotFaultAndDeliversReply` fails deterministically on windev **and on `main`** — pre-existing, needs its own - investigation. + investigation. *Closed (`7da52b6`, `462850a`, `aaeb86b`): test-harness defect — the fake + runtime session stamped STA activity only at construction, so the watchdog correctly + faulted `StaHung` pre-dispatch. Test-only fix; windev worker suite fully green (524/11 + skipped) for the first time.* - `check-codegen.ps1` Check 4 is unrunnable on Windows: the `protoc-gen-go` version banner carries a `.exe` suffix that the exact-string compare in - `clients/go/generate-proto.ps1:10,55` does not tolerate. -- Windev has `protoc-gen-go-grpc` 1.6.1 against the repo's pinned 1.6.2. + `clients/go/generate-proto.ps1:10,55` does not tolerate. *Closed (`c94c4d4`, plus + `8ae0c2f` for a second Windows-only blocker found during verification: PS 5.1 strips + embedded double quotes from the Python probe's `-c` argument). Check 4 verified 4/4 on + windev under both PowerShell 5.1 and pwsh 7.* +- Windev has `protoc-gen-go-grpc` 1.6.1 against the repo's pinned 1.6.2. *Closed: 1.6.2 + installed on windev; `docs/ToolchainLinks.md` corrected from `@latest` to the pinned + install commands (`6b5c737`).* - `clients/java`'s `checkGeneratedClean` is dead under Gradle 9 (`Project.exec` was - removed); it needs `ExecOperations` injection to work again. + removed); it needs `ExecOperations` injection to work again. *Closed (`df45cb4`) via + `ProviderFactory.exec`; verified on Gradle 9.5.1 (macOS) and 9.4.1 (windev), including + a configuration-cache ordering proof.* - `SettingsPage` renders every `EffectiveDashboardConfiguration` member except `RecentFaultLimit` / `RecentSessionLimit` (pre-existing, predates this branch). + *Closed (`a390fe1`, test tightened in `fb68bdb`).* - The ack-leg probe stays blocked; unblock paths are in `docs/AlarmProbeFindings.md`. + *Closed as answered-why (`d1ae43d`, `bc22792`, `1605f54`): the ack is unavailable by + configuration — the test attributes carry `MxSecurityOperate` and no `AlarmAckByName` + overload can carry a credential (inferred, caveated). The GUID-across-ack question + itself stays assumed; the remaining paths need a human at an interactive client.* - The dashboard `AlarmsPage` truncation banner is still poll-driven — it could consume - the new `snapshot_status` feed frame instead. + the new `snapshot_status` feed frame instead. *Closed (`7b6dfba`, `f57a6ae`): the + page's status feed loop now consumes `snapshot_status`; the poll stays as reconcile + baseline.* - **A closed code-review finding regressed, or was never applied.** Server-012 (`code-reviews/Server/findings.md:405-412`) is recorded *Resolved 2026-05-18* and claims it corrected two scope lists to the canonical `*:*` strings — the `CLAUDE.md` @@ -504,7 +522,9 @@ Follow-ups recorded, not started: with a third instance the finding never covered (`docs/Authentication.md`'s `ops.alice` example). The bookkeeping is the follow-up: other `Server-0xx` entries marked Resolved with documentation-only fixes should be spot-checked for the same pattern, since a - finding that reads Resolved is not otherwise re-examined. + finding that reads Resolved is not otherwise re-examined. *Closed (`d3ac527`): 20 + doc-only resolutions audited; two more regressions found and re-fixed (Server-040, + Server-009); four moot (target files deleted); annotations recorded in findings.md.* Explicitly decided, not an omission: **`../scadaproj/CLAUDE.md` needs no update.** The umbrella index records the *set* of `.proto` files this repo owns, and that set is diff --git a/docs/plans/2026-08-18-followups-and-tickets.md b/docs/plans/2026-08-18-followups-and-tickets.md new file mode 100644 index 0000000..dda175c --- /dev/null +++ b/docs/plans/2026-08-18-followups-and-tickets.md @@ -0,0 +1,526 @@ +# Follow-ups and Tickets Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development +> to execute this plan task-by-task in this session. + +**Goal:** Close every follow-up recorded in `docs/plans/2026-08-17-followup-closeout.md` +(as-built notes, "Follow-ups recorded, not started") plus the two ticket-worthy windev +findings — leaving no recorded open item anywhere. + +**Architecture:** No new components. Ten independent closures: two toolchain/script fixes +(Check 4 Windows pin, Gradle 9 `checkGeneratedClean`), two dashboard display gaps +(Settings recent-limit rows, push-driven Alarms truncation banner), two documentation +audits (Server-0xx resolution regression sweep, `Authentication.md` runnable-examples +pass), one worker-test investigation (the deterministic `WorkerPipeSessionTests` failure +on windev), one bounded rig probe (ack-leg unblock paths), a windev verification pass +(including the `protoc-gen-go-grpc` 1.6.2 pin install), and bookkeeping. + +**Tech Stack:** PowerShell (codegen scripts), Gradle/Groovy (Java client), Blazor +server-side Razor + xUnit/bUnit-style render tests (dashboard), .NET Framework 4.8 x86 +xUnit (worker tests, windev-only), wnwrap probe harness (windev rig), Markdown. + +**Branch:** `feat/followups-tickets` off `main` (`45058d5`). + +--- + +## Ground rules for every implementer subagent + +- **Git discipline (Mac tree):** NEVER `git stash`, `git reset`, `git clean`, or + `git checkout `. Commit with **pathspecs on the commit**: + `git commit -m "..." -- `. If `index.lock` blocks you, wait 5 s and retry. +- **Build/test lock:** before any `dotnet build`/`dotnet test`/`cargo`/`gradle`/`go test` + on the Mac, acquire the lock: + `mkdir /private/tmp/claude-501/-Users-dohertj2-Desktop-MxAccessGateway/f36938ae-bbca-4245-b5c9-fac512d69e22/scratchpad/buildlock` + (retry with backoff until it succeeds); `rmdir` it on ALL exit paths, including failure. +- **Quality gates:** `TreatWarningsAsErrors=true`, `Nullable=enable`. Follow + `docs/style-guides/CSharpStyleGuide.md` (file-scoped namespaces, `sealed` by default, + `Async` suffix). Update affected docs in the same commit as source. +- **MXAccess parity:** never synthesize events; the dashboard/gateway forwards only what + the worker/monitor emits. The `snapshot_status` frame is a gateway-status frame (like + `provider_status`), NOT a synthesized MXAccess event. +- **Never log or echo secrets, API keys, credentials, or tag values.** +- **Scope contract:** the task's `Files:` block is the `files_to_edit` contract. If the + task needs other files, that is a plan defect — surface it in your report, don't + silently expand scope. (Exception: Tasks 7 and 8 are investigations; their Files list + is starting points, and they must report every file they end up touching.) +- **Windev access (Tasks 7, 8, 9 only):** `ssh windev` lands in PowerShell. The CI clone + is `C:\build\mxaccessgw-ci`. Quirks: the first `slnx` build after a pull often fails on + stale Contracts obj artifacts (CS2001/CS0016) — clear the Contracts `obj`/`bin` and + rebuild, it is not a regression; the gateway suite is load-sensitive — rerun a filtered + subset before believing a flake. To get branch code there: + `git -C C:\build\mxaccessgw-ci fetch origin && git -C C:\build\mxaccessgw-ci checkout feat/followups-tickets && git -C C:\build\mxaccessgw-ci pull` — + which requires the Mac side to have pushed the branch first (the controller pushes; + ask if the branch tip you need is not on origin). + +--- + +### Task 1: check-codegen Check 4 — tolerate the Windows `.exe` version banner + +**Classification:** small +**Estimated implement time:** ~4 min +**Parallelizable with:** Task 2, Task 3, Task 4, Task 5, Task 7 + +**Files:** +- Modify: `clients/go/generate-proto.ps1` (pin compare at `:52-67`, pin constants at `:10-11`) +- Modify: any doc that states Check 4 is unrunnable on Windows (grep `docs/` and + `code-reviews/` for "Check 4"; `docs/GatewayTesting.md` is the likely holder) + +**Problem.** On Windows the plugins report their invoked name with an `.exe` suffix +(`protoc-gen-go.exe v1.36.11`), so the exact-string compare against +`'protoc-gen-go v1.36.11'` throws and Check 4 (which shells this script) is unrunnable on +any Windows host — including windev, the box where regeneration actually happens. + +**Step 1: Normalize the banner before comparing.** Add a small helper and use it for both +plugin compares (protoc stays warn-only and gets the same normalization for a fair warn): + +```powershell +function Get-NormalizedToolVersion { + # On Windows a plugin reports its argv[0] name, so the banner carries an `.exe` + # suffix ("protoc-gen-go.exe v1.36.11"). Strip it so the pin compare is + # host-independent; the version part must still match exactly. + param([string]$RawBanner) + return ($RawBanner -replace '\.exe(?=\s)', '') +} +``` + +Apply to `$protocGenGoVersion`, `$protocGenGoGrpcVersion`, `$protocVersion` at the point +each is read (`:54`, `:59`, `:64`), keeping the pinned constants unchanged. Update the +comment block at `:4-9` to note the normalization. + +**Step 2: Verify on macOS.** Acquire the build lock, then run +`pwsh scripts/check-codegen.ps1` — all 4 checks must pass (banner has no `.exe` here, so +this proves no regression). Also assert the helper logic directly: +`pwsh -c "& { ; Get-NormalizedToolVersion 'protoc-gen-go.exe v1.36.11' }"` +must print `protoc-gen-go v1.36.11`, and a name-only match check for +`protoc-gen-go-grpc.exe 1.6.2` → `protoc-gen-go-grpc 1.6.2`. + +**Step 3: Fix the stale prose.** Wherever docs state Check 4 cannot run on Windows, +rewrite to: Check 4 runs on Windows; the plugin pins must be installed +(`go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11`, +`go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2`). +Do NOT edit `docs/plans/2026-08-17-followup-closeout.md` — Task 10 owns plan bookkeeping. + +**Step 4: Commit** with pathspecs on the touched files: +`fix(codegen): normalize .exe off plugin version banners so Check 4 runs on Windows`. + +Windows-side proof is deferred to Task 9 (windev runs check-codegen 1–4 after installing +the 1.6.2 pin). + +--- + +### Task 2: Gradle 9 — revive `checkGeneratedClean` without `Project.exec` + +**Classification:** small +**Estimated implement time:** ~4 min +**Parallelizable with:** Task 1, Task 3, Task 4, Task 5, Task 7 + +**Files:** +- Modify: `clients/java/zb-mom-ww-mxgateway-client/build.gradle` (`checkGeneratedClean`, `:71-92`) +- Modify: `docs/GatewayTesting.md` only if it describes the task's mechanism (grep + `checkGeneratedClean`) + +**Problem.** The task's `doLast` calls the script-level `exec {}` (i.e. `Project.exec`), +removed in Gradle 9 — the task is dead on any Gradle 9 host. + +**Step 1: Replace with `ProviderFactory.exec`** (available since Gradle 7.5, so it works +on both the current toolchain and Gradle 9, and is configuration-cache safe). Capture the +provider reference at configuration time, call `.get()` in `doLast` so the git status +runs at execution time: + +```groovy +tasks.register('checkGeneratedClean') { + group = 'verification' + description = 'Fails if the committed generated Java tree differs from a fresh regeneration.' + dependsOn 'generateProto' + def generatedDir = 'clients/java/src/main/generated' + def repoRoot = rootProject.projectDir.parentFile.parentFile + // Project.exec was removed in Gradle 9; ProviderFactory.exec runs the probe lazily + // at doLast time and works on Gradle 7.5+. + def gitStatus = providers.exec { + workingDir = repoRoot + commandLine 'git', 'status', '--porcelain', '--', generatedDir + ignoreExitValue = true + } + doLast { + def dirty = gitStatus.standardOutput.asText.get().trim() + if (!dirty.isEmpty()) { + throw new GradleException( + "Generated Java is stale:\n${dirty}\n" + + "Regenerate and commit the Java client after a .proto change " + + "(gradle :zb-mom-ww-mxgateway-client:generateProto).") + } + } +} +``` + +Preserve the explanatory comment block above the task (`:61-70`) — amend it, don't delete. + +**Step 2: Verify.** Acquire the build lock. From `clients/java`: +`gradle :zb-mom-ww-mxgateway-client:checkGeneratedClean` must pass (tree is clean), and +`gradle --version` must be recorded in your report. Then prove the failure path: touch a +scratch edit inside `clients/java/src/main/generated/` (append a comment line to one +generated file), rerun the task, confirm it fails with the stale message, then **revert +that file with `git checkout -- `** (this narrow file-level checkout is the +one permitted use; the file is committed generated output). + +**Step 3: Commit:** +`fix(java-client): checkGeneratedClean via ProviderFactory.exec — Project.exec is gone in Gradle 9`. + +--- + +### Task 3: SettingsPage — RecentFaultLimit / RecentSessionLimit rows + +**Classification:** small +**Estimated implement time:** ~4 min +**Parallelizable with:** Task 1, Task 2, Task 4, Task 5, Task 7 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SettingsPage.razor` + (dashboard rows around `:98-102`) +- Test: `src/ZB.MOM.WW.MxGateway.Tests/` — extend the existing settings render coverage + (`SettingsPageTagVisibilityRenderTests` or its sibling settings render test class; find + with `grep -rl "SettingsPage" src/ZB.MOM.WW.MxGateway.Tests`) +- Modify: `docs/GatewayDashboardDesign.md` only if it enumerates settings rows (grep + `RecentFaultLimit` / "Snapshot interval" there first) + +**Problem.** `EffectiveDashboardConfiguration` carries `RecentFaultLimit` and +`RecentSessionLimit` (already projected by `GatewayConfigurationProvider.cs:62-63`), but +`SettingsPage` renders every member except these two. + +**Step 1: Write the failing test.** Extend the settings render test: rendered page +contains `Recent fault limit` with the configured value and `Recent session limit` with +the configured value (use non-default values in the arranged options so the assertion +proves plumbing, not defaults). + +**Step 2: Run it, expect FAIL** (`dotnet test --filter "FullyQualifiedName~SettingsPage"` +under the build lock). + +**Step 3: Add the two rows** next to the other Dashboard rows (after "Snapshot interval", +matching the existing `……` idiom): + +```razor +Recent fault limit@Snapshot.Configuration.Dashboard.RecentFaultLimit +Recent session limit@Snapshot.Configuration.Dashboard.RecentSessionLimit +``` + +**Step 4: Run the filtered test, expect PASS.** + +**Step 5: Commit:** +`feat(dashboard): settings page shows RecentFaultLimit and RecentSessionLimit`. + +--- + +### Task 4: Authentication.md — runnable-as-written examples pass + +**Classification:** small +**Estimated implement time:** ~5 min +**Parallelizable with:** Task 1, Task 2, Task 3, Task 5, Task 7 + +**Files:** +- Modify: `docs/Authentication.md` + +**Problem.** The doc's CLI/scope examples were flagged during the previous branch as not +runnable as written — notably samples that grant only invoke/event scopes with no +`session:open`, which since Server-004's validation would produce keys that cannot open a +session (or, for unknown scope strings, be rejected at create time). The canonical scope +catalog is: `session:open`, `session:close`, `invoke:read`, `invoke:write`, +`invoke:secure`, `events:read`, `metadata:read`, `admin`; the verb is `apikey create-key` +with `--key-id` required. + +**Step 1: Sweep every example** (` ```-fenced blocks and inline commands) in the doc. For +each, either (a) make it runnable as written — canonical verb, required flags, only +canonical scopes, scope sets that support what the surrounding prose says the key is for +(a key described as opening sessions needs `session:open`) — or (b) if it is deliberately +a fragment, mark it explicitly (e.g. "illustrative — not a complete command"). Prefer (a); +use (b) only where completeness would obscure the point being made. + +**Step 2: Cross-check** each corrected scope list against +`src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/GatewayScopes.cs` (read-only) — +do not invent scope strings. + +**Step 3: Commit:** `docs(auth): make Authentication.md examples runnable as written`. + +--- + +### Task 5: AlarmsPage — push-driven truncation banner from the snapshot_status frame + +**Classification:** standard +**Estimated implement time:** ~5 min +**Parallelizable with:** Task 1, Task 2, Task 3, Task 4, Task 7 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor` + (in-process feed loop around `:199-230`, `_snapshotTruncated` at `:170`/`:401`) +- Test: `src/ZB.MOM.WW.MxGateway.Tests/` — the AlarmsPage/dashboard alarm test class + (find with `grep -rl "AlarmsPage" src/ZB.MOM.WW.MxGateway.Tests`); if no AlarmsPage + render/behavior test exists, add coverage at the level the existing dashboard tests use +- Modify: `docs/GatewayDashboardDesign.md` (truncation-banner description, if it says the + banner is poll-driven) + +**Problem.** The truncation banner (`_snapshotTruncated`, rendered at `:39`) updates only +from the 3-second poll (`:401`), even though the page already holds an in-process +subscription to the alarm feed (the loop at `:211-213` that feeds the provider-status +badge). The feed now carries an edge-triggered `snapshot_status` frame +(`AlarmFeedMessage.PayloadCase.SnapshotStatus`, shipped `fccf753`) — consume it so the +banner flips on the edge instead of up to 3 s late. + +**Spec:** +- In the existing in-process feed loop, add a case for + `AlarmFeedMessage.PayloadCase.SnapshotStatus`: set `_snapshotTruncated = + message.SnapshotStatus.Truncated` and re-render (same invoke/StateHasChanged pattern the + provider-status case uses). +- Keep the poll's assignment at `:401` — the poll is the reconcile baseline and both + sources derive from the same `GatewayAlarmMonitor` verdict, so they cannot disagree + except transiently. Do not remove or restructure the poll. +- Remember `StreamAsync` primes every subscriber with an unconditional `snapshot_status` + baseline frame after `provider_status` — so on attach the page gets the current verdict + push-side too. No extra priming logic needed in the page. +- Do NOT touch `GatewayAlarmMonitor` or the proto — gateway emission is done and shipped. + +**Steps:** failing test first (a `SnapshotStatus` frame delivered through the in-process +subscription flips the banner state without a poll tick; a `false` frame clears it), then +implement, then filtered dashboard/alarm tests green under the build lock, then commit: +`feat(dashboard): alarms page consumes snapshot_status feed frame for the truncation banner`. + +--- + +### Task 6: Server-0xx resolution audit — doc-only "Resolved" entries re-verified + +**Classification:** standard +**Estimated implement time:** ~5 min (audit) + fixes as found +**Parallelizable with:** none (runs after Task 4 lands — both may touch `docs/Authentication.md`) + +**Files:** +- Modify: `code-reviews/Server/findings.md` (annotations only — do not rewrite history) +- Modify: any file where a claimed correction is found absent (expected candidates: + `CLAUDE.md`, `docs/*.md`, XML doc comments named in findings — report each) + +**Problem.** Server-012 was recorded *Resolved 2026-05-18* claiming two scope-list +corrections that were absent from the tree when `feat/followup-closeout` looked — a +resolution that regressed or was never applied. Findings that read Resolved are never +re-examined, so every *documentation/comment-only* resolution needs the same spot-check. + +**Procedure:** +1. Enumerate every `Server-0xx` entry in `code-reviews/Server/findings.md` whose + Resolution describes documentation-only or comment-only changes (no test named, prose + like "Pure documentation change" — at minimum Server-011, Server-012, Server-013/014 + remarks rewrites; sweep all entries, don't assume). +2. For each, verify the specific claimed text exists in today's tree (grep the exact + phrases/identifiers the resolution names). +3. Where present: append one line to that finding's Resolution: + `Re-verified present 2026-08-18 (feat/followups-tickets).` +4. Where absent: re-apply the correction in the target file, and append: + `Regressed or never applied; re-fixed 2026-08-18 in (feat/followups-tickets).` + (Server-012 itself was already re-fixed on the previous branch — annotate it as such, + citing commits `a5f843c`/`f2a422b`, rather than re-fixing.) +5. Report a table: finding id → verified/regressed → action. + +**Commit:** `docs(reviews): re-verify doc-only Server-0xx resolutions; re-fix regressions`. + +--- + +### Task 7: WorkerPipeSessionTests deterministic failure — investigate and fix (windev) + +**Classification:** high-risk +**Estimated implement time:** investigation timeboxed ~10 min; fix ≤5 min +**Parallelizable with:** Task 1–5 (only windev user in wave 1) + +**Files (starting points — investigation task, report everything touched):** +- Test: `src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs` + (`RunAsync_LongInFlightCommandThatKeepsPumping_DoesNotFaultAndDeliversReply`) +- Suspect: `src/ZB.MOM.WW.MxGateway.Worker/` pipe-session / frame-protocol sources the + test exercises (follow the test's references) + +**Problem.** The test fails deterministically on windev, and reproduces on `main` — +pre-existing, not introduced by any recent branch. Everything else in the worker suite +passes (523/523 otherwise). + +**Procedure (all building/testing on windev over ssh; edits in the Mac tree, pushed by +the controller, pulled on the CI clone — coordinate via your report if you need a push):** +1. Repro on the CI clone at this branch: + `dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/ZB.MOM.WW.MxGateway.Worker.Tests.csproj -p:Platform=x86 --filter "FullyQualifiedName~RunAsync_LongInFlightCommandThatKeepsPumping"`. + Capture the full failure output (assertion text, timeout, stack). +2. Read the test and the code under test. Classify: (a) test defect (bad timing + assumption, races in the harness), (b) product defect in the pipe session under a + long in-flight command, or (c) environment-specific (windev timing/load). + **Timebox: if the cause is not isolated after ~10 minutes of investigation, stop and + report your best hypothesis with evidence — do not churn.** +3. Fix minimally per the classification. A product fix in the frame/pipe layer is + high-risk territory: preserve the frame protocol (`docs/WorkerFrameProtocol.md`), the + STA pumping rules, and MXAccess parity. A test fix must keep the scenario's intent — + a long in-flight command that keeps pumping must not fault the session and must + deliver its reply; don't weaken it into a sleep-and-hope. +4. Verify: the fixed test passes 3 consecutive runs on windev; then the full worker suite + (`-p:Platform=x86`) is green. If you touched product code, also build the full `slnx` + on windev and run the gateway suite filtered to any shared-surface tests. +5. Docs in the same commit if behavior/rules changed. + +**Commit:** `fix(worker): — WorkerPipeSessionTests long-in-flight repro` (adjust +`fix(worker-tests)` if the defect is in the test). + +--- + +### Task 8: Ack-leg probe — bounded unblock attempt (windev rig) + +**Classification:** standard +**Estimated implement time:** timeboxed ~15 min of probing +**Parallelizable with:** Task 6 (runs after Task 7 — serialize windev use) + +**Files (starting points — investigation task):** +- Modify: `docs/AlarmProbeFindings.md` (append a third-attempt section, whatever the outcome) +- Reference (read-only): the probe harness locations named in that doc's earlier attempts; + the mxaccess analysis project at `C:\Users\dohertj2\Desktop\mxaccess` (windev) + +**Problem.** The acknowledge leg remains unobserved: every wnwrap ack surface is +accepted-but-inert (`rc=0`, state stays `UNACK_ALM`), and `.Acked` is write-rejected +(OperationalError 1007). Two recorded unblock paths remain +(`docs/AlarmProbeFindings.md`, "Remaining unblock paths for the acknowledge leg"). + +**Procedure — attempt path 2 first (it is inspectable), path 1 only if non-interactive:** +1. **Path 2 — ack-security configuration:** inspect whether the rig's galaxy/`alarmmgr` + is configured with an alarm-acknowledgement security requirement the wnwrap consumer + (operator *name* string, no authenticated identity) cannot meet. Look in galaxy + configuration (the `ZB` SQL Galaxy Repository — read-only queries only), area/object + security settings for the `TestMachine_00x` objects, and any alarm-security docs in + the mxaccess analysis project. **Read-only: do not change galaxy security config.** +2. **Path 1 — platform-side ack:** only if a *scriptable, non-interactive* path exists on + the rig as-installed (e.g. an existing harness or automation entry point). Do NOT + install software, do NOT drive GUI automation, do NOT change rig state beyond the + established raise/clear pattern on `TestMachine_001.TestAlarm001`. If only interactive + IDE/InTouch paths exist, record that and stop. +3. Whatever the outcome, append the third-attempt section: what was inspected, evidence, + and the leg's final status (observed / unavailable-by-configuration / still assumed + with the paths requiring a human). If the ack was actually observed, record whether + `STATE` reached `ACK_ALM` and whether the GUID survived — that answers the original + question and should update the findings table at the top of the doc. +4. **Stop condition:** at ~15 minutes of probing without a decisive result, write up what + was learned and conclude "still assumed" — the doc itself notes this is a + documentation gap, not a correctness one. + +**Commit:** `docs(probe): ack-leg third attempt — `. + +--- + +### Task 9: Windev toolchain pin + full verification + +**Classification:** verification (no review chain) +**Estimated implement time:** ~15 min wall (mostly build/test wait) +**Parallelizable with:** none (after all code tasks land and are pushed) + +**Files:** none in-repo except possibly `docs/ToolchainLinks.md` (update the windev +`protoc-gen-go-grpc` entry if it records 1.6.1). + +**Procedure (all over `ssh windev`, CI clone `C:\build\mxaccessgw-ci` at this branch tip):** +1. `go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2` — then + `protoc-gen-go-grpc --version` must report 1.6.2. Update `docs/ToolchainLinks.md` if + it pins the old version (commit from the Mac tree). +2. Full `slnx` build — 0 warnings / 0 errors (clear Contracts obj/bin on CS2001/CS0016). +3. Worker tests `-p:Platform=x86` — expect the Task 7 outcome (fully green if fixed; + otherwise the documented known-failure only). +4. Gateway tests — full suite; rerun filtered on load flakes before believing a failure. +5. `powershell scripts/check-codegen.ps1` — **all 4 checks**, proving Task 1 on the + platform that had the bug. +6. `gradle --version` + `gradle :zb-mom-ww-mxgateway-client:checkGeneratedClean` from + `clients\java` if Gradle is installed there — record version and result either way. +7. Live MXAccess smoke: `MXGATEWAY_RUN_LIVE_MXACCESS_TESTS=1`, filter + `WorkerLiveMxAccessSmokeTests` — 8/8. +8. Report every result verbatim (counts, not "green"). + +--- + +### Task 10: Bookkeeping — close the follow-ups record + +**Classification:** trivial +**Estimated implement time:** ~3 min +**Parallelizable with:** none (last) + +**Files:** +- Modify: `docs/plans/2026-08-17-followup-closeout.md` (follow-ups block `:483-507`): + annotate each bullet closed with this branch's closing commit (or precisely narrowed, + e.g. ack leg if still assumed) +- Modify: `docs/plans/2026-08-18-followups-and-tickets.md` (this file): append as-built + notes — per-task commits, review outcomes, verification results, anything learned +- Modify: `docs/plans/2026-08-18-followups-and-tickets.md.tasks.json`: final statuses + +**Commit:** `chore(plan): followups-and-tickets as-built record; prior follow-ups closed`. + +--- + +## Execution notes for the controller + +- **Model/review chains** (subagent-driven-development skill): Tasks 1–4 small → Sonnet + implementer + Haiku code review (diffs <100 LOC). Task 5 standard → Opus implementer, + spec (Haiku) ∥ code (Sonnet). Task 6 standard → Opus implementer, spec ∥ code. Task 7 + high-risk → Opus implementer, serial spec (Haiku) → code (Sonnet). Task 8 standard → + Opus implementer, spec ∥ code. Task 9 verification → Opus, no review. Task 10 trivial → + Sonnet, no review. +- **Waves:** Wave 1 = Tasks 1, 2, 3, 4, 5, 7 (disjoint files; Task 7 alone on windev). + Wave 2 = Task 6 (after 4) and Task 8 (after 7). Wave 3 = push branch, Task 9. + Wave 4 = Task 10, final integration review, then hold for the user's merge decision. +- **Push cadence:** controller pushes the branch before any windev task needs its code + there (Task 7 investigates on-branch; Tasks 8–9 need the wave-1/2 tips). + +--- + +## As-built record (2026-08-18) + +All 10 tasks completed on `feat/followups-tickets`; every classification-driven review +chain resolved **Approved** (Tasks 5, 7, and 8 after fix rounds; Task 7's high-risk chain +took two rounds, ending with the mid-window auto-release race made structurally +unreachable — 30 s `BlockedDispatchSafetyNet` > every test's 20 s cancellation). The +final integration review returned *Ready with reservations*; every reservation was fixed +on-branch: I-2 (`6b5c737` — `ToolchainLinks.md` recorded `@latest` for plugins the +regeneration script hard-pins), I-3 (`db7b1db` — three doc sites claimed full-DN +`GroupToRole`/`GroupToTag` keys work; the Server-040 restoration proves they never match +the pre-stripped groups; rows were pre-existing on `main`), M-2/M-5 (`1605f54`), M-3 +(`fb68bdb`), M-4 (`753d070`), and M-1 was discharged by evidence (a +`--configuration-cache --rerun-tasks` discriminator proved `providers.exec` evaluates at +execution time after `generateProto`; no change needed). + +Verification (Task 9, windev CI clone at `5417222`, post-fix commits re-verified +individually by their owners at their pushed tips): full `slnx` 0W/0E; worker x86 **524 +passed / 11 skipped / 0 failed** — the suite's first fully green windev run; gateway +1155/1155 with no reruns; `check-codegen.ps1` **4/4 on Windows** under pwsh 7, and after +`8ae0c2f` also end-to-end under Windows PowerShell 5.1; Gradle 9.4.1 +`checkGeneratedClean` green on windev (and 9.5.1 on macOS); live MXAccess smoke 8/8; +toolchain pins now real on windev (`protoc-gen-go v1.36.11`, `protoc-gen-go-grpc 1.6.2`). + +Notable findings made along the way: + +- **Check 4 had a second Windows-only blocker** behind the `.exe` banner: Windows + PowerShell 5.1 strips embedded double quotes when marshaling native-exe args, breaking + the Python `grpcio-tools` version probe (`clients/python/generate-proto.ps1:39`). + Fixed (`8ae0c2f`) by swapping quote nesting; no other native-exe invocation in the + codegen scripts carries the pattern. +- **The ack leg is blocked by galaxy security configuration**, not a missing verb: the + probe fixtures carry `MxSecurityOperate`, and no `AlarmAckByName` overload can convey + an authenticated identity (inferred from mechanism, honestly caveated). `gateway.md` + and `docs/Grpc.md` now carry the "acceptance is not application" caveat on the + acknowledge RPC. +- **The `fca978d` tracking-marker sweep deleted substantive prose** along with markers in + at least one place (Server-040's precedence comment plus an operator-facing RDN + paragraph that was never part of the finding). A targeted re-read of that commit's + larger comment deletions is ticket-worthy (203 files swept; only the audited file was + examined). +- **`generateProto`'s up-to-date check does not notice an out-of-band deletion** of a + single generated file (protobuf-gradle-plugin behavior, orthogonal to this branch) — a + manually deleted generated file stays missing until a `.proto` change or + `--rerun-tasks` invalidates the task. +- Windev process notes: the CI clone sat on a detached HEAD, so bare `git pull` silently + no-ops (use `git pull origin `); concurrent MSBuilds on the clone can kill each + 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): + +- The `fca978d` sweep re-read (above) — the one genuinely ticket-worthy item. +- `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. +- 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. + +Explicitly decided, not an omission: **`../scadaproj/CLAUDE.md` needs no update** — no +`.proto`, contract, command, or architecture fact the umbrella index records changed on +this branch. diff --git a/docs/plans/2026-08-18-followups-and-tickets.md.tasks.json b/docs/plans/2026-08-18-followups-and-tickets.md.tasks.json new file mode 100644 index 0000000..a8501b0 --- /dev/null +++ b/docs/plans/2026-08-18-followups-and-tickets.md.tasks.json @@ -0,0 +1,16 @@ +{ + "planPath": "docs/plans/2026-08-18-followups-and-tickets.md", + "tasks": [ + {"id": 1, "subject": "Task 1: check-codegen Check 4 — Windows .exe banner normalization", "status": "completed", "commit": "c94c4d4", "review": "Approved (Haiku code review, no issues); macOS check-codegen 4/4; Windows-side proof deferred to Task 9"}, + {"id": 2, "subject": "Task 2: Gradle 9 checkGeneratedClean via ProviderFactory.exec", "status": "completed", "commit": "df45cb4", "review": "Approved; verified on Gradle 9.5.1 (local toolchain already Gradle 9 — task was dead here), clean-pass and forced-stale-fail both proven"}, + {"id": 3, "subject": "Task 3: SettingsPage RecentFaultLimit/RecentSessionLimit rows", "status": "completed", "commit": "a390fe1", "review": "Approved; fail-first render test with non-default values, 3/3 filtered"}, + {"id": 4, "subject": "Task 4: Authentication.md runnable-as-written examples pass", "status": "completed", "commit": "2b1efb5", "review": "Approved; three examples gained session:open, ops.audit correctly left session-less (metadata:read gates session-less Galaxy RPCs)"}, + {"id": 5, "subject": "Task 5: AlarmsPage push-driven truncation banner (snapshot_status)", "status": "completed", "commits": ["7b6dfba", "f57a6ae"], "review": "Spec compliant (rename judged in-scope); code review Approved after fixes: stale doc table row, monitor-contract comment (restart does NOT complete subscribers), mid-truncation attach priming test; 5/5 + 370/370 filtered"}, + {"id": 6, "subject": "Task 6: Server-0xx doc-only resolution audit", "status": "completed", "commit": "d3ac527", "review": "Spec compliant; code review Approved (controller verified DashboardGroupRoleMapping change comment-only). 20 entries audited; 2 regressions re-fixed (Server-040 swept comment block incl. RDN pre-strip paragraph; Server-009 WAL/busy-timeout prose); 4 moot (target deleted); Server-038 closed-not-regressed. Systemic finding: fca978d tracking-marker sweep deleted substantive prose in at least one place"}, + {"id": 7, "subject": "Task 7: WorkerPipeSessionTests deterministic failure — investigate + fix (windev)", "status": "completed", "commits": ["7da52b6", "462850a", "aaeb86b"], "review": "High-risk chain: spec compliant; code review Approved after two fix rounds (doc-comment attachment; watchdog-window headroom 200ms/1s + 2s window/30-frame floor; 30s BlockedDispatchSafetyNet > 20s CTS makes the mid-window auto-release race structurally unreachable). Root cause: test-harness defect — FakeRuntimeSession stamped LastStaActivityUtc only at construction, watchdog correctly faulted StaHung pre-dispatch. Test-only fix; windev worker suite 524 passed/11 skipped (was 523+1 fail)"}, + {"id": 8, "subject": "Task 8: Ack-leg probe bounded unblock attempt (windev rig)", "status": "completed", "commits": ["d1ae43d", "bc22792"], "review": "Spec compliant; code review Approved after fixes (superseded bullet closed, summary hedged, gobject_id evidenced, ack-caveat added to gateway.md + Grpc.md as authorized scope extension). Outcome: ack unavailable-by-configuration (MxSecurityOperate classification, no credential-bearing AlarmAckByName overload — inferred, honestly caveated); no .Ack attribute exists; no non-interactive platform-side surface; GUID-across-ack row stays Open with reason updated"}, + {"id": 9, "subject": "Task 9: Windev toolchain pin + full verification", "status": "completed", "verifiedAt": "5417222", "commit": "6b5c737", "result": "protoc-gen-go-grpc 1.6.2 installed; slnx 0W/0E; worker x86 524/11/0; gateway 1155/1155; check-codegen 4/4 (pwsh 7; and under PS 5.1 after 8ae0c2f fixed the quote-stripping probe bug it surfaced); Gradle 9.4.1 checkGeneratedClean green; live smoke 8/8; ToolchainLinks @latest rows corrected to pins (integration-review I-2)"}, + {"id": 10, "subject": "Task 10: Bookkeeping — close the follow-ups record", "status": "completed", "review": "None (trivial); closes the 2026-08-17 plan's follow-ups block with per-bullet commits, appends the as-built record (integration-review resolution: I-2 6b5c737, I-3 db7b1db, M-2/M-5 1605f54, M-3 fb68bdb, M-4 753d070, M-1 discharged by evidence; plus 8ae0c2f PS5.1 fix), final statuses here"} + ], + "lastUpdated": "2026-08-18" +} diff --git a/gateway.md b/gateway.md index c46b102..77c113f 100644 --- a/gateway.md +++ b/gateway.md @@ -227,6 +227,15 @@ ack. If the attribute has no writable ack-comment subtag configured, the RPC returns `FailedPrecondition`. In alarm-manager mode, `AlarmAckByName` is used as before. +**Acceptance is not application:** the ack is forwarded to the provider, and a +successful return means the provider accepted the call, not that the +acknowledgement was applied. On galaxies whose alarmed attributes carry a +non-free-access security classification, `AlarmAckByName` returns `rc=0` and the +alarm stays `UNACK_ALM` — it carries an operator name, not an authenticated +identity. (Observed on the probe rig; the mechanism is inferred.) Confirm an ack +by the resulting transition, never by the return code. See +`docs/AlarmProbeFindings.md`. + **Degraded state visibility:** every subtag-mode transition carries `degraded = true` and `source_provider = ALARM_PROVIDER_MODE_SUBTAG` on the `OnAlarmTransitionEvent` and `ActiveAlarmSnapshot` proto fields. The diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor index 0cabe58..d12ecbd 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor @@ -191,13 +191,13 @@ private Task? _pollTask; private DashboardAlarmProviderStatus _providerStatus = DashboardAlarmProviderStatus.Healthy; - private Task? _providerStatusTask; + private Task? _statusFeedTask; /// protected override void OnInitialized() { _pollTask = PollLoopAsync(); - _providerStatusTask = ProviderStatusLoopAsync(); + _statusFeedTask = StatusFeedLoopAsync(); } private string? ProviderStatusTitle() @@ -210,8 +210,13 @@ // The badge tracks the central monitor directly rather than looping back through // /hubs/alarms: the alarm service is an in-process multi-subscriber fan-out, so a // server-rendered page needs no SignalR client, no loopback socket and no auth token. - // Alarm rows still come from the 3-second poll below — this loop only feeds the badge. - private async Task ProviderStatusLoopAsync() + // This loop feeds the two gateway-status indicators — the provider badge and the + // truncation banner — from the feed's own status frames, so both move as soon as the + // monitor's verdict changes instead of on the next 3-second tick. Alarm rows still come + // from the poll below, which also re-asserts the truncation verdict as its reconcile + // baseline: both sources read the same monitor verdict, so they cannot disagree for + // longer than one tick, and neither one is synthesized here. + private async Task StatusFeedLoopAsync() { while (!_cts.IsCancellationRequested) { @@ -221,16 +226,30 @@ .StreamAsync(alarmFilterPrefix: null, _cts.Token) .ConfigureAwait(false)) { - if (message.PayloadCase != AlarmFeedMessage.PayloadOneofCase.ProviderStatus) + switch (message.PayloadCase) { - continue; - } + case AlarmFeedMessage.PayloadOneofCase.ProviderStatus: + await InvokeAsync(() => + { + _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); + StateHasChanged(); + }).ConfigureAwait(false); + break; - await InvokeAsync(() => - { - _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); - StateHasChanged(); - }).ConfigureAwait(false); + // Every subscriber is primed with this frame at open, so a page that + // attaches mid-truncation gets the caveat without waiting for an edge — + // no page-side priming needed. + case AlarmFeedMessage.PayloadOneofCase.SnapshotStatus: + await InvokeAsync(() => + { + _snapshotTruncated = message.SnapshotStatus.Truncated; + StateHasChanged(); + }).ConfigureAwait(false); + break; + + default: + break; + } } } catch (OperationCanceledException) @@ -239,9 +258,13 @@ } catch { - // The monitor completes a subscriber's stream when it falls behind, and - // again when the monitor restarts. Both are recoverable by resubscribing; - // the badge holds its last value in the meantime. + // The monitor drops a subscriber whose queue it cannot write to, completing + // that stream with an error; short of cancellation or disposal that is the + // only way this enumeration ends. A monitor restart is NOT one of them — it + // keeps the channel and pushes the cleared status frames through it — so this + // catch is the fell-behind case, recoverable by resubscribing. The badge and + // banner hold their last values meanwhile, and the resubscribe is primed with + // the current ones. } try @@ -321,7 +344,7 @@ }; } - // Fault handling sits inside the loop, matching ProviderStatusLoopAsync: a query or render + // Fault handling sits inside the loop, matching StatusFeedLoopAsync: a query or render // fault on one tick is transient (a provider blip, a momentarily unavailable session), so it // is surfaced on the page and retried on the next tick rather than ending polling for the // life of the page. Cancellation is the only exit. The loop method itself therefore cannot @@ -398,6 +421,10 @@ { DashboardAlarmQueryResult result = await LiveData.QueryAlarmsAsync(_cts.Token); _queryError = result.Error; + // Kept alongside the feed's snapshot_status frame rather than replaced by it: this is + // the reconcile baseline. Both read the same monitor verdict, so the poll can only + // confirm what the frame already showed — but it also re-establishes the banner for a + // page whose feed subscription is mid-resubscribe after the monitor dropped it. _snapshotTruncated = result.SnapshotTruncated; _workerPid = result.WorkerProcessId; _lastRefresh = DateTimeOffset.UtcNow; @@ -416,7 +443,7 @@ // Drained together, not one after the other: the wedged dispatcher this bound exists // for blocks both loops at once, so sequential drains would time out twice and make // the real bound 10 seconds. DrainAsync tolerates a null task. - await Task.WhenAll(DrainAsync(_pollTask), DrainAsync(_providerStatusTask)) + await Task.WhenAll(DrainAsync(_pollTask), DrainAsync(_statusFeedTask)) .ConfigureAwait(false); _cts.Dispose(); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SettingsPage.razor b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SettingsPage.razor index 5600287..b064d93 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SettingsPage.razor +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SettingsPage.razor @@ -98,6 +98,8 @@ else Dashboard enabled@Snapshot.Configuration.Dashboard.Enabled Anonymous localhost@Snapshot.Configuration.Dashboard.AllowAnonymousLocalhost Snapshot interval@Snapshot.Configuration.Dashboard.SnapshotIntervalMilliseconds ms + Recent fault limit@Snapshot.Configuration.Dashboard.RecentFaultLimit + Recent session limit@Snapshot.Configuration.Dashboard.RecentSessionLimit Show tag values@Snapshot.Configuration.Dashboard.ShowTagValues Untagged session visibility@Snapshot.Configuration.Dashboard.UntaggedSessionVisibility Worker protocol@Snapshot.Configuration.Protocol.WorkerProtocolVersion diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupRoleMapping.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupRoleMapping.cs index 53b9d50..4cd177a 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupRoleMapping.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupRoleMapping.cs @@ -31,6 +31,23 @@ internal static class DashboardGroupRoleMapping { string normalizedGroup = group.Trim(); + // Lookup precedence: the full literal group string is tried first; only if + // that misses do we fall back to the leading RDN value (e.g. "GwAdmin" + // extracted from "ou=GwAdmin,ou=groups,..."). The map's comparer is + // OrdinalIgnoreCase (see DashboardOptions.GroupToRole), so "GwAdmin" and + // "gwadmin" both match. + // + // With the shared ZB.MOM.WW.Auth.Ldap provider, groups arrive here already + // stripped to short RDN names (the library calls FirstRdnValue before + // returning them). So through the live login path the full-string branch + // only ever sees short names and the RDN fallback is effectively a no-op — + // they collapse to the same key. The fallback is retained because this + // mapping is also reachable directly via the IGroupRoleMapper seam + // (DashboardGroupRoleMapper), where a caller could still pass a full DN. + // CONSEQUENCE: configuring a full-DN GroupToRole *key* (e.g. + // "ou=GwAdmin,ou=groups,...") is UNSUPPORTED with the shared library — the + // incoming group is a short name, so it will never equal a full-DN key. + // Keep GroupToRole keys as short group names. if (groupToRole.TryGetValue(normalizedGroup, out string? mapped) || groupToRole.TryGetValue(ExtractFirstRdnValue(normalizedGroup), out mapped)) { diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs index dc54fd7..84be574 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardGroupTagMapping.cs @@ -35,6 +35,13 @@ internal static class DashboardGroupTagMapping { string normalizedGroup = group.Trim(); + // Same lookup semantics as DashboardGroupRoleMapping.MapGroupsToRoles — + // full literal group string first, leading-RDN value as the fallback, over + // an OrdinalIgnoreCase map. See the comment there for the consequence that + // applies verbatim here: the shared ZB.MOM.WW.Auth.Ldap provider delivers + // groups already stripped to short RDN names, so a full-DN GroupToTag *key* + // is UNSUPPORTED — it can never equal the short name that arrives. Keep + // GroupToTag keys as short group names. if (!groupToTag.TryGetValue(normalizedGroup, out string[]? granted) && !groupToTag.TryGetValue( DashboardGroupRoleMapping.ExtractFirstRdnValue(normalizedGroup), diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/AlarmsPageTruncationBannerTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/AlarmsPageTruncationBannerTests.cs index ebb4e54..d14339b 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/AlarmsPageTruncationBannerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/AlarmsPageTruncationBannerTests.cs @@ -1,7 +1,11 @@ +using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Threading.Channels; using Microsoft.AspNetCore.Components.Web.HtmlRendering; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.MxGateway.Server.Alarms; using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Dashboard; @@ -36,6 +40,8 @@ public sealed class AlarmsPageTruncationBannerTests { private const string BannerMarker = "Alarm snapshot may be incomplete"; + private static readonly TimeSpan RenderWaitTimeout = TimeSpan.FromSeconds(10); + /// A capped provider fetch puts the completeness caveat on the page. /// A task that represents the asynchronous operation. [Fact] @@ -61,6 +67,158 @@ public sealed class AlarmsPageTruncationBannerTests Assert.Contains("Active Alarms", html, StringComparison.Ordinal); } + /// + /// The push path. A snapshot_status frame arriving on the page's + /// in-process alarm-feed subscription raises the caveat on its own, with + /// no poll tick behind it — the poll is a 3-second reconcile baseline, and + /// an operator should not stare at an un-caveated alarm list for up to + /// three seconds after the gateway has already decided the set is capped. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AlarmsPage_WhenFeedPushesTruncated_RaisesTheBannerWithoutAPollTick() + { + ScriptedAlarmFeed feed = new(); + await using ServiceProvider provider = BuildPushServices(feed); + await using HtmlRenderer renderer = new( + provider, + provider.GetRequiredService()); + + HtmlRootComponent page = await renderer.Dispatcher.InvokeAsync( + () => renderer.RenderComponentAsync()); + + // The one poll answer this page will ever get said "complete", so everything + // the banner does from here is the feed's doing. + Assert.DoesNotContain(BannerMarker, await HtmlAsync(renderer, page), StringComparison.Ordinal); + + await feed.PushAsync(SnapshotStatusFrame(truncated: true)); + + await WaitForHtmlAsync( + renderer, + page, + html => html.Contains(BannerMarker, StringComparison.Ordinal), + "banner to appear after a truncated snapshot_status frame"); + } + + /// + /// The clearing edge, page-side. Absence authority comes back when the + /// gateway says so; a banner that only ever went up would caveat the alarm + /// list for the life of the circuit. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AlarmsPage_WhenFeedPushesComplete_ClearsTheBannerWithoutAPollTick() + { + ScriptedAlarmFeed feed = new(); + await using ServiceProvider provider = BuildPushServices(feed); + await using HtmlRenderer renderer = new( + provider, + provider.GetRequiredService()); + + HtmlRootComponent page = await renderer.Dispatcher.InvokeAsync( + () => renderer.RenderComponentAsync()); + + await feed.PushAsync(SnapshotStatusFrame(truncated: true)); + await WaitForHtmlAsync( + renderer, + page, + html => html.Contains(BannerMarker, StringComparison.Ordinal), + "banner to appear before the clearing frame is pushed"); + + await feed.PushAsync(SnapshotStatusFrame(truncated: false)); + + await WaitForHtmlAsync( + renderer, + page, + html => !html.Contains(BannerMarker, StringComparison.Ordinal), + "banner to clear after a complete snapshot_status frame"); + } + + /// + /// Attach while the verdict is already truncated. The page carries no + /// priming logic of its own — it relies on StreamAsync opening every + /// subscription with a snapshot_status baseline — so the caveat has + /// to come up off the open sequence alone, with no edge pushed afterwards. + /// A page that only handled the edge would show an un-caveated alarm list + /// to every operator who opened it after the truncation began. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AlarmsPage_AttachingToAnAlreadyTruncatedFeed_RaisesTheBannerFromThePriming() + { + // The monitor's open sequence: provider status, then the unconditional + // completeness baseline. Nothing is pushed after this. + ScriptedAlarmFeed feed = new() + { + Priming = + [ + new AlarmFeedMessage { ProviderStatus = new AlarmProviderStatus() }, + SnapshotStatusFrame(truncated: true), + ], + }; + + await using ServiceProvider provider = BuildPushServices(feed); + await using HtmlRenderer renderer = new( + provider, + provider.GetRequiredService()); + + HtmlRootComponent page = await renderer.Dispatcher.InvokeAsync( + () => renderer.RenderComponentAsync()); + + await WaitForHtmlAsync( + renderer, + page, + html => html.Contains(BannerMarker, StringComparison.Ordinal), + "banner to appear from the feed's open-time snapshot_status baseline"); + } + + private static AlarmFeedMessage SnapshotStatusFrame(bool truncated) + { + return new AlarmFeedMessage + { + SnapshotStatus = new AlarmSnapshotStatus { Truncated = truncated }, + }; + } + + private static ServiceProvider BuildPushServices(ScriptedAlarmFeed feed) + { + ServiceCollection services = new(); + services.AddLogging(); + services.AddSingleton(); + services.AddSingleton(feed); + services.AddSingleton>( + Options.Create(new GatewayOptions { Alarms = new AlarmsOptions { Enabled = true } })); + return services.BuildServiceProvider(); + } + + private static Task HtmlAsync(HtmlRenderer renderer, HtmlRootComponent page) + { + // Serialization has to happen on the renderer's dispatcher, and it reads the + // component's current render tree — so it reflects renders the page's feed + // loop queued after the initial quiescent render. + return renderer.Dispatcher.InvokeAsync(page.ToHtmlString); + } + + private static async Task WaitForHtmlAsync( + HtmlRenderer renderer, + HtmlRootComponent page, + Func predicate, + string expectation) + { + Stopwatch elapsed = Stopwatch.StartNew(); + while (elapsed.Elapsed < RenderWaitTimeout) + { + if (predicate(await HtmlAsync(renderer, page))) + { + return; + } + + await Task.Delay(TimeSpan.FromMilliseconds(20)); + } + + Assert.Fail($"Timed out after {RenderWaitTimeout.TotalSeconds:N0}s waiting for the {expectation}."); + } + private static async Task RenderAsync(bool snapshotTruncated) { ServiceCollection services = new(); @@ -102,4 +260,107 @@ public sealed class AlarmsPageTruncationBannerTests WorkerProcessId: null, SnapshotTruncated: snapshotTruncated)); } + + // Answers exactly one poll — the inline first pass — and parks every later tick + // until the page's disposal cancels it. The parking is what makes the push tests + // measure the push: a second tick would re-assert the poll's own verdict, and + // could either mask a banner the feed raised or raise one the feed did not. + private sealed class SinglePollLiveDataService : IDashboardLiveDataService + { + private int _polls; + + /// + public Task ReadAsync( + IReadOnlyCollection tagAddresses, + CancellationToken cancellationToken) => + Task.FromResult(DashboardLiveReadResult.Empty); + + /// + public async Task QueryAlarmsAsync(CancellationToken cancellationToken) + { + if (Interlocked.Increment(ref _polls) > 1) + { + await Task.Delay(Timeout.Infinite, cancellationToken).ConfigureAwait(false); + } + + return new DashboardAlarmQueryResult( + Alarms: [], + Error: null, + WorkerProcessId: null, + SnapshotTruncated: false); + } + } + + // A hand-driven stand-in for the alarm feed: the test writes the frames the real + // monitor would push. Unbounded and never completed, so a frame written before the + // page's loop attaches is still delivered, and the loop never has to resubscribe. + private sealed class ScriptedAlarmFeed : IGatewayAlarmService + { + private readonly Channel _frames = + Channel.CreateUnbounded(new UnboundedChannelOptions + { + SingleReader = false, + SingleWriter = false, + }); + + /// + public GatewayAlarmMonitorState State => GatewayAlarmMonitorState.Monitoring; + + /// + public string? LastError => null; + + /// + public int? WorkerProcessId => null; + + /// + public IReadOnlyList CurrentAlarms => []; + + /// + public bool SnapshotTruncated { get; set; } + + /// + /// Frames replayed at the head of every subscription, standing in for the + /// monitor's open sequence (provider status, then the unconditional + /// completeness baseline). Empty means the subscriber sees only what the + /// test pushes. + /// + public IReadOnlyList Priming { get; init; } = []; + + /// Pushes one frame onto the feed the page is subscribed to. + /// The feed frame to deliver. + /// A task that represents the asynchronous operation. + public ValueTask PushAsync(AlarmFeedMessage message) => _frames.Writer.WriteAsync(message); + + /// + public async IAsyncEnumerable StreamAsync( + string? alarmFilterPrefix, + [EnumeratorCancellation] CancellationToken cancellationToken) + { + foreach (AlarmFeedMessage primed in Priming) + { + cancellationToken.ThrowIfCancellationRequested(); + yield return primed; + } + + await foreach (AlarmFeedMessage message in _frames.Reader + .ReadAllAsync(cancellationToken) + .ConfigureAwait(false)) + { + yield return message; + } + } + + /// + public Task AcknowledgeAsync( + AcknowledgeAlarmRequest request, + CancellationToken cancellationToken) + { + return Task.FromResult(new AcknowledgeAlarmReply + { + CorrelationId = request.ClientCorrelationId, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + DiagnosticMessage = string.Empty, + }); + } + } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/SettingsPageTagVisibilityRenderTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/SettingsPageTagVisibilityRenderTests.cs index 5de301a..0a99f8e 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/SettingsPageTagVisibilityRenderTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Dashboard/SettingsPageTagVisibilityRenderTests.cs @@ -73,6 +73,28 @@ public sealed class SettingsPageTagVisibilityRenderTests Assert.Contains("Dashboard role mapping", html, StringComparison.Ordinal); } + /// + /// RecentFaultLimit and RecentSessionLimit reach the page, each value landing in + /// the row its own label names — not merely present somewhere in the document. Non-default + /// values prove the projection is plumbed through, not just that the defaults happen to render. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task SettingsPage_RendersRecentFaultAndSessionLimits() + { + string html = await RenderAsync(new DashboardOptions + { + RecentFaultLimit = 123, + RecentSessionLimit = 456, + }); + + // Combined label+value fragments, matching the razor's exact markup (no whitespace between + // tags): a bare Contains on "123"/"456" would pass even if the values landed in the wrong + // row, or any other row on the page. + Assert.Contains("""Recent fault limit123""", html, StringComparison.Ordinal); + Assert.Contains("""Recent session limit456""", html, StringComparison.Ordinal); + } + private static async Task RenderAsync(DashboardOptions dashboard) { EffectiveGatewayConfiguration configuration = diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs index 826e1b0..dca5936 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.IO.Pipes; using System.Runtime.InteropServices; @@ -1451,10 +1452,17 @@ public sealed class WorkerPipeSessionTests /// refresh LastActivityUtc on every wait iteration, so a healthy /// ReadBulk holding the STA far longer than /// HeartbeatStuckCeiling (75 s in production) keeps its activity - /// timestamp fresh. This test compresses the clock — a 100 ms ceiling - /// with a command in flight across a window many multiples longer — and - /// models the pump refresh by continuously advancing the snapshot's - /// LastStaActivityUtc while the command blocks. Contrast + /// timestamp fresh. This test compresses the clock — a 1 s ceiling with + /// a command in flight across a window twice as long — and models the + /// pump refresh with + /// , which + /// stamps activity at every heartbeat capture exactly as the pump's + /// per-iteration MarkActivity() does. The refresh has to be in + /// effect from construction, not from the moment the command blocks: the + /// idle window covering handshake and startup carries no correlation id + /// for the watchdog to suppress on, so a fake whose activity timestamp is + /// frozen at construction is reported StaHung before the scenario + /// under test even starts. Contrast /// , /// where a frozen timestamp beyond the ceiling correctly faults; here /// the refreshed timestamp must keep the fault suppressed and let the @@ -1469,15 +1477,28 @@ public sealed class WorkerPipeSessionTests FakeRuntimeSession runtime = new() { BlockDispatch = true, + + // The pump refreshes STA activity on every wait iteration, so every + // heartbeat capture on a healthy worker sees fresh activity — while + // a command holds the STA and while it is idle alike. Armed before + // RunAsync so the very first beat, sent as soon as the session is + // Ready, is already covered. + RefreshStaActivityOnCapture = true, }; WorkerPipeSession session = CreatePipeSession( pipePair.WorkerStream, runtime, new WorkerPipeSessionOptions { + // Compressed relative to production (75 s ceiling), but no further than the real + // pipe underneath can carry. ReportWatchdogFaultIfNeededAsync measures staleness + // AFTER the heartbeat frame has been written and flushed, so any beat whose pipe + // I/O outlasts the ceiling faults a healthy session. At a 100 ms ceiling that is a + // plausible stall on a loaded box; at 1 s it is not — and this is the one test + // asserting the watchdog NEVER fires, so it has to hold under load. HeartbeatInterval = TimeSpan.FromMilliseconds(20), - HeartbeatGrace = TimeSpan.FromMilliseconds(50), - HeartbeatStuckCeiling = TimeSpan.FromMilliseconds(100), + HeartbeatGrace = TimeSpan.FromMilliseconds(200), + HeartbeatStuckCeiling = TimeSpan.FromSeconds(1), }); Task runTask = session.RunAsync(cancellation.Token); await CompleteGatewayHandshakeAsync(pipePair, cancellation.Token); @@ -1490,49 +1511,57 @@ public sealed class WorkerPipeSessionTests runtime.DispatchStarted.Wait(TimeSpan.FromSeconds(5)), "The long command must reach the runtime and begin dispatch."); - // Model the pump refreshing STA activity on each wait iteration: keep - // the snapshot's LastStaActivityUtc current while the command is in - // flight. - using CancellationTokenSource pumpRefresh = new(); - Task refreshLoop = Task.Run( - async () => - { - while (!pumpRefresh.IsCancellationRequested) - { - runtime.SetSnapshot(new WorkerRuntimeHeartbeatSnapshot( - DateTimeOffset.UtcNow, - pendingCommandCount: 1, - outboundEventQueueDepth: 0, - lastEventSequence: 0, - currentCommandCorrelationId: "long-bulk-read")); - await Task.Delay(TimeSpan.FromMilliseconds(20)).ConfigureAwait(false); - } - }); + // Publish the in-flight shape the heartbeat then reports for the whole + // blocked window; only LastStaActivityUtc moves after this, refreshed by + // the modelled pump at each capture. + runtime.SetSnapshot(new WorkerRuntimeHeartbeatSnapshot( + DateTimeOffset.UtcNow, + pendingCommandCount: 1, + outboundEventQueueDepth: 0, + lastEventSequence: 0, + currentCommandCorrelationId: "long-bulk-read")); - // Inspect a bounded number of frames over a window many multiples of the - // 100 ms ceiling (at least 30 heartbeats at 20 ms ~ 600 ms). None may be - // a WorkerFault while activity is continuously refreshed. - const int framesToInspect = 30; - for (int index = 0; index < framesToInspect; index++) + // Inspect frames across a window twice the stuck ceiling — long enough that a fake whose + // activity timestamp stopped advancing would accumulate staleness past the ceiling and + // fault — and require the beats to have actually flowed while it ran, so an inspection + // that saw a couple of frames and timed out cannot pass for a clean window. None may be a + // WorkerFault while activity is continuously refreshed. Nothing here is racing + // FakeRuntimeSession's blocked-dispatch backstop: that wait is a safety net sized far above + // any window a test opens (and above this test's own cancellation), so the command stays in + // flight for however long a loaded box stretches the loop. Were the two close together, a + // slow run would take the reply mid-window and then fail waiting for a reply already gone + // by — a cancellation at teardown, naming nothing. + TimeSpan inspectionWindow = TimeSpan.FromSeconds(2); + const int minimumFramesInspected = 30; + Stopwatch inspection = Stopwatch.StartNew(); + int frameIndex = 0; + while (inspection.Elapsed < inspectionWindow || frameIndex < minimumFramesInspected) { WorkerEnvelope envelope = await pipePair.GatewayReader .ReadAsync(cancellation.Token); - Assert.NotEqual( - WorkerEnvelope.BodyOneofCase.WorkerFault, - envelope.BodyCase); + AssertNotWorkerFault(envelope, frameIndex++); } - // Stop refreshing and release the command; its reply must be delivered - // because the session never faulted (state stayed Ready). - pumpRefresh.Cancel(); - await refreshLoop; + // Release the command with the pump still running — as it is in + // production while the reply is marshalled off the STA. The reply must + // be delivered (the session never faulted, so its state stayed Ready), + // and no frame on the way to it may be a fault either. runtime.ReleaseDispatch(); - WorkerEnvelope reply = await ReadUntilAsync( - pipePair.GatewayReader, - WorkerEnvelope.BodyOneofCase.WorkerCommandReply, - envelope => envelope.CorrelationId == "long-bulk-read", - cancellation.Token); + WorkerEnvelope reply; + while (true) + { + WorkerEnvelope envelope = await pipePair.GatewayReader + .ReadAsync(cancellation.Token); + AssertNotWorkerFault(envelope, frameIndex++); + if (envelope.BodyCase == WorkerEnvelope.BodyOneofCase.WorkerCommandReply + && envelope.CorrelationId == "long-bulk-read") + { + reply = envelope; + break; + } + } + Assert.Equal( ProtocolStatusCode.Ok, reply.WorkerCommandReply.Reply.ProtocolStatus.Code); @@ -2211,6 +2240,26 @@ public sealed class WorkerPipeSessionTests cancellationToken); } + /// + /// Fails when the frame is a WorkerFault, naming the category and diagnostic message. + /// A bare body-case comparison reports only "expected not WorkerFault", which says nothing + /// about which watchdog or protocol path produced it — the one fact needed to tell a + /// regression from a harness that mis-models the runtime. + /// + /// Frame read from the gateway end. + /// Ordinal of the frame within the inspected run. + private static void AssertNotWorkerFault(WorkerEnvelope envelope, int frameIndex) + { + if (envelope.BodyCase != WorkerEnvelope.BodyOneofCase.WorkerFault) + { + return; + } + + Assert.Fail( + $"Frame {frameIndex} is a WorkerFault ({envelope.WorkerFault.Category}): " + + envelope.WorkerFault.DiagnosticMessage); + } + /// Reads frames until one matches the expected body type and predicate. /// Frame reader. /// Expected body case. diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/TestSupport/FakeRuntimeSession.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/TestSupport/FakeRuntimeSession.cs index b5a4252..456205d 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/TestSupport/FakeRuntimeSession.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/TestSupport/FakeRuntimeSession.cs @@ -20,6 +20,17 @@ namespace ZB.MOM.WW.MxGateway.Worker.Tests.TestSupport; /// internal sealed class FakeRuntimeSession : IWorkerRuntimeSession { + /// + /// Backstop on the wait so a test that never releases leaves no + /// thread parked forever. It is a safety net, never a scenario's timing budget: nothing + /// asserts on it firing, and a test whose blocked window outruns it silently gets its reply + /// mid-window, which then fails as an opaque cancellation somewhere later. Kept far above + /// any test's window — and above the 20 s cancellation those tests arm — so the test's own + /// token always fails first, with its own message. releases the wait + /// regardless, so teardown never depends on this elapsing. + /// + private static readonly TimeSpan BlockedDispatchSafetyNet = TimeSpan.FromSeconds(30); + private readonly ManualResetEventSlim releaseDispatch = new(false); private readonly object gate = new(); private readonly Queue events = new(); @@ -31,6 +42,7 @@ internal sealed class FakeRuntimeSession : IWorkerRuntimeSession // disposed SemaphoreSlim would turn that shutdown into an ObjectDisposedException. private readonly SemaphoreSlim eventSignal = new(0, 1); private TimeSpan? lastWaitForEventsTimeout; + private bool refreshStaActivityOnCapture; private WorkerRuntimeHeartbeatSnapshot snapshot = new( DateTimeOffset.UtcNow, pendingCommandCount: 0, @@ -91,7 +103,7 @@ internal sealed class FakeRuntimeSession : IWorkerRuntimeSession if (BlockDispatch) { - releaseDispatch.Wait(TimeSpan.FromSeconds(5)); + releaseDispatch.Wait(BlockedDispatchSafetyNet); } SetSnapshot(new WorkerRuntimeHeartbeatSnapshot( @@ -127,11 +139,52 @@ internal sealed class FakeRuntimeSession : IWorkerRuntimeSession }); } + /// + /// When set, stamps the snapshot's + /// LastStaActivityUtc with the capture time and leaves every other field as the last + /// left it. Models a live STA whose pump calls + /// MarkActivity() on each wait iteration (StaRuntime.ThreadMain), so a healthy + /// worker is never captured stale — which a watchdog test needs to hold for the whole + /// session, including the handshake window before any command exists for the watchdog to + /// suppress on. A test-owned refresh loop cannot hold it: it is a thread-pool continuation + /// racing a compressed grace, and the gap between this fake being constructed and that loop's + /// first tick is already enough to look hung. + /// + public bool RefreshStaActivityOnCapture + { + get + { + lock (gate) + { + return refreshStaActivityOnCapture; + } + } + + set + { + lock (gate) + { + refreshStaActivityOnCapture = value; + } + } + } + /// public WorkerRuntimeHeartbeatSnapshot CaptureHeartbeat() { lock (gate) { + if (refreshStaActivityOnCapture) + { + snapshot = new WorkerRuntimeHeartbeatSnapshot( + DateTimeOffset.UtcNow, + snapshot.PendingCommandCount, + snapshot.OutboundEventQueueDepth, + snapshot.LastEventSequence, + snapshot.CurrentCommandCorrelationId, + snapshot.StaCallInProgress); + } + return snapshot; } }