fix(modbus): classify per-op timeout + framing violations connection-fatal (05/STAB-3)

2026-07-13 11:37:13 -04:00
parent 141c143586
commit 98f4860103
3 changed files with 113 additions and 21 deletions
@@ -12,7 +12,7 @@
    {
      "id": "B1.2",
      "subject": "B1: ModbusTransportDesyncException + teardown-before-propagate in SendOnceAsync (timeout-OCE + InvalidDataException) \u2014 STAB-3",
-      "status": "pending",
+      "status": "completed",
      "blockedBy": [
        "B1.1"
      ]
@@ -207,6 +207,27 @@ public sealed class ModbusTcpTransport : IModbusTransport
        }
    }

+    /// <summary>
+    ///     Executes exactly one Modbus transaction on the current socket.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         A per-op deadline (linked <see cref="CancellationTokenSource.CancelAfter(TimeSpan)"/>)
+    ///         and framing violations (TxId mismatch / truncated MBAP length) are treated as
+    ///         <b>connection-fatal</b>: the socket may have a half-read or still-in-flight response,
+    ///         so reusing it would read stale bytes and desynchronize the stream permanently. Such
+    ///         failures tear the socket down here (before propagating) and surface as
+    ///         <see cref="ModbusTransportDesyncException"/> — which, being an
+    ///         <see cref="IOException"/>, feeds <see cref="SendAsync"/>'s existing single reconnect
+    ///         retry with no extra plumbing.
+    ///     </para>
+    ///     <para>
+    ///         The timeout-vs-caller-cancel distinction is load-bearing: a <b>caller</b> cancellation
+    ///         (<c>ct.IsCancellationRequested</c>) is a legitimate shutdown and propagates as a plain
+    ///         <see cref="OperationCanceledException"/> with <b>no</b> teardown — only the linked
+    ///         per-op timeout (the caller token is NOT cancelled) is a desync.
+    ///     </para>
+    /// </remarks>
    private async Task<byte[]> SendOnceAsync(byte unitId, byte[] pdu, CancellationToken ct)
    {
        if (_stream is null) throw new InvalidOperationException("Transport not connected");
@@ -225,28 +246,53 @@ public sealed class ModbusTcpTransport : IModbusTransport

        using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct);
        cts.CancelAfter(_timeout);
-        await _stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false);
-        await _stream.FlushAsync(cts.Token).ConfigureAwait(false);
-
-        var header = new byte[7];
-        await ReadExactlyAsync(_stream, header, cts.Token).ConfigureAwait(false);
-        var respTxId = (ushort)((header[0] << 8) | header[1]);
-        if (respTxId != txId)
-            throw new InvalidDataException($"Modbus TxId mismatch: expected {txId} got {respTxId}");
-        var respLen = (ushort)((header[4] << 8) | header[5]);
-        if (respLen < 1) throw new InvalidDataException($"Modbus response length too small: {respLen}");
-        var respPdu = new byte[respLen - 1];
-        await ReadExactlyAsync(_stream, respPdu, cts.Token).ConfigureAwait(false);
-
-        // Exception PDU: function code has high bit set.
-        if ((respPdu[0] & 0x80) != 0)
+        try
        {
-            var fc = (byte)(respPdu[0] & 0x7F);
-            var ex = respPdu[1];
-            throw new ModbusException(fc, ex, $"Modbus exception fc={fc} code={ex}");
-        }
+            await _stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false);
+            await _stream.FlushAsync(cts.Token).ConfigureAwait(false);

-        return respPdu;
+            var header = new byte[7];
+            await ReadExactlyAsync(_stream, header, cts.Token).ConfigureAwait(false);
+            var respTxId = (ushort)((header[0] << 8) | header[1]);
+            if (respTxId != txId)
+                throw new ModbusTransportDesyncException(
+                    ModbusTransportDesyncException.DesyncReason.TxIdMismatch,
+                    $"Modbus TxId mismatch: expected {txId} got {respTxId}");
+            var respLen = (ushort)((header[4] << 8) | header[5]);
+            if (respLen < 1)
+                throw new ModbusTransportDesyncException(
+                    ModbusTransportDesyncException.DesyncReason.TruncatedFrame,
+                    $"Modbus response length too small: {respLen}");
+            var respPdu = new byte[respLen - 1];
+            await ReadExactlyAsync(_stream, respPdu, cts.Token).ConfigureAwait(false);
+
+            // Exception PDU: function code has high bit set. This is a well-formed protocol-level
+            // error — the socket is still coherent, so it MUST propagate without teardown.
+            if ((respPdu[0] & 0x80) != 0)
+            {
+                var fc = (byte)(respPdu[0] & 0x7F);
+                var ex = respPdu[1];
+                throw new ModbusException(fc, ex, $"Modbus exception fc={fc} code={ex}");
+            }
+
+            return respPdu;
+        }
+        catch (ModbusTransportDesyncException)
+        {
+            // Framing violation: the socket is desynchronized — never reuse it.
+            await TearDownAsync().ConfigureAwait(false);
+            throw;
+        }
+        catch (OperationCanceledException) when (!ct.IsCancellationRequested)
+        {
+            // Per-op timeout fired (NOT the caller). The response is unknown / may still land later
+            // and corrupt the next read — tear the socket down and normalize as a desync so the
+            // reconnect retry / status mapping engages.
+            await TearDownAsync().ConfigureAwait(false);
+            throw new ModbusTransportDesyncException(
+                ModbusTransportDesyncException.DesyncReason.Timeout,
+                $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms");
+        }
    }

    /// <summary>
@@ -0,0 +1,46 @@
+namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus;
+
+/// <summary>
+///     Raised when a Modbus TCP transaction leaves the single-flight socket in an unknown /
+///     desynchronized state — a per-op response timeout, or a framing violation (transaction-id
+///     mismatch or truncated MBAP length). Unlike a Modbus <em>exception PDU</em> (a well-formed
+///     protocol-level error where the socket is still coherent and the request must simply
+///     propagate), a desync means bytes may still be in flight or half-read, so the socket is
+///     unusable and MUST be torn down before this is thrown.
+/// </summary>
+/// <remarks>
+///     Derives from <see cref="IOException"/> deliberately: the transport's existing
+///     reconnect-and-retry classifier (<c>IsSocketLevelFailure</c>) already treats
+///     <see cref="IOException"/> as socket-fatal, so with auto-reconnect enabled the transport
+///     transparently reconnects and resends once; with auto-reconnect disabled the (already torn
+///     down) socket is never reused and this surfaces to the driver's status mapping as a
+///     communication error.
+/// </remarks>
+public sealed class ModbusTransportDesyncException : IOException
+{
+    /// <summary>The reason the socket was classified desynchronized.</summary>
+    public enum DesyncReason
+    {
+        /// <summary>The per-op deadline (linked <c>CancelAfter</c>) fired — the response never arrived in time.</summary>
+        Timeout,
+
+        /// <summary>The response MBAP transaction id did not match the request's.</summary>
+        TxIdMismatch,
+
+        /// <summary>The response MBAP length field was below the mandatory minimum (truncated frame).</summary>
+        TruncatedFrame,
+    }
+
+    /// <summary>Gets the reason the socket was classified desynchronized.</summary>
+    public DesyncReason Reason { get; }
+
+    /// <summary>Initializes a new instance of the <see cref="ModbusTransportDesyncException"/> class.</summary>
+    /// <param name="reason">The desync reason.</param>
+    /// <param name="message">A human-readable description.</param>
+    /// <param name="inner">The originating exception, if any.</param>
+    public ModbusTransportDesyncException(DesyncReason reason, string message, Exception? inner = null)
+        : base(message, inner)
+    {
+        Reason = reason;
+    }
+}