docs(archreview): add architecture review + per-domain remediation designs and tracker
Adds the 2026-07-08 architecture review (00-overall + six domain reports) and a remediation/ tree: one design+implementation doc per domain covering every finding, plus 00-tracking.md as the master progress tracker. - 153 findings with stable IDs (GWC/WRK/IPC/SEC/CLI/TST), each with design rationale, implementation steps, tests, docs, and verification. - Tracker rolls findings up by severity and P0/P1/P2 roadmap tier, records cross-cutting clusters and per-finding status (all Not started). - Planning docs only; no source changes.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,447 @@
|
||||
# Gateway Server Core — Remediation Design & Implementation
|
||||
|
||||
Source review: [10-gateway-core.md](../10-gateway-core.md) · Generated: 2026-07-09
|
||||
|
||||
This document turns the Gateway Server Core review into buildable remediation entries. Every cited `path:line` was re-verified against the working tree; all citations still match at the time of writing. Findings are grouped most-severe first, then in the source report's own order (Stability → Performance → Conventions → Underdeveloped). The one Critical (alarm-feed split), both Highs (faulted-session reaping, sparse-array bound), and one Medium (event-backlog stall) carry roadmap tiers from `00-overall.md`; the performance cluster is the P2 hot-path pass.
|
||||
|
||||
## Finding index
|
||||
|
||||
| ID | Sev | Title | Roadmap | Effort | Depends on | Files |
|
||||
|----|-----|-------|---------|--------|-----------|-------|
|
||||
| GWC-01 | Critical | Alarm monitor and distributor pump both drain the single worker event channel | P0 | M | — | Alarms/GatewayAlarmMonitor.cs, Sessions/GatewaySession.cs, Workers/WorkerClient.cs |
|
||||
| GWC-02 | High | Faulted sessions are never swept | P0 | M | — | Sessions/GatewaySession.cs, Sessions/SessionManager.cs |
|
||||
| GWC-03 | High | Documented sparse-array max-length bound is unimplemented | P0 | S | — | Sessions/SparseArrayExpander.cs, Configuration/EventOptions.cs, Configuration/GatewayOptionsValidator.cs |
|
||||
| GWC-04 | Medium | Full event channel stalls the worker read loop behind command replies | P1 | M | — | Workers/WorkerClient.cs |
|
||||
| GWC-05 | Medium | Worker pipe created with no ACL / no CurrentUserOnly | — | S | — | Sessions/SessionWorkerClientFactory.cs |
|
||||
| GWC-06 | Medium | Stopwatch allocated per streamed event | P2 | S | GWC-07, GWC-08 | Grpc/MxAccessGatewayService.cs |
|
||||
| GWC-07 | Medium | Every mapped event is deep-cloned | P2 | S | GWC-06, GWC-08 | Grpc/MxAccessGrpcMapper.cs, Sessions/GatewaySession.cs |
|
||||
| GWC-08 | Medium | Pipe framing allocates per frame and writes twice | P2 | M | GWC-06, GWC-07 | Workers/WorkerFrameReader.cs, Workers/WorkerFrameWriter.cs |
|
||||
| GWC-09 | Medium | Startup probe is a no-op; its retry pipeline can never retry | — | M | — | Workers/WorkerProcessStartedProbe.cs, Workers/WorkerProcessLauncher.cs, Configuration/WorkerOptions.cs |
|
||||
| GWC-10 | Medium | Envelope `sequence` monotonicity is specified but not enforced | — | S | — | Workers/WorkerEnvelopeValidator.cs, Workers/WorkerClient.cs |
|
||||
| GWC-11 | Low | `_workerClient` written under lock but read lock-free | — | S | — | Sessions/GatewaySession.cs |
|
||||
| GWC-12 | Low | `WorkerClient.DisposeAsync` not safe against double-dispose | — | S | — | Workers/WorkerClient.cs |
|
||||
| GWC-13 | Low | Worker-ready wait is a 25 ms poll loop | — | M | — | Sessions/GatewaySession.cs |
|
||||
| GWC-14 | Low | Replay ring is a `LinkedList` with a node alloc per event | P2 | M | — | Sessions/SessionEventDistributor.cs |
|
||||
| GWC-15 | Low | Per-event gauge reads `ChannelReader.Count` every event | P2 | S | — | Grpc/EventStreamService.cs |
|
||||
| GWC-16 | Low | `Invoke` resolves the session twice per command | — | S | — | Grpc/MxAccessGatewayService.cs, Sessions/SessionManager.cs |
|
||||
| GWC-17 | Low | Sessions layer throws `Grpc.Core.RpcException` | — | M | — | Sessions/SparseArrayExpander.cs, Sessions/GatewaySession.cs, Grpc/MxAccessGatewayService.cs |
|
||||
| GWC-18 | Low | `GatewaySession` implements `DisposeAsync` without `IAsyncDisposable` | — | S | — | Sessions/GatewaySession.cs |
|
||||
| GWC-19 | Low | Dead `GatewaySession.KillWorker(string)` + stale doc | — | S | — | Sessions/GatewaySession.cs, docs/Sessions.md |
|
||||
| GWC-20 | Low | Heartbeat config semantics conflated (send vs check interval) | — | S | — | Configuration/WorkerOptions.cs, Sessions/SessionWorkerClientFactory.cs, Workers/WorkerClient.cs |
|
||||
| GWC-21 | Low | `EventChannelFullModeTimeout` / `HeartbeatStuckCeiling` not configurable | — | S | GWC-04 | Sessions/SessionWorkerClientFactory.cs, Configuration/WorkerOptions.cs |
|
||||
| GWC-22 | Low | `StreamDisconnected` always labeled `"Detached"` | — | S | — | Grpc/EventStreamService.cs |
|
||||
| GWC-23 | Info | `MaxEventSubscribersPerSession` dead in default single-subscriber mode | — | — | — | Configuration/GatewayOptionsValidator.cs |
|
||||
|
||||
---
|
||||
|
||||
## GWC-01 — Alarm monitor and distributor pump both drain the single worker event channel `Critical` · `P0`
|
||||
|
||||
**Finding.** `GatewayAlarmMonitor.RunMonitorAsync` drains worker events directly through `_sessionManager.ReadEventsAsync(session.SessionId, …)` (`Alarms/GatewayAlarmMonitor.cs:228-231`) → `GatewaySession.ReadEventsAsync` (`Sessions/GatewaySession.cs:1427-1440`) → `WorkerClient.ReadEventsAsync` (`Workers/WorkerClient.cs:227-236`). But `MarkReady` starts the dashboard mirror (`Sessions/GatewaySession.cs:433-437`), whose distributor pump source `MapWorkerEventsAsync` also calls `ReadEventsAsync` (`Sessions/GatewaySession.cs:701-710`). `WorkerClient._events` is a single channel created with `SingleReader = false` (`Workers/WorkerClient.cs:70-77`), so the two concurrent `ReadAllAsync` enumerators each receive a random subset of events.
|
||||
|
||||
**Impact.** In every production deployment `IDashboardEventBroadcaster` is registered and injected, so the pump starts at session-ready — including on the alarm monitor's own session. Roughly half of `OnAlarmTransition` events land in the dashboard mirror instead of `ApplyTransition`. `ReconcileLoopAsync` eventually repairs Raise/Clear presence, but `ApplyReconcile` broadcasts only presence deltas (`Alarms/GatewayAlarmMonitor.cs:511-550`), so a stolen Acknowledge transition never reaches `StreamAlarms` subscribers — clients show unacked alarms indefinitely. Provider-mode-change events can also be stolen, delaying degraded-state visibility. `docs/Sessions.md:196` documents the exact race single-subscriber mode exists to prevent; the monitor recreates it internally.
|
||||
|
||||
**Design.** Make the alarm monitor a *distributor subscriber* rather than a second raw channel drain, so the single worker channel keeps exactly one consumer (the distributor pump) and the pump fans every event to both the dashboard mirror and the alarm monitor. Add an internal (non-counted) registration path on the session mirroring the existing dashboard mirror lease (`isInternal: true`, `Sessions/GatewaySession.cs:582`), so the alarm subscriber does not consume one of `MaxEventSubscribersPerSession` and a slow reconcile never faults the session.
|
||||
|
||||
To make any *future* dual-consumer regression fail loudly rather than silently split events, additionally flip `WorkerClient._events` to `SingleReader = true` and add a claimed-once guard in `WorkerClient.ReadEventsAsync` (throw if a second enumerator is created). After this fix the distributor pump is the only direct caller, so `SingleReader = true` is correct and the guard converts the bug class into an immediate exception.
|
||||
|
||||
Alternative rejected: giving the alarm monitor its own second worker channel (fan the worker frame to two channels in `EnqueueWorkerEventAsync`) — doubles per-event copy cost on the hot path and duplicates the backpressure/overflow accounting the distributor already owns.
|
||||
|
||||
Parity note: no synthesized events and no MXAccess behavior change — this only fixes *delivery* of events the worker already emits.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/GatewaySession.cs`: add an internal-subscriber accessor the alarm monitor can use, e.g. `AttachInternalEventSubscriber()` that runs `EnsureDistributorCreated` / `distributor.Register(isInternal: true)` / `StartPumpIfRequested` (the same sequence `StartDashboardMirror` uses at `:581-583`) and returns an `IAsyncEnumerable<MxEvent>` (or the lease + its channel reader). Reuse `MapWorkerEventsAsync`'s public `MxEvent` shape so the monitor's existing `MxEvent.BodyCase` switch (`Alarms/GatewayAlarmMonitor.cs:232-246`) is unchanged.
|
||||
- `Sessions/SessionManager.cs`: expose the new subscriber path (e.g. `IAsyncEnumerable<MxEvent> ReadAlarmEventsAsync(sessionId, ct)`) alongside the existing `ReadEventsAsync`, or repoint the monitor at the distributor via the session directly.
|
||||
- `Alarms/GatewayAlarmMonitor.cs:228-231`: consume the distributor subscriber stream (mapped `MxEvent`) instead of `_sessionManager.ReadEventsAsync` (raw `WorkerEvent`). `workerEvent.Event` reads become direct `MxEvent` reads.
|
||||
- `Workers/WorkerClient.cs:70-77`: set `SingleReader = true`; in `ReadEventsAsync` (`:227`) add an `Interlocked.CompareExchange` claimed-once guard that throws `InvalidOperationException` on a second enumerator.
|
||||
- Tests: extend `GatewayAlarmMonitorTests` (or add `GatewayAlarmMonitorDistributorTests`) driving the `FakeWorkerHarness` with a session that has the dashboard mirror active, asserting every `OnAlarmTransition` (including Acknowledge) reaches `StreamAlarms`; add a `WorkerClientTests` case asserting a second `ReadEventsAsync` enumerator throws.
|
||||
- Docs: note in `docs/Sessions.md` that the alarm monitor attaches as an internal distributor subscriber; update `gateway.md` alarm section if it describes the direct drain.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~GatewayAlarmMonitor"` and `--filter "FullyQualifiedName~WorkerClient"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-02 — Faulted sessions are never swept `High` · `P0`
|
||||
|
||||
**Finding.** `MarkFaulted` only flips state and records the reason (`Sessions/GatewaySession.cs:716-728`); it does not kill the worker, stop the distributor, or notify the registry. `CloseExpiredLeasesAsync` sweeps only lease-expired or detach-grace-expired sessions (`Sessions/SessionManager.cs:273-277`); there is no `State == Faulted` branch. Detach-grace deliberately skips faulted sessions (`Sessions/GatewaySession.cs:2022-2028`), so the only exit is lease expiry at `DefaultLeaseSeconds` = 1800 s. In the FailFast single-subscriber overflow case (`Sessions/GatewaySession.cs:690-694`) the worker stays healthy and keeps pumping events while the session is permanently unusable (`EvaluateReadyUnderLock` fails every command).
|
||||
|
||||
**Impact.** A burst-slow client that overflows its queue faults the session; for up to 30 minutes the gateway pins one of `MaxSessions` (default 64) slots and a running x86 MXAccess worker with live COM subscriptions no client can use or close (clients rarely call `CloseSession` on a faulted session). A handful of such faults exhausts session capacity.
|
||||
|
||||
**Design.** Sweep `Faulted` sessions in `CloseExpiredLeasesAsync` so the existing serialized teardown (`TryBeginCloseIfExpired` → `CloseSessionCoreAsync`) reaps them promptly, reusing the proven TOCTOU-safe close gate rather than adding a second teardown path. Add a distinct close reason (`FaultedReason`) so operators can distinguish fault reaps from lease/detach reaps in logs and metrics. Optionally add a short `FaultedGraceSeconds` (default 0) so a monitoring client can still observe the fault via `GetSessionStatus` before the slot is reclaimed; recommend default-immediate to bound blast radius, with the grace knob as the tunable.
|
||||
|
||||
Alternative considered: have `MarkFaulted` synchronously kill the worker and schedule teardown (as `WorkerClient.SetFaulted` does on its side). Rejected as the primary fix because `MarkFaulted` is called from inside the distributor overflow handler and the gRPC pump (`Grpc/EventStreamService.cs:155`) under async contexts where a synchronous kill + registry mutation risks re-entrancy against the close gate; the sweeper path already centralizes teardown ordering. `MarkFaulted` can still *nudge* the sweeper (e.g. set an eligibility timestamp), but the reap stays in `CloseExpiredLeasesAsync`.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/GatewaySession.cs`: add `IsFaultedReapable(now)` (true when `_state == Faulted`, gated by optional `FaultedGraceSeconds` measured from a fault timestamp stamped in `MarkFaulted`).
|
||||
- `Sessions/SessionManager.cs:273-277`: extend the reason ladder — `session.IsLeaseExpired(now) ? LeaseExpiredReason : session.IsFaultedReapable(now) ? FaultedReason : session.IsDetachGraceExpired(now) ? DetachGraceExpiredReason : null`. Ensure `TryBeginCloseIfExpired` treats a faulted session as eligible.
|
||||
- `Configuration/SessionOptions.cs`: add `FaultedGraceSeconds` (default 0); validate `>= 0` in `Configuration/GatewayOptionsValidator.cs`.
|
||||
- Tests: `SessionManagerTests`/`SessionLeaseSweeperTests` — fault a session, advance `TimeProvider`, assert the sweeper closes it and kills the worker; assert a non-faulted leased session is untouched.
|
||||
- Docs: `docs/Sessions.md` (sweeper reasons) and `docs/GatewayConfiguration.md` (`FaultedGraceSeconds`) in the same commit.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~SessionManager"` and `--filter "FullyQualifiedName~Sweeper"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-03 — Documented sparse-array max-length bound is unimplemented `High` · `P0`
|
||||
|
||||
**Finding.** `gateway.md:532` lists "`total_length` exceeds the gateway-configured maximum array length" as an `InvalidArgument` rejection, but `SparseArrayExpander.Expand` validates only `totalLength > (uint)Array.MaxLength` (`Sessions/SparseArrayExpander.cs:65-69`) — ~2.1 billion elements. No configuration option for the bound exists in `Configuration/`.
|
||||
|
||||
**Impact.** A single authorized `Write` carrying `total_length = 500_000_000` forces materialization of a multi-GB `MxArray` (`Sessions/SparseArrayExpander.cs:98-232`) before the 16 MB `MaxMessageBytes` frame check finally rejects the serialized result in `WorkerFrameWriter` (`Workers/WorkerFrameWriter.cs:49-54`) — a memory-exhaustion vector reachable through the normal command path.
|
||||
|
||||
**Design.** Add the documented configurable cap under `MxGateway:Events` (or a new `MxGateway:Sparse` section) and enforce it in `SparseArrayExpander.Expand` *before* allocation, throwing the same `InvalidArgument` the surrounding validators already use. Validate the bound at startup in `GatewayOptionsValidator`. A conservative default (e.g. 1_000_000 elements — comfortably above realistic MXAccess array writes yet far below the frame-size ceiling) matches the parity contract: MXAccess itself has no multi-hundred-million-element array use case, so the cap rejects only abusive inputs and does not alter valid behavior.
|
||||
|
||||
The expander is constructed per invocation from `NormalizeOutboundCommand` (`Sessions/GatewaySession.cs:984-1063`); thread the configured max in through its constructor or an `Expand(..., int maxTotalLength)` parameter sourced from `EventOptions`/a new option object the session already holds.
|
||||
|
||||
**Implementation.**
|
||||
- `Configuration/EventOptions.cs` (or new `SparseArrayOptions`): add `MaxSparseArrayLength` (default 1_000_000).
|
||||
- `Configuration/GatewayOptionsValidator.cs`: validate `MaxSparseArrayLength >= 1` and `<= Array.MaxLength`.
|
||||
- `Sessions/SparseArrayExpander.cs:65-69`: check `totalLength > maxSparseArrayLength` first, with a message naming the configured cap; keep the `Array.MaxLength` guard as a backstop.
|
||||
- Wire the value from options through `GatewaySession.NormalizeOutboundCommand` into the expander.
|
||||
- Tests: `SparseArrayExpanderTests` — assert `total_length` above the cap throws `InvalidArgument` before allocation; assert a value below the cap still expands. Add a `GatewayOptionsValidatorTests` case for the new bound.
|
||||
- Docs: confirm `gateway.md:532` wording matches the option name; add the key to `docs/GatewayConfiguration.md`.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~SparseArrayExpander"` and `--filter "FullyQualifiedName~GatewayOptionsValidator"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-04 — Full event channel stalls the worker read loop behind command replies `Medium` · `P1`
|
||||
|
||||
**Finding.** `ReadLoopAsync` awaits `DispatchEnvelopeAsync` inline (`Workers/WorkerClient.cs:358-362`), and the `WorkerEvent` branch awaits `EnqueueWorkerEventAsync`, which blocks in `WriteAsync` for up to `EventChannelFullModeTimeout` (default 5 s) when `_events` is full (`Workers/WorkerClient.cs:511-553`, `Workers/WorkerClientOptions.cs:13`).
|
||||
|
||||
**Impact.** With no event consumer attached or a stalled distributor, each incoming event costs up to 5 s of read-loop stall before the fault fires. A `WorkerCommandReply` queued behind an event frame is not dispatched, so an in-flight `InvokeAsync` can hit `CommandTimeout` (`Workers/WorkerClient.cs:187-213`) even though the worker replied in time; heartbeats behind the stall feed the very watchdog interplay the code tries to compensate for (`Workers/WorkerClient.cs:394-424`).
|
||||
|
||||
**Design.** Decouple event enqueue from the read loop so replies and heartbeats are never blocked by an event backlog. The class already has a dedicated `WriteLoopAsync` for outbound envelopes (`Workers/WorkerClient.cs:332-338`); mirror that for events. In `DispatchEnvelopeAsync`, the `WorkerEvent` branch should hand the event to `_events` via a **non-blocking** `TryWrite`; when the channel is full, start (or continue) a short bounded backpressure window on a *separate* task rather than blocking the read loop. Command-reply, heartbeat, fault, and shutdown-ack branches dispatch synchronously and immediately, so they can never queue behind events.
|
||||
|
||||
Concretely: keep the `EventChannelFullModeTimeout` semantics but move the waiting `WriteAsync` off the read loop — either (a) route events through a small internal event channel drained by a dedicated writer task that performs the timed `WriteAsync` and faults on timeout, or (b) on a `TryWrite` miss, record a monotonic "backlog since" timestamp and fault only once the backlog persists past the timeout, all without awaiting inside `ReadLoopAsync`. Option (a) is cleaner and symmetrical with the existing write loop; recommend it.
|
||||
|
||||
This is the gateway half of the coordinated backpressure/size pass in `00-overall.md` P1.8 (co-designed with the worker and contracts backlog findings); the fault model (`ProtocolViolation` on sustained overflow) is unchanged.
|
||||
|
||||
**Implementation.**
|
||||
- `Workers/WorkerClient.cs`: add an internal unbounded-or-small event staging channel + `EventWriteLoopAsync` (parallel to `WriteLoopAsync`); `DispatchEnvelopeAsync`'s `WorkerEvent` branch does a non-awaiting hand-off; the writer task owns the timed `WriteAsync`/fault. Register the new task in `WaitForBackgroundTasksAsync` and complete it in `DisposeAsync`.
|
||||
- Tests: `WorkerClientTests` — with `_events` full and no consumer, assert a `WorkerCommandReply` arriving after an event is still dispatched promptly (command does not time out); assert sustained overflow still faults with the existing `ProtocolViolation` message. Reuse the fake pipe harness.
|
||||
- Docs: note the event-vs-reply decoupling in `docs/WorkerFrameProtocol.md` / `docs/MxAccessWorkerInstanceDesign.md` if they describe the read loop.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~WorkerClient"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-05 — Worker pipe created with no ACL / no CurrentUserOnly `Medium` · `—`
|
||||
|
||||
**Finding.** `SessionWorkerClientFactory.CreatePipe` uses the plain `NamedPipeServerStream` constructor with `PipeOptions.Asynchronous` only (`Sessions/SessionWorkerClientFactory.cs:158-166`). `gateway.md:282-283` requires the pipe be "ACL restricted to the gateway identity and the launched worker identity, no anonymous access".
|
||||
|
||||
**Impact.** Any local process can connect to `mxaccess-gateway-{pid}-{sessionId}` before the real worker; with `maxNumberOfServerInstances: 1` the legitimate worker then never connects, so `OpenSession` fails on startup timeout — a trivially repeatable local denial of service. The startup nonce (`Workers/WorkerClient.cs:632-637`) prevents impersonation but not connection stealing.
|
||||
|
||||
**Design.** Create the pipe with an explicit DACL limited to the service identity via `NamedPipeServerStreamAcl.Create` (from `System.IO.Pipes.AccessControl`). Because workers run as the gateway identity (`docs/DesignDecisions.md`), `PipeOptions.CurrentUserOnly` is the simpler equivalent and is the recommended first choice — it restricts the pipe to the creating user's SID with no manual `PipeSecurity` construction. Keep `PipeOptions.Asynchronous`. This is Windows-only surface; the pipe factory is not exercised on the macOS `NonWindows.slnx`, so guard behind the existing Windows compilation/runtime path.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/SessionWorkerClientFactory.cs:158-166`: `return NamedPipeServerStreamAcl.Create(pipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous, 0, 0, pipeSecurity);` with a `PipeSecurity` granting only the current user full control and denying everyone else — or, if acceptable, `new NamedPipeServerStream(name, …, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly)`.
|
||||
- Tests: worker-pipe tests run on Windows only; add a `SessionWorkerClientFactory` test asserting a second connector as a different principal is rejected (or, minimally, that the pipe is created with restricted ACL). Document why it is skipped on non-Windows.
|
||||
- Docs: none needed beyond confirming `gateway.md:282-283` now matches code.
|
||||
|
||||
**Verification.** On the Windows host: `dotnet build src/ZB.MOM.WW.MxGateway.Server`; run the factory/pipe tests. On macOS the change is not compiled into `NonWindows.slnx`; document the skip.
|
||||
|
||||
---
|
||||
|
||||
## GWC-06 — Stopwatch allocated per streamed event `Medium` · `P2`
|
||||
|
||||
**Finding.** `MxAccessGatewayService.StreamEvents` runs `Stopwatch stopwatch = Stopwatch.StartNew()` inside the per-event loop (`Grpc/MxAccessGatewayService.cs:155-157`), one heap allocation per event per subscriber on the highest-volume gateway path.
|
||||
|
||||
**Impact.** Avoidable GC pressure under alarm bursts — the exact load the system exists to handle.
|
||||
|
||||
**Design.** Replace the `Stopwatch` object with the allocation-free timestamp API: `long ts = Stopwatch.GetTimestamp();` before the write and `metrics.RecordEventStreamSend(family, Stopwatch.GetElapsedTime(ts))` after. Behavior and the recorded metric are identical. Co-designed with GWC-07 and GWC-08 as the P2 hot-path pass.
|
||||
|
||||
**Implementation.**
|
||||
- `Grpc/MxAccessGatewayService.cs:155-157`: swap to `Stopwatch.GetTimestamp()` / `Stopwatch.GetElapsedTime(ts)`.
|
||||
- Tests: existing `MxAccessGatewayService`/`EventStreamService` streaming tests cover behavior; no new assertion needed beyond a green run.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~StreamEvents"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-07 — Every mapped event is deep-cloned `Medium` · `P2`
|
||||
|
||||
**Finding.** `MxAccessGrpcMapper.MapEvent` returns `workerEvent.Event?.Clone()` (`Grpc/MxAccessGrpcMapper.cs:64-73`), invoked once per event by the distributor pump via `MapWorkerEventsAsync` (`Sessions/GatewaySession.cs:701-710`).
|
||||
|
||||
**Impact.** A full protobuf deep copy (including value arrays) per event, even though the enclosing `WorkerEvent` is discarded immediately after mapping and nothing else retains the inner message.
|
||||
|
||||
**Design.** Transfer ownership of `workerEvent.Event` instead of cloning — return the inner message directly. The `WorkerEvent` wrapper is created by the frame reader per received frame and is not retained after `MapEvent`, so no other consumer aliases the inner `MxEvent`. Keep a comment stating the ownership-transfer invariant so a future second consumer of the `WorkerEvent` restores the clone. This is safe today precisely because GWC-01 makes the distributor pump the single consumer of the worker channel.
|
||||
|
||||
Depends on GWC-01 (single-consumer guarantee) to be strictly safe; co-designed with GWC-06/GWC-08.
|
||||
|
||||
**Implementation.**
|
||||
- `Grpc/MxAccessGrpcMapper.cs:64-73`: `return workerEvent.Event ?? new MxEvent { Family = MxEventFamily.Unspecified, RawStatus = "…" };` (drop `.Clone()`), with an ownership-transfer comment.
|
||||
- Tests: `MxAccessGrpcMapperTests` — assert the returned reference is the same instance as `workerEvent.Event` (ownership transferred) and the unspecified-family fallback path is unchanged.
|
||||
- Docs: none.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~MxAccessGrpcMapper"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-08 — Pipe framing allocates per frame and writes twice `Medium` · `P2`
|
||||
|
||||
**Finding.** The reader allocates `new byte[sizeof(uint)]` and `new byte[payloadLength]` per frame (`Workers/WorkerFrameReader.cs:32,50`); the writer allocates `new byte[sizeof(uint)]` plus `envelope.ToByteArray()` and performs two `WriteAsync` calls (`Workers/WorkerFrameWriter.cs:56-60`).
|
||||
|
||||
**Impact.** Per-frame GC pressure proportional to event rate and two pipe syscalls per outbound frame. The worker side already uses `ArrayPool`; the gateway side does not.
|
||||
|
||||
**Design.** On the write path, serialize length + payload into a single pooled buffer and issue one `WriteAsync`: rent `4 + payloadLength` from `ArrayPool<byte>.Shared`, write the little-endian length prefix into the first four bytes, serialize the envelope into the remainder via `envelope.WriteTo(new CodedOutputStream(...))` or `envelope.WriteTo(span)`, write once, then return the buffer. On the read path, rent a buffer sized to the frame instead of `new byte[payloadLength]`, parse from the rented span, and return it. Preserve the existing length-validation-before-allocation order (`Workers/WorkerFrameReader.cs:43-48`) — the `MaxMessageBytes` check must still gate the rent size. Co-designed with GWC-06/GWC-07.
|
||||
|
||||
**Implementation.**
|
||||
- `Workers/WorkerFrameWriter.cs:56-60`: rent `4 + payloadLength`, fill prefix + serialized payload, single `WriteAsync(buffer, 0, total)`, `try/finally` return to pool.
|
||||
- `Workers/WorkerFrameReader.cs:32,50`: rent buffers (or reuse a per-reader scratch buffer for the 4-byte prefix), parse from the rented region, return in `finally`. Keep exact-read semantics (`ReadExactlyOrThrowAsync`).
|
||||
- Tests: `WorkerFrameReaderTests`/`WorkerFrameWriterTests` round-trip must stay green; add a large-payload round-trip and an oversize-rejection test to confirm the validation order is preserved. These also protect the `FakeWorkerHarness` which reuses this frame code.
|
||||
- Docs: `docs/WorkerFrameProtocol.md` if it describes the two-write layout.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~WorkerFrame"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-09 — Startup probe is a no-op; its retry pipeline can never retry `Medium` · `—`
|
||||
|
||||
**Finding.** `WorkerProcessStartedProbe.WaitUntilReadyAsync` does one instantaneous `HasExited` check and throws `WorkerProcessLaunchException` on failure (`Workers/WorkerProcessStartedProbe.cs:6-19`); `ShouldRetryStartupProbe` explicitly excludes `WorkerProcessLaunchException` and `OperationCanceledException` from retry (`Workers/WorkerProcessLauncher.cs:291-299`). The Polly pipeline with exponential backoff and jitter (`Workers/WorkerProcessLauncher.cs:264-289`) therefore executes exactly one attempt in every case, so `StartupProbeRetryAttempts` and `StartupProbeRetryDelayMilliseconds` (`Configuration/WorkerOptions.cs:19-22`, validated at `Configuration/GatewayOptionsValidator.cs:119-126`) have no observable effect.
|
||||
|
||||
**Impact.** Two configuration options and their validation are dead; operators may tune them expecting an effect. Actual worker readiness is instead established later by the pipe-connect + nonce handshake, so nothing is *broken*, but the surface is misleading.
|
||||
|
||||
**Design.** Two viable options; recommend **(B) remove the dead surface** unless a real readiness signal is planned, because the pipe handshake already gates readiness and a genuine probe would duplicate it.
|
||||
|
||||
- (A) Implement a real readiness probe with retryable transient failures: have the probe poll a real signal (e.g. pipe-created / worker "ready" marker) and throw a *retryable* exception type (not `WorkerProcessLaunchException`) on transient not-ready, so the Polly pipeline actually retries. Then the two options become live.
|
||||
- (B) Delete the retry pipeline and the two options: replace `CreateStartupProbePipeline` usage with a single direct `WaitUntilReadyAsync` call, remove `StartupProbeRetryAttempts` / `StartupProbeRetryDelayMilliseconds` from `WorkerOptions` and their validator branch, and update `docs/WorkerProcessLauncher.md` in the same commit.
|
||||
|
||||
Open question the review cannot settle: whether a future probe (COM-init readiness) is planned. If yes, do (A); if no, do (B). Recommended: (B) now, reintroduce a real probe if/when COM-readiness signaling lands.
|
||||
|
||||
**Implementation (B).**
|
||||
- `Workers/WorkerProcessLauncher.cs`: drop `CreateStartupProbePipeline` and call the probe directly; remove `ShouldRetryStartupProbe`.
|
||||
- `Configuration/WorkerOptions.cs:19-22` and `Configuration/GatewayOptionsValidator.cs:119-126`: remove the two options and their validation.
|
||||
- Tests: `WorkerProcessLauncherTests` — remove/adjust retry-count assertions; assert an exited-before-ready worker fails fast once.
|
||||
- Docs: `docs/WorkerProcessLauncher.md`, `docs/GatewayConfiguration.md` (drop the two keys) in the same commit.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~WorkerProcessLauncher"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-10 — Envelope `sequence` monotonicity specified but not enforced `Medium` · `—`
|
||||
|
||||
**Finding.** `gateway.md:314` states "`sequence` is monotonic per sender", but `WorkerEnvelopeValidator.Validate` checks only protocol version, session id, and body presence (`Workers/WorkerEnvelopeValidator.cs:15-39`). Nothing on the gateway side detects out-of-order, duplicated, or replayed frames from a misbehaving worker.
|
||||
|
||||
**Impact.** A worker bug that reorders or repeats frames is invisible; the event-ordering guarantee rests solely on the worker's writer discipline. Latent — depends on a worker defect — hence Medium.
|
||||
|
||||
**Design.** Track the last-received `sequence` per `WorkerClient` connection and fault the client with `ProtocolViolation` on a regression (non-increasing sequence), matching the existing fault model (`Workers/WorkerClient.cs:376-380`). Enforce in the read loop after `Validate`, not inside the static validator (which has no per-connection state). Allow the very first frame to establish the baseline. Whether strict `+1` monotonicity or merely strictly-increasing is required depends on whether the worker sends a continuous sequence across all envelope types — recommend strictly-increasing (regression → fault) as the safe superset, since gaps are legal if any frame type is unsequenced.
|
||||
|
||||
**Implementation.**
|
||||
- `Workers/WorkerClient.cs`: add `ulong _lastReceivedSequence` (per connection); in `ReadLoopAsync`/`DispatchEnvelopeAsync` after validation, if `envelope.Sequence <= _lastReceivedSequence` (and not the first frame) call `SetFaulted(ProtocolViolation, …)`; else advance. Keep the check off the hot allocation path.
|
||||
- Tests: `WorkerClientTests` — feed a decreasing/duplicate sequence via the fake pipe and assert the client faults with `ProtocolViolation`; assert normal increasing sequences pass.
|
||||
- Docs: confirm `gateway.md:314` matches; note enforcement point.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~WorkerClient"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-11 — `_workerClient` written under lock but read lock-free `Low` · `—`
|
||||
|
||||
**Finding.** `_workerClient` is written under `_syncRoot` in `AttachWorkerClient` (`Sessions/GatewaySession.cs:368-376`) but read lock-free in `CloseAsync` (`:1470`), `DisposeAsync` (`:1762`), `KillWorker` (`:1617`), and `WorkerProcessId` (`:268`). Reference reads are atomic and the manager's call ordering makes a torn interleaving unlikely, but the discipline documented for `_state` is not applied to the field.
|
||||
|
||||
**Impact.** Latent consistency risk only; no observed failure.
|
||||
|
||||
**Design.** Apply the class's own locking contract: either read `_workerClient` under `_syncRoot` on those paths or mark the field `volatile`. Recommend `volatile` — the reads are single-reference snapshots and lock acquisition on the close/dispose paths is otherwise unnecessary; `volatile` documents the shared-field intent with minimal churn. (`KillWorker` itself is dead — see GWC-19 — so that read disappears if GWC-19 is applied.)
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/GatewaySession.cs`: mark `_workerClient` `volatile` (or wrap the four reads in `lock (_syncRoot)`).
|
||||
- Tests: covered by existing session lifecycle tests; no new assertion.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~GatewaySession"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-12 — `WorkerClient.DisposeAsync` not safe against double-dispose `Low` · `—`
|
||||
|
||||
**Finding.** `DisposeAsync` uses a plain `if (_disposed) return; _disposed = true;` with no interlock (`Workers/WorkerClient.cs:296-303`). Two concurrent disposals would both run kill/complete/dispose, and the second `_stopCts.Cancel()` after `_stopCts.Dispose()` (`:305`, `:328`) would throw `ObjectDisposedException`.
|
||||
|
||||
**Impact.** Latent only — `SessionManager.RemoveSessionAsync`'s registry `TryRemove` gate makes the session's `DisposeAsync` single-shot in practice.
|
||||
|
||||
**Design.** Use `Interlocked.Exchange` to make disposal single-shot, matching the pattern the lease classes already use. Minimal, no behavior change on the single-dispose path.
|
||||
|
||||
**Implementation.**
|
||||
- `Workers/WorkerClient.cs:296-303`: `if (Interlocked.Exchange(ref _disposedFlag, 1) == 1) return;` (int field), keeping the remainder unchanged.
|
||||
- Tests: `WorkerClientTests` — call `DisposeAsync` concurrently twice and assert no throw.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~WorkerClient"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-13 — Worker-ready wait is a 25 ms poll loop `Low` · `—`
|
||||
|
||||
**Finding.** `GetReadyWorkerClientAsync` polls with `Task.Delay(25 ms)` up to `WorkerReadyWaitTimeoutMs` (`Sessions/GatewaySession.cs:1841-1909`). It is default-off (`Configuration/SessionOptions.cs:69`), bounded, and testable via `TimeProvider`, so impact is minor; it is still a poll on the command hot path when enabled.
|
||||
|
||||
**Impact.** Minor latency/CPU when the option is enabled; none by default.
|
||||
|
||||
**Design.** If the option sees real use, replace the poll with a `TaskCompletionSource` pulsed on worker state transitions (the worker client already raises state changes internally). Register a waiter under `_syncRoot` in `GetReadyWorkerClientAsync` when the state is transient, and complete it when the worker becomes Ready or terminal, with the existing timeout as a `CancelAfter`. Keep the existing zero-timeout fail-fast path byte-for-byte. Because this is default-off and low-value, this is a defer-until-used item; the recommended action is to leave the poll but document it as a known trade-off unless the option is turned on in a real deployment.
|
||||
|
||||
**Implementation (if pursued).**
|
||||
- `Sessions/GatewaySession.cs`: add a state-change signal (`TaskCompletionSource`) set on worker Ready/terminal transitions; await it with the timeout instead of the poll loop.
|
||||
- Tests: `GatewaySessionTests` using `TimeProvider` — assert readiness is observed without the 25 ms granularity delay.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~GatewaySession"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-14 — Replay ring is a `LinkedList` with a node alloc per event `Low` · `P2`
|
||||
|
||||
**Finding.** The distributor replay ring is a `LinkedList<ReplayEntry>` (`Sessions/SessionEventDistributor.cs:108`), appended per event under `_replayLock` (`:766-793`). Capacity is fixed (`ReplayBufferCapacity`, default 1024).
|
||||
|
||||
**Impact.** A node allocation and poor cache locality per event on the fan-out hot path — exactly the shape a circular array serves allocation-free.
|
||||
|
||||
**Design.** Replace the `LinkedList` with a fixed-size ring array (`ReplayEntry[]` + head/count) sized to `ReplayBufferCapacity`. Keep the `_replayLock` protocol and the ascending-`WorkerSequence` ordering invariant (`:100-108`) unchanged; append overwrites the oldest slot, and `TryGetReplayFrom` binary-searches or linear-scans from the front. Age-based eviction (`_ageEvictionEnabled`) maps to advancing the tail index. Part of the P2 hot-path pass; behavior (replay contents, gap detection) must be identical.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/SessionEventDistributor.cs`: swap the backing store to a ring array with head/tail/count; update append (`:766-793`) and `TryGetReplayFrom` accordingly; keep capacity-0-disables-retention and retention-eviction semantics.
|
||||
- Tests: `SessionEventDistributorTests` (replay) must stay green — replay-from-sequence, gap detection, capacity overflow eviction, and age eviction; add a wraparound test crossing the ring boundary.
|
||||
- Docs: none (internal).
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~SessionEventDistributor"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-15 — Per-event gauge reads `ChannelReader.Count` every event `Low` · `P2`
|
||||
|
||||
**Finding.** `EventStreamService` reconciles the queue-depth gauge by reading `subscriber.Reader.Count` on every event (`Grpc/EventStreamService.cs:173-179`). Bounded-channel `Count` acquires the channel's internal lock; combined with the metric adjustment this adds measurable per-event overhead at high rates.
|
||||
|
||||
**Impact.** Minor per-event cost on the streaming hot path.
|
||||
|
||||
**Design.** Sample the backlog periodically instead of per event — e.g. every N events (a cheap counter modulo) or on a time interval — and reconcile the gauge then. The gauge is a diagnostic backlog indicator, not an exact-per-event quantity, so coarser sampling is acceptable. Keep the terminal reconcile in the `finally` (`:189-193`) so the gauge returns to zero on disconnect. Part of the P2 hot-path pass.
|
||||
|
||||
**Implementation.**
|
||||
- `Grpc/EventStreamService.cs:173-179`: gate the `Count`/`AdjustGrpcEventStreamQueueDepth` reconciliation behind `if ((++counter % SampleEvery) == 0)`; retain the `finally` zeroing.
|
||||
- Tests: existing streaming tests confirm the gauge zeroes on disconnect; add a case asserting the gauge is still reconciled at least at stream end.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~EventStream"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-16 — `Invoke` resolves the session twice per command `Low` · `—`
|
||||
|
||||
**Finding.** `Invoke` calls `ResolveSession(request.SessionId)` (`Grpc/MxAccessGatewayService.cs:104`) then `sessionManager.InvokeAsync(request.SessionId, …)` (`:122-124`) → `GetRequiredSession` (`Sessions/SessionManager.cs:161`) — a redundant `ConcurrentDictionary` lookup on every command.
|
||||
|
||||
**Impact.** Small but per-command overhead.
|
||||
|
||||
**Design.** Add an `ISessionManager.InvokeAsync(GatewaySession session, WorkerCommand, ct)` overload (or pass the already-resolved session through) so the service reuses the session it resolved for constraint application. Keep the existing `InvokeAsync(sessionId, …)` overload for other callers. No behavior change.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/SessionManager.cs`: add the session-typed overload; the existing one resolves then delegates to it.
|
||||
- `Grpc/MxAccessGatewayService.cs:122-124`: pass `session`.
|
||||
- Tests: `SessionManagerTests` — assert both overloads behave identically; existing `MxAccessGatewayService` invoke tests stay green.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~SessionManager"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-17 — Sessions layer throws `Grpc.Core.RpcException` `Low` · `—`
|
||||
|
||||
**Finding.** `SparseArrayExpander.Invalid` creates `new RpcException(new Status(StatusCode.InvalidArgument, …))` (`Sessions/SparseArrayExpander.cs:283-284`), invoked from `GatewaySession.NormalizeOutboundCommand` (`Sessions/GatewaySession.cs:984-1063`), so `GatewaySession.InvokeAsync` — a transport-agnostic session API also used by the alarm monitor — throws a gRPC type. `gateway.md:1063-1065` places translation at the gRPC layer.
|
||||
|
||||
**Impact.** Layering leak: a non-gRPC caller (alarm monitor, tests) receives a gRPC exception type; convention deviation from the style guide's layer boundaries.
|
||||
|
||||
**Design.** Throw a domain exception from the expander/session — reuse `SessionManagerException` with a `SessionManagerErrorCode` that maps to `InvalidArgument` (the codebase already routes these through `MxAccessGatewayService.MapException`). Add the mapping so the gRPC surface still returns `InvalidArgument`. Coordinate with GWC-03, which also adds an `InvalidArgument` rejection through the same path — both should use the domain exception. Verify no caller currently catches `RpcException` specifically from this path (the `Invoke` catch already rethrows non-`RpcException` via `MapException` at `:135`, so domain exceptions are handled).
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/SparseArrayExpander.cs:283-284`: return a domain exception (e.g. `SessionManagerException(SessionManagerErrorCode.InvalidArgument, message)`); rename `Invalid` accordingly. Add the error code if absent.
|
||||
- `Grpc/MxAccessGatewayService.cs` `MapException`: ensure the new code maps to `StatusCode.InvalidArgument`.
|
||||
- Tests: `SparseArrayExpanderTests` assert the domain exception type; `MxAccessGatewayService`/mapping tests assert it surfaces as `InvalidArgument` over gRPC.
|
||||
- Docs: confirm `gateway.md:1063-1065` layering statement now holds.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~SparseArrayExpander"` and `--filter "FullyQualifiedName~MxAccessGatewayService"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-18 — `GatewaySession` implements `DisposeAsync` without `IAsyncDisposable` `Low` · `—`
|
||||
|
||||
**Finding.** `public sealed class GatewaySession` declares no interfaces (`Sessions/GatewaySession.cs:13`) but exposes a public `async ValueTask DisposeAsync()` (`:1672`). `await using` does not compile against the type; disposal is discoverable only by convention.
|
||||
|
||||
**Impact.** Convention deviation; latent maintainability risk.
|
||||
|
||||
**Design.** Declare `: IAsyncDisposable` on the class. The method already matches the interface signature, so this is a declaration-only change with no behavior impact.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/GatewaySession.cs:13`: `public sealed class GatewaySession : IAsyncDisposable`.
|
||||
- Tests: existing lifecycle tests compile/run unchanged; optionally add an `await using` usage in a test.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~GatewaySession"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-19 — Dead `GatewaySession.KillWorker(string)` + stale doc `Low` · `—`
|
||||
|
||||
**Finding.** `GatewaySession.KillWorker(string)` (`Sessions/GatewaySession.cs:1615-1619`) has no callers (grep across `src/` and `clients/` finds none; the gated `KillWorkerWithCloseGateAsync` is what `SessionManager.KillWorkerAsync` uses). `docs/Sessions.md:57` still states `KillWorkerAsync` "calls `GatewaySession.KillWorker` directly".
|
||||
|
||||
**Impact.** Dead public method and a doc that describes a non-existent call path — violates the repo's docs-with-source rule.
|
||||
|
||||
**Design.** Delete the dead method and correct the doc in the same commit, per the repo convention. Confirm no reflection/DI reference exists (none found — it is a plain public method). This also removes one of the lock-free `_workerClient` reads flagged in GWC-11.
|
||||
|
||||
**Implementation.**
|
||||
- `Sessions/GatewaySession.cs:1615-1619`: delete `KillWorker`.
|
||||
- `docs/Sessions.md:57`: reword to describe `KillWorkerWithCloseGateAsync` (the actual forceful path).
|
||||
- Tests: none reference it; a full build confirms no dangling call site.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~GatewaySession"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-20 — Heartbeat config semantics conflated (send vs check interval) `Low` · `—`
|
||||
|
||||
**Finding.** `WorkerOptions.HeartbeatIntervalSeconds` is documented as "the interval in seconds for worker heartbeats" (`Configuration/WorkerOptions.cs:30-31`, default 5) but is bound to the gateway-side `HeartbeatCheckInterval` (`Sessions/SessionWorkerClientFactory.cs:86`), whose own default is 1 s (`Workers/WorkerClientOptions.cs:10`). Production checks every 5 s while unit-constructed clients check every 1 s, and the option name does not describe what it controls. Separately, `HeartbeatLoopAsync` uses raw `Task.Delay` (`Workers/WorkerClient.cs:400`) rather than the injected `TimeProvider`, unlike the rest of the class.
|
||||
|
||||
**Impact.** Misleading configuration surface; the option name implies the worker's *send* cadence but controls the gateway's *check* cadence. The raw `Task.Delay` makes the heartbeat loop the one loop not virtualizable under `TimeProvider` for tests.
|
||||
|
||||
**Design.** Rename the option to describe what it controls — add `HeartbeatCheckIntervalSeconds` (and correct the XML doc), keeping the old key as a deprecated alias for one release if config compatibility matters, or rename outright since this is pre-1.0. Fix the doc comment to state it is the gateway-side check interval. Separately, route `HeartbeatLoopAsync`'s delay through `_timeProvider` (e.g. a `PeriodicTimer` from `TimeProvider.CreateTimer` or `Task.Delay(interval, _timeProvider, token)`) for consistency and testability.
|
||||
|
||||
**Implementation.**
|
||||
- `Configuration/WorkerOptions.cs:30-31`: rename to `HeartbeatCheckIntervalSeconds`; fix the XML doc.
|
||||
- `Sessions/SessionWorkerClientFactory.cs:86`: bind the renamed option.
|
||||
- `Workers/WorkerClient.cs:400`: use the injected `TimeProvider` for the loop delay.
|
||||
- Tests: `WorkerClientTests` heartbeat-watchdog tests should drive the loop via `FakeTimeProvider` once the delay is virtualized; assert the check-interval semantics.
|
||||
- Docs: `docs/GatewayConfiguration.md` (key rename) and any heartbeat description in `docs/MxAccessWorkerInstanceDesign.md`.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~WorkerClient"` and `--filter "FullyQualifiedName~GatewayOptionsValidator"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-21 — `EventChannelFullModeTimeout` / `HeartbeatStuckCeiling` not configurable `Low` · `—`
|
||||
|
||||
**Finding.** `SessionWorkerClientFactory` populates only four of the six `WorkerClientOptions` fields (`Sessions/SessionWorkerClientFactory.cs:83-89`); `EventChannelFullModeTimeout` and `HeartbeatStuckCeiling` always use the hardcoded defaults (`Workers/WorkerClientOptions.cs:13,23`) and have no `WorkerOptions` counterparts.
|
||||
|
||||
**Impact.** Two operationally significant tunables (event-backlog fault window and the stuck-command watchdog ceiling) cannot be adjusted without a rebuild.
|
||||
|
||||
**Design.** Expose both under `MxGateway:Worker:*` (`EventChannelFullModeTimeoutMilliseconds`, `HeartbeatStuckCeilingSeconds`) with defaults matching the current constants (5000 ms, 75 s), bind them in the factory, and validate ranges in `GatewayOptionsValidator`. Coordinate with GWC-04: once the event enqueue is decoupled from the read loop, `EventChannelFullModeTimeout` becomes the fault window for the dedicated event writer task, and making it configurable is the natural companion. Alternatively, if the values should stay fixed, document them as non-configurable in `docs/GatewayConfiguration.md` — but given GWC-04, exposing them is preferred.
|
||||
|
||||
**Implementation.**
|
||||
- `Configuration/WorkerOptions.cs`: add the two options with defaults.
|
||||
- `Configuration/GatewayOptionsValidator.cs`: validate `> 0` (ceiling `>=` grace, matching the watchdog logic).
|
||||
- `Sessions/SessionWorkerClientFactory.cs:83-89`: bind both.
|
||||
- Tests: `GatewayOptionsValidatorTests` for the new bounds; `WorkerClientTests` confirming the configured timeout drives the overflow fault.
|
||||
- Docs: `docs/GatewayConfiguration.md`.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~GatewayOptionsValidator"` and `--filter "FullyQualifiedName~WorkerClient"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-22 — `StreamDisconnected` always labeled `"Detached"` `Low` · `—`
|
||||
|
||||
**Finding.** The single `metrics.StreamDisconnected(...)` call site hardcodes `"Detached"` in the `finally` block (`Grpc/EventStreamService.cs:195`); the fault/overflow paths above it (`:148-158`) do not differentiate the label.
|
||||
|
||||
**Impact.** The disconnect-reason dimension the metric implies is uninformative for diagnosing overflow-vs-fault-vs-client-cancel.
|
||||
|
||||
**Design.** Record the actual terminal cause. Track a terminal-reason variable in the stream method, set it to `worker-fault` in the `WorkerClientException` catch (`:148-158`), `canceled` on `OperationCanceledException` tied to the caller's token, `overflow` if the subscriber channel completed via overflow, and default `detached` for a clean end; pass it to `StreamDisconnected` in the `finally`. No new metric — only the existing label dimension becomes meaningful.
|
||||
|
||||
**Implementation.**
|
||||
- `Grpc/EventStreamService.cs`: introduce a `string terminalReason = "detached";` updated on each terminal branch; use it at `:195`.
|
||||
- Tests: `EventStreamService` tests asserting the label reflects fault vs cancel vs clean end.
|
||||
- Docs: `docs/Metrics.md` (or wherever the metric's label values are enumerated) if the reason set is documented — note this is in the metrics domain; coordinate with that owner.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests --filter "FullyQualifiedName~EventStream"`.
|
||||
|
||||
---
|
||||
|
||||
## GWC-23 — `MaxEventSubscribersPerSession` dead in default single-subscriber mode `Info` · `—`
|
||||
|
||||
**Finding.** `MaxEventSubscribersPerSession` is a knowingly dead knob under the default single-subscriber configuration; this is acknowledged and justified in a comment (`Configuration/GatewayOptionsValidator.cs:189-196`).
|
||||
|
||||
**Impact / action.** None. Recorded so it is not re-reported. No change proposed — the option becomes live when `AllowMultipleEventSubscribers` is enabled, and the validator comment already explains the intent. Leave as-is.
|
||||
|
||||
**Verification.** N/A (no change).
|
||||
Reference in New Issue
Block a user