Files
Joseph Doherty f4919ba97c
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Successful in 1m19s
ci / java (push) Successful in 2m25s
ci / portable (push) Successful in 8m21s
docs(archreview): cycle-3 architecture review (2026-08-19 @ 8b3e3b3)
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.
2026-08-20 09:10:12 -04:00

39 KiB
Raw Permalink Blame History

Contracts & IPC Protocol — Cycle-3 Review

  • Date: 2026-08-19
  • Commit: 8b3e3b3 (main, clean)
  • Baseline: 4f5371f (2026-07-12 cycle-2 review; ~288 commits since, ~55 of them touching the contracts, frame-protocol, codegen, or IPC-doc paths in scope)
  • Method: static review on macOS; every cited file read at HEAD. No build, no test, no source change. git diff 4f5371f..HEAD -- '*.proto' walked line by line; generated trees grep-checked for every new symbol; the GalaxyRepository package proto compared against the sibling scadaproj checkout.
  • Scope: src/ZB.MOM.WW.MxGateway.Contracts (three protos, Generated/ hygiene, net10.0;net48 multi-targeting, galaxy_repository.proto retention vs the ZB.MOM.WW.GalaxyRepository package), the frame protocol on both sides (Worker/Ipc/*, Server/Workers/WorkerFrame*, WorkerClient envelope/sequence/size handling, hello/handshake, max_frame_bytes negotiation, DrainEvents byte bound), the codegen/descriptor pipeline (scripts/check-codegen.ps1 Checks 1–4, scripts/publish-client-proto-inputs.ps1, clients/proto/*, vendored Rust protos, Go/Python/Java binding freshness guards, Gradle checkGeneratedClean), proto-evolution hygiene, and currency of docs/WorkerFrameProtocol.md, gateway.md (IPC envelope section), docs/Grpc.md, docs/ClientProtoGeneration.md, docs/Contracts.md. Worker-internal mechanics of the drain/write seam are owned by 20-worker.md and are cross-referenced, not re-reviewed.
  • Prior reports: archreview/2026-07-12/30-contracts-ipc.md (cycle 2); archreview/30-contracts-ipc.md (cycle 1); remediation status sources archreview/2026-07-12/remediation/00-tracking.md + 30-contracts-ipc.md, 90-candidate-findings-next-cycle.md; first-cycle backlog archreview/remediation/00-tracking.md.

Proto evolution since baseline — verified additive-only

git diff 4f5371f..HEAD -- src/ZB.MOM.WW.MxGateway.Contracts/Protos/ contains exactly four wire-affecting lines, all additive with next-free tags:

Change Location Tag check
bool snapshot_truncated = 2; on QueryActiveAlarmsReplyPayload mxaccess_gateway.proto:735 message previously ended at field 1
bool from_truncated_snapshot = 16; on ActiveAlarmSnapshot :951 previous highest was source_provider = 15
AlarmSnapshotStatus snapshot_status = 5; oneof arm on AlarmFeedMessage :1028 previous highest arm was provider_status = 4
new message AlarmSnapshotStatus { bool truncated = 1; } :1048-1054 new type

Everything else in the diff is comment-only (write-completion statuses contract at :538-551, DrainEvents truncation at :702-705, ReplayGap empty-ring sentinel at :797-801, AlarmFeedMessage open sequence at :1006-1012, and the GatewayHello.max_frame_bytes byte-cap sentence at mxaccess_worker.proto:50-52). No renumbering, no repurposing; reserved 1 / reserved "session_id" intact on the two retired fields (mxaccess_gateway.proto:964-965, :978-979); wire-compat policy headers present in all three files; gatewayProtocolVersion: 3 / workerProtocolVersion: 1 in clients/proto/proto-inputs.json:4-5 match GatewayContractInfo.cs:12,15 (additive changes correctly did not bump either). galaxy_repository.proto is unchanged and still wire-identical to the package source (see IPC-37).

Generated-tree freshness for the new symbols, grep-verified at HEAD: Contracts/Generated/MxaccessGateway.cs (AlarmSnapshotStatus ×16, from_truncated_snapshot ×2), Go mxaccess_gateway.pb.go (×15 / ×3), Python mxaccess_gateway_pb2.py (descriptor bytes carry \x66rom_truncated_snapshot, AlarmSnapshotStatus), Java MxaccessGateway.java (×106 / ×7), the committed protoset (×2 / ×2), and the Rust vendored copies cmp-identical for all three files. Protos, protoset, and every binding set were last touched in the same commit (c3c603f, 2026-08-18) — the regen-everything-together discipline held, including for a comment-only change.

Prior-finding verification

Cycle-2 findings (IPC-23..32)

Every tracked status was verified against code read at HEAD. All eight Done claims hold; one carries a residual filed as a new finding (IPC-23 → IPC-33).

ID Claimed Verified? Evidence (HEAD) Notes
IPC-23 Done (mechanics in WRK-21) ✅ holds, with caveat Contract half: GatewayHello.max_frame_bytes comment extended (mxaccess_worker.proto:50-52); DrainEventsReply truncation comment (mxaccess_gateway.proto:702-705); docs/WorkerFrameProtocol.md:43-51 states the byte-cap rule and drain-until-empty contract; gateway.md:565-572 documents both axes. Mechanics: byte-budgeted Drain(uint, int) leaves a non-fitting head queued and reports OversizedHeadSequence (Worker/MxAccess/MxAccessEventQueue.cs:389-429, per-event size memoized at enqueue :493-509); reply builder CreateDrainEventsReply consumes it with a truncation DiagnosticMessage (Worker/Ipc/WorkerPipeSession.cs:978-1020); budget = max(Max − 64 KiB, Max/2) (:1034-1039); shared count ceiling GatewayContractInfo.MaxDrainEventsPerCommand (Contracts/GatewayContractInfo.cs:33) consumed by both the gateway validator (Grpc/MxAccessGrpcRequestValidator.cs:81-86) and the worker clamp (:992-994); reply-too-large backstop answers InvalidRequest instead of dying (:847-864). R1–R3 satisfied. Caveat: R1 bounds the reply against the pipe frame max; the same MxCommandReply then crosses the public gRPC boundary whose send cap is MaxGrpcMessageBytes = pipe max − 64 KiB, and the budget's own 64 KiB headroom consumes exactly that margin → IPC-33. The tracker's rationale for leaving docs/Grpc.md without a DrainEvents row ("a worker IPC diagnostic, not a public RPC") is inaccurate — DrainEvents is a public MxCommandKind (mxaccess_gateway.proto:136) reachable through Invoke → IPC-38.
IPC-24 Done ✅ holds Churn-revert step deleted; java job runs gradle test then git diff --exit-code -- clients/java/src/main/generated (.gitea/workflows/ci.yml:135-142 (test step :135-137, diff gate :138-142), comment at :113-119 records the rationale); toolchain pinned grpcVersion 1.76.0 / protobufVersion 4.33.1 (clients/java/build.gradle:8,11); checkGeneratedClean revived for Gradle 9 via providers.exec evaluated lazily in doLast (clients/java/zb-mom-ww-mxgateway-client/build.gradle:71-92, df45cb4); docs updated (docs/ClientProtoGeneration.md:97,100-106). Java aggregate at HEAD carries every new symbol (see above). The lazy ProviderFactory.exec shape is correct: the provider is first realized in doLast, after the generateProto dependency ran, so the git probe sees the regenerated tree.
IPC-25 Done ✅ holds Go/Python bindings regenerated and fresh (grep above; max_frame_bytes now in mxaccess_worker.pb.go ×2 and mxaccess_worker_pb2.py ×1); Check 4 regenerates both via the per-client scripts and fails on any git status --porcelain diff, with tool-missing → fail not skip (scripts/check-codegen.ps1:109-129, header :19-24); Go script asserts protoc-gen-go v1.36.11 / protoc-gen-go-grpc 1.6.2 exactly and warns-only on protoc (clients/go/generate-proto.ps1:13-15,63-78), with the Windows .exe banner normalized (:22-28, c94c4d4); Python script asserts grpcio-tools 1.80.0 with PS5.1-safe quoting (clients/python/generate-proto.ps1:36-49, 8ae0c2f); CI installs the pinned generators before the check (ci.yml:63-72); pins table in docs/ClientProtoGeneration.md:90-98. Windows-side fixes landed after the cycle-2 design (Check 4 now runs on windev too). Backslash-joined paths in both scripts are normalized by pwsh on Unix (verified locally with Test-Path).
IPC-26 Done (mechanics in WRK-22) ✅ holds Contract requirement ("cancelled means never written") implemented by claim/tombstone interlock: TombstoneIfUnclaimed (Worker/Ipc/WorkerFrameWriter.cs:181,308-338), DequeueNext skips tombstones (:588), NEXT-04 fault observer on every abandoned frame (:350); documented with the claimed-mid-write residual at docs/WorkerFrameProtocol.md:202-224. Full mechanics verification in 20-worker.md (WRK-22, NEXT-04/05). Doc landed in the same batch as the code, as the plan required.
IPC-27 Done ✅ holds ClientProtoInputTests.Descriptor_ContainsEveryContractSymbol collects messages, fields, nested/top-level enums and values, services and methods from the protoset (Tests/Contracts/ClientProtoInputTests.cs:37-62,108-146) and walks all three reflection descriptors including GalaxyRepositoryReflection.Descriptor (:65-99); doc prose updated (docs/ClientProtoGeneration.md:76-82). Presence-only (no field numbers / types), per the recorded design; the protoc-pinned script check remains the byte-level gate.
IPC-28 Done ✅ holds docs/Grpc.md:285 lists CommandTooLarge → ResourceExhausted; Invoke-section paragraph at :85 explains the per-correlation failure and the headroom rule; matches Grpc/MxAccessGatewayService.cs:1056.
IPC-29 Done (discharged by WRK-26) ✅ holds docs/WorkerFrameProtocol.md:79-224 "Write Scheduling And Sequencing": two-class scheduler, peek-stamp-commit write-time stamping (:60-64, :98-107), per-frame vs stream failure (:109-120), flush coalescing and boundary flush (:122-172), unparking (:174-200), cancellation tombstones (:202-224); plus a new "Pipe Buffers" section (:226-235, da84635). Each claim cross-checked against Worker/Ipc/WorkerFrameWriter.cs anchors cited in 20-worker.md. The doc now slightly exceeds the code in one place: mxaccess_worker.proto:50-52 says "reply builders truncate to fit" immediately after listing events — events do not truncate (IPC-30 policy). Readable as intended; no action.
IPC-30 Done ✅ holds Batch-path MessageTooLarge is mapped to the first overshooting envelope (Worker/Ipc/WorkerPipeSession.cs:633-648, FindOversizedEvent :648-660), logged identity-only, written as a ProtocolViolation fault with CommandMethod = "EventDrain" (:713), then thrown; policy documented at docs/WorkerFrameProtocol.md:53-58. Gateway side needed no change: DispatchEnvelope already routes WorkerFault to SetFaulted(WorkerFaulted, …) (Server/Workers/WorkerClient.cs:614-618).
IPC-31 N/A (accepted divergence) Superseded — resolved by GWC-28 The gateway now stamps Sequence in WriteLoopAsync immediately before the write (Server/Workers/WorkerClient.cs:465-471), CreateEnvelope leaves it unset (:1229-1236); gateway.md:425-431 rewritten to "both sides stamp at the point of writing". Regression test WorkerClientTests.ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire. The two sides now implement the same semantics; flip the IPC-31 row from N/A to Resolved-by-GWC-28 in the tracker. See IPC-39 for the one interaction the change created with the IPC-03 pre-check.
IPC-32 Done ✅ holds Banners read Check 1/4 … Check 4/4 (scripts/check-codegen.ps1:40,52,78,110); header comment lists four checks (:4-24).

Done claims that do not fully hold: none outright. IPC-23 holds for the defect as filed but leaves the gRPC-boundary residual (IPC-33).

Cycle-1 findings still open at cycle 2 — re-checked at HEAD

ID Cycle-2 status HEAD status Evidence
IPC-08 Open (WorkerCancel defined, handled, never sent) Substantially resolved by 7c1ea12/a1a38b5 — recommend closing with a residual InvokeAsync forwards a WorkerCancel when the gateway's own command timeout fires and it owned the removal (Server/Workers/WorkerClient.cs:269-291); TrySendCancelForTimedOutCommand is structurally no-throw (:835-870, doc :806-833); worker routes it to CancelCommand (Worker/Ipc/WorkerPipeSession.cs:755-756 → MxAccess/MxAccessStaSession.cs:529-532 → Sta/StaCommandDispatcher.CancelQueuedCommand :151); test WorkerClientTests.InvokeAsync_WhenCommandTimesOut_SendsWorkerCancelForThatCorrelation (:185); docs/GatewayProcessDesign.md:596-602 documents it. Residual: the caller-cancellation path sends nothing → IPC-34.
IPC-10 Open (softened) Still open, unchanged Server/Workers/WorkerEnvelopeValidator.cs:15-39 and Worker/Ipc/WorkerEnvelopeValidator.cs check version/session/body only; mxaccess_worker.proto:23 uint64 sequence = 3; still has no comment. gateway.md:425-431 continues to state honestly that sequence is a diagnostic aid, and now that both sides stamp at write (GWC-28/WRK-04) the property an inbound check would rely on actually holds. Acceptable state.
IPC-11 Open Still open, unchanged Strict equality on both sides: Worker/Ipc/WorkerPipeSession.cs:370-375 (ProtocolVersionMismatch), Server/Workers/WorkerClient.cs:913-918; no min/max-range note on supported_protocol_version (mxaccess_worker.proto:43). No action was ever required.
IPC-12 Open Still open, surface unchanged Server/Workers/WorkerFrameWriter.cs:8-11 class doc still has no single-writer remark and the class has no lock; the invariant is now stated at the call site instead (WorkerClient.cs:465-470: "This loop is the channel's single consumer (SingleReader = true)"), and every outbound envelope — hello :171, commands :251, cancels :848-858, shutdown — funnels through _outboundEnvelopes. The gateway reader, by contrast, got its contract written into the class remark when GWC-30 made it stateful (WorkerFrameReader.cs:11-16); the writer deserves the same one-paragraph note.
IPC-16 / IPC-22 N/A Unchanged Correlation id still carried on envelope + inner reply (gateway.md:432-434 documents envelope as authoritative); status-code-plus-prose error model unchanged.
IPC-18 Preserve Preserved See "Proto evolution" above — Generated/, all five client trees, the protoset, and the Rust vendored copies are in sync at HEAD; galaxy_repository.proto retention comment intact (ZB.MOM.WW.MxGateway.Contracts.csproj:36-41).

NEXT rows touching this domain

  • NEXT-04 / NEXT-05 (frame-writer abandonment observation; lazy tombstone purge as a documented decision) — both verified in 20-worker.md; the contract-level statement lives at docs/WorkerFrameProtocol.md:213-224. Nothing further from this domain.
  • NEXT-01 (pipe name length) — verified in 10-gateway-core.md; the frame protocol is unaffected (the name is a transport address, not a wire field).

Also verified in passing

  • TST-28 — the gateway half of the max_frame_bytes handshake is pinned on the CI-run suite: WorkerClientTests.StartAsync_SendsGatewayHelloWithConfiguredMaxFrameBytes (:46-57), hello built at WorkerClient.cs:1172-1184 from FrameOptions.MaxMessageBytes.
  • Negotiation floor/ceiling agreement — worker accepts [1024, 256 MiB] (Worker/Ipc/WorkerFrameProtocolOptions.cs:19,30,137-161); the gateway validator bounds Worker:MaxMessageBytes to the identical range (Configuration/GatewayOptionsValidator.cs:10-11,290-293) and enforces pipe ≥ gRPC + 64 KiB with long arithmetic (:614-628). The worker can never reject a value the gateway's validator admits.
  • Public gRPC caps are actually applied — MaxReceiveMessageSize and MaxSendMessageSize are both set from Protocol:MaxGrpcMessageBytes (Security/Authorization/GrpcAuthorizationServiceCollectionExtensions.cs:36-37); default 16 MiB (Configuration/ProtocolOptions.cs:16). Load-bearing for IPC-33.
  • Check 3 both directions (711793f) — a canonical proto without a vendored Rust copy now fails too (scripts/check-codegen.ps1:95-103), closing the build.rs-prefers-canonical blind spot its comment describes.
  • Contracts package version bumped to 0.2.0 with the CLI-39 release train (ZB.MOM.WW.MxGateway.Contracts.csproj:10); Grpc.Tools 2.80.0 / Google.Protobuf 3.34.1 pins unchanged.

New findings

IDs continue from the register (cycle 2 ended at IPC-32). No IPC-33+ label exists anywhere in the repo or its history (grep), so there is no collision of the GWC-31 kind here.

IPC-33 — The DrainEvents byte budget is sized against the pipe frame maximum, not the public gRPC send cap; the reply can cross the pipe and then fail at the gRPC boundary with the drained events already consumed

  • Severity: Low
  • Files: src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs:31 (DrainReplyFrameHeadroomBytes = 64 KiB), :1034-1039 (ResolveDrainReplyByteBudget = max(MaxMessageBytes − 64 KiB, MaxMessageBytes / 2)); src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs:389-429 (events dequeued as they are packed); src/ZB.MOM.WW.MxGateway.Server/Configuration/WorkerOptions.cs:57-66 (pipe default = gRPC 16 MiB + 64 KiB reserve); src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs:614-628 (pipe ≥ gRPC + reserve, no upper coupling); src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GrpcAuthorizationServiceCollectionExtensions.cs:37 (MaxSendMessageSize = MaxGrpcMessageBytes); src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:20 (Invoke returns the bare MxCommandReply).
  • Description: WRK-21/IPC-23 made the reply fit the pipe (R1), but the same MxCommandReply is then returned verbatim through Invoke, where the gateway's gRPC MaxSendMessageSize applies. The two limits are coupled only from below: the validator requires pipe ≥ gRPC + 64 KiB, and the drain budget subtracts the same 64 KiB — so at default configuration the event bytes are budgeted to exactly MaxGrpcMessageBytes (16 MiB), leaving the MxCommandReply wrapper (34-byte correlation id, protocol status, kind, the drain_events length prefix, and — precisely in the truncated case — the DiagnosticMessage) with zero margin. The per-event RepeatedFieldOverheadBytes slack (+8 per event) absorbs this whenever the packed batch has more than ~30 events, so the default-config overshoot needs a backlog of ≥ 16 MiB composed of a few very large (≥ ~500 KiB) array events — narrow but real. The coupling is entirely absent for the configurations the validator explicitly permits: Worker:MaxMessageBytes may be anything up to 256 MiB while Protocol:MaxGrpcMessageBytes stays 16 MiB, and then every DrainEvents against a backlog above 16 MiB builds a reply of up to pipeMax − 64 KiB, which the worker writes, the pipe carries, the gateway parses — and the gRPC layer refuses to send (ResourceExhausted: Sending message exceeds the maximum configured message size). At that point the events are already dequeued from the worker queue (Drain removes what it packs), so up to 10 000 events are lost to both the diagnostic caller and the event stream, and the caller sees only a failed Invoke. This is the IPC-23 invariant ("no diagnostics command may kill a session") satisfied, and its sibling ("no event loss on truncation", R2) violated one boundary later. The same asymmetry exists for single events in the 64 KiB window between the gRPC cap and the pipe cap (an event the pipe accepts but StreamEvents cannot send), but a single ≥ 16 MiB event is pathological and already fails loudly on the stream; it is noted, not filed.
  • Recommendation: Bound the drain against the smaller of the two boundaries. Cheapest correct fix without a wire change: the gateway knows both caps, so either (a) add an upper coupling in GatewayOptionsValidator (Worker:MaxMessageBytes ≤ MaxGrpcMessageBytes + 2 × reserve, or simply document and validate the equality the default already has) and change the worker budget to MaxMessageBytes − 2 × reserve so the reply wrapper has a full reserve below the gRPC cap; or (b) convey MaxGrpcMessageBytes in GatewayHello (additive uint32, same zero-means-unknown convention as max_frame_bytes) and budget against min(pipe − reserve, grpc − reserve). Add a gateway fake-worker test that drains a reply packed to the budget through the real gRPC pipeline (the TST-24 wire-test harness can host it) so the boundary is exercised end to end.

IPC-34 — WorkerCancel is forwarded only when the gateway's own command timeout fires; a caller-cancelled Invoke (client deadline, disconnect) leaves the command queued on the worker's STA

  • Severity: Low
  • Files: src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs:269-291 (timeout arm → TrySendCancelForTimedOutCommand), :294-305 (OperationCanceledException arm: RemovePendingCommandAsFailed(GatewayShutdown, "Command wait was canceled."), no cancel sent), :307-318 (timer-won-but-token-cancelled arm, same); src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs:138 (context.CancellationToken flows into InvokeAsync); src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs:1100; worker side src/ZB.MOM.WW.MxGateway.Worker/Sta/StaCommandDispatcher.cs:151-185 (CancelQueuedCommand drops only not-yet-started entries); docs/GatewayProcessDesign.md:596-602; gateway.md:913-919.
  • Description: The IPC-08 closure's own rationale — "the gateway has stopped waiting, but the worker has not stopped working … a queued-but-not-started command is dropped instead of occupying the STA behind a caller that is already gone" (WorkerClient.cs:279-284) — applies identically when the caller's token fires, and that is the common abandonment path: gRPC clients routinely set per-call deadlines shorter than the gateway's CommandTimeout, and a client disconnect cancels context.CancellationToken. Both land in the OperationCanceledException arm, which removes the pending entry and returns, so the correlation stays on the single STA queue and executes when its turn comes. Under a burst of deadline-cancelled calls (a client retry storm against a slow provider) the STA works through dead commands while live callers queue behind them — exactly the backlog the cancel was added to shorten. The gateway-side design text still frames cancellation as "stop waiting" only (gateway.md:915), and docs/GatewayProcessDesign.md:596 says "on timeout", so the asymmetry is documented-by-omission rather than decided. Secondary: the arm labels caller cancellation WorkerClientErrorCode.GatewayShutdown (pre-existing; maps to Cancelled at the gRPC layer per docs/Grpc.md:285, so the client sees the right status, but metrics/logs record a shutdown that did not happen).
  • Recommendation: Call the same no-throw TrySendCancelForTimedOutCommand (renamed to TrySendCancelForAbandonedCommand, reason "caller cancelled") from the caller-cancellation arms, gated on the same removed ownership check so a reply that won the race is never chased. Skip it only when _stopCts is the source of the cancellation (the channel is being completed anyway). Add a WorkerClientTests twin of the timeout test driven by the caller token. Separately, introduce a CallerCancelled error code (or reuse CommandTimeout semantics) so the metric label stops saying GatewayShutdown; update the docs/Grpc.md:285 mapping line in the same change.

IPC-35 — A WorkerFault the worker emits during the startup handshake is flattened by the gateway into a generic ProtocolViolation, discarding the structured fault category and diagnostic message

  • Severity: Low
  • Files: src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs:894-907 (ReadHandshakeEnvelopeAsync: any body other than the expected one → WorkerClientException(ProtocolViolation, "Worker handshake expected {expected} but received {actual}")), :164-185 (StartAsync expects WorkerHello then WorkerReady); worker side src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerPipeSession.cs:335-357 (CompleteStartupHandshakeAsync writes a WorkerFault for every WorkerFrameProtocolException and for MxAccessCreationException — COM creation failure, the most likely real-world startup fault — before rethrowing), :1392-1447 (TryWriteFaultAsync overloads); src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs:132-160 (the exception message is what reaches MarkFaulted, the warning log, and the SessionManagerException inner).
  • Description: The worker deliberately reports startup failures as structured frames — MxAccessCreationException.From(...) carries the COM HRESULT/ProgID context and WorkerFaultCategory — so the gateway can tell "MXAccess is not installed / not registered / access denied" apart from a pipe-level protocol problem. The gateway's handshake reader does not look inside: it sees BodyCase == WorkerFault where it expected WorkerReady, throws a ProtocolViolation whose message names only the body type, and SessionManager.OpenSessionAsync surfaces that text to the operator log and to the client's OpenFailed. The post-handshake path does this correctly (DispatchEnvelope → SetFaulted(WorkerFaulted, CreateWorkerFaultMessage(fault)), WorkerClient.cs:614-618), so the information loss is confined to the window in which it matters most — first session open on a freshly deployed host, where the answer is almost always an MXAccess installation or permission problem, and the operator is pointed at "protocol violation" instead. The FakeWorkerHarness only ever scripts well-formed handshakes (Tests/Gateway/Workers/Fakes/FakeWorkerHarness.cs:140-200), so nothing pins the behaviour either way.
  • Recommendation: In ReadHandshakeEnvelopeAsync, special-case BodyCase == WorkerFault: throw WorkerClientException(WorkerFaulted, CreateWorkerFaultMessage(envelope.WorkerFault)) (the same formatter the steady-state path uses) so category and diagnostic reach the log and the OpenFailed detail. Keep ProtocolViolation for every other unexpected body. Add a WorkerClientTests case where the fake writes a WorkerFault{Category = MxaccessCreationFailed} in place of WorkerReady and asserts the category text surfaces.

IPC-36 — The worker launch environment has become a second, undeclared gateway↔worker contract: six variable names duplicated as independent literals on both sides, four of them new since baseline, two documented

  • Severity: Low
  • Files: gateway literals src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerProcessLauncher.cs:18-52 (MXGATEWAY_WORKER_NONCE, …_PIPE_CONNECT_ATTEMPT_TIMEOUT_MS, …_WRITE_COMPLETION_WAIT_MS, MXGATEWAY_ALARM_POLL_INTERVAL_MS, MXGATEWAY_ALARM_MAX_ALARMS_PER_FETCH, MXGATEWAY_EVENT_QUEUE_CAPACITY), set at :207-217; worker literals src/ZB.MOM.WW.MxGateway.Worker/Bootstrap/WorkerOptions.cs, Ipc/WorkerPipeClient.cs, MxAccess/MxAccessStaSession.cs:14-24, MxAccess/MxAccessEventQueue.cs, MxAccess/WnWrapAlarmConsumer.cs (one copy each); the one precedent that is shared: src/ZB.MOM.WW.MxGateway.Contracts/GatewayContractInfo.cs:35-43 (LiveMxAccessOptInVariableName, shared "so any future opt-in tweak does not silently leave one project behind"); docs docs/WorkerProcessLauncher.md:25-27,47-51 (nonce + connect timeout only), docs/MxAccessWorkerInstanceDesign.md:93-98 ("Expected protected environment values": nonce + log context only); public proto comment src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:540 names MXGATEWAY_WORKER_WRITE_COMPLETION_WAIT_MS.
  • Description: Since 4f5371f, four runtime knobs were plumbed to the worker through ProcessStartInfo.Environment rather than through GatewayHello — write-completion wait, alarm poll cadence, alarm fetch cap, event-queue capacity. Environment conveyance is a reasonable choice (the values are needed before or independent of the handshake, and the worker's fallback-to-default on a missing variable keeps old/new pairs working), but it makes the launch environment a wire contract in everything but name, and it is treated with none of the discipline the proto contract gets: each name exists as two unrelated string literals in two projects (a typo on either side silently reverts that knob to its default — the worker's documented "missing or unparseable → default" fallback turns a contract break into a no-op), the Contracts project already demonstrates the right home for exactly this kind of name and was not used, and the documentation that should enumerate the contract lists two of the six. The public mxaccess_gateway.proto comment additionally exposes the internal env-var name to every language-client consumer, where the operator-facing key is MxGateway:Worker:WriteCompletionWaitMilliseconds (the WorkerOptions.cs:28-34 and launcher :25-27 doc comments are also stale — they still say "WriteSecured/WriteSecured2" although b0e65d4 extended the wait to plain Write/Write2).
  • Recommendation: Move the six names into a WorkerLaunchEnvironment static class in ZB.MOM.WW.MxGateway.Contracts (net48-safe const strings, like GatewayContractInfo) and reference it from both sides; add a one-table "Launch environment" section to docs/WorkerProcessLauncher.md (variable, source config key, worker default on absence) and point docs/MxAccessWorkerInstanceDesign.md:93-98 at it; replace the env-var name in the proto comment with the config key; refresh the two "WriteSecured-only" comments. A small test asserting the launcher sets every constant in the class keeps the table and the code from drifting.

IPC-37 — The galaxy_repository.proto wire-identity with the ZB.MOM.WW.GalaxyRepository package is still unguarded, although both descriptors are now loadable in one test process

  • Severity: Low
  • Files: src/ZB.MOM.WW.MxGateway.Contracts/ZB.MOM.WW.MxGateway.Contracts.csproj:36-41 (retention rationale: the proto is the codegen source for all five clients and must stay wire-identical to the package); src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj:23 (ZB.MOM.WW.GalaxyRepository 0.2.0 — the server serves the package's implementation); src/ZB.MOM.WW.MxGateway.Tests/Contracts/ClientProtoInputTests.cs:65-70 (walks GalaxyRepositoryReflection.Descriptor from Contracts only); package proto ~/Desktop/scadaproj/ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Protos/galaxy_repository.proto (last changed 5572edd, 2026-06-23).
  • Description: Verified at HEAD: the two files differ only in option csharp_namespace (comment-stripped diff), so the invariant holds today. Nothing keeps it true. The package version moves with the whole ZB.MOM.WW.* family (repo memory: a version jump is not evidence the package changed), so a routine PackageReference bump can bring a wire change in galaxy_repository.v1 — a new field, a renamed RPC, a different streaming shape — that the server starts serving while the five clients keep generating from the retained, now-stale copy. Check 1–4 and the descriptor test would all stay green, because they compare the clients against the retained proto, never the retained proto against the served one. Cycle 1 recorded this as an optional test (archreview/remediation/30-contracts-ipc.md:340-344) when it would have required a cross-repo path; that is no longer the case — the test project transitively loads the package through the server reference (Tests/Configuration/GalaxyRepositoryOptionsValidatorTests.cs and four other test files already using ZB.MOM.WW.GalaxyRepository), so ZB.MOM.WW.GalaxyRepository.Grpc.GalaxyRepositoryReflection.Descriptor and ZB.MOM.WW.MxGateway.Contracts.Proto.Galaxy.GalaxyRepositoryReflection.Descriptor are both in-process.
  • Recommendation: Add GalaxyContractParityTests: take FileDescriptor.ToProto() from both, clear SourceCodeInfo and Options.CsharpNamespace (and Name if the paths differ), and assert byte-equality of the serialized FileDescriptorProto. One test, no protoc, runs in the portable CI job, and turns a silent package-bump drift into a red build with the exact differing symbol named. Record the new guard in the csproj retention comment and docs/Contracts.md.

IPC-38 — docs/Grpc.md has no coverage of DrainEvents although it is a public MxCommandKind reachable through Invoke; the tracker's reason for omitting it is inaccurate

  • Severity: Info
  • Files: docs/Grpc.md (grep for DrainEvents → 0 hits); src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:136 (DrainEventsCommand drain_events = 103 in the public MxCommand oneof), :520-523, :701-706; src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGrpcRequestValidator.cs:74-88 (public-boundary validation of max_events); archreview/2026-07-12/remediation/00-tracking.md:87 ("docs/Grpc.md intentionally carries no DrainEvents row since it scopes to the seven public gRPC RPCs and DrainEvents is a worker IPC diagnostic"); gateway.md:565-572 (does document it).
  • Description: DrainEvents is not an IPC-only diagnostic: any client with invoke:read can issue it through Invoke, the public validator has a rule for it, and the cycle-2 plan (IPC-23 step 5) asked for a rules-table note because the reply is now truncated on two axes with a drain-until-empty contract that callers must know. The tracker closed that step on a premise the proto contradicts. gateway.md carries the prose, so the information exists; docs/Grpc.md is the document the client READMEs and the rules table point readers at for per-command behaviour, and it is silent.
  • Recommendation: Add a DrainEvents row to the rules table in docs/Grpc.md (count cap, byte cap, DiagnosticMessage truncation marker, drain-until-empty, and that drained events are removed from the session's event stream), cross-linking gateway.md:565. Correct the tracker note.

IPC-39 — The IPC-03 CommandTooLarge pre-check now measures the envelope before GWC-28 stamps the sequence, leaving an ≤ 11-byte window in which the writer's MessageTooLarge is still session-fatal; the explanatory comment has the direction of conservatism backwards for the pre-check itself

  • Severity: Info
  • Files: src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs:236-249 (pre-check: CalculateSize() on an envelope whose Sequence is still 0 → the sequence field is not encoded), :465-471 (WriteLoopAsync stamps Sequence → up to 1 tag + 10 varint bytes added), src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerFrameWriter.cs:42-55 (post-stamp CalculateSize() > max → MessageTooLarge), :478-483 (write-loop exception → SetFaulted(WriteFailed), session-fatal).
  • Description: The comment at :236-241 correctly explains why the pre-check size cannot be reused as the writer's knownSize ("a lower bound … conservative in the right direction") — true for that purpose, but the same fact inverts for the pre-check's own purpose: a lower bound admits envelopes the writer will reject. An envelope in (MaxMessageBytes − 11, MaxMessageBytes] passes :243 and faults the session at the write. Reachability is nil under the shipped validator: every caller-controlled command arrives through gRPC capped at MaxGrpcMessageBytes, which is ≥ 64 KiB below the pipe max, and gateway-authored commands are tiny — so this is a correctness-of-argument note, not a live defect. It is worth recording because the headroom rule is the only thing making it unreachable, and the comment currently reads as if the pre-check were exact.
  • Recommendation: Either compare against MaxMessageBytes − MaxSequenceEncodingBytes (11) in the pre-check, or reword the comment to say the pre-check is a lower bound that relies on the 64 KiB reserve for its safety margin. No test needed beyond the existing CommandTooLarge case.

Severity roll-up (new findings)

Severity Count IDs
Critical 0 —
High 0 —
Medium 0 —
Low 5 IPC-33, IPC-34, IPC-35, IPC-36, IPC-37
Info 2 IPC-38, IPC-39

Prior-finding verification: 8/8 cycle-2 Done claims hold (IPC-23 with the IPC-33 residual); IPC-31 superseded by GWC-28 (recommend flipping to Resolved); cycle-1 IPC-08 substantially resolved by 7c1ea12 (recommend closing, residual tracked as IPC-34); IPC-10/11/12 confirmed still open, none regressed.

Still good / preserve under change

The contract boundary is in the best shape it has been across three cycles. Proto evolution since baseline was purely additive with correct next-free tags and reserved ranges intact, and — for the first time — every generated artifact (C# Generated/, Go, Python, Java, Rust vendored copies, the protoset) moved in the same commit as the proto, including for a comment-only edit; the freshness net that makes this checkable is now genuinely closed (Checks 1–4 + the reflection test + the Java git diff gate, all in CI, all tool-missing-fails-not-skips, all with their generator pins asserted rather than assumed). The frame protocol core is unchanged where it should be — length validated before allocation on both readers, exact-read loops with typed EndOfStream, pooled buffers with sound lifetimes on all four codec halves, single-serialization single-write framing — and materially stronger where it changed: both sides now stamp sequence at the point of writing so the documented "monotonic per sender" contract is finally true on both ends, the negotiated max_frame_bytes handshake has matching [1024, 256 MiB] bounds on both sides so a legal gateway configuration can never be refused by the worker, the DrainEvents reply is bounded on count and bytes with no event loss at the pipe, oversized events die structured and named, and cancelled writes are guaranteed never to reach the wire. WorkerCancel is no longer a dead arm. docs/WorkerFrameProtocol.md is now a faithful, claim-by-claim description of the shipped writer, and gateway.md's envelope section points at the proto as the single source of truth instead of carrying a drifting copy. Preserve in particular: the per-client generator pins and the "tool missing = check fails" rule (a skipped guard is the silent-drift hole the whole P0 codegen batch existed to close); the pipe-over-gRPC 64 KiB reserve invariant in the validator (IPC-33 and IPC-39 both rest on it); the habit of landing proto-comment changes with the full regen wave; and the csproj retention comment on galaxy_repository.proto — until IPC-37's test exists, that comment is the only thing standing between a package bump and a five-client wire drift.

Verification needed on Windows (windev)

  • This review is static. The worker-side halves cited here (byte-budgeted drain, IPC-30 structured death, tombstones, the floor/ceiling handshake rejection) are exercised only by Worker.Tests -p:Platform=x86; the tracker and repo memory record the suite fully green on windev after the harness fixes, which this review did not re-run.
  • IPC-33's end-to-end shape (reply built at budget → Invoke send failure) cannot be reproduced with the fake worker alone on macOS if the fix lands worker-side; the validator/gateway-side variant (option a) is portable and should get a GatewayOptionsTests case plus a TST-24-style wire test.
  • IPC-35's regression test is portable (fake worker writes a handshake-phase WorkerFault); no windev run needed.