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 77be9d4..798e018 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,18 @@ 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, 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 @@ -159,7 +169,104 @@ 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. + +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. +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. + +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 their drain at 5 seconds on dispose, for the same reason +`DashboardPageBase` bounds its watch drain: both loops render through the renderer's +dispatcher, and disposal can run on it. The two are drained concurrently, so the +bound on disposal is 5 seconds in total rather than per loop — a wedged dispatcher +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 `MxGateway.Dashboard.HubClients` policy (cookie OR `MxGateway.Dashboard.HubToken` bearer). Each hub class is `[Authorize(Policy = HubClientsPolicy)]`. @@ -167,46 +274,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 +349,48 @@ 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: 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. + 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 +520,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 +532,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 +692,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 +744,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 @@ -603,9 +770,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 clients. +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 @@ -700,8 +868,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 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/docs/MxAccessWorkerInstanceDesign.md b/docs/MxAccessWorkerInstanceDesign.md index c194340..28f49a1 100644 --- a/docs/MxAccessWorkerInstanceDesign.md +++ b/docs/MxAccessWorkerInstanceDesign.md @@ -887,6 +887,50 @@ 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. 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 — 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 are rejected and queued commands that have not started receive diff --git a/docs/WorkerFrameProtocol.md b/docs/WorkerFrameProtocol.md index 36fe955..e2f25b9 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,40 @@ 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. 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 +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 +163,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/docs/plans/2026-08-15-deferred-remediation.md b/docs/plans/2026-08-15-deferred-remediation.md new file mode 100644 index 0000000..fb42808 --- /dev/null +++ b/docs/plans/2026-08-15-deferred-remediation.md @@ -0,0 +1,374 @@ +# 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. *(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. + +**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). | + +--- + +## 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-deferred-remediation.md.tasks.json b/docs/plans/2026-08-15-deferred-remediation.md.tasks.json new file mode 100644 index 0000000..e399fa5 --- /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": "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": "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-16T00:00:00Z" +} 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/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 ded13c8..f9e7a3a 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,165 @@ using Microsoft.AspNetCore.Components; -using Microsoft.AspNetCore.SignalR.Client; -using ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs; +using Microsoft.Extensions.Logging; 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. + /// + /// 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; + + /// 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!; + + /// 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 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 (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 { - // Hub is best-effort; the initial GetSnapshot() seed remains - // valid and the snapshot service keeps populating its cache for - // the next reconnect cycle. + // Other disposal-time errors are best-effort. + } + + _watchCancellation.Dispose(); + 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) + { + // 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) + { + try + { + 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; + } } } } 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..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 @@ -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 @@ -169,17 +168,24 @@ 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; 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 +195,51 @@ : 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. + await InvokeAsync(() => + { + _providerStatus = DashboardAlarmProviderStatus.FromFeed(message); + 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; + } } } @@ -278,19 +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 + { + // 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. } } @@ -310,32 +398,41 @@ /// public async ValueTask DisposeAsync() { - await _cts.CancelAsync(); + await _cts.CancelAsync().ConfigureAwait(false); - if (_alarmsHub is not null) - { - try - { - await _alarmsHub.DisposeAsync(); - } - catch - { - // Disposal-time errors are best-effort. - } - } - - if (_pollTask is not null) - { - try - { - await _pollTask; - } - catch (OperationCanceledException) - { - } - } + // 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); } + + // 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).ConfigureAwait(false); + } + catch (TimeoutException) + { + } + catch (OperationCanceledException) + { + } + catch + { + // Other disposal-time errors are best-effort. + } + } } 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..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 @@ -1,11 +1,15 @@ @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 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 IDashboardSessionEventSubscriber EventSubscriber Dashboard Session @@ -113,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 @@ -157,7 +162,18 @@ else private DashboardSessionSummary? CurrentSession => Snapshot?.Sessions.FirstOrDefault(session => string.Equals(session.SessionId, SessionId, StringComparison.Ordinal)); - private HubConnection? _eventsHub; + // 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(); @@ -183,8 +199,11 @@ else { if (!string.Equals(_subscribedSessionId, SessionId, StringComparison.Ordinal)) { - await DetachEventsHubAsync().ConfigureAwait(false); - await AttachEventsHubAsync().ConfigureAwait(false); + // 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(); } } @@ -261,68 +280,160 @@ else string ConfirmButtonClass, Func> Action); - private async Task AttachEventsHubAsync() + // 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. + // 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)) { return; } - _eventsHub = HubFactory.Create("/hubs/events"); - _eventsHub.On(EventsHub.EventMessage, async mxEvent => - { - _recentEvents.AddFirst(mxEvent); - while (_recentEvents.Count > MaxRecentEvents) - { - _recentEvents.RemoveLast(); - } + _eventSubscription = EventSubscriber.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); - }; + // 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) + { 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(() => + { + // 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); + } + + 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) + { + // 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); } } - private async Task DetachEventsHubAsync() + // 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) { - HubConnection? hub = _eventsHub; - _eventsHub = null; + 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() + { + IDashboardEventSubscription? subscription = _eventSubscription; + CancellationTokenSource? cancellation = _eventPumpCancellation; + Task? pump = _eventPumpTask; + _eventSubscription = null; + _eventPumpCancellation = null; + _eventPumpTask = null; _eventsConnected = false; _subscribedSessionId = null; _recentEvents.Clear(); - if (hub is not null) + // 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(); + subscription?.Dispose(); + + try { - try + if (pump is not null) { - await hub.DisposeAsync().ConfigureAwait(false); - } - catch - { - // Disposal-time errors are best-effort. + 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) @@ -332,9 +443,14 @@ 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 DetachEventsHubAsync().ConfigureAwait(false); + 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 42e014f..83ef2d1 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardServiceCollectionExtensions.cs @@ -38,19 +38,32 @@ public static class DashboardServiceCollectionExtensions services.AddZbLdapAuth(configuration, "MxGateway:Ldap"); services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); services.AddSingleton, DashboardGroupRoleMapper>(); 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. 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.Server/Dashboard/DashboardSnapshotFeed.cs b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs new file mode 100644 index 0000000..301104f --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs @@ -0,0 +1,372 @@ +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 first subscriber arrives and is cancelled and +/// 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. +/// +/// +/// 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 = []; + + /// + /// 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 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. + 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, + }); + + Subscription subscription = 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(subscription).ConfigureAwait(false); + } + } + + private Subscription Subscribe(Channel channel) + { + lock (_gate) + { + // 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(Subscription subscription) + { + CancellationTokenSource? cancellation; + Task pump; + lock (_gate) + { + if (!_subscribers.Remove(subscription)) + { + // 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; + } + + cancellation = _pumpCancellation; + _pumpCancellation = null; + _generation = NoGeneration; + pump = _pumpTask; + } + + try + { + cancellation?.Cancel(); + } + catch (ObjectDisposedException) + { + // The pump ended on its own and disposed its 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. + } + } + + /// 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. + /// + /// 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); + + // 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) + .GetAsyncEnumerator(cancellationToken); + try + { + 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 this generation's + // subscribers and re-arm so the next one starts a fresh enumeration. + Reset(generation, error: null); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // 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(generation, error); + } + finally + { + cancellation.Dispose(); + } + } + + private void Broadcast(long generation, DashboardSnapshot snapshot) + { + lock (_gate) + { + foreach (Subscription subscriber in _subscribers) + { + if (subscriber.Generation != generation) + { + continue; + } + + // Bounded/DropOldest: always accepted unless the channel is completed. + subscriber.Channel.Writer.TryWrite(snapshot); + } + } + } + + /// + /// Stops handing to new subscribers. Called the instant a + /// pump's source fails or completes, before its enumerator is disposed. + /// + /// The generation that has ended. + private void EndGeneration(long generation) + { + lock (_gate) + { + 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.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/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/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/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.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.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). 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) diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionEventDistributor.cs index a7b653a..04212ab 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; @@ -57,27 +56,29 @@ 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 /// 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. @@ -745,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( @@ -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]); 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/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. + } } } 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, 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..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,22 +27,38 @@ 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); + /// + /// 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); } } 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..8c0091e --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs @@ -0,0 +1,522 @@ +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); + } + + /// + /// 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); + } + + /// + /// 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. + 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 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 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); + + /// 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); + + /// + /// 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 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); + 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); + } + } + + /// + /// 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); + } + } + } +} 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.Tests/Ipc/WorkerPipeSessionTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Ipc/WorkerPipeSessionTests.cs index 4f6703c..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,6 +912,86 @@ public sealed class WorkerPipeSessionTests await Assert.ThrowsAsync(async () => await runTask); } + /// + /// 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 +1961,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 +2233,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) @@ -2165,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(); @@ -2484,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.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/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/WorkerFrameWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs index cfb20e7..d94b118 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,14 @@ 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. 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. @@ -189,7 +216,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 +323,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 +349,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 +415,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); } 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..f849813 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,117 @@ 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). 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 + // 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 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) + { + return; + } + + try + { + _transportStream.Dispose(); + } + catch (Exception exception) + { + _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 a zero-byte read + /// mapped to EndOfStream, and that fault has to be observed — the worker installs no + /// TaskScheduler.UnobservedTaskException handler. + /// + /// + /// + /// 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() + { + 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 +394,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 @@ -272,7 +430,11 @@ 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; try { @@ -281,6 +443,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) @@ -288,7 +453,11 @@ 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; } else if (completedTask == heartbeatTask) { @@ -309,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) @@ -318,6 +513,7 @@ public sealed class WorkerPipeSession .ConfigureAwait(false); if (completedTask != task) { + ObserveFaultWhenever(task); _logger?.Error( "WorkerPipeSessionBackgroundTaskStopTimedOut", new Dictionary @@ -347,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) 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()); }