Joseph Doherty
1a5a12a416
Six-domain re-review at the P2 merge (4f5371f): all prior Done claims
verified (none false, 10 partial), 47 new findings (1 High, 14 Medium),
with per-finding design/implementation plans and a new tracking register.
172 lines
34 KiB
Markdown
172 lines
34 KiB
Markdown
# Gateway Server Core — Remediation Design & Implementation (2026-07-12 cycle)
|
||
|
||
Source review: [10-gateway-core.md](../10-gateway-core.md) · Roadmap: [00-overall.md](../00-overall.md) · Generated: 2026-07-13
|
||
|
||
This document turns the 2026-07-12 re-review's **new** Gateway Server Core findings (GWC-24 through GWC-30) into buildable remediation entries. Every cited `path:line` was re-verified against `main` at `4f5371f`. Tiers follow the overall roadmap: GWC-25 is P0.1 (grouped with the client-side CLI-35/36 ReplayGap handling), GWC-24 is P1.6, and GWC-26/27/28 sit in the P2 batch; GWC-29/30 are untiered polish. Prior-cycle findings still open (GWC-05, 09, 10, 11, 12, 13, 16–22) are tracked in `archreview/remediation/10-gateway-core.md` and are not re-planned here, but two of them (GWC-10, GWC-21) are named below where a new fix should coordinate with them.
|
||
|
||
## Finding index
|
||
|
||
| ID | Sev | Tier | Eff | Dep | Status | Title |
|
||
|----|-----|------|-----|-----|--------|-------|
|
||
| GWC-24 | Medium | P1 | M | GWC-21 (coord) | Not started | Unbounded event staging channel: sustained slow drain grows memory silently and invisibly |
|
||
| GWC-25 | Medium | P0 | S | CLI-35/36 (coord) | Not started | Empty-ring ReplayGap sentinel carries `oldest_available_sequence = 0`, dead-streaming a compliant client |
|
||
| GWC-26 | Low | P2 | M | GWC-27 | Not started | Alarm monitor attaches its subscriber after SubscribeAlarms; window transitions bypass the feed, missed Acknowledge never repaired |
|
||
| GWC-27 | Low | P2 | S | GWC-26 | Not started | `AttachInternalEventSubscriber` bypasses the readiness gate; premature attach poisons the distributor permanently |
|
||
| GWC-28 | Low | P2 | S | GWC-10 (coord) | Not started | Gateway→worker envelope `sequence` stamped at creation, not at write — non-monotonic on the wire under concurrent invokes |
|
||
| GWC-29 | Low | — | S | — | Not started | `Invoke` deep-clones the entire request only to discard the cloned command |
|
||
| GWC-30 | Info | — | S | — | Not started | Frame reader allocates a fresh 4-byte length-prefix array per frame |
|
||
|
||
Dependency notes: GWC-26 and GWC-27 both change the internal-subscriber attach path (`GatewaySession.AttachInternalEventSubscriber` and its `SessionManager`/alarm-monitor callers) — land GWC-27's readiness gate first (or in the same commit), then GWC-26's reorder, so the reordered monitor attach is proven against the gate. GWC-24 is the direct successor of the prior cycle's GWC-04 backpressure fix and raises the value of the still-open GWC-21 (making `EventChannelFullModeTimeout` configurable); GWC-28 is the gateway half of the worker's WRK-04 fix and must be coordinated with the still-open GWC-10 if inbound sequence enforcement is ever added. GWC-25 is server-complete on its own, but the end-to-end reconnect story also needs the client-domain CLI-35/36 fixes (Python CLI crashes on the sentinel, Go CLI destroys it).
|
||
|
||
---
|
||
|
||
## GWC-24 — Unbounded event staging channel: sustained slow drain grows memory silently and invisibly `Medium` · `P1`
|
||
|
||
**Finding.** The GWC-04 remediation decoupled the read loop from event backpressure by staging events into `_eventStaging`, an **unbounded** channel (`Workers/WorkerClient.cs:93-100`). `StageWorkerEvent`'s `TryWrite` therefore always succeeds (`:565-573`), and the sustained-overflow `ProtocolViolation` fault fires only when a *single* timed `WriteAsync` against the bounded `_events` exceeds `EventChannelFullModeTimeout` (default 5 s, `:610-647`). The queue-depth gauge counts only `_events` — `_eventQueueDepth` is incremented in `EnqueueWorkerEventAsync` (`:616`, `:627`) and decremented in `ReadEventsCoreAsync` (`:289`), so staged-but-unqueued events are invisible to `SetWorkerEventQueueDepth`. The field comment (`:28-33`) claims staging "only fills during the bounded EventChannelFullModeTimeout window", which is true only for a full consumer stall, not for a consumer that drains slower than the worker produces while each individual write still completes inside the window.
|
||
|
||
**Impact.** Before GWC-04, a full `_events` blocked the read loop, which propagated backpressure through the pipe to the worker's own bounded queue — gateway memory per session was structurally bounded end-to-end. Now a slow-but-live distributor pump (each `WriteAsync` completing in, say, 4 s while the worker emits faster) grows `_eventStaging` without bound, without any fault, and without any metric showing it. The repo's fail-fast backpressure invariant currently rests on pump liveness (the pump's own fan-out is non-blocking `TryWrite`, `Sessions/SessionEventDistributor.cs:582-594`) rather than on channel bounds — unlikely to bite, but no longer structurally impossible, and undiagnosable when it does.
|
||
|
||
**Design.** Do **both** halves of the review's recommendation — restore a structural bound *and* make the backlog observable:
|
||
|
||
1. **Bound the staging channel** at `2 × EventChannelCapacity` (derived from the existing option; no new configuration key). Total gateway-side buffering per session becomes `3 × MxGateway:Events:QueueCapacity` (bounded consumer channel + 2× staging), which absorbs the same bursts the unbounded design absorbed in practice while capping the pathological case. Create it with `FullMode = BoundedChannelFullMode.Wait` so `TryWrite` returns `false` when full (the same Wait+TryWrite overflow-detection idiom the distributor documents at `SessionEventDistributor.cs:354-361`).
|
||
2. **Fault immediately on a staging `TryWrite` miss.** A full staging channel means `EventWriteLoopAsync` has been saturated against a full `_events` for however long the worker took to emit `2 × capacity` further events — that *is* the sustained-slow-drain signal, with no timer needed. `StageWorkerEvent` treats a `false` return as `SetFaulted(ProtocolViolation, …)` unless the client is already terminal (a completed channel also returns `false` during shutdown — guard with the existing `IsTerminalState()` so the shutdown path stays a silent drop, as documented today). `SetFaulted` is non-blocking, so calling it from `DispatchEnvelope` on the read loop preserves the GWC-04 guarantee that the read loop never awaits behind events. Keep the existing `EventChannelFullModeTimeout` timed-write fault in `EnqueueWorkerEventAsync` unchanged — it catches the full-stall case earlier than the staging bound would.
|
||
3. **Unify the queue-depth gauge over both channels.** Move the `Interlocked.Increment(ref _eventQueueDepth)` + `SetWorkerEventQueueDepth` from `EnqueueWorkerEventAsync` (`:616-617`, `:627-628`) to `StageWorkerEvent` (on successful `TryWrite`). The decrement already happens at consumer read (`ReadEventsCoreAsync:289`), so the single counter then reports *total undelivered events* (staged + queued) with no second counter and no extra hot-path work. Record `_metrics?.QueueOverflow("worker-event-staging")` on the staging fault so it is distinguishable from the existing `"worker-events"` overflow label.
|
||
|
||
Rejected alternatives: (a) unbounded channel + a total-depth ceiling check — same effect but needs a second configured ceiling and a watermark comparison per event; the bounded channel gives the ceiling for free and fails at exactly the point the invariant is violated. (b) fault-on-sustained-growth (timestamp when the backlog first appears, fault when it persists past a window) — more machinery and a weaker guarantee, since memory still grows throughout the window. Fail-fast is the repo's documented backpressure policy (`docs/DesignDecisions.md`); a loud session fault with a worker kill is the correct outcome, not silent buffering.
|
||
|
||
Coordinate with (do not block on) open GWC-21: if `EventChannelFullModeTimeout` is made configurable in the same pass, the 2× staging multiplier is the natural companion knob; until then the derived constant needs no validator change.
|
||
|
||
**Implementation.**
|
||
- `Workers/WorkerClient.cs` constructor (`:93-100`): replace `Channel.CreateUnbounded<WorkerEvent>` with `Channel.CreateBounded<WorkerEvent>(new BoundedChannelOptions(checked(2 * _options.EventChannelCapacity)) { SingleReader = true, SingleWriter = true, FullMode = BoundedChannelFullMode.Wait, AllowSynchronousContinuations = false })`. Rewrite the `_eventStaging` field comment (`:28-33`) — it must state the bound, the overflow fault, and that the depth gauge covers staged + queued.
|
||
- `StageWorkerEvent` (`:565-573`): on `TryWrite` success, `Interlocked.Increment(ref _eventQueueDepth)` + `_metrics?.SetWorkerEventQueueDepth(...)`; on failure, `if (IsTerminalState()) return;` then `_metrics?.QueueOverflow("worker-event-staging")` and `SetFaulted(WorkerClientErrorCode.ProtocolViolation, …)` with a message naming the staging capacity (`2 × EventChannelCapacity`), the current depth, and the actionable fix (attach/unblock the `StreamEvents` consumer or raise `MxGateway:Events:QueueCapacity`). Update the XML doc, which currently asserts `TryWrite` always succeeds.
|
||
- `EnqueueWorkerEventAsync` (`:610-647`): remove the two depth increments (now counted at staging); keep the timed `WriteAsync`, both fault paths, and the `Volatile.Read` depth in the overflow message.
|
||
- `ReadEventsCoreAsync` (`:284-293`): unchanged — the decrement site is already correct for the unified counter.
|
||
- Docs, same commit: `docs/MxAccessWorkerInstanceDesign.md` / `docs/GatewayProcessDesign.md` wherever the GWC-04 read-loop/event-writer decoupling is described (state the staging bound and the two fault modes); `docs/GatewayConfiguration.md` under `MxGateway:Events:QueueCapacity` (total per-session gateway buffering is 3× the value; overflow of either bound faults the session); the metrics doc if it defines the worker event queue-depth gauge (now includes staged events).
|
||
- Tests (`Gateway/Workers/WorkerClientTests.cs`), new:
|
||
- `StagingChannelOverflowFaultsWorkerWithoutWaitingForFullModeTimeout` — small `EventChannelCapacity` (e.g. 4) via `WorkerClientOptions`, no event consumer attached, long `EventChannelFullModeTimeout` (e.g. 5 min so the timed-write fault cannot be the trigger); push `> 3 × capacity` events through the fake pipe; assert the client faults `ProtocolViolation` with the staging message, and that a command reply interleaved before the fault still completed (read loop never stalled).
|
||
- `WorkerEventQueueDepthGaugeCountsStagedEvents` — no consumer, push N events (N below the staging bound but above `EventChannelCapacity`); assert the gauge reports N (staged + queued); attach the consumer, drain, assert the gauge returns to 0.
|
||
- Existing GWC-04 tests (reply-not-stalled-behind-events, sustained-overflow fault) must stay green.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~WorkerClientTests"`. Gateway-side only — no worker change, no windev/x86 needed.
|
||
|
||
---
|
||
|
||
## GWC-25 — Empty-ring ReplayGap sentinel carries `oldest_available_sequence = 0`, dead-streaming a compliant client `Medium` · `P0`
|
||
|
||
**Finding.** `RegisterWithReplay`'s empty-ring branch sets `oldestAvailableSequence = 0` even when `gap == true` (`Sessions/SessionEventDistributor.cs:463-467`), and `EventStreamService` emits that verbatim in the ReplayGap sentinel (`Grpc/EventStreamService.cs:133-139`, `:210-222`). The proto contract (`src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto`, `ReplayGap.oldest_available_sequence`) instructs a client that wants a gapless resume to send `after_worker_sequence = oldest_available_sequence − 1`. With `oldest = 0`, a uint64 client computes `2^64−1`; on the next resume `RegisterWithReplay` replays nothing and reports no gap (the `afterSequence == ulong.MaxValue` wrap guard at `:471`/`:757` suppresses it), and the live filter `mxEvent.WorkerSequence <= afterWorkerSequence` (`EventStreamService.cs:179`) then drops **every** live event — a silently dead stream. Reachable by default: `ReplayRetentionSeconds = 300` age-evicts inside `RegisterWithReplay` via `EvictAged()` (`:458`), and `ReplayBufferCapacity = 0` with a behind cursor hits the same branch. All five clients shipped the `oldest − 1` formula (CLI-15).
|
||
|
||
**Impact.** A client that reconnects after ≥ 5 minutes detached (or against a retention-disabled gateway) and follows the documented resume formula gets a permanently event-less stream with no error — the exact resilience feature (detach-grace + replay, on by default) fails in its headline scenario.
|
||
|
||
**Design.** In the empty-ring-with-gap branch, emit `oldest_available_sequence = _highestSequenceSeen + 1` — the next sequence that can possibly be delivered. The client's `oldest − 1` then equals `_highestSequenceSeen`: the follow-up resume replays nothing, reports no gap, and the live filter passes every event newer than the highest already seen. Nothing is lost relative to today (the evicted events are unrecoverable either way — the sentinel's whole job is to say "re-snapshot"), and no client changes. Keep `0` for the no-gap case, where the field is documented as meaningless and never emitted. `TryGetReplayFrom` (`:735-774`) does not surface an oldest-available value, so it needs no change. Rejected alternative: define `0` as a special "nothing retained" case in the proto and handle it in all five clients — five client releases plus a version bump (CLI-39 is already blocking publishes) versus a one-line server fix that keeps the single resume formula universal.
|
||
|
||
The proto comment currently states "`oldest_available_sequence` itself IS still retained", which becomes false in the empty-ring case — per the docs-with-source rule, amend the field comment in the same commit to define the empty-ring value ("when nothing is retained, this is the next sequence that can be delivered — `highest observed + 1` — and the `oldest − 1` resume formula remains valid; the interval evicted is unchanged"). This is a comment-only proto change (no descriptor delta), but the repo's codegen rules still apply — see the steps.
|
||
|
||
**Implementation.**
|
||
- `Sessions/SessionEventDistributor.cs:463-467`: replace `oldestAvailableSequence = 0;` with `oldestAvailableSequence = gap ? _highestSequenceSeen + 1 : 0;` plus a comment explaining the `oldest − 1` client formula this must keep valid (cite this finding).
|
||
- `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto` (`ReplayGap.oldest_available_sequence`, ~line 759): append the empty-ring sentence above. Then regenerate per repo rules: delete `src/ZB.MOM.WW.MxGateway.Contracts/Generated/*.cs`, `dotnet build src/ZB.MOM.WW.MxGateway.Contracts/ZB.MOM.WW.MxGateway.Contracts.csproj`, and **commit `Generated/`** (net48 worker builds break otherwise). Sync the vendored client copies of the proto byte-identical (`clients/*/`); a comment-only edit changes no descriptor, so: Python `*_pb2*` output is unchanged (comments are not embedded — regenerate with the pinned grpcio-tools only if the files actually differ), Go/C#/Rust generated doc comments will churn — regenerate those per each client README, and revert spurious Java aggregate-file churn if no message-level delta appears (per the established Java convention).
|
||
- `docs/Sessions.md` (~lines 228-234, ReplayGap section): document the empty-ring sentinel value and that `after_worker_sequence = oldest_available_sequence − 1` is the universal resume formula in both the retained and fully-evicted cases.
|
||
- Tests, new:
|
||
- `Gateway/Sessions/SessionEventDistributorTests.cs` — `RegisterWithReplayReportsNextDeliverableSequenceWhenRingEmptiedByAge`: FakeTimeProvider, capacity > 0, short retention; pump events up to highest sequence H; advance the clock past retention; `RegisterWithReplay(afterSequence: 1, …)` → asserts `gap == true`, `oldestAvailableSequence == H + 1`, `replayedEvents` empty, `liveResumeSequence == 1`.
|
||
- `RegisterWithReplayReportsNextDeliverableSequenceWithRetentionDisabled`: capacity-0 internal ctor overload, events seen to H, resume behind → `gap == true`, `oldestAvailableSequence == H + 1`.
|
||
- `ResumeUsingSentinelFormulaAfterEmptyRingGapDeliversLiveEvents`: after the gap above, re-register with `afterSequence = (H + 1) − 1 = H` → no gap, no replay, and a newly pumped event `H + 1` is delivered on the live channel — the direct regression test for the dead-stream failure.
|
||
- `Gateway/GatewayEndToEndReconnectReplayTests.cs` — `ReconnectAfterFullAgeEvictionResumesWithSentinelFormula`: fake-worker e2e; stream, detach, evict everything by advancing time past `ReplayRetentionSeconds`, reconnect with the last-seen watermark, apply `oldest − 1` from the received sentinel on the follow-up resume, assert new live events arrive.
|
||
|
||
Cross-domain: the client-side halves — Python CLI crashing on the sentinel (CLI-35) and the Go CLI destroying it (CLI-36) — are planned in the clients remediation doc; this server fix is independent and safe to land first (roadmap P0.1 groups all three).
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx` (includes the contracts regen); `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~SessionEventDistributorTests"` and `--filter "FullyQualifiedName~GatewayEndToEndReconnectReplay"`. If the proto comment lands: the committed `Generated/` keeps the net48 worker buildable, but an actual x86 worker build/test run needs windev — flag as the standard windev verification (comment-only change, so functional risk is nil).
|
||
|
||
---
|
||
|
||
## GWC-26 — Alarm monitor attaches its subscriber after SubscribeAlarms; window transitions bypass the feed, missed Acknowledge never repaired `Low` · `P2`
|
||
|
||
**Finding.** `RunMonitorAsync` invokes `SubscribeAlarmsAsync` and the first `ReconcileAsync` (`Alarms/GatewayAlarmMonitor.cs:213-214`) *before* the internal distributor subscriber is attached inside `_sessionManager.ReadAlarmEventsAsync` (`:232-234` → `Sessions/SessionManager.cs:195-208` → `GatewaySession.AttachInternalEventSubscriber`, `Sessions/GatewaySession.cs:553-562`). The distributor pump has been running since `MarkReady` started the dashboard mirror (`GatewaySession.cs:445-449`, `:621`), and a late subscriber misses previously fanned events by design (`SessionEventDistributor.cs:579-582`). Raise/Clear lost in the window are repaired by the next reconcile, but `ApplyReconcile` broadcasts only presence deltas (`GatewayAlarmMonitor.cs:514-553`): an Acknowledge that lands in the window updates `_alarms` silently on the snapshot replace (`:547-551`) with no `Broadcast`, so live `StreamAlarms` subscribers show the alarm unacked until it clears.
|
||
|
||
**Impact.** A one-shot window of a few hundred ms (two worker command round-trips) per monitor lifecycle in which alarm transitions reach only the dashboard mirror; a missed Acknowledge is never broadcast to alarm-feed subscribers. Same defect class as the fixed GWC-01, shrunk from permanent to transient.
|
||
|
||
**Design.** Two independent halves:
|
||
|
||
1. **Attach before subscribing.** The monitor already holds the `GatewaySession` instance returned by `OpenSessionAsync` (`:203-208`), so take the internal lease directly — `session.AttachInternalEventSubscriber()` — *before* `SubscribeAlarmsAsync`, and enumerate `lease.Reader.ReadAllAsync(...)` after reconcile. Transitions arriving during subscribe + reconcile simply buffer in the lease's bounded channel (`MxGateway:Events:QueueCapacity`, default 1024 — ample for a two-round-trip window); if it ever overflowed, the internal subscriber is merely disconnected (never faults the session, per the `isInternal` contract), the enumeration ends, and the monitor's supervisor loop restarts — acceptable and loud. Processing buffered transitions *after* `ApplyReconcile` is order-safe: `ApplyTransition` handles alarms already present in the reconciled snapshot. With the monitor calling the session directly, `ISessionManager.ReadAlarmEventsAsync` loses its only caller — remove it rather than leave dead public surface (mirrors the GWC-19 precedent). Rejected alternative: buffer inside `ReadAlarmEventsAsync` by splitting attach/enumerate on the manager — same effect, but keeps an indirection whose only job was to hide the session object the monitor already has.
|
||
2. **Repair acked-state in reconcile.** In `ApplyReconcile`, for references present in both `_alarms` and the incoming snapshot, broadcast an `Acknowledge` feed transition when the state changed to acked (`existing.CurrentState != incoming.CurrentState && incoming.CurrentState == AlarmConditionState.ActiveAcked`), via the existing `TransitionFromSnapshot(incoming, AlarmTransitionKind.Acknowledge)`. This is defense-in-depth for any future missed window (worker restart, overflow disconnect), not just this one. Invariant note: `ApplyReconcile` already broadcasts snapshot-derived Raise/Clear transitions on the **alarm feed** (`AlarmFeedMessage`, the StreamAlarms/dashboard surface — see the method comment "broadcasting a synthetic transition for any alarm the live stream missed"); extending that established feed-level repair to acked-state deltas does not touch the gRPC `StreamEvents` path and therefore does not violate the "never synthesize events" rule, which governs `MxEvent` emission.
|
||
|
||
**Implementation.**
|
||
- `Alarms/GatewayAlarmMonitor.cs` `RunMonitorAsync` (`:211-250`): after `OpenSessionAsync`/`_session` publication, `using IEventSubscriberLease alarmLease = session.AttachInternalEventSubscriber();` **before** `SubscribeAlarmsAsync(...)`; replace the `_sessionManager.ReadAlarmEventsAsync(...)` enumeration with `await foreach (MxEvent mxEvent in alarmLease.Reader.ReadAllAsync(linked.Token))`; update the `:228-231` comment to state the attach-before-subscribe ordering and why.
|
||
- `Sessions/SessionManager.cs:195-208` and `ISessionManager`: remove `ReadAlarmEventsAsync` (re-verify zero remaining callers with a grep at implementation time; repoint any test that used it at `GatewaySession.AttachInternalEventSubscriber`).
|
||
- `Alarms/GatewayAlarmMonitor.cs` `ApplyReconcile` (`:514-553`): before the snapshot replace, add the both-present acked-delta loop described above.
|
||
- Docs, same commit: `docs/Sessions.md`'s note that the alarm monitor attaches as an internal distributor subscriber (added by the GWC-01 fix) — extend with "attached before SubscribeAlarms so no transition window is missed"; `gateway.md`'s alarm-monitor section if it describes the startup ordering.
|
||
- Tests:
|
||
- `GatewayAlarmMonitorAttachOrderTests` (new, or extend the existing alarm monitor tests in `Alarms/`): fake worker emits an `Acknowledge` (and a `Raise`) transition immediately after the `SubscribeAlarms` reply and before the first reconcile reply; assert a `StreamAlarms` subscriber observes both — the direct regression test for the window.
|
||
- `ApplyReconcileBroadcastsAcknowledgeDelta` (unit): seed the cache with an `Active` alarm, reconcile with the same reference `ActiveAcked`; assert exactly one `Acknowledge` feed message and no Raise/Clear.
|
||
- Existing alarm monitor + `SessionManagerTests` suites stay green after the `ReadAlarmEventsAsync` removal.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayAlarmMonitor"` and `--filter "FullyQualifiedName~SessionManagerTests"`. Depends on GWC-27 landing first (or together) so the earlier attach point runs against the readiness gate.
|
||
|
||
---
|
||
|
||
## GWC-27 — `AttachInternalEventSubscriber` bypasses the readiness gate; premature attach poisons the distributor permanently `Low` · `P2`
|
||
|
||
**Finding.** `GatewaySession.AttachInternalEventSubscriber` (`Sessions/GatewaySession.cs:553-562`) runs `EnsureDistributorCreated` → `Register(isInternal: true)` → `StartPumpIfRequested` with no state check, unlike `AttachEventSubscriber`, which requires `_state == Ready && _workerClient?.State == Ready` under `_syncRoot` (`:889-896`). `SessionManager.ReadAlarmEventsAsync` (`Sessions/SessionManager.cs:195-208`) exposes the unchecked path publicly. If invoked before Ready, the pump's source (`MapWorkerEventsAsync` → `GetReadyWorkerClientAsync`, `:740-749`, `:1910`) throws `SessionNotReady`; `PumpAsync` completes all subscribers with the error and latches `_completed` (`SessionEventDistributor.cs:603-612`, `:662-680`). `_eventDistributorStarted` is never reset (`GatewaySession.cs:527-531`) and the distributor is replaced only in `DisposeAsync`, so every later registration — including gRPC `StreamEvents` — receives an immediately-completed channel carrying the stale error: a Ready session with permanently dead event streaming. Unreachable today (the alarm monitor attaches only after `OpenSessionAsync` returns Ready), but one future call site away and failing in the worst way (silent, permanent, per-session).
|
||
|
||
**Impact.** Latent lifecycle hazard: any refactor or new caller that attaches internally before Ready silently kills event streaming for that session's whole lifetime, with no fault and no log pointing at the cause.
|
||
|
||
**Design.** Mirror `AttachEventSubscriber`'s readiness gate at the top of `AttachInternalEventSubscriber`: under `_syncRoot`, throw `SessionManagerException(SessionManagerErrorCode.SessionNotReady, …)` when `_state != SessionState.Ready || _workerClient?.State != WorkerClientState.Ready`, *before* `EnsureDistributorCreated` — so a premature attach fails loudly and never constructs/starts the distributor. Rejected alternative: make the distributor resettable after a pump fault — a much larger lifecycle change (replay ring, `_completed` latch, dashboard mirror lease all assume one distributor per session) that would also mask caller bugs instead of surfacing them. `StartDashboardMirror` keeps its direct `distributor.Register(isInternal: true)` path — it is called from `MarkReady` under its own state guard (`:604`) and is not a public entry point.
|
||
|
||
**Implementation.**
|
||
- `Sessions/GatewaySession.cs:553-562`: add the `_syncRoot` readiness check (reuse the exact exception code and message shape from `AttachEventSubscriber:891-896`); then proceed with the existing `EnsureDistributorCreated`/`Register`/`StartPumpIfRequested` sequence outside the lock (matching `AttachEventSubscriber`'s lock discipline — the check is under the lock, the distributor calls are not). Extend the XML remarks to state the gate.
|
||
- `Sessions/SessionManager.cs:195-208`: no separate check needed — the path inherits the gate via the session call, and GWC-26 removes the method entirely.
|
||
- Tests (`Gateway/Sessions/GatewaySessionTests.cs`), new `AttachInternalEventSubscriberBeforeReadyThrowsAndDoesNotPoisonDistributor`: on a not-yet-Ready session, assert `SessionManagerException`/`SessionNotReady`; then drive the session to Ready and assert `AttachEventSubscriber` still yields a live stream (the key assertion: the distributor was never created/started by the failed attach).
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewaySessionTests"`. Land before (or with) GWC-26.
|
||
|
||
---
|
||
|
||
## GWC-28 — Gateway→worker envelope `sequence` stamped at creation, not at write `Low` · `P2`
|
||
|
||
**Finding.** `CreateEnvelope` stamps `Sequence = (ulong)Interlocked.Increment(ref _nextSequence)` at envelope construction (`Workers/WorkerClient.cs:1031-1044`); envelopes are then enqueued to `_outboundEnvelopes` (`EnqueueAsync:957-972`) and written by the single `WriteLoopAsync` (`:390-409`); `WorkerFrameWriter` does not re-stamp. Two concurrent `InvokeAsync` callers can stamp 1 and 2 but enqueue in the order 2, 1 — non-monotonic sequences on the pipe, violating `gateway.md`'s "monotonic per sender" contract. This is exactly what WRK-04 fixed on the worker side (stamp at the point of writing, under the writer's serialization); the gateway half was not given the same treatment. Benign today — neither side validates inbound sequence (GWC-10/IPC-10 open) — but it would start faulting sessions the moment worker-side validation is added.
|
||
|
||
**Impact.** Contract violation lying in wait: adding the specified sequence enforcement on the worker side (the mirror of GWC-10) would immediately fault healthy sessions under concurrent command load.
|
||
|
||
**Design.** Stamp at write, matching WRK-04: `WriteLoopAsync` assigns `envelope.Sequence` immediately before `_writer.WriteAsync`. The write loop is the channel's single consumer (`SingleReader = true`, `:74`), so writes are naturally serialized — the counter becomes a plain `ulong` incremented only on the write loop, and the `Interlocked` goes away. Every outbound envelope flows through `_outboundEnvelopes` (hello `:150`, commands `:223`, shutdown `:317`), so coverage is total. Rejected alternative: re-stamp inside `WorkerFrameWriter` — the frame writer is a pure framing layer shared with the `FakeWorkerHarness` and tests; giving it protocol state would leak envelope semantics into the wire layer on both sides.
|
||
|
||
**Implementation.**
|
||
- `Workers/WorkerClient.cs:1031-1044` (`CreateEnvelope`): remove the `Sequence` assignment; add a comment that the sequence is stamped in `WriteLoopAsync` at the point of writing (cite WRK-04 parity and this finding).
|
||
- `Workers/WorkerClient.cs:390-409` (`WriteLoopAsync`): before `_writer.WriteAsync(envelope, …)`, `envelope.Sequence = ++_nextSequence;` — change the `_nextSequence` field (`:37`) from `long` + `Interlocked` to a plain `ulong` touched only by the write loop, and update its usage accordingly.
|
||
- Docs, same commit: the `gateway.md` envelope-sequence paragraph (the "monotonic per sender" contract, ~line 314) — note both sides stamp at write under their single write path; cross-reference open GWC-10 for the (still absent) inbound enforcement.
|
||
- Tests (`Gateway/Workers/WorkerClientTests.cs`), new `ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire`: fake pipe; fire N (e.g. 32) parallel `InvokeAsync` calls; capture the frames the "worker" side receives; assert the `Sequence` values are strictly increasing in wire order. Existing handshake/shutdown tests confirm hello/shutdown envelopes still carry sequences.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~WorkerClientTests"` and `--filter "FullyQualifiedName~WorkerFrameProtocolTests"`. Gateway-side only; the worker's own WRK-04 stamping is untouched.
|
||
|
||
---
|
||
|
||
## GWC-29 — `Invoke` deep-clones the entire request only to discard the cloned command `Low` · `—`
|
||
|
||
**Finding.** `MxAccessGatewayService.Invoke` runs `MxCommandRequest invokeRequest = request.Clone(); invokeRequest.Command = commandToInvoke;` (`Grpc/MxAccessGatewayService.cs:119-120`) — a deep clone of the whole request *including its command payload* (potentially a large bulk-write graph), whose cloned command is immediately overwritten and discarded. `MapCommand` then performs the one clone actually needed (`request.Command.Clone()`, `Grpc/MxAccessGrpcMapper.cs:27-37`). Net cost: one full wasted command deep-clone per `Invoke` — the same cost class GWC-07/IPC-05 eliminated on the event and envelope paths.
|
||
|
||
**Impact.** Avoidable allocation and copy proportional to command size on every command, worst for exactly the bulk writes that are largest.
|
||
|
||
**Design.** Skip the request clone entirely: add a `MapCommand(MxCommand command)` overload that builds the `WorkerCommand` from the command alone (`Command = command.Clone()`, `EnqueueTimestamp` as today) — `MapCommand` reads nothing else from the request. `Invoke` passes `commandToInvoke` directly. Keep the existing `MapCommand(MxCommandRequest)` overload delegating to the new one so other callers and tests are untouched. The single remaining clone is still required and must stay: `commandToInvoke` may be the caller-owned `request.Command` (gRPC-owned graph) and is also read *after* the invoke by `session.TrackCommandReply(commandToInvoke, …)` (`MxAccessGatewayService.cs:132`), so ownership transfer (à la GWC-07) is not safe here — the clone in `MapCommand` is what keeps `WorkerClient.CreateCommandEnvelope`'s documented no-aliasing invariant (`Workers/WorkerClient.cs:1000-1004`) true. That comment stays accurate unchanged.
|
||
|
||
**Implementation.**
|
||
- `Grpc/MxAccessGrpcMapper.cs`: add `public WorkerCommand MapCommand(MxCommand command)` (null-check, clone, timestamp); retarget `MapCommand(MxCommandRequest request)` to validate and delegate.
|
||
- `Grpc/MxAccessGatewayService.cs:119-121`: delete the `request.Clone()` and the `invokeRequest` local; `WorkerCommand workerCommand = mapper.MapCommand(commandToInvoke);`.
|
||
- Tests: `Gateway/Grpc/MxAccessGrpcMapperTests.cs` — new `MapCommandFromCommandClonesPayload` (mutating the returned `WorkerCommand.Command` does not affect the input command; `EnqueueTimestamp` populated; both overloads produce equal results). Existing `MxAccessGatewayServiceTests` and `MxAccessGatewayServiceConstraintTests` must stay green — the constraint path (`bulkConstraintPlan.Command` flowing through, denied-merge on the reply) is the behavior most worth re-running.
|
||
- Docs: none (internal hot-path change; no contract or config surface).
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~MxAccessGrpcMapperTests"` and `--filter "FullyQualifiedName~MxAccessGatewayService"`.
|
||
|
||
---
|
||
|
||
## GWC-30 — Frame reader allocates a fresh 4-byte length-prefix array per frame `Info` · `—`
|
||
|
||
**Finding.** `WorkerFrameReader.ReadAsync` allocates `new byte[sizeof(uint)]` per frame (`Workers/WorkerFrameReader.cs:33`); the GWC-08 pass pooled the payload buffer but left the prefix. Gen-0, 4 bytes — negligible; recorded so the next hot-path pass knows it was seen.
|
||
|
||
**Impact.** One tiny short-lived allocation per inbound frame. No functional risk.
|
||
|
||
**Design.** A reusable per-reader scratch field: `private readonly byte[] _lengthPrefix = new byte[sizeof(uint)];`. The reader is single-consumer by construction — exactly one read loop per `WorkerClient` calls `ReadAsync`, never concurrently (handshake reads happen before the read loop starts) — so a per-instance buffer is safe; document that `ReadAsync` is not reentrant. Rejected: pooling 4 bytes via `ArrayPool` — rent/return overhead exceeds the allocation it saves.
|
||
|
||
**Implementation.**
|
||
- `Workers/WorkerFrameReader.cs`: add the `_lengthPrefix` field, use it at `:33-36`, and add a one-line class remark that `ReadAsync` must not be invoked concurrently (single-consumer contract).
|
||
- Tests: existing `Gateway/Workers/WorkerFrameProtocolTests.cs` round-trips cover it; add (if absent) a sequential multi-frame read on one reader instance asserting all frames parse — guarding the shared-scratch reuse.
|
||
- Docs: none.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~WorkerFrameProtocolTests"`.
|