diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 8ec4789..5353b77 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -63,9 +63,9 @@ Full design + implementation for each row lives in the linked domain doc under i | GWC-25 | Medium | P0 | S | CLI-35/36 (coord) | Done | Empty-ring ReplayGap sentinel carries `oldest_available_sequence = 0` | | GWC-26 | Low | P2 | M | GWC-27 | Done | Alarm monitor attaches its subscriber after SubscribeAlarms; window transitions bypass the feed | | GWC-27 | Low | P2 | S | GWC-26 | Done | `AttachInternalEventSubscriber` bypasses the readiness gate; premature attach poisons the distributor | -| GWC-28 | Low | P2 | S | GWC-10 (coord, old tracker) | Not started | Gateway→worker envelope `sequence` stamped at creation, not at write | -| GWC-29 | Low | — | S | — | Not started | `Invoke` deep-clones the entire request only to discard the cloned command | -| GWC-30 | Info | — | S | — | Not started | Frame reader allocates a fresh 4-byte length-prefix array per frame | +| GWC-28 | Low | P2 | S | GWC-10 (coord, old tracker) | Done | Gateway→worker envelope `sequence` stamped at creation, not at write | +| GWC-29 | Low | — | S | — | Done | `Invoke` deep-clones the entire request only to discard the cloned command | +| GWC-30 | Info | — | S | — | Done | Frame reader allocates a fresh 4-byte length-prefix array per frame | ### Worker — [20-worker.md](20-worker.md) @@ -129,7 +129,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-28 | Low | P2 | S | relates IPC-02 (old) | Not started | Gateway-side `max_frame_bytes` handshake untested in the CI-run suite | +| 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) | @@ -169,3 +169,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-08-07 | **CLI-37 + CLI-38 -> `Done`** (branch `fix/cli-37-38-conformance`), one cross-client conformance commit; **closes old-tracker CLI-08**. Canonical rules landed everywhere: an `MxStatusProxy` entry fails iff `category != MX_STATUS_CATEGORY_OK` (`success` is the raw COM member, diagnostics only; absent entry = success, present entry with `UNSPECIFIED` = failure), and a reply fails on HRESULT iff `hresult` is present and `< 0` (so `S_FALSE = 1` passes). Edits: .NET `MxStatusProxyExtensions.IsSuccess` (drop the `Success != 0` conjunct) + `MxCommandReplyExtensions` (`!= 0` -> `< 0`); Go `StatusSucceeded` (category) + `errors.go` (`< 0`); Java `MxStatuses.succeeded` (category, Javadoc corrected) + `MxGatewayErrors` (`< 0`); Python `errors.py` (category); Rust `ensure_mxaccess_success` (category, doc comment corrected). Four shared fixtures added under `clients/proto/fixtures/behavior/command-replies/` (`write.status-category-{error-success-set,ok-success-zero}.reply.json`, `write.hresult-{s-false,e-fail}.reply.json`) + manifest + `docs/ClientBehaviorFixtures.md`; each of the five suites now runs all four fixture-driven, plus a per-language table test for the two edges fixtures cannot express (nil/null entry, `UNSPECIFIED` category). Docs same commit: `ClientLibrariesDesign.md` per-item rule sentence (its existing HRESULT `< 0` claim is now true), .NET/Go/Java README error sections. Also fixed a Java test fake that built a status with a bare `setSuccess(1)` and no category. Verification: dotnet build 0 warnings + 110 passed/1 skipped; `gofmt -l` clean, `go build ./...`, `go test ./...` all ok; `gradle test` BUILD SUCCESSFUL with **no** generated-file churn to revert this time (no `.proto` changed and `generateProto` stayed up to date); `python -m pytest` 155 passed/1 skipped; `cargo fmt` (no unrelated reformat), `cargo check`, `cargo test --workspace` 100 passed, `cargo clippy --all-targets -- -D warnings` clean. Gateway-side `ClientBehaviorFixtureTests` 8/8 re-run because the new fixtures are validated there. | | 2026-08-07 | **CLI-45 → `Done`** on `fix/cli-45-credential-envvar`. All five CLIs now share one credential contract for `authenticate-user`: flags `--password` / `--password-env` (Go: `-password` / `-password-env`) defaulting to env `MXGATEWAY_VERIFY_PASSWORD`, resolution flag-then-env, and a resolved credential that is missing **or empty** is a usage error naming the flag and the variable — never the value, never sent to the wire. Go and Java previously authenticated with an empty password: Go now returns the guard error before dialing, Java throws a picocli `ParameterException` instead of falling back to `""`. Python's `--password-env` gained the canonical default (its `UsageError` was already conformant) and its message now names the resolved variable. Rust treats an empty `--password` or empty env value as missing (resolution extracted into a testable `resolve_verify_user_password`). .NET adopted the canonical flags and keeps its pre-existing names as **deprecated aliases for one release** — order: `--password`, `--verify-user-password`, the variable named by `--password-env` (or the deprecated `--verify-user-password-env`; default `MXGATEWAY_VERIFY_PASSWORD`), then `MXGATEWAY_VERIFY_USER_PASSWORD`. Tests: `TestRunAuthenticateUser{RejectsEmptyPassword,ReadsPasswordFromCanonicalEnv}` (Go), 3 picocli cases (Java), 3 click cases (Python), 2 clap/resolver cases (Rust), 4 xUnit cases covering the canonical flag, both env-name paths, the deprecated flag+env aliases, and the missing/empty failure (.NET). Docs same commit: `docs/CrossLanguageSmokeMatrix.md` gained a "Credential contract for `authenticate-user`" section **and** the per-CLI subcommand-coverage table — the half of this finding that is documented rather than fixed (.NET exposes all nine single-item session commands; Rust `unregister` + the credential pair; Go/Python/Java the credential pair only; verified against each dispatch table, and every gap is CLI surface only since all five *libraries* implement all nine helpers). All five client READMEs name the canonical variable and the fail-fast rule; the .NET README gained an `authenticate-user` credentials section carrying the deprecation note. **Deviation:** Java keeps `isBlank()` (per this design's "null or blank" wording for Java) where the other four test emptiness, so a whitespace-only credential is additionally rejected there. Verification (all five, on macOS): Go `gofmt -l .` clean, `go build ./...` clean, `go test ./...` ok; Java `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` BUILD SUCCESSFUL, CLI suite 51 tests / 0 failures — **no generated-tree churn appeared this run**, `git status` for `clients/java/**/generated` clean with no revert needed (no `.proto` changed); Python `python -m pytest` 148 passed / 1 skipped (TLS opt-in); .NET `dotnet build …Client.slnx` 0 warnings / 0 errors and client tests 108 passed / 1 skipped (live-gateway opt-in); Rust `cargo fmt` (diff confined to the new code), `cargo check --workspace`, `cargo test --workspace` 100 tests across 6 targets all green, `cargo clippy --all-targets -- -D warnings` clean. | | 2026-08-07 | **SEC-31 + SEC-32 → `Done`** (branch `fix/sec-31-32-limiter`, one change set as planned). `ApiKeyFailureLimiter` reworked from `IsBlocked/RecordFailure/Reset(string peer)` to a partition-pair API (`Check/RecordFailure/Reset(ApiKeyThrottlePartition)` returning `ApiKeyThrottleDecision`): layer 1 is the composite `(transport peer, key id)` partition, layer 2 a per-key-id aggregate across peers (`ApiKeyFailureAggregateLimit`, default 30), and an over-limit state now admits one probe per `ApiKeyFailureProbeIntervalSeconds` (default 5) instead of blocking absolutely — so a success can reset the state while throttled, killing the 10-packets-per-minute lockout. SEC-32 rides along: the interceptor validates token shape (`mxgw` prefix, ≥3 non-empty `_` segments, key id ≤ 64 chars) before minting a key-id partition, each peer may mint at most 32 of them (overflow collapses to its fallback partition), and eviction prefers expired windows, never dropping an over-limit partition below a 2× transient overshoot ceiling. New counter `mxgateway.auth.throttled` tagged `stage=peer\|aggregate` only (no key material — `/metrics` is still unauthenticated per open SEC-14). Docs updated in the same commit (`docs/GatewayConfiguration.md` limiter rows + two new keys, `docs/Authentication.md` hot-path paragraph, `docs/Authorization.md` SEC-11 section, limiter/`SecurityOptions` XML remarks). Evidence: `dotnet build …Server` clean; `--filter ~ApiKeyFailureLimiter` 11/11 passed (new `ApiKeyFailureLimiterTests`), `--filter ~GatewayGrpcAuthorizationInterceptor` 20/20 passed (incl. the four SEC-31 contract tests and `NonMxgwToken_FallsBackToTransportPeerPartition`), `--filter ~GatewayOptionsValidator` 66/66 passed. Full suite on macOS: 804 passed / 44 failed — all 44 are the pre-existing named-pipe fake-worker classes (`WorkerClientTests`, `FakeWorkerHarnessTests`, `SessionWorkerClientFactoryFakeWorkerTests`, `GatewayEndToEnd*`), verified identical (44) on the unmodified tree. Follow-up unchanged: the new `MxGateway:Security` keys belong in old **SEC-24**'s effective-config projection when that is picked up. **Code review of the branch found two defects in the first pass, both fixed before merge:** (1) probe admission was check-then-act across two lock scopes, so a burst arriving at an interval boundary could all observe "due" and all be admitted — the claim is now a single critical section (`TryConsumeProbe`), and because the two layers are claimed one at a time, a slot claimed on the partition is compensated (`ReleaseProbe`) when the aggregate then refuses; (2) `Reset` on a success whose key id had been collapsed into the address's shared fallback partition removed that shared partition, letting one authentication wipe an in-progress spray from the same address — it is now left to decay by window expiry, while the key's aggregate is still cleared. Tests added: `ProbeAdmission_UnderConcurrentArrivals_GrantsExactlyOneSlot`, `ProbeAdmission_WhenAggregateRefuses_ReturnsTheClaimedPeerSlot`, `Reset_WithOverCapKeyId_DoesNotClearSharedFallbackPartition`. **A second review pass found a residual defect in that compensation path:** the release identified its own reservation by comparing `NextProbeAtTicks` to `now + interval`, the identical expression a failure re-arm writes — so a concurrent `RecordFailure` on the same state sharing a clock tick (routine at ~1 ms resolution) was mistaken for the caller's own claim and stomped back to the stale, already-due value, prematurely reopening the probe slot. Replaced with a monotonic per-state `ProbeVersion` bumped by every writer of `NextProbeAtTicks` (claim and re-arm alike); the release restores only when the version still matches the one its claim stamped, and bumps it again on restore so no other stale release can match. Covered by `ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick`, made deterministic by a new `internal ProbeReleaseInterleaveHook` test seam (null in production, one null check on the refused path) because the claim-to-release window is nanoseconds wide and racing threads cannot hit it reliably — verified as a genuine red against the timestamp guard (`Expected: ThrottledByPeer / Actual: ProbeAdmitted`). Limiter suite 11 → 15. | +| 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). | diff --git a/archreview/2026-07-12/remediation/10-gateway-core.md b/archreview/2026-07-12/remediation/10-gateway-core.md index e6a12a5..f35710c 100644 --- a/archreview/2026-07-12/remediation/10-gateway-core.md +++ b/archreview/2026-07-12/remediation/10-gateway-core.md @@ -12,9 +12,9 @@ This document turns the 2026-07-12 re-review's **new** Gateway Server Core findi | GWC-25 | Medium | P0 | S | CLI-35/36 (coord) | Done | Empty-ring ReplayGap sentinel carries `oldest_available_sequence = 0`, dead-streaming a compliant client | | GWC-26 | Low | P2 | M | GWC-27 | Done | Alarm monitor attaches its subscriber after SubscribeAlarms; window transitions bypass the feed, missed Acknowledge never repaired | | GWC-27 | Low | P2 | S | GWC-26 | Done | `AttachInternalEventSubscriber` bypasses the readiness gate; premature attach poisons the distributor permanently | -| GWC-28 | Low | P2 | S | GWC-10 (coord) | Not started | Gateway→worker envelope `sequence` stamped at creation, not at write — non-monotonic on the wire under concurrent invokes | -| GWC-29 | Low | — | S | — | Not started | `Invoke` deep-clones the entire request only to discard the cloned command | -| GWC-30 | Info | — | S | — | Not started | Frame reader allocates a fresh 4-byte length-prefix array per frame | +| GWC-28 | Low | P2 | S | GWC-10 (coord) | Done | Gateway→worker envelope `sequence` stamped at creation, not at write — non-monotonic on the wire under concurrent invokes | +| GWC-29 | Low | — | S | — | Done | `Invoke` deep-clones the entire request only to discard the cloned command | +| GWC-30 | Info | — | S | — | Done | Frame reader allocates a fresh 4-byte length-prefix array per frame | Dependency notes: GWC-26 and GWC-27 both change the internal-subscriber attach path (`GatewaySession.AttachInternalEventSubscriber` and its `SessionManager`/alarm-monitor callers) — land GWC-27's readiness gate first (or in the same commit), then GWC-26's reorder, so the reordered monitor attach is proven against the gate. GWC-24 is the direct successor of the prior cycle's GWC-04 backpressure fix and raises the value of the still-open GWC-21 (making `EventChannelFullModeTimeout` configurable); GWC-28 is the gateway half of the worker's WRK-04 fix and must be coordinated with the still-open GWC-10 if inbound sequence enforcement is ever added. GWC-25 is server-complete on its own, but the end-to-end reconnect story also needs the client-domain CLI-35/36 fixes (Python CLI crashes on the sentinel, Go CLI destroys it). 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 3f47234..7f4e23f 100644 --- a/archreview/2026-07-12/remediation/60-testing-docs-gaps.md +++ b/archreview/2026-07-12/remediation/60-testing-docs-gaps.md @@ -13,7 +13,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-28 | Low | P2 | S | relates IPC-02 | Not started | Gateway-side `max_frame_bytes` handshake field untested in the CI-run suite | +| 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/gateway.md b/gateway.md index 1c15cd1..4280be2 100644 --- a/gateway.md +++ b/gateway.md @@ -348,9 +348,13 @@ messages, tagged from 10 upward: Rules: -- `sequence` is a monotonic per-sender counter used as a diagnostic aid; it is - not validated for gaps or ordering on receive (the named pipe already - guarantees FIFO delivery). +- `sequence` is a monotonic per-sender counter used as a diagnostic aid. Both + sides stamp it at the point of writing, inside their single write path — the + gateway on its outbound-channel write loop, the worker on its own writer — so + the numbers stay monotonic in wire order no matter how concurrent callers + interleave while building envelopes. It is not validated for gaps or ordering + on receive (the named pipe already guarantees FIFO delivery); if inbound + enforcement is ever added, this is the property it will rely on. - `correlation_id` links a command to its reply; it is authoritative on the envelope, and the inner `MxCommandReply.correlation_id` echoes it for MXAccess parity. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index 7e756fa..691bc8a 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -116,9 +116,12 @@ public sealed class MxAccessGatewayService( return bulkConstraintPlan.CreateDeniedReply(request); } - MxCommandRequest invokeRequest = request.Clone(); - invokeRequest.Command = commandToInvoke; - WorkerCommand workerCommand = mapper.MapCommand(invokeRequest); + // Map from the command alone: cloning the whole request only to overwrite its command with + // commandToInvoke deep-cloned the (potentially large) original payload for nothing, since + // MapCommand reads nothing but the command (GWC-29). The one clone that matters still + // happens inside MapCommand, which is what keeps the worker-bound graph unaliased from + // commandToInvoke — the caller still reads it below via TrackCommandReply. + WorkerCommand workerCommand = mapper.MapCommand(commandToInvoke); WorkerCommandReply workerReply = await sessionManager .InvokeAsync(request.SessionId, workerCommand, context.CancellationToken) .ConfigureAwait(false); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGrpcMapper.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGrpcMapper.cs index ea6d0f7..fa5e254 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGrpcMapper.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGrpcMapper.cs @@ -29,9 +29,27 @@ public sealed class MxAccessGrpcMapper ArgumentNullException.ThrowIfNull(request); ArgumentNullException.ThrowIfNull(request.Command); + return MapCommand(request.Command); + } + + /// + /// Maps a gRPC MX command to a worker command. Callers that already hold the command — including + /// the constraint pipeline, whose rewritten command is not the one on the request — use this + /// overload rather than cloning a whole request to carry a single field (GWC-29); nothing outside + /// the command is read here. + /// + /// Command payload. + /// The mapped ready for worker dispatch. + public WorkerCommand MapCommand(MxCommand command) + { + ArgumentNullException.ThrowIfNull(command); + + // The clone is required and must stay: the caller may hand us the gRPC-owned request command, + // and the caller keeps reading it after dispatch (TrackCommandReply). Cloning here is what makes + // WorkerClient.CreateCommandEnvelope's no-aliasing invariant true. return new WorkerCommand { - Command = request.Command.Clone(), + Command = command.Clone(), EnqueueTimestamp = Timestamp.FromDateTimeOffset(_timeProvider.GetUtcNow()), }; } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs index 207574e..3ad808c 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs @@ -40,7 +40,9 @@ public sealed class WorkerClient : IWorkerClient private readonly ConcurrentDictionary _pendingCommands = new(StringComparer.Ordinal); private readonly SemaphoreSlim _pendingCommandSlots; private readonly CancellationTokenSource _stopCts = new(); - private long _nextSequence; + // Touched only by WriteLoopAsync — the single consumer of _outboundEnvelopes — so it needs no + // interlocking. See WriteLoopAsync for why the stamp happens there rather than at construction. + private ulong _nextSequence; private WorkerClientState _state; private DateTimeOffset _lastHeartbeatAt; private int? _processId; @@ -404,6 +406,13 @@ public sealed class WorkerClient : IWorkerClient { await foreach (WorkerEnvelope envelope in _outboundEnvelopes.Reader.ReadAllAsync(_stopCts.Token).ConfigureAwait(false)) { + // GWC-28: stamp the sequence at the point of writing, not when the envelope is built. + // Stamping at construction let two concurrent InvokeAsync callers take 1 and 2 and then + // enqueue in the order 2, 1 — non-monotonic on the wire, breaking gateway.md's + // "monotonic per sender" contract. This loop is the channel's single consumer + // (SingleReader = true), so wire order and stamp order are the same thing here and + // _nextSequence needs no interlocking. Mirrors the worker's WRK-04 fix. + envelope.Sequence = unchecked(++_nextSequence); await _writer.WriteAsync(envelope, _stopCts.Token).ConfigureAwait(false); } } @@ -1072,11 +1081,13 @@ public sealed class WorkerClient : IWorkerClient string correlationId, Action setBody) { + // Sequence is deliberately left unset here: WriteLoopAsync stamps it immediately before the + // frame goes out, so the numbers are monotonic in wire order however the callers interleave + // between construction and enqueue (GWC-28, mirroring the worker's WRK-04 fix). WorkerEnvelope envelope = new() { ProtocolVersion = _connection.FrameOptions.ProtocolVersion, SessionId = SessionId, - Sequence = (ulong)Interlocked.Increment(ref _nextSequence), CorrelationId = correlationId, }; setBody(envelope); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerFrameReader.cs b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerFrameReader.cs index 95cbe13..1c79432 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerFrameReader.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerFrameReader.cs @@ -5,11 +5,24 @@ using ZB.MOM.WW.MxGateway.Contracts.Proto; namespace ZB.MOM.WW.MxGateway.Server.Workers; +/// +/// Reads length-prefixed WorkerEnvelope protobuf frames from a stream. +/// +/// +/// is not reentrant: the reader keeps a per-instance length-prefix scratch +/// buffer, so exactly one consumer may be inside a read at a time. That matches how the reader is +/// used — a single read loop per WorkerClient, with handshake reads completing before the +/// loop starts. +/// public sealed class WorkerFrameReader { private readonly WorkerFrameProtocolOptions _options; private readonly Stream _stream; + // Reused across frames rather than allocated per read (GWC-30). Safe because ReadAsync is + // single-consumer by construction; the prefix is fully overwritten by every read. + private readonly byte[] _lengthPrefix = new byte[sizeof(uint)]; + /// /// Initializes a new instance of . /// @@ -30,10 +43,9 @@ public sealed class WorkerFrameReader /// Parsed worker envelope. public async ValueTask ReadAsync(CancellationToken cancellationToken = default) { - byte[] lengthPrefix = new byte[sizeof(uint)]; - await ReadExactlyOrThrowAsync(lengthPrefix, cancellationToken).ConfigureAwait(false); + await ReadExactlyOrThrowAsync(_lengthPrefix, cancellationToken).ConfigureAwait(false); - uint payloadLength = BinaryPrimitives.ReadUInt32LittleEndian(lengthPrefix); + uint payloadLength = BinaryPrimitives.ReadUInt32LittleEndian(_lengthPrefix); if (payloadLength == 0) { throw new WorkerFrameProtocolException( diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs index b7249ba..4bda8ae 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGrpcMapperTests.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Time.Testing; using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.MxGateway.Server.Grpc; @@ -38,6 +39,48 @@ public sealed class MxAccessGrpcMapperTests Assert.NotNull(workerCommand.EnqueueTimestamp); } + /// + /// The command-only overload exists so Invoke does not deep-clone the whole request just to + /// overwrite and discard its command (GWC-29). It must still perform the one clone that keeps the + /// worker-bound graph unaliased from the caller-owned gRPC command, and must produce the same + /// as the request overload. + /// + [Fact] + public void MapCommandFromCommandClonesPayload() + { + FakeTimeProvider timeProvider = new(new DateTimeOffset(2026, 8, 7, 12, 0, 0, TimeSpan.Zero)); + MxAccessGrpcMapper mapper = new(timeProvider); + MxCommand command = new() + { + Kind = MxCommandKind.Write, + Write = new WriteCommand + { + ServerHandle = 10, + ItemHandle = 20, + UserId = 30, + Value = new MxValue + { + DataType = MxDataType.String, + StringValue = "value", + }, + }, + }; + MxCommandRequest request = new() + { + SessionId = "session-1", + Command = command.Clone(), + }; + + WorkerCommand fromCommand = mapper.MapCommand(command); + WorkerCommand fromRequest = mapper.MapCommand(request); + command.Write.Value.StringValue = "changed"; + + Assert.Equal(MxCommandKind.Write, fromCommand.Command.Kind); + Assert.Equal("value", fromCommand.Command.Write.Value.StringValue); + Assert.NotNull(fromCommand.EnqueueTimestamp); + Assert.Equal(fromRequest, fromCommand); + } + /// Verifies that command reply mapping preserves HRESULT and status information. [Fact] public void MapCommandReply_PreservesHresultStatusesAndPayload() diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs index 53a0867..3be1668 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerClientTests.cs @@ -4,6 +4,7 @@ using ZB.MOM.WW.MxGateway.Contracts; using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Workers; +using ZB.MOM.WW.MxGateway.Tests.Gateway.Workers.Fakes; using ZB.MOM.WW.MxGateway.Tests.TestSupport; namespace ZB.MOM.WW.MxGateway.Tests.Gateway.Workers; @@ -29,6 +30,33 @@ public sealed class WorkerClientTests Assert.Equal(WorkerProcessId, client.ProcessId); } + /// + /// The GatewayHello carries the negotiated worker-frame maximum so the worker adopts the + /// configured limit instead of its own default (IPC-02); a regression to sending 0 means + /// "older gateway, use default" to the worker and would silently downgrade the negotiated limit. + /// The adoption half is asserted only in the Windows-only worker suite, so the gateway half is + /// pinned here, in the portable suite (TST-28). Both the default and an override are covered so + /// the assertion tracks configuration rather than a constant. + /// + /// Configured worker-frame maximum to negotiate. + /// A task that represents the asynchronous operation. + [Theory] + [InlineData(WorkerFrameProtocolOptions.DefaultMaxMessageBytes)] + [InlineData(2 * 1024 * 1024)] + public async Task StartAsync_SendsGatewayHelloWithConfiguredMaxFrameBytes(int maxMessageBytes) + { + await using FakeWorkerHarness harness = + await FakeWorkerHarness.CreateConnectedPairAsync(maxMessageBytes: maxMessageBytes); + await using WorkerClient client = harness.CreateClient(); + + Task startTask = client.StartAsync(CancellationToken.None); + WorkerEnvelope gatewayHello = await harness.CompleteStartupAsync().WaitAsync(TestTimeout); + await startTask.WaitAsync(TestTimeout); + + Assert.NotEqual(0u, gatewayHello.GatewayHello.MaxFrameBytes); + Assert.Equal((uint)maxMessageBytes, gatewayHello.GatewayHello.MaxFrameBytes); + } + /// Verifies that InvokeAsync completes a pending command when a matching reply arrives. /// A task that represents the asynchronous operation. [Fact] @@ -141,6 +169,48 @@ public sealed class WorkerClientTests Assert.Equal(MxCommandKind.GetWorkerInfo, reply.Reply.Kind); } + /// + /// The envelope sequence is a monotonic per-sender counter (gateway.md), so the values + /// observed on the pipe must be strictly increasing in wire order. Stamping the sequence when + /// the envelope is constructed lets two concurrent invokes stamp 1 and 2 but enqueue 2 then 1 + /// (GWC-28); stamping on the single-consumer write loop makes wire order and sequence order the + /// same thing by construction. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire() + { + const int commandCount = 32; + await using PipePair pipePair = await PipePair.CreateAsync(); + await using WorkerClient client = CreateClient(pipePair); + await CompleteHandshakeAsync(client, pipePair); + + Task[] invokeTasks = Enumerable.Range(0, commandCount) + .Select(_ => Task.Run(async () => await client.InvokeAsync( + CreateCommand(MxCommandKind.Ping), + TestTimeout, + CancellationToken.None))) + .ToArray(); + + ulong previousSequence = 0; + for (int index = 0; index < commandCount; index++) + { + WorkerEnvelope commandEnvelope = await pipePair.WorkerReader.ReadAsync().AsTask().WaitAsync(TestTimeout); + Assert.Equal(WorkerEnvelope.BodyOneofCase.WorkerCommand, commandEnvelope.BodyCase); + Assert.True( + commandEnvelope.Sequence > previousSequence, + $"Command {index} arrived with sequence {commandEnvelope.Sequence} after {previousSequence}; " + + "envelope sequences must be strictly increasing in wire order."); + previousSequence = commandEnvelope.Sequence; + + await pipePair.WorkerWriter.WriteAsync( + CreateCommandReplyEnvelope(commandEnvelope.CorrelationId, MxCommandKind.Ping)); + } + + await Task.WhenAll(invokeTasks).WaitAsync(TestTimeout); + Assert.Equal(WorkerClientState.Ready, client.State); + } + /// Verifies that ReadEventsAsync yields events in pipe order from the worker. /// A task that represents the asynchronous operation. [Fact] diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerFrameProtocolTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerFrameProtocolTests.cs index 910e77c..6663b0f 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerFrameProtocolTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Workers/WorkerFrameProtocolTests.cs @@ -55,6 +55,41 @@ public sealed class WorkerFrameProtocolTests Assert.Equal(original, parsed); } + /// + /// One reader instance reads many frames in sequence. The reader reuses a single length-prefix + /// scratch buffer across calls (GWC-30), so varying the payload length frame to frame proves the + /// reused buffer is fully overwritten each time rather than carrying a stale prefix forward. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task ReadAsync_WithMultipleFramesOnOneReader_ParsesEveryFrame() + { + const int frameCount = 5; + WorkerFrameProtocolOptions options = new(SessionId); + await using MemoryStream stream = new(); + WorkerFrameWriter writer = new(stream, options); + + List originals = []; + for (int index = 1; index <= frameCount; index++) + { + WorkerEnvelope envelope = CreateEnvelope(); + envelope.Sequence = (ulong)index; + + // Differing payload lengths so a stale length prefix would misparse rather than pass. + envelope.WorkerHello.WorkerVersion = new string('v', index * 37); + originals.Add(envelope); + await writer.WriteAsync(envelope); + } + + stream.Position = 0; + WorkerFrameReader reader = new(stream, options); + foreach (WorkerEnvelope original in originals) + { + WorkerEnvelope parsed = await reader.ReadAsync(); + Assert.Equal(original, parsed); + } + } + /// Verifies that reading a frame with partial reads reassembles the frame correctly. /// A task that represents the asynchronous operation. [Fact]