From eeee3e48a3768ed5cde0be336d39085d9da63e28 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Fri, 7 Aug 2026 06:15:39 -0400
Subject: [PATCH] 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;
 
+/// <summary>
+/// Reads length-prefixed WorkerEnvelope protobuf frames from a stream.
+/// </summary>
+/// <remarks>
+///     <see cref="ReadAsync"/> 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 <c>WorkerClient</c>, with handshake reads completing before the
+///     loop starts.
+/// </remarks>
 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)];
+
     /// <summary>
     /// Initializes a new instance of <see cref="WorkerFrameReader"/>.
     /// </summary>
@@ -30,10 +43,9 @@ public sealed class WorkerFrameReader
     /// <returns>Parsed worker envelope.</returns>
     public async ValueTask<WorkerEnvelope> 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);
     }
 
+    /// <summary>
+    ///     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.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [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<WorkerEnvelope> 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);
+        }
+    }
+
     /// <summary>Verifies that reading a frame with partial reads reassembles the frame correctly.</summary>
     /// <returns>A task that represents the asynchronous operation.</returns>
     [Fact]