From f27eb280639fd802191f8d006e788c45a2e1d576 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:15:20 -0400 Subject: [PATCH 1/4] fix(GWC-28): stamp gateway envelope sequence at write, not construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CreateEnvelope stamped Sequence with an interlocked increment when the envelope was built, so two concurrent InvokeAsync callers could 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. Benign today (neither side validates inbound sequence, old GWC-10 still open) but it would fault healthy sessions the moment worker-side validation lands. WriteLoopAsync now stamps immediately before _writer.WriteAsync. It is the outbound channel's single consumer (SingleReader = true), so wire order and stamp order are the same thing by construction and _nextSequence drops to a plain ulong with no interlocking. This mirrors the worker's WRK-04 fix, which the gateway half never received. Also adds TST-28: a [Theory] pinning that GatewayHello.MaxFrameBytes carries the configured worker-frame maximum (default + 2 MiB override). The adoption half is asserted only in the Windows-only worker suite, so a regression to sending 0 — "older gateway, use default" to the worker — would silently downgrade the negotiated IPC-02 limit with every CI test still green. Mutation-checked (hard-coded 0 fails both cases). Tests: WorkerClientTests.ConcurrentInvokesEmitStrictlyIncreasingSequences OnTheWire (32 parallel invokes; failed 3/3 pre-fix) and .StartAsync_SendsGatewayHelloWithConfiguredMaxFrameBytes. --- gateway.md | 10 ++- .../Workers/WorkerClient.cs | 15 +++- .../Gateway/Workers/WorkerClientTests.cs | 70 +++++++++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) 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/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.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] From a044f92c5d212ab37140f330786137447f076f22 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:15:31 -0400 Subject: [PATCH 2/4] fix(GWC-29): drop the wasted request clone on the Invoke hot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Invoke deep-cloned the whole MxCommandRequest — including its command payload, potentially a large bulk-write graph — only to overwrite the cloned command with commandToInvoke and discard it. MapCommand then did the one clone actually needed. Net cost: a full wasted command deep-clone per Invoke, worst for exactly the bulk writes that are largest. Adds a MapCommand(MxCommand) overload (MapCommand reads nothing else off the request) and has Invoke pass commandToInvoke directly; the request overload delegates so other callers are untouched. The remaining clone inside MapCommand stays and is now documented as required rather than incidental: commandToInvoke may be the gRPC-owned request.Command, and the caller reads it again after dispatch via TrackCommandReply, so ownership transfer (à la GWC-07) is not safe here. That clone is what keeps WorkerClient.CreateCommandEnvelope's no-aliasing invariant true. Tests: MxAccessGrpcMapperTests.MapCommandFromCommandClonesPayload (mutating the input leaves the mapped command untouched; both overloads produce equal results under a fixed TimeProvider). --- .../Grpc/MxAccessGatewayService.cs | 9 ++-- .../Grpc/MxAccessGrpcMapper.cs | 20 ++++++++- .../Gateway/Grpc/MxAccessGrpcMapperTests.cs | 43 +++++++++++++++++++ 3 files changed, 68 insertions(+), 4 deletions(-) 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.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() From eeee3e48a3768ed5cde0be336d39085d9da63e28 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:15:39 -0400 Subject: [PATCH 3/4] fix(GWC-30): reuse the frame reader's length-prefix scratch buffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReadAsync allocated a fresh 4-byte array per inbound frame; the GWC-08 pass pooled the payload buffer but left the prefix. Replaced with a per-instance scratch field — the reader is single-consumer by construction (one read loop per WorkerClient, handshake reads complete before the loop starts), so a per-instance buffer is safe and the non-reentrancy that makes it safe is now stated on the class. Pooling four bytes via ArrayPool would cost more than the allocation it saves. Tests: WorkerFrameProtocolTests.ReadAsync_WithMultipleFramesOnOneReader_ ParsesEveryFrame reads five frames of differing payload length through one reader, so a stale prefix carried between calls would misparse. --- .../Workers/WorkerFrameReader.cs | 18 ++++++++-- .../Workers/WorkerFrameProtocolTests.cs | 35 +++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) 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/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] From 404f7cd993690f15170b2f2cbb34599fb567e563 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:15:45 -0400 Subject: [PATCH 4/4] docs(archreview): close GWC-28, GWC-29, GWC-30, TST-28 Flip the four findings to Done in the 2026-07-12 tracking registers (Gateway core + Testing) and in the per-domain registers of 10-gateway-core.md and 60-testing-docs-gaps.md; append the 2026-08-07 change-log row recording what shipped, the pre-fix red for GWC-28, and the TST-28 mutation check. --- archreview/2026-07-12/remediation/00-tracking.md | 9 +++++---- archreview/2026-07-12/remediation/10-gateway-core.md | 6 +++--- .../2026-07-12/remediation/60-testing-docs-gaps.md | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 808f059..876f872 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) | @@ -166,3 +166,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-08-07 | **ReplayGap end-to-end cluster (GWC-25 + CLI-35 + CLI-36) → `Done`** on `fix/gwc-25-replaygap-trio`. GWC-25: `SessionEventDistributor.RegisterWithReplay`'s empty-ring branch now reports `oldestAvailableSequence = _highestSequenceSeen + 1` when `gap == true` (still `0` when no gap), so the universal `oldest - 1` resume formula no longer wraps to `ulong.MaxValue` and dead-stream the subscriber; `docs/Sessions.md` documents the empty-ring value. CLI-35: the Python CLI renders a `ReplayGap` as a `{"replayGap": {...}}` row via a new `_event_row` helper instead of crashing in `MessageToDict`. CLI-36: the Go CLI branches on `result.IsReplayGap()` and prints the typed `REPLAY_GAP requested_after= oldest_available=` line / `replayGap` JSON row instead of formatting the library's cleared `Event`. `docs/CrossLanguageSmokeMatrix.md` gained a per-CLI gap-rendering table (one edit covering both client findings). Four new tests as designed (3 × `SessionEventDistributorTests`, `GatewayEndToEndReconnectReplayTests.ReconnectAfterFullAgeEvictionResumesWithSentinelFormula`) plus `test_stream_events_renders_replay_gap` (Python) and `TestRunStreamEventsPrintsReplayGap` (Go); all written red first and each reproducing its defect verbatim. **Deferred:** GWC-25's `ReplayGap.oldest_available_sequence` proto-comment amendment is **not** in this change — it is comment-only but triggers the full five-client regen fan-out, so it lands with the later codegen wave (alongside IPC-23's proto-comment edits) rather than forcing a regen for one sentence. Note for that wave: the fake-worker gateway e2e suite cannot run on the macOS worktree without `TMPDIR` shortened (macOS caps the Unix-domain-socket path backing .NET named pipes at 104 chars; `TMPDIR=/tmp dotnet test …` works and was used here). | | 2026-08-07 | **GWC-27 → `Done`, GWC-26 → `Done`** (branch `fix/gwc-26-27-alarm-attach`). GWC-27: `GatewaySession.AttachInternalEventSubscriber` now mirrors `AttachEventSubscriber`'s readiness gate under `_syncRoot`, before `EnsureDistributorCreated`, so a premature attach can no longer latch a poisoned distributor. GWC-26: the alarm monitor takes its internal lease directly from the session **before** `SubscribeAlarms` and drains it after the first reconcile; `ISessionManager.ReadAlarmEventsAsync` removed (zero remaining callers); `ApplyReconcile` now broadcasts an `Acknowledge` feed transition for a both-present alarm whose state advanced to `ActiveAcked` (feed-level repair on `AlarmFeedMessage`, not `MxEvent` synthesis). New tests `GatewaySessionTests.AttachInternalEventSubscriberBeforeReadyThrowsAndDoesNotPoisonDistributor` and `GatewayAlarmMonitorAttachOrderTests` (`TransitionsDuringSubscribeWindow_StillReachTheAlarmFeed`, `ApplyReconcileBroadcastsAcknowledgeDelta`); the alarm-monitor fakes now hand the monitor a real Ready `GatewaySession` with a dashboard mirror so the window is actually reproducible. Verification: NonWindows build 0 warnings/0 errors; `GatewayAlarmMonitor` 16 passed, `SessionManagerTests` 38 passed, `GatewaySessionTests` 19 passed, `AlarmFailoverEndToEndTests` 2 passed. | | 2026-08-07 | Code review of `fix/gwc-26-27-alarm-attach` surfaced a **known pre-existing characteristic, now documented**: the alarm monitor's reconcile-derived feed repairs are **at-least-once, not exactly-once**. A reconcile reads the worker's current state while the matching live transition may still be buffered in the monitor's internal lease, so both broadcast and the duplicates are indistinguishable on the alarm feed (`StreamAlarms` + dashboard alarm hub). This pre-dates GWC-26 — the Raise/Clear presence repair has always had it, since nothing serializes a reconcile pass against the in-flight live stream — so closing it (reconcile/live serialization or transition-timestamp dedup) was ruled out of scope for a P2 fix. Documented instead in `GatewayAlarmMonitor.ApplyReconcile`, `gateway.md`, and `docs/Sessions.md`, with the consumer-side contract stated explicitly (apply transitions idempotently — "set this alarm to this state", never increment/toggle). **Candidate finding for the next review cycle.** | +| 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) |