mxaccessgw/archreview/30-contracts-ipc.md

Contracts & IPC Protocol — Architecture Review

Scope & method

This review covers the contracts project (src/ZB.MOM.WW.MxGateway.Contracts: mxaccess_gateway.proto, mxaccess_worker.proto, galaxy_repository.proto, GatewayContractInfo.cs, Generated/), the named-pipe frame protocol on both sides (src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerFrame* + WorkerClient.cs; src/ZB.MOM.WW.MxGateway.Worker/Ipc/*), and the proto→client codegen pipeline (clients/proto/proto-inputs.json, scripts/publish-client-proto-inputs.ps1, per-client generation scripts and gradle/tonic/Grpc.Tools configuration). Method: static reading of source, protos, generated output, scripts, and the related docs (gateway.md, docs/Contracts.md, docs/WorkerFrameProtocol.md, docs/Grpc.md, docs/ClientProtoGeneration.md, docs/style-guides/ProtobufStyleGuide.md), plus git history and a byte-level diff of the Galaxy protos. No builds were run (macOS tree); no source files were modified.

Executive summary

• The frame protocol core is sound on both sides: length validation before allocation, configurable 16 MiB cap, exact-read loops for partial reads, typed error codes for malformed/oversized/EOF frames, nonce validation in both directions, and disciplined late-reply handling. The disposable-worker fault model makes "desync = kill session" an acceptable recovery strategy.
• The single worst concrete defect is in the codegen pipeline: the checked-in client descriptor set (clients/proto/descriptors/mxaccessgw-client-v1.protoset) is seven weeks stale — it predates MxSparseArray, ReplayGap, and the alarm provider-status surface — and nothing (no CI, no test) runs the existing -Check mode.
• The pipe frame size limit is hard-coded to 16 MiB in the worker while the gateway's is configurable to 256 MiB; the value is never conveyed across the handshake, so raising the gateway config silently produces frames the worker rejects as protocol faults.
• MaxGrpcMessageBytes and pipe MaxMessageBytes share the same 16 MiB default with zero headroom for envelope overhead, and a write-side oversized frame faults the whole session instead of failing the single command.
• WorkerCancel is a dead contract arm: the worker dispatches it, the gateway never sends it. The cancellation behavior documented in gateway.md is half-implemented.
• The worker-side frame writer/reader received single-buffer and pooled-buffer optimizations that were never back-ported to the gateway side; the gateway also deep-clones every command twice and every event once on the hot path.
• Proto style discipline is good: versioned packages, UNSPECIFIED zero enums, reserved on retired fields, wire-compat policy headers, credential-field comments. Generated code is in sync with the protos in all five clients and in Contracts/Generated/; galaxy_repository.proto is wire-identical to the GalaxyRepository package copy (only csharp_namespace differs).
• Documentation drift exists at the contract boundary: docs/Grpc.md says six RPCs (there are seven), gateway.md's WorkerEnvelope sketch has the wrong correlation_id type and field numbers, and two docs name the wrong Python generated-output directory.
• The known codegen fragilities (Python grpcio-tools pin, Java tracked-generated churn, Generated/ commit-after-regen for net48) are guarded by nothing in the repo — no version pins in scripts, no check tasks, no written rule in docs/Contracts.md.

Findings

Stability

S1 — Medium — The worker's max frame size is hard-coded and cannot follow the gateway's configured limit. Evidence: src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameProtocolOptions.cs:15-21 (the WorkerOptions ctor always passes DefaultMaxMessageBytes = 16 MiB); src/ZB.MOM.WW.MxGateway.Server/Configuration/WorkerOptions.cs:37 and GatewayOptionsValidator.cs:150-153 (gateway-side MxGateway:Worker:MaxMessageBytes is configurable from 1 KiB to 256 MiB) wired at src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionWorkerClientFactory.cs:76. Neither GatewayHello (mxaccess_worker.proto:41-45) nor the worker launch arguments carry the value. Failure scenario: an operator raises the gateway limit above 16 MiB for large-array workloads; the gateway now emits frames the worker's reader rejects (Worker/Ipc/WorkerFrameReader.cs:44-49) with MessageTooLarge, faulting the session, and the worker still cannot emit anything above 16 MiB in the other direction. The configuration appears to work until the first large frame. Recommendation: carry the negotiated max frame size in GatewayHello/WorkerHello (additive fields) or a launch argument, and fail the handshake on disagreement instead of failing mid-traffic.

S2 — Medium — Public gRPC max message size equals the pipe max frame size with zero headroom, and an oversized outbound frame faults the whole session. Evidence: src/ZB.MOM.WW.MxGateway.Server/Configuration/ProtocolOptions.cs:16 (MaxGrpcMessageBytes default 16 MiB) and WorkerOptions.cs:37 (pipe max 16 MiB); a gRPC-accepted Invoke payload near the limit gains WorkerCommand + WorkerEnvelope overhead (session id, sequence, correlation id, enqueue timestamp, oneof tags) before hitting Server/Workers/WorkerFrameWriter.cs:49-54, whose WorkerFrameProtocolException escapes the write loop at Server/Workers/WorkerClient.cs:344-350 and calls SetFaulted, which kills the worker process (WorkerClient.cs:722-749). Failure scenario: one legitimate, gateway-accepted oversized write tears down the session, all its subscriptions, and its event stream, instead of failing that single command. Recommendation: enforce a public payload cap with explicit headroom below the pipe max, or catch MessageTooLarge at the enqueue/write boundary and fail only the offending correlation id.

S3 — Medium — DrainEvents with max_events = 0 packs the entire event queue into one reply envelope. Evidence: src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs:172-192 (0 = drain all); the reply is a single DrainEventsReply (mxaccess_gateway.proto:678-680) written as one frame in Worker/Ipc/WorkerPipeSession.cs:522-542 and 470-481. MxAccessGrpcRequestValidator imposes no cap on DrainEventsCommand.max_events (rules table, docs/Grpc.md:164-176). Failure scenario: a deep queue of large array events produces a reply exceeding MaxMessageBytes; the control-reply write throws, the exception propagates out of the message loop, and the worker exits — a diagnostics command kills the session. Recommendation: cap the effective drain count worker-side, or chunk the reply across multiple envelopes.

S4 — Low — The envelope sequence field is write-only; monotonicity is never validated on receive. Evidence: sequences are assigned at Server/Workers/WorkerClient.cs:930-936 and Worker/Ipc/WorkerPipeSession.cs:1005-1018, but neither validator checks them (Server/Workers/WorkerEnvelopeValidator.cs:15-39, Worker/Ipc/WorkerEnvelopeValidator.cs:12-36). gateway.md:312-314 states "sequence is monotonic per sender" as a protocol rule. Impact: gap/duplication detection promised by the design is diagnostics-only; ordering integrity rests entirely on pipe FIFO semantics (which is fine for a local pipe, but the rule is unenforced). Recommendation: either validate monotonicity in the envelope validators (cheap) or annotate the proto/doc that sequence is diagnostic only.

S5 — Low — No protocol version negotiation exists despite a field name that implies it. Evidence: GatewayHello.supported_protocol_version (mxaccess_worker.proto:42) is a single value compared for strict equality (Worker/Ipc/WorkerPipeSession.cs:215-219); the worker additionally hard-pins its own version to GatewayContractInfo.WorkerProtocolVersion at options construction (Worker/Ipc/WorkerFrameProtocolOptions.cs:65-70), and every envelope re-checks equality in both validators. Impact: acceptable for a lockstep-deployed pair (and gateway.md:319 documents mismatch-fails-session), but any future skewed upgrade of gateway and worker requires new machinery; the singular "supported" field cannot express a range. Recommendation: none required now; when the worker protocol first changes, add a min/max supported range rather than bumping the single constant.

S6 — Low — The gateway-side frame writer has no internal write lock; frame integrity rests on an undocumented single-writer invariant. Evidence: Server/Workers/WorkerFrameWriter.cs has no synchronization, while the worker's writer serializes writers with a SemaphoreSlim (Worker/Ipc/WorkerFrameWriter.cs:14,68-77) because its heartbeat, event-drain, and command tasks write concurrently. The gateway is safe only because all writes funnel through the single-reader outbound channel loop (Server/Workers/WorkerClient.cs:62-69,332-339). Impact: a future direct call to _writer.WriteAsync outside the write loop interleaves the two stream writes (prefix and payload are separate WriteAsync calls, WorkerFrameWriter.cs:59-60) and corrupts the stream unrecoverably. Recommendation: add the same write lock or an assertion, and document the invariant on the writer.

Verified sound (no finding): length-prefix validation happens before payload allocation on both sides (Server/Workers/WorkerFrameReader.cs:35-51, Worker/Ipc/WorkerFrameReader.cs:36-60); partial reads use exact-read loops with a typed EndOfStream error; malformed protobuf maps to InvalidEnvelope; late/unknown command replies are dropped with a debug log (WorkerClient.cs:565-571); the worker drops completed-command replies once the state leaves Ready with a diagnostic (WorkerPipeSession.cs:604-608,645-655); nonces are validated in both directions; pipe connect uses deadline-bounded exponential retry (Worker/Ipc/WorkerPipeClient.cs:161-213). Proto compatibility discipline is real: wire-compat policy headers on all three files, reserved ranges and names on the retired session_id fields (mxaccess_gateway.proto:912-916,927-930), UNSPECIFIED = 0 on every enum, and ProtocolStatusCode.OK = 1 distinct from the zero value.

Performance

P1 — Medium — Redundant deep copies on the command and event hot paths. Evidence: MxAccessGrpcMapper.MapCommand clones the inbound MxCommand (documented in docs/Grpc.md:197-211), then WorkerClient.CreateCommandEnvelope clones the entire WorkerCommand a second time (Server/Workers/WorkerClient.cs:896-903, command.Clone()); MapEvent deep-clones every worker event (Server/Grpc/MxAccessGrpcMapper.cs:64-73). Impact: each Invoke materializes the command graph three times (gRPC parse, mapper clone, worker-client clone) before pipe serialization; each event is parsed once and cloned once. For array-heavy OnDataChange streams this is measurable allocation pressure. Recommendation: drop the second clone in CreateCommandEnvelope (the mapper's clone already isolates the graph), and evaluate whether MapEvent can transfer ownership instead of cloning.

P2 — Low — The gateway frame writer serializes each envelope twice and issues two stream writes; the worker side already fixed this. Evidence: Server/Workers/WorkerFrameWriter.cs:41-60 calls CalculateSize() then ToByteArray() (which re-runs size calculation) and writes prefix and payload separately; the worker writer builds one prefixed buffer with a single WriteTo(Span) and one write (Worker/Ipc/WorkerFrameWriter.cs:58-72, with a comment explaining exactly this rationale). Impact: extra CPU pass and extra pipe write per frame on the higher-volume side of the connection (the gateway writes every command). Recommendation: back-port the worker's single-buffer implementation.

P3 — Low — The gateway frame reader allocates a fresh array per frame; the worker rents from ArrayPool. Evidence: Server/Workers/WorkerFrameReader.cs:50 (new byte[payloadLength]) versus Worker/Ipc/WorkerFrameReader.cs:51-77 (rent/return with a comment noting ParseFrom copies). Impact: large event frames (arrays near the 16 MiB cap) allocate LOH buffers per frame on the side that receives the entire event stream. Recommendation: mirror the pooled read.

P4 — Low — Worker event delivery is a 25 ms poll with one envelope write and flush per event. Evidence: Worker/Ipc/WorkerPipeSession.cs:17-19 (EventDrainInterval 25 ms, batch size 128) and 333-368 (per-event WriteAsync, each acquiring the writer lock and calling FlushAsync inside Worker/Ipc/WorkerFrameWriter.cs:71-72). Impact: an idle-to-active latency floor of up to 25 ms per batch, plus per-event flush syscalls under burst; gateway.md:849-850 lists worker→gateway event batching as a planned optimization that does not exist (there is no multi-event envelope body). Recommendation: acceptable for v1 parity; when event rate targets firm up, add a batched event body (additive oneof arm) rather than tightening the poll.

P5 — Info — Correlation id is carried twice per reply. Evidence: WorkerEnvelope.correlation_id (mxaccess_worker.proto:24) and MxCommandReply.correlation_id (mxaccess_gateway.proto:520); CompleteCommand must fall back from one to the other (Server/Workers/WorkerClient.cs:559-563). Impact: two sources of truth for the same value; harmless today, a divergence hazard for future writers. Recommendation: document which is authoritative (the envelope) in the proto comment.

Conventions

C1 — Medium — gateway.md's Worker Envelope section no longer matches the shipped contract. Evidence: gateway.md:291-309 shows uint64 correlation_id = 4 and body cases worker_hello = 10; gateway_hello = 11; command = 20; command_reply = 21; event = 22; heartbeat = 23; cancel = 24; shutdown = 25; fault = 26; the actual contract is string correlation_id = 4 and gateway_hello = 10; worker_hello = 11; worker_command = 13; worker_command_reply = 14; worker_cancel = 15; worker_shutdown = 16; worker_shutdown_ack = 17; worker_event = 18; worker_heartbeat = 19; worker_fault = 20 (mxaccess_worker.proto:20-39). WorkerShutdownAck is absent from the sketch entirely. Impact: field numbers and types in the top-level architecture doc are wrong; anyone implementing an alternate worker (the doc explicitly contemplates a C++ worker, gateway.md:1077-1099) from this section produces an incompatible peer. This violates the repo rule that docs change with the source. Recommendation: replace the sketch with the real message or a pointer to mxaccess_worker.proto.

C2 — Medium — docs/Grpc.md says the service has six RPCs; it has seven. Evidence: docs/Grpc.md:13 and :32 ("six in total") omit QueryActiveAlarms, which is declared at mxaccess_gateway.proto:37 and implemented at src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs:212. Impact: the authoritative gRPC-layer doc undercounts the public surface and documents no validation/handler behavior for the missing RPC. Recommendation: update the count, the collaborators table, and add a QueryActiveAlarms handler section.

C3 — Low — Two docs name the wrong Python generated-output directory. Evidence: docs/ClientProtoGeneration.md:81 and :144-146 (and CLAUDE.md's generated-code bullet) say clients/python/src/mxgateway/generated; the manifest (clients/proto/proto-inputs.json, generatedOutputs.python) and the tree use clients/python/src/zb_mom_ww_mxgateway/generated, which is also what clients/python/generate-proto.ps1 writes. Impact: stale path in the generation guide; a follow-the-doc regeneration writes to a dead directory. Recommendation: fix both docs to the manifest path.

C4 — Info (positive) — Generated-code hygiene and Galaxy wire-identity check out. Evidence: src/ZB.MOM.WW.MxGateway.Contracts/Generated/*.cs carry <auto-generated> headers and contain the newest contract symbols (MxSparseArray, 39 hits in MxaccessGateway.cs), so the committed output matches the protos; clients/go/internal/generated, clients/java/src/main/generated, and clients/python/.../generated all contain the sparse-array surface and the Java generated tree is clean in git (last regen committed 2026-06-18); Rust generates at build time via tonic-build with clients/rust/src/generated held as a documented .gitkeep placeholder (clients/rust/src/generated.rs header). diff of galaxy_repository.proto against the GalaxyRepository package source (/Users/dohertj2/Desktop/scadaproj/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Protos/galaxy_repository.proto) shows the only difference is csharp_namespace — wire-identical, as the retention comment in the contracts csproj requires (ZB.MOM.WW.MxGateway.Contracts.csproj:29-33).

C5 — Low — The Generated/-must-be-committed rule for net48 consumers is undocumented and unguarded. Evidence: ZB.MOM.WW.MxGateway.Contracts.csproj:26-35 (Compile Remove="Generated\**\*.cs" + Protobuf ... OutputDir="Generated") regenerates tracked files on every build; the worker consumes the contracts via ProjectReference (ZB.MOM.WW.MxGateway.Worker.csproj:22). docs/Contracts.md:97-98 says only "do not hand-edit"; the operational rule that a proto edit requires regenerating and committing Generated/ (or the net48 build fails on new types) appears nowhere in the docs, and no test compares Generated/ to the protos. Impact: same silent-drift class as the descriptor (U1) — the committed C# can lag the protos with nothing failing until a downstream consumer breaks. Recommendation: state the rule in docs/Contracts.md and add a freshness check (e.g., a contracts test that reflects over MxaccessGatewayReflection.Descriptor and compares against the .proto file descriptor).

Underdeveloped

U1 — High — The published client descriptor set is seven weeks stale and nothing enforces its freshness. Evidence: clients/proto/descriptors/mxaccessgw-client-v1.protoset was last committed 2026-04-30 (git log), while mxaccess_gateway.proto changed through 2026-06-18 (MxSparseArray), 2026-06-16 (ReplayGap), and 2026-06-15 (alarm provenance); strings over the protoset finds zero occurrences of MxSparseArray, replay_gap, or provider_status. docs/Contracts.md:127-131 mandates regenerating the descriptor after any proto change; scripts/publish-client-proto-inputs.ps1 provides a -Check mode, but the repo has no CI workflow directory at all and src/ZB.MOM.WW.MxGateway.Tests/Contracts/ClientProtoInputTests.cs validates only manifest versions and path existence — never descriptor content. Impact: the artifact documented as the "stable client input" (docs/ClientProtoGeneration.md:48-69) silently misrepresents the contract; any consumer that "prefers a descriptor input" (the doc's own words) generates against a schema missing three shipped features, and grpcurl users on the reflection-disabled deployments get a stale schema. Recommendation: regenerate and commit the descriptor now; add an automated freshness gate (test or CI step) and pin the protoc version it uses.

U2 — Medium — WorkerCancel is defined and handled but never sent. Evidence: mxaccess_worker.proto:71-73 defines it, Worker/Ipc/WorkerPipeSession.cs:399-401 dispatches it to _runtimeSession.CancelCommand, but a repo-wide grep of src/ZB.MOM.WW.MxGateway.Server finds zero references — WorkerClient.InvokeAsync handles timeout/cancel purely by abandoning the pending correlation (Server/Workers/WorkerClient.cs:195-213). Impact: the cancellation contract in gateway.md:713-719 ("the worker should finish the COM call and discard or log the late reply if the correlation was canceled") is half-implemented: the worker never learns a correlation was canceled, so it always writes the late reply (which the gateway then drops), and any worker-side discard logic behind CancelCommand is unreachable. Recommendation: send WorkerCancel from the gateway on timeout/caller-cancel, or mark the arm as reserved-for-future in the proto comment and delete the dead worker handling.

U3 — Medium — The known codegen fragilities have no in-repo guards. Evidence: (a) Python — clients/python/pyproject.toml:41 allows grpcio-tools>=1.80,<2 and clients/python/generate-proto.ps1 performs no version check, so regenerating with any newer 1.x stamps a GRPC_GENERATED_VERSION above the pinned runtime and breaks pytest (a known, previously-hit failure); (b) Java — clients/java/zb-mom-ww-mxgateway-client/build.gradle:50 points generatedFilesBaseDir at the tracked src/main/generated, so every gradle build rewrites ~64k lines of tracked output with protobuf-version churn and no check task detects spurious diffs; (c) portability — clients/go/generate-proto.ps1:8-9 and clients/python/generate-proto.ps1:7 hard-code C:\Users\dohertj2\... tool paths, and scripts/publish-client-proto-inputs.ps1 resolves only protoc.exe, making every generation step single-machine and Windows-only. Impact: contract evolution safety depends on operator memory rather than tooling; a regeneration on a different machine or tool version silently produces incompatible or noisy output. Recommendation: pin exact generator versions in the scripts (fail fast on mismatch), add a Java checkGeneratedClean-style task, and resolve tools from PATH with a documented version assertion instead of absolute user paths.

U4 — Low — The descriptor -Check compares raw bytes, which is protoc-version-sensitive. Evidence: scripts/publish-client-proto-inputs.ps1 (Compare-FileBytes) byte-compares a descriptor built with --include_source_info; source-info bytes differ across protoc releases even for identical schemas. Impact: once a check exists (U1), a protoc upgrade produces a false "stale" failure — or forces a churn commit — unless the protoc version is pinned. Recommendation: pin protoc for descriptor generation, or compare descriptors semantically (drop source info) instead of byte-wise.

U5 — Low — Contract surface promised in gateway.md but absent: the bidirectional Session RPC. Evidence: gateway.md:328-345 sketches rpc Session(stream ClientMessage) returns (stream ServerMessage) as the "best long-term shape" with a rollout plan whose step 3 is unimplemented; mxaccess_gateway.proto:17-38 has no such RPC. Impact: intentional phasing, not a defect — but the doc presents it inside the service definition block rather than as future work, compounding C1's staleness. Recommendation: mark it explicitly as not-yet-implemented in gateway.md.

U6 — Info — Public error detail model stops at status codes plus prose. Evidence: MxCommandReply preserves MXAccess parity detail well (hresult, statuses, diagnostic_message, mxaccess_gateway.proto:518-529), but transport-level failures surface only as gRPC status codes with message strings (docs/Grpc.md:217-241); no google.rpc error details are attached, and AcknowledgeAlarmReply.status is a permanently-unset placeholder documented as such (mxaccess_gateway.proto:940-945). Impact: machine consumers must parse prose to distinguish sub-causes within a status code; acceptable for the current client set. Recommendation: none now; consider google.rpc.ErrorInfo if third-party clients appear.

Top 5 recommendations

1. Regenerate and commit clients/proto/descriptors/mxaccessgw-client-v1.protoset, then add an automated freshness gate (a contracts test or CI step running the -Check equivalent with a pinned protoc) so the doc-mandated regeneration step cannot be skipped silently again (U1, U4, C5).
2. Make the pipe frame limit a negotiated value: carry it (or at least assert agreement) in the GatewayHello/WorkerHello exchange, keep MaxGrpcMessageBytes below the pipe max by an explicit headroom margin, and convert write-side MessageTooLarge from a session-killing fault into a per-command failure (S1, S2, S3).
3. Close the cancellation gap: either send WorkerCancel from WorkerClient on timeout/cancel so the worker's CancelCommand path is reachable, or reserve the arm and remove the dead handling, updating gateway.md either way (U2).
4. Back-port the worker's frame I/O optimizations to the gateway (single-buffer write, pooled read) and remove the redundant second Clone in WorkerClient.CreateCommandEnvelope (P1, P2, P3).
5. Fix the contract-boundary doc drift in one pass: gateway.md envelope sketch and unimplemented Session RPC, docs/Grpc.md RPC count and missing QueryActiveAlarms section, and the Python generated-directory path in docs/ClientProtoGeneration.md/CLAUDE.md; document the Generated/ regen-and-commit rule in docs/Contracts.md (C1, C2, C3, C5, U5).