Merge pull request 'fix(s7): bound async wire ops with a wall-clock deadline (R2-01 read leg)' (#453) from fix/r2-01-s7-read-wallclock-timeout into master

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs

@@ -87,6 +87,7 @@ public interface IS7PlcFactory
 internal sealed class S7PlcAdapter : IS7Plc
 {
     private readonly Plc _plc;
+    private readonly TimeSpan _operationTimeout;
 
     /// <summary>Initializes a new instance of the <see cref="S7PlcAdapter"/> class.</summary>
     /// <param name="cpuType">The CPU family.</param>
@@ -98,9 +99,14 @@ internal sealed class S7PlcAdapter : IS7Plc
     public S7PlcAdapter(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout)
     {
         _plc = new Plc(S7CpuTypeMap.ToS7Net(cpuType), host, port, rack, slot);
+        _operationTimeout = timeout;
         // S7netplus writes timeouts into the underlying TcpClient via ReadTimeout / WriteTimeout
         // (milliseconds). Set before OpenAsync so the handshake itself honours the bound — matches
-        // the original inline construction in S7Driver.InitializeAsync.
+        // the original inline construction in S7Driver.InitializeAsync. NOTE: these map to the
+        // TcpClient's ReceiveTimeout / SendTimeout, which govern only SYNCHRONOUS socket calls; the
+        // async read/write paths S7.Net actually uses ignore them, so every data-plane op below is
+        // additionally bounded by S7OperationDeadline (a real wall-clock ceiling). OpenAsync is left
+        // to S7Driver.EnsureConnectedAsync's own CancelAfter, so it is not double-bounded here.
         var ms = (int)timeout.TotalMilliseconds;
         _plc.ReadTimeout = ms;
         _plc.WriteTimeout = ms;
@@ -117,23 +123,30 @@ internal sealed class S7PlcAdapter : IS7Plc
 
     /// <inheritdoc />
     public Task<object?> ReadAsync(string address, CancellationToken cancellationToken) =>
-        _plc.ReadAsync(address, cancellationToken)!;
+        S7OperationDeadline.RunAsync(
+            t => _plc.ReadAsync(address, t)!, _operationTimeout, $"read of '{address}'", cancellationToken);
 
     /// <inheritdoc />
     public Task<byte[]> ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) =>
-        _plc.ReadBytesAsync(area, db, startByteAdr, count, cancellationToken);
+        S7OperationDeadline.RunAsync(
+            t => _plc.ReadBytesAsync(area, db, startByteAdr, count, t),
+            _operationTimeout, $"block read of {area} db{db} @{startByteAdr}+{count}", cancellationToken);
 
     /// <inheritdoc />
     public Task WriteAsync(string address, object value, CancellationToken cancellationToken) =>
-        _plc.WriteAsync(address, value, cancellationToken);
+        S7OperationDeadline.RunAsync(
+            t => _plc.WriteAsync(address, value, t), _operationTimeout, $"write of '{address}'", cancellationToken);
 
     /// <inheritdoc />
     public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) =>
-        _plc.WriteBytesAsync(area, db, startByteAdr, value, cancellationToken);
+        S7OperationDeadline.RunAsync(
+            t => _plc.WriteBytesAsync(area, db, startByteAdr, value, t),
+            _operationTimeout, $"block write of {area} db{db} @{startByteAdr}", cancellationToken);
 
     /// <inheritdoc />
-    public async Task ReadStatusAsync(CancellationToken cancellationToken) =>
+    public Task ReadStatusAsync(CancellationToken cancellationToken) =>
-        _ = await _plc.ReadStatusAsync(cancellationToken).ConfigureAwait(false);
+        S7OperationDeadline.RunAsync(
+            t => _plc.ReadStatusAsync(t), _operationTimeout, "status read", cancellationToken);
 
     /// <inheritdoc />
     // Plc implements IDisposable explicitly, so cast to invoke it; disposal tears down the TcpClient.

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs

@@ -1287,7 +1287,9 @@ public sealed class S7Driver
     ///     True when <paramref name="ex"/> (or any inner exception) is a socket-level / connection
     ///     loss OR an ISO-on-TCP framing/desync fault that a reopen can repair — a
     ///     <see cref="System.Net.Sockets.SocketException"/> / <see cref="System.IO.IOException"/> /
-    ///     <see cref="ObjectDisposedException"/>; an S7.Net framing violation
+    ///     <see cref="ObjectDisposedException"/>; a <see cref="TimeoutException"/> from a wire op that
+    ///     blew its <see cref="S7OperationDeadline"/> wall-clock ceiling (an unresponsive but still
+    ///     TCP-established peer — the READ-leg sibling of the connect-timeout fix, STAB-14); an S7.Net framing violation
     ///     (<see cref="TPKTInvalidException"/> / <see cref="TPDUInvalidException"/> /
     ///     <see cref="WrongNumberOfBytesException"/>); or a <see cref="PlcException"/> carrying
     ///     <see cref="ErrorCode.ConnectionError"/> or <see cref="ErrorCode.WrongNumberReceivedBytes"/>.
@@ -1314,6 +1316,13 @@ public sealed class S7Driver
         {
             if (e is System.Net.Sockets.SocketException or System.IO.IOException or ObjectDisposedException)
                 return true;
+            // A wire op that blew its wall-clock deadline (S7OperationDeadline — the async read/write
+            // ceiling S7.Net's ignored socket ReadTimeout/WriteTimeout can't provide) means the
+            // connection is unresponsive: discard + reopen. This is the READ-leg sibling of the
+            // connect-timeout fix (STAB-14) — without it a frozen-but-established peer wedges the
+            // poll loop forever instead of surfacing Bad + reconnecting.
+            if (e is TimeoutException)
+                return true;
             // ISO-on-TCP framing/desync surface (STAB-15a): the stream position is untrustworthy —
             // a half-read PDU left by a timeout/cancellation makes every later response mis-frame.
             // NOTE: deliberately NOT System.IO.InvalidDataException — ReinterpretRawValue throws it

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7OperationDeadline.cs

@@ -0,0 +1,80 @@
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7;
+
+/// <summary>
+///     Bounds a single S7 wire operation with a wall-clock deadline, surfacing an overrun as a
+///     <see cref="TimeoutException"/>.
+/// </summary>
+/// <remarks>
+///     <para>
+///         S7.Net's <c>ReadTimeout</c> / <c>WriteTimeout</c> set the underlying
+///         <see cref="System.Net.Sockets.TcpClient"/>'s <c>ReceiveTimeout</c> / <c>SendTimeout</c>,
+///         which govern only <b>synchronous</b> socket calls — the <c>async</c> read/write paths
+///         S7.Net actually uses ignore them entirely. Without an explicit ceiling, a read on an
+///         established-but-frozen connection (a frozen PLC, a firewall DROP, or a cable pulled
+///         mid-flow — where the TCP session stays <i>open</i> but no reply ever arrives) blocks until
+///         the OS TCP stack gives up, which can be minutes. That silently wedges the driver's poll
+///         loop: no Bad tick, no reconnect — the STAB-14 "silently killed poll loop" failure, on the
+///         READ leg that the connect-timeout fix (<c>S7Driver.EnsureConnectedAsync</c>'s
+///         <c>CancelAfter</c>) never covered.
+///     </para>
+///     <para>
+///         The timed-out operation surfaces as a <see cref="TimeoutException"/>, which
+///         <c>S7Driver.IsS7ConnectionFatal</c> classifies connection-fatal → the handle is marked
+///         dead → the next <c>EnsureConnectedAsync</c> reopens. The end-to-end proof is the live gate
+///         <c>S7_1500ConnectTimeoutOutageTests</c> (a real snap7 sim frozen with <c>docker pause</c>).
+///     </para>
+/// </remarks>
+internal static class S7OperationDeadline
+{
+    /// <summary>Runs <paramref name="operation"/> under a <paramref name="timeout"/> wall-clock ceiling.</summary>
+    /// <typeparam name="T">The operation result type.</typeparam>
+    /// <param name="operation">The wire operation; receives a token cancelled at the deadline.</param>
+    /// <param name="timeout">The wall-clock ceiling for the operation.</param>
+    /// <param name="what">A short description of the operation for the timeout message (e.g. <c>read of 'DB1.DBW0'</c>).</param>
+    /// <param name="ct">The caller's cancellation token — its cancellation propagates as an <see cref="OperationCanceledException"/>, not a timeout.</param>
+    /// <returns>The operation result when it completes within the deadline.</returns>
+    /// <exception cref="TimeoutException">The operation did not complete within <paramref name="timeout"/>.</exception>
+    public static async Task<T> RunAsync<T>(
+        Func<CancellationToken, Task<T>> operation, TimeSpan timeout, string what, CancellationToken ct)
+    {
+        ArgumentNullException.ThrowIfNull(operation);
+        using var opCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
+        opCts.CancelAfter(timeout);
+        try
+        {
+            // WaitAsync guarantees control returns at the deadline even if the underlying op ignores
+            // its token; passing opCts.Token additionally lets an op that DOES honour cancellation
+            // unwind cleanly instead of leaking a wedged socket read behind an abandoned task.
+            return await operation(opCts.Token).WaitAsync(timeout, ct).ConfigureAwait(false);
+        }
+        catch (TimeoutException)
+        {
+            // WaitAsync's own deadline fired (the op ignored the token).
+            throw new TimeoutException(FormatMessage(what, timeout));
+        }
+        catch (OperationCanceledException) when (opCts.IsCancellationRequested && !ct.IsCancellationRequested)
+        {
+            // OUR deadline fired and the op honoured opCts.Token — not the caller's cancellation.
+            throw new TimeoutException(FormatMessage(what, timeout));
+        }
+    }
+
+    /// <summary>Runs a result-less <paramref name="operation"/> under a <paramref name="timeout"/> wall-clock ceiling.</summary>
+    /// <param name="operation">The wire operation; receives a token cancelled at the deadline.</param>
+    /// <param name="timeout">The wall-clock ceiling for the operation.</param>
+    /// <param name="what">A short description of the operation for the timeout message.</param>
+    /// <param name="ct">The caller's cancellation token.</param>
+    /// <returns>A task that completes when the operation completes within the deadline.</returns>
+    /// <exception cref="TimeoutException">The operation did not complete within <paramref name="timeout"/>.</exception>
+    public static Task RunAsync(
+        Func<CancellationToken, Task> operation, TimeSpan timeout, string what, CancellationToken ct)
+    {
+        ArgumentNullException.ThrowIfNull(operation);
+        return RunAsync(
+            async token => { await operation(token).ConfigureAwait(false); return true; },
+            timeout, what, ct);
+    }
+
+    private static string FormatMessage(string what, TimeSpan timeout) =>
+        $"S7 {what} did not complete within {(int)timeout.TotalMilliseconds} ms.";
+}

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs

@@ -63,6 +63,40 @@ public sealed class S7DriverReconnectTests
         factory.Created[1].Disposed.ShouldBeFalse();
     }
 
+    /// <summary>
+    ///     R2-01 read-leg gate. A <see cref="TimeoutException"/> — what <c>S7OperationDeadline</c>
+    ///     raises when a wire read blows its wall-clock ceiling on an established-but-frozen peer
+    ///     (the async read S7.Net's ignored socket <c>ReadTimeout</c> can't bound) — is classified
+    ///     connection-fatal: the handle is marked dead and the NEXT read reopens a fresh connection,
+    ///     exactly like a socket drop. Before the fix a read-timeout was NOT fatal, so the wedged
+    ///     handle was reused and the poll loop never recovered (<c>Created.Count</c> pinned at 1).
+    /// </summary>
+    [Fact]
+    public async Task Read_reopens_after_a_wire_operation_timeout()
+    {
+        var factory = new FakeS7PlcFactory();
+        using var drv = new S7Driver(Options(), "s7-read-timeout-fatal", factory);
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        factory.Created.Count.ShouldBe(1);
+
+        // The next read blows its deadline — surfaces as a TimeoutException, the shape
+        // S7OperationDeadline raises for a frozen-but-established peer.
+        factory.Created[0].NextReadThrows = new TimeoutException("S7 read of 'DB1.DBW0' did not complete within 250 ms.");
+
+        // Read #1 — timeout → Degraded, handle marked dead, no reopen yet.
+        var bad = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken);
+        bad[0].StatusCode.ShouldNotBe(0u);
+        drv.GetHealth().State.ShouldBe(DriverState.Degraded);
+        factory.Created.Count.ShouldBe(1);
+
+        // Read #2 — disposes the dead handle, opens a SECOND connection, reads Good, recovers.
+        var recovered = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken);
+        recovered[0].StatusCode.ShouldBe(0u);
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+        factory.Created.Count.ShouldBe(2);
+        factory.Created[0].Disposed.ShouldBeTrue();
+    }
+
     /// <summary>
     ///     A protocol / data-address error (S7.Net <see cref="ErrorCode.ReadData"/>) is NOT a
     ///     connection loss — the driver keeps the same connection and does not churn a reopen.

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7OperationDeadlineTests.cs

@@ -0,0 +1,95 @@
+using System.Diagnostics;
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests;
+
+/// <summary>
+///     Unit tests for <see cref="S7OperationDeadline"/> — the wall-clock ceiling that bounds every
+///     S7 async wire op (R2-01 read-leg gate). Async S7.Net reads/writes ignore the socket
+///     ReadTimeout/WriteTimeout, so a frozen-but-established peer would otherwise block for minutes;
+///     these pin the deadline behaviour without a live PLC.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class S7OperationDeadlineTests
+{
+    private static readonly TimeSpan ShortTimeout = TimeSpan.FromMilliseconds(150);
+
+    /// <summary>An operation that completes within the deadline returns its value untouched.</summary>
+    [Fact]
+    public async Task Completes_within_deadline_returns_value()
+    {
+        var result = await S7OperationDeadline.RunAsync(
+            _ => Task.FromResult(42), ShortTimeout, "read", TestContext.Current.CancellationToken);
+        result.ShouldBe(42);
+    }
+
+    /// <summary>
+    ///     An operation that ignores its token and never completes is cut at the deadline and
+    ///     surfaces a <see cref="TimeoutException"/> — the frozen-established-peer shape. Uses a bare
+    ///     <see cref="TaskCompletionSource"/> that never completes, so the token is genuinely ignored.
+    /// </summary>
+    [Fact]
+    public async Task Token_ignoring_hang_times_out()
+    {
+        var neverCompletes = new TaskCompletionSource<int>().Task;
+        var sw = Stopwatch.StartNew();
+
+        var ex = await Should.ThrowAsync<TimeoutException>(async () =>
+            await S7OperationDeadline.RunAsync(
+                _ => neverCompletes, ShortTimeout, "read of 'DB1.DBW0'", TestContext.Current.CancellationToken));
+
+        sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(5));
+        ex.Message.ShouldContain("did not complete within 150 ms");
+    }
+
+    /// <summary>
+    ///     An operation that honours its token (the well-behaved async-read case) is cancelled at the
+    ///     deadline; the resulting cancellation is normalised to a <see cref="TimeoutException"/>,
+    ///     NOT surfaced as an <see cref="OperationCanceledException"/>.
+    /// </summary>
+    [Fact]
+    public async Task Token_honouring_hang_times_out_as_timeout_not_cancel()
+    {
+        await Should.ThrowAsync<TimeoutException>(async () =>
+            await S7OperationDeadline.RunAsync(
+                async token =>
+                {
+                    await Task.Delay(Timeout.Infinite, token);
+                    return 0;
+                },
+                ShortTimeout, "read", TestContext.Current.CancellationToken));
+    }
+
+    /// <summary>
+    ///     Caller cancellation (as opposed to the deadline firing) propagates as an
+    ///     <see cref="OperationCanceledException"/> — the deadline wrapper must not swallow genuine
+    ///     cancellation into a timeout.
+    /// </summary>
+    [Fact]
+    public async Task Caller_cancellation_propagates_as_operation_cancelled()
+    {
+        using var cts = new CancellationTokenSource();
+        cts.CancelAfter(50);
+
+        // A long deadline so the CALLER token fires first, not the internal deadline.
+        await Should.ThrowAsync<OperationCanceledException>(async () =>
+            await S7OperationDeadline.RunAsync(
+                async token =>
+                {
+                    await Task.Delay(Timeout.Infinite, token);
+                    return 0;
+                },
+                TimeSpan.FromSeconds(30), "read", cts.Token));
+    }
+
+    /// <summary>The result-less overload also bounds a hang and surfaces a <see cref="TimeoutException"/>.</summary>
+    [Fact]
+    public async Task Resultless_overload_times_out()
+    {
+        var neverCompletes = new TaskCompletionSource().Task;
+        await Should.ThrowAsync<TimeoutException>(async () =>
+            await S7OperationDeadline.RunAsync(
+                _ => neverCompletes, ShortTimeout, "write of 'DB1.DBW0'", TestContext.Current.CancellationToken));
+    }
+}