Joseph Doherty
1a5a12a416
Six-domain re-review at the P2 merge (4f5371f): all prior Done claims
verified (none false, 10 partial), 47 new findings (1 High, 14 Medium),
with per-finding design/implementation plans and a new tracking register.
254 lines
40 KiB
Markdown
254 lines
40 KiB
Markdown
# Contracts & IPC Protocol — Remediation Design & Implementation (2026-07-12 cycle)
|
||
|
||
Source review: [../30-contracts-ipc.md](../30-contracts-ipc.md) (main @ `4f5371f`) · Prior cycle's plan: [../../remediation/30-contracts-ipc.md](../../remediation/30-contracts-ipc.md) · Roadmap: [../00-overall.md](../00-overall.md) · Generated: 2026-07-13
|
||
|
||
This cycle's findings cluster in two seams left by the prior remediation: **residual byte-size failure modes one layer out from the shipped fixes** (IPC-23/IPC-30 — the DrainEvents count cap and the per-frame-rejection machinery both stop short of the invariant "no single oversized payload may kill a session silently"), and **holes in the codegen freshness net** (IPC-24/IPC-25 — the CI Java churn-revert masks exactly the files where message-level drift lands, and the Go/Python worker bindings are demonstrably stale at HEAD with no guard). The rest is documentation drift and test-coverage blind spots.
|
||
|
||
Two findings in this domain share a defect with the worker domain and split ownership the same way the review does: **IPC-23 ≡ WRK-21** and **IPC-26 ≡ WRK-22**. For those, this plan defines the *contract-level requirements and documentation* and defers the worker-code mechanics to the worker plan; the worker plan's fix must satisfy the requirements stated here. IPC-30 (oversized *event* frame) is designed and implemented fully in this plan because the decision is a protocol-policy call, but its code lands in the same worker file and the same P0 batch as WRK-21, sharing one windev verification run.
|
||
|
||
All `path:line` citations were re-verified against the working tree at `4f5371f`.
|
||
|
||
## Finding register
|
||
|
||
| ID | Sev | Tier | Eff | Dep | Status | Title |
|
||
|----|-----|------|-----|-----|--------|-------|
|
||
| IPC-23 | Medium | P0 | S¹ | WRK-21 | Not started | DrainEvents bound is count-based only; byte-heavy queue still builds a session-killing reply frame (contract requirements here; fix mechanics in WRK-21) |
|
||
| IPC-24 | Medium | P0 | S | — | Not started | CI's unconditional Java churn-revert masks real generated-code drift for message-level proto changes |
|
||
| IPC-25 | Medium | P0 | M | — | Not started | Committed Go/Python worker bindings are stale at HEAD; no guard covers them |
|
||
| IPC-26 | Low | P2 | S¹ | WRK-22 | Not started | Cancelled write leaves a ghost frame that is still written (contract requirement here; fix mechanics in WRK-22) |
|
||
| IPC-27 | Low | P2 | S | — | Not started | Descriptor freshness test blind to enums, enum values, services/methods, and the Galaxy contract |
|
||
| IPC-28 | Low | — | S | — | Not started | `docs/Grpc.md` omits the `CommandTooLarge` → `ResourceExhausted` mapping |
|
||
| IPC-29 | Low | — | S | — | Not started | Worker writer priority scheduling and write-time sequence stamping undocumented in the frame-protocol doc |
|
||
| IPC-30 | Low | P0 | M | WRK-21 (same file/batch) | Not started | Oversized worker→gateway event frame is session-fatal — make the death deliberate, structured, and diagnosable |
|
||
| IPC-31 | Info | — | — | — | N/A | Gateway stamps sequence at creation, worker at write — accepted divergence; sequence is documented diagnostic-only (`gateway.md:328-330`); revisit only if sequence ever becomes load-bearing |
|
||
| IPC-32 | Info | — | S | IPC-25 | Not started | `check-codegen.ps1` check labels miscounted (folded into the IPC-25 script edit) |
|
||
|
||
¹ Effort for the work owned by *this* plan (proto comments + docs + acceptance criteria). The code mechanics are M and are tracked under WRK-21 / WRK-22 in the worker plan.
|
||
|
||
---
|
||
|
||
## IPC-23 — DrainEvents bound is count-based only; a byte-heavy queue still builds a session-killing reply frame `Medium` · `P0` · fix mechanics in **WRK-21**
|
||
|
||
**Finding.** `Worker/Ipc/WorkerPipeSession.cs:537-562` (`CreateDrainEventsReply`) clamps the drain to `MaxDrainEventsPerReply = 10_000` (`:26`) but never sizes the reply. At the default negotiated frame max (16 MiB + 64 KiB), events averaging above ~1.7 KiB overshoot `MaxMessageBytes`; the writer correctly rejects the frame per-frame (`Worker/Ipc/WorkerFrameWriter.cs:250-255`, `IsPerFrameRejection` `:192-198`) so the stream survives, but the exception propagates from `WriteControlReplyAsync` (`:481,485-496`) through `DispatchGatewayEnvelopeAsync` (`:406`) into `RunMessageLoopAsync` (`:280`) and out of `RunAsync` — the worker exits, and because `runtimeSession.DrainEvents(maxEvents)` already removed the events from the queue (`:551`), they are lost. This is the prior IPC-04 fix narrowed, not closed.
|
||
|
||
**Impact.** A diagnostics command (`DrainEvents`, `max_events = 0` or any large value) against an array-heavy queue — exactly the payload profile this gateway exists for — kills the session and destroys the drained events. Violates the invariant the roadmap states for this P0 item: *no diagnostics command may be session-fatal*.
|
||
|
||
**Design (contract-level requirements — the WRK-21 implementation must satisfy all of these).**
|
||
|
||
1. **R1 — Reply fits the negotiated frame.** Every worker→gateway control reply, `DrainEventsReply` included, must serialize (as a full `WorkerEnvelope`) within the negotiated `MaxMessageBytes`. Concretely: `CreateDrainEventsReply` must stop packing when the running `reply.CalculateSize()` plus the next event's serialized size plus a fixed envelope-overhead allowance would exceed the cap. The byte cap binds *in addition to* the existing `MaxDrainEventsPerReply` count cap, whichever is hit first.
|
||
2. **R2 — No event loss on truncation.** Events that do not fit the current reply must remain in (or be returned to) the event queue in order — the drain must be incremental (peek-pack-commit or drain-one-at-a-time), not bulk-drain-then-pack. Losing undelivered events would silently break the event stream's faithfulness.
|
||
3. **R3 — Truncation is expressible with the existing contract; no new field.** `DrainEventsReply` (`mxaccess_gateway.proto:678-680`) already permits returning fewer events than requested or available; the caller contract is *drain iteratively until an empty reply*. An additive `uint32 remaining`/`bool truncated` field was considered and **rejected for this pass**: it forces a full five-client regeneration fan-out (plus descriptor + `Generated/` + Rust vendored copies) for a diagnostics nicety the loop semantics already provide. Revisit only if operators need one-shot depth reporting — `WorkerHeartbeat.outbound_event_queue_depth` (`mxaccess_worker.proto:99`) already reports depth out-of-band.
|
||
4. **R4 — Byte-cap semantics are documented at the contract.** Proto comments and `docs/WorkerFrameProtocol.md` must state the rule (see Implementation).
|
||
|
||
**Implementation.** (Ordered; steps 1–2 are this plan's deliverable and should land in the *same commit* as the WRK-21 code fix so the proto-comment regeneration wave happens exactly once.)
|
||
|
||
1. `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_worker.proto`: extend the `GatewayHello.max_frame_bytes` comment (`:45-50`) with one sentence: *every worker→gateway frame — events, heartbeats, faults, and control replies including DrainEvents — must serialize within this limit; reply builders truncate to fit rather than emit an oversized frame.*
|
||
2. `src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto` (`DrainEventsReply`, `:678-680`): add a comment: *the reply is bounded by both a server-side count cap and the negotiated worker-frame byte cap; a reply may therefore carry fewer events than `max_events` and fewer than are queued. Callers drain iteratively until an empty reply.*
|
||
3. Comment-only proto edits still trigger the full regen chain (comments surface in generated C# XML docs, Go doc comments, Java javadoc, and the Rust vendored copies are byte-compared by `check-codegen.ps1` Check 3):
|
||
- regenerate `Contracts/Generated/` (`dotnet build src/ZB.MOM.WW.MxGateway.Contracts/ZB.MOM.WW.MxGateway.Contracts.csproj`, after `rm src/ZB.MOM.WW.MxGateway.Contracts/Generated/*.cs`) and **commit** (net48 CS0246 rule);
|
||
- refresh `clients/rust/protos/*.proto` (byte-copy from Contracts/Protos);
|
||
- regenerate Go (`pwsh clients/go/generate-proto.ps1`, pinned tools per IPC-25), Java (`gradle generateProto` on the pinned toolchain), Python (`pwsh clients/python/generate-proto.ps1` — comment-only edits normally produce *no* Python delta since `_pb2` embeds a source-info-free descriptor; commit only a real delta, revert noise);
|
||
- regenerate the descriptor set (`pwsh scripts/publish-client-proto-inputs.ps1` with pinned protoc 34.1) so grpcurl users see the new comments.
|
||
4. `docs/WorkerFrameProtocol.md`: add the byte-cap rule to the size-limits paragraph (`:19-30`): control replies are size-aware-packed; `DrainEvents` truncates to fit; drain-until-empty caller contract. (This lands in the same section IPC-29 adds — do the two doc edits together.)
|
||
5. `docs/Grpc.md` DrainEvents rules row (`:180` area): note the reply may be truncated by byte size as well as count; drain iteratively.
|
||
6. Worker code fix + tests: **deferred to WRK-21** (size-aware packing loop in `CreateDrainEventsReply`, incremental drain to satisfy R2, worker test that a queue of ~10,000 large events drains over multiple replies with the session alive and no event lost).
|
||
|
||
**Verification.**
|
||
- Contract side (this plan): `pwsh scripts/check-codegen.ps1` passes after the regen wave (all checks); `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx`; `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter FullyQualifiedName~ClientProtoInputTests`.
|
||
- Mechanics (WRK-21, on windev via the isolated `C:\build` worktree): `dotnet build src/ZB.MOM.WW.MxGateway.Worker/ZB.MOM.WW.MxGateway.Worker.csproj -p:Platform=x86` and `dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/ZB.MOM.WW.MxGateway.Worker.Tests.csproj -p:Platform=x86 --filter FullyQualifiedName~WorkerPipeSession` — including the WRK-21 repro test (enqueue ~10,000 large events, `DrainEvents max_events=0`, assert session survives and total drained count equals enqueued count across iterative replies).
|
||
|
||
---
|
||
|
||
## IPC-24 — CI's unconditional churn-revert step masks real Java generated-code drift for message-level proto changes `Medium` · `P0`
|
||
|
||
**Finding.** `.gitea/workflows/ci.yml:114-119` runs `git checkout -- clients/java/.../MxaccessGateway.java` and `.../MxaccessWorker.java` unconditionally before the "Verify generated tree is clean" step (`:120-121`). The Java bindings are single-file aggregates (`java_multiple_files` unset), so any message/field-level proto change lands **only** in those two files — a `.proto` edit committed without regenerating the Java client passes CI because the freshly regenerated (correct) files are reverted to the stale committed ones before the diff check. Only service-level changes (which touch the separate `*Grpc.java` stubs) would still be caught. The step's own comment scopes it to "when no `.proto` changed", but nothing enforces that condition.
|
||
|
||
**Impact.** The `checkGeneratedClean` intent from IPC-09 is neutered for the most common contract-change class. Combined with IPC-25, message-level drift currently has **zero** automated coverage in three of five clients.
|
||
|
||
**Design.** The churn the revert step works around is protobuf-gencode-version drift (`validateProtobufGencodeVersion(... major/minor/patch ...)` stanzas rewritten when the regenerating toolchain differs from the one that produced the committed files — repo memory `project_java_generated_churn`, ~64k lines). But the toolchain is now fully pinned: `clients/java/build.gradle:8,11` pin `grpcVersion = 1.76.0` / `protobufVersion = 4.33.1`, the protoc artifact and grpc plugin derive from those pins (`zb-mom-ww-mxgateway-client/build.gradle:40-46`), and the committed `MxaccessWorker.java` already stamps gencode 4.33.1. Under matching pins, regeneration should be **byte-identical** — meaning the revert step is a fossil from the pre-pin era, and the correct fix is to *delete it* and let `git diff --exit-code` be the real gate.
|
||
|
||
Discriminator design, in preference order:
|
||
|
||
1. **Preferred — prove the churn is gone under the pin and delete the revert.** Empirically regenerate on the pinned toolchain and check the tree. If clean, remove the revert step outright. Simplest, no new machinery, converts the Java job into a true drift gate.
|
||
2. **Fallback (only if churn persists) — diff modulo the known volatile lines.** Replace the unconditional checkout with a script step: after regeneration, take `git diff -U0` over the two aggregates, strip additions/deletions matching the volatile patterns (`validateProtobufGencodeVersion`, the `/* major= */ … /* patch= */` literals, generator-version header comments); empty residue → churn → revert; non-empty residue → **fail** with "Java generated bindings are stale — regenerate with the pinned toolchain and commit". Put the script in `scripts/check-java-generated.sh` so local use matches CI.
|
||
3. **Rejected — gate the revert on the push/PR diff containing no `*.proto` change** (`git diff --name-only $BASE..HEAD -- '*.proto'`). Two defects: the base-ref plumbing differs per trigger (push `github.event.before` vs PR base vs the nightly `schedule` run, which has no meaningful base), and it discriminates on the *delta*, not the *tree* — a stale binding merged in an earlier commit passes every subsequent run forever. The tree-level checks above are strictly stronger.
|
||
|
||
**Implementation.**
|
||
1. Prove/disprove churn locally (the Java client builds on the Mac now — memory `project_java_build_host`): `cd clients/java && JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle generateProto && git status --porcelain -- src/main/generated`. Expected: clean (pins match the committed gencode stamps).
|
||
2. If clean: delete the "Revert spurious protobuf-version churn" step (`.gitea/workflows/ci.yml:114-119`), keep `Verify generated tree is clean` (`:120-121`) as-is. Also delete the churn-revert comment at `:92-95`.
|
||
3. If churn persists: add `scripts/check-java-generated.sh` implementing design 2 and replace `ci.yml:114-119` with a step invoking it.
|
||
4. Same-commit doc updates (docs-change-with-source rule): `docs/GatewayTesting.md:421-425` (CI section prose describing the revert), `docs/ClientProtoGeneration.md:96-100` (the "revert that one file when no `.proto` changed" caveat), and the `checkGeneratedClean` comment block in `clients/java/zb-mom-ww-mxgateway-client/build.gradle:61-70` (the "CI reverts that one file" caveat). Update the repo memory note `project_java_generated_churn` guidance if step 1 shows the churn is gone.
|
||
5. Negative-path proof on a throwaway branch: add a scratch field to `mxaccess_worker.proto`, regenerate *only* `Contracts/Generated/` (not Java), push, and confirm the `java` CI job now **fails** at the diff step. Delete the branch.
|
||
|
||
**Verification.**
|
||
- Local: step 1's `git status --porcelain` output recorded in the PR description (clean or not — that decides branch 2 vs 3).
|
||
- CI-run evidence: a green `java` job on the fix branch (churn really is gone / discriminator correctly classifies), plus the deliberately-red run from step 5 proving message-level drift is now caught.
|
||
- No .NET/proto surface changes → no gateway/worker builds needed beyond CI's normal jobs.
|
||
|
||
---
|
||
|
||
## IPC-25 — Committed Go and Python worker bindings are stale at HEAD, and no guard covers them `Medium` · `P0`
|
||
|
||
**Finding.** `clients/go/internal/generated/mxaccess_worker.pb.go` (last regenerated 2026-05-23, commit `397d3c5`) and `clients/python/src/zb_mom_ww_mxgateway/generated/mxaccess_worker_pb2.py` (2026-06-18, `e328758`) both predate the 2026-07-09 `max_frame_bytes` addition — `grep -c max_frame_bytes` returns 0 for both at HEAD, while the Java aggregate has it (×6). The freshness net covers `Contracts/Generated` (check-codegen Check 2), the descriptor (Check 1 + `ClientProtoInputTests`), Rust vendored protos (Check 3), and Java (`checkGeneratedClean`) — nothing compares the committed Go/Python bindings against the protos, and their unit tests don't exercise the worker types, so CI stays green.
|
||
|
||
**Impact.** Functional impact today is nil (no language client consumes `GatewayHello`), but the published packages misrepresent the wire contract, and the CLAUDE.md verification-matrix rule ("regenerate every generated client touched by the contract") was violated with zero signal — the exact silent-drift class IPC-01/IPC-09 were meant to end.
|
||
|
||
**Design.** Two parts: (1) regenerate and commit both binding sets now; (2) close the guard hole with a **Check 4** in `scripts/check-codegen.ps1` that regenerates Go and Python bindings via the existing per-client scripts and fails on any git diff — the same regenerate-and-diff pattern Check 2 uses for `Generated/`. The per-client scripts already carry the churn-tolerance machinery this needs: `clients/python/generate-proto.ps1:36-48` hard-asserts grpcio-tools 1.80.0 (so a regen is deterministic and a diff means real drift, not toolchain noise), and `clients/go/generate-proto.ps1:9-43` resolves pinned-on-PATH tools. The one gap: the Go script does not assert plugin versions, and plugin-version drift stamps header churn (`protoc-gen-go v1.36.11` / `protoc-gen-go-grpc v1.6.2` in the committed headers) — add the same fail-fast version assertion the Python script has, so Check 4 cannot false-fail (or worse, mask drift under churn) on an off-pin machine.
|
||
|
||
Rejected alternatives: a semantic grep for known-new symbols (catches this instance, not the class); folding Go/Python regen into `ClientProtoInputTests` (the test project must stay toolchain-free — that is its documented value, `docs/ClientProtoGeneration.md:76-81`); comparing committed bindings against the descriptor set semantically (a weaker parse-level check that misses generator-behavior drift and requires per-language descriptor parsing for no gain over regenerate-and-diff).
|
||
|
||
**Implementation.**
|
||
1. **Pin the Go generators in the script.** `clients/go/generate-proto.ps1`: after `Resolve-Tool`, assert `& $protocGenGo --version` equals `protoc-gen-go v1.36.11` and `& $protocGenGoGrpc --version` equals `protoc-gen-go-grpc 1.6.2` (match the committed header stamps); throw with an install hint (`go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11`, `go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2`) on mismatch. protoc itself: warn-if-off-pin (34.1) matching `publish-client-proto-inputs.ps1` semantics.
|
||
2. **Regenerate Go** (macOS or Windows; PATH-first scripts work on both):
|
||
```
|
||
go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11
|
||
go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.6.2
|
||
pwsh clients/go/generate-proto.ps1
|
||
```
|
||
Expected delta: `max_frame_bytes` additions confined to `mxaccess_worker.pb.go` (descriptor bytes + `MaxFrameBytes` accessor). If other files churn, the pins are wrong — stop and reconcile before committing. Then from `clients/go`: `gofmt -l .` (must be empty), `go build ./...`, `go test ./...`.
|
||
3. **Regenerate Python** with the pinned toolchain (memory `project_python_client_regen_pin`):
|
||
```
|
||
python -m pip install 'grpcio-tools==1.80.0' # bundles protobuf 6.31.1 codegen
|
||
pwsh clients/python/generate-proto.ps1 # Assert-GrpcioToolsVersion enforces the pin
|
||
```
|
||
Expected delta: the serialized-descriptor line(s) in `mxaccess_worker_pb2.py` only — keep only the real descriptor delta; revert any file the regen did not meaningfully change. Then from `clients/python`: `python -m pytest`.
|
||
4. **Java untouched** (no `.proto` changed): if gradle ran incidentally, `git checkout -- clients/java/src/main/generated` (memory `project_java_generated_churn`; goes away if IPC-24 step 1 confirms the pin killed the churn).
|
||
5. **Add Check 4 to `scripts/check-codegen.ps1`:** "Go and Python client bindings match a fresh regeneration" — invoke `clients/go/generate-proto.ps1` and `clients/python/generate-proto.ps1`, then fail on non-empty `git status --porcelain -- clients/go/internal/generated clients/python/src/zb_mom_ww_mxgateway/generated`. Tool-missing must **fail** the check, not skip it (a skipped guard is this finding). Update the script's header comment ("Three checks" → four, list the new check) and **relabel all check banners `1/4`…`4/4`** (absorbs IPC-32).
|
||
6. **CI:** in `.gitea/workflows/ci.yml` `portable` job, before the `Codegen / descriptor freshness` step (`:60-62`), add the toolchain installs Check 4 needs: `go install .../protoc-gen-go@v1.36.11`, `go install .../protoc-gen-go-grpc@v1.6.2` (Go is already set up at `:28-30`), and `python -m pip install 'grpcio-tools==1.80.0'` (Python at `:32-34`; protoc 34.1 already installed at `:43-47`).
|
||
7. **Same-commit docs:** `docs/ClientProtoGeneration.md` pinned-versions table (`:88-95`): add `protoc-gen-go v1.36.11` / `protoc-gen-go-grpc v1.6.2` rows and change the Go/Python guard column to "check-codegen Check 4"; `docs/Contracts.md` cross-link if it enumerates the checks.
|
||
8. **Publishing note (cross-domain, CLI):** the currently *published* Go/Python packages carry the stale worker bindings. Do not republish from this fix alone — CLI-39 (version constants aligned to an already-published 0.1.2) must be resolved first; flag to the clients-domain plan.
|
||
|
||
**Verification.**
|
||
- `pwsh scripts/check-codegen.ps1` locally: all four checks green post-commit; then a negative run — touch a scratch field in `mxaccess_worker.proto`, regenerate only `Contracts/Generated/`, rerun and confirm **Check 4 fails** naming both stale directories; revert.
|
||
- `grep -c max_frame_bytes clients/go/internal/generated/mxaccess_worker.pb.go clients/python/src/zb_mom_ww_mxgateway/generated/mxaccess_worker_pb2.py` → both non-zero.
|
||
- `cd clients/go && gofmt -l . && go build ./... && go test ./...`; `cd clients/python && python -m pytest`.
|
||
- CI-run evidence: green `portable` job on the fix branch with the Check 4 banner visible in the log.
|
||
- Nothing here needs windev (no worker/x86 surface).
|
||
|
||
---
|
||
|
||
## IPC-26 — Worker frame writer: cancelling while waiting for the write lock leaves a ghost frame that is still written `Low` · `P2` · fix mechanics in **WRK-22**
|
||
|
||
**Finding.** `Worker/Ipc/WorkerFrameWriter.cs:87-113`: the `PendingFrame` is enqueued (`:88-98`) *before* `await _writeLock.WaitAsync(cancellationToken)` (`:103`). If the caller's token fires while waiting, `WriteAsync` throws `OperationCanceledException` but the frame stays queued — the next lock-holder writes it anyway (`DequeueNext` `:200-216` has no tombstone concept), its `Completion` resolves unobserved, and during shutdown leftover cancelled event frames can drain *behind* the `WorkerShutdownAck` that is nominally the last frame. If no writer ever follows, the frame lingers unwritten with an unobserved completion.
|
||
|
||
**Impact.** Benign today (the gateway drops post-shutdown/unknown frames), but the "cancelled means not written" contract is silently violated, and post-ack event emission is exactly the kind of latent seam a future gateway-side strictness change would trip over.
|
||
|
||
**Design (contract-level requirement — the WRK-22 implementation must satisfy this).** *A `WriteAsync` that completes with `OperationCanceledException` guarantees its frame is never subsequently written to the stream.* The recommended shape (owned by WRK-22): on cancellation, tombstone the pending frame — `Completion.TrySetCanceled()` under `_gate`, and have `DequeueNext` (or the drain loop) skip frames whose completion is already terminal. Removal-by-scan is equivalent; tombstoning avoids O(n) queue rewrites. Additionally, `WorkerShutdownAck` remains the last frame written: since the ack is a control frame and the scheduler drains control-before-event, the tombstone rule alone restores this (cancelled event frames were the only leak path). No proto or gateway change; no doc change beyond the IPC-29 section noting the cancellation semantics once WRK-22 lands.
|
||
|
||
**Implementation.**
|
||
1. Code + tests: **deferred to WRK-22** (`Worker/Ipc/WorkerFrameWriter.cs`; worker test: cancel a `WriteAsync` blocked on the lock, then drive another write, assert the cancelled envelope never reaches the stream and its task is `Canceled`).
|
||
2. This plan: when writing the IPC-29 doc section, include one sentence — *a write cancelled while queued is tombstoned and never reaches the wire* — phrased to land in the same commit as the WRK-22 fix (do not document it before it is true).
|
||
|
||
**Verification.** WRK-22's tests on windev: `dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/ZB.MOM.WW.MxGateway.Worker.Tests.csproj -p:Platform=x86 --filter FullyQualifiedName~WorkerFrameProtocolTests`. Doc cross-check against the shipped behavior.
|
||
|
||
---
|
||
|
||
## IPC-27 — Descriptor freshness test checks messages and fields only; enums, enum values, services/methods, and the Galaxy contract are blind spots `Low` · `P2`
|
||
|
||
**Finding.** `src/ZB.MOM.WW.MxGateway.Tests/Contracts/ClientProtoInputTests.cs:35-52` collects only `{message}` and `{message}/{field}` symbols and enumerates only `MxaccessGatewayReflection.Descriptor` and `MxaccessWorkerReflection.Descriptor`. A new enum value (e.g. a new `MxCommandKind` case — the most likely future contract change), a new RPC, or any change confined to `galaxy_repository.proto` would not redden the test. The script-side `-Check` in CI does catch all of these (full descriptor compare), so the gate as-a-whole holds — but `docs/ClientProtoGeneration.md:76-81` presents the test as the protoc-free **primary** gate, and on any runner without protoc it is the only one.
|
||
|
||
**Impact.** The documented primary guard is weaker than its documentation claims; a protoc-less environment (e.g. a future runner change) silently loses enum/service/Galaxy coverage.
|
||
|
||
**Design.** Extend the same reflection walk — all surfaces are available on `FileDescriptor` without protoc:
|
||
- **Enums:** top-level `file.EnumTypes` and nested `message.EnumTypes`, collecting `{enumFullName}` and `{enumFullName}/{valueName}` (value *presence* suffices; a renumber would be a non-additive change caught by review and the script check — asserting numbers here would duplicate that at the cost of a noisier failure mode).
|
||
- **Services/methods:** `file.Services`, collecting `{serviceFullName}` and `{serviceFullName}/{methodName}`.
|
||
- **Galaxy:** add `GalaxyRepositoryReflection.Descriptor` to the enumerated files (it is compiled into Contracts — `galaxy_repository.proto` is retained there as the client-codegen source per the csproj comment).
|
||
The published-side collection (`CollectPublishedSymbols`, `:61-79`) grows matching walks over `FileDescriptorProto.EnumType`/`Service` and `DescriptorProto.EnumType`. Keep the flat string-set comparison — it stays order-insensitive and protoc-free.
|
||
|
||
**Implementation.**
|
||
1. `ClientProtoInputTests.cs`: extend `Descriptor_ContainsEveryContractMessageAndField` (rename to `Descriptor_ContainsEveryContractSymbol`) per the design; failure message keeps listing missing symbols.
|
||
2. `docs/ClientProtoGeneration.md:76-81`: update the prose from "message or field" to the full symbol coverage — same commit.
|
||
3. No proto, config, or CI change (the test already runs in the portable job via the gateway test step).
|
||
|
||
**Verification.**
|
||
- `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter FullyQualifiedName~ClientProtoInputTests` (macOS).
|
||
- Red-path proof during development: extract the pre-IPC-01 stale protoset (`git show 0f88a95:clients/proto/descriptors/mxaccessgw-client-v1.protoset > <scratch>/stale.protoset`), point the test's path at it temporarily, and confirm it fails naming (at minimum) the `max_frame_bytes` field and — with the extension — any post-`0f88a95` enum values. Restore before commit.
|
||
|
||
---
|
||
|
||
## IPC-28 — `docs/Grpc.md` exception-mapping prose omits `CommandTooLarge` → `ResourceExhausted` `Low` · `—`
|
||
|
||
**Finding.** `Grpc/MxAccessGatewayService.cs:955` maps `WorkerClientErrorCode.CommandTooLarge` to `ResourceExhausted` (the IPC-03 per-correlation fix), but `docs/Grpc.md:249` still enumerates only `CommandTimeout`/`GatewayShutdown`/`InvalidState`/`ProtocolViolation` with "unmapped codes fall through to `Unavailable`" — a reader concludes an oversized command surfaces as `Unavailable`. (`docs/GatewayConfiguration.md:120-129` does document the headroom rule.)
|
||
|
||
**Impact.** Doc drift on a shipped, client-visible status mapping; violates the docs-change-with-source rule retroactively.
|
||
|
||
**Design.** Doc-only. Add `CommandTooLarge → ResourceExhausted` to the `WorkerClientException` mapping sentence, and one line in the Invoke handler section: an accepted gRPC payload whose worker envelope would exceed the pipe frame limit fails *that command* with `ResourceExhausted` (per-correlation, session stays alive) — cross-reference `docs/GatewayConfiguration.md`'s headroom rule.
|
||
|
||
**Implementation.**
|
||
1. `docs/Grpc.md:249`: extend the mapping prose.
|
||
2. `docs/Grpc.md` Invoke section: add the oversized-payload sentence.
|
||
3. If IPC-23/WRK-21 lands first, fold its DrainEvents-truncation doc row (IPC-23 step 5) into the same edit pass.
|
||
|
||
**Verification.** Doc-only; cross-check the prose against the `switch` in `Grpc/MxAccessGatewayService.cs:950-960` after editing.
|
||
|
||
---
|
||
|
||
## IPC-29 — Worker writer priority scheduling and write-time sequence stamping undocumented in the frame-protocol doc `Low` · `—`
|
||
|
||
**Finding.** The worker writer is now a cooperative priority scheduler (control frames drain ahead of event frames, `Worker/Ipc/WorkerFrameWriter.cs:11-17,116-124`) that stamps `Sequence` at the moment of writing under the lock (`:238-240`) and coalesces flushes per batch (`:125-182`). These are protocol-relevant semantics: a per-frame rejection consumes a sequence number (stamped at `:240` before the size check at `:250` throws), so the wire sequence has gaps after rejections, and event frames can be arbitrarily delayed behind a control backlog. `docs/WorkerFrameProtocol.md:1-7` still describes the frame layer as only "message boundaries, size limits, protobuf parsing, and envelope validation"; `gateway.md:328-330` covers the diagnostic-sequence half but not scheduling.
|
||
|
||
**Impact.** Anyone implementing an alternate-language worker (contemplated in `gateway.md`) or debugging sequence gaps from a pipe capture has no doc for the shipped semantics — the exact drift class the repo's docs-change-with-source rule exists to prevent.
|
||
|
||
**Design.** Doc-only: add a "Write scheduling and sequencing" section to `docs/WorkerFrameProtocol.md` covering (a) the two-class priority queues and the control-before-event drain guarantee; (b) enqueue-then-contend locking, single-writer drain; (c) sequence stamped at the point of writing so wire order and sequence order always agree on the worker side; (d) sequence gaps after per-frame rejections are expected and benign (sequence is diagnostic-only — cross-ref `gateway.md:328-330`; the gateway side stamps at creation, see IPC-31, so its sequence order is *not* a wire-order guarantee); (e) flush coalescing with the written-and-flushed completion contract; (f) once WRK-22 lands, the cancellation tombstone rule (see IPC-26 — do not document before it ships).
|
||
|
||
**Implementation.**
|
||
1. `docs/WorkerFrameProtocol.md`: insert the section after "Envelope Validation"; fold in IPC-23's byte-cap sentence (step 4 there) if landing together.
|
||
2. Optionally one cross-reference line in `gateway.md:328-330` pointing to the new section. No code change.
|
||
|
||
**Verification.** Doc-only; cross-check each claim against `Worker/Ipc/WorkerFrameWriter.cs` and `WorkerFrameWritePriority.cs` at time of writing.
|
||
|
||
---
|
||
|
||
## IPC-30 — Oversized worker→gateway event frame is still session-fatal despite the per-frame rejection machinery `Low` · `P0` (lands with WRK-21 in the same file/batch)
|
||
|
||
**Finding.** `Worker/Ipc/WorkerPipeSession.cs:374-382`: the event drain loop awaits each event write; a `MessageTooLarge` per-frame rejection from the writer (an MXAccess array event serializing above the negotiated max) throws out of `RunEventDrainLoopAsync`, is rethrown by the message loop's `await eventDrainTask` (`:294`), and terminates `RunAsync` — the whole session dies for one unsendable event, with no fault frame and no record of *which* event. The writer survives the rejection (`WorkerFrameWriter.cs:141-147`); the session dies anyway one layer up, wasting the machinery.
|
||
|
||
**Impact.** One pathological tag (a huge array) kills the session for every item the client has, and the operator gets a generic protocol-violation exit with no way to find the offending tag.
|
||
|
||
**Design — position taken: session-fatal is the correct contract; the defect is that the death is unstructured.** Rationale: an event above the negotiated frame max is **undeliverable end-to-end** — the negotiated pipe max sits only `EnvelopeOverheadReserveBytes` (64 KiB, `Server/Workers/WorkerFrameProtocolOptions.cs:20`) above the public gRPC cap, so any event the pipe rejects would also be rejected by the client-facing gRPC stream. Given that, the honest options reduce to how the session dies, not whether:
|
||
- **Silently drop and continue — rejected.** The event stream would silently stop being faithful; that breaks the delivery expectation behind every subscription, and the repo's "don't synthesize events" rule bars papering over it with a fabricated placeholder.
|
||
- **Drop + synthesized loss-marker event — rejected.** `ReplayGap` is a gateway-lifecycle stream element (replay-ring eviction), not a worker event; reusing it would lie about replay state, and a *new* loss-marker event is exactly the synthesized event the conventions prohibit.
|
||
- **Chunked/segmented event frames — rejected.** Contract surgery plus reassembly state on both sides for a case that indicates misconfiguration (`MaxMessageBytes` too small for the workload); MXAccess parity treats an event as atomic.
|
||
- **Chosen: deliberate, structured session-fatal.** Catch the per-frame `MessageTooLarge` rejection at the drain-loop seam, log the event identity — family, item/tag name, worker sequence, serialized size; **never the value payload** (redaction rule) — write a `WorkerFault` so the gateway sees a structured fault instead of a raw pipe drop, then exit. This mirrors the existing fail-fast precedent (`WORKER_FAULT_CATEGORY_QUEUE_OVERFLOW` faults the session on event-queue overflow) and the drain loop's existing fault pattern (`WorkerPipeSession.cs:356-365`). Operator remediation is config (`MxGateway:Worker:MaxMessageBytes`), and now the log names the tag that needs it.
|
||
- **Fault category: reuse `WORKER_FAULT_CATEGORY_PROTOCOL_VIOLATION` (`mxaccess_worker.proto:130`)** with a diagnostic message carrying the event identity and sizes. A dedicated additive enum value (`..._EVENT_TOO_LARGE = 12`) was considered and rejected *for this pass*: it triggers the full five-client + descriptor + `Generated/` regen fan-out for a distinction the diagnostic message already conveys; revisit if dashboards need to alert on it specifically. This keeps IPC-30 proto-neutral.
|
||
|
||
**Implementation.** (Same file and same P0 batch as WRK-21 — one commit or adjacent commits, one windev verification run.)
|
||
1. `Worker/Ipc/WorkerPipeSession.cs`, `RunEventDrainLoopAsync` (`:374-382`): wrap the per-event `WriteAsync` in a `catch (WorkerFrameProtocolException ex) when (ex.ErrorCode == WorkerFrameProtocolErrorCode.MessageTooLarge)`. In the handler: log event identity + serialized size (structured log, no values); set `_state = WorkerState.Faulted`; `await TryWriteFaultAsync(new WorkerFault { Category = ProtocolViolation, CommandMethod = "EventDrain", DiagnosticMessage = "<family> event for '<item>' (worker sequence N) serialized to X bytes, exceeding the negotiated frame max Y; raise MxGateway:Worker:MaxMessageBytes for this workload" }, ct)`; then rethrow/throw `InvalidOperationException` so `RunAsync` exits exactly as today — the fault frame is small and control-priority, so it precedes the exit. Other per-frame rejection codes (`InvalidEnvelope` etc.) keep today's behavior — they indicate worker bugs, not workload size.
|
||
2. Gateway side: no change — `WorkerClient`'s existing `WorkerFault` handling already surfaces structured faults to the session and dashboard.
|
||
3. Worker test (`Worker.Tests`, x86): fake runtime session emitting one event whose envelope exceeds a small test `MaxMessageBytes`; assert (a) a `WorkerFault` frame with `ProtocolViolation` and the item name in `DiagnosticMessage` is written before the pipe session ends, (b) the writer stream is not corrupted (a subsequent well-formed frame parse on the captured stream), (c) the session task completes faulted.
|
||
4. Same-commit docs: `docs/WorkerFrameProtocol.md` (in/next to the IPC-29 section): oversized-event policy — undeliverable end-to-end, session faults with a structured `WorkerFault` naming the event, remediation is `MaxMessageBytes` config. `docs/MxAccessWorkerInstanceDesign.md` event-drain description gets the same one-paragraph policy.
|
||
|
||
**Verification.**
|
||
- windev (isolated `C:\build` worktree, memory `project_windev_worker_verify`): `dotnet build src/ZB.MOM.WW.MxGateway.Worker/ZB.MOM.WW.MxGateway.Worker.csproj -p:Platform=x86`; `dotnet test src/ZB.MOM.WW.MxGateway.Worker.Tests/ZB.MOM.WW.MxGateway.Worker.Tests.csproj -p:Platform=x86 --filter FullyQualifiedName~WorkerPipeSession` (covers this and WRK-21 in one run).
|
||
- Gateway fake-worker regression: `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter FullyQualifiedName~WorkerClient` (fault propagation unchanged).
|
||
- Until TST-25's automated Windows tier exists, record the windev run (pass counts, date) in the tracking log — CI cannot evidence this one.
|
||
|
||
---
|
||
|
||
## IPC-31 — Gateway envelope sequence stamped at creation, not at write `Info` · N/A
|
||
|
||
**Finding.** `Server/Workers/WorkerClient.cs:1039` stamps `Sequence` via `Interlocked.Increment` inside `CreateEnvelope`; concurrent `InvokeAsync` callers can enqueue out of stamp order, so gateway→worker wire order can disagree with sequence order — the opposite semantics from the worker's write-time stamping (`Worker/Ipc/WorkerFrameWriter.cs:238-240`).
|
||
|
||
**Disposition: N/A — no action.** `gateway.md:328-330` already declares sequence a per-sender diagnostic aid, never validated; both validators ignore it; the divergence is harmless and now explicitly documented by the IPC-29 doc section (which states the two sides' stamping points). If sequence ever becomes load-bearing, stamp in the gateway write loop (`WorkerClient.cs:390-397`) — recorded here so the next editor finds the decision.
|
||
|
||
---
|
||
|
||
## IPC-32 — `check-codegen.ps1` check labels are miscounted `Info` · `—` (rides IPC-25)
|
||
|
||
**Finding.** `scripts/check-codegen.ps1:30,42,68` print "Check 1/2", "Check 2/2", then "Check 3/3" — the middle labels predate the third check. Cosmetic; CI log readers see a miscounted progression.
|
||
|
||
**Impact.** Log-readability only.
|
||
|
||
**Design/Implementation.** Folded into the IPC-25 script edit: when Check 4 is added, relabel every banner `1/4`…`4/4` and update the header comment's check list in the same change (IPC-25 implementation step 5). If IPC-25 were descoped (it must not be — it is P0), the standalone fix is relabeling `1/3`, `2/3`, `3/3`.
|
||
|
||
**Verification.** Banner text in the `pwsh scripts/check-codegen.ps1` output during the IPC-25 verification run; visible in the CI `portable` job log.
|
||
|
||
---
|
||
|
||
## Cross-domain dependencies and sequencing
|
||
|
||
- **WRK-21 / IPC-23 / IPC-30 cluster (P0):** one worker-side change set in `Worker/Ipc/WorkerPipeSession.cs` (+ the proto-comment/doc wave from IPC-23). Land as one batch so the x86 build, `WorkerPipeSession`-filtered test run on windev, and the proto regen fan-out each happen once. WRK-21 owns the size-aware DrainEvents packing (must satisfy IPC-23's R1–R3); IPC-30's structured-fault seam is in the same drain loop.
|
||
- **WRK-22 / IPC-26 (P2):** worker plan owns the tombstone fix; this plan's IPC-29 doc section documents the cancellation semantics only once it ships.
|
||
- **IPC-24 + IPC-25 vs TST-25 (CI):** both edit `.gitea/workflows/ci.yml` (java job step removal; portable job toolchain installs). TST-25's Windows-tier work will extend the same file — coordinate branches to avoid workflow-merge conflicts, and note that everything windev-verified in this plan (IPC-23/30, WRK-21/22) has no CI evidence until TST-25 lands; record windev runs in the tracking log meanwhile.
|
||
- **IPC-25 vs CLI-39 (publishing):** regenerated Go/Python bindings must not be republished to Gitea until the clients-domain version-collision finding (CLI-39) is resolved.
|