Joseph Doherty
f4919ba97c
Six domain reports + overall synthesis. All 47 cycle-2 Done claims verified at HEAD with zero false Dones and zero regressions; 34 new findings (0 Critical, 0 High, 4 Medium, 25 Low, 5 Info): GWC-31..38, WRK-32..34, IPC-33..39, SEC-37..41, CLI-46..50, TST-31..36.
145 lines
34 KiB
Markdown
145 lines
34 KiB
Markdown
# Gateway Server Core — Cycle-3 Review
|
||
|
||
- **Date:** 2026-08-19
|
||
- **Commit:** `8b3e3b3` (`main`, clean)
|
||
- **Baseline:** `4f5371f` (2026-07-12 cycle-2 review; ~288 commits since, ~99 of them touching `src/ZB.MOM.WW.MxGateway.Server`)
|
||
- **Method:** static review on macOS; every cited file read at HEAD. No build, no test, no source change.
|
||
- **Scope:** gateway server process — `Sessions/`, `Workers/`, `Grpc/` (`MxAccessGatewayService`, `EventStreamService`, mapper, validator), `Alarms/` (monitor, watch-list resolver, subtag-fallback gateway half), the session/alarm-feed plumbing the dashboard consumes (`DashboardLiveDataService` session use, `AlarmsHubPublisher` feed consumption), and the `ZB.MOM.WW.GalaxyRepository` package adoption (`AddZbGalaxyRepository`/`MapZbGalaxyRepository` wiring, `GatewayBrowseScopeProvider`, `DashboardGalaxySummaryProjector`). Excludes `Security/` internals and dashboard UI (security-dashboard reviewer), except the browse-scope provider assigned here.
|
||
- **Prior reports:** `archreview/2026-07-12/10-gateway-core.md`; remediation status source `archreview/2026-07-12/remediation/00-tracking.md` (+ `10-gateway-core.md`); first-cycle backlog `archreview/remediation/00-tracking.md`.
|
||
|
||
## Prior-finding verification
|
||
|
||
### Cycle-2 findings (GWC-24..30, all tracked Done)
|
||
|
||
Every Done claim was verified against code read at HEAD. **All 7 hold; 3 are verified with caveats** — in each case the fix is present and sound for the reported defect, but a residual failure mode one layer out survives and is filed as a new finding.
|
||
|
||
| ID | Claimed | Verified? | Evidence (HEAD) | Notes |
|
||
|----|---------|-----------|-----------------|-------|
|
||
| GWC-24 | Done | ✅ holds, **with caveat** | `Workers/WorkerClient.cs:107-119` (staging channel bounded at `checked(2 * EventChannelCapacity)`, Wait+TryWrite idiom documented), `:646-676` (`StageWorkerEvent` faults `ProtocolViolation` on a rejected `TryWrite`, message names the bound, the depth, and the actionable fix), `:653-658`/`:350-359` (single `_eventQueueDepth` counter incremented at staging, decremented at consumer read — total undelivered depth), `:120-121` + `Metrics/GatewayMetrics.cs:524` (pull-model gauge registration), `:44-48` (field comment states bound + fault + gauge coverage); startup overflow guard `Configuration/GatewayOptionsValidator.cs:343-357` (`QueueCapacity ≤ int.MaxValue / 2` so the 2× derivation cannot overflow); regression tests `Tests/Gateway/Workers/WorkerClientTests.cs:781` (`StagingChannelOverflowFaultsWorkerWithoutWaitingForFullModeTimeout`), `:865` (`WorkerEventQueueDepthGaugeCountsStagedEvents`) | Implemented exactly per the remediation design, including the shutdown-drop guard (`IsTerminalState()`), and the design's extra validator guard. Caveat: the shutdown-drop guard does not cover `DisposeAsync`, which completes the channels **without** entering a terminal state — a dispose-race `TryWrite` failure is then misdiagnosed as the backpressure fault → **GWC-32**. A faulted-but-retained client also keeps contributing stale depth to the gauge until disposal → **GWC-33** (Info). |
|
||
| GWC-25 | Done | ✅ holds | `Sessions/SessionEventDistributor.cs:519-535` (empty-ring branch: `oldestAvailableSequence = gap ? _highestSequenceSeen + 1 : 0`, with a full explanatory comment citing the finding), out-param contract updated `:430-440`; emitted verbatim `Grpc/EventStreamService.cs:139-145`, `:227-239`; proto comment amended `Contracts/Protos/mxaccess_gateway.proto:797-798` ("When nothing is retained … `highest observed + 1` … resume formula remains valid"); `docs/Sessions.md:262` (dedicated empty-ring paragraph); tests `Tests/Gateway/Sessions/SessionEventDistributorTests.cs:935` (`ResumeUsingSentinelFormulaAfterEmptyRingGapDeliversLiveEvents`) and `Tests/Gateway/GatewayEndToEndReconnectReplayTests.cs:246` (`ReconnectAfterFullAgeEvictionResumesWithSentinelFormula`) | Sound end to end. The `afterSequence == ulong.MaxValue` wrap guards in both branches (`:521`, `:540`) are preserved, so the old dead-stream input is also inert. Proto + docs + server + tests all landed as one story; the client halves (CLI-35/36, NEXT-02) are closed in the tracking record. |
|
||
| GWC-26 | Done | ✅ holds, **with caveat** | `Alarms/GatewayAlarmMonitor.cs:246-258` (internal lease attached **before** `SubscribeAlarmsAsync` + first `ReconcileAsync`, with the window rationale in the comment), `:272-281` (drain the same lease after reconcile; order-safety noted), `ApplyReconcile` acked-delta repair `:674-684` (both-present, `existing.CurrentState != incoming.CurrentState && incoming == ActiveAcked` → `Acknowledge` feed transition); `ISessionManager.ReadAlarmEventsAsync` removed (grep: absent from `Sessions/ISessionManager.cs` and `SessionManager.cs`); repair-scope + at-least-once contract documented `:607-633`; regression tests `Tests/Alarms/GatewayAlarmMonitorAttachOrderTests.cs` | Implemented per design, including the method-removal half and the "never synthesize MxEvents" boundary note (`:610-614`: repairs are `AlarmFeedMessage`s, never `MxEvent`s). Caveat: the state-delta repair is one-directional — only unacked→acked is repaired; an acked alarm silently replaced by a new **unacked** instance is absorbed without a feed transition → **GWC-35**. |
|
||
| GWC-27 | Done | ✅ holds, **with caveat** | `Sessions/GatewaySession.cs:590-617` (`AttachInternalEventSubscriber` readiness gate under `_syncRoot` before `EnsureDistributorCreated`, same exception code/message shape as `AttachEventSubscriber:955-961`; the poison mechanism is spelled out in the comment); test `Tests/Gateway/Sessions/GatewaySessionTests.cs:683` (`AttachInternalEventSubscriberBeforeReadyThrowsAndDoesNotPoisonDistributor`) | The premature-attach poison path is closed. Caveat: the disposal-side twin remains — `EnsureDistributorCreated` recreates a never-started distributor after `DisposeAsync` nulls `_eventDistributor`, silently hanging a racing attach → **GWC-34**. |
|
||
| GWC-28 | Done | ✅ holds | `Workers/WorkerClient.cs:463-472` (`WriteLoopAsync` stamps `envelope.Sequence = unchecked(++_nextSequence)` immediately before `_writer.WriteAsync`, single-consumer rationale in comment), `:49-51` (field is a plain `ulong` touched only by the write loop, no interlock), `CreateEnvelope:1225-1241` (sequence deliberately unset, comment cites GWC-28/WRK-04); test `Tests/Gateway/Workers/WorkerClientTests.cs:228` (`ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire`) | Exactly the WRK-04-mirror design. All outbound envelopes flow through `_outboundEnvelopes` (hello `:171`, commands `:251`, cancel `:849/:858`, shutdown `:383`), so coverage is total. Note the `InvokeAsync` size pre-check comment (`:236-241`) correctly explains why the computed size cannot be reused as the writer's `knownSize` post-GWC-28 — the interaction was thought through. |
|
||
| GWC-29 | Done | ✅ holds | `Grpc/MxAccessGrpcMapper.cs:43-55` (new `MapCommand(MxCommand)` overload; the one required clone kept with the aliasing rationale), `:27-33` (request overload delegates); `Grpc/MxAccessGatewayService.cs:131-136` (`mapper.MapCommand(commandToInvoke)` — the request clone and `invokeRequest` local are gone; `TrackCommandReply` still reads `commandToInvoke` at `:147`, which is why the inner clone must stay and does) | Sound. `WorkerClient.CreateCommandEnvelope`'s no-aliasing invariant comment (`:1194-1198`) remains accurate. |
|
||
| GWC-30 | Done | ✅ holds | `Workers/WorkerFrameReader.cs:22-24` (per-reader `_lengthPrefix` scratch buffer), `:46` (used per frame), `:11-16` (class remark documents the non-reentrancy contract) | As designed. (Unrelatedly, the *label* "GWC-30" has since been reused by an off-register perf pass for a different change — see **GWC-31**.) |
|
||
|
||
### Cycle-1 findings still open at cycle 2 — re-checked at HEAD
|
||
|
||
All were re-read at HEAD. **None regressed; none silently fixed; two materially changed shape** (GWC-16 is now an explicitly documented deferral; GWC-20's client-side half improved).
|
||
|
||
| ID | Cycle-2 status | HEAD status | Evidence |
|
||
|----|----------------|-------------|----------|
|
||
| GWC-05 | Open | **Still open** | `Sessions/SessionWorkerClientFactory.cs:175-185` — pipe created with explicit 128 KiB buffers (the zero-quota fix, `da84635`) but still no ACL and no `PipeOptions.CurrentUserOnly`. The local pipe-squatting exposure and the `gateway.md` pipe-security contract gap are unchanged. |
|
||
| GWC-09 | Open | **Still open** | `Workers/WorkerProcessStartedProbe.cs:5-19` (no-op `HasExited` probe throwing `WorkerProcessLaunchException`); `Workers/WorkerProcessLauncher.cs:331-339` (`ShouldRetryStartupProbe` still excludes `WorkerProcessLaunchException`); `Configuration/WorkerOptions.cs:19-22` (both dead retry options remain). The retry pipeline still can never retry. |
|
||
| GWC-10 | Open | **Still open** | `Workers/WorkerEnvelopeValidator.cs:15-39` — version/session/body only; no inbound sequence tracking anywhere in `WorkerClient`. (The outbound half was fixed by GWC-28; enforcement remains unimplemented on both sides, consistent with IPC-31's recorded decision.) |
|
||
| GWC-11 | Open | **Still open** | `Sessions/GatewaySession.cs:23` (`_workerClient` not volatile); lock-free reads at `:308` (`WorkerProcessId`), `:313` (`WorkerClient`), `:1579` (`CloseAsync`), `:1732`, `:1765`, `:1877` (`DisposeAsync`). |
|
||
| GWC-12 | Open | **Still open** | `Workers/WorkerClient.cs:419-426` — plain `if (_disposed) return; _disposed = true;`, no interlock. (Now compounds with new GWC-32: dispose also never enters a terminal state.) |
|
||
| GWC-13 | Open | **Still open** | `Sessions/GatewaySession.cs:1956-2024` — 25 ms poll loop, routed through the injected `TimeProvider` (unchanged since cycle 2). |
|
||
| GWC-16 | Open | **Still open — now a recorded deferral; recommend closing as Won't-fix** | `Grpc/MxAccessGatewayService.cs:111-115` — a `PERF(followup)` comment now documents the double session lookup as measured-negligible and deliberately not collapsed (the fix would duplicate `InvokeAsync`'s fault mapping at a second entry point). The rationale is sound; the tracker row should be flipped to `Won't fix` citing this comment rather than left `Not started`. |
|
||
| GWC-17 | Open | **Still open** | `Sessions/SparseArrayExpander.cs:306` (`RpcException` factory below the gRPC layer), `:75` (GWC-03's cap rejection still flows through the same leak); invocation comment at `Sessions/GatewaySession.cs:1103-1111` still documents the layering leak explicitly. |
|
||
| GWC-18 | Open | **Still open** | `Sessions/GatewaySession.cs:14` — no `IAsyncDisposable`; `DisposeAsync` at `:1787`. |
|
||
| GWC-19 | Open | **Still open** | `Sessions/GatewaySession.cs:1730-1734` — `KillWorker(string)`; grep across `src/` + `clients/` finds zero callers (one doc-comment mention in `SessionManagerTests.cs:1433`). |
|
||
| GWC-20 | Open | **Still open (client half improved)** | `Configuration/WorkerOptions.cs:52` — the config key is still `HeartbeatIntervalSeconds` ("interval … for worker heartbeats") while it is bound to the **check** interval at `Sessions/SessionWorkerClientFactory.cs:98`; the `WorkerClientOptions` field was renamed to `HeartbeatCheckInterval` (`Workers/WorkerClientOptions.cs:40`), which fixes the client-side naming but leaves the config-key conflation. The heartbeat loop still uses raw `Task.Delay` without `TimeProvider` (`Workers/WorkerClient.cs:534`). |
|
||
| GWC-21 | Open | **Still open** | `Sessions/SessionWorkerClientFactory.cs:95-101` populates 4 of 6 `WorkerClientOptions`; `EventChannelFullModeTimeout` / `HeartbeatStuckCeiling` remain fixed defaults (`Workers/WorkerClientOptions.cs:13`, `:23`). Post-GWC-24 the staging bound is *derived* from `EventChannelCapacity` (which is configurable), so the exposure gap is narrower than at cycle 2 but the two knobs are still hard-coded. |
|
||
| GWC-22 | Open | **Still open** | `Grpc/EventStreamService.cs:217` — `metrics.StreamDisconnected("Detached")` hardcoded in the `finally`; overflow/fault exits are not differentiated. |
|
||
| GWC-23 | N/A | Unchanged | Decision stands; multi-subscriber mode remains config-gated and the validator comment survives. |
|
||
|
||
### NEXT rows touching this domain
|
||
|
||
- **NEXT-01** (macOS `sun_path` pipe names) — **verified resolved**: `Sessions/SessionManager.cs:532-536` builds `mxgw-{Environment.ProcessId}-{sessionUid}` with the length-budget rationale in the comment; `CLAUDE.md`/`gateway.md` describe the same format.
|
||
- **NEXT-03** (reconcile/live duplicate broadcasts) — **verified resolved as designed (best-effort, positive-match-only)**: `Alarms/GatewayAlarmMonitor.cs:483-489` (`IsDuplicateOfCachedState`, timestamp+state marker), `:496-507` (`IsDuplicateOfReconcileClear`, tombstone consumed on match), `:645-664` (tombstones recorded per reconcile generation, cleared at `:649`); at-least-once contract restated at `:616-625`; race-driving regressions in `Tests/Alarms/GatewayAlarmMonitorAttachOrderTests.cs:159`, `:218`. The one-generation tombstone lifetime and the unset-timestamp passthrough match the recorded design; consumers must (and are documented to) treat transitions idempotently.
|
||
- NEXT-08 / NEXT-10 (GLAuth TLS posture, glauth.md table) are security/docs-domain rows, not promoted here.
|
||
|
||
### Also verified in passing (other-domain fixes landing in gateway-core files)
|
||
|
||
- **WRK-21/WRK-28/IPC-23 gateway half** — `Grpc/MxAccessGrpcRequestValidator.cs:74-88` rejects `DrainEvents.max_events` above the now-shared `GatewayContractInfo.MaxDrainEventsPerCommand` constant, with the byte-cap division of labor documented in the comment.
|
||
- **Zero-quota pipe fix** (`da84635`) — `SessionWorkerClientFactory.cs:15-25`, `:175-185`: explicit 128 KiB in/out buffers with the windev-wedge rationale; this was the root cause of the historical full-suite testhost hang.
|
||
- **Dead `ISessionManager.ReadEventsAsync` chain removed** (`fa9eb0c`) and `StreamEventsAsync` now consumes the subscriber channel directly (`Grpc/EventStreamService.cs:110-204`, `935f002`) — the WaitToReadAsync/TryRead loop preserves the fault-surface semantics (`:158-204`) including the worker-fault → `MarkFaulted` + metric path.
|
||
- **Reply ownership transfer** (`3ff073d`) — `Grpc/MxAccessGrpcMapper.cs:62-85`: single-consumer argument for moving `reply.Reply` is documented and holds (one frame → one `PendingCommand` awaiter → one mapper call; `MergeDeniedInto` splices in place on the owned instance).
|
||
- **GalaxyRepository adoption** — `GatewayApplication.cs:151-176`: scope provider registered *before* `AddZbGalaxyRepository` so the library's `TryAddSingleton` null default cannot win (order-sensitive, correctly commented); snapshot-path default seeded pre-bind with `GalaxyRepositoryOptionsValidator` + `ValidateOnStart` (SEC-33); `MapZbGalaxyRepository` at `:351`. `A2-galaxyrepository-adoption-handoff.md`'s caveats are all marked resolved and match the code. One hardening gap filed as **GWC-38**.
|
||
- **Exception-total shutdown drain** (`95f8ba9`/`0bc13b5`) — `Sessions/SessionManager.cs:354-427`: bounded-parallel (4) close with an exception-total body, kill fallback on `CancellationToken.None`, and the deliberate token asymmetry against `CloseExpiredLeasesAsync` documented at length; `ISessionManager.ShutdownAsync`'s doc (`ISessionManager.cs:100-110`) states the bounded overrun honestly. Sweep selection stays sequential with the TOCTOU re-check inside `TryBeginCloseIfExpired` (`GatewaySession.cs:1671-1695`) — the cycle-2 close-gate invariants are preserved under the parallelization.
|
||
|
||
## New findings
|
||
|
||
IDs continue from the register. **Note on ID allocation:** the codebase already contains comment references to "GWC-31" (and a re-used "GWC-30") coined by an off-register remediation pass — those labels are *not* register entries (see GWC-31 below, which claims the ID and records the collision). Register IDs assigned here are authoritative.
|
||
|
||
### GWC-31 — The 2026-08-15 perf remediation stamped unregistered "GWC-3x" IDs into code comments, colliding with this register
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Documentation & comments
|
||
- **Location:** `Workers/WorkerClient.cs:44` ("GWC-30" = pull-model queue-depth gauge), `:236` ("PERF(GWC-31)"), `:253` ("GWC-31: one pooled timer…"); `Metrics/GatewayMetrics.cs:46`, `:213`, `:284`, `:463`, `:524` ("GWC-30" = gauge pull model); `Tests/Gateway/Workers/WorkerClientTests.cs:163`, `:179` ("GWC-31" = WorkerCancel forwarding); analogous worker-side citations (`Worker/Ipc/WorkerFrameReader.cs`/`WorkerFrameWriter.cs`, per `docs/plans/2026-08-15-perf-review-remediation.md:544-548`)
|
||
- **Description:** The 2026-08-15 performance review (whose findings live only in `docs/plans/2026-08-15-*.md`, not in `archreview/`) labeled its changes with GWC-series IDs that were never added to any register. "GWC-30" now means two different things — the archreview register says *frame-reader 4-byte prefix allocation*; the code comments say *pull-model worker queue-depth gauge* — and "GWC-31" exists only in comments. Finding IDs are the repo's cross-reference currency ("cite it in commits, branches, and PRs" — remediation tracker); a reader following a code comment to the register lands on the wrong finding, and future review cycles (this one included) must dodge already-burned numbers. REVIEW-PROCESS.md's "never reused" rule is violated in spirit.
|
||
- **Recommendation:** Sweep the perf-pass citations to a distinct prefix (e.g. `PERF-nn` keyed to `docs/plans/2026-08-15-perf-review-remediation.md` task numbers) or append the perf findings to the register under fresh, unclaimed IDs and fix the comments to match. Add a line to REVIEW-PROCESS.md stating that GWC/WRK/IPC/SEC/CLI/TST prefixes are reserved for the archreview registers.
|
||
|
||
### GWC-32 — `WorkerClient.DisposeAsync` never enters a terminal state, so the GWC-24 staging-overflow fault can misfire during a dispose race and the "already terminal" shutdown-drop claim is not guaranteed
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Concurrency & thread safety (residual of GWC-24)
|
||
- **Location:** `Workers/WorkerClient.cs:419-434` (`DisposeAsync` sets `_disposed`, cancels, completes all three channels — but `_state` is untouched), `:661-676` (`StageWorkerEvent` fallback: `TryWrite` false → `if (IsTerminalState()) return;` → otherwise `ProtocolViolation` fault), `:632-644` (XML doc: a completed staging channel is "safely dropped because the client is already terminal")
|
||
- **Description:** `MarkClosed` and `SetFaulted` both flip `_state` to a terminal value *before* completing the channels, so the `IsTerminalState()` guard in `StageWorkerEvent` correctly classifies a completed-channel `TryWrite` miss as shutdown. `DisposeAsync` is the third channel-completing path and does not: a client disposed while still `Ready` keeps reporting `IsTerminalState() == false`. If the read loop is inside `DispatchEnvelope` when `DisposeAsync` completes `_eventStaging` (the `_stopCts.Cancel()` only interrupts the *next* `ReadAsync`), the in-flight `StageWorkerEvent` takes the overflow arm: a spurious `ProtocolViolation` session fault with the misleading "consumer is draining slower than the worker produces" message, a `worker-event-staging` overflow metric, a redundant `KillOwnedProcess`, and a `WorkerStopped("ProtocolViolation")` metric — all during an intentional teardown. Reachability is narrow: in the normal teardown order (`CloseAsync`/`Kill` → `DisposeAsync`) the state is already terminal; the exposed path is disposing a still-Ready client, which `SessionManager.OpenSessionAsync`'s failure arm does (`Sessions/SessionManager.cs:133-140` — `MarkFaulted` faults the *session*, not the worker client) and `SessionWorkerClientFactory`'s catch avoids only because it calls `Kill` first (`SessionWorkerClientFactory.cs:120-132`). Impact is corrupted diagnostics/metrics, not data loss — but the fault fires on exactly the code path an operator investigates after a failed open, pointing them at backpressure that never happened. It also makes the `StageWorkerEvent` XML doc's "already terminal" claim false on one of the three completion paths.
|
||
- **Recommendation:** Treat disposal as terminal in the guard — `if (_disposed || IsTerminalState()) return;` — or have `DisposeAsync` transition `_state` to `Closed` under `_syncRoot` before completing the channels (which would also give GWC-12's double-dispose interlock a natural home). Update the `StageWorkerEvent` doc to name all three completion paths.
|
||
|
||
### GWC-33 — A faulted-but-retained worker client keeps contributing its stale undelivered-event depth to the pull-model gauge until session disposal
|
||
|
||
- **Severity:** Info
|
||
- **Category:** Performance & resource management (observability)
|
||
- **Location:** `Workers/WorkerClient.cs:120-121` (gauge source registered at construction), `:426-429` (unregistered only in `DisposeAsync`), `:1011-1039` (`SetFaulted` completes channels but leaves staged+queued events uncounted-down); retention window: `Configuration/SessionOptions.cs` `FaultedGraceSeconds` + `Sessions/SessionManager.cs:283-289` (faulted sessions swept only when reapable)
|
||
- **Description:** The GWC-24 gauge counts staged + queued events and is decremented only on consumer read. When a client faults, its channels are completed with the fault and never drained, so `_eventQueueDepth` freezes at whatever was undelivered. The registration is removed only in `DisposeAsync`; with a positive `FaultedGraceSeconds` (deliberately configurable so operators can observe faults before the reap) the dead client's frozen depth inflates the aggregate `worker event queue depth` gauge for the whole grace window. With the default `FaultedGraceSeconds = 0` the window is one sweep interval, so the default posture is fine — recorded so the next metrics pass knows it was seen.
|
||
- **Recommendation:** Optional: dispose the gauge registration inside `SetFaulted`/`MarkClosed` (the comment at `:427-428` already articulates the principle: "the sum must not keep counting a client that is going away").
|
||
|
||
### GWC-34 — `EnsureDistributorCreated` recreates a never-started distributor after `DisposeAsync`; an attach that races a session teardown registers on a pump-less distributor and hangs silently
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Concurrency & thread safety (residual of GWC-27 — the disposal-side twin)
|
||
- **Location:** `Sessions/GatewaySession.cs:1865-1875` (`DisposeAsync` nulls `_eventDistributor`; `_eventDistributorStarted` is never reset — `:41`, `:554-559`), `:535-563` (`EnsureDistributorCreated`: `_eventDistributor is null` → constructs a fresh distributor; `startNow` is false because the started flag is already latched), `:946-998` (`AttachEventSubscriber`: readiness gate under `_syncRoot`, then the distributor calls run *outside* the lock)
|
||
- **Description:** GWC-27 closed the premature-attach poison; the mirror-image hole remains open on the teardown side. An `AttachEventSubscriber` (or `AttachEventSubscriberWithReplay` / `AttachInternalEventSubscriber`) that passes its readiness gate while the session is still `Ready`, then loses the CPU until `CloseAsync` + `DisposeAsync` complete, reaches `EnsureDistributorCreated` after `_eventDistributor` was nulled: a **new** distributor is constructed, `startNow` comes back `false` (the latch is one-way), the subscriber registers on a distributor whose pump will never start and whose channel will never be completed — `WaitToReadAsync` in `EventStreamService.cs:167` then blocks until the client's own deadline/cancel. No error, no log, no fault; the replacement distributor is also never disposed. Contrast the adjacent race that *is* handled: if disposal instead wins between `EnsureDistributorCreated` returning the **old** distributor and `Register`, the old distributor's disposed check throws `ObjectDisposedException` loudly and the attach rolls back (`RegisterSubscriber`, `SessionEventDistributor.cs:361-386`). Reachability is small (the attach must stall across an entire multi-second close + dispose between two adjacent statements), but the failure mode is the worst kind — silent, per-stream, and invisible to metrics (the backlog registration reports 0 forever). This is the same "one distributor per session, never reset" lifecycle assumption GWC-27 documented, violated from the other end.
|
||
- **Recommendation:** Make the latch symmetric with the null-out: in `DisposeAsync`, set a `_eventDistributorDisposed` flag (or reuse a session-terminal check) under `_syncRoot` when nulling `_eventDistributor`, and have `EnsureDistributorCreated` throw `SessionManagerException(SessionNotReady)` instead of re-creating when it is set. One flag, one throw — the attach paths' existing catch/rollback already handles the exception.
|
||
|
||
### GWC-35 — `ApplyReconcile`'s state-delta repair is one-directional: an acked alarm replaced by a new unacked instance (or any acked→active regression) is absorbed silently by the snapshot replace
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Correctness & logic (alarm feed completeness; residual of GWC-26)
|
||
- **Location:** `Alarms/GatewayAlarmMonitor.cs:666-691` (`ApplyReconcile` both-present loop: broadcasts only when `incoming.Value.CurrentState == AlarmConditionState.ActiveAcked`), snapshot replace at `:687-691`
|
||
- **Description:** The GWC-26 repair covers three legs — missing→present (`Raise`), present→missing (`Clear`), and unacked→acked (`Acknowledge`). The fourth leg is absent: an alarm the cache holds as `ActiveAcked` that the worker snapshot now reports as `Active` (the acked instance cleared and re-raised as a new unacked instance while the live `Clear`+`Raise` transitions were lost — e.g. the internal alarm lease overflowed and disconnected, or worker-side event loss) is absorbed by the `_alarms` replace with no feed transition. `CurrentAlarms`/`QueryActiveAlarms` are correct (they read the replaced cache), but live `StreamAlarms`/dashboard subscribers keep rendering the alarm as acknowledged until its next genuine transition — for a re-raised alarm, precisely the state an operator must not misread. Reachability is much lower than the pre-GWC-26 window (a lease overflow restarts the whole monitor lifecycle, which re-snapshots every subscriber), so this is a completeness gap in the defense-in-depth rather than a live bug.
|
||
- **Recommendation:** Generalize the both-present delta to any `CurrentState` change: broadcast `Acknowledge` for `→ ActiveAcked` and `Raise` (with the incoming snapshot's new instance identity) for `→ Active`. The existing `TransitionFromSnapshot` already carries the timestamps the NEXT-03 dedup needs to suppress the live duplicates.
|
||
|
||
### GWC-36 — `EventSubscriberReplayAttachment.OldestAvailableSequence` doc still describes the pre-GWC-25 empty-ring semantics
|
||
|
||
- **Severity:** Info
|
||
- **Category:** Documentation & comments
|
||
- **Location:** `Sessions/EventSubscriberReplayAttachment.cs:26-30` ("The oldest worker sequence still retained and replayable; `0` when nothing is retained.")
|
||
- **Description:** GWC-25 changed the empty-ring-with-gap value to `highest observed + 1`; the distributor's out-param doc (`SessionEventDistributor.cs:430-440`), the proto, and `docs/Sessions.md` were all updated, but this record's XML doc was missed. "`0` when nothing is retained" is now exactly the value the fix exists to avoid — a reader implementing against this doc re-introduces the dead-stream formula.
|
||
- **Recommendation:** Mirror the distributor's out-param wording (retained-oldest, or `highest observed + 1` when the ring is empty; `0` only when `Gap` is false).
|
||
|
||
### GWC-37 — The alarm subtag watch-list is resolved once per monitor lifecycle: a Galaxy-Repository outage at startup silently empties the subtag failover's coverage for the session's entire lifetime
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Error handling & resilience (alarm subtag fallback)
|
||
- **Location:** `Alarms/GatewayAlarmMonitor.cs:330-357` (`SubscribeAlarmsAsync` resolves the watch list once and sends it in the `SubscribeAlarms` command); `Alarms/AlarmWatchListResolver.cs:54-83` (GR discovery failure → warn and continue with a config-only, typically empty set), `:141-145` (zero-target warning); no re-resolve anywhere — `ApplyProviderModeChangeAsync` (`GatewayAlarmMonitor.cs:513-566`) triggers only a reconcile on failover
|
||
- **Description:** The subtag fallback's coverage is fixed at `SubscribeAlarms` time: the resolved `AlarmSubtagTarget` list travels once to the worker and is never refreshed while the monitor session lives. Discovery failure is deliberately non-fatal (`ResolveAsync` catches, warns, and returns whatever config contributes) — reasonable for the resolver, but the monitor then subscribes with an empty watch list and runs indefinitely. In `Auto` mode the alarmmgr primary works fine, so nothing looks wrong until the moment a failover to subtag fires — at which point the degraded provider covers zero alarms, the feed goes quiet, and the only evidence is a single `LogWarning` emitted possibly weeks earlier at startup. The same staleness applies to Galaxy objects added after monitor start (a documented design property), but the outage-at-startup case differs in kind: the operator configured discovery, it silently produced nothing, and the safety net the feature exists to provide is empty. The monitor's supervisor restarts (and thus re-resolves) only when the session faults — a healthy alarmmgr session never does.
|
||
- **Recommendation:** Re-resolve and re-subscribe on the failover edge (`ApplyProviderModeChangeAsync` when `toMode == Subtag`), or treat "GR discovery failed AND the effective mode can need the watch list (Auto/ForceSubtag) AND zero targets resolved" as a lifecycle failure so the supervisor's backoff loop retries resolution instead of latching the empty set. At minimum, surface the empty-watch-list condition on the provider status (`Degraded`/`Reason`) rather than only in a startup log line.
|
||
|
||
### GWC-38 — `GatewayBrowseScopeProvider` fails open to the full Galaxy hierarchy when the ambient identity is absent, resting the browse-scoping control on an undeclared interceptor-ordering invariant
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Security (defense-in-depth; GalaxyRepository adoption)
|
||
- **Location:** `Security/Authorization/GatewayBrowseScopeProvider.cs:14-25` (`identityAccessor.Current?.EffectiveConstraints ?? ApiKeyConstraints.Empty` → empty `BrowseSubtrees` → library treats as "no scoping"); registration-order dependency documented at `GatewayApplication.cs:151-154`
|
||
- **Description:** The per-key browse-subtree scoping — the one piece of authorization mxaccessgw adds on top of the shared GalaxyRepository package — resolves to *unscoped* (full hierarchy) whenever `IGatewayRequestIdentityAccessor.Current` is null. The code's own comment says this is "safe only because the global interceptor has already authenticated and authorized the request first. Do not reorder the interceptor without revisiting this." That is a correct description of today's wiring and an honest warning, but it makes the control's fail state *open*: any future path that reaches the library's galaxy service without the interceptor populating the ambient identity (a new endpoint mapping, an interceptor scoped per-service instead of globally, a library upgrade changing pipeline order) silently grants every scoped key the full hierarchy, with nothing failing and nothing logged. Scoping is a constraint-narrowing control, so its absence is invisible in normal operation. (Whether identity-absent is legitimately reachable under `Authentication:Mode=Disabled` is the security reviewer's domain; this finding is about the fail-open shape itself.)
|
||
- **Recommendation:** Distinguish "authenticated with no BrowseSubtrees constraint" (legitimately unscoped) from "no ambient identity" (invariant broken): when `Current` is null *and* API-key authentication is enabled, throw `RpcException(PermissionDenied)` (or log-and-deny) instead of returning unscoped. One branch converts a silent authorization bypass into a loud misconfiguration signal.
|
||
|
||
## Severity roll-up (new findings)
|
||
|
||
| Severity | Count | IDs |
|
||
|----------|:-:|-----|
|
||
| Critical | 0 | — |
|
||
| High | 0 | — |
|
||
| Medium | 0 | — |
|
||
| Low | 6 | GWC-31, GWC-32, GWC-34, GWC-35, GWC-37, GWC-38 |
|
||
| Info | 2 | GWC-33, GWC-36 |
|
||
|
||
Prior-finding verification: 7/7 cycle-2 Done claims hold (3 with caveats filed as GWC-32/34/35); 13 still-open cycle-1 findings re-confirmed open with none regressed; GWC-16 recommended for closure as a recorded Won't-fix.
|
||
|
||
## Still good / preserve under change
|
||
|
||
The cycle-2 remediation wave and the subsequent perf pass landed without breaking the core invariants, and several of them got structurally *stronger*. The backpressure topology is now bounded end to end again — worker queue → pipe → bounded staging (2×) → bounded consumer channel → bounded per-subscriber channels — with a loud fail-fast fault at every seam and a single observable depth gauge across the first two gateway stages; the GWC-24 implementation even added the startup overflow guard the design didn't ask for. The replay→live handoff argument is intact after the GWC-25 change, and the empty-ring sentinel arithmetic is documented in four places that all agree (distributor, proto, docs/Sessions.md, e2e test). The GWC-28 write-loop stamping is the textbook fix, and the `InvokeAsync` size-pre-check comment proves the interaction with it was reasoned about rather than stumbled past. The alarm monitor's attach-before-subscribe plus the NEXT-03 positive-match dedup give the alarm feed a defensible, *stated* delivery contract (at-least-once, idempotent-state), and the reconcile repairs are correctly quarantined to the feed layer — the "never synthesize events" rule for `MxEvent`s is respected everywhere, with the ReplayGap sentinel still the sole, documented control signal. Session state remains single-lock disciplined through the new detach-grace/faulted-grace/close-gate arms; the parallelized sweep and shutdown preserved the TOCTOU re-check and made the shutdown body exception-total with an honestly documented token asymmetry. The GalaxyRepository adoption is clean consumer wiring: scope-provider-before-package registration order is commented, the snapshot path is validated at startup, and the host-side projector is a pure function memoizable on the cache sequence. Comment quality across the new code is exceptional — most of this cycle's findings were *found via* comments that honestly documented their own residual assumptions ("safe only because…", "already terminal") — preserve that habit; it is what keeps one-layer-out defects findable. `GatewaySession` has grown again (2,173 lines) and the long-standing split-out recommendation stands.
|