diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index a2619a4..6911b42 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -76,7 +76,7 @@ Full design + implementation for each row lives in the linked domain doc under i | WRK-23 | Low | — | S | WRK-21 | Done | Rejected frames consume sequence numbers, producing wire gaps | | WRK-24 | Low | — | S | — | Not started | `AdoptNegotiatedMaxMessageBytes` has no lower-bound sanity check | | WRK-25 | Low | P2 | S | WRK-22 (shared seam) | Not started | WRK-12 flush coalescing never engages on the event hot path | -| WRK-26 | Low | P1 | S | WRK-23 (soft); discharges IPC-29 | Not started | Write-priority and overflow doc drift from the WRK-07 change | +| WRK-26 | Low | P1 | S | WRK-23 (soft); discharges IPC-29 | Done | Write-priority and overflow doc drift from the WRK-07 change | | WRK-27 | Low | — | S | — | Not started | Alarm poll bypasses the watchdog's in-flight suppression (15 s vs 75 s) | | WRK-28 | Low | — | S | WRK-21 (same batch) | Done | 10,000 drain cap is a duplicated magic constant | @@ -89,8 +89,8 @@ Full design + implementation for each row lives in the linked domain doc under i | IPC-25 | Medium | P0 | M | — | Not started | Stale Go/Python worker bindings: regenerate (pinned toolchains) + check-codegen Check 4 | | IPC-26 | Low | P2 | S | WRK-22 (mechanics) | Not started | Cancelled write leaves ghost frame — cancelled means never written | | IPC-27 | Low | P2 | S | — | Not started | Descriptor-freshness test blind to enums/services/galaxy descriptor | -| IPC-28 | Low | — | S | — | Not started | docs/Grpc.md missing CommandTooLarge → ResourceExhausted mapping | -| IPC-29 | Low | — | S | WRK-26 (discharged by) | Not started | WorkerFrameProtocol.md missing write-scheduling/sequencing section | +| IPC-28 | Low | — | S | — | Done | docs/Grpc.md missing CommandTooLarge → ResourceExhausted mapping | +| IPC-29 | Low | — | S | WRK-26 (discharged by) | Done (discharged by WRK-26) | WorkerFrameProtocol.md missing write-scheduling/sequencing section | | IPC-30 | Low | P0 | M | WRK-21 (same batch) | Done | Oversized event frame: keep session-fatal, make the death structured | | IPC-31 | Info | — | — | — | N/A | Gateway creation-time sequence stamping accepted; diagnostic-only, decision recorded | | IPC-32 | Info | — | S | IPC-25 (folded in) | Not started | check-codegen banner relabel 1/4…4/4 | @@ -117,8 +117,8 @@ Full design + implementation for each row lives in the linked domain doc under i | CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 (land last) | Not started | Bump client versions off published 0.1.2 (converge on 0.2.0); registry-collision guard in pack-clients.ps1 | | CLI-40 | Low | — | M | — | Done | Port the exact-secret credential scrub to Rust/Java/.NET | | CLI-41 | Low | — | M | — | Done | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | -| CLI-42 | Low | P1 | S | — | Not started | Document the vendored Rust proto layout (CLI-02's missing doc half) | -| CLI-43 | Low | — | S | — | Not started | Java style guide still prescribes "Java 21 preferred" | +| CLI-42 | Low | P1 | S | — | Done | Document the vendored Rust proto layout (CLI-02's missing doc half) | +| CLI-43 | Low | — | S | — | Done | Java style guide still prescribes "Java 21 preferred" | | CLI-44 | Low | — | S | — | Done | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` | | CLI-45 | Low | P1 | M | — | Done | Standardize CLI credential env-var names; fail fast on missing/empty passwords | @@ -128,7 +128,7 @@ Full design + implementation for each row lives in the linked domain doc under i |---|---|:-:|:-:|---|---|---| | TST-25 | High | P1 | M | unlocks old TST-05, TST-24 | Done | Windows/x86 test tier has zero automation — SSH-driven windev CI job | | TST-26 | Medium | P1 | S | TST-25 (same commit) | Done | Docs/scripts describe removed CI jobs; Generated/-guard reattributed to check-codegen | -| TST-27 | Medium | P1 | S | — | Not started | `ShowTagValues` config row still says "Reserved" after SEC-25 made the flag live | +| TST-27 | Medium | P1 | S | — | Done | `ShowTagValues` config row still says "Reserved" after SEC-25 made the flag live | | TST-28 | Low | P2 | S | relates IPC-02 (old) | Done | Gateway-side `max_frame_bytes` handshake untested in the CI-run suite | | TST-29 | Low | P2 | S | — | Done | Retire `oldtasks.md` (fold Phase-5 governance into DesignDecisions.md); delete root artifacts | | TST-30 | Low | P2 | M | — | Not started | Single shared Gitea runner is a CI throughput/availability bottleneck (cross-repo contention, no run cancel/delete API) | @@ -150,7 +150,7 @@ Sequence these together rather than piecemeal — several are one change set spa - Close **CLI-24** and **CLI-34** as `Done` (incidentally fixed; evidence in [../50-clients.md](../50-clients.md)). - ~~When CLI-38 lands, close old **CLI-08** with a pointer here.~~ Done 2026-08-07: CLI-38 landed and old CLI-08 is now `Done` in the first-cycle tracker, pointing at [CLI-38](50-clients.md#cli-38--align-netgojava-on-hresult--0-lands-prior-cli-08-cures-the-doc-drift---medium--p1). -- When WRK-26 lands, its doc section also discharges the WorkerFrameProtocol gap; when TST-25 lands, revisit old **TST-05** (scheduled live smoke) and **TST-24** (client wire tests), which it unlocks. +- ~~When WRK-26 lands, its doc section also discharges the WorkerFrameProtocol gap~~ Done 2026-08-07: WRK-26 landed and **IPC-29** is discharged, pointing at [WRK-26](20-worker.md#wrk-26--write-priority-and-overflow-doc-drift-from-the-wrk-07-change---low--p1). When TST-25 lands, revisit old **TST-05** (scheduled live smoke) and **TST-24** (client wire tests), which it unlocks. ## Change log @@ -174,3 +174,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-08-07 | **GWC-28, GWC-29, GWC-30, TST-28 → `Done`** (branch `fix/gwc-28-29-30-polish`). GWC-28: `WorkerClient.WriteLoopAsync` now stamps `envelope.Sequence = unchecked(++_nextSequence)` immediately before `_writer.WriteAsync`, and `CreateEnvelope` leaves it unset; `_nextSequence` dropped from `long` + `Interlocked` to a plain `ulong` touched only by the write loop (the channel's single consumer, `SingleReader = true`), so wire order and sequence order are the same thing by construction. Mirrors the worker's WRK-04 stamping, which the gateway half had never received; `gateway.md`'s envelope-sequence rule now states that both sides stamp at write inside their single write path and that inbound enforcement (still open, old **GWC-10**) would rely on it. New `WorkerClientTests.ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire` (32 parallel invokes, sequences asserted strictly increasing in wire order) failed 3/3 pre-fix. GWC-29: added `MxAccessGrpcMapper.MapCommand(MxCommand)`; `Invoke` no longer deep-clones the whole `MxCommandRequest` just to overwrite and discard its command. The one clone inside `MapCommand` stays and is documented as required — `commandToInvoke` may be the gRPC-owned `request.Command` and is read again after dispatch by `TrackCommandReply`, so it is what keeps `CreateCommandEnvelope`'s no-aliasing invariant true. New `MxAccessGrpcMapperTests.MapCommandFromCommandClonesPayload` (isolation + both overloads equal under a `FakeTimeProvider`). GWC-30: `WorkerFrameReader` reuses a per-instance `_lengthPrefix` scratch buffer instead of allocating 4 bytes per frame, with a class remark that `ReadAsync` is not reentrant (single read loop per `WorkerClient`; handshake reads complete before the loop starts); guarded by new `WorkerFrameProtocolTests.ReadAsync_WithMultipleFramesOnOneReader_ParsesEveryFrame` (5 frames, varying payload lengths, one reader). TST-28: new `[Theory] WorkerClientTests.StartAsync_SendsGatewayHelloWithConfiguredMaxFrameBytes` over the default and a 2 MiB override via `FakeWorkerHarness.CreateConnectedPairAsync(maxMessageBytes:)` — test-only, and the mutation check (hard-code `MaxFrameBytes = 0`) failed both cases before being reverted. Verification: `NonWindows.slnx` 0 warnings/0 errors; `WorkerClientTests` 25 passed, `WorkerFrameProtocolTests` 11 passed, `MxAccessGrpcMapperTests` 6 passed, `MxAccessGatewayService*` 29 passed, full gateway suite 844 passed / 0 failed (`TMPDIR=/tmp` on macOS). | | 2026-08-07 | **WRK-21 + WRK-28 + WRK-23 + IPC-30 → `Done`** (branch `fix/wrk-21-drain-cluster`, commits `33ba612` + test-fixture follow-ups `7c2eaf0`/`a256560`). WRK-21: `MxAccessEventQueue` gains a byte-budgeted `Drain(maxEvents, maxTotalBytes)` returning the new `WorkerEventDrainResult`, sizing inside the queue lock so an event that will not fit is never dequeued; `CreateDrainEventsReply` budgets against the negotiated frame max less a 64 KiB wrapper reserve and reports truncation through the existing `DiagnosticMessage` (no proto change), satisfying IPC-23 R1–R3; both reply-write seams (`HandleControlCommandAsync`, `ProcessCommandAsync`) now catch `MessageTooLarge` and answer the correlation with an `InvalidRequest` reply instead of unwinding/faulting the session. WRK-28: the 10,000 ceiling moved to `GatewayContractInfo.MaxDrainEventsPerCommand`, referenced by the gateway validator and the worker clamp (C# const, no `.proto` change). WRK-23: `WorkerFrameWriter` peek-stamps then commits `Sequence` only immediately before the stream write, so rejections leave no wire gap. IPC-30: an oversized event frame stays session-fatal but writes a `PROTOCOL_VIOLATION` `WorkerFault` with `command_method = EventDrain` naming family/handles/sequence/sizes (never the value) before exiting. Docs same commit: `MxAccessWorkerInstanceDesign.md`, `WorkerFrameProtocol.md`, `gateway.md`. **IPC-23 → `In progress`** — mechanics landed here; the proto-comment/doc wave (and its regen fan-out) is still pending and must not be folded into this branch. **Evidence** — macOS: `dotnet build src/ZB.MOM.WW.MxGateway.NonWindows.slnx` 0 warnings/0 errors, `dotnet test …MxGateway.Tests --filter FullyQualifiedName~MxAccessGrpcRequestValidator` 4/4 passed. windev (`scripts/ci/windev-worker-ci.ps1 -Sha a2565604 -Mode test`, 2026-08-07 06:47): x86 Worker build 0 warnings/0 errors, `Worker.Tests` **367 passed / 0 failed / 11 skipped** (skips are the live-MXAccess/dev-rig opt-ins), script exit 0. **Harness note:** `PipePair` runs both pipe ends in one process with blocking `FlushFileBuffers` per frame, so it wedges on multi-MB frames or after ~85 large round trips; the pipe tests therefore negotiate a 128 KiB frame maximum and walk 1,000 events to empty, while the full 10,000-event drain-to-empty no-loss proof runs at the queue layer (`MxAccessEventQueueTests`). | | 2026-08-07 | Code-review follow-ups on the same branch (commit `6bc3f9b`). (1) **Important** — `ResolveDrainReplyByteBudget` was a step, not a floor: just above the 64 KiB reserve the budget collapsed to a few bytes (exactly 1024 at the validator floor `MaxMessageBytes = 1024 + 64 KiB`), so a byte-heavy `DrainEvents` truncated on every call and the drain-until-empty loop never terminated. Now `Math.Max(frameMax - reserve, frameMax / 2)` — monotonic, never below half the frame max. New test `WorkerPipeSessionTests.DrainEvents_AtValidatorFloorFrameMax_MakesProgressAndTerminates` drives a byte-heavy queue at the exact validator floor and asserts drain-to-empty with no head reported oversized. (2) **Hardening** — the reply-too-large fallback write is now itself size-guarded (`WriteReplyTooLargeFallbackAsync`, shared by the control and STA reply seams) so a pathologically tiny negotiated max below the gateway floor (the WRK-24 gap) cannot make even the backstop session-fatal; log-and-swallow, comment points at WRK-24. (3) **Comment** — corrected the `RepeatedFieldOverheadBytes` docs: `WorkerEvent.CalculateSize()` already includes the event's tag+length, so the 8 bytes is pure slack, not wrapper compensation. **Evidence** — macOS build 0/0, validator filter 4/4. windev (`windev-worker-ci.ps1 -Sha 6bc3f9b -Mode test`, 07:07): x86 Worker build 0/0, `Worker.Tests` **368 passed / 0 failed / 11 skipped**, script exit 0. (An earlier run of the same SHA flaked on the pre-existing `RunAsync_WhenStaActivityIsStale_WritesWatchdogFault` — a 5 s CTS timeout under first-run load, untouched by this change; it passed on the clean re-run and in both prior full runs.) | +| 2026-08-07 | **P1 doc-drift batch → `Done`: TST-27, WRK-26 (discharges IPC-29), CLI-42, CLI-43, IPC-28** (branch `fix/doc-drift-batch`). Doc-only; no source, proto, or test changes — cross-checked against HEAD in this worktree. **TST-27**: `docs/GatewayConfiguration.md`'s `ShowTagValues` row no longer says "Reserved" — it now states what `false` (default) does (`DashboardEventBroadcaster` blanks tag values from a deep-cloned `MxEvent` before the SignalR events-hub mirror, metadata still renders), the security relevance (the per-session hub ACL, SEC-25 roadmap item 12, still does not exist, so this redaction is the only thing between a low-trust Viewer and other sessions' tag values), and the honest scope limit (the flag does **not** cover `/browse`). **WRK-26** (discharges **IPC-29**): `docs/MxAccessWorkerInstanceDesign.md`'s "Outbound Queues" section rewritten from the stale five-level priority list to the two-class `Control`/`Event` scheduler actually shipped (`WorkerFrameWriter`/`WorkerFrameWritePriority.cs`), with the collapsed-decision rationale recorded, and the overflow paragraph rewritten to the implemented fail-fast (`WorkerFault` category `QueueOverflow` → fault frame written → `RunAsync` unwinds → generic `WorkerExitCode.UnexpectedFailure`, dedicated code still open). `docs/WorkerFrameProtocol.md` gained a new "Write Scheduling And Sequencing" section: the two priority classes, enqueue-then-contend/single-lock-holder-drains-all, write-time peek-stamp-commit sequencing, per-frame-rejection vs. stream-failure semantics, and flush coalescing — stated truthfully as landed (WRK-23's peek-stamp-commit is live at HEAD) or not (the drain loop still awaits each event `WriteAsync` individually, so WRK-25's N-events-one-flush batching has **not** landed and the section says so explicitly). Cancellation is deliberately **not** documented as a firm contract — a one-paragraph placeholder notes it is pending WRK-22, which has not landed (confirmed by reading `WorkerFrameWriter.cs`: no `Claimed`/tombstone machinery exists yet). `gateway.md:328-330` was cross-checked and left unchanged — its sequence prose (both sides stamp at write, per GWC-28) already reads true. **CLI-42**: `clients/rust/README.md` and `docs/ClientPackaging.md`'s Rust section now document the vendored proto layout matching `clients/rust/build.rs` exactly — repo-path-first resolution (`../../src/ZB.MOM.WW.MxGateway.Contracts/Protos`) falling back to `clients/rust/protos/` when the canonical path is absent (published-tarball case), the same-commit refresh rule enforced by `scripts/check-codegen.ps1` Check 3, and why `cargo package`/`cargo publish` run without `--no-verify` (matches `scripts/pack-clients.ps1:190-192`). **CLI-43**: `docs/style-guides/JavaStyleGuide.md` line 8 now says "Target Java 17 (the Ignition 8.3 baseline...)" mirroring the CLI-12 wording, matching the shipped `clients/java/build.gradle` toolchain-17 build. **IPC-28**: `docs/Grpc.md`'s exception-mapping prose gained `CommandTooLarge` → `ResourceExhausted` (verified against the live `switch` in `Grpc/MxAccessGatewayService.cs:950-960`), and the `Invoke` section gained one sentence on the oversized-payload path (`WorkerClient.InvokeAsync` rejects at the enqueue boundary per-correlation, session not faulted — verified against `WorkerClient.cs:220-234`), cross-referencing the headroom rule already documented in `docs/GatewayConfiguration.md:120-129`. Did not touch the DrainEvents-truncation row or the proto/`Generated/` trees — those belong to a parallel codegen task per the handoff note. **Source files cross-read for accuracy** (no edits): `src/ZB.MOM.WW.MxGateway.Worker/Ipc/WorkerFrameWriter.cs`, `.../WorkerFrameWritePriority.cs`, `.../WorkerPipeSession.cs` (confirmed two-class scheduler, WRK-21/23/28/30 landed, WRK-25/WRK-22 not landed), `src/ZB.MOM.WW.MxGateway.Worker/WorkerApplication.cs` (exit-code mapping), `src/ZB.MOM.WW.MxGateway.Worker/MxAccess/MxAccessEventQueue.cs` (overflow fault path), `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/DashboardEventBroadcaster.cs` + `Configuration/DashboardOptions.cs` + `docs/GatewayDashboardDesign.md:170` (ShowTagValues), `src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs:940-963` + `Workers/WorkerClient.cs:205-244` + `Workers/WorkerClientErrorCode.cs` (CommandTooLarge mapping), `clients/rust/build.rs`, `clients/rust/Cargo.toml`, `scripts/check-codegen.ps1`, `scripts/pack-clients.ps1` (Rust vendoring), `gateway.md:326-360` (sequence-prose cross-check). Verification (greps, doc-only — no build required): `grep -n 'Reserved' docs/GatewayConfiguration.md` no longer matches the `ShowTagValues` row; `grep -n 'faults' docs/MxAccessWorkerInstanceDesign.md` shows no remaining five-level list; `grep -n 'scheduling' docs/WorkerFrameProtocol.md` finds the new section; `grep -rn 'Java 21' docs/style-guides/` empty; `grep -i vendored docs/ClientPackaging.md clients/rust/README.md` non-empty in both; `grep -n 'CommandTooLarge' docs/Grpc.md` shows the mapping. | diff --git a/archreview/2026-07-12/remediation/20-worker.md b/archreview/2026-07-12/remediation/20-worker.md index 8b03ccb..63f85ff 100644 --- a/archreview/2026-07-12/remediation/20-worker.md +++ b/archreview/2026-07-12/remediation/20-worker.md @@ -21,7 +21,7 @@ members, no positional records). The worker builds and tests only on the Windows | WRK-23 | Low | — | S | WRK-21 (rejection path becomes backstop-only) | Done | Rejected frames consume sequence numbers, producing wire gaps | | WRK-24 | Low | — | S | — | Not started | `AdoptNegotiatedMaxMessageBytes` has no lower-bound sanity check | | WRK-25 | Low | P2 | S | WRK-22 (both touch enqueue/dequeue) | Not started | WRK-12 flush coalescing never engages on the event hot path | -| WRK-26 | Low | P1 | S | WRK-23 (soft — sequence prose); discharges IPC-29 | Not started | Write-priority and overflow doc drift from the WRK-07 change | +| WRK-26 | Low | P1 | S | WRK-23 (soft — sequence prose); discharges IPC-29 | Done | Write-priority and overflow doc drift from the WRK-07 change | | WRK-27 | Low | — | S | — | Not started | Alarm poll bypasses the watchdog's in-flight suppression (15 s vs 75 s) | | WRK-28 | Low | — | S | WRK-21 (land in the same commit cluster) | Done | 10,000 drain cap is a duplicated magic constant with a comment-only sync contract | diff --git a/archreview/2026-07-12/remediation/30-contracts-ipc.md b/archreview/2026-07-12/remediation/30-contracts-ipc.md index e495d42..ce367d9 100644 --- a/archreview/2026-07-12/remediation/30-contracts-ipc.md +++ b/archreview/2026-07-12/remediation/30-contracts-ipc.md @@ -17,8 +17,8 @@ All `path:line` citations were re-verified against the working tree at `4f5371f` | 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-28 | Low | — | S | — | Done | `docs/Grpc.md` omits the `CommandTooLarge` → `ResourceExhausted` mapping | +| IPC-29 | Low | — | S | — | Done (discharged by WRK-26) | 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) | Done | 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) | diff --git a/archreview/2026-07-12/remediation/50-clients.md b/archreview/2026-07-12/remediation/50-clients.md index db30076..a82835a 100644 --- a/archreview/2026-07-12/remediation/50-clients.md +++ b/archreview/2026-07-12/remediation/50-clients.md @@ -23,8 +23,8 @@ Operating constraints carried from prior work: | CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 | Not started | Bump client versions off the already-published 0.1.2 before the next publish; add registry-collision guard | | CLI-40 | Low | — | M | — | Done | Port the exact-secret credential scrub to Rust/Java/.NET | | CLI-41 | Low | — | M | — | Done | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | -| CLI-42 | Low | P1 | S | — | Not started | Document the vendored Rust proto layout (CLI-02's missing doc half) | -| CLI-43 | Low | — | S | — | Not started | Java style guide still prescribes "Java 21 preferred" | +| CLI-42 | Low | P1 | S | — | Done | Document the vendored Rust proto layout (CLI-02's missing doc half) | +| CLI-43 | Low | — | S | — | Done | Java style guide still prescribes "Java 21 preferred" | | CLI-44 | Low | — | S | — | Done | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` | | CLI-45 | Low | P1 | M | — | Done | Standardize CLI credential env-var name and fail fast on missing/empty passwords | diff --git a/archreview/2026-07-12/remediation/60-testing-docs-gaps.md b/archreview/2026-07-12/remediation/60-testing-docs-gaps.md index 7f4e23f..9db9bd5 100644 --- a/archreview/2026-07-12/remediation/60-testing-docs-gaps.md +++ b/archreview/2026-07-12/remediation/60-testing-docs-gaps.md @@ -12,7 +12,7 @@ Prior-cycle open findings (TST-05..24 where still open) are tracked in the prior |----|-----|------|-----|-----|--------|-------| | TST-25 | High | P1 | M | — (unlocks TST-05, TST-24) | Done | Windows/x86 test tier has zero automation — restore via SSH-driven windev CI job | | TST-26 | Medium | P1 (folded into TST-25) | S | TST-25 | Done | docs/GatewayTesting.md, check-codegen.ps1, and ci.yml comments describe removed CI jobs | -| TST-27 | Medium | P1 (doc batch) | S | — | Not started | `ShowTagValues` config row still says "Reserved" after SEC-25 made the flag live | +| TST-27 | Medium | P1 (doc batch) | S | — | Done | `ShowTagValues` config row still says "Reserved" after SEC-25 made the flag live | | TST-28 | Low | P2 | S | relates IPC-02 | Done | Gateway-side `max_frame_bytes` handshake field untested in the CI-run suite | | TST-29 | Low | P2 | S | — | Done | Retire `oldtasks.md` after folding the Phase-5 governance record into DesignDecisions.md; delete root docs-review artifacts | | TST-30 | Low | P2 | M | — | Not started | Single shared Gitea runner is a CI throughput/availability bottleneck (cross-repo contention, no run cancel/delete) | diff --git a/clients/rust/README.md b/clients/rust/README.md index a4aa80a..78671c6 100644 --- a/clients/rust/README.md +++ b/clients/rust/README.md @@ -18,9 +18,22 @@ clients/rust/ crates/mxgw-cli/ ``` -`build.rs` reads the `.proto` files from -`../../src/ZB.MOM.WW.MxGateway.Contracts/Protos` and generates `tonic`/`prost` bindings -into Cargo build output. `src/generated.rs` declares the Rust modules that +`build.rs` resolves the `.proto` inputs repo-path-first: it prefers the +canonical protos at `../../src/ZB.MOM.WW.MxGateway.Contracts/Protos` (two +levels above `clients/rust`) so a local in-repo `.proto` edit is picked up +live without any extra step, and falls back to the vendored copies checked +into `clients/rust/protos/` only when that canonical directory is absent — +the case for a consumer building the crate unpacked from a published +tarball, where the rest of the mxaccessgw repo does not exist. The vendored +copies are shipped in the published `.crate` via `Cargo.toml`'s `include` +list, which is what makes the crate buildable standalone; they are build +inputs only, never a second source of truth. **Refresh rule:** any commit +that edits a Contracts proto (`mxaccess_gateway.proto`, `mxaccess_worker.proto`, +`galaxy_repository.proto`) must copy the changed file(s) into +`clients/rust/protos/` in that same commit — `scripts/check-codegen.ps1` +Check 3 fails the build on byte drift between the vendored copies and the +canonical Contracts protos. `tonic`/`prost` bindings are generated into +Cargo build output. `src/generated.rs` declares the Rust modules that include those generated files. `src/generated` remains reserved for checked-in generator output if the crate later changes to source-tree generation. diff --git a/docs/ClientPackaging.md b/docs/ClientPackaging.md index 46fac8b..3ccac54 100644 --- a/docs/ClientPackaging.md +++ b/docs/ClientPackaging.md @@ -117,6 +117,22 @@ The Rust workspace builds the `mxgateway-client` library crate and the `mxgw` CLI crate. `build.rs` generates `tonic` and `prost` modules into Cargo build output on each build that needs updated protobuf output. +`build.rs` resolves its `.proto` inputs repo-path-first, then vendored: it +prefers the canonical protos under `src/ZB.MOM.WW.MxGateway.Contracts/Protos` +so an in-repo edit is live immediately, and falls back to the copies vendored +into `clients/rust/protos/` only when the canonical directory is absent — the +case for a published crate unpacked outside this repo. The vendored copies +are declared in `Cargo.toml`'s `include` list, so `cargo package`/`cargo +publish` ship them inside the `.crate`, making the crate buildable standalone +with no access to the rest of the mxaccessgw repo. Any Contracts proto change +must refresh `clients/rust/protos/` in the same commit; `scripts/check-codegen.ps1` +Check 3 byte-compares the vendored copies against the canonical protos and +fails on drift. Because the vendored protos make a standalone build possible, +`cargo package`/`cargo publish` run **with** verification (no `--no-verify`) — +a `cargo package` that cannot build from the vendored tree alone would mean +the vendored copies are stale, and verification is what catches that before +publish. + Regenerate and compile Rust bindings: ```powershell diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index 13e774f..dbb7f2b 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -186,7 +186,7 @@ events (a "gap") and must re-snapshot; whatever is still retained is replayed. | `MxGateway:Dashboard:SnapshotIntervalMilliseconds` | `1000` | Dashboard snapshot refresh interval used by the snapshot SignalR hub and the pages that subscribe to it. | | `MxGateway:Dashboard:RecentFaultLimit` | `100` | Maximum number of fault summaries projected into each dashboard snapshot. | | `MxGateway:Dashboard:RecentSessionLimit` | `200` | Maximum number of session summaries projected into each dashboard snapshot. | -| `MxGateway:Dashboard:ShowTagValues` | `false` | Reserved display control for tag values. The dashboard does not show full tag values by default. | +| `MxGateway:Dashboard:ShowTagValues` | `false` | Controls whether tag values reach the dashboard's SignalR events hub mirror. `false` (default): `DashboardEventBroadcaster` blanks tag values from a deep-cloned copy of each `MxEvent` before it reaches any hub subscriber — event metadata (tag reference, quality, status, timestamps) still renders; see `docs/GatewayDashboardDesign.md`'s `EventsHub` row for the mechanism. Security-relevant because the per-session hub ACL that would scope a Viewer to specific sessions does not exist yet: with no per-session scoping, this redaction is currently the only thing standing between a low-trust Viewer and other sessions' tag values, so setting this `true` exposes every session's tag values to every authenticated dashboard viewer. The flag gates only the SignalR hub mirror — it does **not** cover the `/browse` live-value display, which remains a separate, still-open residual. | | `MxGateway:Dashboard:GroupToRole` | _(empty)_ | LDAP group → dashboard role mapping. Keys are LDAP group names (short CN or full DN — leading-RDN match). Values must be `Admin` (read/write, API-key CRUD) or `Viewer` (read-only). A user whose LDAP groups don't intersect this map cannot sign in; with no mapping at all, only the loopback bypass admits anyone. | | `MxGateway:Dashboard:DisableLogin` | `false` | Dev/test only. When `true`, replaces the cookie authentication handler with `DashboardAutoLoginAuthenticationHandler`, which auto-authenticates every dashboard request — including requests from remote browsers, not just loopback — as `AutoLoginUser` holding both `Administrator` and `Viewer` roles. No login form, LDAP bind, or cookie is involved. A loud one-time startup warning is logged. Differs from `AllowAnonymousLocalhost`: `DisableLogin` mints a real authenticated principal (so role-gated write affordances appear), whereas `AllowAnonymousLocalhost` satisfies the authorization requirement on loopback only without minting a principal (write affordances stay hidden). Never enable in production. **Production hard-stop (SEC-04):** when the host runs in the `Production` environment and `DisableLogin` is `true`, startup validation fails and the process aborts — the flag is only accepted outside Production, where the one-time startup warning still fires. | | `MxGateway:Dashboard:AutoLoginUser` | `(null)` | Username stamped on the synthetic principal when `DisableLogin` is `true`. Default `(null)` — a null or blank value falls back to `multi-role`. Has no effect when `DisableLogin` is `false`. | diff --git a/docs/Grpc.md b/docs/Grpc.md index fc3b851..271434c 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -82,6 +82,8 @@ return mapper.MapCommandReply(workerReply); Carrying the enqueue timestamp into the worker layer is what lets queue-wait time be measured separately from worker-side execution time when troubleshooting timeouts. +An accepted gRPC command payload can still be too large for the worker pipe: the envelope built around it must fit `MxGateway:Worker:MaxMessageBytes`, which is validated at startup to sit at least a fixed envelope-overhead reserve above `MaxGrpcMessageBytes` (see the headroom rule in [Gateway Configuration](./GatewayConfiguration.md)) so this should not occur for a conformant payload, but if it does, `WorkerClient` raises `WorkerClientException(CommandTooLarge)` and `Invoke` reports `ResourceExhausted` for that command — the session is not faulted, so a client can retry with a smaller payload without reopening the session. + ### `StreamEvents` `StreamEvents` is a server-streaming RPC. The handler delegates the full pipeline to `IEventStreamService` and just forwards each `MxEvent` onto the response stream. Keeping the channel and producer/consumer machinery out of the handler means cancellation, exception mapping, and metric bookkeeping live in one place. @@ -247,7 +249,7 @@ StatusCode statusCode = exception.ErrorCode switch }; ``` -`WorkerClientException` follows the same pattern: `CommandTimeout` becomes `DeadlineExceeded`, `GatewayShutdown` becomes `Cancelled`, `InvalidState` becomes `FailedPrecondition`, `ProtocolViolation` becomes `Internal`, and unmapped codes fall through to `Unavailable`. +`WorkerClientException` follows the same pattern: `CommandTimeout` becomes `DeadlineExceeded`, `GatewayShutdown` becomes `Cancelled`, `InvalidState` becomes `FailedPrecondition`, `ProtocolViolation` becomes `Internal`, `CommandTooLarge` becomes `ResourceExhausted`, and unmapped codes fall through to `Unavailable`. ## Event Streaming Model diff --git a/docs/MxAccessWorkerInstanceDesign.md b/docs/MxAccessWorkerInstanceDesign.md index b67c438..ccc4c42 100644 --- a/docs/MxAccessWorkerInstanceDesign.md +++ b/docs/MxAccessWorkerInstanceDesign.md @@ -638,27 +638,42 @@ Rules: ## Outbound Queues -The worker should use bounded outbound queues for replies, events, heartbeats, -and faults. +`WorkerFrameWriter` is a two-class cooperative priority scheduler, not the +five-level queue an earlier design draft called for. Every outbound frame is +enqueued at one of two `WorkerFrameWritePriority` values: -Priority order when writing: +- **Control** — hello, ready, command replies, faults, heartbeats, shutdown + acknowledgements. +- **Event** — MXAccess events drained from the queue. -1. faults, -2. command replies, -3. shutdown acknowledgements, -4. heartbeats, -5. events. +A caller enqueues its frame under a lock, then contends for a single write +lock; whichever caller wins drains **every** currently-queued frame before +releasing the lock, so a reply, fault, or heartbeat enqueued while a large +event backlog is draining still gets written on the very next drain pass +rather than waiting behind it. Within a class the queue is strict FIFO — a +fault does not jump ahead of an already-queued heartbeat or reply. This +collapses the originally-specified five-level order (faults > replies > +shutdown acks > heartbeats > events) into two classes; the decision is +deliberate, not a shortfall: the control queue is shallow in practice (faults +and shutdown acks are rare, replies and heartbeats are the steady traffic), so +the FIFO delay within it is bounded, and a two-class scheduler is simpler to +reason about and test than a five-level one for that same bound. See +`docs/WorkerFrameProtocol.md`'s "Write scheduling and sequencing" section for +the wire-level detail (sequence stamping, per-frame vs. stream-failure +semantics, flush coalescing). -Event overflow policy defaults to fail-fast for parity testing. If the event -queue fills: - -1. Capture overflow metrics. -2. Send `WorkerFault` if possible. -3. Stop accepting new commands. -4. Let the gateway close or kill the worker. - -Production coalescing may be added later, but it must be explicit and tested. -Do not drop or coalesce events in v1. +Event overflow policy is fail-fast, not "stop accepting new commands and let +the gateway close or kill the worker." When `MxAccessEventQueue.Enqueue` finds +the queue full, it throws and the queue self-records a `WorkerFault` with +category `QueueOverflow`. The event drain loop's next pass observes the fault +through `DrainFault()`, writes the fault frame — a Control-priority frame, so +it is not stuck behind a queued event backlog — and then throws to unwind +`RunAsync`: the worker process exits rather than continuing in a state where +events are being silently lost. The exit currently uses the generic +`WorkerExitCode.UnexpectedFailure` rather than a dedicated overflow code; a +distinct exit code remains open (tracked separately). Do not drop or coalesce +events to avoid this exit — that is explicitly out of scope for v1. Production +coalescing may be added later, but it must be explicit and tested. The gateway side of the event path is bounded to match. `WorkerClient` buffers inbound events in a bounded consumer channel plus a staging channel bounded at diff --git a/docs/WorkerFrameProtocol.md b/docs/WorkerFrameProtocol.md index e03867e..5bf4018 100644 --- a/docs/WorkerFrameProtocol.md +++ b/docs/WorkerFrameProtocol.md @@ -65,6 +65,68 @@ Protocol violations throw `WorkerFrameProtocolException` with a `WorkerFrameProtocolErrorCode` so callers can distinguish malformed frames, oversized frames, protocol version mismatches, and session mismatches. +## Write Scheduling And Sequencing + +This section covers write scheduling (priority classes, enqueue-then-contend, +flush coalescing) and sequencing (write-time stamping) together, because both +are properties of the same single write lock. + +`WorkerFrameWriter` is a two-class cooperative priority scheduler +(`WorkerFrameWritePriority.Control` and `.Event`), not a strict per-kind +priority order. A caller enqueues its frame into the control or event queue +under a lock, then contends for a single write lock; whichever caller wins +drains every frame queued at that moment, control frames first and each class +in FIFO order, so a command reply, fault, heartbeat, or shutdown +acknowledgement is never delayed behind a backlog of queued events. Priority +only reorders *which frame writes next* — it does not affect the sequence +value a frame receives (see below), so a caller cannot infer priority class +from the wire sequence. + +The envelope `Sequence` is stamped by the draining lock-holder at the actual +moment of writing, not when the frame is enqueued, so the on-wire order and +the stamped sequence always agree regardless of caller concurrency or +priority reordering. Stamping uses peek-stamp-commit: a candidate sequence is +assigned and the frame is validated (size, non-empty payload) against that +stamped value, but the counter is committed only immediately before the +stream write. A per-frame rejection therefore leaves the counter untouched — +the next accepted frame reuses the candidate number, so the wire sequence +stays contiguous across rejections and an operator reading a pipe capture +never sees a phantom gap from a rejected frame. + +Two failure shapes are distinguished during a drain pass: + +- **Per-frame rejection** (`InvalidEnvelope`, `MessageTooLarge`, + `ProtocolVersionMismatch`, `SessionMismatch`) is specific to the one frame + that failed validation or sizing. Nothing was written for it, so it fails + only that frame's completion and draining continues with the next queued + frame. +- **Stream failure** (anything else — a broken pipe, an I/O error) means the + underlying stream itself is no longer trustworthy. It fails the frame that + triggered it, every frame already written this batch but not yet flushed, + and every frame still queued, then stops draining entirely so no caller + waits forever on a stream that will not recover. + +Flushes are coalesced across a drained batch: each frame in the batch is +written to the stream without an individual flush, then one `FlushAsync` +runs after the whole batch, and only then does every successfully-written +frame's completion resolve — so a caller's `WriteAsync` still does not +complete until its bytes are both written *and* flushed, but a batch that +happened to contain several queued frames pays one flush instead of one per +frame. In practice this coalescing currently engages only when multiple +frames are queued at the moment a lock-holder starts draining. The event +drain loop (`WorkerPipeSession.RunEventDrainLoopAsync`) awaits each drained +event's `WriteAsync` individually before writing the next, so today at most +one event frame is queued per drain pass and each event still costs its own +flush; a dedicated batch write entry point that submits a whole drained +event batch under one lock acquisition is designed but not yet landed, so a +burst of N events currently costs N flushes on the event hot path, not one. + +Cancellation semantics for a `WriteAsync` call that is still waiting for the +write lock when its token fires are not yet defined at this layer — pending +a fix that will tombstone the queued frame so a cancelled call is guaranteed +never to reach the wire. Until that lands, a cancelled caller may still see +its frame written by whichever caller next holds the lock. + ## Verification The frame protocol lives in `ZB.MOM.WW.MxGateway.Worker.Ipc` (`WorkerFrameReader`, diff --git a/docs/style-guides/JavaStyleGuide.md b/docs/style-guides/JavaStyleGuide.md index b46036e..1e12f48 100644 --- a/docs/style-guides/JavaStyleGuide.md +++ b/docs/style-guides/JavaStyleGuide.md @@ -5,7 +5,9 @@ library, CLI, and tests. ## Baseline -- Target the Java version defined by the client build, with Java 21 preferred. +- Target Java 17 (the Ignition 8.3 baseline; the client build enforces + `options.release = 17` with a Gradle toolchain 17). Code must compile and + run on 17; newer JDKs may host the build. - Use Gradle unless the repository standardizes on Maven. - Apply a formatter such as Spotless or Google Java Format when configured. - Keep generated protobuf code separate from handwritten wrappers.