From a756e476820e835de879375ff163f843cf853b8d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:03:52 -0400 Subject: [PATCH 01/23] =?UTF-8?q?docs(plans):=20deferred-findings=20remedi?= =?UTF-8?q?ation=20plan=20=E2=80=94=2014=20tasks=20over=20the=20six=20defe?= =?UTF-8?q?rred=20findings=20+=20the=20Windows=20secrets-test=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/2026-08-15-deferred-remediation.md | 310 ++++++++++++++++++ ...6-08-15-deferred-remediation.md.tasks.json | 20 ++ 2 files changed, 330 insertions(+) create mode 100644 docs/plans/2026-08-15-deferred-remediation.md create mode 100644 docs/plans/2026-08-15-deferred-remediation.md.tasks.json diff --git a/docs/plans/2026-08-15-deferred-remediation.md b/docs/plans/2026-08-15-deferred-remediation.md new file mode 100644 index 0000000..e9dd38e --- /dev/null +++ b/docs/plans/2026-08-15-deferred-remediation.md @@ -0,0 +1,310 @@ +# Deferred-Findings Remediation Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development to implement this plan task-by-task (Opus implementers per the user's instruction). + +**Goal:** Resolve the six findings the 2026-08-15 perf-review remediation explicitly deferred (`docs/plans/2026-08-15-perf-review-remediation.md:611-620`) plus the pre-existing Windows-only `SecretsStorePathGuardTests` failure, so the deferred table empties and windev returns to a clean 1046/1046 gateway suite. + +**Architecture:** Two phases. Phase A is gateway-side (net10, fully verifiable on macOS): the secrets-test fix, the distributor dictionary swap, event-path iterator flattening, and the dashboard in-process refactor that removes the Blazor pages' loopback SignalR hop while preserving the idle gate, mirror viewer gating, and clone-then-redact invariants. Phase B is worker-side (net48 x86, verified on windev over ssh): control-frame completion decoupling in the two-class frame writer, pipe-read teardown restructuring, and value-cache clone removal per the completed aliasing audit. + +**Tech Stack:** .NET 10 / ASP.NET Core / Blazor Server / System.Threading.Channels (gateway); .NET Framework 4.8 x86 (worker); xUnit; windev CI clone `C:\build\mxaccessgw-ci` via `ssh windev`. + +**Branch:** `perf/deferred-remediation` off local `main` (`15f188e`). + +--- + +## Ground rules for every implementer subagent + +- Shared working tree at `/Users/dohertj2/Desktop/MxAccessGateway`. **NEVER run `git stash`, `git reset`, `git clean`, `git checkout `, or any command that touches files outside your task's `Files:` list.** Commit with explicit pathspecs only (`git add && git commit`). +- Build/test lock: before `dotnet build` or `dotnet test`, acquire the lock with `mkdir /private/tmp/claude-501/-Users-dohertj2-Desktop-MxAccessGateway/f36938ae-bbca-4245-b5c9-fac512d69e22/scratchpad/buildlock` (retry loop with sleep until it succeeds); `rmdir` it in ALL exit paths. +- `TreatWarningsAsErrors=true`, `Nullable=enable` repo-wide. Follow `docs/style-guides/CSharpStyleGuide.md`: file-scoped namespaces, `sealed` by default, `Async` suffix, MXAccess-aligned names. +- Worker projects (`ZB.MOM.WW.MxGateway.Worker*`) are net48/x86 and DO NOT COMPILE on macOS. For Phase B tasks: edit carefully, self-review for net48 compatibility (target-typed `new` and file-scoped namespaces ARE valid — `LangVersion=latest`; but no `Span`-based BCL overloads, no `IAsyncDisposable` on BCL types, `Channel` comes from System.Threading.Channels package which the worker already references). Compilation and tests happen at the Task 13 windev gate. +- Update affected docs in the same commit as the source (repo rule), except the dashboard design doc which Task 8 consolidates (deliberate, to avoid parallel edits to one file). +- MXAccess parity: never synthesize events, never mutate an event already handed to the outbound queue or wire. + +--- + +## Phase A — gateway (macOS-verifiable) + +### Task 1: Windows-safe cleanup in SecretsStorePathGuardTests + +**Classification:** small +**Estimated implement time:** ~3 min +**Parallelizable with:** Task 2, Task 3, Task 4 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs` +- Modify: `docs/GatewayTesting.md` (lines ~557-565, the "fails deterministically on Windows" note) + +**Why:** `CreateBuilder_AcceptsSecretsStoreOutsideContentRoot_AndCreatesIt` (lines 84-105) fails deterministically on Windows: `GatewayApplication.CreateBuilder` migrates the secrets store through `SecretsSqliteConnectionFactory` (`Pooling = true`, WAL), disposal returns the connection to the Microsoft.Data.Sqlite pool with the native handle open, and the `finally`'s `Directory.Delete(directory, recursive: true)` (line 103) hits a sharing violation. macOS passes only because Unix unlinks open files. The repo fixes this pattern twice already: `TestSupport/../TempDatabaseDirectory.cs:57` and `Configuration/PreHostSecretExpansionTests.cs:130-153`. + +**Spec:** +1. In the failing test's `finally`, before `Directory.Delete`: call `Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools();` and wrap the delete in `try { ... } catch (IOException) { } catch (UnauthorizedAccessException) { }` (best-effort, mirroring `TempDatabaseDirectory.Dispose`). Add a comment mirroring the one in `PreHostSecretExpansionTests.cs:133-137` (WAL + pooling keeps the handle alive past dispose). +2. Leave the rejection test alone (the guard means its file is never created). +3. Update `docs/GatewayTesting.md`: replace the "subtract it from the expected pass count on Windows" paragraph with a short note that the test's cleanup now clears the SQLite pool first and the failure is fixed as of this branch. + +**Steps:** edit → `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~SecretsStorePathGuardTests"` (expect 2/2 on macOS; the real proof is the Task 13 windev gate) → commit `fix(tests): clear the SQLite pool before deleting the secrets path-guard temp dir — Windows sharing violation`. + +--- + +### Task 2: SessionEventDistributor `_subscribers` → plain `Dictionary` + +**Classification:** small +**Estimated implement time:** ~3 min +**Parallelizable with:** Task 1, Task 3, Task 4 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs` + +**Why:** All five access sites (`:365`, `:555`, `:771`, `:799`, `:813`) are inside `lock (_lifecycleLock)`; the lock-free hot path reads the copy-on-write `_subscriberSnapshot` array (`:958`, `:302`), never the dictionary. The concurrent type buys nothing. Audit confirmed no external/reflection access. + +**Spec:** Change the field at `:107` to `Dictionary`; `TryRemove(subscriber.Id, out _)` at `:799` becomes `Remove(subscriber.Id)`. Reword the type remarks at `:69-80`, `:111-123`, and `:298-300` where they name `ConcurrentDictionary` by design — the invariant to state is now: "the dictionary is only ever touched under `_lifecycleLock`; lock-free readers use `_subscriberSnapshot`." + +**Steps:** edit → `dotnet test ... --filter "FullyQualifiedName~SessionEventDistributorTests"` (29 facts, expect all green) → commit `refactor(sessions): _subscribers to plain Dictionary — every access is under _lifecycleLock`. + +--- + +### Task 3: Merge the session event-source pass-through iterator + +**Classification:** standard +**Estimated implement time:** ~5 min +**Parallelizable with:** Task 1, Task 2, Task 4 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs` (`MapWorkerEventsAsync` ~:767-776, `ReadEventsAsync` ~:1517-1530) +- Test: `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/GatewaySessionTests.cs` (existing) + +**Why:** The worker→distributor source chain nests three compiler-generated async iterators per event: `WorkerClient.ReadEventsCoreAsync` → `GatewaySession.ReadEventsAsync` (pure pass-through: `TouchClientActivity(); yield return`) → `GatewaySession.MapWorkerEventsAsync` (`yield return mapper.MapEvent(...)`). The pass-through layer is two extra `MoveNextAsync` state-machine hops per event for no semantic value. + +**Spec:** +1. FIRST grep all callers of `ReadEventsAsync`. If `MapWorkerEventsAsync` is its only caller, inline it: `MapWorkerEventsAsync` calls `GetReadyWorkerClientAsync`, iterates `client.ReadEventsAsync(ct)` directly, calls `TouchClientActivity()` per event, and `yield return mapper.MapEvent(workerEvent)`. Delete `ReadEventsAsync`. If other callers exist, keep the method for them but make `MapWorkerEventsAsync` self-contained as above — do NOT change any caller outside this file; report the finding. +2. Behavior must be byte-identical: same activity-touch cadence (per event), same exception propagation (WorkerClientException flows to the distributor pump unchanged), no event synthesis, worker order preserved. +3. `WorkerClient.ReadEventsCoreAsync`'s single-reader claim (`_eventsReaderClaimed`) must still be exercised exactly once per attach — do not add a second call site. + +**Steps:** grep callers → edit → `dotnet test ... --filter "FullyQualifiedName~GatewaySession"` and `--filter "FullyQualifiedName~SessionEventDistributorTests"` → commit `perf(sessions): fold the ReadEventsAsync pass-through into MapWorkerEventsAsync — one fewer iterator per event`. + +--- + +### Task 4: EventStreamService direct channel reads in the live loop + +**Classification:** high-risk +**Estimated implement time:** ~5 min +**Parallelizable with:** Task 1, Task 2, Task 3 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs` +- Test: `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs` (existing 17 facts — must pass unchanged) + +**Why:** The subscriber-side live loop materializes `subscriber.Reader.ReadAllAsync(ct).GetAsyncEnumerator(ct)` (`:109-111`) — a BCL async-iterator wrapper costing a state-machine hop per event on the hottest gateway path. Direct `ChannelReader` consumption (`WaitToReadAsync` + drain-with-`TryRead`) removes it. + +**Spec:** +1. Replace the enumerator with direct reads: `while (await reader.WaitToReadAsync(ct)) { while (reader.TryRead(out MxEvent? mxEvent)) { ...existing per-event body... } }`; loop ends when `WaitToReadAsync` returns false (channel completed). +2. EVERY invariant in the current body survives, verbatim where possible: + - ReplayGap sentinel emitted exactly once, first, only when `replayGap` (`:133-139`) — untouched, it precedes the live loop. + - Replay batch stitching (`:141-150`) — untouched. + - Per-RPC dedup watermark `if (mxEvent.WorkerSequence <= afterWorkerSequence) continue;` (`:179-182`) — must apply to every live event. + - `WorkerClientException` catch → `session.MarkFaulted` → metrics → rethrow (`:164-174`): a completed-with-exception channel surfaces its exception from `WaitToReadAsync` — the catch must wrap the wait/read, preserving identical fault classification. Terminal `SessionManagerException(EventQueueOverflow)` propagates unchanged. + - `finally` ordering (`:192-200`): with no enumerator to dispose, the remaining order is backlog-gauge registration disposal → lease disposal → `metrics.StreamDisconnected("Detached")`. Keep the comments explaining why. +3. Cancellation: `WaitToReadAsync(ct)` throws `OperationCanceledException` on detach — must reach the same code path the enumerator's cancellation did (the gRPC layer treats it as client disconnect). Verify against `StreamEventsAsync_WhenCanceled_DetachesSubscriber`. +4. No public-surface change; `MxAccessGatewayService` (`:151-179`) is untouched. + +**Steps:** edit → run the full `EventStreamServiceTests` class + `GatewayEndToEndReconnectReplayTests` + `GatewayEndToEndMultiSubscriberTests` → commit `perf(grpc): consume the subscriber channel directly in StreamEventsAsync — drops the ReadAllAsync iterator hop`. + +--- + +### Task 5: In-process dashboard snapshot feed + page switch + +**Classification:** high-risk +**Estimated implement time:** ~8 min (accepted overage; splitting further would split one invariant) +**Parallelizable with:** Task 6, Task 7 + +**Files:** +- Create: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSnapshotFeed.cs` +- Create: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs` +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs` +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs` (one `AddSingleton` line) +- Create: `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs` + +**Why:** Eight pages inherit `DashboardPageBase` and each opens a loopback `/hubs/snapshot` HubConnection (`DashboardPageBase.cs:62`) — a WebSocket round trip back into the same process per circuit. `IDashboardSnapshotService.WatchSnapshotsAsync` exists but is NOT multicast (each enumeration = its own `PeriodicTimer` + snapshot build), so pages must not call it directly; a shared feed does one enumeration and fans out. + +**Spec:** +1. `IDashboardSnapshotFeed` (singleton): `IAsyncEnumerable WatchAsync(CancellationToken ct)`. Internally: per-subscriber `Channel` with capacity 1 and `BoundedChannelFullMode.DropOldest` (a dashboard viewer only ever wants the latest snapshot; a slow circuit must never buffer unboundedly or stall others). +2. **Idle gating (the invariant this task must not lose):** the feed enumerates `IDashboardSnapshotService.WatchSnapshotsAsync` on a background task started when the subscriber count goes 0→1 and cancelled when it goes 1→0. While zero subscribers, the feed holds no timer and builds no snapshot. Guard subscriber add/remove with a plain lock; restart cleanly on resubscribe (mirror the start/stop discipline of `GatewayAlarmMonitor.StreamAsync` registration, `GatewayAlarmMonitor.cs:739-752`). If the underlying watch throws or completes, complete all subscriber channels with the error and reset so the next subscriber restarts it (mirror `DashboardSnapshotPublisher.ExecuteAsync`'s reconnect-after-delay posture, but per-feed). +3. `DashboardPageBase`: remove the HubConnection path (`:62` and the factory usage); keep the synchronous first render via `snapshotService.GetSnapshot()` (`:37`); then a background loop `await foreach (var s in feed.WatchAsync(_cts.Token)) { Snapshot = s; await InvokeAsync(StateHasChanged); }` started in `OnAfterRenderAsync(firstRender)` or `OnInitializedAsync` (match current lifecycle), cancelled + awaited in `DisposeAsync`. Update the class XML doc that narrates the hub subscription history (`:7-14`). +4. Hubs, `DashboardSnapshotPublisher`, `DashboardSnapshotHubConnectionCounter`, `DashboardHubConnectionFactory`, and `/hubs/token` all stay — they remain the remote/external surface. Do not touch them. +5. Auth: the pages are mapped behind `ViewerPolicy` (`DashboardEndpointRouteBuilderExtensions.cs:136`), which remains the gate for in-process consumption; add one comment on `WatchAsync` saying so. +6. Tests (`DashboardSnapshotFeedTests`): (a) zero subscribers → underlying service's `WatchSnapshotsAsync` never enumerated (fake service counts enumerations/`MoveNextAsync`); (b) first subscriber starts exactly one enumeration; two subscribers share it; (c) last unsubscribe cancels it; resubscribe restarts it; (d) slow subscriber observes latest-wins (push 3 snapshots, read 1, it is the newest) while a fast subscriber sees all; (e) underlying fault completes subscribers with the error and a fresh subscriber restarts. + +**Steps:** write feed tests first (fail) → implement feed → page switch → `dotnet test ... --filter "FullyQualifiedName~DashboardSnapshotFeed"` then `--filter "FullyQualifiedName~Dashboard"` (whole dashboard test folder) → commit `feat(dashboard): in-process snapshot feed replaces the pages' loopback /hubs/snapshot hop`. + +--- + +### Task 6: In-process session event subscription + SessionDetailsPage switch + +**Classification:** high-risk +**Estimated implement time:** ~8 min +**Parallelizable with:** Task 5, Task 7 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs` +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/EventsHubViewerRegistry.cs` (only if a member is needed for synthetic connection ids; prefer reusing the existing API) +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor` (the `/hubs/events` connection at `:271,297`) +- Test: `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardEventBroadcasterTests.cs` (extend) + +**Why:** `SessionDetailsPage` opens a loopback `/hubs/events` connection. The broadcaster already short-circuits on `!viewerRegistry.HasViewers(sessionId)` BEFORE the redaction deep clone (`DashboardEventBroadcaster.cs:51-56`) — the mirror viewer gating shipped last round. An in-process subscription must keep feeding that registry or every unwatched session pays `MxEvent.Clone()` per event again. + +**Spec:** +1. Add to `DashboardEventBroadcaster` an in-process subscribe API: `IDashboardEventSubscription Subscribe(string sessionId)` returning a disposable that exposes `ChannelReader Reader` (bounded, capacity ~256, `DropOldest` — this is a UI mirror, loss is acceptable and already documented for the hub path). On subscribe: register a synthetic connection id (e.g. `"inproc-" + Guid.NewGuid().ToString("N")`) with `EventsHubViewerRegistry.AddViewer(connectionId, sessionId)`; on dispose: `RemoveViewer` + `ReleaseConnection` in the order the hub uses (`EventsHub.cs:86,99`). Registry stays the single source of truth for `HasViewers`. +2. `Publish` (`:39-86`): after the existing `HasViewers` check and the clone-then-redact (`RedactValues` `:97-109`), `TryWrite` the SAME redacted clone to each in-process subscriber of that session, in addition to the hub group send. The source `MxEvent` is shared with the gRPC stream and replay ring — the existing never-mutate-in-place rule holds; in-process subscribers receive the redacted clone only. +3. `SessionDetailsPage`: replace the HubConnection + `SubscribeSession` invoke with `broadcaster.Subscribe(SessionId)` and a read loop marshalling to the renderer via `InvokeAsync(StateHasChanged)`; dispose the subscription in `DisposeAsync`. Keep the existing per-session ACL posture (any Viewer may watch any session — SEC-25 is tracked separately; do not widen or narrow it here). +4. Tests to add in `DashboardEventBroadcasterTests`: (a) in-process subscriber receives the redacted event when `ShowTagValues=false` and the source event is not mutated; (b) subscribing flips `HasViewers` so `Publish` stops short-circuiting (proves mirror gating integration); (c) disposing the last in-process subscriber restores the no-viewers short-circuit (no clone, no send — reuse the existing `Publish_WithNoRegisteredViewers_DoesNotCloneOrSend` fake pattern); (d) hub viewers and in-process viewers are independently counted. + +**Steps:** tests first → implement → `dotnet test ... --filter "FullyQualifiedName~DashboardEventBroadcaster"` + `--filter "FullyQualifiedName~EventsHubViewerRegistry"` + `--filter "FullyQualifiedName~GatewaySessionDashboardMirror"` → commit `feat(dashboard): in-process session event subscription feeds the viewer registry — SessionDetailsPage drops its /hubs/events hop`. + +--- + +### Task 7: AlarmsPage provider-status via IGatewayAlarmService + +**Classification:** standard +**Estimated implement time:** ~4 min +**Parallelizable with:** Task 5, Task 6 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/AlarmsPage.razor` (`:194` HubConnection, `:281-304` poll loop untouched) + +**Why:** `AlarmsPage` opens `/hubs/alarms` but only consumes `ProviderStatus` payloads from it (alarm rows come from the 3 s `QueryAlarmsAsync` poll). `IGatewayAlarmService.StreamAsync` (`GatewayAlarmMonitor.cs:724-777`) is already a true multi-subscriber in-process fan-out. + +**Spec:** Replace the HubConnection with a background loop over `alarmService.StreamAsync(alarmFilterPrefix: null, ct)`, handling only `PayloadOneofCase.ProviderStatus` (skip snapshot/live alarm payloads — the poll stays authoritative for rows). The monitor's drop policy completes a lagging subscriber's channel (`:700-712`): on completion or fault, delay ~1 s and resubscribe (matching the hub path's `WithAutomaticReconnect` posture). Dispose via the page's existing cancellation. Leave the poll loop alone. + +**Steps:** edit → `dotnet build src/ZB.MOM.WW.MxGateway.Server` → `dotnet test ... --filter "FullyQualifiedName~Alarms"` → commit `feat(dashboard): AlarmsPage reads provider status from IGatewayAlarmService in-process`. + +--- + +### Task 8: Dashboard design-doc update (consolidated) + +**Classification:** small +**Estimated implement time:** ~4 min +**Parallelizable with:** none (runs after 5, 6, 7 land) + +**Files:** +- Modify: `docs/GatewayDashboardDesign.md` (sections at ~:112-114, :162-178, :190-217, :228-247, :535-541, :581-595) + +**Spec:** Rewrite the affected sections to describe: pages consume in-process seams (`IDashboardSnapshotFeed`, `DashboardEventBroadcaster.Subscribe`, `IGatewayAlarmService.StreamAsync`); the three hubs and `/hubs/token` remain as the remote/external surface; idle gating is now two-tier (hub connection counter gates the hub publisher; feed subscriber count gates the in-process pump — while nobody watches, neither builds a snapshot); mirror gating counts hub viewers AND in-process viewers through the one registry; clone-then-redact still happens once in the broadcaster before any delivery; ViewerPolicy on the component endpoint is the in-process auth gate; SEC-25 per-session ACL gap unchanged. Present tense, why-not-what, no marketing. + +**Commit:** `docs(dashboard): in-process page feeds, two-tier idle gating, hubs as the external surface` + +--- + +### Task 9: Phase A gate — full gateway suite on macOS + +**Classification:** trivial (verification only) +**Parallelizable with:** none (after Tasks 1-8) + +Run `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx` (expect 0 warnings) and the full `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj` (expect ≥1046 passed, 0 failed; new feed/broadcaster tests raise the count). Fix-forward any failure before Phase B. + +--- + +## Phase B — worker (net48 x86, verified on windev) + +### Task 10: Control-frame completion decoupling in WorkerFrameWriter + +**Classification:** high-risk +**Estimated implement time:** ~6 min +**Parallelizable with:** Task 11, Task 12 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs` +- Modify: `src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs` +- Modify: `docs/WorkerFrameProtocol.md` (~:120-131 completion-semantics paragraph) + +**Why:** Wire ordering is already correct — `DequeueNext` (`:383-413`) re-checks `_controlFrames` before every frame. The coupling is completion latency: `DrainQueuedFramesAsync` (`:304-361`) defers the single `FlushAsync` and ALL `TrySetResult` calls to after the whole drain pass, so a heartbeat/command-reply/fault/shutdown-ack `Task` awaited by its writer does not resolve until up to 128 event frames behind it are written and flushed. The XML docs claim "never delayed behind an event backlog" — true of bytes, false of the awaited completion. + +**Spec:** +1. Record the priority class on `PendingFrame` (`:23-48`), set at construction in `WriteAsync` (`:109`) and `WriteBatchAsync` (`:192`). +2. In `DrainQueuedFramesAsync`: when `DequeueNext` returns an `Event` frame while `written` contains one or more not-yet-completed `Control` frames, first `FlushAsync` + complete + clear `written`, then continue draining. Exit-path flush at `:339-360` unchanged. Net effect: a control frame's completion never waits on an event frame dequeued after it; the pure-event 128-batch hot path still pays exactly one flush (guarded by the existing `WriteAsync_WhenBatchDrainedTogether_FlushesOnce` and `EventBurst_DrainLoopCoalescesFlushes`); a pure-control burst still pays one flush. Do NOT flush per control frame unconditionally — that reintroduces the pre-WRK-12 syscall-per-heartbeat cost. +3. Failure handling: `FailFrames(written, ...)` / `FailAllQueued` (`:327-336`) operate on the current `written` list; after an early flush+complete+clear, frames already completed must not be failable — verify the clear ordering makes that structurally true, and extend the fault-injection tests if the early-flush path adds a new failure window (a `FlushAsync` fault with a partially-completed pass). +4. New test (use the existing `GatedWriteStream` harness ~`:880`): queue a control frame behind N gated event frames within one drain pass; assert the control frame's `WriteAsync` task completes before the last event write is released. Keep all 9 existing writer tests green — sequence stamping (`:431-483`), claim/tombstone interlock (`:244-274`), and wire order must be untouched. +5. `docs/WorkerFrameProtocol.md`: update the completion-semantics paragraph — completion now resolves at the class-transition flush, still meaning "written AND flushed". + +**Commit:** `perf(worker): control-frame completions resolve at the class-transition flush, not after the event batch` + +--- + +### Task 11: Worker pipe-read teardown — dispose-to-unblock and observe the abandoned read + +**Classification:** high-risk +**Estimated implement time:** ~8 min +**Parallelizable with:** Task 10, Task 12 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs` (`RunMessageLoopAsync` `:267-310`, ctor `:55-68`) +- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeClient.cs` (`:143-159`) — only if ownership must move; prefer not +- Modify: `src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs` +- Modify: `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs` (comment only) + +**Why:** On net48, `NamedPipeClientStream.ReadAsync` ignores its `CancellationToken` (`WorkerFrameReader.cs:109-111`). Fault-path exits (event-drain fault, oversized event, watchdog, heartbeat write failure) leave `readTask` pending; it is unblocked only when `WorkerPipeClient`'s `using` disposes the pipe, at which point it faults with `ObjectDisposedException`/`IOException` on a Task nobody observes (the finally at `:303-309` awaits only heartbeat and drain). The frame-pooling change (GWC-30) makes this sharper: the abandoned read owns the per-instance `_lengthPrefix` buffer and possibly a rented ArrayPool payload — the reader's single-consumer invariant holds today only because nothing ever reads again after abandonment. + +**Spec — constraints, implementer designs within them:** +1. **No unobserved faulted Task.** After the stream is disposed, `readTask`'s fault must be awaited/observed (reuse `ObserveBackgroundTaskStopAsync`'s timeout-and-log shape, `:312-348`) before `WorkerPipeClient.RunAsync` returns. +2. **Ordering: final writes complete before disposal.** The shutdown ack (`WriteShutdownAckAsync` `:1064-1069`) and fault frames (`TryWriteFaultAsync` `:1164+`) are written after the message loop exits on some paths — trace every exit path and place the stream disposal AFTER the last possible write on each. The clean design: `WorkerPipeSession` keeps a reference to the ctor `Stream`; `RunAsync`'s outermost finally (after runtime-session disposal and any fault write, `:133-145`) disposes the stream and then observes `readTask` (stored in a field by `RunMessageLoopAsync`). `WorkerPipeClient`'s `using` then double-disposes harmlessly. If the trace shows a fault write that happens in `WorkerPipeClient` after `session.RunAsync` returns (there is none known), fall back to moving observation into `WorkerPipeClient`. +3. **Never a second read.** After abandonment, no code path may call `_reader.ReadAsync` again (pooled-buffer use-after-return). The message loop already guarantees this (`return` before reassignment on the graceful path); keep it structurally true and assert it in a comment on `_lengthPrefix` (`WorkerFrameReader.cs:23-25`). +4. **Graceful path unchanged:** `WorkerShutdown`/`ShutdownWorker` exits have no pending read; disposal+observation must be a no-op there (observe a completed/absent task). +5. Document the net48 token-ignoring fact where the read is issued (`RunMessageLoopAsync` and/or `ReadExactlyOrThrowAsync`) — the research found zero comments acknowledging it. +6. Tests (net48 project, real `PipePair` harness `:2433-2485`): (a) fault-path exit (reuse the `RunAsync_EventFrameTooLarge_...` shape `:868`) — assert `RunAsync` completes within the existing 5 s bound AND, via a `TaskScheduler.UnobservedTaskException` hook armed in the test with a forced GC, that no unobserved exception leaks; (b) graceful shutdown still completes with no pending read; (c) the session disposes the stream (harness observes the gateway-side stream faulting its own pending read promptly rather than at `PipePair.Dispose`). + +**Commit:** `fix(worker): session-owned stream disposal unblocks and observes the net48 pipe read at teardown` + +--- + +### Task 12: Value-cache clone removal per the aliasing audit + +**Classification:** standard +**Estimated implement time:** ~5 min +**Parallelizable with:** Task 10, Task 11 + +**Files:** +- Modify: `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs` (`Set` `:82,83,97`; `CachedValue` `:275`) +- Modify: `src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessValueCacheTests.cs` (rewrite the `:58` test) +- Modify: `src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessCommandExecutorTests.cs` (add cached-read test) + +**Why (audit result):** All three clones in `Set` — `Value.Clone()` (deep, recursive for arrays), `SourceTimestamp.Clone()`, `Statuses.Clone()` (container + N proxies) — are removable. The event is fully stamped BEFORE `Set` runs (`Enqueue` at `MxAccessBaseEventSink.cs:263` precedes `postPublish` at `:288`; sequence/timestamp stamped inside `Enqueue`, `MxAccessEventQueue.cs:269-270`) and the queue's ownership invariant forbids later mutation. The alias already exists on the read side: `SucceededRead` (`MxAccessSession.cs:1086,1091,1096`) hands the cache's own `Value`/`SourceTimestamp` instances into every `BulkReadResult`, which downstream only wraps and serializes. Worker↔gateway is a process boundary — no gateway consumer can alias. + +**Spec:** +1. Remove all three clones; `CachedValue` stores the event's own references. +2. Ownership contract comment on `Set` and on `CachedValue`: the cache holds borrowed references into an enqueued, write-once `MxEvent`; consumers may read and serialize, never mutate; mutation would additionally invalidate `QueuedEvent.Size` — the enqueue-time memoized serialized size that the byte-budgeted `Drain` charges (`MxAccessEventQueue.cs:499-506`), so a grown message could overshoot the negotiated frame max and fault the session via `MessageTooLarge`. +3. Rewrite `Set_StoresIndependentSnapshot_UnaffectedByLaterEventMutation` (`:58` — it codifies the invariant being reversed) into the aliasing contract: `Set` then `TryGet` returns the same `Value`/`SourceTimestamp`/`Statuses`-element instances (`Assert.Same`), with the doc comment explaining the write-once borrow. +4. Add the missing cached-read-path test in `MxAccessCommandExecutorTests`: seed the cache, dispatch a `ReadBulk` that hits `TryGetCachedReadFor` → assert `WasCached == true` and `result.Value` is reference-equal to the cached instance (closing the coverage gap the audit found — nothing today exercises `WasCached == true` end-to-end in the worker). +5. `MxAccessWriteCompletionCache.Record`'s parallel `statuses.Clone()` (`:76`) is left AS-IS deliberately (different lifecycle, not in the finding) — add one cross-reference comment there pointing at the value-cache ownership contract. + +**Commit:** `perf(worker): value cache borrows the write-once event's instances — three clones per OnDataChange removed` + +--- + +### Task 13: Phase B gate — windev full verification + +**Classification:** trivial (verification only) +**Parallelizable with:** none (after Tasks 10-12; Phase A gate must be green) + +Push the branch to origin, then on windev (`ssh windev`, clone `C:\build\mxaccessgw-ci`): fetch + checkout the branch; `dotnet build src/ZB.MOM.WW.MxGateway.slnx` (0 warnings); `dotnet build src/ZB.MOM.WW.MxGateway.Worker/... -p:Platform=x86`; `dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/... -p:Platform=x86` (expect 501+ passed, 0 failed — new tests raise the count); `dotnet test src/ZB.MOM.WW.MxGateway.Tests/...` (expect **0 failed including SecretsStorePathGuardTests** — the Task 1 proof). Known caveat: the reconnect-replay test is load-sensitive on windev; re-run isolated before treating it as a regression (documented in `docs/GatewayTesting.md`). + +--- + +### Task 14: Wrap-up — deferred table closure, docs sweep, final review + +**Classification:** small +**Parallelizable with:** none (last) + +- Append a closure note to `docs/plans/2026-08-15-perf-review-remediation.md`'s deferred table (one line: resolved by this plan, date, branch). +- Sweep: `gateway.md` / `docs/WorkerFrameProtocol.md` / `docs/GatewayDashboardDesign.md` / `docs/GatewayTesting.md` consistency with as-built behavior; record any accepted deviations in THIS plan's "As-built notes" section (add it). +- Update `.tasks.json` statuses; update auto-memory (`perf-remediation-branch.md` or successor) with the branch state. +- Dispatch the final integration code review (Opus) over `git diff main..perf/deferred-remediation` before reporting done. Merge remains the user's decision. + +--- + +## Explicitly out of scope + +| Item | Why | +|---|---| +| wnwrap alarm GUID identity semantics; `ALARM_RECORDS/@COUNT` probe | Need live alarms on windev — external state this plan cannot provide. Still tracked in the prior plan's follow-ups. | +| Structural alarm-truncation degraded-status signal | Contract-level design (proto change candidate) — separate effort. | +| SEC-25 per-session dashboard event ACL | Security roadmap item; Task 6 deliberately preserves the current posture. | +| `MxAccessWriteCompletionCache` clone | Different lifecycle than the value cache; consciously kept (Task 12.5). | diff --git a/docs/plans/2026-08-15-deferred-remediation.md.tasks.json b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json new file mode 100644 index 0000000..86bcb1b --- /dev/null +++ b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json @@ -0,0 +1,20 @@ +{ + "planPath": "docs/plans/2026-08-15-deferred-remediation.md", + "tasks": [ + { "id": 1, "subject": "Task 1: Windows-safe cleanup in SecretsStorePathGuardTests", "status": "pending" }, + { "id": 2, "subject": "Task 2: SessionEventDistributor _subscribers to plain Dictionary", "status": "pending" }, + { "id": 3, "subject": "Task 3: Merge the session event-source pass-through iterator", "status": "pending" }, + { "id": 4, "subject": "Task 4: EventStreamService direct channel reads in the live loop", "status": "pending" }, + { "id": 5, "subject": "Task 5: In-process dashboard snapshot feed + page switch", "status": "pending" }, + { "id": 6, "subject": "Task 6: In-process session event subscription + SessionDetailsPage switch", "status": "pending" }, + { "id": 7, "subject": "Task 7: AlarmsPage provider-status via IGatewayAlarmService", "status": "pending" }, + { "id": 8, "subject": "Task 8: Dashboard design-doc update (consolidated)", "status": "pending", "blockedBy": [5, 6, 7] }, + { "id": 9, "subject": "Task 9: Phase A gate — full gateway suite on macOS", "status": "pending", "blockedBy": [1, 2, 3, 4, 5, 6, 7, 8] }, + { "id": 10, "subject": "Task 10: Control-frame completion decoupling in WorkerFrameWriter", "status": "pending", "blockedBy": [9] }, + { "id": 11, "subject": "Task 11: Worker pipe-read teardown — dispose-to-unblock and observe", "status": "pending", "blockedBy": [9] }, + { "id": 12, "subject": "Task 12: Value-cache clone removal per the aliasing audit", "status": "pending", "blockedBy": [9] }, + { "id": 13, "subject": "Task 13: Phase B gate — windev full verification", "status": "pending", "blockedBy": [10, 11, 12] }, + { "id": 14, "subject": "Task 14: Wrap-up — deferred table closure, docs sweep, final review", "status": "pending", "blockedBy": [13] } + ], + "lastUpdated": "2026-08-15T00:00:00Z" +} From 25f07f89ddea82e9e5b2ac9fc930ae429b3c607c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:04:59 -0400 Subject: [PATCH 02/23] =?UTF-8?q?fix(tests):=20clear=20the=20SQLite=20pool?= =?UTF-8?q?=20before=20deleting=20the=20secrets=20path-guard=20temp=20dir?= =?UTF-8?q?=20=E2=80=94=20Windows=20sharing=20violation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/GatewayTesting.md | 12 ++++++------ .../SecretsStorePathGuardTests.cs | 19 ++++++++++++++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/docs/GatewayTesting.md b/docs/GatewayTesting.md index a543d02..8679b12 100644 --- a/docs/GatewayTesting.md +++ b/docs/GatewayTesting.md @@ -557,12 +557,12 @@ real-clock deadlines into failures. ### Two more findings from the 2026-08-15 windev gate - `SecretsStorePathGuardTests.CreateBuilder_AcceptsSecretsStoreOutsideContentRoot_AndCreatesIt` - fails **deterministically on Windows, on `main` as well as on any branch**, so it is not a - signal about the change under test. Creating the builder opens `secrets.db`, and - `Microsoft.Data.Sqlite`'s connection pool keeps the file handle alive past the test body, - so the recursive directory delete in the cleanup hits a still-open file — a sharing - violation Windows enforces and Unix does not. Pre-existing and tracked separately; do not - chase it as a regression. Subtract it from the expected pass count on Windows. + used to fail deterministically on Windows: creating the builder opens `secrets.db`, + `Microsoft.Data.Sqlite`'s connection pool kept the file handle alive past the test body, and + the cleanup's recursive directory delete hit a sharing violation Windows enforces and Unix + does not. The cleanup now clears the SQLite connection pool before deleting the temp + directory (the same pattern as `TempDatabaseDirectory` and `PreHostSecretExpansionTests`), + so the test passes on Windows and macOS alike — count it as a pass on both. - The `StaWaitHelper` timing tests (`WaitForSignalOrMessages_*`) flake on a loaded box with a signature that reads like a broken wait but is not: the helper wakes on *input being present*, so a message posted to the test thread ends the wait early. That is the helper diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs index d5aabcd..44da881 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs @@ -100,7 +100,24 @@ public sealed class SecretsStorePathGuardTests finally { Environment.SetEnvironmentVariable(SqlitePathVariable, original); - Directory.Delete(directory, recursive: true); + + // The store runs in WAL mode with connection pooling, so a pooled handle can outlive the + // migration and keep secrets.db (plus its -wal/-shm sidecars) open. Windows refuses to + // delete a directory holding open files where Unix does not, so clear the pool first; + // the catch is belt-and-braces for a sidecar whose handle outlasts even that. + Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools(); + try + { + Directory.Delete(directory, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup of the temp store; a locked file must not fail the test. + } + catch (UnauthorizedAccessException) + { + // Best-effort cleanup of the temp store; a locked file must not fail the test. + } } } From 935f002dbfd3738092de1313b484f92de48dfdaa Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:06:48 -0400 Subject: [PATCH 03/23] =?UTF-8?q?perf(grpc):=20consume=20the=20subscriber?= =?UTF-8?q?=20channel=20directly=20in=20StreamEventsAsync=20=E2=80=94=20dr?= =?UTF-8?q?ops=20the=20ReadAllAsync=20iterator=20hop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Grpc/EventStreamService.cs | 61 ++++++++++++------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs index a41f67b..ea1cf48 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs @@ -1,4 +1,5 @@ using System.Runtime.CompilerServices; +using System.Threading.Channels; using Microsoft.Extensions.Options; using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.MxGateway.Server.Configuration; @@ -37,7 +38,7 @@ public sealed class EventStreamService( // non-blocking. When this subscriber's channel is full the pump applies the per-subscriber // backpressure policy and completes this subscriber's channel with a SessionManagerException // (SessionManagerErrorCode.EventQueueOverflow). That terminal fault surfaces here when the - // reader's MoveNextAsync throws, and it propagates to the gRPC client unchanged. The overflow + // reader's WaitToReadAsync throws, and it propagates to the gRPC client unchanged. The overflow // metric, and (in the legacy single-subscriber FailFast case) the session fault + fault metric, // are recorded by the distributor's overflow handler so the session, the pump, and other // subscribers are isolated from this subscriber's slowness. @@ -106,9 +107,14 @@ public sealed class EventStreamService( options.Value.Sessions.MaxEventSubscribersPerSession); } - IAsyncEnumerator reader = subscriber.Reader - .ReadAllAsync(cancellationToken) - .GetAsyncEnumerator(cancellationToken); + // Consume the subscriber channel directly (WaitToReadAsync + an inner TryRead drain) + // rather than through ReadAllAsync's IAsyncEnumerable wrapper. This is the hottest + // per-event path in the gateway and the wrapper added a second async state machine hop + // per event for no behavioral benefit: WaitToReadAsync observes cancellation and a + // faulted completion exactly as MoveNextAsync did, and TryRead drains what is already + // buffered without allocating a wait. StreamEventsAsync itself stays an async iterator — + // its `yield return` is what feeds the gRPC writer. + ChannelReader reader = subscriber.Reader; // GWC-15: register this subscriber's channel as a live backlog source instead of // reconciling the queue-depth gauge on every event. The gauge previously read the @@ -151,15 +157,14 @@ public sealed class EventStreamService( while (true) { - MxEvent mxEvent; + bool hasMore; try { - if (!await reader.MoveNextAsync().ConfigureAwait(false)) - { - break; - } - - mxEvent = reader.Current; + // A cleanly completed channel returns false here (end of stream); a channel + // completed WITH a fault rethrows that fault from the wait once the buffer + // is drained — the same surface MoveNextAsync presented, so the terminal + // SessionManagerException(EventQueueOverflow) still propagates unchanged. + hasMore = await reader.WaitToReadAsync(cancellationToken).ConfigureAwait(false); } catch (WorkerClientException workerException) { @@ -173,24 +178,36 @@ public sealed class EventStreamService( throw; } - // Per-RPC filter stays at the subscriber boundary: each request may resume - // from a different AfterWorkerSequence, so the shared pump fans raw events and - // this loop drops the ones at or below the caller's watermark. - if (mxEvent.WorkerSequence <= afterWorkerSequence) + if (!hasMore) { - continue; + break; } - // The queue-depth gauge is maintained lazily via the backlog registration above - // (GWC-15): the metric reads this subscriber's channel Count only when scraped, - // so there is no per-event gauge bookkeeping on this hot path. - yield return mxEvent; + // Drain everything already buffered before waiting again. TryRead never throws; + // a fault left on the channel is observed by the next WaitToReadAsync above. + while (reader.TryRead(out MxEvent? mxEvent)) + { + // Per-RPC filter stays at the subscriber boundary: each request may resume + // from a different AfterWorkerSequence, so the shared pump fans raw events + // and this loop drops the ones at or below the caller's watermark. It + // applies to every live event, drained or awaited alike. + if (mxEvent.WorkerSequence <= afterWorkerSequence) + { + continue; + } + + // The queue-depth gauge is maintained lazily via the backlog registration + // above (GWC-15): the metric reads this subscriber's channel Count only when + // scraped, so there is no per-event gauge bookkeeping on this hot path. + yield return mxEvent; + } } } finally { - await reader.DisposeAsync().ConfigureAwait(false); - + // Nothing to dispose for the reader: consuming the ChannelReader directly means + // there is no enumerator wrapper holding the cancellation registration. + // // Remove this subscriber's live backlog contribution before disposing the lease so // the gauge stops counting a channel that is about to be completed; after this the // gauge reflects only the remaining subscribers (zero when none remain). From 94dbe9f1af4d3579ca2def4b0ccdc38eafc9b1b2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:07:11 -0400 Subject: [PATCH 04/23] =?UTF-8?q?perf(sessions):=20fold=20the=20ReadEvents?= =?UTF-8?q?Async=20pass-through=20into=20MapWorkerEventsAsync=20=E2=80=94?= =?UTF-8?q?=20one=20fewer=20iterator=20per=20event?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Sessions/GatewaySession.cs | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs index 0b9fa38..ac426a4 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs @@ -764,11 +764,24 @@ public sealed class GatewaySession // The distributor's single event source. Drains the worker event stream once (the // distributor guarantees a single consumer) and maps each frame to the public MxEvent, // preserving worker order. Mirrors the former ProduceEventsAsync mapping exactly. + // + // This deliberately duplicates the three lines of ReadEventsAsync rather than enumerating + // it: every worker event crosses this source, and routing it through a second pure + // pass-through iterator cost two extra MoveNextAsync state-machine hops per event for no + // semantic value. ReadEventsAsync stays for ISessionManager.ReadEventsAsync; keep the two + // bodies in step. Only one of them may run per attach — WorkerClient.ReadEventsAsync + // single-reader-claims the event channel and throws on a second consumer — and on the + // distributor path that one consumer is this method. private async IAsyncEnumerable MapWorkerEventsAsync( [EnumeratorCancellation] CancellationToken cancellationToken) { MxAccessGrpcMapper mapper = _eventStreaming.Mapper; - await foreach (WorkerEvent workerEvent in ReadEventsAsync(cancellationToken) + IWorkerClient workerClient = await GetReadyWorkerClientAsync(cancellationToken).ConfigureAwait(false); + TouchClientActivity(_eventStreaming.TimeProvider.GetUtcNow()); + + await foreach (WorkerEvent workerEvent in workerClient + .ReadEventsAsync(cancellationToken) + .WithCancellation(cancellationToken) .ConfigureAwait(false)) { yield return mapper.MapEvent(workerEvent); @@ -1513,6 +1526,13 @@ public sealed class GatewaySession /// Reads events from the worker as an asynchronous enumerable stream. /// /// Token to cancel the asynchronous operation. + /// + /// Backs ISessionManager.ReadEventsAsync. The distributor does not come + /// through here — MapWorkerEventsAsync inlines this body to save a per-event + /// iterator hop, so changes made here belong there too. The two are mutually exclusive + /// per attach: claims the worker event + /// channel for a single reader and throws on the second consumer. + /// /// An asynchronous stream of worker events. public async IAsyncEnumerable ReadEventsAsync( [EnumeratorCancellation] CancellationToken cancellationToken) From 59420d8568a925f69e443d98145d6812cb353214 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:07:15 -0400 Subject: [PATCH 05/23] =?UTF-8?q?refactor(sessions):=20=5Fsubscribers=20to?= =?UTF-8?q?=20plain=20Dictionary=20=E2=80=94=20every=20access=20is=20under?= =?UTF-8?q?=20=5FlifecycleLock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Sessions/SessionEventDistributor.cs | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs index a7b653a..e2bc100 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using System.Threading.Channels; using ZB.MOM.WW.MxGateway.Contracts.Proto; @@ -66,18 +65,20 @@ public delegate void SubscriberOverflowHandler(bool isOnlySubscriber, bool isInt /// EventStreamService.ProduceEventsAsync ordering. /// /// -/// Concurrency. The subscriber set is a -/// keyed by a monotonic id, used -/// for keyed add/remove only. Fan-out does NOT enumerate the dictionary: every -/// mutation (, , lease -/// disposal, overflow disconnect) happens inside the _lifecycleLock critical -/// section and rebuilds an immutable copy-on-write Subscriber[] snapshot, -/// which the pump reads once per event. This matters because -/// ConcurrentDictionary.Values is a PROPERTY that acquires every internal -/// lock and materializes a fresh List plus a read-only wrapper on each call -/// — per event, on the hot fan-out path. The subscriber set is tiny (one to a -/// handful) and mutates rarely, so paying a full array rebuild per registration to -/// make fan-out a bare array walk is the right trade. No lock is held across an +/// Concurrency. The subscriber set is a plain +/// keyed by a monotonic id, used for keyed +/// add/remove only. It needs no concurrent collection type because it is never +/// touched outside the _lifecycleLock critical section: every mutation +/// (, , lease disposal, +/// overflow disconnect) and every read (the terminal completion sweep, the snapshot +/// rebuild) holds that lock, and each mutation rebuilds an immutable copy-on-write +/// Subscriber[] snapshot inside the same section. The lock-free readers see only that snapshot, +/// never the dictionary: the pump reads it once per event and +/// reads its length. Fan-out therefore does NOT +/// enumerate the dictionary — it walks a captured array, with no dictionary +/// traversal and no per-event allocation on the hot path. The subscriber set is +/// tiny (one to a handful) and mutates rarely, so paying a full array rebuild per +/// registration to buy that is the right trade. No lock is held across an /// await. Each subscriber channel has a single writer — the pump — so /// per-channel writes never race. A subscriber registered after the pump captured /// the array for the in-flight event misses that event, which matches "late @@ -104,7 +105,11 @@ public sealed class SessionEventDistributor : IAsyncDisposable private readonly TimeSpan _shutdownTimeout; private readonly ILogger _logger; private readonly TimeProvider _timeProvider; - private readonly ConcurrentDictionary _subscribers = new(); + // Keyed subscriber set. Touched ONLY under _lifecycleLock (add in RegisterSubscriber and + // RegisterWithReplay, remove in RemoveSubscriber, read in CompleteAllSubscribers and + // RebuildSubscriberSnapshot), which is why a plain Dictionary suffices: lock-free readers + // never see this field, they read _subscriberSnapshot below. + private readonly Dictionary _subscribers = []; private readonly CancellationTokenSource _shutdownCts = new(); private readonly object _lifecycleLock = new(); @@ -120,7 +125,7 @@ public sealed class SessionEventDistributor : IAsyncDisposable // may legitimately observe the previous array, which IS the documented "late subscribers // see events after they register" window. Where visibility must be guaranteed — the // RegisterWithReplay handoff — it comes from the _replayLock edge, not from Volatile. - // See the type remarks for why fan-out must not touch ConcurrentDictionary.Values. + // See the type remarks for why fan-out walks this array instead of enumerating _subscribers. private Subscriber[] _subscriberSnapshot = []; // Replay ring buffer. Appended on the pump thread and queried from arbitrary @@ -295,9 +300,10 @@ public sealed class SessionEventDistributor : IAsyncDisposable /// (gRPC) subscribers and excludes the internal dashboard subscriber. /// /// - /// Read from the copy-on-write snapshot rather than ConcurrentDictionary.Count - /// (which acquires every internal lock). The snapshot is rebuilt in the same - /// _lifecycleLock section that mutates the dictionary, so the two never diverge. + /// Read from the copy-on-write snapshot rather than the dictionary, because this + /// property is a lock-free reader and the dictionary may only be touched under + /// _lifecycleLock. The snapshot is rebuilt in the same _lifecycleLock + /// section that mutates the dictionary, so the two never diverge. /// public int SubscriberCount => Volatile.Read(ref _subscriberSnapshot).Length; @@ -648,7 +654,7 @@ public sealed class SessionEventDistributor : IAsyncDisposable // register". A subscriber UNREGISTERED after the capture is still written to, // and TryWrite on its completed channel returns false — from here that is // indistinguishable from a real overflow. The window predates the - // copy-on-write array (ConcurrentDictionary.Values materialized its list up + // copy-on-write array (enumerating the dictionary materialized its values up // front too) and its outcome is NOT benign, so telling a graceful unregister // apart from a genuine overflow is OnSubscriberOverflow's job, not this // loop's. @@ -796,7 +802,7 @@ public sealed class SessionEventDistributor : IAsyncDisposable { lock (_lifecycleLock) { - if (!_subscribers.TryRemove(subscriber.Id, out _)) + if (!_subscribers.Remove(subscriber.Id)) { return false; } @@ -808,7 +814,8 @@ public sealed class SessionEventDistributor : IAsyncDisposable // Republishes the fan-out array from the current dictionary contents. MUST be called with // _lifecycleLock held — holding that lock across the dictionary mutation and this rebuild is - // what keeps the array and the dictionary from diverging. + // what keeps the array and the dictionary from diverging, and it is also what makes the plain + // (non-concurrent) Dictionary safe: this enumeration never races a mutation. private void RebuildSubscriberSnapshot() => Volatile.Write(ref _subscriberSnapshot, [.. _subscribers.Values]); From d44fe1d6b5a0e17abab957a4c122c18e2811a4f4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:10:51 -0400 Subject: [PATCH 06/23] feat(dashboard): AlarmsPage reads provider status from IGatewayAlarmService in-process --- .../Components/Pages/AlarmsPage.razor | 86 ++++++++++++------- 1 file changed, 53 insertions(+), 33 deletions(-) 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 93180af..2aecd24 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 @@ -1,10 +1,9 @@ @page "/alarms" @implements IAsyncDisposable -@using Microsoft.AspNetCore.SignalR.Client -@using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs +@using ZB.MOM.WW.MxGateway.Server.Alarms @inject IDashboardLiveDataService LiveData @inject IOptions GatewayOptions -@inject DashboardHubConnectionFactory HubFactory +@inject IGatewayAlarmService AlarmService Dashboard Alarms @@ -173,13 +172,13 @@ private Task? _pollTask; private DashboardAlarmProviderStatus _providerStatus = DashboardAlarmProviderStatus.Healthy; - private HubConnection? _alarmsHub; + private Task? _providerStatusTask; /// protected override void OnInitialized() { _pollTask = PollLoopAsync(); - _ = AttachAlarmsHubAsync(); + _providerStatusTask = ProviderStatusLoopAsync(); } private string? ProviderStatusTitle() @@ -189,26 +188,48 @@ : null; } - private async Task AttachAlarmsHubAsync() + // 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() { - _alarmsHub = HubFactory.Create("/hubs/alarms"); - _alarmsHub.On(AlarmsHub.AlarmMessage, async message => + while (!_cts.IsCancellationRequested) { - if (message.PayloadCase == AlarmFeedMessage.PayloadOneofCase.ProviderStatus) + try { - _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); - await InvokeAsync(StateHasChanged).ConfigureAwait(false); - } - }); + await foreach (AlarmFeedMessage message in AlarmService + .StreamAsync(alarmFilterPrefix: null, _cts.Token) + .ConfigureAwait(false)) + { + if (message.PayloadCase != AlarmFeedMessage.PayloadOneofCase.ProviderStatus) + { + continue; + } - try - { - await _alarmsHub.StartAsync(_cts.Token).ConfigureAwait(false); - } - catch - { - // The badge is best-effort; it stays at the healthy default until - // the hub reconnects and delivers a fresh provider-status message. + _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); + await InvokeAsync(StateHasChanged).ConfigureAwait(false); + } + } + catch (OperationCanceledException) + { + return; + } + 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. + } + + try + { + await Task.Delay(TimeSpan.FromSeconds(1), _cts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + return; + } } } @@ -312,18 +333,6 @@ { await _cts.CancelAsync(); - if (_alarmsHub is not null) - { - try - { - await _alarmsHub.DisposeAsync(); - } - catch - { - // Disposal-time errors are best-effort. - } - } - if (_pollTask is not null) { try @@ -335,6 +344,17 @@ } } + if (_providerStatusTask is not null) + { + try + { + await _providerStatusTask; + } + catch (OperationCanceledException) + { + } + } + _cts.Dispose(); GC.SuppressFinalize(this); } From e23f816bfbb5ecdcab4fac4718bcf60b47ef4e27 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:15:52 -0400 Subject: [PATCH 07/23] =?UTF-8?q?feat(dashboard):=20in-process=20session?= =?UTF-8?q?=20event=20subscription=20feeds=20the=20viewer=20registry=20?= =?UTF-8?q?=E2=80=94=20SessionDetailsPage=20drops=20its=20/hubs/events=20h?= =?UTF-8?q?op?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Components/Pages/SessionDetailsPage.razor | 126 ++++++---- .../Hubs/DashboardEventBroadcaster.cs | 232 +++++++++++++++++- .../Hubs/IDashboardEventSubscription.cs | 26 ++ .../Hubs/IDashboardSessionEventSubscriber.cs | 34 +++ .../DashboardEventBroadcasterTests.cs | 141 ++++++++++- 5 files changed, 501 insertions(+), 58 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardEventSubscription.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardSessionEventSubscriber.cs diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor index d05ef32..af50f53 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor @@ -1,11 +1,11 @@ @page "/sessions/{SessionId}" @inherits DashboardPageBase @implements IAsyncDisposable -@using Microsoft.AspNetCore.SignalR.Client @using ZB.MOM.WW.MxGateway.Contracts.Proto @using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs @inject AuthenticationStateProvider AuthenticationStateProvider @inject IDashboardSessionAdminService SessionAdminService +@inject IDashboardEventBroadcaster EventBroadcaster Dashboard Session @@ -157,7 +157,8 @@ else private DashboardSessionSummary? CurrentSession => Snapshot?.Sessions.FirstOrDefault(session => string.Equals(session.SessionId, SessionId, StringComparison.Ordinal)); - private HubConnection? _eventsHub; + private IDashboardEventSubscription? _eventSubscription; + private CancellationTokenSource? _eventPumpCancellation; private bool _eventsConnected; private string? _subscribedSessionId; private readonly LinkedList _recentEvents = new(); @@ -179,13 +180,17 @@ else CanManage = SessionAdminService.CanManage(authenticationState.User); } - protected override async Task OnParametersSetAsync() + protected override Task OnParametersSetAsync() { + // Attach/detach are synchronous now that the feed is in-process; the override + // stays on the async lifecycle member so the base class's contract is untouched. if (!string.Equals(_subscribedSessionId, SessionId, StringComparison.Ordinal)) { - await DetachEventsHubAsync().ConfigureAwait(false); - await AttachEventsHubAsync().ConfigureAwait(false); + DetachEvents(); + AttachEvents(); } + + return Task.CompletedTask; } private PendingConfirm? PendingAction { get; set; } @@ -261,68 +266,91 @@ else string ConfirmButtonClass, Func> Action); - private async Task AttachEventsHubAsync() + // The dashboard runs in the same process as the broadcaster, so this page reads + // the session's mirrored events straight from it. It used to open a loopback + // SignalR connection to /hubs/events — mint a hub token, negotiate, hold a + // WebSocket, serialize every event — to reach data already sitting in memory. + // The subscription still registers with EventsHubViewerRegistry, so the + // broadcaster's "nobody is watching" gate keeps working for both audiences. + // ACL posture is unchanged from the hub path: any dashboard Viewer may watch + // any session (SEC-25 tracks the per-session ACL for both seams). + private void AttachEvents() { - if (string.IsNullOrWhiteSpace(SessionId)) + if (string.IsNullOrWhiteSpace(SessionId) || EventBroadcaster is not IDashboardSessionEventSubscriber subscriber) { return; } - _eventsHub = HubFactory.Create("/hubs/events"); - _eventsHub.On(EventsHub.EventMessage, async mxEvent => - { - _recentEvents.AddFirst(mxEvent); - while (_recentEvents.Count > MaxRecentEvents) - { - _recentEvents.RemoveLast(); - } + _eventSubscription = subscriber.Subscribe(SessionId); + _eventPumpCancellation = new CancellationTokenSource(); + _eventsConnected = true; + _subscribedSessionId = SessionId; - await InvokeAsync(StateHasChanged).ConfigureAwait(false); - }); - - _eventsHub.Closed += _ => - { - _eventsConnected = false; - return InvokeAsync(StateHasChanged); - }; - _eventsHub.Reconnected += _ => - { - _eventsConnected = true; - return InvokeAsync(StateHasChanged); - }; + _ = PumpEventsAsync(_eventSubscription, _eventPumpCancellation.Token); + } + private async Task PumpEventsAsync(IDashboardEventSubscription subscription, CancellationToken cancellationToken) + { try { - await _eventsHub.StartAsync().ConfigureAwait(false); - await _eventsHub.SendAsync("SubscribeSession", SessionId).ConfigureAwait(false); - _eventsConnected = true; - _subscribedSessionId = SessionId; + while (await subscription.Reader.WaitToReadAsync(cancellationToken).ConfigureAwait(false)) + { + // Drain what is queued and render once: a burst costs one render pass, + // not one per event. Reading past the display cap would be wasted work, + // and anything left queued is picked up on the next pass. + List batch = new(); + while (batch.Count < MaxRecentEvents && subscription.Reader.TryRead(out MxEvent? mxEvent)) + { + batch.Add(mxEvent); + } + + if (batch.Count == 0) + { + continue; + } + + await InvokeAsync(() => + { + foreach (MxEvent mxEvent in batch) + { + _recentEvents.AddFirst(mxEvent); + } + + while (_recentEvents.Count > MaxRecentEvents) + { + _recentEvents.RemoveLast(); + } + + StateHasChanged(); + }).ConfigureAwait(false); + } } - catch + catch (OperationCanceledException) { - _eventsConnected = false; + // The page navigated to another session or was disposed. + } + catch (ObjectDisposedException) + { + // The renderer went away while a batch was being dispatched. } } - private async Task DetachEventsHubAsync() + private void DetachEvents() { - HubConnection? hub = _eventsHub; - _eventsHub = null; + IDashboardEventSubscription? subscription = _eventSubscription; + CancellationTokenSource? cancellation = _eventPumpCancellation; + _eventSubscription = null; + _eventPumpCancellation = null; _eventsConnected = false; _subscribedSessionId = null; _recentEvents.Clear(); - if (hub is not null) - { - try - { - await hub.DisposeAsync().ConfigureAwait(false); - } - catch - { - // Disposal-time errors are best-effort. - } - } + // Cancel first so the pump stops touching the renderer, then dispose the + // subscription — that is what releases the viewer registration and lets the + // broadcaster go back to skipping mirror work for this session. + cancellation?.Cancel(); + cancellation?.Dispose(); + subscription?.Dispose(); } private static string EventStatusLabel(MxEvent evt) @@ -334,7 +362,7 @@ else public new async ValueTask DisposeAsync() { - await DetachEventsHubAsync().ConfigureAwait(false); + DetachEvents(); await base.DisposeAsync().ConfigureAwait(false); } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs index 33597d6..cf94e1d 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs @@ -1,3 +1,4 @@ +using System.Threading.Channels; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Options; using ZB.MOM.WW.MxGateway.Contracts.Proto; @@ -6,11 +7,13 @@ using ZB.MOM.WW.MxGateway.Server.Configuration; namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; /// -/// Broadcasts MxEvents to clients subscribed to the -/// session's group. Fire-and-forget: we hand the send to the hub context -/// and return immediately so the source gRPC stream is never blocked. -/// Errors are logged once and dropped — keeping the SignalR mirror best-effort -/// preserves the gRPC contract that exists today. +/// Broadcasts MxEvents to the two dashboard audiences for a session: remote +/// clients subscribed to the session's group, and +/// in-process subscribers opened through +/// . Fire-and-forget: we +/// hand the send to the hub context and return immediately so the source gRPC +/// stream is never blocked. Errors are logged once and dropped — keeping the +/// SignalR mirror best-effort preserves the gRPC contract that exists today. /// /// /// When MxGateway:Dashboard:ShowTagValues is false (the default), tag @@ -23,7 +26,10 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; /// /// Hub context used to send to the session's group. /// -/// Live-subscriber registry consulted before any per-event work is done. +/// Live-subscriber registry consulted before any per-event work is done. Both +/// audiences register here — hub connections by their SignalR connection id, +/// in-process subscriptions by a synthetic one — so the gate stays a single +/// source of truth. /// /// Gateway options supplying Dashboard:ShowTagValues. /// Logger for best-effort mirror failures. @@ -31,10 +37,36 @@ public sealed class DashboardEventBroadcaster( IHubContext hubContext, EventsHubViewerRegistry viewerRegistry, IOptions options, - ILogger logger) : IDashboardEventBroadcaster + ILogger logger) : IDashboardEventBroadcaster, IDashboardSessionEventSubscriber { + /// + /// Queue depth per in-process subscriber. The consumer is a Blazor page + /// rendering the newest handful of events, so a burst it cannot keep up with + /// is dropped oldest-first rather than allowed to grow — same best-effort + /// contract the SignalR mirror already has. + /// + private const int InProcessQueueCapacity = 256; + private readonly bool _showTagValues = options.Value.Dashboard.ShowTagValues; + private readonly object _syncRoot = new(); + + /// + /// In-process subscribers per session. Values are treated as immutable once + /// stored: a subscribe or dispose swaps in a new array under + /// , so can grab the reference + /// and write to it after releasing the lock. + /// + private readonly Dictionary _inProcessSubscribers = + new(StringComparer.Ordinal); + + /// + /// Total live in-process subscribers, read without the lock so the common + /// case — nobody has a session-details page open — never contends on it. + /// Written only under . + /// + private int _inProcessSubscriberCount; + /// public void Publish(string sessionId, MxEvent mxEvent) { @@ -55,6 +87,10 @@ public sealed class DashboardEventBroadcaster( MxEvent outbound = _showTagValues ? mxEvent : RedactValues(mxEvent); + // In-process delivery first: it is synchronous, cannot throw, and must not be + // skipped by the early return the hub send's guard clause takes. + DeliverInProcess(sessionId, outbound); + // Wrap the Task acquisition in a try/catch so a hypothetical synchronous throw // from SendAsync (e.g. an implementation that throws before returning the Task) // cannot escape Publish. The interface contract is never-throw; fire-and-forget. @@ -85,6 +121,117 @@ public sealed class DashboardEventBroadcaster( } } + /// + public IDashboardEventSubscription Subscribe(string sessionId) + { + ArgumentException.ThrowIfNullOrWhiteSpace(sessionId); + + // A synthetic connection id keeps the registry's per-connection bookkeeping + // usable for a subscriber that has no SignalR connection behind it. The + // "inproc-" prefix cannot collide with a SignalR connection id and makes the + // origin obvious in a debugger. + string connectionId = "inproc-" + Guid.NewGuid().ToString("N"); + InProcessSubscription subscription = new(this, sessionId, connectionId, InProcessQueueCapacity); + + // Register before the subscriber becomes a delivery target, exactly as + // EventsHub.SubscribeSession registers before joining the group: the reverse + // order would leave a window in which this subscriber is a delivery target but + // Publish's gate still reports the session unwatched, silently dropping events + // it should receive. The cost of this order is at worst a redaction clone that + // reaches nobody for the width of the window. + viewerRegistry.AddViewer(connectionId, sessionId); + + lock (_syncRoot) + { + _inProcessSubscribers[sessionId] = + _inProcessSubscribers.TryGetValue(sessionId, out InProcessSubscription[]? existing) + ? [.. existing, subscription] + : [subscription]; + + Volatile.Write(ref _inProcessSubscriberCount, _inProcessSubscriberCount + 1); + } + + return subscription; + } + + /// + /// Hands the already-redacted event to every in-process subscriber of the + /// session. Writes are non-blocking and lossy by construction, so this never + /// stalls the caller's event pipeline. + /// + /// Session the event belongs to. + /// The event as the dashboard should see it. + private void DeliverInProcess(string sessionId, MxEvent outbound) + { + // The gate above admits hub-only viewers too, so check for in-process + // subscribers before touching the lock at all. + if (Volatile.Read(ref _inProcessSubscriberCount) == 0) + { + return; + } + + InProcessSubscription[] subscribers; + lock (_syncRoot) + { + if (!_inProcessSubscribers.TryGetValue(sessionId, out InProcessSubscription[]? found)) + { + return; + } + + subscribers = found; + } + + // The array is never mutated in place, so the writes happen outside the lock. + foreach (InProcessSubscription subscriber in subscribers) + { + subscriber.TryWrite(outbound); + } + } + + /// + /// Removes a disposed subscription from the delivery map and releases its + /// viewer registration. Called at most once per subscription. + /// + /// The subscription being disposed. + private void Unsubscribe(InProcessSubscription subscription) + { + // Drop the delivery target first and deregister after, mirroring + // EventsHub.UnsubscribeSession: the mirror stays enabled for the brief overlap + // rather than dropping events still owed to the session's other subscribers. + lock (_syncRoot) + { + if (_inProcessSubscribers.TryGetValue(subscription.SessionId, out InProcessSubscription[]? existing)) + { + InProcessSubscription[] remaining = + [.. existing.Where(candidate => !ReferenceEquals(candidate, subscription))]; + + // Equal lengths mean it was never in this bucket, so the counter it + // would decrement is not its own to release. + if (remaining.Length != existing.Length) + { + if (remaining.Length == 0) + { + // Drop the key so the map does not grow one entry per session ever viewed. + _inProcessSubscribers.Remove(subscription.SessionId); + } + else + { + _inProcessSubscribers[subscription.SessionId] = remaining; + } + + Volatile.Write(ref _inProcessSubscriberCount, _inProcessSubscriberCount - 1); + } + } + } + + viewerRegistry.RemoveViewer(subscription.ConnectionId, subscription.SessionId); + + // The synthetic connection id is used once and never reconnects, so nothing + // else will ever call ReleaseConnection for it; without this the registry + // would retain an empty per-connection entry per subscription ever opened. + viewerRegistry.ReleaseConnection(subscription.ConnectionId); + } + /// /// Produces a deep clone of with every tag-value /// field cleared, leaving tag reference, quality, status, and timestamps @@ -107,4 +254,75 @@ public sealed class DashboardEventBroadcaster( return redacted; } + + /// + /// One in-process subscriber's feed: a bounded, drop-oldest channel plus the + /// registry bookkeeping that keeps 's viewer gate honest + /// while the feed is live. + /// + private sealed class InProcessSubscription : IDashboardEventSubscription + { + private readonly DashboardEventBroadcaster _owner; + + private readonly Channel _channel; + + private int _disposed; + + /// Initializes a new instance of the class. + /// Broadcaster to deregister from on disposal. + /// Session whose events this subscription carries. + /// Synthetic connection id registered with the viewer registry. + /// Queue depth before the oldest queued event is dropped. + internal InProcessSubscription( + DashboardEventBroadcaster owner, + string sessionId, + string connectionId, + int capacity) + { + _owner = owner; + SessionId = sessionId; + ConnectionId = connectionId; + _channel = Channel.CreateBounded(new BoundedChannelOptions(capacity) + { + // DropOldest, not Wait: a write must never block the gRPC event + // pipeline that calls Publish, and the newest events are the ones a + // live view wants. + FullMode = BoundedChannelFullMode.DropOldest, + SingleReader = true, + SingleWriter = false, + }); + } + + /// + public ChannelReader Reader => _channel.Reader; + + /// Gets the session this subscription is watching. + internal string SessionId { get; } + + /// Gets the synthetic connection id held in the viewer registry. + internal string ConnectionId { get; } + + /// + /// Queues an event for the subscriber, dropping the oldest queued event when + /// the reader has fallen behind. Never blocks and never throws. + /// + /// The event to queue. + internal void TryWrite(MxEvent mxEvent) => _channel.Writer.TryWrite(mxEvent); + + /// + /// Deregisters the subscription and completes its channel so a reader's + /// loop ends. Idempotent — a second call does nothing, so it can never + /// release a viewer count that a sibling subscription owns. + /// + public void Dispose() + { + if (Interlocked.Exchange(ref _disposed, 1) != 0) + { + return; + } + + _owner.Unsubscribe(this); + _channel.Writer.TryComplete(); + } + } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardEventSubscription.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardEventSubscription.cs new file mode 100644 index 0000000..9b6685b --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardEventSubscription.cs @@ -0,0 +1,26 @@ +using System.Threading.Channels; +using ZB.MOM.WW.MxGateway.Contracts.Proto; + +namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; + +/// +/// A live in-process feed of one session's dashboard-mirrored MxEvents, handed +/// out by . Server-side +/// Blazor components read it directly instead of looping back through +/// over a loopback SignalR connection. +/// +/// +/// Events are delivered exactly as a hub client would see them — the same +/// redacted clone the group send carries, so MxGateway:Dashboard:ShowTagValues +/// governs both paths identically. The feed is a bounded, lossy queue: a +/// consumer that falls behind loses the oldest queued events, matching the +/// best-effort contract the SignalR mirror already has. Disposing the +/// subscription deregisters it, which is what lets the broadcaster go back to +/// skipping all mirror work for a session nobody is watching — so callers must +/// dispose. Dispose is idempotent. +/// +public interface IDashboardEventSubscription : IDisposable +{ + /// Gets the reader delivering this session's mirrored events. + ChannelReader Reader { get; } +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardSessionEventSubscriber.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardSessionEventSubscriber.cs new file mode 100644 index 0000000..b60ea9e --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/IDashboardSessionEventSubscriber.cs @@ -0,0 +1,34 @@ +namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; + +/// +/// In-process subscription seam on the dashboard event mirror. Implemented by +/// alongside +/// . +/// +/// +/// The interactive-server dashboard runs in the same process as the broadcaster, +/// so a session-details page has no reason to open a loopback SignalR connection +/// back to — mint a hub token, negotiate, hold a +/// WebSocket, and serialize every event — just to read events the broadcaster +/// already holds. It subscribes here instead. The registry gate stays honest +/// either way: an in-process subscription registers a synthetic connection id +/// with exactly as the hub registers a real +/// one, so keeps skipping the +/// redaction clone for sessions nobody is watching. +/// +/// It is a separate interface rather than a member of +/// because publishing and consuming are +/// different roles: the session pipeline only ever publishes, and its test +/// doubles should not have to implement a subscription feed. +/// +/// +public interface IDashboardSessionEventSubscriber +{ + /// Opens an in-process feed of the session's mirrored events. + /// Session id whose events the caller wants. + /// + /// The subscription. Dispose it to stop the feed and release the viewer + /// registration that keeps the mirror enabled for this session. + /// + IDashboardEventSubscription Subscribe(string sessionId); +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardEventBroadcasterTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardEventBroadcasterTests.cs index 701573c..55aa942 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardEventBroadcasterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardEventBroadcasterTests.cs @@ -121,6 +121,140 @@ public sealed class DashboardEventBroadcasterTests Assert.NotNull(sent.OnAlarmTransition.LimitValue); } + /// + /// An in-process subscriber gets the same redacted clone the hub group gets, + /// and the shared source event is still left untouched. + /// + [Fact] + public void Subscribe_WhenShowTagValuesFalse_DeliversRedactedCloneWithoutMutatingSource() + { + CapturingHubContext hubContext = new(); + EventsHubViewerRegistry viewers = new(); + DashboardEventBroadcaster broadcaster = Create(hubContext, showTagValues: false, viewers); + using IDashboardEventSubscription subscription = broadcaster.Subscribe("session-1"); + MxEvent source = BuildEventWithValue(); + + broadcaster.Publish("session-1", source); + + MxEvent received = ReadOne(subscription); + Assert.Null(received.Value); + Assert.Null(received.OnAlarmTransition.CurrentValue); + Assert.Null(received.OnAlarmTransition.LimitValue); + Assert.Equal("Tank01.Level.HiHi", received.OnAlarmTransition.AlarmFullReference); + + // One clone feeds both audiences — the hub group and the in-process feed. + Assert.Same(hubContext.LastArgument, received); + + // The source is shared with the gRPC stream and the replay ring. + Assert.NotSame(source, received); + Assert.NotNull(source.Value); + Assert.Equal(42.5, source.Value.DoubleValue); + Assert.NotNull(source.OnAlarmTransition.CurrentValue); + Assert.NotNull(source.OnAlarmTransition.LimitValue); + } + + /// An in-process subscription opens the viewer gate the same way a hub client does. + [Fact] + public void Subscribe_OpensTheViewerGate() + { + CapturingHubContext hubContext = new(); + EventsHubViewerRegistry viewers = new(); + DashboardEventBroadcaster broadcaster = Create(hubContext, showTagValues: false, viewers); + + broadcaster.Publish("session-1", BuildEventWithValue()); + Assert.Equal(0, hubContext.SendCount); + Assert.Null(hubContext.LastArgument); + + using IDashboardEventSubscription subscription = broadcaster.Subscribe("session-1"); + + Assert.True(viewers.HasViewers("session-1")); + + broadcaster.Publish("session-1", BuildEventWithValue()); + + Assert.Equal(1, hubContext.SendCount); + Assert.NotNull(hubContext.LastArgument); + } + + /// Disposing the last in-process subscription restores the no-viewers short-circuit. + [Fact] + public void Dispose_OfLastInProcessSubscription_RestoresTheShortCircuit() + { + CapturingHubContext hubContext = new(); + EventsHubViewerRegistry viewers = new(); + DashboardEventBroadcaster broadcaster = Create(hubContext, showTagValues: false, viewers); + IDashboardEventSubscription subscription = broadcaster.Subscribe("session-1"); + + broadcaster.Publish("session-1", BuildEventWithValue()); + Assert.Equal(1, hubContext.SendCount); + Assert.Equal("session-1", ReadOne(subscription).SessionId); + + subscription.Dispose(); + + Assert.False(viewers.HasViewers("session-1")); + + broadcaster.Publish("session-1", BuildEventWithValue()); + + Assert.Equal(1, hubContext.SendCount); + Assert.False(subscription.Reader.TryRead(out _)); + } + + /// Hub viewers and in-process subscribers are audiences of their own session only. + [Fact] + public void Publish_DeliversOnlyToTheSubscribedSessionsAudience() + { + CapturingHubContext hubContext = new(); + EventsHubViewerRegistry viewers = new(); + viewers.AddViewer("conn-1", "session-1"); + DashboardEventBroadcaster broadcaster = Create(hubContext, showTagValues: false, viewers); + using IDashboardEventSubscription subscription = broadcaster.Subscribe("session-2"); + + // The hub viewer's session must not spill into the in-process feed. + broadcaster.Publish("session-1", BuildEventWithValue()); + + Assert.Equal(1, hubContext.SendCount); + Assert.False(subscription.Reader.TryRead(out _)); + + broadcaster.Publish("session-2", BuildEventWithValue("session-2")); + + Assert.Equal("session-2", ReadOne(subscription).SessionId); + + // A session with no audience at all still short-circuits. + broadcaster.Publish("session-3", BuildEventWithValue("session-3")); + + Assert.Equal(2, hubContext.SendCount); + } + + /// Disposing twice is a no-op and cannot release a sibling subscription's registration. + [Fact] + public void Dispose_CalledTwice_IsSafeAndLeavesSiblingSubscriptionsAlone() + { + CapturingHubContext hubContext = new(); + EventsHubViewerRegistry viewers = new(); + DashboardEventBroadcaster broadcaster = Create(hubContext, showTagValues: false, viewers); + IDashboardEventSubscription first = broadcaster.Subscribe("session-1"); + using IDashboardEventSubscription second = broadcaster.Subscribe("session-1"); + + first.Dispose(); + first.Dispose(); + + Assert.True(viewers.HasViewers("session-1")); + + broadcaster.Publish("session-1", BuildEventWithValue()); + + Assert.Equal(1, hubContext.SendCount); + Assert.False(first.Reader.TryRead(out _)); + Assert.Equal("session-1", ReadOne(second).SessionId); + } + + /// Reads exactly one event from a subscription, failing the test if none is queued. + /// The subscription to read from. + /// The event that was read. + private static MxEvent ReadOne(IDashboardEventSubscription subscription) + { + Assert.True(subscription.Reader.TryRead(out MxEvent? received)); + return Assert.IsType(received); + } + private static DashboardEventBroadcaster Create( CapturingHubContext hubContext, bool showTagValues, @@ -147,12 +281,15 @@ public sealed class DashboardEventBroadcasterTests return viewers; } - private static MxEvent BuildEventWithValue() + /// Builds a value-bearing alarm-transition event for the given session. + /// Session id stamped on the event. + /// The event. + private static MxEvent BuildEventWithValue(string sessionId = "session-1") { return new MxEvent { Family = MxEventFamily.OnAlarmTransition, - SessionId = "session-1", + SessionId = sessionId, ServerHandle = 7, ItemHandle = 11, Quality = 192, From e245237c2bb0a58c8d4df9ceb610952e54f89a31 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:16:29 -0400 Subject: [PATCH 08/23] feat(dashboard): in-process snapshot feed replaces the pages' loopback /hubs/snapshot hop --- .../Dashboard/Components/DashboardPageBase.cs | 106 +++--- .../DashboardServiceCollectionExtensions.cs | 1 + .../Dashboard/DashboardSnapshotFeed.cs | 216 ++++++++++++ .../Dashboard/IDashboardSnapshotFeed.cs | 26 ++ .../Dashboard/DashboardSnapshotFeedTests.cs | 333 ++++++++++++++++++ 5 files changed, 637 insertions(+), 45 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSnapshotFeed.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs index ded13c8..670b8fe 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs @@ -1,80 +1,96 @@ using Microsoft.AspNetCore.Components; -using Microsoft.AspNetCore.SignalR.Client; -using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Components; /// -/// Base class for Blazor dashboard pages that watch gateway metrics -/// snapshots. The previous implementation polled -/// directly; we -/// now subscribe to so updates are -/// pushed and disconnects survive reconnects via SignalR's -/// auto-reconnect. +/// Base class for Blazor dashboard pages that watch gateway metrics snapshots. +/// Pages subscribe to the in-process , which +/// multicasts a single +/// enumeration to every circuit. An earlier implementation had each page open its +/// own SignalR connection to /hubs/snapshot — a loopback WebSocket back into +/// this same process, per page. The snapshot hub and its publisher remain for +/// external (non-circuit) clients; server-rendered pages no longer use them. /// public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable { - private HubConnection? _hub; + /// + /// Upper bound on waiting for the watch loop while disposing. The loop marshals + /// renders through the renderer's dispatcher and disposal can run on that same + /// dispatcher, so the wait is bounded rather than unconditional. + /// + private static readonly TimeSpan WatchDrainTimeout = TimeSpan.FromSeconds(5); - /// Snapshot service used to seed the initial render before the hub connects. + private readonly CancellationTokenSource _watchCancellation = new(); + private Task? _watchTask; + + /// Snapshot service used to seed the initial render before the first feed update. [Inject] protected IDashboardSnapshotService SnapshotService { get; set; } = null!; - /// Factory that builds the SignalR connection (mints the hub bearer token). + /// Shared in-process snapshot feed this page renders from. [Inject] - protected DashboardHubConnectionFactory HubFactory { get; set; } = null!; + protected IDashboardSnapshotFeed SnapshotFeed { get; set; } = null!; /// /// The most recent gateway metric snapshot. Synchronously seeded from - /// for the very - /// first render, then refreshed by hub push. + /// for the very first + /// render, then refreshed from the feed. /// protected DashboardSnapshot? Snapshot { get; private set; } /// - protected override async Task OnInitializedAsync() + protected override Task OnInitializedAsync() { Snapshot = SnapshotService.GetSnapshot(); - await ConnectHubAsync().ConfigureAwait(false); + + // Deliberately not awaited: the watch loop runs for the lifetime of the page + // and is cancelled and drained by DisposeAsync. + _watchTask = WatchSnapshotsAsync(_watchCancellation.Token); + return Task.CompletedTask; } - /// Disposes the SignalR hub connection created for this page, tolerating disposal-time errors. + /// Cancels the snapshot subscription created for this page, tolerating disposal-time errors. /// A task that represents the asynchronous operation. public async ValueTask DisposeAsync() { - if (_hub is not null) - { - try - { - await _hub.DisposeAsync().ConfigureAwait(false); - } - catch - { - // Disposal-time errors are best-effort. - } - } - - GC.SuppressFinalize(this); - } - - private async Task ConnectHubAsync() - { - _hub = HubFactory.Create("/hubs/snapshot"); - _hub.On(DashboardSnapshotHub.SnapshotMessage, async snapshot => - { - Snapshot = snapshot; - await InvokeAsync(StateHasChanged).ConfigureAwait(false); - }); - try { - await _hub.StartAsync().ConfigureAwait(false); + await _watchCancellation.CancelAsync().ConfigureAwait(false); + + if (_watchTask is not null) + { + await _watchTask.WaitAsync(WatchDrainTimeout).ConfigureAwait(false); + } } catch { - // Hub is best-effort; the initial GetSnapshot() seed remains - // valid and the snapshot service keeps populating its cache for - // the next reconnect cycle. + // Disposal-time errors (including a drain timeout) are best-effort. + } + + _watchCancellation.Dispose(); + GC.SuppressFinalize(this); + } + + private async Task WatchSnapshotsAsync(CancellationToken cancellationToken) + { + try + { + await foreach (DashboardSnapshot snapshot in SnapshotFeed + .WatchAsync(cancellationToken) + .ConfigureAwait(false)) + { + Snapshot = snapshot; + await InvokeAsync(StateHasChanged).ConfigureAwait(false); + } + } + catch (OperationCanceledException) + { + // The page is going away. + } + catch + { + // The feed is best-effort: the last rendered snapshot stays on screen and + // the snapshot service keeps serving GetSnapshot() for the next page load. } } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs index 42e014f..aece649 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs @@ -38,6 +38,7 @@ public static class DashboardServiceCollectionExtensions services.AddZbLdapAuth(configuration, "MxGateway:Ldap"); services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); services.AddSingleton, DashboardGroupRoleMapper>(); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs new file mode 100644 index 0000000..72a89cb --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs @@ -0,0 +1,216 @@ +using System.Runtime.CompilerServices; +using System.Threading.Channels; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace ZB.MOM.WW.MxGateway.Server.Dashboard; + +/// +/// Fans one enumeration out to +/// every dashboard circuit. The underlying watch is not multicast — each enumeration owns a +/// timer and builds its own snapshot per tick — so subscribing per page would multiply the +/// snapshot cost by the number of open pages. +/// +/// +/// The pump is idle-gated: it starts when the subscriber count goes 0 → 1 and is cancelled +/// and awaited when it goes 1 → 0, so an unwatched gateway runs no timer and builds no +/// snapshots. Successive pumps are chained through _pumpTask, so a rapid +/// unsubscribe/resubscribe restarts a fresh pump without ever running two enumerations at +/// once. +/// +public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed +{ + private readonly IDashboardSnapshotService _snapshotService; + private readonly ILogger _logger; + private readonly object _gate = new(); + private readonly List> _subscribers = []; + + /// + /// The most recent pump, completed while idle. A starting pump awaits its predecessor + /// before enumerating, which is what guarantees a single live enumeration. + /// + private Task _pumpTask = Task.CompletedTask; + + /// Cancellation for the live pump; null when no pump is running or one is being torn down. + private CancellationTokenSource? _pumpCancellation; + + /// Initializes a new instance of the class. + /// Snapshot source to multicast. + /// Optional logger for pump faults. + public DashboardSnapshotFeed( + IDashboardSnapshotService snapshotService, + ILogger? logger = null) + { + _snapshotService = snapshotService ?? throw new ArgumentNullException(nameof(snapshotService)); + _logger = logger ?? NullLogger.Instance; + } + + /// + public async IAsyncEnumerable WatchAsync( + [EnumeratorCancellation] CancellationToken cancellationToken) + { + // Capacity 1 + DropOldest: a viewer only ever wants the latest snapshot, so a + // circuit that renders slowly neither buffers without bound nor blocks the pump + // (TryWrite always succeeds) — it just skips the snapshots it was too slow for. + Channel channel = Channel.CreateBounded( + new BoundedChannelOptions(1) + { + FullMode = BoundedChannelFullMode.DropOldest, + SingleReader = true, + SingleWriter = false, + }); + + Subscribe(channel); + try + { + await foreach (DashboardSnapshot snapshot in channel.Reader + .ReadAllAsync(cancellationToken) + .ConfigureAwait(false)) + { + yield return snapshot; + } + } + finally + { + // Untokened on purpose: teardown must run to completion even when this + // subscriber is unwinding because its own token fired. + await UnsubscribeAsync(channel).ConfigureAwait(false); + } + } + + private void Subscribe(Channel channel) + { + lock (_gate) + { + _subscribers.Add(channel); + if (_subscribers.Count != 1) + { + return; + } + + CancellationTokenSource cancellation = new(); + Task previous = _pumpTask; + _pumpCancellation = cancellation; + + // Task.Run, not a direct call: an async iterator runs synchronously up to its + // first suspension, and the first pull of the underlying watch can read the API + // key table. That must not run on the subscribing circuit's thread, let alone + // while this lock is held. + _pumpTask = Task.Run(() => PumpAsync(previous, cancellation, cancellation.Token)); + } + } + + private async Task UnsubscribeAsync(Channel channel) + { + CancellationTokenSource? cancellation; + Task pump; + lock (_gate) + { + if (!_subscribers.Remove(channel) || _subscribers.Count != 0) + { + // Either the pump already dropped this channel (it completed or faulted + // and reset itself), or other viewers are still watching. + return; + } + + cancellation = _pumpCancellation; + _pumpCancellation = null; + pump = _pumpTask; + } + + try + { + cancellation?.Cancel(); + } + catch (ObjectDisposedException) + { + // The pump reset itself and disposed its own cancellation source first. + } + + try + { + await pump.ConfigureAwait(false); + } + catch (Exception) + { + // A pump fault has already been reported to the subscribers it had; the + // unsubscribing caller is only waiting for the enumeration to stop. + } + } + + private async Task PumpAsync(Task previous, CancellationTokenSource cancellation, CancellationToken cancellationToken) + { + try + { + // Never overlap with the enumeration this pump replaces. + await previous.ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); + + await foreach (DashboardSnapshot snapshot in _snapshotService + .WatchSnapshotsAsync(cancellationToken) + .ConfigureAwait(false)) + { + Broadcast(snapshot); + } + + // The source completed on its own; hand the completion to the subscribers + // and re-arm so the next one starts a fresh enumeration. + Reset(cancellation, error: null); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // Last subscriber left: the unsubscribing caller already detached the + // channels and cleared the pump state. + } + catch (Exception error) + { + _logger.LogWarning(error, "Dashboard snapshot feed stopped; the next subscriber restarts it."); + Reset(cancellation, error); + } + finally + { + cancellation.Dispose(); + } + } + + private void Broadcast(DashboardSnapshot snapshot) + { + lock (_gate) + { + foreach (Channel subscriber in _subscribers) + { + // Bounded/DropOldest: always accepted unless the channel is completed. + subscriber.Writer.TryWrite(snapshot); + } + } + } + + /// + /// Detaches every subscriber and clears the pump state so the next subscriber starts a + /// new enumeration. The detached subscribers observe (or a + /// clean end of stream) from their own WatchAsync. + /// + /// The calling pump's cancellation source, used as its ownership token. + /// Failure to surface, or null when the source completed cleanly. + private void Reset(CancellationTokenSource cancellation, Exception? error) + { + Channel[] detached; + lock (_gate) + { + if (!ReferenceEquals(_pumpCancellation, cancellation)) + { + // A newer pump (or an in-flight teardown) owns the state now: its + // subscribers must not be detached by this pump's exit. + return; + } + + detached = _subscribers.ToArray(); + _subscribers.Clear(); + _pumpCancellation = null; + } + + foreach (Channel subscriber in detached) + { + subscriber.Writer.TryComplete(error); + } + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSnapshotFeed.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSnapshotFeed.cs new file mode 100644 index 0000000..7e72875 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSnapshotFeed.cs @@ -0,0 +1,26 @@ +namespace ZB.MOM.WW.MxGateway.Server.Dashboard; + +/// +/// In-process multicast over . +/// One enumeration of the underlying watch is fanned out to every subscriber, so N +/// dashboard circuits cost one snapshot build per tick instead of N — and while nobody +/// subscribes, nothing runs at all. +/// +/// +/// There is no authentication or authorization gate here: the feed is reached only from +/// Blazor dashboard components, whose endpoints already require +/// , so every caller is a circuit +/// authorized as Viewer. Remote (non-circuit) consumers still go through +/// /hubs/snapshot, which applies the hub authorization policy itself. +/// +public interface IDashboardSnapshotFeed +{ + /// + /// Watches the shared snapshot stream. Each caller gets the snapshots produced while + /// it is subscribed; a caller that reads slowly sees only the newest snapshot rather + /// than a backlog, and never delays the other subscribers. + /// + /// Token that ends this caller's subscription. + /// An asynchronous stream of dashboard snapshots. + IAsyncEnumerable WatchAsync(CancellationToken cancellationToken); +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs new file mode 100644 index 0000000..b86bfd7 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs @@ -0,0 +1,333 @@ +using System.Runtime.CompilerServices; +using System.Threading.Channels; +using ZB.MOM.WW.MxGateway.Server.Dashboard; + +namespace ZB.MOM.WW.MxGateway.Tests.Gateway.Dashboard; + +/// +/// Covers the in-process snapshot fan-out that replaced the dashboard pages' +/// loopback /hubs/snapshot connections. The invariants under test are the +/// ones that make the feed cheaper than the hub hop: exactly one underlying +/// enumeration for any +/// number of viewers, nothing at all while nobody is watching, and a slow viewer +/// that can neither buffer without bound nor stall the others. +/// +public sealed class DashboardSnapshotFeedTests +{ + private static readonly TimeSpan TestTimeout = TimeSpan.FromSeconds(5); + + /// + /// With no page subscribed, the feed must not touch the snapshot service at + /// all — no timer, no snapshot build. This is the whole point of the idle + /// gate: an unattended gateway does no dashboard work. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WithNoSubscribers_NeverEnumeratesUnderlyingWatch() + { + FakeSnapshotService service = new(); + DashboardSnapshotFeed feed = new(service); + + // Obtaining the enumerable without enumerating it must not subscribe + // either: the pump starts on the first MoveNextAsync, not before. + _ = feed.WatchAsync(CancellationToken.None); + await Task.Delay(TimeSpan.FromMilliseconds(100)); + + Assert.Equal(0, service.EnumerationCount); + } + + /// + /// Two viewers share one underlying enumeration and both see the same + /// pushed snapshot. Before the feed, each page opened its own SignalR + /// connection and the publisher pulled its own snapshot stream. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WithTwoSubscribers_SharesASingleUnderlyingEnumeration() + { + FakeSnapshotService service = new(); + DashboardSnapshotFeed feed = new(service); + + using CancellationTokenSource firstCancellation = new(); + using CancellationTokenSource secondCancellation = new(); + IAsyncEnumerator first = + feed.WatchAsync(firstCancellation.Token).GetAsyncEnumerator(firstCancellation.Token); + Task firstMove = first.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 1); + + IAsyncEnumerator second = + feed.WatchAsync(secondCancellation.Token).GetAsyncEnumerator(secondCancellation.Token); + Task secondMove = second.MoveNextAsync().AsTask(); + + // Push until both have observed a snapshot: a subscriber only becomes + // visible to the pump once its MoveNextAsync has registered the channel, + // so a single push could race the second registration. + await PushUntilAsync(service, Task.WhenAll(firstMove, secondMove)); + + Assert.True(await firstMove.WaitAsync(TestTimeout)); + Assert.True(await secondMove.WaitAsync(TestTimeout)); + Assert.StartsWith("push-", first.Current.GatewayVersion, StringComparison.Ordinal); + Assert.StartsWith("push-", second.Current.GatewayVersion, StringComparison.Ordinal); + Assert.Equal(1, service.EnumerationCount); + + await firstCancellation.CancelAsync(); + await secondCancellation.CancelAsync(); + await DrainAsync(first, firstMove); + await DrainAsync(second, secondMove); + } + + /// + /// The last viewer leaving must cancel the underlying enumeration (idle + /// gate re-armed), and the next viewer must restart it — the rapid + /// unsubscribe/resubscribe path a page navigation exercises. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WhenLastSubscriberLeaves_CancelsPumpAndRestartsForTheNext() + { + FakeSnapshotService service = new(); + DashboardSnapshotFeed feed = new(service); + + using CancellationTokenSource firstCancellation = new(); + IAsyncEnumerator first = + feed.WatchAsync(firstCancellation.Token).GetAsyncEnumerator(firstCancellation.Token); + Task firstMove = first.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 1); + + await firstCancellation.CancelAsync(); + await DrainAsync(first, firstMove); + + await WaitUntilAsync(() => service.CompletedEnumerationCount >= 1); + Assert.True(service.LastEnumerationWasCancelled); + + using CancellationTokenSource secondCancellation = new(); + IAsyncEnumerator second = + feed.WatchAsync(secondCancellation.Token).GetAsyncEnumerator(secondCancellation.Token); + Task secondMove = second.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 2); + + Assert.Equal(2, service.EnumerationCount); + + await secondCancellation.CancelAsync(); + await DrainAsync(second, secondMove); + } + + /// + /// A viewer that is not reading must not stall the pump or accumulate + /// snapshots: its bounded channel drops the oldest, so its next read is the + /// newest snapshot the pump has broadcast, not a backlog head. The fast + /// reader's progress is what makes the assertion deterministic — once it has + /// seen the third snapshot the pump has provably broadcast all three. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WithASlowSubscriber_KeepsOnlyTheNewestSnapshot() + { + FakeSnapshotService service = new(); + DashboardSnapshotFeed feed = new(service); + + using CancellationTokenSource fastCancellation = new(); + using CancellationTokenSource slowCancellation = new(); + IAsyncEnumerator fast = + feed.WatchAsync(fastCancellation.Token).GetAsyncEnumerator(fastCancellation.Token); + Task fastMove = fast.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 1); + + // The slow subscriber registers but never advances until the very end. + IAsyncEnumerator slow = + feed.WatchAsync(slowCancellation.Token).GetAsyncEnumerator(slowCancellation.Token); + Task slowMove = slow.MoveNextAsync().AsTask(); + await PushUntilAsync(service, Task.WhenAll(fastMove, slowMove)); + Assert.True(await fastMove.WaitAsync(TestTimeout)); + Assert.True(await slowMove.WaitAsync(TestTimeout)); + + service.Push(CreateSnapshot("s1")); + service.Push(CreateSnapshot("s2")); + service.Push(CreateSnapshot("s3")); + + // Drain the fast reader until it sees s3; that proves the pump broadcast + // all three to every subscriber, so the slow channel now holds exactly s3. + string fastLatest = fast.Current.GatewayVersion; + while (fastLatest != "s3") + { + Assert.True(await fast.MoveNextAsync().AsTask().WaitAsync(TestTimeout)); + fastLatest = fast.Current.GatewayVersion; + } + + Assert.True(await slow.MoveNextAsync().AsTask().WaitAsync(TestTimeout)); + Assert.Equal("s3", slow.Current.GatewayVersion); + + await fastCancellation.CancelAsync(); + await slowCancellation.CancelAsync(); + await DrainAsync(fast, Task.FromResult(true)); + await DrainAsync(slow, Task.FromResult(true)); + } + + /// + /// A fault in the underlying watch is surfaced to the current viewers rather + /// than silently hanging them, and it resets the feed so the next viewer + /// starts a fresh pump instead of attaching to a dead one. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WhenUnderlyingWatchFaults_PropagatesAndRestartsForTheNextSubscriber() + { + FakeSnapshotService service = new(); + DashboardSnapshotFeed feed = new(service); + + using CancellationTokenSource firstCancellation = new(); + IAsyncEnumerator first = + feed.WatchAsync(firstCancellation.Token).GetAsyncEnumerator(firstCancellation.Token); + Task firstMove = first.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 1); + + service.Fault(new InvalidOperationException("simulated snapshot source failure")); + + InvalidOperationException failure = + await Assert.ThrowsAsync(() => firstMove.WaitAsync(TestTimeout)); + Assert.Equal("simulated snapshot source failure", failure.Message); + await first.DisposeAsync(); + + using CancellationTokenSource secondCancellation = new(); + IAsyncEnumerator second = + feed.WatchAsync(secondCancellation.Token).GetAsyncEnumerator(secondCancellation.Token); + Task secondMove = second.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 2); + + await PushUntilAsync(service, secondMove); + Assert.True(await secondMove.WaitAsync(TestTimeout)); + + await secondCancellation.CancelAsync(); + await DrainAsync(second, secondMove); + } + + /// Builds a snapshot whose version string identifies it in assertions. + /// Identity marker carried in GatewayVersion. + /// A snapshot carrying the supplied identity marker. + private static DashboardSnapshot CreateSnapshot(string version) + { + return new DashboardSnapshot( + GeneratedAt: DateTimeOffset.UnixEpoch, + GatewayStartedAt: DateTimeOffset.UnixEpoch, + GatewayUptime: TimeSpan.Zero, + GatewayStatus: "Healthy", + GatewayVersion: version, + Sessions: Array.Empty(), + Workers: Array.Empty(), + Metrics: Array.Empty(), + Faults: Array.Empty(), + ApiKeys: Array.Empty(), + Configuration: null!, + Galaxy: null!); + } + + /// + /// Pushes snapshots until the supplied task completes, so a test never + /// depends on a single push landing after a subscriber has registered. + /// + /// Fake snapshot source to push through. + /// Task whose completion stops the pushes. + /// A task that represents the asynchronous operation. + private static async Task PushUntilAsync(FakeSnapshotService service, Task until) + { + using CancellationTokenSource cancellation = new(TestTimeout); + int sequence = 0; + while (!until.IsCompleted) + { + service.Push(CreateSnapshot($"push-{sequence++}")); + await Task.Delay(TimeSpan.FromMilliseconds(5), cancellation.Token); + } + } + + /// + /// Observes the cancellation of a pending enumeration and disposes the + /// enumerator, mirroring how await foreach unwinds a cancelled watch. + /// + /// Enumerator to unwind. + /// The in-flight move, if any. + /// A task that represents the asynchronous operation. + private static async Task DrainAsync(IAsyncEnumerator enumerator, Task pending) + { + try + { + await pending.WaitAsync(TestTimeout); + } + catch (OperationCanceledException) + { + } + + try + { + await enumerator.DisposeAsync(); + } + catch (OperationCanceledException) + { + } + } + + private static async Task WaitUntilAsync(Func predicate) + { + using CancellationTokenSource cancellation = new(TestTimeout); + while (!predicate()) + { + await Task.Delay(TimeSpan.FromMilliseconds(5), cancellation.Token); + } + } + + /// + /// Snapshot source under the feed's control: counts enumerations, records how + /// each one ended, and lets the test drive snapshots (or a fault) into the + /// live enumeration. + /// + private sealed class FakeSnapshotService : IDashboardSnapshotService + { + private readonly Channel _pushes = Channel.CreateUnbounded(); + private int _enumerationCount; + private int _completedEnumerationCount; + private volatile bool _lastEnumerationWasCancelled; + + /// Gets the number of times the feed started enumerating this source. + public int EnumerationCount => Volatile.Read(ref _enumerationCount); + + /// Gets the number of enumerations that have finished (cancelled, faulted, or completed). + public int CompletedEnumerationCount => Volatile.Read(ref _completedEnumerationCount); + + /// Gets a value indicating whether the most recently finished enumeration ended cancelled. + public bool LastEnumerationWasCancelled => _lastEnumerationWasCancelled; + + /// Queues a snapshot for the live enumeration to yield. + /// Snapshot to yield. + public void Push(DashboardSnapshot snapshot) => _pushes.Writer.TryWrite(snapshot); + + /// Queues a failure for the live enumeration to throw. + /// Exception to throw from the enumeration. + public void Fault(Exception error) => _pushes.Writer.TryWrite(error); + + /// + public DashboardSnapshot GetSnapshot() => CreateSnapshot("current"); + + /// + public async IAsyncEnumerable WatchSnapshotsAsync( + [EnumeratorCancellation] CancellationToken cancellationToken) + { + Interlocked.Increment(ref _enumerationCount); + try + { + await foreach (object item in _pushes.Reader.ReadAllAsync(cancellationToken).ConfigureAwait(false)) + { + if (item is Exception error) + { + throw error; + } + + yield return (DashboardSnapshot)item; + } + } + finally + { + _lastEnumerationWasCancelled = cancellationToken.IsCancellationRequested; + Interlocked.Increment(ref _completedEnumerationCount); + } + } + } +} From 10406a3541c3b4da9ad9ab14c6fbe427c4451be3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:17:07 -0400 Subject: [PATCH 09/23] fix(dashboard): mutate provider-status state inside the renderer dispatch --- .../Dashboard/Components/Pages/AlarmsPage.razor | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 2aecd24..0110b90 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 @@ -207,8 +207,11 @@ continue; } - _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); - await InvokeAsync(StateHasChanged).ConfigureAwait(false); + await InvokeAsync(() => + { + _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); + StateHasChanged(); + }).ConfigureAwait(false); } } catch (OperationCanceledException) From 38dd7678f2cd2c2059b90eba370ec5167b6ccf23 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:26:42 -0400 Subject: [PATCH 10/23] fix(dashboard): guard stale-session batches inside the renderer dispatch; register IDashboardSessionEventSubscriber --- .../Components/Pages/SessionDetailsPage.razor | 81 ++++++++++++++----- .../DashboardServiceCollectionExtensions.cs | 12 ++- .../DashboardHubsRegistrationTests.cs | 19 +++++ 3 files changed, 91 insertions(+), 21 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor index af50f53..aebe8f4 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor @@ -5,7 +5,7 @@ @using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs @inject AuthenticationStateProvider AuthenticationStateProvider @inject IDashboardSessionAdminService SessionAdminService -@inject IDashboardEventBroadcaster EventBroadcaster +@inject IDashboardSessionEventSubscriber EventSubscriber Dashboard Session @@ -157,8 +157,18 @@ else private DashboardSessionSummary? CurrentSession => Snapshot?.Sessions.FirstOrDefault(session => string.Equals(session.SessionId, SessionId, StringComparison.Ordinal)); + // Upper bound on waiting for the event pump while detaching, mirroring + // DashboardPageBase's snapshot-watch drain: the pump marshals renders through the + // renderer's dispatcher and a detach can run on that same dispatcher, so the wait + // is bounded rather than unconditional. + private static readonly TimeSpan EventPumpDrainTimeout = TimeSpan.FromSeconds(5); + + // Written only on the renderer's dispatcher (the lifecycle methods below), and read + // on it from inside the pump's dispatched callback — that pairing is what makes the + // stale-batch guard in PumpEventsAsync reliable. private IDashboardEventSubscription? _eventSubscription; private CancellationTokenSource? _eventPumpCancellation; + private Task? _eventPumpTask; private bool _eventsConnected; private string? _subscribedSessionId; private readonly LinkedList _recentEvents = new(); @@ -180,17 +190,16 @@ else CanManage = SessionAdminService.CanManage(authenticationState.User); } - protected override Task OnParametersSetAsync() + protected override async Task OnParametersSetAsync() { - // Attach/detach are synchronous now that the feed is in-process; the override - // stays on the async lifecycle member so the base class's contract is untouched. if (!string.Equals(_subscribedSessionId, SessionId, StringComparison.Ordinal)) { - DetachEvents(); + // Deliberately no ConfigureAwait(false): the resumption must stay on the + // renderer's dispatcher so the new subscription is published to + // _eventSubscription from the same thread the pump's guard reads it on. + await DetachEventsAsync(); AttachEvents(); } - - return Task.CompletedTask; } private PendingConfirm? PendingAction { get; set; } @@ -266,27 +275,31 @@ else string ConfirmButtonClass, Func> Action); - // The dashboard runs in the same process as the broadcaster, so this page reads + // The dashboard runs in the same process as the event mirror, so this page reads // the session's mirrored events straight from it. It used to open a loopback // SignalR connection to /hubs/events — mint a hub token, negotiate, hold a // WebSocket, serialize every event — to reach data already sitting in memory. - // The subscription still registers with EventsHubViewerRegistry, so the - // broadcaster's "nobody is watching" gate keeps working for both audiences. + // IDashboardSessionEventSubscriber resolves to the same singleton that serves + // IDashboardEventBroadcaster, and the subscription registers with + // EventsHubViewerRegistry, so the "nobody is watching" gate keeps working for + // both audiences. // ACL posture is unchanged from the hub path: any dashboard Viewer may watch // any session (SEC-25 tracks the per-session ACL for both seams). private void AttachEvents() { - if (string.IsNullOrWhiteSpace(SessionId) || EventBroadcaster is not IDashboardSessionEventSubscriber subscriber) + if (string.IsNullOrWhiteSpace(SessionId)) { return; } - _eventSubscription = subscriber.Subscribe(SessionId); + _eventSubscription = EventSubscriber.Subscribe(SessionId); _eventPumpCancellation = new CancellationTokenSource(); _eventsConnected = true; _subscribedSessionId = SessionId; - _ = PumpEventsAsync(_eventSubscription, _eventPumpCancellation.Token); + // Deliberately not awaited: the pump runs for as long as the page watches this + // session and is cancelled and drained by DetachEventsAsync. + _eventPumpTask = PumpEventsAsync(_eventSubscription, _eventPumpCancellation.Token); } private async Task PumpEventsAsync(IDashboardEventSubscription subscription, CancellationToken cancellationToken) @@ -311,6 +324,17 @@ else await InvokeAsync(() => { + // The batch was read before this callback was dispatched, and a + // session switch can land in between. Rendering it then would show + // the previous session's events under the new session's heading, so + // a batch whose subscription is no longer the live one is dropped. + // Safe as an unsynchronized read: _eventSubscription is written on + // this same dispatcher. + if (!ReferenceEquals(_eventSubscription, subscription)) + { + return; + } + foreach (MxEvent mxEvent in batch) { _recentEvents.AddFirst(mxEvent); @@ -331,26 +355,43 @@ else } catch (ObjectDisposedException) { - // The renderer went away while a batch was being dispatched. + // Either the renderer went away mid-dispatch, or the drain below timed out + // and disposed the cancellation source this loop is still reading. } } - private void DetachEvents() + private async Task DetachEventsAsync() { IDashboardEventSubscription? subscription = _eventSubscription; CancellationTokenSource? cancellation = _eventPumpCancellation; + Task? pump = _eventPumpTask; _eventSubscription = null; _eventPumpCancellation = null; + _eventPumpTask = null; _eventsConnected = false; _subscribedSessionId = null; _recentEvents.Clear(); - // Cancel first so the pump stops touching the renderer, then dispose the - // subscription — that is what releases the viewer registration and lets the - // broadcaster go back to skipping mirror work for this session. + // Cancel and drop the subscription before draining. Disposing it releases the + // viewer registration — the whole point of the gate — and completes the channel, + // so the pump has an exit even if cancellation is missed. cancellation?.Cancel(); - cancellation?.Dispose(); subscription?.Dispose(); + + try + { + if (pump is not null) + { + await pump.WaitAsync(EventPumpDrainTimeout); + } + } + catch + { + // Detach-time errors (including a drain timeout) are best-effort. + } + + // Disposed after the drain so the pump is no longer reading the token. + cancellation?.Dispose(); } private static string EventStatusLabel(MxEvent evt) @@ -362,7 +403,7 @@ else public new async ValueTask DisposeAsync() { - DetachEvents(); + await DetachEventsAsync(); await base.DisposeAsync().ConfigureAwait(false); } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs index aece649..e80508b 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs @@ -51,7 +51,17 @@ public static class DashboardServiceCollectionExtensions // Singleton: EventsHub instances are transient (one per hub invocation), so the // subscriber bookkeeping they share with the broadcaster must outlive them. services.AddSingleton(); - services.AddSingleton(); + + // One instance behind two interfaces, registered concretely and forwarded: the + // publish side (IDashboardEventBroadcaster, driven by the session pipeline) and + // the in-process subscribe side (IDashboardSessionEventSubscriber, used by the + // session-details page) share subscriber bookkeeping, so resolving them to two + // instances would leave the page subscribed to a mirror nobody publishes to. + services.AddSingleton(); + services.AddSingleton( + static provider => provider.GetRequiredService()); + services.AddSingleton( + static provider => provider.GetRequiredService()); services.AddSingleton(); services.AddHostedService(); services.AddHostedService(); diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs index 638dbd5..f6af323 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs @@ -45,4 +45,23 @@ public sealed class DashboardHubsRegistrationTests .GetRequiredService(); Assert.NotNull(factory); } + + /// + /// The publish and in-process subscribe faces of the event mirror must resolve to + /// one instance: two would leave the session-details page reading a mirror the + /// session pipeline never publishes to, and the viewer gate would never open. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Build_WhenDashboardEnabled_ResolvesBothEventMirrorInterfacesToOneInstance() + { + await using WebApplication app = GatewayApplication.Build([]); + + IDashboardEventBroadcaster broadcaster = app.Services.GetRequiredService(); + IDashboardSessionEventSubscriber subscriber = app.Services + .GetRequiredService(); + + Assert.Same(broadcaster, subscriber); + Assert.Same(app.Services.GetRequiredService(), broadcaster); + } } From b0f5941e46d14a5382da302c8a74b2c43381ea98 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:32:07 -0400 Subject: [PATCH 11/23] fix(dashboard): generation-tagged feed subscribers survive pump teardown races; drain-timeout observability --- .../Dashboard/Components/DashboardPageBase.cs | 30 ++- .../Dashboard/DashboardSnapshotFeed.cs | 248 +++++++++++++----- .../Dashboard/DashboardSnapshotFeedTests.cs | 119 ++++++++- 3 files changed, 330 insertions(+), 67 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs index 670b8fe..ca13526 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs @@ -1,4 +1,5 @@ using Microsoft.AspNetCore.Components; +using Microsoft.Extensions.Logging; namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Components; @@ -31,6 +32,10 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable [Inject] protected IDashboardSnapshotFeed SnapshotFeed { get; set; } = null!; + /// Logger used to report a snapshot subscription that ended or would not drain. + [Inject] + protected ILogger? Logger { get; set; } + /// /// The most recent gateway metric snapshot. Synchronously seeded from /// for the very first @@ -62,9 +67,21 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable await _watchTask.WaitAsync(WatchDrainTimeout).ConfigureAwait(false); } } + catch (TimeoutException) + { + // Accepted limitation: the abandoned loop still holds its feed subscription, so + // the feed's idle gate stays open until it does unwind. There is no way to force + // a detach — the loop is parked on a dispatcher that is not draining — so the + // warning is the operator's only signal that a circuit teardown wedged. + Logger?.LogWarning( + "Dashboard page {Page} did not release its snapshot subscription within {Timeout}; " + + "the shared snapshot feed stays active until it unwinds.", + GetType().Name, + WatchDrainTimeout); + } catch { - // Disposal-time errors (including a drain timeout) are best-effort. + // Other disposal-time errors are best-effort. } _watchCancellation.Dispose(); @@ -87,10 +104,15 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable { // The page is going away. } - catch + catch (Exception error) { - // The feed is best-effort: the last rendered snapshot stays on screen and - // the snapshot service keeps serving GetSnapshot() for the next page load. + // The feed is best-effort: the last rendered snapshot stays on screen and the + // snapshot service keeps serving GetSnapshot() for the next page load. Logged + // once here, on the way out of the loop — never per snapshot. + Logger?.LogWarning( + error, + "Live snapshot updates ended for dashboard page {Page}; it keeps the last rendered snapshot.", + GetType().Name); } } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs index 72a89cb..ede8f0e 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs @@ -12,18 +12,30 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard; /// snapshot cost by the number of open pages. /// /// -/// The pump is idle-gated: it starts when the subscriber count goes 0 → 1 and is cancelled -/// and awaited when it goes 1 → 0, so an unwatched gateway runs no timer and builds no +/// +/// The pump is idle-gated: it starts when the first subscriber arrives and is cancelled and +/// awaited when the last one leaves, so an unwatched gateway runs no timer and builds no /// snapshots. Successive pumps are chained through _pumpTask, so a rapid /// unsubscribe/resubscribe restarts a fresh pump without ever running two enumerations at /// once. +/// +/// +/// Every subscriber is tagged with the pump generation it joined under, and a dying pump only +/// ever detaches its own generation. A pump ends its generation the instant its source fails +/// or completes — before the (possibly slow) enumerator disposal — so a subscriber arriving +/// while a pump unwinds starts a fresh generation instead of silently attaching to a dead +/// pump that is about to detach everybody and leave nobody watching. +/// /// public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed { + /// Generation value meaning "no pump is accepting subscribers". + private const long NoGeneration = 0; + private readonly IDashboardSnapshotService _snapshotService; private readonly ILogger _logger; private readonly object _gate = new(); - private readonly List> _subscribers = []; + private readonly List _subscribers = []; /// /// The most recent pump, completed while idle. A starting pump awaits its predecessor @@ -31,9 +43,15 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed /// private Task _pumpTask = Task.CompletedTask; - /// Cancellation for the live pump; null when no pump is running or one is being torn down. + /// Cancellation for the live pump; null when no generation is accepting subscribers. private CancellationTokenSource? _pumpCancellation; + /// The generation new subscribers join, or when no pump is live. + private long _generation = NoGeneration; + + /// Last generation handed out; only ever incremented under _gate. + private long _lastGeneration = NoGeneration; + /// Initializes a new instance of the class. /// Snapshot source to multicast. /// Optional logger for pump faults. @@ -60,7 +78,7 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed SingleWriter = false, }); - Subscribe(channel); + Subscription subscription = Subscribe(channel); try { await foreach (DashboardSnapshot snapshot in channel.Reader @@ -74,47 +92,40 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed { // Untokened on purpose: teardown must run to completion even when this // subscriber is unwinding because its own token fired. - await UnsubscribeAsync(channel).ConfigureAwait(false); + await UnsubscribeAsync(subscription).ConfigureAwait(false); } } - private void Subscribe(Channel channel) + private Subscription Subscribe(Channel channel) { lock (_gate) { - _subscribers.Add(channel); - if (_subscribers.Count != 1) - { - return; - } - - CancellationTokenSource cancellation = new(); - Task previous = _pumpTask; - _pumpCancellation = cancellation; - - // Task.Run, not a direct call: an async iterator runs synchronously up to its - // first suspension, and the first pull of the underlying watch can read the API - // key table. That must not run on the subscribing circuit's thread, let alone - // while this lock is held. - _pumpTask = Task.Run(() => PumpAsync(previous, cancellation, cancellation.Token)); + // A live generation is joined; otherwise this subscriber starts one. Keying on + // "is a generation live" rather than "is this the first subscriber" is what makes + // a subscriber arriving while a pump unwinds start a fresh pump for itself. + long generation = _pumpCancellation is null ? StartPumpLocked() : _generation; + Subscription subscription = new(channel, generation); + _subscribers.Add(subscription); + return subscription; } } - private async Task UnsubscribeAsync(Channel channel) + private async Task UnsubscribeAsync(Subscription subscription) { CancellationTokenSource? cancellation; Task pump; lock (_gate) { - if (!_subscribers.Remove(channel) || _subscribers.Count != 0) + if (!_subscribers.Remove(subscription) || _subscribers.Count != 0) { - // Either the pump already dropped this channel (it completed or faulted - // and reset itself), or other viewers are still watching. + // Either the pump already detached this subscription (it completed or + // faulted), or other viewers are still watching. return; } cancellation = _pumpCancellation; _pumpCancellation = null; + _generation = NoGeneration; pump = _pumpTask; } @@ -124,7 +135,7 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed } catch (ObjectDisposedException) { - // The pump reset itself and disposed its own cancellation source first. + // The pump ended on its own and disposed its cancellation source first. } try @@ -138,33 +149,90 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed } } - private async Task PumpAsync(Task previous, CancellationTokenSource cancellation, CancellationToken cancellationToken) + /// + /// Starts a pump generation. Must be called while holding _gate; the caller adds + /// the subscribers that belong to the returned generation. + /// + /// The new generation identifier. + private long StartPumpLocked() + { + long generation = ++_lastGeneration; + CancellationTokenSource cancellation = new(); + Task previous = _pumpTask; + _generation = generation; + _pumpCancellation = cancellation; + + // Task.Run, not a direct call: an async iterator runs synchronously up to its + // first suspension, and the first pull of the underlying watch can read the API + // key table. That must not run on the subscribing circuit's thread, let alone + // while this lock is held. + _pumpTask = Task.Run(() => PumpAsync(generation, previous, cancellation, cancellation.Token)); + return generation; + } + + private async Task PumpAsync( + long generation, + Task previous, + CancellationTokenSource cancellation, + CancellationToken cancellationToken) { try { // Never overlap with the enumeration this pump replaces. await previous.ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); - await foreach (DashboardSnapshot snapshot in _snapshotService + // Enumerated by hand rather than with await foreach so the generation can be + // ended the moment the source fails or completes — await foreach would run the + // enumerator's disposal first, and a subscriber arriving during that disposal + // would join a generation that is already doomed. + IAsyncEnumerator snapshots = _snapshotService .WatchSnapshotsAsync(cancellationToken) - .ConfigureAwait(false)) + .GetAsyncEnumerator(cancellationToken); + try { - Broadcast(snapshot); + while (true) + { + bool moved; + try + { + moved = await snapshots.MoveNextAsync().ConfigureAwait(false); + } + catch + { + EndGeneration(generation); + throw; + } + + if (!moved) + { + EndGeneration(generation); + break; + } + + Broadcast(generation, snapshots.Current); + } + } + finally + { + await snapshots.DisposeAsync().ConfigureAwait(false); } - // The source completed on its own; hand the completion to the subscribers - // and re-arm so the next one starts a fresh enumeration. - Reset(cancellation, error: null); + // The source completed on its own; hand the completion to this generation's + // subscribers and re-arm so the next one starts a fresh enumeration. + Reset(generation, error: null); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - // Last subscriber left: the unsubscribing caller already detached the - // channels and cleared the pump state. + // The production DashboardSnapshotService swallows cancellation and yield-breaks, + // so normal teardown exits through the fall-through above (with an ownership-checked + // Reset that finds no subscribers); an implementation that propagates the token + // instead exits here. Both shapes end the generation exactly once. + EndGeneration(generation); } catch (Exception error) { _logger.LogWarning(error, "Dashboard snapshot feed stopped; the next subscriber restarts it."); - Reset(cancellation, error); + Reset(generation, error); } finally { @@ -172,45 +240,101 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed } } - private void Broadcast(DashboardSnapshot snapshot) + private void Broadcast(long generation, DashboardSnapshot snapshot) { lock (_gate) { - foreach (Channel subscriber in _subscribers) + foreach (Subscription subscriber in _subscribers) { + if (subscriber.Generation != generation) + { + continue; + } + // Bounded/DropOldest: always accepted unless the channel is completed. - subscriber.Writer.TryWrite(snapshot); + subscriber.Channel.Writer.TryWrite(snapshot); } } } /// - /// Detaches every subscriber and clears the pump state so the next subscriber starts a - /// new enumeration. The detached subscribers observe (or a - /// clean end of stream) from their own WatchAsync. + /// Stops handing to new subscribers. Called the instant a + /// pump's source fails or completes, before its enumerator is disposed. /// - /// The calling pump's cancellation source, used as its ownership token. - /// Failure to surface, or null when the source completed cleanly. - private void Reset(CancellationTokenSource cancellation, Exception? error) + /// The generation that has ended. + private void EndGeneration(long generation) { - Channel[] detached; lock (_gate) { - if (!ReferenceEquals(_pumpCancellation, cancellation)) - { - // A newer pump (or an in-flight teardown) owns the state now: its - // subscribers must not be detached by this pump's exit. - return; - } - - detached = _subscribers.ToArray(); - _subscribers.Clear(); - _pumpCancellation = null; - } - - foreach (Channel subscriber in detached) - { - subscriber.Writer.TryComplete(error); + EndGenerationLocked(generation); } } + + /// Clears the live-pump state if still owns it. + /// The generation that has ended. + private void EndGenerationLocked(long generation) + { + if (_generation != generation) + { + return; + } + + _generation = NoGeneration; + _pumpCancellation = null; + } + + /// + /// Detaches the subscribers of a finished generation and re-arms the feed. Subscribers of + /// any other generation are left alone — they belong to a pump that is still running (or + /// about to), so a dying pump must not take them down with it. + /// + /// The generation whose subscribers are being detached. + /// Failure to surface, or null when the source completed cleanly. + private void Reset(long generation, Exception? error) + { + List> detached = []; + lock (_gate) + { + for (int index = _subscribers.Count - 1; index >= 0; index--) + { + if (_subscribers[index].Generation != generation) + { + continue; + } + + detached.Add(_subscribers[index].Channel); + _subscribers.RemoveAt(index); + } + + EndGenerationLocked(generation); + + if (_subscribers.Count > 0 && _pumpCancellation is null) + { + // Belt and braces: subscribers left with no live pump would be frozen for + // good, because only a subscriber that finds no generation starts one. + long restarted = StartPumpLocked(); + foreach (Subscription subscriber in _subscribers) + { + subscriber.Generation = restarted; + } + } + } + + foreach (Channel channel in detached) + { + channel.Writer.TryComplete(error); + } + } + + /// One viewer's delivery channel plus the pump generation serving it. + /// Delivery channel for this viewer. + /// Pump generation this viewer joined under. + private sealed class Subscription(Channel channel, long generation) + { + /// Gets the viewer's delivery channel. + public Channel Channel { get; } = channel; + + /// Gets or sets the pump generation currently serving this viewer. + public long Generation { get; set; } = generation; + } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs index b86bfd7..330ac17 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs @@ -201,6 +201,59 @@ public sealed class DashboardSnapshotFeedTests await DrainAsync(second, secondMove); } + /// + /// The race the generation tagging exists for: a page subscribes in the window between + /// the source failing and the dying pump detaching its subscribers. Without generations + /// the newcomer joined the doomed pump, was detached with its error, and no pump ever + /// restarted (only a first subscriber started one) — that page froze for good. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WhenASubscriberJoinsWhileAFaultedPumpUnwinds_IsServedByAFreshPump() + { + FakeSnapshotService service = new(); + service.HoldDisposal(); + DashboardSnapshotFeed feed = new(service); + + using CancellationTokenSource firstCancellation = new(); + IAsyncEnumerator first = + feed.WatchAsync(firstCancellation.Token).GetAsyncEnumerator(firstCancellation.Token); + Task firstMove = first.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 1); + + service.Fault(new InvalidOperationException("simulated snapshot source failure")); + + // The pump has observed the failure and is parked disposing the enumerator — the + // exact window in which a page used to attach itself to a doomed pump. + await service.DisposalReached.WaitAsync(TestTimeout); + + using CancellationTokenSource secondCancellation = new(); + IAsyncEnumerator second = + feed.WatchAsync(secondCancellation.Token).GetAsyncEnumerator(secondCancellation.Token); + + // An async iterator body runs synchronously up to its first await, so the + // subscription is registered by the time MoveNextAsync hands back its task. + Task secondMove = second.MoveNextAsync().AsTask(); + + service.ReleaseDisposal(); + + // The subscriber that was there when the source broke still learns about it... + InvalidOperationException failure = + await Assert.ThrowsAsync(() => firstMove.WaitAsync(TestTimeout)); + Assert.Equal("simulated snapshot source failure", failure.Message); + await first.DisposeAsync(); + + // ...and the one that joined mid-unwind is served by a restarted enumeration + // instead of inheriting the failure. + await WaitUntilAsync(() => service.EnumerationCount >= 2); + await PushUntilAsync(service, secondMove); + Assert.True(await secondMove.WaitAsync(TestTimeout)); + Assert.StartsWith("push-", second.Current.GatewayVersion, StringComparison.Ordinal); + + await secondCancellation.CancelAsync(); + await DrainAsync(second, secondMove); + } + /// Builds a snapshot whose version string identifies it in assertions. /// Identity marker carried in GatewayVersion. /// A snapshot carrying the supplied identity marker. @@ -282,13 +335,19 @@ public sealed class DashboardSnapshotFeedTests private sealed class FakeSnapshotService : IDashboardSnapshotService { private readonly Channel _pushes = Channel.CreateUnbounded(); + private readonly TaskCompletionSource _disposalReached = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _disposalRelease = new(TaskCreationOptions.RunContinuationsAsynchronously); private int _enumerationCount; private int _completedEnumerationCount; private volatile bool _lastEnumerationWasCancelled; + private volatile bool _holdDisposal; /// Gets the number of times the feed started enumerating this source. public int EnumerationCount => Volatile.Read(ref _enumerationCount); + /// Gets a task that completes when a held enumerator disposal is reached. + public Task DisposalReached => _disposalReached.Task; + /// Gets the number of enumerations that have finished (cancelled, faulted, or completed). public int CompletedEnumerationCount => Volatile.Read(ref _completedEnumerationCount); @@ -303,11 +362,34 @@ public sealed class DashboardSnapshotFeedTests /// Exception to throw from the enumeration. public void Fault(Exception error) => _pushes.Writer.TryWrite(error); + /// + /// Parks enumerator disposal until , which holds the pump + /// in the window between observing the source's failure and detaching its subscribers. + /// + public void HoldDisposal() => _holdDisposal = true; + + /// Releases a held enumerator disposal. + public void ReleaseDisposal() => _disposalRelease.TrySetResult(); + /// public DashboardSnapshot GetSnapshot() => CreateSnapshot("current"); /// - public async IAsyncEnumerable WatchSnapshotsAsync( + public IAsyncEnumerable WatchSnapshotsAsync(CancellationToken cancellationToken) + => new GatedEnumerable(this); + + private async Task OnDisposingAsync() + { + if (!_holdDisposal) + { + return; + } + + _disposalReached.TrySetResult(); + await _disposalRelease.Task.ConfigureAwait(false); + } + + private async IAsyncEnumerable EnumerateAsync( [EnumeratorCancellation] CancellationToken cancellationToken) { Interlocked.Increment(ref _enumerationCount); @@ -329,5 +411,40 @@ public sealed class DashboardSnapshotFeedTests Interlocked.Increment(ref _completedEnumerationCount); } } + + /// + /// Wraps the iterator so disposal is a control point of its own: the feed ends a pump + /// generation when MoveNextAsync fails, which is strictly before this disposal runs. + /// + /// The fake whose enumeration is being wrapped. + private sealed class GatedEnumerable(FakeSnapshotService owner) : IAsyncEnumerable + { + /// Creates a gated enumerator over the fake's enumeration. + /// Token to observe for cancellation. + /// The gated enumerator. + public IAsyncEnumerator GetAsyncEnumerator( + CancellationToken cancellationToken = default) + => new GatedEnumerator(owner, owner.EnumerateAsync(cancellationToken).GetAsyncEnumerator(cancellationToken)); + } + + private sealed class GatedEnumerator( + FakeSnapshotService owner, + IAsyncEnumerator inner) : IAsyncEnumerator + { + /// Gets the current snapshot. + public DashboardSnapshot Current => inner.Current; + + /// Advances the wrapped enumeration. + /// A task that yields whether another snapshot is available. + public ValueTask MoveNextAsync() => inner.MoveNextAsync(); + + /// Parks while the fake holds disposal, then disposes the wrapped enumeration. + /// A task that represents the asynchronous operation. + public async ValueTask DisposeAsync() + { + await owner.OnDisposingAsync().ConfigureAwait(false); + await inner.DisposeAsync().ConfigureAwait(false); + } + } } } From 756296886b6baf1f9651057d44808e6e4e31c1d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:43:17 -0400 Subject: [PATCH 12/23] =?UTF-8?q?fix(dashboard):=20generation-scoped=20idl?= =?UTF-8?q?e=20gate=20in=20UnsubscribeAsync=20=E2=80=94=20no=20zero-subscr?= =?UTF-8?q?iber=20pump=20survives?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Dashboard/DashboardSnapshotFeed.cs | 42 +++++++++-- .../Dashboard/DashboardSnapshotFeedTests.cs | 72 +++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs index ede8f0e..301104f 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs @@ -14,8 +14,9 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard; /// /// /// The pump is idle-gated: it starts when the first subscriber arrives and is cancelled and -/// awaited when the last one leaves, so an unwatched gateway runs no timer and builds no -/// snapshots. Successive pumps are chained through _pumpTask, so a rapid +/// awaited when the last subscriber of the live generation leaves, so an unwatched +/// gateway runs no timer and builds no snapshots. Successive pumps are chained through +/// _pumpTask, so a rapid /// unsubscribe/resubscribe restarts a fresh pump without ever running two enumerations at /// once. /// @@ -116,10 +117,25 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed Task pump; lock (_gate) { - if (!_subscribers.Remove(subscription) || _subscribers.Count != 0) + if (!_subscribers.Remove(subscription)) { - // Either the pump already detached this subscription (it completed or - // faulted), or other viewers are still watching. + // The pump already detached this subscription (it completed or faulted). + return; + } + + if (subscription.Generation != _generation) + { + // This viewer belonged to a generation that has already ended. The live + // pump — if there is one — serves other viewers and must not be cancelled + // on their behalf; the dying pump is stopping under its own steam. + return; + } + + if (HasSubscribersLocked(_generation)) + { + // Other viewers are still watching the live generation. Counting the whole + // list here would be wrong: subscribers of an ending generation linger in it + // until that pump's Reset runs, and they must not hold the idle gate open. return; } @@ -149,6 +165,22 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed } } + /// Reports whether any subscriber is still being served by a generation. + /// The generation to look for. + /// True when at least one subscriber carries that generation. + private bool HasSubscribersLocked(long generation) + { + foreach (Subscription subscriber in _subscribers) + { + if (subscriber.Generation == generation) + { + return true; + } + } + + return false; + } + /// /// Starts a pump generation. Must be called while holding _gate; the caller adds /// the subscribers that belong to the returned generation. diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs index 330ac17..8c0091e 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs @@ -254,6 +254,72 @@ public sealed class DashboardSnapshotFeedTests await DrainAsync(second, secondMove); } + /// + /// The idle gate is per generation, not per subscriber count. A viewer that joins while a + /// pump unwinds starts a new generation, and the old generation's viewers linger in the + /// list until that pump's reset runs — so a global "is the list empty" check let the new + /// viewer leave without cancelling the generation it had just started, leaving a pump + /// enumerating the snapshot source with nobody watching it. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task WatchAsync_WhenAJoinerLeavesWhileAnOldGenerationLingers_LeavesNoPumpRunning() + { + FakeSnapshotService service = new(); + service.HoldDisposal(); + DashboardSnapshotFeed feed = new(service); + + using CancellationTokenSource oldCancellation = new(); + IAsyncEnumerator oldSubscriber = + feed.WatchAsync(oldCancellation.Token).GetAsyncEnumerator(oldCancellation.Token); + Task oldMove = oldSubscriber.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => service.EnumerationCount >= 1); + + service.Fault(new InvalidOperationException("simulated snapshot source failure")); + await service.DisposalReached.WaitAsync(TestTimeout); + + // Joins mid-unwind (starting a fresh generation) and leaves again before the dying + // pump has detached the subscriber that is still lingering in the list. + using CancellationTokenSource joinerCancellation = new(); + IAsyncEnumerator joiner = + feed.WatchAsync(joinerCancellation.Token).GetAsyncEnumerator(joinerCancellation.Token); + Task joinerMove = joiner.MoveNextAsync().AsTask(); + await joinerCancellation.CancelAsync(); + + // The joiner's unwind is a continuation of its cancelled channel read; give it time to + // run its unsubscribe before the dying pump is released. Only the interleaving depends + // on this delay — the assertions below hold either way. + await Task.Delay(TimeSpan.FromMilliseconds(150)); + + service.ReleaseDisposal(); + + await Assert.ThrowsAsync(() => oldMove.WaitAsync(TestTimeout)); + await oldSubscriber.DisposeAsync(); + + // Completes only once the joiner's unsubscribe has awaited its generation's pump. + await DrainAsync(joiner, joinerMove); + + // Nobody is watching, so nothing may consume a snapshot: a surviving pump would drain + // this push within its first read. + service.Push(CreateSnapshot("orphan-check")); + await Task.Delay(TimeSpan.FromMilliseconds(150)); + Assert.Equal(1, service.PendingPushCount); + + // ...and the next viewer still starts cleanly, picking up the queued snapshot. + int enumerationsBefore = service.EnumerationCount; + using CancellationTokenSource nextCancellation = new(); + IAsyncEnumerator next = + feed.WatchAsync(nextCancellation.Token).GetAsyncEnumerator(nextCancellation.Token); + Task nextMove = next.MoveNextAsync().AsTask(); + + await WaitUntilAsync(() => service.EnumerationCount > enumerationsBefore); + Assert.True(await nextMove.WaitAsync(TestTimeout)); + Assert.Equal("orphan-check", next.Current.GatewayVersion); + + await nextCancellation.CancelAsync(); + await DrainAsync(next, nextMove); + } + /// Builds a snapshot whose version string identifies it in assertions. /// Identity marker carried in GatewayVersion. /// A snapshot carrying the supplied identity marker. @@ -348,6 +414,12 @@ public sealed class DashboardSnapshotFeedTests /// Gets a task that completes when a held enumerator disposal is reached. public Task DisposalReached => _disposalReached.Task; + /// + /// Gets the number of queued snapshots no enumeration has taken yet. A live pump + /// drains this even with nobody watching, so a stable count proves the feed is idle. + /// + public int PendingPushCount => _pushes.Reader.Count; + /// Gets the number of enumerations that have finished (cancelled, faulted, or completed). public int CompletedEnumerationCount => Volatile.Read(ref _completedEnumerationCount); From 53881c6220cf0bbf063320f58b6988a39f6b33cb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 20:51:36 -0400 Subject: [PATCH 13/23] docs(dashboard): in-process page feeds, two-tier idle gating, hubs as the external surface --- docs/GatewayDashboardDesign.md | 202 ++++++++++++++++++++++++++------- 1 file changed, 161 insertions(+), 41 deletions(-) diff --git a/docs/GatewayDashboardDesign.md b/docs/GatewayDashboardDesign.md index 77be9d4..97500d3 100644 --- a/docs/GatewayDashboardDesign.md +++ b/docs/GatewayDashboardDesign.md @@ -98,6 +98,7 @@ ZB.MOM.WW.MxGateway.Server StatusBadge.razor FaultList.razor DashboardSnapshotService.cs + DashboardSnapshotFeed.cs DashboardAuthorizationHandler.cs DashboardAuthenticator.cs DashboardApiKeyAuthorization.cs @@ -110,9 +111,15 @@ ZB.MOM.WW.MxGateway.Server ``` The dashboard exposes three named SignalR hubs in addition to Blazor Server's -internal circuit; pages connect to those hubs from within the circuit via the -`DashboardHubConnectionFactory` helper. The hubs publish snapshot, alarm, and -per-session event updates that the pages render in place of polling. +internal circuit. The hubs are the **remote** surface: they publish snapshot, +alarm, and per-session event updates to clients outside the gateway process. +Server-rendered Blazor pages do not use them. A page runs inside this process, +so it consumes the producing services directly through in-process seams — +`IDashboardSnapshotFeed`, `IDashboardSessionEventSubscriber`, and +`IGatewayAlarmService` — instead of opening a loopback WebSocket back into its +own heap. `DashboardHubConnectionFactory`, the helper a circuit used to open +those connections, stays registered for out-of-tree consumers, but no in-repo +page resolves it. ## Dashboard Data Source @@ -159,7 +166,57 @@ gateway internals. ## Realtime Updates -Updates flow over three SignalR hubs, all guarded by the +Realtime data reaches two audiences over two seams: + +- **in-process**, for the server-rendered Blazor pages, which run inside the + gateway process and read the producing services directly; +- **SignalR hubs**, for clients outside the process. + +Pages originally took the hub path too, which put a loopback WebSocket, a +hub-token mint, and a serialize/deserialize round trip between a Blazor component +and an object already in its own heap. The in-process seams remove that hop. The +hubs stay for the audience that genuinely needs a wire. + +### In-process page feeds + +| Page | Seam | Producer | +|---|---|---| +| 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 | + +The snapshot feed multicasts rather than handing each page its own enumeration: +`WatchSnapshotsAsync` is not multicast on its own — each enumeration owns a timer +and builds its own snapshot per tick — so a subscription per page would multiply +the snapshot cost by the number of open pages. Each subscriber reads through a +capacity-1 drop-oldest channel, so a circuit that renders slowly skips snapshots +instead of buffering without bound or stalling the pump. + +`DashboardPageBase` seeds `Snapshot` synchronously from +`IDashboardSnapshotService.GetSnapshot()` in `OnInitializedAsync` so the first +render is non-empty, then calls `InvokeAsync(StateHasChanged)` for every snapshot +the feed yields. On dispose it cancels the watch and waits at most **5 seconds** +for the loop to drain, logging a warning on timeout. The bound is deliberate: the +loop marshals renders through the renderer's dispatcher and disposal can run on +that same dispatcher, so an unconditional wait would hang on a wedged dispatcher. +The accepted cost is that an abandoned loop still holds its feed subscription — the +feed's idle gate stays open until it unwinds — and the warning is the operator's +only signal that a circuit teardown wedged. + +`SessionDetailsPage` subscribes for the current session id and renders the most +recent N events (default 50) in a "Recent events" table. Its pump drains everything +queued and renders once per batch rather than once per event, and it re-checks +**inside the renderer dispatch** — where the subscription field is written, making +the check an unsynchronized read of dispatcher-owned state — that the batch's +subscription is still the live one. A batch read before a session switch would +otherwise render the previous session's events under the new session's heading. +Detaching cancels the pump, disposes the subscription (which releases the viewer +registration and completes the channel, so the pump has an exit even if +cancellation is missed), then drains under its own timeout. + +### SignalR hubs (remote clients) + +Updates for out-of-process clients flow over three SignalR hubs, all guarded by the `MxGateway.Dashboard.HubClients` policy (cookie OR `MxGateway.Dashboard.HubToken` bearer). Each hub class is `[Authorize(Policy = HubClientsPolicy)]`. @@ -167,46 +224,68 @@ bearer). Each hub class is `[Authorize(Policy = HubClientsPolicy)]`. |---|---|---|---|---| | `DashboardSnapshotHub` | `/hubs/snapshot` | `DashboardSnapshotPublisher` (BackgroundService consuming `IDashboardSnapshotService.WatchSnapshotsAsync`) | `DashboardSnapshot` | Sent to all connected clients on every snapshot tick, but only while at least one client is connected (see "Idle gating" below); new connections receive the current snapshot synchronously in `OnConnectedAsync`. | | `AlarmsHub` | `/hubs/alarms` | `AlarmsHubPublisher` (BackgroundService consuming `IGatewayAlarmService.StreamAsync(filter: null)`) | `AlarmFeedMessage` (`active_alarm` / `snapshot_complete` / `transition`) | Connected clients auto-join `__alarms__`; all clients receive every message. Publisher auto-reconnects every 5s on stream faults. | -| `EventsHub` | `/hubs/events` | `DashboardEventBroadcaster` invoked by each session's internal dashboard-mirror subscriber on its `SessionEventDistributor` (registered when the session becomes Ready) | `MxEvent` | Clients call `SubscribeSession(sessionId)` to join `session:{id}`, which also registers them in `EventsHubViewerRegistry` — the mirror is gated on that registry (see "Mirror gating" below). The dashboard is a first-class distributor subscriber, so it receives the session's events whether or not a gRPC client is streaming. It sees RAW session events — not the per-gRPC-subscriber `AfterWorkerSequence` filtering that `EventStreamService` applies at its own boundary — because the dashboard is a separate LDAP-authenticated monitoring view meant to show the session's full event activity. Tag values are stripped from the mirrored `MxEvent` copy by `DashboardEventBroadcaster` when `Dashboard:ShowTagValues` is false (the default) — event metadata (tag reference, quality, status, timestamps) still renders, but the value fields are blanked, so no value leaks through this seam. The per-session hub ACL that would scope a Viewer to specific sessions is still outstanding (SEC-25 / remediation roadmap item 12); the value redaction is the near-term hardening that closes the value-leak seam independently of that ACL. | +| `EventsHub` | `/hubs/events` | `DashboardEventBroadcaster` invoked by each session's internal dashboard-mirror subscriber on its `SessionEventDistributor` (registered when the session becomes Ready) | `MxEvent` | Clients call `SubscribeSession(sessionId)` to join `session:{id}`, which also registers them in `EventsHubViewerRegistry` — the mirror is gated on that registry, which counts hub and in-process viewers alike (see "Mirror gating" below). The dashboard is a first-class distributor subscriber, so it receives the session's events whether or not a gRPC client is streaming. It sees RAW session events — not the per-gRPC-subscriber `AfterWorkerSequence` filtering that `EventStreamService` applies at its own boundary — because the dashboard is a separate LDAP-authenticated monitoring view meant to show the session's full event activity. Tag values are stripped from the mirrored `MxEvent` copy by `DashboardEventBroadcaster` when `Dashboard:ShowTagValues` is false (the default) — event metadata (tag reference, quality, status, timestamps) still renders, but the value fields are blanked, so no value leaks through this seam. The per-session ACL that would scope a Viewer to specific sessions is still outstanding for this seam and the in-process one alike (SEC-25 / remediation roadmap item 12); the value redaction is the near-term hardening that closes the value-leak seam independently of that ACL. | -`DashboardPageBase` opens a `DashboardSnapshotHub` connection via the connection -factory in `OnInitializedAsync`, seeds `Snapshot` synchronously from -`IDashboardSnapshotService.GetSnapshot()` so the first render is non-empty, and -calls `InvokeAsync(StateHasChanged)` on every `SnapshotUpdated` push. SignalR's -`WithAutomaticReconnect` handles transient disconnects. +### Default cadences -`SessionDetailsPage` additionally opens an `EventsHub` connection for the -current session id and renders the most recent N events (default 50) in a -"Recent events" table with a live/offline connection pill. - -Default cadences: +Both seams consume the same producing services, so they share these cadences: - snapshot service produces one snapshot per `MxGateway:Dashboard:SnapshotIntervalMilliseconds` (default 1s); - 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`). + to its internal dashboard-mirror subscriber (independent of any gRPC `StreamEvents`); +- the alarms page's provider-status badge 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. ### Idle gating and snapshot cost A snapshot is not free: each one takes a session-registry snapshot and sorts it, copies the metrics dictionaries under the global metrics lock, and projects sessions, workers, faults, and the Galaxy summary. Without gating that work ran -once a second for the life of the process even when no browser was connected. +once a second for the life of the process even when nothing was watching. -`DashboardSnapshotHub` counts live connections into the singleton +Gating is two-tier, because the two seams have independent audiences and each must +be able to reach zero on its own. + +**Hub tier.** `DashboardSnapshotHub` counts live connections into the singleton `DashboardSnapshotHubConnectionCounter` (`OnConnectedAsync` / `OnDisconnectedAsync`, clamped at zero). `DashboardSnapshotPublisher` reads that count before advancing the snapshot enumerator: while it is zero the publisher does not call `MoveNextAsync` at all, so the producing iterator stays suspended at its `yield` and builds nothing — the gate removes the snapshot *build*, not just the broadcast. The publisher -re-checks once a second while idle, so the first viewer to connect resumes the tick -within roughly one snapshot interval. That viewer does not wait for it either: -`DashboardPageBase` seeds its first render synchronously from -`IDashboardSnapshotService.GetSnapshot()`, and `OnConnectedAsync` pushes a snapshot -to the new connection immediately. +re-checks once a second while idle, so the first client to connect resumes the tick +within roughly one snapshot interval, and `OnConnectedAsync` pushes the current +snapshot to that connection immediately. Now that no in-repo page connects to the +hub, this tier stays idle unless a remote client connects. -Two per-tick costs inside the snapshot itself are bounded independently of the gate: +**In-process tier.** `DashboardSnapshotFeed` gates its own pump on its subscriber +list: the first subscriber starts the pump, the last one leaving cancels it and +awaits it, so a gateway with no page open runs no timer and builds no snapshots on +this seam either. Successive pumps are chained through the previous pump's task, so +an unsubscribe immediately followed by a resubscribe restarts a fresh pump without +ever running two enumerations at once. A page does not wait for the pump's first +tick — `DashboardPageBase` seeds its first render synchronously from +`IDashboardSnapshotService.GetSnapshot()`. + +That gate is generation-scoped rather than a plain subscriber count. Each pump owns +a generation, each subscriber is tagged with the generation it joined under, a pump +ends its generation the instant its source faults or completes — before the possibly +slow enumerator disposal — and a dying pump only ever detaches its own generation's +subscribers. Two races motivate the extra state: + +- a subscriber arriving mid-teardown must start a fresh generation rather than + attach to a pump that is about to detach everybody and leave nobody watching; +- an unsubscribe must compare its own generation against the live one before it + cancels anything. Subscribers of an ending generation linger in the list until + that pump's reset runs, so counting the whole list would let them hold the idle + gate open, and cancelling on their behalf would stop a live pump that other + viewers depend on. + +Two per-tick costs inside the snapshot itself are bounded independently of either gate: - the effective configuration (`EffectiveGatewayConfiguration`) is built once and cached. It is a projection of `IOptions`, which the gateway binds @@ -220,26 +299,43 @@ Two per-tick costs inside the snapshot itself are bounded independently of the g retried on the next tick and the previous summaries stay on screen. Avoid pushing every MXAccess data-change event into a wider broadcast group. -The current design routes events strictly through `session:{id}` groups; the -snapshot hub continues to carry aggregate event counters and rates. +Events are routed strictly per session (`session:{id}` groups on the hub, +per-session subscriber lists in process); the snapshot seams continue to carry +aggregate event counters and rates. ### Mirror gating Each session's dashboard-mirror subscriber calls `DashboardEventBroadcaster.Publish` for every event the session produces, -independently of whether any browser is watching that session. SignalR does not -expose group membership, so the broadcaster cannot ask whether `session:{id}` is -empty. `EventsHubViewerRegistry` (singleton) supplies that answer: `EventsHub` -mirrors its own `AddToGroup` / `RemoveFromGroup` calls into it, and -`OnDisconnectedAsync` releases every subscription a dropped connection held — the -only reliable signal for a browser tab that closes without unsubscribing. -`Publish` returns immediately when `HasViewers(sessionId)` is false, **before** -the redaction clone. That matters because redaction is on by default +independently of whether anything is watching that session. `Publish` returns +immediately when `EventsHubViewerRegistry.HasViewers(sessionId)` is false, +**before** the redaction clone. That matters because redaction is on by default (`Dashboard:ShowTagValues` false), so the unwatched steady state — nobody on any session-details page — previously paid a deep protobuf clone plus a send to an empty group for every event of every session. Behaviour for a watched session is unchanged. +The registry counts both audiences, which is what lets one gate serve both seams. +`EventsHub` mirrors its own `AddToGroup` / `RemoveFromGroup` calls into it — +SignalR does not expose group membership, so the broadcaster cannot ask whether +`session:{id}` is empty — and `OnDisconnectedAsync` releases every subscription a +dropped connection held, the only reliable signal for a browser tab that closes +without unsubscribing. An in-process subscription registers the same way under a +synthetic `inproc-`-prefixed connection id, which cannot collide with a SignalR +connection id and makes the origin obvious in a debugger; disposing it removes the +viewer and releases the synthetic connection, because that id never reconnects and +nothing else would ever release its per-connection entry. Both paths use the same +ordering — register before becoming a delivery target, deregister after ceasing to +be one — so the widest a race window opens is a redaction clone that reaches +nobody, never a dropped event that was owed to a live viewer. + +Redaction happens once per event, not once per audience: `Publish` produces a +single redacted clone and hands that same instance to the in-process subscribers +and to the hub group. In-process delivery runs first and synchronously — it cannot +throw, and it must not be skipped by the guard clause around the hub send — into +per-subscriber bounded drop-oldest channels, so a page that falls behind loses its +oldest queued events rather than blocking the session's event pipeline. + The mirror subscriber itself is still registered on the `SessionEventDistributor` for the session's whole lifetime; only the per-event work is gated. Starting and stopping the mirror lease lazily with the first and last viewer was considered @@ -369,8 +465,9 @@ panel. The panel shows each subscribed tag's live value, MXAccess data type, quality and source timestamp, refreshed every two seconds. The subscription panel is the explicit opt-in tag-value surface: it always shows values regardless of `Dashboard:ShowTagValues`, which governs the diagnostic -session/worker views and the per-session `EventsHub` mirror (values are -redacted from the mirrored events when the flag is false). +session/worker views and the per-session event mirror — both its hub and +in-process audiences (values are redacted from the mirrored events when the flag +is false). ### Alarms page @@ -380,7 +477,11 @@ defaults to showing unacknowledged `Active` alarms; filters add acknowledged alarms and narrow by area, severity range, and a reference/source/description text search. Cleared alarms are not retained — the gateway holds no alarm-history store, so the page reflects only the live active set. The page is -read-only; it does not acknowledge alarms. If `MxGateway:Alarms:Enabled` 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 false the central monitor never starts, and the page says so instead of showing an empty list with no explanation. @@ -536,6 +637,14 @@ Three authorization policies are registered: cookie OR a `MxGateway.Dashboard.HubToken` bearer (used by WebSocket upgrades where the cookie can't be forwarded). +The in-process page feeds carry no authentication of their own, and need none: +`MapRazorComponents()` applies `RequireAuthorization(ViewerPolicy)` to the +component endpoints, so a page can only run inside a circuit whose principal is +already an authorized Viewer. The hub-token flow below therefore covers only the +remote hub surface. Neither seam scopes a Viewer to particular sessions — SEC-25 +(the per-session ACL) is outstanding for both, and the mirror's value redaction +remains the near-term mitigation, unchanged by the move in-process. + Two environmental bypasses still apply, both scoped to **read-only** access: `MxGateway:Authentication:Mode = Disabled` and `MxGateway:Dashboard:AllowAnonymousLocalhost` (default `true`, loopback only) each satisfy a requirement that includes the Viewer @@ -580,12 +689,15 @@ surface is affected. Never enable in production. ### Hub bearer flow +This flow serves remote hub clients only; in-process pages are authorized by the +component endpoint's `ViewerPolicy` and never mint a token. + SignalR connections cannot reuse the `__Host-` cookie when the JS client upgrades to WebSocket — the cookie's `SameSite=Strict; Path=/` keeps it from being forwarded by the browser's WebSocket layer in some edge cases. The dashboard mints short-lived bearer tokens for the connection: -1. The cookie-authenticated Blazor page calls `GET /hubs/token` +1. The cookie-authenticated client calls `GET /hubs/token` (gated by `ViewerPolicy`, cookie-only). 2. `HubTokenService.Issue(user)` serializes the user's name, NameIdentifier, and role claims to JSON, encrypts with the ASP.NET Core data-protection @@ -605,7 +717,9 @@ dashboard mints short-lived bearer tokens for the connection: `DashboardHubConnectionFactory` (scoped to the Blazor circuit) wraps the HubConnectionBuilder and supplies a fresh token via `AccessTokenProvider` on -every (re)connect, so the short 5-minute lifetime is transparent to clients. +every (re)connect, so the short 5-minute lifetime is transparent to whoever uses +it. It remains registered, but no in-repo page opens a hub connection any more; +external clients implement the equivalent refresh themselves. Caveat — logout does not revoke outstanding tokens. Logout clears the dashboard cookie, but hub bearer tokens are self-contained, data-protection-encrypted, and @@ -700,8 +814,14 @@ Integration tests should verify: - a user in a Viewer-mapped LDAP group can render every page but cannot invoke the Admin-only management actions, - a user with no mapped LDAP group cannot sign in at all, -- live snapshot updates when a fake session changes state are delivered - via the `/hubs/snapshot` push, not by polling. +- live snapshot updates when a fake session changes state reach a page through + the in-process `IDashboardSnapshotFeed` and reach a remote client through the + `/hubs/snapshot` push — neither by polling; +- the snapshot feed's idle gate: no subscribers means no pump, the last + subscriber of the live generation stops it, and a subscriber arriving + mid-teardown gets a fresh generation rather than a dead one; +- the event mirror's viewer gate counts in-process subscriptions as well as hub + connections, and a disposed in-process subscription releases its viewer count. ## Initial Implementation Slice From 9871d4772df023ee96fb3303acb7911bd57c8504 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:03:35 -0400 Subject: [PATCH 14/23] =?UTF-8?q?perf(worker):=20value=20cache=20borrows?= =?UTF-8?q?=20the=20write-once=20event's=20instances=20=E2=80=94=20three?= =?UTF-8?q?=20clones=20per=20OnDataChange=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MxAccessValueCache.Set deep-copied the Value (recursive for an MxArray), the SourceTimestamp, and the Statuses RepeatedField (container plus every MxStatusProxy) on every OnDataChange. The aliasing audit found all three removable: the sink enqueues the event first — which stamps WorkerSequence/WorkerTimestamp inside the queue lock — and only then runs the postPublish hook that reaches Set, so the event is write-once by then and the queue's ownership invariant forbids later mutation. The producer never reuses instances (fresh MxEvent per mapper call, fresh MxValue per convert), and the alias already existed on the read side: MxAccessSession.SucceededRead puts the cache's own Value/SourceTimestamp/status references on every BulkReadResult, which the worker only serializes onto the IPC pipe. Set and CachedValue now carry the ownership contract: the cache holds borrowed references into an enqueued, write-once MxEvent; consumers may read and serialize, never mutate. Mutation would corrupt the still-queued event AND invalidate QueuedEvent.Size — the enqueue-time memoized serialized size the byte-budgeted Drain charges — so a grown message could overshoot the negotiated frame max and fault the session with MessageTooLarge. MxAccessEventQueue's class remark, which claimed the cache keeps an independent snapshot, is corrected to point at the borrow. MxAccessWriteCompletionCache.Record keeps its parallel statuses.Clone() deliberately, with a cross-reference explaining why: it takes a bare RepeatedField whose provenance its signature cannot constrain, and it is on the command-rate write path, not the streaming hot path. Tests: Set_StoresIndependentSnapshot_UnaffectedByLaterEventMutation codified the invariant being reversed, so it is replaced by Set_BorrowsTheEventsOwnInstances_ByOwnershipContract (Assert.Same on Value, SourceTimestamp, and the status row). Adds the missing cached-read-path test to MxAccessCommandExecutorTests — nothing in the worker exercised was_cached == true end to end — asserting the cache hit, reference identity out to the BulkReadResult, and that no COM call is made for the read. Not built or tested here: these are net48/x86 worker files that cannot compile on the macOS tree. Verification is deferred to the windev gate. --- .../MxAccess/MxAccessCommandExecutorTests.cs | 101 ++++++++++++++++++ .../MxAccess/MxAccessValueCacheTests.cs | 41 ++++--- .../MxAccess/MxAccessEventQueue.cs | 7 +- .../MxAccess/MxAccessValueCache.cs | 90 +++++++++++----- .../MxAccess/MxAccessWriteCompletionCache.cs | 13 +++ 5 files changed, 206 insertions(+), 46 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessCommandExecutorTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessCommandExecutorTests.cs index a8ac61d..7533db1 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessCommandExecutorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessCommandExecutorTests.cs @@ -669,6 +669,107 @@ public sealed class MxAccessCommandExecutorTests Assert.Contains("RemoveItem:90:900", fakeComObject.OperationNames); } + /// + /// Verifies ReadBulk's cached fast path end to end — the + /// was_cached = true half of the command, which nothing else in + /// the worker suite exercises. With the tag already added AND advised + /// and a value in the per-session cache, the executor answers from the + /// cache: the result is successful and flagged cached, and no COM call + /// is made for the read at all, so the subscription the caller did not + /// create is left exactly as it was. + /// + /// It also pins the borrow contract through the whole read path: the + /// Value, SourceTimestamp, and status row on the + /// BulkReadResult are the cached event's own instances, not + /// copies (see ). The worker only + /// serializes them onto the IPC pipe, so the alias never escapes the + /// process. + /// + /// Driven through directly rather + /// than because seeding the cache needs + /// a handle on it: only shares the + /// sink's cache for the production MxAccessBaseEventSink, which + /// casts the COM object to LMXProxyServerClass and so cannot take + /// a fake, while CreateForTesting accepts the cache directly. The + /// cached path neither waits nor pumps, so it needs no STA. + /// + [Fact] + public void Execute_ReadBulk_WhenTagIsAdvisedAndCached_ServesTheCachedInstancesWithoutTouchingTheSubscription() + { + FakeMxAccessComObject fakeComObject = new( + registerHandle: 92, + addItemHandle: 920); + MxAccessValueCache valueCache = new(); + using MxAccessSession session = MxAccessSession.CreateForTesting( + mxAccessServer: fakeComObject, + eventSink: new NoopEventSink(), + valueCache: valueCache); + MxAccessCommandExecutor executor = new( + session, + new ZB.MOM.WW.MxGateway.Worker.Conversion.VariantConverter()); + + // Registry half of the fast path: the tag must resolve to a live item + // handle on this server AND carry an advice, or TryGetCachedReadFor + // falls through to the AddItem/Advise snapshot lifecycle. + MxCommandReply registerReply = executor.Execute( + CreateRegisterCommand("register-before-cached-read", "client-a")); + MxCommandReply addItemReply = executor.Execute( + CreateAddItemCommand("add-before-cached-read", 92, "Galaxy.Tag.Value")); + MxCommandReply adviseReply = executor.Execute( + CreateAdviseCommand("advise-before-cached-read", 92, 920)); + Assert.Equal(ProtocolStatusCode.Ok, registerReply.ProtocolStatus.Code); + Assert.Equal(ProtocolStatusCode.Ok, addItemReply.ProtocolStatus.Code); + Assert.Equal(ProtocolStatusCode.Ok, adviseReply.ProtocolStatus.Code); + + // Cache half: stand in for the event sink's post-publish hook, which + // records the event it just enqueued. + MxEvent cachedEvent = new() + { + Family = MxEventFamily.OnDataChange, + ServerHandle = 92, + ItemHandle = 920, + Quality = 192, + SourceTimestamp = Timestamp.FromDateTime(new(2026, 8, 15, 10, 0, 0, DateTimeKind.Utc)), + Value = new MxValue + { + DataType = MxDataType.Integer, + VariantType = "VT_I4", + Int32Value = 7788, + }, + OnDataChange = new OnDataChangeEvent(), + }; + cachedEvent.Statuses.Add(new MxStatusProxy { Category = MxStatusCategory.Ok }); + valueCache.Set(92, 920, cachedEvent); + + MxCommandReply reply = executor.Execute(CreateReadBulkCommand( + "read-bulk-cached", + serverHandle: 92, + tagAddresses: new[] { "Galaxy.Tag.Value" }, + timeoutMs: 80)); + + Assert.Equal(ProtocolStatusCode.Ok, reply.ProtocolStatus.Code); + Assert.Equal(MxCommandKind.ReadBulk, reply.Kind); + BulkReadResult result = Assert.Single(reply.ReadBulk.Results); + Assert.True(result.WasSuccessful); + Assert.True(result.WasCached); + Assert.Equal("Galaxy.Tag.Value", result.TagAddress); + Assert.Equal(920, result.ItemHandle); + Assert.Equal(192, result.Quality); + Assert.Equal(7788, result.Value.Int32Value); + + // Borrowed, not copied — all the way from the event handed to + // MxAccessValueCache.Set out to the reply the worker serializes. + Assert.Same(cachedEvent.Value, result.Value); + Assert.Same(cachedEvent.SourceTimestamp, result.SourceTimestamp); + Assert.Same(cachedEvent.Statuses[0], Assert.Single(result.Statuses)); + + // No second AddItem, and above all no UnAdvise/RemoveItem: only the + // three setup calls ever reached MXAccess. + Assert.Equal( + new[] { "Register:client-a", "AddItem:92:Galaxy.Tag.Value", "Advise:92:920" }, + fakeComObject.OperationNames); + } + /// Verifies that ReadBulk with no payload returns an invalid request error. /// A task that represents the asynchronous operation. [Fact] diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessValueCacheTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessValueCacheTests.cs index bad8580..2b9151e 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessValueCacheTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/MxAccessValueCacheTests.cs @@ -48,14 +48,28 @@ public sealed class MxAccessValueCacheTests } /// - /// Verifies that Set stores an independent deep-copied snapshot: mutating - /// the source event's protobuf sub-messages after caching does not alter - /// the cached value. WRK-11 stopped the event sink cloning before enqueue, - /// so the same MxEvent instance now flows to the outbound queue; the cache - /// must own its own copy so the two never share mutable state. + /// Pins the ownership contract: Set borrows the event's own + /// protobuf sub-messages instead of deep-copying them, so TryGet + /// hands back the very Value, SourceTimestamp, and + /// MxStatusProxy instances the caller passed in. + /// + /// This is safe only because the event is write-once by the time + /// Set runs: the sink enqueues it (which stamps the worker + /// sequence and timestamp) and only then post-publishes it here, and + /// 's ownership invariant forbids + /// mutating an enqueued event. This test therefore asserts reference + /// identity and deliberately does NOT mutate the event afterwards — + /// doing so is exactly what the contract forbids, and it would also + /// invalidate the serialized size the queue memoized at enqueue. + /// + /// It replaced a test asserting the opposite (an independent deep-copied + /// snapshot). The three clones that test pinned — the MxValue, the + /// Timestamp, and the RepeatedField plus every status row in it — ran on + /// every OnDataChange and bought nothing: the read path already aliased + /// the cache's instances into every BulkReadResult. /// [Fact] - public void Set_StoresIndependentSnapshot_UnaffectedByLaterEventMutation() + public void Set_BorrowsTheEventsOwnInstances_ByOwnershipContract() { MxAccessValueCache cache = new(); Timestamp sourceTimestamp = Timestamp.FromDateTime(new(2026, 5, 19, 9, 0, 0, DateTimeKind.Utc)); @@ -63,19 +77,12 @@ public sealed class MxAccessValueCacheTests cache.Set(7, 21, mxEvent); - // Mutate the event in place after it was cached — as if it kept flowing - // through the (unrelated) outbound path. None of this must reach the cache. - mxEvent.Value.Int32Value = 999; - mxEvent.Quality = 0; - mxEvent.SourceTimestamp = Timestamp.FromDateTime(new(2030, 1, 1, 0, 0, 0, DateTimeKind.Utc)); - mxEvent.Statuses[0].Category = MxStatusCategory.SecurityError; - Assert.True(cache.TryGet(7, 21, out MxAccessValueCache.CachedValue cached)); - Assert.Equal(100, cached.Value.Int32Value); + Assert.Same(mxEvent.Value, cached.Value); + Assert.Same(mxEvent.SourceTimestamp, cached.SourceTimestamp); + Assert.Same(mxEvent.Statuses, cached.Statuses); + Assert.Same(mxEvent.Statuses[0], Assert.Single(cached.Statuses)); Assert.Equal(192, cached.Quality); - Assert.Equal(sourceTimestamp, cached.SourceTimestamp); - Assert.Single(cached.Statuses); - Assert.Equal(MxStatusCategory.Ok, cached.Statuses[0].Category); } /// Verifies that TryGet returns false for unknown handles. diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs index dd07094..eb4c471 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs @@ -20,8 +20,11 @@ namespace ZB.MOM.WW.MxGateway.Worker.MxAccess; /// that it does not retain, reuse, or mutate after the /// call returns. All production callers (MxAccessBaseEventSink, /// MxAccessAlarmEventSink, AlarmCommandHandler) build a new event per -/// Enqueue via the mapper and satisfy this; the value cache stores its own -/// independent snapshot (see ). +/// Enqueue via the mapper and satisfy this. "Does not mutate" is the load- +/// bearing half for the post-publish value cache, which deliberately borrows +/// the enqueued event's own value/timestamp/status instances rather than +/// copying them (see ) — it reads and +/// serializes them, and this invariant is what keeps that safe. /// /// The byte-budgeted relies on the same invariant: each /// event's serialized size is measured once at enqueue and stored beside it, which is diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs index b6ecd3a..3841d5f 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessValueCache.cs @@ -57,7 +57,12 @@ public sealed class MxAccessValueCache /// Records a fresh OnDataChange payload for the given handle pair. /// MXAccess server handle. /// MXAccess item handle. - /// The protobuf MxEvent created by the event mapper. + /// + /// The protobuf MxEvent created by the event mapper, already handed to + /// the outbound queue. The cache borrows this event's + /// Value/SourceTimestamp/Statuses instances rather + /// than copying them — see the ownership contract in the method body. + /// public void Set( int serverHandle, int itemHandle, @@ -68,20 +73,39 @@ public sealed class MxAccessValueCache throw new ArgumentNullException(nameof(mxEvent)); } - // WRK-11: the event sink no longer clones before enqueue, so the passed - // mxEvent is the very instance handed to the outbound queue. Deep-copy - // the value/timestamp/statuses payload we retain here so the cache's - // snapshot stays independent of the enqueued (and later serialized) - // event — the two must never share mutable protobuf sub-messages. - // Value is always set for OnDataChange; SourceTimestamp may be unset when - // the source timestamp could not be parsed, so both are cloned only when - // present. The null-forgiving result matches CachedValue's non-null- - // annotated parameters, which already accepted a runtime-null value or - // timestamp before WRK-11 (the ternary keeps the compiler's null-state - // from poisoning to maybe-null, which a plain null check would do). - MxValue cachedValue = mxEvent.Value is null ? null! : mxEvent.Value.Clone(); - Timestamp cachedTimestamp = mxEvent.SourceTimestamp is null ? null! : mxEvent.SourceTimestamp.Clone(); - + // Ownership contract: the cache BORROWS, it does not copy. CachedValue + // retains the event's own Value, SourceTimestamp, and Statuses + // instances. + // + // Sound because the event is write-once by the time this runs. + // MxAccessBaseEventSink.EnqueueEvent calls eventQueue.Enqueue first — + // which stamps WorkerSequence/WorkerTimestamp inside the queue lock — + // and only then runs the postPublish hook that lands here; the queue's + // ownership invariant (see MxAccessEventQueue's class remarks) forbids + // mutating an event after it is enqueued. The producer side never + // reuses instances either: the mapper builds a fresh MxEvent, and the + // VariantConverter a fresh MxValue, per COM callback. The three deep + // copies this replaced (the MxValue — recursive for an MxArray — the + // Timestamp, and the RepeatedField container plus every MxStatusProxy + // in it) therefore bought nothing but garbage on the worker's hottest + // path. + // + // Consumers of TryGet / TryWaitForUpdate may read and serialize what + // they get back; they must never mutate it. ReadBulk already depends on + // that: MxAccessSession.SucceededRead puts these very instances on the + // BulkReadResult it returns, which the worker only serializes onto the + // IPC pipe — worker↔gateway is a process boundary, so no gateway-side + // consumer can alias them. Mutating one would corrupt the event still + // queued for the outbound stream AND invalidate QueuedEvent.Size, the + // serialized size memoized at enqueue that the byte-budgeted Drain + // charges against its budget: a message grown after enqueue could + // overshoot the negotiated frame max and fault the session with + // MessageTooLarge (WorkerPipeSession.FaultOnOversizedEventAsync). + // + // Value is always set for OnDataChange; SourceTimestamp can be null when + // the source timestamp could not be parsed. CachedValue's parameters are + // annotated non-null but have always accepted a runtime null for both, + // and the read side null-checks accordingly. long key = CreateItemKey(serverHandle, itemHandle); lock (syncRoot) { @@ -91,10 +115,10 @@ public sealed class MxAccessValueCache entries[key] = new CachedValue( nextVersion, - cachedValue, + mxEvent.Value, mxEvent.Quality, - cachedTimestamp, - mxEvent.Statuses.Clone()); + mxEvent.SourceTimestamp, + mxEvent.Statuses); } // Signaled outside the lock: the waiter re-takes syncRoot (through @@ -228,23 +252,35 @@ public sealed class MxAccessValueCache } /// - /// Snapshot of the most recent OnDataChange payload for a handle pair. + /// The most recent OnDataChange payload for a handle pair. /// increments by one on every /// call so the bulk read executor can detect "a new value arrived /// since I started waiting". /// /// - /// Plain readonly struct (not a record) so this compiles under the - /// worker's net48 target, which lacks IsExternalInit. + /// + /// Borrowed references, not copies. , + /// , and are the + /// very instances hanging off the MxEvent that was enqueued for the + /// outbound stream — carries the full ownership + /// contract. Read them and serialize them; never mutate them and + /// never hand them to something that will. That event is write-once + /// from the moment it is enqueued, and its serialized size is + /// memoized at that point for the byte-budgeted drain. + /// + /// + /// Plain readonly struct (not a record) so this compiles under the + /// worker's net48 target, which lacks IsExternalInit. + /// /// public readonly struct CachedValue { /// Initializes a new cached value snapshot. /// Version counter incremented on each update. - /// The MXAccess value. + /// The MXAccess value, borrowed from the enqueued event. /// The MXAccess quality code. - /// The source timestamp of the value. - /// The MXAccess status codes. + /// The source timestamp of the value, borrowed from the enqueued event. + /// The MXAccess status codes, borrowed from the enqueued event. public CachedValue( ulong version, MxValue value, @@ -262,16 +298,16 @@ public sealed class MxAccessValueCache /// Monotonic per-handle version counter. public ulong Version { get; } - /// The cached MxValue payload. + /// The OnDataChange event's own MxValue payload. Read-only to consumers. public MxValue Value { get; } /// Quality code from the OnDataChange event. public int Quality { get; } - /// Source timestamp from the OnDataChange event. + /// The OnDataChange event's own source timestamp. Read-only to consumers. public Timestamp SourceTimestamp { get; } - /// MxStatusProxy entries from the OnDataChange event. + /// The OnDataChange event's own MxStatusProxy collection. Read-only to consumers. public RepeatedField Statuses { get; } } } diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessWriteCompletionCache.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessWriteCompletionCache.cs index 78a4114..f846cd2 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessWriteCompletionCache.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessWriteCompletionCache.cs @@ -73,6 +73,19 @@ public sealed class MxAccessWriteCompletionCache ulong version = entries.TryGetValue(key, out CompletionEntry existing) ? existing.Version + 1 : 1UL; + + // Still a defensive copy, deliberately — this is NOT an oversight + // left behind by the borrow that MxAccessValueCache.Set adopted (see + // the ownership contract there). The value cache is handed the whole + // enqueued MxEvent, so the queue's write-once ownership invariant + // covers everything it retains. This method is handed a bare + // RepeatedField whose provenance its signature cannot constrain: + // the production sink does pass an enqueued event's Statuses, but + // callers that build and keep their own rows are equally valid + // against this API, and a borrowed alias would then let a later + // mutation rewrite an already-recorded completion. The write path is + // command-rate, not the per-OnDataChange streaming hot path, so the + // clone costs nothing worth reclaiming. entries[key] = new CompletionEntry(version, statuses.Clone()); } From aac79579ab57b8e3f2c423372451cd7095e012d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:05:57 -0400 Subject: [PATCH 15/23] perf(worker): control-frame completions resolve at the class-transition flush, not after the event batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-class writer already got control bytes out ahead of a queued event backlog, but a frame counts as delivered only once flushed, and the drain deferred its single FlushAsync — and every TrySetResult — to the end of the pass. A heartbeat, command reply, fault, or shutdown ack was therefore written first and completed last, behind up to a full 128-frame event batch. The drain now records each frame's priority class on PendingFrame and flushes at every control-to-event boundary, completing and clearing the written set there. Cost stays bounded: a pure-event pass still pays exactly one flush, a run of control frames still pays one for the run, and only a pass that mixes both classes pays a second — never one flush per control frame, the syscall-per-heartbeat cost WRK-12 removed. A boundary flush that itself fails is a new failure window and is handled like the end-of-pass flush failure, additionally failing the event frame the drain had already claimed off its queue and every frame still queued. Frames a boundary flush completed leave the written set, so a later failure in the same pass can no longer reach back and fail an already-delivered control frame. The awaited task of a caller that lost the write-lock race is still bounded by the winning drainer's pass — that enqueue-then-contend parking is unchanged and now documented on WriteAsync and in docs/WorkerFrameProtocol.md. --- docs/WorkerFrameProtocol.md | 57 +++- .../Ipc/WorkerFrameProtocolTests.cs | 272 +++++++++++++++++- .../Ipc/WorkerFrameWriter.cs | 103 +++++-- 3 files changed, 401 insertions(+), 31 deletions(-) diff --git a/docs/WorkerFrameProtocol.md b/docs/WorkerFrameProtocol.md index 36fe955..3bab2a3 100644 --- a/docs/WorkerFrameProtocol.md +++ b/docs/WorkerFrameProtocol.md @@ -88,7 +88,9 @@ priority order. A caller enqueues its frame into the control or event queue under a lock, then contends for a single write lock; whichever caller wins drains every frame queued at that moment, control frames first and each class in FIFO order, so a command reply, fault, heartbeat, or shutdown -acknowledgement is never delayed behind a backlog of queued events. Priority +acknowledgement is never delayed behind a backlog of queued events — neither +in the bytes written nor in the flush that marks them delivered (see the +class-boundary flush under flush coalescing below). Priority only reorders *which frame writes next* — it does not affect the sequence value a frame receives (see below), so a caller cannot infer priority class from the wire sequence. @@ -117,13 +119,36 @@ Two failure shapes are distinguished during a drain pass: and every frame still queued, then stops draining entirely so no caller waits forever on a stream that will not recover. -Flushes are coalesced across a drained batch: each frame in the batch is -written to the stream without an individual flush, then one `FlushAsync` -runs after the whole batch, and only then does every successfully-written -frame's completion resolve — so a caller's `WriteAsync` still does not -complete until its bytes are both written *and* flushed, but a batch that -happened to contain several queued frames pays one flush instead of one per -frame. Note the ordering this implies at the peer: the frames reach the pipe +Flushes are coalesced across a *run of same-class frames* inside a drain +pass: each frame in the run is written to the stream without an individual +flush, then one `FlushAsync` runs — at the end of the pass, and additionally +at every control-to-event boundary — and only then does every +successfully-written frame of that run resolve its completion. A caller's +`WriteAsync` therefore still does not complete until its bytes are both +written *and* flushed; what changed is *when* that moment arrives +for a control frame that a pass writes ahead of queued events. It used to be +the end of the pass, so a heartbeat, command reply, fault, or shutdown +acknowledgement was written first but only counted as delivered after up to a +full event batch had been written and flushed behind it. The boundary flush +closes the control run out before the events are written, so the priority +class governs the frame's delivery point and not just its byte order. The +cost stays bounded: a pure-event pass — the event hot path — still pays +exactly one flush however many frames drain together, a run of control +frames still pays one for the whole run (never one per heartbeat, the +syscall-per-frame cost the coalescing removed), and only a pass that actually +mixes both classes pays a second. + +One consequence of the boundary flush is worth stating: a control frame whose +run has already been flushed and completed is out of the drain's +written-but-unflushed set, so a *later* failure in the same pass — a broken +write, or a failed end-of-pass flush — no longer reaches back and fails it. +That is the honest outcome: its bytes were flushed, so it was delivered. A +failure of the boundary flush itself is treated exactly like a failed +end-of-pass flush, and additionally fails the event frame the drain had +already claimed off its queue (nothing else would ever complete it) along +with every frame still queued. + +Note the ordering all of this implies at the peer: the frames reach the pipe before the flush that follows them, so the gateway can read a whole batch while the writer has not yet flushed it. Anything observing the flush itself (a test counting flushes, for instance) must wait for the flush, not infer it @@ -134,12 +159,22 @@ drains them together, so a burst of N events costs one flush rather than N — the coalescing the batch machinery was built for now engages on the event hot path, not only when independent producers happen to queue behind a blocked write. Intra-batch order is preserved (FIFO enqueue under one lock), and a -concurrently queued control frame is still drained ahead of the batch. A -per-frame rejection inside a batch (for example one oversized event) surfaces -from the batch's awaited completions as that frame's +concurrently queued control frame is still drained — and now flushed and +completed — ahead of the batch's remaining events, which is why a batch a +control frame cuts into pays one extra flush while an uninterrupted batch +still pays exactly one. A per-frame rejection inside a batch (for example one +oversized event) surfaces from the batch's awaited completions as that frame's `WorkerFrameProtocolException`; the remaining completions are still observed so none faults unobserved. +The completion is the frame's delivery point, not necessarily the instant its +caller returns. A caller that loses the race for the write lock only observes +its own completion after the winning drainer releases the lock, so its return +remains bounded by that drain pass even though its control frame was flushed +and completed at the class boundary inside it. The boundary flush is what +makes the delivery point honest; unparking a lock-race loser from the winner's +pass would be a separate change to the enqueue-then-contend shape. + Cancellation of a `WriteAsync`/`WriteBatchAsync` call that is still waiting for the write lock when its token fires tombstones the queued frame: the cancelled caller marks its frame under `_gate`, and the draining lock-holder's diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs index bdeea32..a58fed6 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerFrameProtocolTests.cs @@ -377,6 +377,14 @@ public sealed class WorkerFrameProtocolTests /// Verifies the writer coalesces the flush across a batch of frames drained together: four frames /// queued behind an in-progress write drain in a single pass and share one FlushAsync, not four. /// Every frame still reaches the wire intact. + /// + /// The burst is all-event on purpose. The control-frame completion decoupling made the drain flush + /// at each control-to-event boundary, so a pass that mixes classes legitimately pays one flush per + /// class run; the property + /// worth pinning is that a run of same-class frames — the event hot path — still costs exactly one + /// flush no matter how many frames drain together. The mixed shape has its own count assertion in + /// . + /// /// /// A task that represents the asynchronous operation. [Fact] @@ -387,7 +395,7 @@ public sealed class WorkerFrameProtocolTests WorkerFrameWriter writer = new(stream, options); // A blocked first write occupies the writer and holds the lock while more frames queue behind it. - Task firstWrite = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + Task firstWrite = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); await AwaitWithTimeoutAsync(stream.FirstWriteStarted); Task eventWrite1 = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); @@ -410,6 +418,179 @@ public sealed class WorkerFrameProtocolTests } } + /// + /// Control-frame completion decoupling. A control frame's delivery point must not be charged for + /// the event backlog behind it. The priority scheduler already wrote control bytes first, + /// but a frame counts as + /// delivered only once flushed, and the pass deferred its single flush — and every completion — + /// until after the events. The drain now flushes at the control-to-event boundary: with two control + /// frames written and the first event write blocked inside the stream, the flush that closes out the + /// control run has already run, so the heartbeat or reply is on the pipe rather than waiting behind + /// the batch. Exactly two flushes for the pass — one per class run, not one per control frame. + /// + /// The assertion is on the flush, not on the queued callers' returned tasks, because those tasks are + /// still gated by the write lock they lost to the drainer (see the latency contract on + /// WorkerFrameWriter.WriteAsync): the completion resolves at the boundary flush, but a + /// lock-race loser observes it only once the drainer releases the lock. + /// + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task DrainPass_MixedClasses_FlushesControlRunBeforeWritingEvents() + { + WorkerFrameProtocolOptions options = CreateOptions(); + // Frame 1 (control) gates the pass open; frame 3 is the pass's first event write, which blocks + // so the boundary flush can be observed with the event batch still unwritten. + using GatedWriteStream stream = new(secondGateWriteIndex: 3); + WorkerFrameWriter writer = new(stream, options); + + Task firstControl = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + await AwaitWithTimeoutAsync(stream.FirstWriteStarted); + + Task secondControl = writer.WriteAsync(CreateShutdownAckEnvelope(), WorkerFrameWritePriority.Control); + Task eventWrite1 = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + Task eventWrite2 = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + await Task.Delay(50); + + stream.ReleaseFirstWrite(); + + // The drain writes both control frames and is now blocked on the first event write. + await AwaitWithTimeoutAsync(stream.SecondGateWriteStarted); + + // The control run was flushed before the event batch was written — not after it. + Assert.Equal(1, stream.FlushCount); + Assert.False(eventWrite1.IsCompleted); + Assert.False(eventWrite2.IsCompleted); + + stream.ReleaseSecondGateWrite(); + await AwaitWithTimeoutAsync( + Task.WhenAll(firstControl, secondControl, eventWrite1, eventWrite2)); + + // One flush per class run: the control run, then the event run at the end of the pass. + Assert.Equal(2, stream.FlushCount); + + stream.Position = 0; + WorkerFrameReader reader = new(stream, options); + WorkerEnvelope frame1 = await reader.ReadAsync(); + WorkerEnvelope frame2 = await reader.ReadAsync(); + WorkerEnvelope frame3 = await reader.ReadAsync(); + WorkerEnvelope frame4 = await reader.ReadAsync(); + Assert.Equal(WorkerEnvelope.BodyOneofCase.GatewayHello, frame1.BodyCase); + Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerShutdownAck, frame2.BodyCase); + Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerEvent, frame3.BodyCase); + Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerEvent, frame4.BodyCase); + Assert.Equal(stream.Length, stream.Position); + } + + /// + /// Control-frame completion decoupling, the inverse guard. An event frame's completion boundary is + /// still the end-of-pass flush: a pure-event pass takes no boundary flush, so with two events + /// already written and the third + /// blocked mid-write, nothing has been flushed and no event can have been reported delivered. Only + /// a class transition may move a flush earlier — a plain event backlog may not. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task DrainPass_PureEventRun_DoesNotFlushBeforeThePassEnds() + { + WorkerFrameProtocolOptions options = CreateOptions(); + using GatedWriteStream stream = new(secondGateWriteIndex: 3); + WorkerFrameWriter writer = new(stream, options); + + Task eventWrite1 = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + await AwaitWithTimeoutAsync(stream.FirstWriteStarted); + + Task eventWrite2 = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + Task eventWrite3 = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + await Task.Delay(50); + + stream.ReleaseFirstWrite(); + await AwaitWithTimeoutAsync(stream.SecondGateWriteStarted); + + // Two event frames written, none flushed: no event frame's delivery point has been reached. + Assert.Equal(0, stream.FlushCount); + Assert.False(eventWrite1.IsCompleted); + Assert.False(eventWrite2.IsCompleted); + + stream.ReleaseSecondGateWrite(); + await AwaitWithTimeoutAsync(Task.WhenAll(eventWrite1, eventWrite2, eventWrite3)); + Assert.Equal(1, stream.FlushCount); + } + + /// + /// Control-frame completion decoupling. The boundary flush is charged per class run, not per + /// control frame: a pass carrying nothing but control frames still pays exactly one flush. Flushing + /// after every control frame + /// would reinstate the syscall-per-heartbeat cost WRK-12 removed. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task DrainPass_PureControlRun_FlushesOnce() + { + WorkerFrameProtocolOptions options = CreateOptions(); + using GatedWriteStream stream = new(); + WorkerFrameWriter writer = new(stream, options); + + Task firstControl = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + await AwaitWithTimeoutAsync(stream.FirstWriteStarted); + + Task secondControl = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + Task thirdControl = writer.WriteAsync(CreateShutdownAckEnvelope(), WorkerFrameWritePriority.Control); + await Task.Delay(50); + + stream.ReleaseFirstWrite(); + await AwaitWithTimeoutAsync(Task.WhenAll(firstControl, secondControl, thirdControl)); + + Assert.Equal(1, stream.FlushCount); + } + + /// + /// Control-frame completion decoupling, the new failure window. The boundary flush is a new place + /// the pipe can break with frames written but not yet delivered, so it must fail exactly like the + /// end-of-pass flush: every written control + /// frame fails, and so do the event frame the drain had already claimed off its queue (nothing else + /// would ever complete it) and every frame still queued, so no caller waits forever on a stream that + /// will not recover. The event bytes never reach the wire — the drain stops at the fault. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task DrainPass_WhenBoundaryFlushFails_FailsWrittenClaimedAndQueuedFrames() + { + const string faultMessage = "boundary flush failed"; + WorkerFrameProtocolOptions options = CreateOptions(); + using FlushFaultingGatedStream stream = new(faultMessage); + WorkerFrameWriter writer = new(stream, options); + + Task firstControl = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control); + await AwaitWithTimeoutAsync(stream.FirstWriteStarted); + + Task secondControl = writer.WriteAsync(CreateShutdownAckEnvelope(), WorkerFrameWritePriority.Control); + Task claimedEvent = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + Task queuedEvent = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event); + await Task.Delay(50); + + // The drain writes both control frames, claims the first event, and faults on the boundary flush. + stream.ReleaseFirstWrite(); + + // AwaitWithTimeoutAsync turns a frame nobody ever completes into a TimeoutException — a failed + // assertion rather than a hung test run. + foreach (Task write in new[] { firstControl, secondControl, claimedEvent, queuedEvent }) + { + IOException failure = await Assert.ThrowsAsync( + async () => await AwaitWithTimeoutAsync(write)); + Assert.Equal(faultMessage, failure.Message); + } + + // Only the control frames reached the wire; the claimed event was never written. + stream.Position = 0; + WorkerFrameReader reader = new(stream, options); + WorkerEnvelope frame1 = await reader.ReadAsync(); + WorkerEnvelope frame2 = await reader.ReadAsync(); + Assert.Equal(WorkerEnvelope.BodyOneofCase.GatewayHello, frame1.BodyCase); + Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerShutdownAck, frame2.BodyCase); + Assert.Equal(stream.Length, stream.Position); + } + /// /// Verifies a per-frame rejection does not burn a sequence number (WRK-23). The sequence is a /// diagnostic counter, so a gap breaks nothing functionally — but an operator correlating a pipe @@ -877,24 +1058,108 @@ public sealed class WorkerFrameProtocolTests } // A MemoryStream whose first WriteAsync blocks until released, so a test can queue additional frames - // behind an in-progress write and observe the writer's priority ordering. + // behind an in-progress write and observe the writer's priority ordering. A second, optional gate on + // a chosen write index lets a test stop a drain pass mid-flight — at a class boundary, say — and + // sample what the writer has already flushed while the rest of the pass is still unwritten. private sealed class GatedWriteStream : MemoryStream { private readonly SemaphoreSlim _release = new SemaphoreSlim(0); + private readonly SemaphoreSlim _secondGateRelease = new SemaphoreSlim(0); private readonly TaskCompletionSource _firstWriteStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _secondGateWriteStarted = + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly int _secondGateWriteIndex; private int _writeCount; private int _flushCount; + /// Initializes a new instance of the GatedWriteStream class. + /// + /// One-based index of a later write to block as well, or 0 (the default) to gate only the first + /// write. Write indexes start at 1, so 0 never matches. + /// + public GatedWriteStream(int secondGateWriteIndex = 0) + { + _secondGateWriteIndex = secondGateWriteIndex; + } + /// Gets a task that completes once the first call has started blocking. public Task FirstWriteStarted => _firstWriteStarted.Task; + /// Gets a task that completes once the second gated call has started blocking. + public Task SecondGateWriteStarted => _secondGateWriteStarted.Task; + /// Gets the number of calls observed so far. public int FlushCount => Volatile.Read(ref _flushCount); /// Releases the first blocked write so it can complete. public void ReleaseFirstWrite() => _release.Release(); + /// Releases the second gated write so it can complete. + public void ReleaseSecondGateWrite() => _secondGateRelease.Release(); + + /// + public override async Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + int writeIndex = Interlocked.Increment(ref _writeCount); + if (writeIndex == 1) + { + _firstWriteStarted.TrySetResult(true); + await _release.WaitAsync(cancellationToken); + } + else if (writeIndex == _secondGateWriteIndex) + { + _secondGateWriteStarted.TrySetResult(true); + await _secondGateRelease.WaitAsync(cancellationToken); + } + + await base.WriteAsync(buffer, offset, count, cancellationToken); + } + + /// + public override Task FlushAsync(CancellationToken cancellationToken) + { + Interlocked.Increment(ref _flushCount); + return base.FlushAsync(cancellationToken); + } + + /// + protected override void Dispose(bool disposing) + { + if (disposing) + { + _release.Dispose(); + _secondGateRelease.Dispose(); + } + + base.Dispose(disposing); + } + } + + // A MemoryStream whose first write blocks until released and whose every FlushAsync throws, so a test + // can fault the class-boundary flush with control frames already written and an event frame already + // claimed off its queue. + private sealed class FlushFaultingGatedStream : MemoryStream + { + private readonly SemaphoreSlim _release = new SemaphoreSlim(0); + private readonly TaskCompletionSource _firstWriteStarted = + new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly string _faultMessage; + private int _writeCount; + + /// Initializes a new instance of the FlushFaultingGatedStream class. + /// Message carried by the every flush throws. + public FlushFaultingGatedStream(string faultMessage) + { + _faultMessage = faultMessage; + } + + /// Gets a task that completes once the first call has started blocking. + public Task FirstWriteStarted => _firstWriteStarted.Task; + + /// Releases the first blocked write so it can complete. + public void ReleaseFirstWrite() => _release.Release(); + /// public override async Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { @@ -910,8 +1175,7 @@ public sealed class WorkerFrameProtocolTests /// public override Task FlushAsync(CancellationToken cancellationToken) { - Interlocked.Increment(ref _flushCount); - return base.FlushAsync(cancellationToken); + return Task.FromException(new IOException(_faultMessage)); } /// diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs index cfb20e7..61db4c7 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs @@ -14,9 +14,11 @@ namespace ZB.MOM.WW.MxGateway.Worker.Ipc; /// Writes worker frames to a stream with length-prefixed protobuf serialization. Callers enqueue a /// frame at a and then contend for a single write lock; whoever /// holds the lock drains every queued frame, control frames first, so a reply, fault, or heartbeat is -/// never delayed behind an event backlog. The envelope Sequence is stamped by the -/// draining lock-holder at the moment of writing, so the on-wire order and the stamped sequence always -/// agree even under concurrent callers and priority reordering. +/// never delayed behind an event backlog — neither in the bytes it writes nor in the flush that +/// delivers them, because the drain flushes at every control-to-event boundary rather than only at the +/// end of the pass. The envelope Sequence is stamped by the draining lock-holder at the moment +/// of writing, so the on-wire order and the stamped sequence always agree even under concurrent callers +/// and priority reordering. /// public sealed class WorkerFrameWriter { @@ -24,15 +26,25 @@ public sealed class WorkerFrameWriter { /// Initializes a new instance of the PendingFrame class. /// Worker envelope awaiting write. - public PendingFrame(WorkerEnvelope envelope) + /// Priority class the frame was queued at. + public PendingFrame(WorkerEnvelope envelope, WorkerFrameWritePriority priority) { Envelope = envelope; + IsControl = priority != WorkerFrameWritePriority.Event; Completion = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); } /// Gets the worker envelope awaiting write. public WorkerEnvelope Envelope { get; } + /// + /// Gets a value indicating whether this frame was queued as control-plane traffic. Recorded at + /// construction from the same expression that picks the queue, so the class the drain sees can + /// never disagree with the queue the frame sits in. The drain uses it to flush and complete + /// written control frames at the moment it turns to events (see ). + /// + public bool IsControl { get; } + /// Gets the completion source signaled once the frame has been written or has failed. public TaskCompletionSource Completion { get; } @@ -95,6 +107,15 @@ public sealed class WorkerFrameWriter /// the canceller behind the very write it is abandoning would defeat the point of cancellation. /// The abandoned frame's completion gets a fault-observing continuation so a write failure after /// the caller unwinds never raises an unobserved-task exception (NEXT-04). + /// + /// Latency contract: a control frame's bytes are written, flushed, and its completion + /// resolved before the events a drain pass writes after it — the delivery point of a heartbeat, + /// reply, fault, or shutdown ack is never charged for the event backlog behind it. The returned + /// task can still be later than that instant for a caller that lost the write-lock race: it only + /// observes its completion after the winning drainer releases the lock, so its own return remains + /// bounded by that pass. That parking is deliberate — the alternative is to race the lock wait + /// against the completion, which buys nothing for the frame's delivery. + /// /// public async Task WriteAsync( WorkerEnvelope envelope, @@ -106,7 +127,7 @@ public sealed class WorkerFrameWriter throw new ArgumentNullException(nameof(envelope)); } - PendingFrame frame = new PendingFrame(envelope); + PendingFrame frame = new PendingFrame(envelope, priority); lock (_gate) { if (priority == WorkerFrameWritePriority.Event) @@ -153,8 +174,12 @@ public sealed class WorkerFrameWriter /// rather than one per frame (WRK-25, realizing the WRK-12 coalescing on the path it was built /// for). Intra-batch order is preserved because the enqueue is atomic under _gate and each /// class queue is FIFO; the control-before-event guarantee still holds because any concurrently - /// queued control frame is drained ahead of this batch by . Every frame's - /// "written and flushed before completion" contract is unchanged. + /// queued control frame is drained ahead of this batch by , and — since + /// the control-frame completion decoupling — is also flushed and completed before this batch's + /// remaining events are written, so a batch in flight does not delay a control frame's delivery. + /// Every frame's "written and flushed before completion" contract is unchanged. An event batch that + /// a control frame cuts into therefore pays one extra flush; an uninterrupted batch still pays + /// exactly one. /// /// Envelopes to write, in order. /// Scheduling priority for the whole batch. @@ -189,7 +214,7 @@ public sealed class WorkerFrameWriter { WorkerEnvelope envelope = envelopes[index] ?? throw new ArgumentException("Batch envelopes must not contain null.", nameof(envelopes)); - frames[index] = new PendingFrame(envelope); + frames[index] = new PendingFrame(envelope, priority); } lock (_gate) @@ -296,14 +321,24 @@ public sealed class WorkerFrameWriter // The stream write itself is not cancellable: a frame is written atomically or fails, never left // half-written on the pipe because a caller gave up waiting. // - // Flushes are coalesced across the whole drained batch (WRK-12 / IPC-15): each frame is written to - // the stream but not flushed individually; a single FlushAsync runs after the batch, then every - // successfully-written frame is completed. A caller's Completion therefore still signals only after - // its bytes have been written AND flushed, so the "written and flushed" contract is unchanged — but - // a burst of N events now costs one flush syscall instead of N. + // Flushes are coalesced within a priority class rather than blindly across the whole pass (WRK-12 / + // IPC-15, narrowed by the control-frame completion decoupling): each frame is written to the stream + // but not flushed individually, and one FlushAsync runs at the end of the pass — plus one at each + // control-to-event boundary, which flushes and completes the control frames written so far before + // the event backlog behind them is written, instead of after it. Without that boundary flush the + // priority scheduler only got control *bytes* out early: their delivery point, and every waiting + // caller's completion, still sat behind up to a full event batch. + // + // A caller's Completion therefore still signals only after its bytes have been written AND flushed — + // the contract is unchanged, the moment it is reached simply stops being pinned to the end of the + // pass. Cost is bounded: a pure-event pass (the event hot path) still pays exactly one flush, a burst + // of control frames still pays one for the whole burst, and only a pass that actually mixes both + // classes pays a second — never one flush per control frame, which is the syscall-per-heartbeat cost + // WRK-12 removed. private async Task DrainQueuedFramesAsync() { List written = new List(); + bool writtenHoldsControl = false; while (true) { PendingFrame? frame = DequeueNext(); @@ -312,10 +347,40 @@ public sealed class WorkerFrameWriter break; } + if (writtenHoldsControl && !frame.IsControl) + { + // Class transition: the frames written so far include at least one control frame whose + // caller is waiting on delivery. Flush and complete them here rather than parking them + // behind the events this pass is about to write. Charged once per transition, not once + // per control frame. Event frames already in the list ride along — they too are written + // and now flushed, so completing them early is the same contract, earlier. + try + { + await _stream.FlushAsync(CancellationToken.None).ConfigureAwait(false); + } + catch (Exception exception) + { + // Same shape as the end-of-pass flush failure: the bytes reached the stream but the + // flush that guarantees delivery failed, so the pipe is broken. Fail the frame just + // claimed (it is out of its queue and nothing else will ever complete it), every + // written-but-unflushed frame, and everything still queued, then stop draining. + frame.Completion.TrySetException(exception); + FailFrames(written, exception); + FailAllQueued(exception); + return; + } + + // Completed frames leave the list, so a later failure in this pass cannot fail them. + CompleteFrames(written); + written.Clear(); + writtenHoldsControl = false; + } + try { await WriteFrameAsync(frame.Envelope).ConfigureAwait(false); written.Add(frame); + writtenHoldsControl |= frame.IsControl; } catch (WorkerFrameProtocolException exception) when (IsPerFrameRejection(exception)) { @@ -348,13 +413,19 @@ public sealed class WorkerFrameWriter catch (Exception exception) { // The batch reached the stream but the flush that guarantees delivery failed: the pipe is - // broken. Fail every frame in the batch (the queue was already drained) so no caller treats - // an unflushed write as delivered. + // broken. Fail every frame still in the batch (the queue was already drained) so no caller + // treats an unflushed write as delivered. Frames a boundary flush already completed are not + // in the list — their bytes were flushed, so this later failure does not reach back to them. FailFrames(written, exception); return; } - foreach (PendingFrame frame in written) + CompleteFrames(written); + } + + private static void CompleteFrames(List frames) + { + foreach (PendingFrame frame in frames) { frame.Completion.TrySetResult(true); } From e913dab5db69f4523f74a92a082e7fe9e9fb4ec1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:06:03 -0400 Subject: [PATCH 16/23] fix(worker): session-owned stream disposal unblocks and observes the net48 pipe read at teardown --- docs/MxAccessWorkerInstanceDesign.md | 30 +++ .../Ipc/WorkerPipeSessionTests.cs | 218 +++++++++++++++++- .../Ipc/WorkerFrameReader.cs | 14 ++ .../Ipc/WorkerPipeClient.cs | 7 + .../Ipc/WorkerPipeSession.cs | 142 +++++++++++- 5 files changed, 408 insertions(+), 3 deletions(-) diff --git a/docs/MxAccessWorkerInstanceDesign.md b/docs/MxAccessWorkerInstanceDesign.md index c194340..35c6cf1 100644 --- a/docs/MxAccessWorkerInstanceDesign.md +++ b/docs/MxAccessWorkerInstanceDesign.md @@ -887,6 +887,36 @@ Graceful shutdown sequence: If shutdown wedges, the gateway kills the process. The worker should be written so process kill does not corrupt other sessions. +### Ending the pipe read (net48) + +Step 8 above cannot be done by cancellation. On .NET Framework 4.8 +`NamedPipeClientStream.ReadAsync` accepts a `CancellationToken` and then never +wires it to the overlapped I/O, so a read parked waiting for gateway bytes stays +parked no matter what the worker cancels. Closing the handle is the only thing +that ends it. + +`WorkerPipeSession.RunMessageLoopAsync` races one outstanding read against the +heartbeat and event-drain loops, so every fault exit — an event-drain fault, an +event too large to frame, a failed heartbeat write — unwinds while that read is +still pending. The session therefore owns the transport: `RunAsync`'s outermost +`finally` disposes the stream as its last teardown step and then awaits the read +that disposal unblocks. Both halves matter. Disposal has to come last because +every frame the session will ever write is complete by then (the frame writer +signals a write only after it has been written *and* flushed), and the await has +to happen while the session still holds the read task, because the worker +installs no `TaskScheduler.UnobservedTaskException` handler — otherwise the +read's `ObjectDisposedException`/`IOException` lands on a task nobody observes, +still holding the reader's reused length-prefix buffer and its pooled payload +buffer. + +Two invariants follow. Nothing may call `WorkerFrameReader.ReadAsync` again once +a read has been abandoned: a second read would race the first for those buffers +and could return a pooled buffer twice. And `WorkerPipeClient`'s `using` on the +pipe stays as a backstop for the paths the session never reaches (a session +factory that throws), not as the primary owner — disposal is idempotent, so its +second `Dispose` is a no-op. Graceful shutdown leaves no pending read at all, so +the observation step is a no-op on that path. + `MxAccessStaSession.ShutdownGracefullyAsync` implements the current cleanup path. It first calls `StaCommandDispatcher.RequestShutdown()` so new commands are rejected and queued commands that have not started receive 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 4f6703c..18580cd 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs @@ -912,6 +912,192 @@ public sealed class WorkerPipeSessionTests await Assert.ThrowsAsync(async () => await runTask); } + /// + /// WRK-31. A fault exit unwinds the message loop while its frame read is still pending, and + /// on net48 nothing can cancel that read — NamedPipeClientStream.ReadAsync ignores + /// the token, so only closing the handle ends it. The session must therefore dispose the + /// transport itself and then await the read that disposal unblocks: the worker installs no + /// TaskScheduler.UnobservedTaskException handler, so before this the read faulted on + /// a task nobody held — carrying the reader's reused prefix buffer and its pooled payload + /// buffer with it — and surfaced only when the finalizer got round to it. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task RunAsync_WhenFaultEndsSession_ObservesTheAbandonedPipeRead() + { + const uint tinyMaxFrameBytes = 4096; + object unobservedGate = new(); + List unobservedPipeExceptions = new(); + EventHandler unobservedHandler = (_, args) => + { + // TaskScheduler.UnobservedTaskException is process-global and xUnit runs this + // assembly's test classes in parallel, so the capture is narrowed to the failure under + // test: a pipe stream's own teardown exception. SetObserved is deliberately NOT called + // — the default policy already swallows these, and observing them here would mask the + // very regression a concurrently running test might be reporting. + foreach (Exception inner in args.Exception.Flatten().InnerExceptions) + { + if ((inner is ObjectDisposedException || inner is IOException) + && inner.Message.IndexOf("pipe", StringComparison.OrdinalIgnoreCase) >= 0) + { + lock (unobservedGate) + { + unobservedPipeExceptions.Add(inner); + } + } + } + }; + + RecordingWorkerLogger logger = new(); + TaskScheduler.UnobservedTaskException += unobservedHandler; + try + { + using CancellationTokenSource cancellation = new(TimeSpan.FromSeconds(15)); + using PipePair pipePair = await PipePair.CreateAsync(cancellation.Token); + FakeRuntimeSession runtime = new(); + WorkerPipeSession session = CreatePipeSession( + pipePair.WorkerStream, + runtime, + new WorkerPipeSessionOptions + { + HeartbeatInterval = TimeSpan.FromMilliseconds(100), + HeartbeatGrace = TimeSpan.FromSeconds(5), + }, + logger); + runtime.EnqueueEvent(CreateOversizedWorkerEvent(sequence: 91, payloadBytes: 16 * 1024)); + Task runTask = session.RunAsync(cancellation.Token); + await CompleteGatewayHandshakeAsync(pipePair, tinyMaxFrameBytes, cancellation.Token); + + await ReadUntilAsync( + pipePair.GatewayReader, + WorkerEnvelope.BodyOneofCase.WorkerFault, + cancellation.Token); + + // The same 5s bound the sibling oversized-event test uses: teardown must not stall on + // the read it abandoned. + Task completedTask = await Task.WhenAny( + runTask, + Task.Delay(TimeSpan.FromSeconds(5), cancellation.Token)); + Assert.Same(runTask, completedTask); + await Assert.ThrowsAsync(async () => await runTask); + + // Deterministic evidence the read was both abandoned and observed: the shared + // observe-with-timeout helper logs the fault it swallowed, tagged "PipeRead". An + // assertion on the exception type would be over-specified — a handle closed under a + // pending overlapped read surfaces as ObjectDisposedException, IOException, or a + // zero-byte read mapped to EndOfStream depending on how the I/O completes — and the + // contract here is that the fault is observed at all, not which one it is. + Assert.Contains( + logger.Events, + entry => entry.EventName == "WorkerPipeSessionBackgroundTaskStopFailed" + && entry.Fields.TryGetValue("task", out object? task) + && (task as string) == "PipeRead"); + + // ...and that the disposal is what ended it: had the read stayed parked, the helper + // would have given up after BackgroundTaskStopTimeout and logged the timeout instead. + Assert.DoesNotContain( + logger.Events, + entry => entry.EventName == "WorkerPipeSessionBackgroundTaskStopTimedOut" + && entry.Fields.TryGetValue("task", out object? task) + && (task as string) == "PipeRead"); + + // Drive any task that faulted without an awaiter through its finalizer, which is what + // raises UnobservedTaskException. Nothing from the pipe read may surface. + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + finally + { + TaskScheduler.UnobservedTaskException -= unobservedHandler; + } + + lock (unobservedGate) + { + Assert.Empty(unobservedPipeExceptions); + } + } + + /// + /// WRK-31, the other side of the invariant. The graceful path leaves the message loop + /// through its return after the shutdown ack, with that iteration's read already + /// awaited — so there is no abandoned read for teardown to account for. Pinning this keeps + /// the new disposal-and-observe step a pure no-op on the path production takes every time a + /// session closes normally: no "PipeRead" observation, and therefore no chance of paying + /// the observation timeout on a healthy shutdown. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task RunAsync_GracefulShutdown_LeavesNoPendingPipeReadToObserve() + { + using CancellationTokenSource cancellation = new(TimeSpan.FromSeconds(10)); + using PipePair pipePair = await PipePair.CreateAsync(cancellation.Token); + FakeRuntimeSession runtime = new(); + RecordingWorkerLogger logger = new(); + WorkerPipeSession session = CreatePipeSession( + pipePair.WorkerStream, + runtime, + new WorkerPipeSessionOptions + { + HeartbeatInterval = TimeSpan.FromMilliseconds(100), + HeartbeatGrace = TimeSpan.FromSeconds(5), + }, + logger); + Task runTask = session.RunAsync(cancellation.Token); + await CompleteGatewayHandshakeAsync(pipePair, cancellation.Token); + + // Reads the ack and bounds RunAsync's completion at 5s. + await SendShutdownAndWaitAsync(pipePair, runTask, cancellation.Token); + + Assert.True(runtime.Disposed, "Graceful shutdown must dispose the runtime session."); + Assert.DoesNotContain( + logger.Events, + entry => (entry.EventName == "WorkerPipeSessionBackgroundTaskStopFailed" + || entry.EventName == "WorkerPipeSessionBackgroundTaskStopTimedOut") + && entry.Fields.TryGetValue("task", out object? task) + && (task as string) == "PipeRead"); + } + + /// + /// WRK-31. The session now owns and closes the transport rather than leaving it to + /// WorkerPipeClient's using, because the read it unblocks has to be observed + /// while the session still holds it. This asserts the closure actually happens on the + /// session's own timeline: the gateway end of the pipe must see disconnection while + /// is still undisposed, so nothing but the worker side of the + /// session can have closed it. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task RunAsync_WhenSessionEnds_ClosesTransportBeforeTheHarnessDoes() + { + using CancellationTokenSource cancellation = new(TimeSpan.FromSeconds(10)); + using PipePair pipePair = await PipePair.CreateAsync(cancellation.Token); + FakeRuntimeSession runtime = new(); + WorkerPipeSession session = CreatePipeSession(pipePair.WorkerStream, runtime); + Task runTask = session.RunAsync(cancellation.Token); + await CompleteGatewayHandshakeAsync(pipePair, cancellation.Token); + + await SendShutdownAndWaitAsync(pipePair, runTask, cancellation.Token); + + // PipePair.Dispose has not run — its `using` is still in scope — so the only thing that can + // have closed the worker end is the session. A gateway-side read is uncancellable on net48 + // exactly as the worker's is, so a session that left the pipe open would park this read + // until the harness disposes; the bound below is what catches that. + Task disconnectTask = ReadUntilDisconnectedAsync(pipePair.GatewayReader); + Task completedTask = await Task.WhenAny( + disconnectTask, + Task.Delay(TimeSpan.FromSeconds(5), cancellation.Token)); + Assert.Same(disconnectTask, completedTask); + + Exception disconnect = await disconnectTask; + Assert.True( + disconnect is IOException + || disconnect is ObjectDisposedException + || (disconnect is WorkerFrameProtocolException frameException + && frameException.ErrorCode == WorkerFrameProtocolErrorCode.EndOfStream), + $"Expected the gateway read to observe a pipe disconnection, got {disconnect}."); + } + /// /// Verifies that ShutdownWorker returns its OK reply BEFORE the graceful /// shutdown runs and disposes the runtime session, and that the message @@ -1881,7 +2067,11 @@ public sealed class WorkerPipeSessionTests () => 1234, sessionOptions, () => runtime, - logger); + logger, + // Hand the session the same ownership the production WorkerPipeClient path gives it, so + // these tests exercise the real teardown: the session closes the transport itself and + // then observes the read that closure unblocks. + transportStream: stream); } private static WorkerFrameProtocolOptions CreateOptions() @@ -2149,6 +2339,32 @@ public sealed class WorkerPipeSessionTests } } + /// + /// Reads a pipe end until it stops producing frames, returning whatever ended it. Frames + /// still buffered from before the peer closed its handle — a trailing heartbeat, say — are + /// drained first, because Windows named pipes hand over buffered bytes ahead of the + /// broken-pipe signal. + /// + /// Frame reader over the end being watched. + /// The exception that ended the read. + private static async Task ReadUntilDisconnectedAsync(WorkerFrameReader reader) + { + while (true) + { + try + { + await reader.ReadAsync(CancellationToken.None).ConfigureAwait(false); + } + catch (Exception exception) when ( + exception is IOException + || exception is ObjectDisposedException + || exception is WorkerFrameProtocolException) + { + return exception; + } + } + } + private static WorkerEnvelope[] ReadWrittenFrames( MemoryStream stream, WorkerFrameProtocolOptions options) diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs index 041e879..8ae6601 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameReader.cs @@ -22,6 +22,14 @@ public sealed class WorkerFrameReader // Reused across frames rather than allocated per read (GWC-30). Safe because ReadAsync is // single-consumer by construction; the prefix is fully overwritten by every read. + // + // The single-consumer invariant has to survive teardown as well as steady state, because a read + // abandoned by WorkerPipeSession's message loop still owns this buffer (and its rented payload + // buffer) until it faults. Nothing may call ReadAsync again after that point: a second read + // would race the abandoned one for the prefix, and could hand a pooled payload buffer back to + // ArrayPool twice. The loop guarantees it structurally — it only ever issues a read after + // awaiting the previous one, and it never re-enters after unwinding — and teardown only awaits + // the abandoned read, never reissues it (WorkerPipeSession.ObserveAbandonedPipeReadAsync). private readonly byte[] _lengthPrefix = new byte[sizeof(uint)]; /// Initializes the reader with a stream and protocol options. @@ -106,6 +114,12 @@ public sealed class WorkerFrameReader int offset = 0; while (offset < count) { + // The token is forwarded but is NOT a bound on a pipe read: on .NET Framework 4.8 + // NamedPipeClientStream.ReadAsync accepts a CancellationToken and never wires it to the + // overlapped I/O, so a read waiting on gateway bytes ignores cancellation entirely. Only + // closing the handle ends it (WorkerPipeSession disposes the transport at teardown for + // exactly this reason). It is still passed because non-pipe streams — the + // MemoryStream-backed unit tests, and any future transport — do honor it. int bytesRead = await _stream .ReadAsync(buffer, offset, count - offset, cancellationToken) .ConfigureAwait(false); diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeClient.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeClient.cs index ab47518..03885cf 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeClient.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeClient.cs @@ -151,6 +151,13 @@ public sealed class WorkerPipeClient : IWorkerPipeClient WorkerFrameProtocolOptions frameOptions = new(options); + // The session disposes this pipe itself as its last teardown step — that disposal is what + // unblocks a net48 pipe read the message loop abandoned, and it has to happen while the + // session still holds the read task so the resulting fault is observed rather than orphaned + // (see WorkerPipeSession.RunAsync). The `using` stays as the backstop for the paths the + // session never reaches: a session factory that throws, or a RunAsync that never gets past + // its own construction. Disposal is idempotent, so the second Dispose is a no-op — do not + // "clean up" this `using` on the assumption that it is now redundant. using NamedPipeClientStream pipe = await ConnectWithRetryAsync(options.PipeName, cancellationToken) .ConfigureAwait(false); diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs index 8934946..7f1eb3e 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs @@ -39,8 +39,21 @@ public sealed class WorkerPipeSession private readonly WorkerFrameWriter _writer; private readonly object _commandTaskGate = new(); private readonly HashSet _activeCommandTasks = new(); + + // The transport the reader and writer share, when this session was handed one to own. Null for + // the reader/writer constructors, whose callers keep ownership of whatever streams they built the + // pair over. Owning it is what lets teardown close the handle (WRK-31): on net48 a pipe read + // parked in the kernel cannot be cancelled, only unblocked by disposal. + private readonly Stream? _transportStream; + private IWorkerRuntimeSession? _runtimeSession; + // The one outstanding, not-yet-awaited frame read, or null when no read is in flight. Written + // only by the message loop (which sets it at each read issue and clears it before awaiting that + // read itself) and read only by RunAsync's finally — which runs after the loop's task has + // completed, so the await supplies the happens-before edge and no interlock is needed. + private Task? _pendingReadTask; + // Mutated from the message loop, command tasks, the heartbeat loop and the // shutdown path; volatile so cross-thread reads observe the latest state // without tearing (WorkerState is an int-backed protobuf enum). @@ -63,7 +76,8 @@ public sealed class WorkerPipeSession () => Process.GetCurrentProcess().Id, new WorkerPipeSessionOptions(), () => new MxAccessStaSession((eq, affinity, comFactory) => new AlarmCommandHandler(eq, () => new WnWrapAlarmConsumer(), affinity, comFactory, standbyFactory: null)), - logger) + logger, + stream) { } @@ -96,6 +110,12 @@ public sealed class WorkerPipeSession /// Session-specific options. /// Factory creating the MXAccess runtime session. /// Optional logger for diagnostic output. + /// + /// Stream the reader and writer share, when this session is to own it. Supplying it makes + /// dispose the transport as its last teardown step, which is the only + /// way to unblock a pending net48 pipe read (see ). Null + /// leaves ownership — and the disposal — with the caller. + /// public WorkerPipeSession( WorkerFrameReader reader, WorkerFrameWriter writer, @@ -103,7 +123,8 @@ public sealed class WorkerPipeSession Func processIdProvider, WorkerPipeSessionOptions sessionOptions, Func runtimeSessionFactory, - IWorkerLogger? logger = null) + IWorkerLogger? logger = null, + Stream? transportStream = null) { _reader = reader ?? throw new ArgumentNullException(nameof(reader)); _writer = writer ?? throw new ArgumentNullException(nameof(writer)); @@ -112,6 +133,7 @@ public sealed class WorkerPipeSession _sessionOptions = sessionOptions ?? throw new ArgumentNullException(nameof(sessionOptions)); _runtimeSessionFactory = runtimeSessionFactory ?? throw new ArgumentNullException(nameof(runtimeSessionFactory)); _logger = logger; + _transportStream = transportStream; _sessionOptions.Validate(); } @@ -142,9 +164,92 @@ public sealed class WorkerPipeSession _runtimeSession?.Dispose(); _runtimeSession = null; _state = WorkerState.Stopped; + + // Closing the transport is what actually ends a pipe read parked in the kernel: on net48 + // NamedPipeClientStream.ReadAsync ignores its CancellationToken, so the message loop's + // cancellation can never reach one (WRK-31). This is deliberately the LAST teardown step, + // because every frame this session will ever write has completed by the time control + // reaches here: WorkerFrameWriter.WriteAsync signals only after the frame is written AND + // flushed, and every exit path awaits its final write before unwinding — the shutdown ack + // and shutdown-timeout fault inside the loop's dispatch, the event-drain and + // oversized-event faults inside the drain task the loop awaits, the watchdog fault inside + // the heartbeat task the loop awaits, and the handshake fault inside + // CompleteStartupHandshakeAsync's catch. In-flight command replies are the one class of + // write that can still be racing here, and they raced the identical disposal before this + // change (WorkerPipeClient's `using` fired on the very next statement after RunAsync); + // both ProcessCommandAsync's Ready-state gate and TryWriteFaultAsync's + // ObjectDisposedException/IOException swallow already cover that race. + // + // Owning the disposal here — rather than leaving it to WorkerPipeClient's `using` — is + // what makes the abandoned read observable: the fault it takes on disposal lands on a + // task this session still holds, so ObserveAbandonedPipeReadAsync can await it instead of + // leaving it for a TaskScheduler.UnobservedTaskException handler the worker does not have. + DisposeTransportStream(); + await ObserveAbandonedPipeReadAsync().ConfigureAwait(false); } } + /// + /// Disposes the transport this session owns, if it was handed one. Dispose-time failures are + /// logged and swallowed: this runs inside 's finally, where letting an + /// escape would replace the exception that actually ended the + /// session (a shutdown timeout, a protocol violation, an event too large to frame) with a + /// far less actionable one. Disposal is idempotent, so WorkerPipeClient's outer + /// using re-disposing the same stream immediately afterwards is a no-op. + /// + private void DisposeTransportStream() + { + if (_transportStream is null) + { + return; + } + + try + { + _transportStream.Dispose(); + } + catch (Exception exception) when (exception is IOException || exception is ObjectDisposedException) + { + _logger?.Error( + "WorkerPipeSessionTransportDisposeFailed", + new Dictionary + { + ["session_id"] = _options.SessionId, + ["exception"] = exception.ToString(), + }); + } + } + + /// + /// Awaits the frame read the message loop walked away from, if there is one. + /// races an uncancellable read against the heartbeat and + /// event-drain loops, so every fault exit (event-drain fault, oversized event, heartbeat + /// write failure) leaves a read outstanding on a task the loop never awaits again. Once + /// has closed the handle that read faults with + /// or ; awaiting it here is + /// what keeps the fault observed, because the worker installs no + /// TaskScheduler.UnobservedTaskException handler. + /// + /// + /// No second read can follow this one. The message loop only ever issues a read after + /// awaiting the previous one, and it never re-enters after unwinding, so + /// 's single-consumer invariant — and with it the safety of + /// its reused length-prefix buffer and its pooled payload buffer, which the abandoned read + /// still owns until it faults — holds through teardown. + /// + /// A task that represents the asynchronous operation. + private async Task ObserveAbandonedPipeReadAsync() + { + Task? readTask = _pendingReadTask; + _pendingReadTask = null; + if (readTask is null) + { + return; + } + + await ObserveBackgroundTaskStopAsync(readTask, "PipeRead").ConfigureAwait(false); + } + /// Completes the gateway startup handshake using default MXAccess initialization. /// Token to cancel the asynchronous operation. /// A task that represents the asynchronous operation. @@ -264,6 +369,34 @@ public sealed class WorkerPipeSession return _writer.WriteAsync(CreateEnvelope(ready), cancellationToken); } + /// + /// Runs the post-handshake message loop, racing one outstanding frame read against the + /// heartbeat and event-drain loops. + /// + /// + /// + /// loopCancellation does NOT bound the read. On .NET Framework 4.8 + /// NamedPipeClientStream.ReadAsync accepts a and + /// then ignores it — the token never reaches the overlapped I/O, so a read parked + /// waiting for gateway bytes stays parked no matter what is cancelled. The token is + /// still passed because the reader's contract takes one and a non-pipe stream (the + /// MemoryStream-backed unit tests) does honor it. What actually ends a parked read is + /// closing the handle, which does at the end of teardown. + /// + /// + /// That asymmetry is why the loop records its outstanding read in + /// _pendingReadTask. Every fault exit — an event-drain fault, an event too large + /// to frame, a failed heartbeat write — unwinds through Task.WhenAny while the + /// read is still pending, and the fault that read eventually takes has to be observed by + /// somebody (see ). The field is cleared + /// before the loop awaits a read itself, so it is non-null exactly when a read is + /// outstanding and unobserved. Graceful exits — the return below, after a + /// WorkerShutdown envelope or a ShutdownWorker command — leave no pending + /// read at all, so teardown's observation is a no-op there. + /// + /// + /// Token to cancel the asynchronous operation. + /// A task that represents the asynchronous operation. private async Task RunMessageLoopAsync(CancellationToken cancellationToken) { using CancellationTokenSource loopCancellation = CancellationTokenSource @@ -273,6 +406,7 @@ public sealed class WorkerPipeSession Task heartbeatTask = RunHeartbeatLoopAsync(heartbeatCancellation.Token); Task eventDrainTask = RunEventDrainLoopAsync(heartbeatCancellation.Token); Task readTask = _reader.ReadAsync(loopCancellation.Token); + _pendingReadTask = readTask; try { @@ -281,6 +415,9 @@ public sealed class WorkerPipeSession Task completedTask = await Task.WhenAny(readTask, heartbeatTask, eventDrainTask).ConfigureAwait(false); if (completedTask == readTask) { + // The loop observes this read itself, whether it yields an envelope or throws, + // so it is no longer the abandoned one teardown has to account for. + _pendingReadTask = null; WorkerEnvelope envelope = await readTask.ConfigureAwait(false); bool keepReading = await DispatchGatewayEnvelopeAsync(envelope, cancellationToken).ConfigureAwait(false); if (!keepReading) @@ -289,6 +426,7 @@ public sealed class WorkerPipeSession } readTask = _reader.ReadAsync(loopCancellation.Token); + _pendingReadTask = readTask; } else if (completedTask == heartbeatTask) { From 3ef56be2dd2d707b306404f714f013946eacac53 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:21:25 -0400 Subject: [PATCH 17/23] fix(worker): unconditional fault observation for abandoned pipe I/O; exception-total transport dispose --- docs/MxAccessWorkerInstanceDesign.md | 42 ++- .../Ipc/WorkerPipeSessionTests.cs | 239 ++++++++++-------- .../Ipc/WorkerPipeSession.cs | 126 +++++++-- 3 files changed, 262 insertions(+), 145 deletions(-) diff --git a/docs/MxAccessWorkerInstanceDesign.md b/docs/MxAccessWorkerInstanceDesign.md index 35c6cf1..28f49a1 100644 --- a/docs/MxAccessWorkerInstanceDesign.md +++ b/docs/MxAccessWorkerInstanceDesign.md @@ -900,22 +900,36 @@ heartbeat and event-drain loops, so every fault exit — an event-drain fault, a event too large to frame, a failed heartbeat write — unwinds while that read is still pending. The session therefore owns the transport: `RunAsync`'s outermost `finally` disposes the stream as its last teardown step and then awaits the read -that disposal unblocks. Both halves matter. Disposal has to come last because -every frame the session will ever write is complete by then (the frame writer -signals a write only after it has been written *and* flushed), and the await has -to happen while the session still holds the read task, because the worker -installs no `TaskScheduler.UnobservedTaskException` handler — otherwise the -read's `ObjectDisposedException`/`IOException` lands on a task nobody observes, -still holding the reader's reused length-prefix buffer and its pooled payload -buffer. +that disposal unblocks. Disposal comes last because in the ordinary case every +frame the session will ever write is already complete by then — the frame writer +signals a write only after it has been written *and* flushed. It is not last +because that is guaranteed: the wait on the heartbeat and drain loops is +budgeted, and a stream write is genuinely uncancellable, so an overrunning write +can still be in flight against the stream being disposed. Disposal is +consequently exception-*total*, catching anything the handle close throws and +logging it, because nothing raised while releasing a handle is more actionable +than the terminal exception that ended the session, and nothing may displace it. + +Observation is unconditional; only the *logging* of it is budgeted. +`ObserveBackgroundTaskStopAsync` waits `BackgroundTaskStopTimeout` for a task to +stop and logs what it saw, but when it gives up it hands the task a +fault-observing continuation before returning. Windows owes no deadline for a +completion torn off a closed handle, so a bounded await on its own would reopen +the very orphaning window it was added to close. The same helper — and so the +same guarantee — covers the abandoned read, the heartbeat loop, and the +event-drain loop. This matters because the worker installs no +`TaskScheduler.UnobservedTaskException` handler: an unheld faulted task would +otherwise surface only at finalization, still holding the reader's reused +length-prefix buffer and its pooled payload buffer. Two invariants follow. Nothing may call `WorkerFrameReader.ReadAsync` again once -a read has been abandoned: a second read would race the first for those buffers -and could return a pooled buffer twice. And `WorkerPipeClient`'s `using` on the -pipe stays as a backstop for the paths the session never reaches (a session -factory that throws), not as the primary owner — disposal is idempotent, so its -second `Dispose` is a no-op. Graceful shutdown leaves no pending read at all, so -the observation step is a no-op on that path. +a read has been abandoned — a second read would race the first for those buffers +and could return a pooled buffer twice — which `Debug.Assert`s at both read-issue +sites guard. And `WorkerPipeClient`'s `using` on the pipe stays as a backstop for +the paths the session never reaches (a session factory that throws), not as the +primary owner; disposal is idempotent, so its second `Dispose` is a no-op. +Graceful shutdown leaves no pending read at all, so the observation step is a +no-op on that path. `MxAccessStaSession.ShutdownGracefullyAsync` implements the current cleanup path. It first calls `StaCommandDispatcher.RequestShutdown()` so new commands 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 18580cd..826e1b0 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs @@ -912,112 +912,6 @@ public sealed class WorkerPipeSessionTests await Assert.ThrowsAsync(async () => await runTask); } - /// - /// WRK-31. A fault exit unwinds the message loop while its frame read is still pending, and - /// on net48 nothing can cancel that read — NamedPipeClientStream.ReadAsync ignores - /// the token, so only closing the handle ends it. The session must therefore dispose the - /// transport itself and then await the read that disposal unblocks: the worker installs no - /// TaskScheduler.UnobservedTaskException handler, so before this the read faulted on - /// a task nobody held — carrying the reader's reused prefix buffer and its pooled payload - /// buffer with it — and surfaced only when the finalizer got round to it. - /// - /// A task that represents the asynchronous operation. - [Fact] - public async Task RunAsync_WhenFaultEndsSession_ObservesTheAbandonedPipeRead() - { - const uint tinyMaxFrameBytes = 4096; - object unobservedGate = new(); - List unobservedPipeExceptions = new(); - EventHandler unobservedHandler = (_, args) => - { - // TaskScheduler.UnobservedTaskException is process-global and xUnit runs this - // assembly's test classes in parallel, so the capture is narrowed to the failure under - // test: a pipe stream's own teardown exception. SetObserved is deliberately NOT called - // — the default policy already swallows these, and observing them here would mask the - // very regression a concurrently running test might be reporting. - foreach (Exception inner in args.Exception.Flatten().InnerExceptions) - { - if ((inner is ObjectDisposedException || inner is IOException) - && inner.Message.IndexOf("pipe", StringComparison.OrdinalIgnoreCase) >= 0) - { - lock (unobservedGate) - { - unobservedPipeExceptions.Add(inner); - } - } - } - }; - - RecordingWorkerLogger logger = new(); - TaskScheduler.UnobservedTaskException += unobservedHandler; - try - { - using CancellationTokenSource cancellation = new(TimeSpan.FromSeconds(15)); - using PipePair pipePair = await PipePair.CreateAsync(cancellation.Token); - FakeRuntimeSession runtime = new(); - WorkerPipeSession session = CreatePipeSession( - pipePair.WorkerStream, - runtime, - new WorkerPipeSessionOptions - { - HeartbeatInterval = TimeSpan.FromMilliseconds(100), - HeartbeatGrace = TimeSpan.FromSeconds(5), - }, - logger); - runtime.EnqueueEvent(CreateOversizedWorkerEvent(sequence: 91, payloadBytes: 16 * 1024)); - Task runTask = session.RunAsync(cancellation.Token); - await CompleteGatewayHandshakeAsync(pipePair, tinyMaxFrameBytes, cancellation.Token); - - await ReadUntilAsync( - pipePair.GatewayReader, - WorkerEnvelope.BodyOneofCase.WorkerFault, - cancellation.Token); - - // The same 5s bound the sibling oversized-event test uses: teardown must not stall on - // the read it abandoned. - Task completedTask = await Task.WhenAny( - runTask, - Task.Delay(TimeSpan.FromSeconds(5), cancellation.Token)); - Assert.Same(runTask, completedTask); - await Assert.ThrowsAsync(async () => await runTask); - - // Deterministic evidence the read was both abandoned and observed: the shared - // observe-with-timeout helper logs the fault it swallowed, tagged "PipeRead". An - // assertion on the exception type would be over-specified — a handle closed under a - // pending overlapped read surfaces as ObjectDisposedException, IOException, or a - // zero-byte read mapped to EndOfStream depending on how the I/O completes — and the - // contract here is that the fault is observed at all, not which one it is. - Assert.Contains( - logger.Events, - entry => entry.EventName == "WorkerPipeSessionBackgroundTaskStopFailed" - && entry.Fields.TryGetValue("task", out object? task) - && (task as string) == "PipeRead"); - - // ...and that the disposal is what ended it: had the read stayed parked, the helper - // would have given up after BackgroundTaskStopTimeout and logged the timeout instead. - Assert.DoesNotContain( - logger.Events, - entry => entry.EventName == "WorkerPipeSessionBackgroundTaskStopTimedOut" - && entry.Fields.TryGetValue("task", out object? task) - && (task as string) == "PipeRead"); - - // Drive any task that faulted without an awaiter through its finalizer, which is what - // raises UnobservedTaskException. Nothing from the pipe read may surface. - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - } - finally - { - TaskScheduler.UnobservedTaskException -= unobservedHandler; - } - - lock (unobservedGate) - { - Assert.Empty(unobservedPipeExceptions); - } - } - /// /// WRK-31, the other side of the invariant. The graceful path leaves the message loop /// through its return after the shutdown ack, with that iteration's read already @@ -2381,6 +2275,123 @@ public sealed class WorkerPipeSessionTests return envelopes.ToArray(); } + /// + /// The one teardown test that has to arm , + /// which is process-global: a task faulting in any concurrently running test class can be + /// finalized inside this test's window and read as its result. It therefore lives in its own + /// non-parallel collection (see ) rather + /// than alongside its siblings. Nested so it can still reach + /// 's private harness — PipePair, + /// CreatePipeSession, RecordingWorkerLogger — without widening any of it. + /// + [Collection(WorkerPipeSessionNonParallelCollection.Name)] + public sealed class AbandonedPipeReadTeardownTests + { + /// + /// WRK-31. A fault exit unwinds the message loop while its frame read is still pending, + /// and on net48 nothing can cancel that read — NamedPipeClientStream.ReadAsync + /// ignores the token, so only closing the handle ends it. The session must therefore + /// dispose the transport itself and account for the read that disposal unblocks: the + /// worker installs no TaskScheduler.UnobservedTaskException handler, so before + /// this the read faulted on a task nobody held — still carrying the reader's reused + /// prefix buffer and its pooled payload buffer — and surfaced only at finalization. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task RunAsync_WhenFaultEndsSession_ObservesTheAbandonedPipeRead() + { + const uint tinyMaxFrameBytes = 4096; + object unobservedGate = new(); + List unobservedPipeExceptions = new(); + EventHandler unobservedHandler = (_, args) => + { + // Narrowed to a pipe stream's own teardown exception even though the collection is + // non-parallel, because the handler stays armed across this test's own async + // machinery. SetObserved is deliberately NOT called — the default policy already + // swallows these, and observing them here would mask a regression rather than + // report it. + foreach (Exception inner in args.Exception.Flatten().InnerExceptions) + { + if ((inner is ObjectDisposedException || inner is IOException) + && inner.Message.IndexOf("pipe", StringComparison.OrdinalIgnoreCase) >= 0) + { + lock (unobservedGate) + { + unobservedPipeExceptions.Add(inner); + } + } + } + }; + + RecordingWorkerLogger logger = new(); + TaskScheduler.UnobservedTaskException += unobservedHandler; + try + { + using CancellationTokenSource cancellation = new(TimeSpan.FromSeconds(15)); + using PipePair pipePair = await PipePair.CreateAsync(cancellation.Token); + FakeRuntimeSession runtime = new(); + WorkerPipeSession session = CreatePipeSession( + pipePair.WorkerStream, + runtime, + new WorkerPipeSessionOptions + { + HeartbeatInterval = TimeSpan.FromMilliseconds(100), + HeartbeatGrace = TimeSpan.FromSeconds(5), + }, + logger); + runtime.EnqueueEvent(CreateOversizedWorkerEvent(sequence: 91, payloadBytes: 16 * 1024)); + Task runTask = session.RunAsync(cancellation.Token); + await CompleteGatewayHandshakeAsync(pipePair, tinyMaxFrameBytes, cancellation.Token); + + await ReadUntilAsync( + pipePair.GatewayReader, + WorkerEnvelope.BodyOneofCase.WorkerFault, + cancellation.Token); + + // The same 5s bound the sibling oversized-event test uses: teardown must not stall + // on the read it abandoned. + Task completedTask = await Task.WhenAny( + runTask, + Task.Delay(TimeSpan.FromSeconds(5), cancellation.Token)); + Assert.Same(runTask, completedTask); + await Assert.ThrowsAsync(async () => await runTask); + + // Evidence the read was abandoned and that teardown took responsibility for it: the + // shared observe-with-timeout helper records it under the "PipeRead" tag either way. + // Which of the two entries lands is a timing detail, not a contract. The fault + // normally arrives at once (StopFailed), but Windows owes no deadline for a + // completion torn off a closed handle, so on a loaded box it can arrive after + // BackgroundTaskStopTimeout (StopTimedOut). Both are correct, because observation is + // unconditional — the helper attaches a fault-observing continuation when it gives + // up waiting — and the unobserved-exception assertion below is what actually pins + // that. That the transport really is closed is pinned deterministically by + // RunAsync_WhenSessionEnds_ClosesTransportBeforeTheHarnessDoes, so insisting on + // "StopFailed within 1s" here would buy nothing but a flake at the windev gate. + Assert.Contains( + logger.Events, + entry => (entry.EventName == "WorkerPipeSessionBackgroundTaskStopFailed" + || entry.EventName == "WorkerPipeSessionBackgroundTaskStopTimedOut") + && entry.Fields.TryGetValue("task", out object? task) + && (task as string) == "PipeRead"); + + // Drive any task that faulted without an awaiter through its finalizer, which is + // what raises UnobservedTaskException. Nothing from the pipe read may surface. + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + finally + { + TaskScheduler.UnobservedTaskException -= unobservedHandler; + } + + lock (unobservedGate) + { + Assert.Empty(unobservedPipeExceptions); + } + } + } + private sealed class RecordingWorkerLogger : ZB.MOM.WW.MxGateway.Worker.Bootstrap.IWorkerLogger { private readonly object gate = new(); @@ -2700,3 +2711,19 @@ public sealed class WorkerPipeSessionTests } } } + +/// +/// Collection for tests that observe process-global state and so cannot share the runner with +/// anything else. Its only member today is +/// , which arms +/// and forces a GC: a task faulting in any +/// concurrently running test class would be finalized inside that window and misread as this +/// session's orphaned pipe read. Keep membership minimal — every test added here is a test the +/// rest of the suite has to wait for. +/// +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class WorkerPipeSessionNonParallelCollection +{ + /// Collection name referenced by . + public const string Name = "WorkerPipeSessionNonParallel"; +} diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs index 7f1eb3e..f849813 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs @@ -167,18 +167,24 @@ public sealed class WorkerPipeSession // Closing the transport is what actually ends a pipe read parked in the kernel: on net48 // NamedPipeClientStream.ReadAsync ignores its CancellationToken, so the message loop's - // cancellation can never reach one (WRK-31). This is deliberately the LAST teardown step, - // because every frame this session will ever write has completed by the time control - // reaches here: WorkerFrameWriter.WriteAsync signals only after the frame is written AND - // flushed, and every exit path awaits its final write before unwinding — the shutdown ack - // and shutdown-timeout fault inside the loop's dispatch, the event-drain and - // oversized-event faults inside the drain task the loop awaits, the watchdog fault inside - // the heartbeat task the loop awaits, and the handshake fault inside - // CompleteStartupHandshakeAsync's catch. In-flight command replies are the one class of - // write that can still be racing here, and they raced the identical disposal before this - // change (WorkerPipeClient's `using` fired on the very next statement after RunAsync); - // both ProcessCommandAsync's Ready-state gate and TryWriteFaultAsync's - // ObjectDisposedException/IOException swallow already cover that race. + // cancellation can never reach one (WRK-31). It is deliberately the LAST teardown step, + // because in the ordinary case every frame this session will ever write has completed by + // the time control reaches here: WorkerFrameWriter.WriteAsync signals only after the + // frame is written AND flushed, and each exit path awaits its final write before + // unwinding — the shutdown ack and shutdown-timeout fault inside the loop's dispatch, the + // event-drain and oversized-event faults inside the drain task the loop awaits, the + // watchdog fault inside the heartbeat task the loop awaits, and the handshake fault + // inside CompleteStartupHandshakeAsync's catch. + // + // "Ordinary" is the honest word, not "always": the loop's wait on the heartbeat and + // drain tasks is budgeted (BackgroundTaskStopTimeout), and a stream write is genuinely + // uncancellable, so a write that overran the budget can still be in flight against the + // stream being disposed here. In-flight command replies are in the same position, and + // they raced the identical disposal before this change (WorkerPipeClient's `using` fired + // on the very next statement after RunAsync). That is precisely why disposal below is + // exception-tolerant and why every abandoned task gets a fault-observing continuation + // from ObserveBackgroundTaskStopAsync — a write losing its stream mid-flight must be a + // logged non-event, not a lost terminal exception or an unobserved task. // // Owning the disposal here — rather than leaving it to WorkerPipeClient's `using` — is // what makes the abandoned read observable: the fault it takes on disposal lands on a @@ -191,12 +197,21 @@ public sealed class WorkerPipeSession /// /// Disposes the transport this session owns, if it was handed one. Dispose-time failures are - /// logged and swallowed: this runs inside 's finally, where letting an - /// escape would replace the exception that actually ended the - /// session (a shutdown timeout, a protocol violation, an event too large to frame) with a - /// far less actionable one. Disposal is idempotent, so WorkerPipeClient's outer - /// using re-disposing the same stream immediately afterwards is a no-op. + /// logged and swallowed: this runs inside 's finally, where letting a + /// failure escape would replace the exception that actually ended the session (a shutdown + /// timeout, a protocol violation, an event too large to frame) with a far less actionable + /// one. Disposal is idempotent, so WorkerPipeClient's outer using re-disposing + /// the same stream immediately afterwards is a no-op. /// + /// + /// The catch is deliberately total rather than the / + /// pair the fault-write paths use, matching + /// 's shape. Narrowing it to the expected types + /// would let an unexpected one — a Win32Exception surfaced by the handle close, say — + /// do the exact harm this guard exists to prevent. The rule is about the position in the + /// code, not about which exceptions are plausible: nothing thrown while releasing a handle + /// is more actionable than the session's terminal exception, so nothing may displace it. + /// private void DisposeTransportStream() { if (_transportStream is null) @@ -208,7 +223,7 @@ public sealed class WorkerPipeSession { _transportStream.Dispose(); } - catch (Exception exception) when (exception is IOException || exception is ObjectDisposedException) + catch (Exception exception) { _logger?.Error( "WorkerPipeSessionTransportDisposeFailed", @@ -226,16 +241,26 @@ public sealed class WorkerPipeSession /// event-drain loops, so every fault exit (event-drain fault, oversized event, heartbeat /// write failure) leaves a read outstanding on a task the loop never awaits again. Once /// has closed the handle that read faults with - /// or ; awaiting it here is - /// what keeps the fault observed, because the worker installs no + /// , , or a zero-byte read + /// mapped to EndOfStream, and that fault has to be observed — the worker installs no /// TaskScheduler.UnobservedTaskException handler. /// /// - /// No second read can follow this one. The message loop only ever issues a read after - /// awaiting the previous one, and it never re-enters after unwinding, so - /// 's single-consumer invariant — and with it the safety of - /// its reused length-prefix buffer and its pooled payload buffer, which the abandoned read - /// still owns until it faults — holds through teardown. + /// + /// The fault is observed unconditionally; it is only logged within + /// . Windows is under no obligation to deliver + /// the abandoned read's completion inside that budget, so a bounded await alone would + /// reopen the orphaning window it was added to close. + /// therefore hands the task a fault-observing continuation when it gives up waiting, + /// which makes the budget a diagnostics decision rather than a correctness one. + /// + /// + /// No second read can follow this one. The message loop only ever issues a read after + /// awaiting the previous one, and it never re-enters after unwinding, so + /// 's single-consumer invariant — and with it the safety + /// of its reused length-prefix buffer and its pooled payload buffer, which the abandoned + /// read still owns until it faults — holds through teardown. + /// /// /// A task that represents the asynchronous operation. private async Task ObserveAbandonedPipeReadAsync() @@ -405,6 +430,9 @@ public sealed class WorkerPipeSession .CreateLinkedTokenSource(cancellationToken); Task heartbeatTask = RunHeartbeatLoopAsync(heartbeatCancellation.Token); Task eventDrainTask = RunEventDrainLoopAsync(heartbeatCancellation.Token); + Debug.Assert( + _pendingReadTask is null, + "A frame read must never be issued while another is outstanding: WorkerFrameReader is single-consumer, and a second read would race the abandoned one for the reused prefix buffer and could return a pooled payload buffer twice."); Task readTask = _reader.ReadAsync(loopCancellation.Token); _pendingReadTask = readTask; @@ -425,6 +453,9 @@ public sealed class WorkerPipeSession return; } + Debug.Assert( + _pendingReadTask is null, + "The previous read must have been awaited before the next is issued: WorkerFrameReader is single-consumer."); readTask = _reader.ReadAsync(loopCancellation.Token); _pendingReadTask = readTask; } @@ -447,6 +478,32 @@ public sealed class WorkerPipeSession } } + /// + /// Waits a bounded time for a background task to stop and records what happened. + /// + /// + /// + /// The budget bounds the logging, never the observation. Every task this is + /// asked to observe is uncancellable at the point that matters — a net48 pipe read + /// ignores its token outright, and WorkerFrameWriter.WriteFrameAsync issues the + /// stream write under CancellationToken.None so a frame is never left + /// half-written on the wire — so any of them can outlive the budget and only then fault, + /// typically against a transport has since disposed. Overrunning + /// the budget and returning is therefore not enough: the task would be left with nobody + /// holding it, which is the exact orphaning this method exists to prevent. + /// + /// + /// So the timeout path hands the task a fault-observing continuation before returning. + /// The fault is then observed unconditionally, whenever it arrives; the budget only + /// decides whether it also gets logged here or is swallowed silently by the + /// continuation. That distinction matters because the worker installs no + /// TaskScheduler.UnobservedTaskException handler, so an unheld faulted task + /// surfaces only at finalization. + /// + /// + /// Background task being stopped. + /// Name recorded in the diagnostic logs. + /// A task that represents the asynchronous operation. private async Task ObserveBackgroundTaskStopAsync( Task task, string taskName) @@ -456,6 +513,7 @@ public sealed class WorkerPipeSession .ConfigureAwait(false); if (completedTask != task) { + ObserveFaultWhenever(task); _logger?.Error( "WorkerPipeSessionBackgroundTaskStopTimedOut", new Dictionary @@ -485,6 +543,24 @@ public sealed class WorkerPipeSession } } + /// + /// Attaches a continuation that observes 's exception whenever the + /// task eventually faults, so a task nobody is awaiting any more can never reach the + /// finalizer with an unobserved exception. Mirrors the shape + /// WorkerFrameWriter.ObserveAbandonedFault uses for frames a cancelled caller stops + /// awaiting (NEXT-04). Faulting is the only outcome that runs the continuation, and the + /// continuation is scheduled inline, so this costs nothing on the ordinary path. + /// + /// Task that may fault after its awaiter has walked away. + private static void ObserveFaultWhenever(Task task) + { + _ = task.ContinueWith( + static faultedTask => _ = faultedTask.Exception, + CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } + private async Task RunEventDrainLoopAsync(CancellationToken cancellationToken) { while (!cancellationToken.IsCancellationRequested) From bb7aa6209f328b99fcd42238bce02ee568ff912a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:25:15 -0400 Subject: [PATCH 18/23] chore(plans): tasks 1-12 complete through review chains --- ...6-08-15-deferred-remediation.md.tasks.json | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/plans/2026-08-15-deferred-remediation.md.tasks.json b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json index 86bcb1b..34f10c3 100644 --- a/docs/plans/2026-08-15-deferred-remediation.md.tasks.json +++ b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json @@ -1,18 +1,18 @@ { "planPath": "docs/plans/2026-08-15-deferred-remediation.md", "tasks": [ - { "id": 1, "subject": "Task 1: Windows-safe cleanup in SecretsStorePathGuardTests", "status": "pending" }, - { "id": 2, "subject": "Task 2: SessionEventDistributor _subscribers to plain Dictionary", "status": "pending" }, - { "id": 3, "subject": "Task 3: Merge the session event-source pass-through iterator", "status": "pending" }, - { "id": 4, "subject": "Task 4: EventStreamService direct channel reads in the live loop", "status": "pending" }, - { "id": 5, "subject": "Task 5: In-process dashboard snapshot feed + page switch", "status": "pending" }, - { "id": 6, "subject": "Task 6: In-process session event subscription + SessionDetailsPage switch", "status": "pending" }, - { "id": 7, "subject": "Task 7: AlarmsPage provider-status via IGatewayAlarmService", "status": "pending" }, - { "id": 8, "subject": "Task 8: Dashboard design-doc update (consolidated)", "status": "pending", "blockedBy": [5, 6, 7] }, - { "id": 9, "subject": "Task 9: Phase A gate — full gateway suite on macOS", "status": "pending", "blockedBy": [1, 2, 3, 4, 5, 6, 7, 8] }, - { "id": 10, "subject": "Task 10: Control-frame completion decoupling in WorkerFrameWriter", "status": "pending", "blockedBy": [9] }, - { "id": 11, "subject": "Task 11: Worker pipe-read teardown — dispose-to-unblock and observe", "status": "pending", "blockedBy": [9] }, - { "id": 12, "subject": "Task 12: Value-cache clone removal per the aliasing audit", "status": "pending", "blockedBy": [9] }, + { "id": 1, "subject": "Task 1: Windows-safe cleanup in SecretsStorePathGuardTests", "status": "completed" }, + { "id": 2, "subject": "Task 2: SessionEventDistributor _subscribers to plain Dictionary", "status": "completed" }, + { "id": 3, "subject": "Task 3: Merge the session event-source pass-through iterator", "status": "completed" }, + { "id": 4, "subject": "Task 4: EventStreamService direct channel reads in the live loop", "status": "completed" }, + { "id": 5, "subject": "Task 5: In-process dashboard snapshot feed + page switch", "status": "completed" }, + { "id": 6, "subject": "Task 6: In-process session event subscription + SessionDetailsPage switch", "status": "completed" }, + { "id": 7, "subject": "Task 7: AlarmsPage provider-status via IGatewayAlarmService", "status": "completed" }, + { "id": 8, "subject": "Task 8: Dashboard design-doc update (consolidated)", "status": "completed", "blockedBy": [5, 6, 7] }, + { "id": 9, "subject": "Task 9: Phase A gate — full gateway suite on macOS", "status": "completed", "blockedBy": [1, 2, 3, 4, 5, 6, 7, 8] }, + { "id": 10, "subject": "Task 10: Control-frame completion decoupling in WorkerFrameWriter", "status": "completed", "blockedBy": [9] }, + { "id": 11, "subject": "Task 11: Worker pipe-read teardown — dispose-to-unblock and observe", "status": "completed", "blockedBy": [9] }, + { "id": 12, "subject": "Task 12: Value-cache clone removal per the aliasing audit", "status": "completed", "blockedBy": [9] }, { "id": 13, "subject": "Task 13: Phase B gate — windev full verification", "status": "pending", "blockedBy": [10, 11, 12] }, { "id": 14, "subject": "Task 14: Wrap-up — deferred table closure, docs sweep, final review", "status": "pending", "blockedBy": [13] } ], From e1ff05c605da64b33a253b96bd574a15d32f8a4d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:33:28 -0400 Subject: [PATCH 19/23] docs: deferred-table closure, stale-comment sweep, as-built execution record --- docs/WorkerFrameProtocol.md | 6 +- docs/plans/2026-08-15-deferred-remediation.md | 64 +++++++++++++++++++ .../2026-08-15-perf-review-remediation.md | 5 ++ .../Sessions/SessionEventDistributor.cs | 12 ++-- .../Ipc/WorkerFrameWriter.cs | 4 +- 5 files changed, 83 insertions(+), 8 deletions(-) diff --git a/docs/WorkerFrameProtocol.md b/docs/WorkerFrameProtocol.md index 3bab2a3..e2f25b9 100644 --- a/docs/WorkerFrameProtocol.md +++ b/docs/WorkerFrameProtocol.md @@ -136,7 +136,11 @@ cost stays bounded: a pure-event pass — the event hot path — still pays exactly one flush however many frames drain together, a run of control frames still pays one for the whole run (never one per heartbeat, the syscall-per-frame cost the coalescing removed), and only a pass that actually -mixes both classes pays a second. +mixes both classes pays a second. Mixed passes are not exotic: command replies +travel at Control priority alongside heartbeats and faults, so a session under +sustained command traffic concurrent with event streaming can hit them +routinely. The cost stays bounded either way — one extra flush per class +transition present in the pass, not per frame. One consequence of the boundary flush is worth stating: a control frame whose run has already been flushed and completed is out of the drain's diff --git a/docs/plans/2026-08-15-deferred-remediation.md b/docs/plans/2026-08-15-deferred-remediation.md index e9dd38e..e1cf33e 100644 --- a/docs/plans/2026-08-15-deferred-remediation.md +++ b/docs/plans/2026-08-15-deferred-remediation.md @@ -308,3 +308,67 @@ Push the branch to origin, then on windev (`ssh windev`, clone `C:\build\mxacces | Structural alarm-truncation degraded-status signal | Contract-level design (proto change candidate) — separate effort. | | SEC-25 per-session dashboard event ACL | Security roadmap item; Task 6 deliberately preserves the current posture. | | `MxAccessWriteCompletionCache` clone | Different lifecycle than the value cache; consciously kept (Task 12.5). | + +--- + +## As-built notes (execution record) + +Where the delivered work differs from the task text above, or where the route to it +is worth keeping, this is the record. + +**Task 3 — `ReadEventsAsync` retained.** The method was not removed after +`MapWorkerEventsAsync` inlined the read-then-map chain: a second caller reaches it +through `ISessionManager.ReadEventsAsync`. That interface member itself has zero +production call sites — only test fakes implement and exercise it. Deleting it is a +mechanical but wide change (~15 test-fake touches), so it is recorded as a follow-up +rather than done here. + +**Task 5 — dashboard event feed, two review rounds.** Review caught two races that +the first cut did not have. First, subscription lifetime: subscriptions are now +generation-tagged, a generation ends at the time the fault is *observed* (not when it +is raised), `Reset` is scoped to the dying generation so it cannot cancel its +successor, and a backstop restart covers the case where no subscriber is left to +drive recovery. Second, `UnsubscribeAsync` needed a generation-scoped idle gate so a +teardown for an old generation cannot tear down the new one. Both fixes are pinned by +tests verified against mutations of the fixed code. + +**Task 6 — subscribe API placement.** The subscribe surface lives on +`IDashboardSessionEventSubscriber`, with DI forwarding to a single instance so every +consumer shares one feed. Batches that arrive for a session the renderer has already +moved off are dropped by a subscription identity check inside the renderer dispatch, +which is what makes a stale batch harmless rather than a cross-session leak. + +**Task 10 — the delivered property is the delivery point, not awaited latency.** The +spec asked for control-frame completion to be observable before the pass's event +writes. That is unachievable in the enqueue-then-contend shape: a caller that loses +the write-lock race does not run again until the winning drainer releases the lock, +so its `await` cannot return early no matter when its frame completes. What shipped +is the honest half: control frames are written *and flushed* at the class-transition +boundary, so the priority class governs the frame's delivery point rather than only +its byte order. Getting the awaited-latency win too requires unparking the lock-race +loser from the winner's pass — a change to the write-lock shape, recorded as a +follow-up. One extra `FlushFileBuffers` per mixed pass is the accepted cost. + +**Task 11 — teardown ordering and unconditional fault observation.** Teardown disposes +the session-owned transport first, then observes the read that dispose abandoned. +Fault observation is unconditional — a `ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted)` +continuation, so the budget that bounds the wait is diagnostics-only and can never be +the reason a fault goes unobserved. The same continuation covers heartbeat and drain +overrun. `DisposeTransportStream` is exception-total: no dispose path can throw out of +teardown. + +**Task 12 — three clones removed, plan rationale corrected in-code.** All three +`OnDataChange` value-cache clones are gone. The plan's stated reason for keeping the +`MxAccessWriteCompletionCache` clone ("different lifecycle than the value cache") is +wrong and was corrected where the code documents it: the clone is kept on +*provenance* grounds — the cached payload comes from a caller-supplied object the +worker does not own — not on lifecycle grounds. + +**Task 13 — windev verification.** Solution build 0 warnings / 0 errors after +clearing stale `Contracts` `obj` artifacts (an infrastructure problem on the box, not +a regression from this branch). Worker x86: 509/509 (+8 new). Gateway: 1059/1059, +including `SecretsStorePathGuardTests` — the first fully green Windows gateway run, +that suite having been red before this branch. One load flake +(`InvokeAsync_WhenWorkerHandshakingThenReadyWithinTimeout_Succeeds`) passed in +isolation and on re-run, consistent with the load-sensitivity caveat documented in +`docs/GatewayTesting.md`. diff --git a/docs/plans/2026-08-15-perf-review-remediation.md b/docs/plans/2026-08-15-perf-review-remediation.md index 5b20b1c..83a975e 100644 --- a/docs/plans/2026-08-15-perf-review-remediation.md +++ b/docs/plans/2026-08-15-perf-review-remediation.md @@ -619,6 +619,11 @@ Commit anything found: `docs: remediation plan doc sweep` | Event-path triple async-iterator flattening | LOW-rated; touches the most invariant-dense code in the gateway for two `MoveNextAsync` hops per event. Reconsider after Tasks 3/4 land and if profiling still shows it. | | `SessionEventDistributor._subscribers` `ConcurrentDictionary` → plain `Dictionary` (Task 25 / Task 3 review) | Every mutation is already inside `_lifecycleLock`, so the concurrent type buys nothing. Behavior-neutral refactor with no measurable win, proposed after the windev gate was already green — not worth re-running the verification matrix for. Comment cleanups from the same review landed; this swap did not. | +All six rows above were resolved on 2026-08-15 by the follow-up plan +[`docs/plans/2026-08-15-deferred-remediation.md`](2026-08-15-deferred-remediation.md) +(branch `perf/deferred-remediation`), which also fixed the pre-existing Windows +`SecretsStorePathGuardTests` failure. + ## Execution notes for the orchestrator - Branch: `git checkout -b perf/review-remediation` before Task 1. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs index e2bc100..04212ab 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs @@ -56,9 +56,9 @@ public delegate void SubscriberOverflowHandler(bool isOnlySubscriber, bool isInt /// producing an /// of already-mapped public /// s, given a . This is the -/// cleanest seam: it can pass -/// ct => session.ReadEventsAsync(ct).Select(mapper.MapEvent) (or a -/// channel reader's ReadAllAsync), while unit tests pass a plain +/// cleanest seam: production passes GatewaySession.MapWorkerEventsAsync, +/// which reads the worker event channel and maps each frame in one iterator +/// (inlining what used to be a read-then-Select chain), while unit tests pass a plain /// channel reader's ReadAllAsync with no real session. The pump owns the /// single consumption of this enumerable; fan-out happens on the public /// after mapping, mirroring today's @@ -751,9 +751,9 @@ public sealed class SessionEventDistributor : IAsyncDisposable } // Disconnect ONLY this subscriber: it is already out of the fan-out set (removed above), - // so complete its channel with the overflow fault. Its gRPC reader's MoveNextAsync then - // throws the SessionManagerException, which EventStreamService surfaces to the client - // exactly as the pre-epic per-RPC overflow did. The pump and every other subscriber are + // so complete its channel with the overflow fault. EventStreamService consumes the channel + // directly, so its next WaitToReadAsync throws the SessionManagerException, which it surfaces + // to the client exactly as the pre-epic per-RPC overflow did. The pump and every other subscriber are // untouched. This runs even when the handler above threw — the subscriber must never be // left attached with an un-completed channel. subscriber.Channel.Writer.TryComplete(new SessionManagerException( diff --git a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs index 61db4c7..d94b118 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs @@ -179,7 +179,9 @@ public sealed class WorkerFrameWriter /// remaining events are written, so a batch in flight does not delay a control frame's delivery. /// Every frame's "written and flushed before completion" contract is unchanged. An event batch that /// a control frame cuts into therefore pays one extra flush; an uninterrupted batch still pays - /// exactly one. + /// exactly one. Mixed passes are not exotic — command replies are Control priority too, so + /// sustained command traffic concurrent with event streaming can hit them routinely; the cost + /// stays bounded at one extra flush per class transition present in the pass. /// /// Envelopes to write, in order. /// Scheduling priority for the whole batch. From 3faa272db9449926b32c4b42b857365ecfb02ef6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 16 Aug 2026 04:12:25 -0400 Subject: [PATCH 20/23] fix(dashboard): bounded alarm drains, feed resubscribe, live pill, drop dead hub factory; doc corrections --- docs/GatewayConfiguration.md | 24 ++--- docs/GatewayDashboardDesign.md | 62 ++++++++++--- gateway.md | 18 ++-- .../Dashboard/Components/DashboardPageBase.cs | 87 ++++++++++++++----- .../Components/Pages/AlarmsPage.razor | 52 ++++++----- .../Components/Pages/SessionDetailsPage.razor | 46 ++++++++++ .../DashboardServiceCollectionExtensions.cs | 4 +- .../Dashboard/HubTokenService.cs | 17 ++-- .../Hubs/DashboardHubConnectionFactory.cs | 39 --------- .../ZB.MOM.WW.MxGateway.Server.csproj | 1 - .../DashboardHubsRegistrationTests.cs | 17 ++-- 11 files changed, 240 insertions(+), 127 deletions(-) delete mode 100644 src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardHubConnectionFactory.cs diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index 5feb37a..210454f 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -215,24 +215,28 @@ Three authorization policies are registered out of these options: ### SignalR hubs -When the dashboard is enabled, three hubs are mapped under `/hubs/*`: +When the dashboard is enabled, three hubs are mapped under `/hubs/*`. They are +the **remote** surface — for clients outside the gateway process. Server-rendered +pages do not use them: a page runs in this process and reads the producing +services through in-process seams (`IDashboardSnapshotFeed`, +`IDashboardSessionEventSubscriber`, `IGatewayAlarmService`) rather than opening a +loopback WebSocket back into its own heap. - `GET /hubs/snapshot` — pushes `DashboardSnapshot` whenever the snapshot - service produces a new one. Drives every page that inherits - `DashboardPageBase`; replaces the earlier polling loop. + service produces a new one. Idle-gated on connected clients, so it stays + dormant unless a remote client connects. - `GET /hubs/alarms` — re-broadcasts the `AlarmFeedMessage` stream from the central alarm monitor to all connected clients (group `__alarms__`). - `GET /hubs/events` — per-session MxEvent feed. Clients call `SubscribeSession(sessionId)` to join `session:{id}`. Events are mirrored - from the corresponding gRPC `StreamEvents` call as a fire-and-forget - side-effect; the dashboard only sees events while a gRPC client is also - subscribed to that session. + from the session's own event distributor, gated on `EventsHubViewerRegistry` + so an unwatched session pays nothing. `GET /hubs/token` (cookie-only) mints a 5-minute data-protected bearer -token for the calling user; the Blazor pages use it via -`DashboardHubConnectionFactory` to authenticate the SignalR connection. -The factory refreshes the token on every (re)connect, so the short lifetime -(SEC-05) is transparent to clients. The token is not server-side revocable; +token for the calling user, so a remote hub client can authenticate the +SignalR connection without forwarding the HttpOnly dashboard cookie. Such a +client is expected to re-fetch on every (re)connect, which makes the short +lifetime (SEC-05) transparent. The token is not server-side revocable; its short lifetime bounds exposure of a captured token (see [GatewayDashboardDesign](./GatewayDashboardDesign.md)). diff --git a/docs/GatewayDashboardDesign.md b/docs/GatewayDashboardDesign.md index 97500d3..ea16c5d 100644 --- a/docs/GatewayDashboardDesign.md +++ b/docs/GatewayDashboardDesign.md @@ -118,8 +118,11 @@ so it consumes the producing services directly through in-process seams — `IDashboardSnapshotFeed`, `IDashboardSessionEventSubscriber`, and `IGatewayAlarmService` — instead of opening a loopback WebSocket back into its own heap. `DashboardHubConnectionFactory`, the helper a circuit used to open -those connections, stays registered for out-of-tree consumers, but no in-repo -page resolves it. +those connections, has been deleted along with the `Microsoft.AspNetCore.SignalR.Client` +package reference: nothing in this process dials a hub, and a registered-but-unused +client factory only invites a page to reintroduce the loopback. Remote consumers +build their own connection; the hubs, `/hubs/token`, and `HubTokenService` remain +for them. ## Dashboard Data Source @@ -195,7 +198,27 @@ instead of buffering without bound or stalling the pump. `DashboardPageBase` seeds `Snapshot` synchronously from `IDashboardSnapshotService.GetSnapshot()` in `OnInitializedAsync` so the first render is non-empty, then calls `InvokeAsync(StateHasChanged)` for every snapshot -the feed yields. On dispose it cancels the watch and waits at most **5 seconds** +the feed yields. + +A subscription is not a lifetime, so the watch is a loop, not a single enumeration: +the feed detaches its subscribers whenever a pump's source faults or completes, and +a page that treated that as terminal would sit on its last snapshot until the +operator navigated. On any end other than its own cancellation the page waits one +second — honouring its own token, so teardown is not delayed — and resubscribes. The +fault is logged at Warning once per fault *transition*, not once per retry: a feed +that is down stays down for many iterations, and one line per second per open page +is noise. The last rendered snapshot stays on screen throughout, and the next page +load still seeds from `IDashboardSnapshotService.GetSnapshot()`. + +That resubscribe is also the feed's primary recovery path, not just the page's: +only a subscriber that finds no live generation starts a pump, so a page coming back +is what restarts the enumeration. `Reset`'s belt-and-braces restart — if subscribers +of *other* generations are still attached when a generation dies, it starts a fresh +pump for them and re-tags them — remains the backstop for the case where no +subscriber is left to drive recovery, but it is no longer the only thing standing +between a faulted feed and a permanently stale page. + +On dispose the page cancels the watch and waits at most **5 seconds** for the loop to drain, logging a warning on timeout. The bound is deliberate: the loop marshals renders through the renderer's dispatcher and disposal can run on that same dispatcher, so an unconditional wait would hang on a wedged dispatcher. @@ -214,6 +237,19 @@ Detaching cancels the pump, disposes the subscription (which releases the viewer registration and completes the channel, so the pump has an exit even if cancellation is missed), then drains under its own timeout. +The page's live/offline pill tracks that pump rather than a connection: it is set +live on attach and cleared when the pump exits, through the same dispatcher-owned +identity check the render batch uses, so a stale pump cannot darken the pill of the +subscription that replaced it. Because detach clears the subscription field before +cancelling, a detach-driven exit leaves the pill to the incoming subscription; what +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 each 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. + ### SignalR hubs (remote clients) Updates for out-of-process clients flow over three SignalR hubs, all guarded by the @@ -329,9 +365,14 @@ ordering — register before becoming a delivery target, deregister after ceasin be one — so the widest a race window opens is a redaction clone that reaches nobody, never a dropped event that was owed to a live viewer. -Redaction happens once per event, not once per audience: `Publish` produces a -single redacted clone and hands that same instance to the in-process subscribers -and to the hub group. In-process delivery runs first and synchronously — it cannot +Redaction happens once per event, not once per audience: with +`Dashboard:ShowTagValues` false (the default) `Publish` produces a single redacted +clone and hands that same instance to the in-process subscribers and to the hub +group. With `ShowTagValues` true there is no clone at all — the original `MxEvent` +instance is handed to both audiences — so the "one clone per event" cost holds only +in the redacting configuration, and in the value-showing one both audiences share a +reference to the session pipeline's own event object. In-process delivery runs first +and synchronously — it cannot throw, and it must not be skipped by the guard clause around the hub send — into per-subscriber bounded drop-oldest channels, so a page that falls behind loses its oldest queued events rather than blocking the session's event pipeline. @@ -715,11 +756,10 @@ dashboard mints short-lived bearer tokens for the connection: 5. The hubs' `[Authorize(Policy = HubClientsPolicy)]` accepts the resulting identity. -`DashboardHubConnectionFactory` (scoped to the Blazor circuit) wraps the -HubConnectionBuilder and supplies a fresh token via `AccessTokenProvider` on -every (re)connect, so the short 5-minute lifetime is transparent to whoever uses -it. It remains registered, but no in-repo page opens a hub connection any more; -external clients implement the equivalent refresh themselves. +There is no in-repo hub client: the helper that once wrapped `HubConnectionBuilder` +for a circuit was deleted when the pages moved to the in-process seams. An external +client re-fetches `/hubs/token` on every (re)connect itself, which is what makes the +short 5-minute lifetime transparent. Caveat — logout does not revoke outstanding tokens. Logout clears the dashboard cookie, but hub bearer tokens are self-contained, data-protection-encrypted, and diff --git a/gateway.md b/gateway.md index 5ab5d27..1105639 100644 --- a/gateway.md +++ b/gateway.md @@ -117,13 +117,17 @@ project without binding to a metrics exporter. effective configuration into immutable DTOs for read-only dashboard rendering. The Blazor Server dashboard mounts at the host root and renders those snapshots at `/`, `/sessions`, `/workers`, `/events`, `/galaxy`, `/alarms`, `/apikeys`, -and `/settings`. Pages connect to `/hubs/snapshot` (a SignalR hub published by -`DashboardSnapshotPublisher`) and refresh on every push instead of polling. -`/hubs/alarms` broadcasts `AlarmFeedMessage` values from the central alarm -monitor; `/hubs/events` mirrors per-session `MxEvent` traffic from -`EventStreamService` to clients subscribed to `session:{id}`. The dashboard -uses local Bootstrap CSS and JavaScript plus a small local stylesheet; it does -not use a Blazor UI component library. +and `/settings`. Pages run inside this process, so they consume the producing +services directly through in-process seams — `IDashboardSnapshotFeed`, +`IDashboardSessionEventSubscriber`, and `IGatewayAlarmService` — and re-render on +every update instead of polling. The three SignalR hubs are the **remote** +surface, for clients outside the gateway process: `/hubs/snapshot` pushes +`DashboardSnapshot` from `DashboardSnapshotPublisher`, `/hubs/alarms` broadcasts +`AlarmFeedMessage` values from the central alarm monitor, and `/hubs/events` +mirrors per-session `MxEvent` traffic to clients subscribed to `session:{id}`. +No in-repo page opens a hub connection. The dashboard uses local Bootstrap CSS +and JavaScript plus a small local stylesheet; it does not use a Blazor UI +component library. `/browse` walks the `IGalaxyHierarchyCache` tree and reads subscribed tag values live through `IDashboardLiveDataService`, which owns one shared, diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs index ca13526..f9e7a3a 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs @@ -21,6 +21,13 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable /// private static readonly TimeSpan WatchDrainTimeout = TimeSpan.FromSeconds(5); + /// + /// Delay between a feed subscription ending and the resubscribe that replaces it. + /// Long enough that a feed failing on every attempt cannot spin, short enough that + /// the page is stale for about a snapshot interval rather than until navigation. + /// + private static readonly TimeSpan ResubscribeDelay = TimeSpan.FromSeconds(1); + private readonly CancellationTokenSource _watchCancellation = new(); private Task? _watchTask; @@ -88,31 +95,71 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable GC.SuppressFinalize(this); } + /// + /// Renders every snapshot the feed yields, resubscribing whenever the subscription ends + /// for any reason other than this page going away. + /// + /// + /// The feed detaches a subscriber when its pump's source faults or completes, so a single + /// enumeration is not a lifetime: without the outer loop the first fault froze the page on + /// its last snapshot until the operator navigated. Resubscribing is also what restarts the + /// feed — only a subscriber that finds no live generation starts a pump — so the page is + /// the recovery path, not merely its beneficiary. + /// + /// Cancelled by when the page goes away. + /// A task that completes when the page is disposed. private async Task WatchSnapshotsAsync(CancellationToken cancellationToken) { - try + // One log line per fault *transition*, not per retry: a feed that is down stays down + // for many iterations, and a warning per second per open page is noise, not signal. + bool faultLogged = false; + + while (!cancellationToken.IsCancellationRequested) { - await foreach (DashboardSnapshot snapshot in SnapshotFeed - .WatchAsync(cancellationToken) - .ConfigureAwait(false)) + try { - Snapshot = snapshot; - await InvokeAsync(StateHasChanged).ConfigureAwait(false); + await foreach (DashboardSnapshot snapshot in SnapshotFeed + .WatchAsync(cancellationToken) + .ConfigureAwait(false)) + { + Snapshot = snapshot; + faultLogged = false; + await InvokeAsync(StateHasChanged).ConfigureAwait(false); + } + } + catch (OperationCanceledException) + { + // The page is going away. + return; + } + catch (Exception error) when (!faultLogged) + { + // The last rendered snapshot stays on screen while the retry runs, and the + // snapshot service keeps serving GetSnapshot() for the next page load. + faultLogged = true; + Logger?.LogWarning( + error, + "Live snapshot updates failed for dashboard page {Page}; retrying every {Delay}. " + + "It keeps the last rendered snapshot until they resume.", + GetType().Name, + ResubscribeDelay); + } + catch (Exception) + { + // Same fault, already reported above; the retry below is unconditional. + } + + // The enumeration's own disposal (run by the await foreach on every exit path) + // is what releases the dead subscription, so the delay below is only paced — + // there is nothing left of the old subscription to unwind here. + try + { + await Task.Delay(ResubscribeDelay, cancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + return; } - } - catch (OperationCanceledException) - { - // The page is going away. - } - catch (Exception error) - { - // The feed is best-effort: the last rendered snapshot stays on screen and the - // snapshot service keeps serving GetSnapshot() for the next page load. Logged - // once here, on the way out of the loop — never per snapshot. - Logger?.LogWarning( - error, - "Live snapshot updates ended for dashboard page {Page}; it keeps the last rendered snapshot.", - GetType().Name); } } } 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 0110b90..86d2838 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 @@ -168,6 +168,13 @@ private int _maxSeverity = 1000; private string _search = string.Empty; + // Upper bound on waiting for either background loop while disposing, mirroring + // DashboardPageBase's snapshot-watch drain: both loops marshal renders through the + // renderer's dispatcher and disposal can run on that same dispatcher, so the wait is + // bounded rather than unconditional — an unconditional one hangs teardown for good on + // a wedged dispatcher. + private static readonly TimeSpan LoopDrainTimeout = TimeSpan.FromSeconds(5); + private readonly CancellationTokenSource _cts = new(); private Task? _pollTask; @@ -336,29 +343,32 @@ { await _cts.CancelAsync(); - if (_pollTask is not null) - { - try - { - await _pollTask; - } - catch (OperationCanceledException) - { - } - } - - if (_providerStatusTask is not null) - { - try - { - await _providerStatusTask; - } - catch (OperationCanceledException) - { - } - } + await DrainAsync(_pollTask); + await DrainAsync(_providerStatusTask); _cts.Dispose(); GC.SuppressFinalize(this); } + + // The accepted cost of the bound is an abandoned loop that keeps its alarm-service + // subscription (and its poll timer) until it does unwind; the alternative — waiting + // forever on a dispatcher that is not draining — wedges the circuit teardown itself. + private static async Task DrainAsync(Task? loop) + { + if (loop is null) + { + return; + } + + try + { + await loop.WaitAsync(LoopDrainTimeout); + } + catch (TimeoutException) + { + } + catch (OperationCanceledException) + { + } + } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor index aebe8f4..770af52 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor @@ -1,5 +1,9 @@ @page "/sessions/{SessionId}" @inherits DashboardPageBase +@* Load-bearing: DisposeAsync below hides the base method with `new`, so Blazor only calls + it because this directive re-declares IAsyncDisposable on the derived component. Drop + this line and the base's DisposeAsync runs instead — the event subscription and pump + leak, silently. *@ @implements IAsyncDisposable @using ZB.MOM.WW.MxGateway.Contracts.Proto @using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs @@ -358,6 +362,43 @@ else // Either the renderer went away mid-dispatch, or the drain below timed out // and disposed the cancellation source this loop is still reading. } + finally + { + await MarkDisconnectedAsync(subscription).ConfigureAwait(false); + } + } + + // This pump is the only thing feeding the "live" pill, so the pill goes dark the moment + // the pump stops — the subscription's channel completing under a page that is still + // watching (the broadcaster dropped it, the session ended) is exactly what the pill + // exists to show, and without this it read "live" until navigation. + // A pump whose subscription has already been replaced must not touch it: the newer + // subscription's pump owns the pill now. Same dispatcher-owned identity check the + // render batch uses — and detach nulls _eventSubscription before cancelling, so a + // detach-driven exit correctly falls through here without repainting. + private async Task MarkDisconnectedAsync(IDashboardEventSubscription subscription) + { + try + { + await InvokeAsync(() => + { + if (!ReferenceEquals(_eventSubscription, subscription)) + { + return; + } + + _eventsConnected = false; + StateHasChanged(); + }).ConfigureAwait(false); + } + catch (ObjectDisposedException) + { + // The renderer went away; there is no pill left to repaint. + } + catch (OperationCanceledException) + { + // The circuit is tearing down; same. + } } private async Task DetachEventsAsync() @@ -401,6 +442,11 @@ else : string.Empty; } + // `new` hides DashboardPageBase.DisposeAsync rather than overriding it (the base method + // is not virtual), so this runs only via the IAsyncDisposable interface slot the + // `@implements IAsyncDisposable` directive at the top of this file re-declares on the + // derived type. Remove either half and disposal silently resolves to the base method: + // the snapshot watch is cancelled, the event subscription and pump are not. public new async ValueTask DisposeAsync() { await DetachEventsAsync(); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs index e80508b..83ef2d1 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs @@ -45,8 +45,10 @@ public static class DashboardServiceCollectionExtensions services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); + // Singleton, and the only consumer scope left is HubTokenAuthenticationHandler plus + // the /hubs/token endpoint: server-rendered pages read the in-process feeds, so + // nothing in this process builds a hub connection or needs a token for one. services.AddSingleton(); - services.AddScoped(); services.AddScoped(); // Singleton: EventsHub instances are transient (one per hub invocation), so the // subscriber bookkeeping they share with the broadcaster must outlive them. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs index bcc5842..1ea957b 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/HubTokenService.cs @@ -14,10 +14,12 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard; /// /// This service is registered as a singleton in /// and -/// is shared by two consumer scopes: DashboardHubConnectionFactory -/// (scoped, per-circuit; calls from the cookie-authenticated -/// dashboard) and HubTokenAuthenticationHandler (transient, per-request; -/// calls from the SignalR negotiate / connection path). +/// is shared by two consumer scopes: the /hubs/token endpoint (calls +/// for a cookie-authenticated caller) and +/// HubTokenAuthenticationHandler (transient, per-request; calls +/// from the SignalR negotiate / connection path). Both +/// serve external/remote hub consumers — server-rendered dashboard pages read the +/// in-process feeds and never mint a hub token. /// The underlying is thread-safe, so /// minting and validating concurrently from any number of callers is safe; /// future maintainers should preserve the singleton lifetime to keep the @@ -31,9 +33,10 @@ public sealed class HubTokenService // revocable. A short lifetime bounds the exposure window of a token captured from a proxy // or log after logout (the cookie is cleared on logout, but outstanding tokens are not), and // bounds how long a stale role set survives a role change. Five minutes is transparent to - // clients because DashboardHubConnectionFactory mints a fresh token on every (re)connect; - // see docs/GatewayDashboardDesign.md. Heavier jti-denylist revocation is deliberately - // deferred until per-session hub ACLs land, when tokens gain session binding. + // clients that re-fetch from /hubs/token on every (re)connect, which is what a remote hub + // consumer is expected to do; see docs/GatewayDashboardDesign.md. Heavier jti-denylist + // revocation is deliberately deferred until per-session hub ACLs land, when tokens gain + // session binding. internal static readonly TimeSpan TokenLifetime = TimeSpan.FromMinutes(5); private readonly ITimeLimitedDataProtector _protector; diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardHubConnectionFactory.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardHubConnectionFactory.cs deleted file mode 100644 index ae78309..0000000 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardHubConnectionFactory.cs +++ /dev/null @@ -1,39 +0,0 @@ -using Microsoft.AspNetCore.Components; -using Microsoft.AspNetCore.Components.Authorization; -using Microsoft.AspNetCore.SignalR.Client; - -namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; - -/// -/// Client-side helper that builds a targeted at a -/// dashboard hub. Mints a fresh data-protected bearer token via -/// on every (re)connect so the connection -/// authenticates against -/// without needing to forward the browser's HttpOnly cookie. -/// -public sealed class DashboardHubConnectionFactory( - NavigationManager navigation, - HubTokenService tokens, - AuthenticationStateProvider authState) -{ - /// Creates a new hub connection to the specified hub path. - /// The relative hub path (e.g., "/hubs/snapshot"). - /// A configured hub connection with automatic reconnection and token authentication. - public HubConnection Create(string hubPath) - { - ArgumentException.ThrowIfNullOrWhiteSpace(hubPath); - - Uri hubUrl = navigation.ToAbsoluteUri(hubPath); - return new HubConnectionBuilder() - .WithUrl(hubUrl, options => - { - options.AccessTokenProvider = async () => - { - AuthenticationState state = await authState.GetAuthenticationStateAsync().ConfigureAwait(false); - return tokens.Issue(state.User); - }; - }) - .WithAutomaticReconnect() - .Build(); - } -} diff --git a/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj b/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj index c35f7c1..bfdf761 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj +++ b/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj @@ -27,7 +27,6 @@ - diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs index f6af323..a2f9cb2 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardHubsRegistrationTests.cs @@ -27,23 +27,20 @@ public sealed class DashboardHubsRegistrationTests endpoint.Metadata.GetMetadata()?.EndpointName == "DashboardHubToken"); } - /// Verifies that dashboard build registers hub token service and connection factory. + /// + /// Verifies that dashboard build registers the hub token service. It is a singleton + /// shared by the /hubs/token endpoint and HubTokenAuthenticationHandler; + /// there is deliberately no client-side hub-connection factory to resolve, because + /// server-rendered pages read the in-process feeds instead of dialling their own hubs. + /// /// A task that represents the asynchronous operation. [Fact] - public async Task Build_WhenDashboardEnabled_RegistersHubTokenServiceAndConnectionFactory() + public async Task Build_WhenDashboardEnabled_RegistersHubTokenService() { await using WebApplication app = GatewayApplication.Build([]); - // HubTokenService is singleton; DashboardHubConnectionFactory is scoped - // (it captures NavigationManager and AuthenticationStateProvider which - // are themselves per-circuit). HubTokenService tokens = app.Services.GetRequiredService(); Assert.NotNull(tokens); - - using IServiceScope scope = app.Services.CreateScope(); - DashboardHubConnectionFactory factory = scope.ServiceProvider - .GetRequiredService(); - Assert.NotNull(factory); } /// From 7cfb2e727da1b33da53e9135934a5182ebd2d833 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 16 Aug 2026 04:20:59 -0400 Subject: [PATCH 21/23] fix(dashboard): parallel bounded alarm drains, best-effort disposal catch-alls, ConfigureAwait alignment --- docs/GatewayDashboardDesign.md | 6 +++-- .../Components/Pages/AlarmsPage.razor | 23 +++++++++++++++---- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/docs/GatewayDashboardDesign.md b/docs/GatewayDashboardDesign.md index ea16c5d..10e2152 100644 --- a/docs/GatewayDashboardDesign.md +++ b/docs/GatewayDashboardDesign.md @@ -246,9 +246,11 @@ the pill therefore reports is the case it exists for — the channel completing 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 each drain at 5 seconds on dispose, for the same reason +badge) 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. +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 +blocks both loops at once, and draining them in sequence would time out twice. ### SignalR hubs (remote clients) 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 86d2838..0b1825c 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 @@ -323,6 +323,14 @@ catch (OperationCanceledException) { } + catch + { + // Catch-all for the same reason ProviderStatusLoopAsync has one: a teardown race + // can fault InvokeAsync (a disposed renderer) after cancellation has already been + // requested. Letting that fault the task would surface it out of the drain in + // DisposeAsync, skipping _cts.Dispose(). The loop ends here and the page holds its + // last rendered rows — it is being disposed or has nothing left to poll with. + } } private async Task RefreshAlarmsAsync() @@ -341,10 +349,13 @@ /// public async ValueTask DisposeAsync() { - await _cts.CancelAsync(); + await _cts.CancelAsync().ConfigureAwait(false); - await DrainAsync(_pollTask); - await DrainAsync(_providerStatusTask); + // 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)) + .ConfigureAwait(false); _cts.Dispose(); GC.SuppressFinalize(this); @@ -362,7 +373,7 @@ try { - await loop.WaitAsync(LoopDrainTimeout); + await loop.WaitAsync(LoopDrainTimeout).ConfigureAwait(false); } catch (TimeoutException) { @@ -370,5 +381,9 @@ catch (OperationCanceledException) { } + catch + { + // Other disposal-time errors are best-effort. + } } } From a37c304f265d62854fa4c171057cb29b315e1f39 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 16 Aug 2026 04:22:34 -0400 Subject: [PATCH 22/23] chore(plans): tasks 13-14 complete; as-built note on the hub-connection-factory removal --- docs/plans/2026-08-15-deferred-remediation.md | 2 +- docs/plans/2026-08-15-deferred-remediation.md.tasks.json | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/plans/2026-08-15-deferred-remediation.md b/docs/plans/2026-08-15-deferred-remediation.md index e1cf33e..fb42808 100644 --- a/docs/plans/2026-08-15-deferred-remediation.md +++ b/docs/plans/2026-08-15-deferred-remediation.md @@ -130,7 +130,7 @@ 1. `IDashboardSnapshotFeed` (singleton): `IAsyncEnumerable WatchAsync(CancellationToken ct)`. Internally: per-subscriber `Channel` with capacity 1 and `BoundedChannelFullMode.DropOldest` (a dashboard viewer only ever wants the latest snapshot; a slow circuit must never buffer unboundedly or stall others). 2. **Idle gating (the invariant this task must not lose):** the feed enumerates `IDashboardSnapshotService.WatchSnapshotsAsync` on a background task started when the subscriber count goes 0→1 and cancelled when it goes 1→0. While zero subscribers, the feed holds no timer and builds no snapshot. Guard subscriber add/remove with a plain lock; restart cleanly on resubscribe (mirror the start/stop discipline of `GatewayAlarmMonitor.StreamAsync` registration, `GatewayAlarmMonitor.cs:739-752`). If the underlying watch throws or completes, complete all subscriber channels with the error and reset so the next subscriber restarts it (mirror `DashboardSnapshotPublisher.ExecuteAsync`'s reconnect-after-delay posture, but per-feed). 3. `DashboardPageBase`: remove the HubConnection path (`:62` and the factory usage); keep the synchronous first render via `snapshotService.GetSnapshot()` (`:37`); then a background loop `await foreach (var s in feed.WatchAsync(_cts.Token)) { Snapshot = s; await InvokeAsync(StateHasChanged); }` started in `OnAfterRenderAsync(firstRender)` or `OnInitializedAsync` (match current lifecycle), cancelled + awaited in `DisposeAsync`. Update the class XML doc that narrates the hub subscription history (`:7-14`). -4. Hubs, `DashboardSnapshotPublisher`, `DashboardSnapshotHubConnectionCounter`, `DashboardHubConnectionFactory`, and `/hubs/token` all stay — they remain the remote/external surface. Do not touch them. +4. Hubs, `DashboardSnapshotPublisher`, `DashboardSnapshotHubConnectionCounter`, `DashboardHubConnectionFactory`, and `/hubs/token` all stay — they remain the remote/external surface. Do not touch them. *(As-built deviation, integration-fix commit: once the pages stopped using it, `DashboardHubConnectionFactory` had zero consumers, so the final integration review had it deleted along with the `Microsoft.AspNetCore.SignalR.Client` package reference. Hubs, publisher, counter, and `/hubs/token` remain the remote surface as specified.)* 5. Auth: the pages are mapped behind `ViewerPolicy` (`DashboardEndpointRouteBuilderExtensions.cs:136`), which remains the gate for in-process consumption; add one comment on `WatchAsync` saying so. 6. Tests (`DashboardSnapshotFeedTests`): (a) zero subscribers → underlying service's `WatchSnapshotsAsync` never enumerated (fake service counts enumerations/`MoveNextAsync`); (b) first subscriber starts exactly one enumeration; two subscribers share it; (c) last unsubscribe cancels it; resubscribe restarts it; (d) slow subscriber observes latest-wins (push 3 snapshots, read 1, it is the newest) while a fast subscriber sees all; (e) underlying fault completes subscribers with the error and a fresh subscriber restarts. diff --git a/docs/plans/2026-08-15-deferred-remediation.md.tasks.json b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json index 34f10c3..e399fa5 100644 --- a/docs/plans/2026-08-15-deferred-remediation.md.tasks.json +++ b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json @@ -13,8 +13,8 @@ { "id": 10, "subject": "Task 10: Control-frame completion decoupling in WorkerFrameWriter", "status": "completed", "blockedBy": [9] }, { "id": 11, "subject": "Task 11: Worker pipe-read teardown — dispose-to-unblock and observe", "status": "completed", "blockedBy": [9] }, { "id": 12, "subject": "Task 12: Value-cache clone removal per the aliasing audit", "status": "completed", "blockedBy": [9] }, - { "id": 13, "subject": "Task 13: Phase B gate — windev full verification", "status": "pending", "blockedBy": [10, 11, 12] }, - { "id": 14, "subject": "Task 14: Wrap-up — deferred table closure, docs sweep, final review", "status": "pending", "blockedBy": [13] } + { "id": 13, "subject": "Task 13: Phase B gate — windev full verification", "status": "completed", "blockedBy": [10, 11, 12] }, + { "id": 14, "subject": "Task 14: Wrap-up — deferred table closure, docs sweep, final review", "status": "completed", "blockedBy": [13] } ], - "lastUpdated": "2026-08-15T00:00:00Z" + "lastUpdated": "2026-08-16T00:00:00Z" } From dcbec978ee06ef707e7d53b3cd831598ed443d3d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 16 Aug 2026 04:32:54 -0400 Subject: [PATCH 23/23] fix(dashboard): alarm poll loop retries through faults and surfaces them; correct stale session-events empty-state copy --- docs/GatewayDashboardDesign.md | 12 ++++ .../Components/Pages/AlarmsPage.razor | 69 ++++++++++++++++--- .../Components/Pages/SessionDetailsPage.razor | 5 +- 3 files changed, 74 insertions(+), 12 deletions(-) diff --git a/docs/GatewayDashboardDesign.md b/docs/GatewayDashboardDesign.md index 10e2152..798e018 100644 --- a/docs/GatewayDashboardDesign.md +++ b/docs/GatewayDashboardDesign.md @@ -252,6 +252,18 @@ 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 blocks both loops at once, and draining them in sequence would time out twice. +Both loops handle faults *inside* the loop and retry: only cancellation ends them. +A failing alarm query or a render that faults on one tick leaves the page's last +rows in place and is retried on the next tick — a fault that stopped polling for the +life of the page would leave stale rows behind with nothing to say so. The poll loop +also surfaces the fault in the same `Alarm query failed` banner that a query error +uses, and clears it on the first tick that succeeds; that dispatch is itself +best-effort, because the fault being reported may be an `InvokeAsync` against a +disposed renderer, in which case reporting fails the same way and the loop simply +exits on its next cancellation check. Neither loop method can fault, which is what +the bounded drain relies on — it must never observe a faulted loop task, since that +would surface out of `DisposeAsync` and skip the `CancellationTokenSource` dispose. + ### SignalR hubs (remote clients) Updates for out-of-process clients flow over three SignalR hubs, all guarded by 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 0b1825c..634b4ba 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 @@ -309,27 +309,76 @@ }; } + // Fault handling sits inside the loop, matching ProviderStatusLoopAsync: 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 + // fault, which is what DrainAsync in DisposeAsync relies on. private async Task PollLoopAsync() + { + if (!await PollOnceAsync().ConfigureAwait(false)) + { + return; + } + + using PeriodicTimer timer = new(TimeSpan.FromSeconds(3)); + while (true) + { + try + { + if (!await timer.WaitForNextTickAsync(_cts.Token).ConfigureAwait(false)) + { + return; + } + } + catch (OperationCanceledException) + { + return; + } + + if (!await PollOnceAsync().ConfigureAwait(false)) + { + return; + } + } + } + + // Returns false only when cancellation has ended the poll; a non-cancellation fault returns + // true so the caller waits for the next tick and tries again. + private async Task PollOnceAsync() { try { await InvokeAsync(RefreshAlarmsAsync).ConfigureAwait(false); - using PeriodicTimer timer = new(TimeSpan.FromSeconds(3)); - while (await timer.WaitForNextTickAsync(_cts.Token).ConfigureAwait(false)) - { - await InvokeAsync(RefreshAlarmsAsync).ConfigureAwait(false); - } + return true; } catch (OperationCanceledException) { + return false; + } + catch (Exception ex) + { + await ReportPollFaultAsync(ex).ConfigureAwait(false); + return true; + } + } + + private async Task ReportPollFaultAsync(Exception fault) + { + try + { + await InvokeAsync(() => + { + _queryError = fault.Message; + StateHasChanged(); + }).ConfigureAwait(false); } catch { - // Catch-all for the same reason ProviderStatusLoopAsync has one: a teardown race - // can fault InvokeAsync (a disposed renderer) after cancellation has already been - // requested. Letting that fault the task would surface it out of the drain in - // DisposeAsync, skipping _cts.Dispose(). The loop ends here and the page holds its - // last rendered rows — it is being disposed or has nothing left to poll with. + // Reporting is best-effort: the fault being reported may itself be the teardown race + // this catch-all exists for — an InvokeAsync against a disposed renderer — in which + // case the dispatch fails the same way and there is no page left to show it on. The + // poll loop keeps ticking either way and exits on the next cancellation check. } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor index 770af52..af26880 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor @@ -117,8 +117,9 @@ else @if (_recentEvents.Count == 0) {
- Waiting for events. The dashboard mirrors the session's gRPC event stream — events - appear here only while a gRPC client is also consuming this session's events. + Waiting for events. The dashboard subscribes to this session's events directly, so + rows appear as the session's worker emits them while this page is open — no gRPC + client has to be consuming the session.
} else