mxaccessgw/archreview/10-gateway-core.md

Gateway Server Core — Architecture Review

Scope & method

Static review of the Gateway server core in src/ZB.MOM.WW.MxGateway.Server, excluding Security/, Dashboard/, Metrics/, and Diagnostics/ (covered by another agent; DI wiring and metric call sites in scope were followed into those directories only to confirm behavior). Every file in Program.cs, GatewayApplication.cs, Configuration/, Sessions/, Workers/, Grpc/, and Alarms/ was read in full, including GatewaySession.cs (2,058 lines), WorkerClient.cs, SessionEventDistributor.cs, SessionManager.cs, MxAccessGatewayService.cs, EventStreamService.cs, GatewayAlarmMonitor.cs, the frame reader/writer, the process launcher stack, both hosted services, and the full options/validator set. Architecture context comes from CLAUDE.md, gateway.md, and docs/Sessions.md. No source file was modified and no build or test was run (macOS working tree).

Executive summary

• The session/worker lifecycle machinery is unusually well hardened: state writes are single-lock disciplined, close/kill paths are serialized through a per-session close gate, TOCTOU races between the lease sweeper and reconnecting subscribers are re-checked atomically, and the distributor's replay→live handoff is provably gap- and duplicate-free. The inline documentation of these invariants is exemplary.
• One serious correctness defect exists: the central alarm monitor consumes the worker event channel directly while the session's own event-distributor pump (started for the dashboard mirror on every production session) consumes the same channel concurrently. Events are split between the two consumers, so alarm transitions are randomly lost from the alarm feed; acknowledge transitions are never repaired by the reconcile pass.
• Faulted sessions are never reaped. MarkFaulted neither kills the worker nor schedules teardown, and the lease sweeper only checks lease/detach-grace expiry, so a faulted session can hold a session slot and a live x86 worker process for up to DefaultLeaseSeconds (30 minutes by default).
• The worker pipe is created without any ACL or PipeOptions.CurrentUserOnly, despite gateway.md promising a restricted-ACL, no-anonymous-access pipe. A local process can steal the single pipe instance and fail session startup at will.
• The worker read loop blocks on a full gateway event channel for up to 5 seconds per event, stalling command replies and heartbeat processing behind an event burst.
• gateway.md promises a gateway-configured maximum sparse-array length; the code only caps at Array.MaxLength, so one write request can force multi-hundred-megabyte allocations before the frame-size check rejects the result.
• The worker startup probe is a no-op whose failure exception is excluded from its own retry pipeline, making StartupProbeRetryAttempts/StartupProbeRetryDelayMilliseconds dead configuration.
• Hot-path efficiency is mostly sound (bounded channels, non-blocking fan-out, TryWrite overflow detection) but carries avoidable per-event costs: a Stopwatch allocation per streamed event, a full protobuf clone per mapped event, per-frame byte-array allocations with two stream writes, and a LinkedList-based replay ring.
• Convention adherence to docs/style-guides/CSharpStyleGuide.md is strong (file-scoped namespaces, sealed by default, Async suffix, MXAccess-aligned naming). The main deviations are a gRPC type (RpcException) thrown from the Sessions layer, a DisposeAsync on GatewaySession without declaring IAsyncDisposable, and one dead public method with a stale doc reference.

Findings

Stability

[Critical] The alarm monitor and the session's event-distributor pump both drain the same single worker event channel, so alarm events are randomly stolen from the alarm feed. Evidence: GatewayAlarmMonitor.RunMonitorAsync consumes worker events directly via _sessionManager.ReadEventsAsync(session.SessionId, ...) (src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs:228-231), which calls GatewaySession.ReadEventsAsync → WorkerClient.ReadEventsAsync (Sessions/GatewaySession.cs:1427-1440, Workers/WorkerClient.cs:227-236). But GatewaySession.MarkReady starts the dashboard mirror, which creates and starts the SessionEventDistributor pump (Sessions/GatewaySession.cs:433-437, 554-583), and that pump's source is the same GatewaySession.ReadEventsAsync (Sessions/GatewaySession.cs:701-710). IDashboardEventBroadcaster is registered unconditionally (Dashboard/DashboardServiceCollectionExtensions.cs:50) and injected into SessionManager (Sessions/SessionManager.cs:65, passed to every session at Sessions/SessionManager.cs:440), so in every production deployment the pump starts at session-ready — including on the alarm monitor's own session. 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. docs/Sessions.md:196 states the design intent explicitly: single-subscriber mode exists to prevent "two gRPC streams from racing on the same worker event channel" — the alarm monitor recreates exactly that race internally. Failure scenario: roughly half of OnAlarmTransition events land in the dashboard mirror instead of ApplyTransition. Raise/Clear are eventually repaired by ReconcileLoopAsync (up to ReconcileIntervalSeconds, floor 5 s, default 30 s late), but ApplyReconcile broadcasts only presence deltas (Alarms/GatewayAlarmMonitor.cs:511-550), so a stolen Acknowledge transition is never delivered to StreamAlarms subscribers at all — clients show unacked alarms indefinitely. Provider-mode-change events can also be stolen, delaying degraded-state visibility. Recommendation: make the alarm monitor a distributor subscriber (attach via the session's lease API, or an internal Register on the distributor) instead of calling ReadEventsAsync directly; alternatively assert single consumption of WorkerClient.ReadEventsAsync (SingleReader = true plus a claimed-once guard) so this class of bug fails loudly instead of silently splitting events.

[High] Faulted sessions are never swept, leaving a live worker process and a consumed session slot for up to the full lease duration. Evidence: 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. The sweeper closes only lease-expired or detach-grace-expired sessions (Sessions/SessionManager.cs:273-277); there is no State == Faulted branch. Detach-grace deliberately does not stamp faulted sessions (Sessions/GatewaySession.cs:2022-2028), so the only path out is lease expiry at DefaultLeaseSeconds = 1800 s (Configuration/SessionOptions.cs:21). In the FailFast overflow case (Sessions/GatewaySession.cs:690-694) the worker is healthy and keeps pumping events into the distributor while the session is permanently unusable (EvaluateReadyUnderLock fails every command, Sessions/GatewaySession.cs:1930-1948). Failure scenario: a burst-slow client overflows its queue in single-subscriber FailFast mode; the session faults; for the next 30 minutes the gateway holds one of MaxSessions (default 64) slots and a running x86 MXAccess worker with live COM subscriptions that no client can use or close (clients rarely call CloseSession on a faulted session). A handful of such faults can exhaust session capacity. Recommendation: sweep Faulted sessions in CloseExpiredLeasesAsync (immediately or after a short grace), or have MarkFaulted schedule teardown (kill worker + registry removal) the way SetFaulted does on the worker-client side.

[Medium] A full gateway event channel stalls the worker read loop — command replies and heartbeats queue behind the 5-second full-mode wait. Evidence: 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). Failure scenario: 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 sitting behind the 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 heartbeat watchdog interplay the code specifically tries to compensate for (Workers/WorkerClient.cs:394-424). Recommendation: dispatch command replies/heartbeats before (or independently of) event enqueue — e.g. fault-or-drop on event backlog without blocking the loop, or route events through a dedicated writer task so replies never queue behind events.

[Medium] The worker named pipe is created with no ACL and without CurrentUserOnly, contradicting the documented pipe-security model. Evidence: SessionWorkerClientFactory.CreatePipe uses the plain NamedPipeServerStream constructor with PipeOptions.Asynchronous only (Sessions/SessionWorkerClientFactory.cs:158-166). gateway.md:279-284 requires "ACL restricted to the gateway identity and the launched worker identity, no anonymous access". Failure scenario: any local process can connect to mxaccess-gateway-{pid}-{sessionId} before the real worker does; with maxNumberOfServerInstances: 1 the legitimate worker can then never connect, so OpenSession fails on startup timeout — a trivially repeatable local denial of service. The nonce prevents impersonation (Workers/WorkerClient.cs:632-637) but not connection stealing. Recommendation: create the pipe via NamedPipeServerStreamAcl.Create with a DACL limited to the service identity (or PipeOptions.CurrentUserOnly, since workers run as the gateway identity per docs/DesignDecisions.md).

[Low] GatewaySession._workerClient is written under _syncRoot but read without it on several paths. Evidence: written in AttachWorkerClient under lock (Sessions/GatewaySession.cs:368-376); read lock-free in CloseAsync (Sessions/GatewaySession.cs:1470), DisposeAsync (Sessions/GatewaySession.cs:1762), KillWorker (Sessions/GatewaySession.cs:1617), and WorkerProcessId (Sessions/GatewaySession.cs: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. Recommendation: read _workerClient under _syncRoot (or mark it volatile) for consistency with the class's own locking contract.

[Low] WorkerClient.DisposeAsync is not safe against concurrent double-dispose. Evidence: 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() would throw ObjectDisposedException. Impact: latent only — SessionManager.RemoveSessionAsync's registry TryRemove gate makes the session's DisposeAsync single-shot in practice. Recommendation: use Interlocked.Exchange as the lease classes already do.

[Low] Worker-ready wait is a 25 ms poll loop rather than a state-change signal. Evidence: GetReadyWorkerClientAsync polls with Task.Delay(25ms) up to WorkerReadyWaitTimeoutMs (Sessions/GatewaySession.cs:1841-1909). Default-off (Configuration/SessionOptions.cs:69), bounded, and testable via TimeProvider, so the impact is minor; it is still a poll on the command hot path when enabled. Recommendation: if the option sees real use, replace with a TaskCompletionSource pulsed on worker state transitions.

Performance

[Medium] A Stopwatch instance is allocated for every streamed event. Evidence: MxAccessGatewayService.StreamEvents runs Stopwatch stopwatch = Stopwatch.StartNew() inside the per-event loop (Grpc/MxAccessGatewayService.cs:155-157). Impact: one heap allocation per event per subscriber on the highest-volume path in the gateway. Recommendation: use long ts = Stopwatch.GetTimestamp() + Stopwatch.GetElapsedTime(ts) (allocation-free).

[Medium] Every mapped event is deep-cloned. Evidence: MxAccessGrpcMapper.MapEvent returns workerEvent.Event?.Clone() (Grpc/MxAccessGrpcMapper.cs:64-73), invoked once per event by the distributor pump (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. Recommendation: transfer ownership of workerEvent.Event instead of cloning; keep the clone only if a second consumer of the WorkerEvent is ever introduced.

[Medium] Pipe framing allocates fresh buffers per frame and issues two stream writes per envelope. Evidence: reader allocates new byte[4] and new byte[payloadLength] per frame (Workers/WorkerFrameReader.cs:32, 50); writer allocates new byte[4] 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. Recommendation: rent from ArrayPool<byte>, serialize length + payload into one buffer, and write once; on the read side reuse a pooled buffer sized to the frame.

[Low] The replay ring is a LinkedList with a node allocation per retained event. Evidence: Sessions/SessionEventDistributor.cs:108, appended per event under _replayLock (Sessions/SessionEventDistributor.cs:766-793). Impact: node allocation + poor cache locality on the fan-out hot path; capacity is fixed (ReplayBufferCapacity, default 1024), which is exactly the shape a circular array buffer serves allocation-free. Recommendation: replace with a fixed-size ring array; keep the _replayLock protocol unchanged.

[Low] Per-event gauge reconciliation reads ChannelReader.Count on every event. Evidence: 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. Recommendation: sample the backlog on an interval (or every N events) rather than per event.

[Low] Invoke resolves the session twice per command. Evidence: ResolveSession(request.SessionId) (Grpc/MxAccessGatewayService.cs:104) followed by sessionManager.InvokeAsync(request.SessionId, ...) → GetRequiredSession (Sessions/SessionManager.cs:161). Impact: a redundant ConcurrentDictionary lookup per command — small, but on every command. Recommendation: add an ISessionManager.InvokeAsync(GatewaySession, ...) overload or pass the resolved session through.

Conventions

[Low] The Sessions layer throws Grpc.Core.RpcException, leaking a gRPC concern below the service boundary. Evidence: SparseArrayExpander.Invalid creates RpcException(Status(StatusCode.InvalidArgument, ...)) (Sessions/SparseArrayExpander.cs:283-284) and is 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 exception type. gateway.md:1063-1065 calls for translation code to live at the gRPC layer. Recommendation: throw a domain exception (e.g. SessionManagerException with an InvalidArgument-mapping error code) and map it in MxAccessGatewayService.MapException.

[Low] GatewaySession implements DisposeAsync without declaring IAsyncDisposable. Evidence: public sealed class GatewaySession with no interface list (Sessions/GatewaySession.cs:13) but a public DisposeAsync (Sessions/GatewaySession.cs:1672). Impact: await using does not compile against the type; disposal is only discoverable by convention. Recommendation: declare IAsyncDisposable.

[Low] Dead method with stale documentation: GatewaySession.KillWorker(string) has no callers. Evidence: no .KillWorker( call sites exist in server or test sources (grep across src/); the gated variant KillWorkerWithCloseGateAsync (Sessions/GatewaySession.cs:1637-1658) is what SessionManager.KillWorkerAsync uses (Sessions/SessionManager.cs:225). docs/Sessions.md:57 still states that KillWorkerAsync "calls GatewaySession.KillWorker directly". Recommendation: delete KillWorker (Sessions/GatewaySession.cs:1615-1619) and correct docs/Sessions.md in the same change, per the repo's docs-with-source rule.

[Low] Heartbeat configuration semantics are conflated between the worker's send interval and the gateway's check interval. Evidence: WorkerOptions.HeartbeatIntervalSeconds is documented as "the interval in seconds for worker heartbeats" (Configuration/WorkerOptions.cs:31) but is bound to the gateway-side HeartbeatCheckInterval (Sessions/SessionWorkerClientFactory.cs:86), whose own default is 1 s (Workers/WorkerClientOptions.cs:10). Production therefore checks every 5 s while unit-constructed clients check every 1 s, and the option name does not describe what it controls. Recommendation: either rename the option or add a distinct HeartbeatCheckIntervalSeconds; also note HeartbeatLoopAsync uses raw Task.Delay without the injected TimeProvider (Workers/WorkerClient.cs:400), unlike the rest of the class.

Positive: the reviewed code otherwise adheres closely to CSharpStyleGuide.md — file-scoped namespaces throughout, sealed classes by default, Async suffixes, MXAccess-aligned naming (ServerHandle, ItemHandle, MxStatusProxy shapes), and no hand-edited generated code. The GatewaySession class is, however, 2,058 lines mixing lifecycle, distributor wiring, bulk-command wrappers, and item tracking; splitting the bulk-command facade out would restore single-responsibility without behavior change.

Underdeveloped

[High] The documented "gateway-configured maximum array length" bound on sparse-array writes is not implemented. Evidence: gateway.md:536 lists "total_length exceeds the gateway-configured maximum array length" as an InvalidArgument rejection; SparseArrayExpander.Expand validates only totalLength > (uint)Array.MaxLength (Sessions/SparseArrayExpander.cs:65-69) — about 2.1 billion elements. No configuration option for the bound exists in Configuration/. Failure scenario: a single authorized Write carrying total_length = 500_000_000 forces the gateway to materialize a ~2-4 GB MxArray (Sessions/SparseArrayExpander.cs:98-232) before the 16 MB MaxMessageBytes frame check finally rejects it in WorkerFrameWriter (Workers/WorkerFrameWriter.cs:49-54) — a memory-exhaustion vector reachable through normal command flow. Recommendation: add the documented configurable cap (validated in GatewayOptionsValidator) and enforce it before allocation.

[Medium] The worker startup probe is a no-op and its retry pipeline can never retry, making the probe configuration dead. Evidence: WorkerProcessStartedProbe.WaitUntilReadyAsync performs 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 all cases, and StartupProbeRetryAttempts / StartupProbeRetryDelayMilliseconds (Configuration/WorkerOptions.cs:19-22, validated at Configuration/GatewayOptionsValidator.cs:119-126) have no observable effect. Recommendation: either implement a real readiness probe (retryable transient failures) or delete the pipeline and the two dead options; docs/WorkerProcessLauncher.md should be updated in the same change.

[Medium] The envelope sequence monotonicity rule is specified but never enforced. Evidence: gateway.md:313 states "sequence is monotonic per sender"; 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; event ordering guarantees ("keep event order stable per worker") rest solely on the worker's writer discipline. Recommendation: track last-received sequence per connection and fault the worker client on regression (ProtocolViolation), which matches the existing fault model.

[Low] EventChannelFullModeTimeout and HeartbeatStuckCeiling are not reachable from configuration. Evidence: SessionWorkerClientFactory populates only four of the six WorkerClientOptions fields (Sessions/SessionWorkerClientFactory.cs:83-89); the other two always use hardcoded defaults (Workers/WorkerClientOptions.cs:13, 23) and have no WorkerOptions counterparts. Recommendation: expose them under MxGateway:Worker:* or document them as fixed.

[Low] metrics.StreamDisconnected is always labeled "Detached", even when the stream ends by overflow or worker fault. Evidence: single call site in the finally block (Grpc/EventStreamService.cs:195); the fault paths above it (Grpc/EventStreamService.cs: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. Recommendation: record the actual terminal cause (detached / overflow / worker-fault / canceled).

[Info] MaxEventSubscribersPerSession is a knowingly dead knob in the default single-subscriber configuration — this is acknowledged and justified in a comment (Configuration/GatewayOptionsValidator.cs:189-196) and needs no change; noted here so it is not re-reported.

Top 5 recommendations

1. Fix the alarm monitor's event consumption (Alarms/GatewayAlarmMonitor.cs:228): attach through the session's SessionEventDistributor instead of a second raw ReadEventsAsync drain, and add a claimed-once guard on WorkerClient.ReadEventsAsync so any future dual-consumer bug fails loudly. This is the only Critical finding and silently corrupts the production alarm feed today.
2. Reap faulted sessions promptly (Sessions/SessionManager.cs:255, Sessions/GatewaySession.cs:716): sweep Faulted state in CloseExpiredLeasesAsync or trigger teardown from MarkFaulted, so a faulted session does not pin a worker process and a MaxSessions slot for 30 minutes.
3. Apply the documented ACL to the worker pipe (Sessions/SessionWorkerClientFactory.cs:158): use NamedPipeServerStreamAcl.Create (or CurrentUserOnly) to close the local pipe-squatting denial of service and match gateway.md's pipe-security contract.
4. Enforce the configured sparse-array length bound before allocation (Sessions/SparseArrayExpander.cs:65): implement the gateway.md-promised maximum, validated at startup, to remove the memory-exhaustion vector.
5. Decouple event enqueue from the worker read loop and trim per-event costs (Workers/WorkerClient.cs:511, Grpc/MxAccessGatewayService.cs:155, Grpc/MxAccessGrpcMapper.cs:68, Workers/WorkerFrameReader.cs:50): stop command replies and heartbeats from queueing behind a full event channel, then remove the per-event Stopwatch allocation, the per-event protobuf clone, and the per-frame buffer allocations.