From fcdcf0e2fba10040e9a24ea4f9e3d67194358fda Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:48:42 -0400 Subject: [PATCH 01/13] fix(r2-01): extend reconnect fakes with token-honouring hang + probe-fault modes (task 0) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../S7DriverReconnectTests.cs | 71 ++++++++++++++++--- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index eef3b8a8..5f65d09b 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -4,7 +4,7 @@ { "id": 0, "subject": "Extend reconnect-test fakes with token-honouring hang + probe-fault modes (FakeS7Plc/FakeS7PlcFactory)", - "status": "pending", + "status": "completed", "blockedBy": [] }, { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index e4195df6..ece17260 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -143,6 +143,21 @@ public sealed class S7DriverReconnectTests /// When set, the NEXT d connection throws this on OpenAsync (once). public Exception? NextOpenThrows { get; set; } + /// + /// Number of subsequently-created connections whose + /// hangs until its token is cancelled (simulating the unreachable-but-not-refusing host + /// — pulled cable / firewall DROP — that STAB-14 needs and docker restart can't + /// produce). Each decrements it while positive. + /// + public int OpenHangsUntilCancelledCount { get; set; } + + /// + /// When set, copied onto every created connection's persistent + /// so a probe fails on every tick. Clear it + /// after init to arm connection #1 only. + /// + public Exception? ReadStatusThrowsForNewConnections { get; set; } + public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) { var plc = new FakeS7Plc(); @@ -151,6 +166,13 @@ public sealed class S7DriverReconnectTests plc.OpenThrowsOnce = ex; NextOpenThrows = null; } + if (OpenHangsUntilCancelledCount > 0) + { + plc.OpenHangsUntilCancelled = true; + OpenHangsUntilCancelledCount--; + } + if (ReadStatusThrowsForNewConnections is { } rex) + plc.ReadStatusThrows = rex; Created.Add(plc); return plc; } @@ -164,45 +186,76 @@ public sealed class S7DriverReconnectTests /// Thrown once by , then cleared. public Exception? OpenThrowsOnce { get; set; } + /// + /// When true, hangs until its token is cancelled, then throws + /// — the connect-timeout shape (an unreachable host + /// whose SYNs are dropped) the live rig can't cheaply produce (STAB-14). + /// + public bool OpenHangsUntilCancelled { get; set; } + /// Thrown once by the next , then cleared. public Exception? NextReadThrows { get; set; } + /// When true, hangs until its token is cancelled (STAB-15 cancel-mid-I/O). + public bool ReadHangsUntilCancelled { get; set; } + + /// When true, hangs until its token is cancelled (STAB-15 cancel-mid-I/O). + public bool WriteHangsUntilCancelled { get; set; } + + /// Thrown by EVERY (persistent, not one-shot) — arms a probe fault (STAB-15b). + public Exception? ReadStatusThrows { get; set; } + + /// When true, hangs until its token is cancelled (probe-timeout, STAB-15b). + public bool ReadStatusHangsUntilCancelled { get; set; } + /// Boxed value returned by a good read (UInt16 tag → ushort). public object? ReadValue { get; set; } = (ushort)42; - public Task OpenAsync(CancellationToken cancellationToken) + public async Task OpenAsync(CancellationToken cancellationToken) { + if (OpenHangsUntilCancelled) + await Task.Delay(System.Threading.Timeout.Infinite, cancellationToken).ConfigureAwait(false); if (OpenThrowsOnce is { } ex) { OpenThrowsOnce = null; - return Task.FromException(ex); + throw ex; } IsConnected = true; - return Task.CompletedTask; } public void Close() => IsConnected = false; - public Task ReadAsync(string address, CancellationToken cancellationToken) + public async Task ReadAsync(string address, CancellationToken cancellationToken) { + if (ReadHangsUntilCancelled) + await Task.Delay(System.Threading.Timeout.Infinite, cancellationToken).ConfigureAwait(false); if (NextReadThrows is { } ex) { NextReadThrows = null; - return Task.FromException(ex); + throw ex; } - return Task.FromResult(ReadValue); + return ReadValue; } public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) => throw new NotSupportedException("FakeS7Plc: block reads not needed for the reconnect tests"); - public Task WriteAsync(string address, object value, CancellationToken cancellationToken) => - Task.CompletedTask; + public async Task WriteAsync(string address, object value, CancellationToken cancellationToken) + { + if (WriteHangsUntilCancelled) + await Task.Delay(System.Threading.Timeout.Infinite, cancellationToken).ConfigureAwait(false); + } public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) => Task.CompletedTask; - public Task ReadStatusAsync(CancellationToken cancellationToken) => Task.CompletedTask; + public async Task ReadStatusAsync(CancellationToken cancellationToken) + { + if (ReadStatusHangsUntilCancelled) + await Task.Delay(System.Threading.Timeout.Infinite, cancellationToken).ConfigureAwait(false); + if (ReadStatusThrows is { } ex) + throw ex; + } public void Dispose() { -- 2.52.0 From 9dedb7936b24bc8984efc33d5244b75075db8488 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:50:07 -0400 Subject: [PATCH 02/13] fix(r2-01): RED connect-timeout regression tests T1+T3 (task 1) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../S7DriverReconnectTests.cs | 50 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 5f65d09b..262ff018 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -10,7 +10,7 @@ { "id": 1, "subject": "RED: connect-timeout regression tests T1+T3 (read/write degrade instead of throwing OCE) — must FAIL at f6eaa267", - "status": "pending", + "status": "completed", "blockedBy": [0] }, { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index ece17260..57567c54 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -133,6 +133,56 @@ public sealed class S7DriverReconnectTests drv.GetHealth().State.ShouldBe(DriverState.Healthy); } + // ---- STAB-14: connect-timeout must degrade, not escape as OCE ---- + + /// + /// T1 (STAB-14 regression). When the reopen HANGS and is cut by the internal + /// CancelAfter(_options.Timeout) (the unreachable-host outage — SYNs dropped, not + /// refused), the read batch must degrade to a communication error with Degraded health, + /// NOT throw the timeout-born up to the caller. + /// Before the fix the OCE escaped . + /// + [Fact] + public async Task Read_degrades_batch_on_connect_timeout_instead_of_throwing() + { + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(Options(), "s7-read-timeout", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + // Drop connection #1 so the next read reopens; every subsequent open hangs until the + // internal connect-timeout CTS fires. + factory.Created[0].NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped"); + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); // marks dead + factory.OpenHangsUntilCancelledCount = int.MaxValue; + + // Must NOT throw — the connect timeout degrades the batch. + var timedOut = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + timedOut[0].StatusCode.ShouldNotBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// + /// T3 (STAB-14 regression). The same connect-timeout degrade through + /// — EnsureConnectedAsync is called before the + /// per-item loop, so a hanging reopen must degrade the whole write batch, not throw OCE. + /// + [Fact] + public async Task Write_degrades_batch_on_connect_timeout_instead_of_throwing() + { + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(Options(), "s7-write-timeout", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + factory.Created[0].NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped"); + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); // marks dead + factory.OpenHangsUntilCancelledCount = int.MaxValue; + + var timedOut = await drv.WriteAsync( + [new WriteRequest("W0", (ushort)1)], TestContext.Current.CancellationToken); + timedOut[0].StatusCode.ShouldNotBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + // ---- fakes ---- private sealed class FakeS7PlcFactory : IS7PlcFactory -- 2.52.0 From 78133041997635275dc211e7adbc00b2fd199556 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:51:57 -0400 Subject: [PATCH 03/13] fix(r2-01): RED poll-loop-survival T2 + caller-cancel pinning T4 (task 2) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../S7DriverReconnectTests.cs | 64 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 262ff018..066c703e 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -16,7 +16,7 @@ { "id": 2, "subject": "RED: poll-loop-survival test T2 (subscription survives connect-timeout outage) + caller-cancel pinning test T4", - "status": "pending", + "status": "completed", "blockedBy": [0, 1] }, { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index 57567c54..d39ae618 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -183,6 +183,70 @@ public sealed class S7DriverReconnectTests drv.GetHealth().State.ShouldBe(DriverState.Degraded); } + /// + /// T2 (STAB-14 regression). A subscription poll loop that ticks during an + /// unreachable-host outage (each reconnect times out) must SURVIVE and recover: a Good + /// OnDataChange must arrive after a Bad one once the host returns. Before the fix + /// the timeout-born OCE escaped ReadAsync, the poll loop's bare + /// catch (OperationCanceledException) { return; } read it as teardown, and the loop + /// died permanently — no recovery event ever fired (10 s deadline trips). + /// + [Fact] + public async Task Subscription_poll_loop_survives_a_connect_timeout_outage_and_recovers() + { + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(Options(), "s7-poll-survive", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var sawBad = false; + drv.OnDataChange += (_, e) => + { + if (e.Snapshot.StatusCode != 0u) sawBad = true; + else if (sawBad) tcs.TrySetResult(); + }; + + // First poll tick drops the socket (marks dead); the next two reconnect attempts hang + // until the internal connect-timeout CTS fires; then opens succeed again. + factory.Created[0].NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped"); + factory.OpenHangsUntilCancelledCount = 2; + + await drv.SubscribeAsync(["W0"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + + // Must observe a Good change AFTER a Bad one within the deadline — proves the loop lived. + await tcs.Task.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + sawBad.ShouldBeTrue(); + } + + /// + /// T4 (pinning). Real CALLER cancellation during a connect must still propagate as + /// — the STAB-14 fix must not over-rotate into + /// swallowing genuine cancellation. Passes before and after the fix. + /// + [Fact] + public async Task Read_still_propagates_caller_cancellation_during_connect() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromSeconds(5), // long, so the CALLER token fires first, not the internal timeout + Probe = new S7ProbeOptions { Enabled = false }, + Tags = [new S7TagDefinition("W0", "DB1.DBW0", S7DataType.UInt16, Writable: false)], + }; + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(opts, "s7-caller-cancel", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + factory.Created[0].NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped"); + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); // marks dead + factory.OpenHangsUntilCancelledCount = 1; + + using var cts = new CancellationTokenSource(); + cts.CancelAfter(50); + await Should.ThrowAsync( + async () => await drv.ReadAsync(["W0"], cts.Token)); + } + // ---- fakes ---- private sealed class FakeS7PlcFactory : IS7PlcFactory -- 2.52.0 From 4bb25fc03977ee23b346b2ea5d7e0e1ff0060d4f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:52:42 -0400 Subject: [PATCH 04/13] =?UTF-8?q?fix(r2-01):=20surface=20connect-timeout?= =?UTF-8?q?=20as=20TimeoutException,=20not=20OCE=20=E2=80=94=20poll=20loop?= =?UTF-8?q?s=20no=20longer=20die=20on=20unreachable-host=20outages=20(STAB?= =?UTF-8?q?-14,=20task=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 066c703e..8136a897 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -22,7 +22,7 @@ { "id": 3, "subject": "GREEN: EnsureConnectedAsync + InitializeAsync connect-timeout OCE -> TimeoutException conversion (STAB-14 root fix)", - "status": "pending", + "status": "completed", "blockedBy": [1, 2] }, { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 282fecd1..31afceed 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -187,6 +187,16 @@ public sealed class S7Driver cts.CancelAfter(_options.Timeout); await plc.OpenAsync(cts.Token).ConfigureAwait(false); } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + // The linked CancelAfter fired — a connect TIMEOUT, not a caller cancellation. + // Surface it as a TimeoutException so init-time and reconnect-time timeouts report + // the same exception type (STAB-14 consistency); the outer catch still faults health. + try { plc.Dispose(); } catch { } + throw new TimeoutException( + $"S7 connect to {_options.Host}:{_options.Port} timed out after " + + $"{(int)_options.Timeout.TotalMilliseconds} ms."); + } catch { // Dispose the partially-constructed connection so a failed init doesn't leak it @@ -1205,6 +1215,17 @@ public sealed class S7Driver cts.CancelAfter(_options.Timeout); await plc.OpenAsync(cts.Token).ConfigureAwait(false); } + catch (OperationCanceledException) when (!ct.IsCancellationRequested) + { + // The linked CancelAfter fired — a connect TIMEOUT, not a caller cancellation. + // Surface it as a connection FAILURE (TimeoutException) so callers route it through + // their degrade paths: an escaping OCE reads as teardown and permanently killed the + // subscription poll loops (STAB-14, the unreachable-host regression in the Crit-3 fix). + try { plc.Dispose(); } catch { /* best-effort */ } + throw new TimeoutException( + $"S7 connect to {_options.Host}:{_options.Port} timed out after " + + $"{(int)_options.Timeout.TotalMilliseconds} ms."); + } catch { try { plc.Dispose(); } catch { /* best-effort */ } -- 2.52.0 From 963543a578455aa7ec9ab0d3ccd73bb3969649b9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:54:03 -0400 Subject: [PATCH 05/13] fix(r2-01): filter OCE catches on the owning token in read/write wrappers + poll loop (STAB-14 defense-in-depth, task 4) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 21 ++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 8136a897..c6b2dd2e 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -28,7 +28,7 @@ { "id": 4, "subject": "Defensive when-filters on ensure-wrapper OCE rethrows (:488/:1000) + poll-loop OCE catches (:1383/:1396/:1404)", - "status": "pending", + "status": "completed", "blockedBy": [3] }, { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 31afceed..1e93068b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -495,7 +495,10 @@ public sealed class S7Driver { plc = await EnsureConnectedAsync(cancellationToken).ConfigureAwait(false); } - catch (OperationCanceledException) { throw; } + // Rethrow ONLY caller cancellation. A timeout-born OCE (none expected after the + // EnsureConnectedAsync conversion, but e.g. an S7.Net-internal CTS) must degrade the + // batch below, not escape as teardown (STAB-14). + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (Exception ex) { for (var i = 0; i < fullReferences.Count; i++) @@ -1007,7 +1010,9 @@ public sealed class S7Driver { plc = await EnsureConnectedAsync(cancellationToken).ConfigureAwait(false); } - catch (OperationCanceledException) { throw; } + // Rethrow ONLY caller cancellation — a timeout-born OCE degrades the batch below rather + // than escaping as teardown (STAB-14 defense-in-depth, mirrors ReadAsync). + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (Exception ex) { for (var i = 0; i < writes.Count; i++) @@ -1401,7 +1406,10 @@ public sealed class S7Driver await PollOnceAsync(state, forceRaise: true, ct).ConfigureAwait(false); consecutiveFailures = 0; } - catch (OperationCanceledException) { return; } + // Only a teardown OCE (the loop's own token) exits the loop; any other OCE (e.g. a + // connect-timeout that slipped past the wrapper filter) falls to the backoff path below + // so the loop lives (STAB-14 defense-in-depth). + catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } catch (Exception ex) { // First-read error — polling continues; log so the operator has an event trail. @@ -1415,14 +1423,17 @@ public sealed class S7Driver // ticks reset consecutiveFailures back to 0 so the cadence snaps back to Interval. var delay = ComputeBackoffDelay(state.Interval, consecutiveFailures); try { await Task.Delay(delay, ct).ConfigureAwait(false); } - catch (OperationCanceledException) { return; } + // Task.Delay can only observe ct-triggered OCE, so this filter is purely for uniformity. + catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } try { await PollOnceAsync(state, forceRaise: false, ct).ConfigureAwait(false); consecutiveFailures = 0; } - catch (OperationCanceledException) { return; } + // Only teardown exits; any non-teardown OCE (e.g. a slipped-through connect timeout) + // backs off instead of killing the loop (STAB-14 defense-in-depth). + catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } catch (Exception ex) { // Sustained polling error — loop continues with backoff; log + update health. -- 2.52.0 From 066514fed321801c9235500070a9c69134475816 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:55:39 -0400 Subject: [PATCH 06/13] =?UTF-8?q?docs(r2-01):=20Finding-1=20close=20gate?= =?UTF-8?q?=20=E2=80=94=20S7=20unit=20238/238=20green;=20record=20pre-exis?= =?UTF-8?q?ting=20out-of-scope=20CLI=20comment-test=20failure=20(task=205)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- archreview/plans/R2-01-s7-fault-hardening-plan.md | 4 ++++ archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md b/archreview/plans/R2-01-s7-fault-hardening-plan.md index c77dd0fa..f47bc8cd 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md @@ -730,3 +730,7 @@ Steps: | STAB-8 note (Task 10) | **S** (trivial) | None — doc only | | Live gate + close-out (Tasks 11-12) | **S** | None locally (env-gated skip); live run is operator-driven | | **Overall** | **S-M** (~2-3 h implementer wall-time) | Single-file production change (`S7Driver.cs`); no contract/DI/schema changes | + +## Execution deviations (R2-01) + +- **Pre-existing, out-of-scope CLI test failure (Task 5/12 gate).** `SubscribeCommandConsoleHandlerCommentTests.SubscribeCommand_explains_why_OnDataChange_uses_console_Output_synchronously` in `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests` FAILS at the branch base (`1676c8f4`) — the source-scanning assertion expects the literal phrase `"CliFx console"` in `SubscribeCommand.cs`, which commit `9cad9ed0` (docs: strip tracking-ID comments) removed. R2-01 never touches the CLI project (`git diff --stat 1676c8f4 HEAD -- src/Drivers/Cli tests/Drivers/Cli` is empty). Left as-is: not caused by, nor in scope for, S7 fault-path hardening. The rest of the S7 CLI suite is green (48/49). The S7 unit suite (`Driver.S7.Tests`) is fully green (238/238). diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index c6b2dd2e..536f17d4 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -34,7 +34,7 @@ { "id": 5, "subject": "Finding-1 close gate: full S7 unit + CLI test-project sweep green", - "status": "pending", + "status": "completed", "blockedBy": [4] }, { -- 2.52.0 From 8aed9ac365e3c7374fd2651bbfb0164131ddce39 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:57:08 -0400 Subject: [PATCH 07/13] fix(r2-01): RED framing-fault classification Theory T5 (STAB-15a, task 6) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../S7DriverReconnectTests.cs | 44 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 536f17d4..bb19fed9 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -40,7 +40,7 @@ { "id": 6, "subject": "RED: framing-fault classification Theory T5 (TPKT/TPDU/WrongNumberOfBytes/PlcException-WrongNumberReceivedBytes reopen) — must FAIL", - "status": "pending", + "status": "completed", "blockedBy": [0] }, { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index d39ae618..ab9ffdf8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -247,6 +247,50 @@ public sealed class S7DriverReconnectTests async () => await drv.ReadAsync(["W0"], cts.Token)); } + // ---- STAB-15a: ISO-on-TCP framing/desync faults must reopen ---- + + /// + /// T5 (STAB-15a). A framing/desync fault on ISO-on-TCP (S7.Net's + /// / / + /// , or a carrying + /// ) leaves the stream position + /// untrustworthy — only a reopen recovers. The next read must dispose connection #1 and + /// open a fresh #2. Before the fix these were not classified fatal, so the desynced handle + /// was reused forever (Created.Count pinned at 1). + /// + [Theory] + [InlineData("tpkt")] + [InlineData("tpdu")] + [InlineData("wrongbytes")] + [InlineData("plc-wrongnumber")] + public async Task Framing_faults_are_classified_connection_fatal_and_reopen(string kind) + { + Exception framing = kind switch + { + "tpkt" => new TPKTInvalidException(), + "tpdu" => new TPDUInvalidException(), + "wrongbytes" => new WrongNumberOfBytesException(), + "plc-wrongnumber" => new PlcException(ErrorCode.WrongNumberReceivedBytes, "short read"), + _ => throw new ArgumentOutOfRangeException(nameof(kind)), + }; + + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(Options(), "s7-framing", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + // Read #1 hits the framing fault — Bad status, handle marked dead, but no reopen yet. + factory.Created[0].NextReadThrows = framing; + var bad = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + bad[0].StatusCode.ShouldNotBe(0u); + factory.Created.Count.ShouldBe(1); + + // Read #2 reopens a fresh connection and reads Good. + var recovered = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + recovered[0].StatusCode.ShouldBe(0u); + factory.Created.Count.ShouldBe(2); + factory.Created[0].Disposed.ShouldBeTrue(); + } + // ---- fakes ---- private sealed class FakeS7PlcFactory : IS7PlcFactory -- 2.52.0 From dab0029add271b5f7e515633cbd30087673f10dd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:58:11 -0400 Subject: [PATCH 08/13] fix(r2-01): classify ISO-on-TCP framing/desync faults as connection-fatal (STAB-15a, task 7) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 30 +++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index bb19fed9..06a23643 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -46,7 +46,7 @@ { "id": 7, "subject": "GREEN: broaden IsS7ConnectionFatal to the ISO-on-TCP framing surface (NOT InvalidDataException) + xmldoc (STAB-15a)", - "status": "pending", + "status": "completed", "blockedBy": [6] }, { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 1e93068b..0fc53c50 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -1256,10 +1256,24 @@ public sealed class S7Driver /// /// True when (or any inner exception) is a socket-level / connection - /// loss that a reopen can repair — a / - /// / , or an S7.Net - /// carrying . A - /// data-address / type error (S7.Net's / + /// loss OR an ISO-on-TCP framing/desync fault that a reopen can repair — a + /// / / + /// ; an S7.Net framing violation + /// ( / / + /// ); or a carrying + /// or . + /// A framing violation means the single serialized stream's position is untrustworthy — a + /// half-read PDU left by a timeout/cancellation makes every later response mis-frame, so only + /// a reopen recovers (STAB-15a: the Modbus STAB-3 desync mode rebuilt in S7). + /// + /// Deliberately NOT : the driver's own decode + /// backstop ( / ) throws + /// it for a declared-type/address-size CONFIG mismatch on a HEALTHY socket — classifying + /// it would churn a full reopen on every read of a mis-authored tag. A genuinely desynced + /// stream's very next PDU still lands in TPKT/TPDU/WrongNumber territory, which now tears + /// down, so a real desync self-heals within one extra failed call. + /// + /// A data-address / type error (S7.Net's / /// for a bad address, PUT/GET-denied, etc.) is deliberately /// NOT treated as fatal — reopening wouldn't help and would churn a healthy connection. /// @@ -1271,7 +1285,13 @@ public sealed class S7Driver { if (e is System.Net.Sockets.SocketException or System.IO.IOException or ObjectDisposedException) return true; - if (e is PlcException { ErrorCode: ErrorCode.ConnectionError }) + // 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 + // for a declared-type/address-size CONFIG mismatch on a healthy socket. + if (e is TPKTInvalidException or TPDUInvalidException or WrongNumberOfBytesException) + return true; + if (e is PlcException { ErrorCode: ErrorCode.ConnectionError or ErrorCode.WrongNumberReceivedBytes }) return true; } return false; -- 2.52.0 From 33044061c33f3536fc4b6ab1ac346d1691334d01 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:01:20 -0400 Subject: [PATCH 09/13] fix(r2-01): cancellation observed mid-PDU marks the connection dead before propagating (STAB-15a, task 8) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 19 +++++- .../S7DriverReconnectTests.cs | 60 +++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 06a23643..106e06ab 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -52,7 +52,7 @@ { "id": 8, "subject": "RED->GREEN: cancellation-during-I/O marks handle dead (T6 read + T7 write; per-item OCE catches set _plcDead before propagating)", - "status": "pending", + "status": "completed", "blockedBy": [7] }, { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 0fc53c50..91d1d7f8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -521,6 +521,17 @@ public sealed class S7Driver results[i] = new DataValueSnapshot(value, 0u, now, now); _health = new DriverHealth(DriverState.Healthy, now, null); } + catch (OperationCanceledException) + { + // Cancellation observed mid-PDU abandons a half-read ISO-on-TCP response on the + // single gated stream — the handle must be reopened, not reused (STAB-15a). Mark + // dead (we hold _gate) and propagate: the caller cancelled, nobody consumes the + // batch. Marking is unconditional on OCE (not routed through the type classifier): + // whether bytes moved is unknowable, and a false positive costs one cheap reopen + // versus permanent desync for a false negative. + _plcDead = true; + throw; + } catch (NotSupportedException) { results[i] = new DataValueSnapshot(null, StatusBadNotSupported, null, now); @@ -1057,8 +1068,12 @@ public sealed class S7Driver } catch (OperationCanceledException) { - // Let cancellation propagate rather than turning it into - // a status code — the gate is still held so Release() runs in finally. + // Cancellation observed mid-PDU abandons a half-written ISO-on-TCP request on + // the single gated stream — the handle must be reopened, not reused (STAB-15a). + // Mark dead (we hold _gate) and propagate: the caller cancelled, nobody consumes + // the batch, and Release() still runs in finally. Marking is unconditional on + // OCE — a false positive costs one cheap reopen versus permanent desync. + _plcDead = true; throw; } catch (NotSupportedException) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index ab9ffdf8..026f4abd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -291,6 +291,66 @@ public sealed class S7DriverReconnectTests factory.Created[0].Disposed.ShouldBeTrue(); } + // ---- STAB-15a: cancellation observed mid-I/O must mark the handle dead ---- + + /// + /// T6 (STAB-15a). Cancellation observed WHILE a read is in flight abandons a half-read + /// ISO-on-TCP response on the single gated stream — the handle can't be reused. The OCE + /// must propagate (caller cancelled; nobody consumes the batch) AND mark the handle dead so + /// the next read reopens. Before the fix the read path converted the OCE to a Bad snapshot + /// and kept the desynced handle. + /// + [Fact] + public async Task Cancelled_read_marks_handle_dead_and_next_read_reopens() + { + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(Options(), "s7-cancel-read", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + factory.Created[0].ReadHangsUntilCancelled = true; + using var cts = new CancellationTokenSource(); + cts.CancelAfter(50); + await Should.ThrowAsync( + async () => await drv.ReadAsync(["W0"], cts.Token)); + + // Fresh-token read reopens a fresh connection. + var recovered = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + recovered[0].StatusCode.ShouldBe(0u); + factory.Created.Count.ShouldBe(2); + factory.Created[0].Disposed.ShouldBeTrue(); + } + + /// + /// T7 (STAB-15a). Cancellation observed while a WRITE is in flight likewise marks the + /// handle dead before propagating (the OCE already propagated pre-fix; the reopen is what + /// was missing). + /// + [Fact] + public async Task Cancelled_write_marks_handle_dead_and_next_read_reopens() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Probe = new S7ProbeOptions { Enabled = false }, + Tags = [new S7TagDefinition("WW0", "DB1.DBW0", S7DataType.UInt16, Writable: true)], + }; + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(opts, "s7-cancel-write", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + factory.Created[0].WriteHangsUntilCancelled = true; + using var cts = new CancellationTokenSource(); + cts.CancelAfter(50); + await Should.ThrowAsync( + async () => await drv.WriteAsync([new WriteRequest("WW0", (ushort)1)], cts.Token)); + + var recovered = await drv.ReadAsync(["WW0"], TestContext.Current.CancellationToken); + recovered[0].StatusCode.ShouldBe(0u); + factory.Created.Count.ShouldBe(2); + factory.Created[0].Disposed.ShouldBeTrue(); + } + // ---- fakes ---- private sealed class FakeS7PlcFactory : IS7PlcFactory -- 2.52.0 From 8999c722a063c3238ab30c497bf8ee3a8e0232c9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:03:36 -0400 Subject: [PATCH 10/13] fix(r2-01): probe failures mark the handle dead under the gate so the next tick reopens (STAB-15b, task 9) --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 36 +++++++-- .../S7DriverReconnectTests.cs | 77 +++++++++++++++++++ 3 files changed, 107 insertions(+), 8 deletions(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 106e06ab..1eff77e8 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -58,7 +58,7 @@ { "id": 9, "subject": "RED->GREEN: probe marks handle dead under the gate (T8 persistent probe fault + T9 probe timeout; ProbeLoopAsync inner catch restructure, STAB-15b)", - "status": "pending", + "status": "completed", "blockedBy": [8] }, { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 91d1d7f8..df642c02 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -1596,17 +1596,39 @@ public sealed class S7Driver await _gate.WaitAsync(probeCts.Token).ConfigureAwait(false); try { - // EnsureConnectedAsync doubles as the reconnect backstop: if a read/write - // marked the handle dead (or the socket dropped between ticks), the probe - // reopens it here so recovery doesn't wait for the next data call. - var plc = await EnsureConnectedAsync(probeCts.Token).ConfigureAwait(false); - await plc.ReadStatusAsync(probeCts.Token).ConfigureAwait(false); - success = true; + try + { + // EnsureConnectedAsync doubles as the reconnect backstop: if a read/write + // marked the handle dead (or the socket dropped between ticks), the probe + // reopens it here so recovery doesn't wait for the next data call. + var plc = await EnsureConnectedAsync(probeCts.Token).ConfigureAwait(false); + await plc.ReadStatusAsync(probeCts.Token).ConfigureAwait(false); + success = true; + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Loop teardown — rethrow to the outer filter (after finally releases the gate). + throw; + } + catch (OperationCanceledException) + { + // Probe deadline fired mid-PDU: the CPU-status response may still be in + // flight, so the handle can't be reused (half-read ⇒ desync). We hold _gate + // here, keeping _plcDead gate-guarded (STAB-15b). + _plcDead = true; + } + catch (Exception ex) + { + // STAB-15b: classify + mark while holding _gate so a wire-dead/desynced + // handle whose TcpClient still claims Connected is reopened by the NEXT + // tick's EnsureConnectedAsync instead of being re-probed forever. + MarkConnectionDeadIfFatal(ex); + } } finally { _gate.Release(); } } catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } - catch { /* transport/timeout/exception — treated as Stopped below */ } + catch { /* gate-wait timeout / other — treated as Stopped below (gate contention is not a connection fault) */ } TransitionTo(success ? HostState.Running : HostState.Stopped); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index 026f4abd..c9372b3c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -351,6 +351,83 @@ public sealed class S7DriverReconnectTests factory.Created[0].Disposed.ShouldBeTrue(); } + // ---- STAB-15b: the probe must mark a lying/hung handle dead so the next tick reopens ---- + + /// + /// T8 (STAB-15b). A probe failure on a handle whose TcpClient.Connected still lies + /// true (a wire-dead or desynced socket) must mark the handle dead under the gate, so the + /// NEXT probe tick's EnsureConnectedAsync disposes it and reopens. Before the fix the + /// probe swallowed the failure without marking dead, so the lying handle was re-probed + /// forever (Created.Count pinned at 1). + /// + [Fact] + public async Task Probe_failure_on_connected_socket_marks_dead_and_next_tick_reopens() + { + var opts = ProbeEnabledOptions(); + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(opts, "s7-probe-fail", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + // Arm connection #1 to fail EVERY probe with a connection-fatal fault (persistent). Later + // connections stay healthy (default ReadStatusThrows == null). + factory.Created[0].ReadStatusThrows = new System.Net.Sockets.SocketException(); + + await WaitUntilAsync(() => factory.Created.Count >= 2, TimeSpan.FromSeconds(5)); + factory.Created.Count.ShouldBeGreaterThanOrEqualTo(2); + factory.Created[0].Disposed.ShouldBeTrue(); + + await WaitUntilAsync(() => drv.GetHostStatuses()[0].State == HostState.Running, TimeSpan.FromSeconds(5)); + drv.GetHostStatuses()[0].State.ShouldBe(HostState.Running); + } + + /// + /// T9 (STAB-15b). A probe that TIMES OUT mid-ReadStatusAsync (the CPU-status PDU may + /// still be in flight ⇒ desync) likewise marks the handle dead under the gate so the next + /// tick reopens. Before the fix the probe-deadline OCE fell to the bare swallow and the + /// handle was reused forever. + /// + [Fact] + public async Task Probe_timeout_marks_dead() + { + var opts = ProbeEnabledOptions(); + var factory = new FakeS7PlcFactory(); + using var drv = new S7Driver(opts, "s7-probe-timeout", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + factory.Created[0].ReadStatusHangsUntilCancelled = true; + + await WaitUntilAsync(() => factory.Created.Count >= 2, TimeSpan.FromSeconds(5)); + factory.Created.Count.ShouldBeGreaterThanOrEqualTo(2); + factory.Created[0].Disposed.ShouldBeTrue(); + + await WaitUntilAsync(() => drv.GetHostStatuses()[0].State == HostState.Running, TimeSpan.FromSeconds(5)); + drv.GetHostStatuses()[0].State.ShouldBe(HostState.Running); + } + + private static S7DriverOptions ProbeEnabledOptions() => new() + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Probe = new S7ProbeOptions + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(50), + Timeout = TimeSpan.FromMilliseconds(250), + }, + Tags = [new S7TagDefinition("W0", "DB1.DBW0", S7DataType.UInt16, Writable: false)], + }; + + /// Polls until it holds or elapses (returns either way). + private static async Task WaitUntilAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (DateTime.UtcNow < deadline) + { + if (condition()) return; + await Task.Delay(25).ConfigureAwait(false); + } + } + // ---- fakes ---- private sealed class FakeS7PlcFactory : IS7PlcFactory -- 2.52.0 From 92b2ac489181b22de1b142a11ec86811a66d5cc6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:04:22 -0400 Subject: [PATCH 11/13] docs(r2-01): mark EnsureConnectedAsync as the R2-09 connect-throttle seam (STAB-8 note, task 10) --- .../plans/R2-01-s7-fault-hardening-plan.md.tasks.json | 2 +- src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 1eff77e8..7070e3ab 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -64,7 +64,7 @@ { "id": 10, "subject": "Doc: R2-09 connect-throttle seam note on EnsureConnectedAsync (STAB-8 S7 part, note-only)", - "status": "pending", + "status": "completed", "blockedBy": [3] }, { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index df642c02..c25eeedc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -1211,6 +1211,15 @@ public sealed class S7Driver /// MUST be called while holding — it mutates / /// and reuses the single-connection-per-PLC invariant the gate /// enforces. The read / write / probe paths all take the gate before calling it. + /// + /// While the PLC is down every data call and probe tick pays a full connect attempt + /// bounded only by _options.Timeout (STAB-8). This method is the single + /// choke-point all S7 reconnects flow through (already gate-serialized), so the + /// fleet-wide connect-attempt throttle planned in R2-09 plugs in here: a + /// last-failed-attempt timestamp checked at the top of the slow path. Deliberately NOT + /// implemented driver-locally — see + /// archreview/plans/R2-01-s7-fault-hardening-plan.md Finding 3. + /// /// /// Cancellation token for the (re)connect attempt. /// A live . -- 2.52.0 From a1b22f979dbdcd322b2989ab4c2610e0f287c99b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:06:08 -0400 Subject: [PATCH 12/13] =?UTF-8?q?test(r2-01):=20env-gated=20live=20connect?= =?UTF-8?q?-timeout=20outage=20test=20=E2=80=94=20the=20STAB-14=20outage?= =?UTF-8?q?=20class=20the=20bounce=20test=20cannot=20produce=20(task=2011)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...2-01-s7-fault-hardening-plan.md.tasks.json | 3 +- .../S7_1500ConnectTimeoutOutageTests.cs | 138 ++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ConnectTimeoutOutageTests.cs diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 7070e3ab..93f78ee9 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -70,7 +70,8 @@ { "id": 11, "subject": "Env-gated live connect-timeout outage test (S7_TIMEOUT_OUTAGE_START_CMD/STOP_CMD blackhole design; skips cleanly offline)", - "status": "pending", + "status": "deferred-live", + "note": "env-gated; runs in serial live pass (integration suite; serialized heavy pass). Test authored + committed; verified it SKIPs cleanly offline.", "blockedBy": [5] }, { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ConnectTimeoutOutageTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ConnectTimeoutOutageTests.cs new file mode 100644 index 00000000..07793557 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ConnectTimeoutOutageTests.cs @@ -0,0 +1,138 @@ +using System.Diagnostics; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for STAB-14 — the connect-TIMEOUT outage class the ordinary bounce test +/// () structurally cannot produce. A docker restart +/// yields connection-refused (an immediate SocketException); STAB-14 only fires +/// on connect-timeout — the SYN-blackhole shape (packets DROPPED, not rejected) of a +/// pulled cable / powered-off PLC / firewall DROP. Before the fix, a subscription poll loop +/// that ticked during such an outage was silently and permanently killed (the timeout-born OCE +/// read as teardown); this test would hang at the recovery assertion. +/// +/// +/// +/// Triple-gated so it never runs (or leaves the shared sim blackholed) by accident: +/// +/// — the sim must be reachable; +/// S7_TIMEOUT_OUTAGE_START_CMD — begins DROPPING traffic to the sim port +/// (a blackhole, not a reset); and +/// S7_TIMEOUT_OUTAGE_STOP_CMD — removes the block. +/// +/// Any absent gate ⇒ clean (safe offline on macOS). A +/// try/finally ALWAYS runs the stop command once the start succeeded, so a failure +/// mid-test never leaves the shared sim unreachable for other suites. +/// +/// +/// Operator run recipe (also drives 10.100.0.35 via lmxopcua-fix up s7 s7_1500): +/// +/// lmxopcua-fix up s7 s7_1500 +/// # docker pause = ACKless blackhole for established flows + unanswered SYNs for new connects: +/// export S7_TIMEOUT_OUTAGE_START_CMD='ssh dohertj2@10.100.0.35 "docker pause otopcua-python-snap7-s7_1500"' +/// export S7_TIMEOUT_OUTAGE_STOP_CMD='ssh dohertj2@10.100.0.35 "docker unpause otopcua-python-snap7-s7_1500"' +/// # iptables DROP variant (more faithful to a firewall blackhole): +/// # START: ssh dohertj2@10.100.0.35 "sudo iptables -I INPUT -p tcp --dport 1102 -j DROP" +/// # STOP: ssh dohertj2@10.100.0.35 "sudo iptables -D INPUT -p tcp --dport 1102 -j DROP" +/// dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests --filter "FullyQualifiedName~ConnectTimeoutOutage" +/// +/// +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Category", "Reconnect")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500ConnectTimeoutOutageTests(Snap7ServerFixture sim) +{ + private const string StartCmdEnvVar = "S7_TIMEOUT_OUTAGE_START_CMD"; + private const string StopCmdEnvVar = "S7_TIMEOUT_OUTAGE_STOP_CMD"; + + /// + /// Subscribes against the live sim, blackholes its traffic (each reconnect now TIMES OUT + /// rather than being refused), observes at least one Bad tick, then restores traffic and + /// asserts a Good OnDataChange resumes on the SAME subscription — the poll loop + /// survived the connect-timeout outage. + /// + [Fact] + public async Task Subscription_survives_a_live_connect_timeout_outage_ConnectTimeoutOutage() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var startCmd = Environment.GetEnvironmentVariable(StartCmdEnvVar); + var stopCmd = Environment.GetEnvironmentVariable(StopCmdEnvVar); + if (string.IsNullOrWhiteSpace(startCmd) || string.IsNullOrWhiteSpace(stopCmd)) + Assert.Skip($"Set both {StartCmdEnvVar} and {StopCmdEnvVar} (SYN-blackhole start/stop) to run this destructive live test."); + + var ct = TestContext.Current.CancellationToken; + var options = S7_1500Profile.BuildOptions(sim.Host, sim.Port); + await using var drv = new S7Driver(options, driverInstanceId: "s7-connect-timeout-live"); + await drv.InitializeAsync("{}", ct); + + var sawBad = false; + var recoveredTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + drv.OnDataChange += (_, e) => + { + if (e.Snapshot.StatusCode != 0u) sawBad = true; + else if (sawBad) recoveredTcs.TrySetResult(); + }; + + await drv.SubscribeAsync([S7_1500Profile.ProbeTag], TimeSpan.FromMilliseconds(250), ct); + + // Good baseline first — wait for at least one Good tick before the outage. + var baselineTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + void Baseline(object? _, DataChangeEventArgs e) + { + if (e.Snapshot.StatusCode == 0u) baselineTcs.TrySetResult(); + } + drv.OnDataChange += Baseline; + await baselineTcs.Task.WaitAsync(TimeSpan.FromSeconds(15), ct); + drv.OnDataChange -= Baseline; + + var blackholed = false; + try + { + // Begin dropping traffic — every reconnect attempt now TIMES OUT (SYN blackhole), + // the exact class STAB-14 killed. docker restart could not produce this. + await RunShellCommandAsync(startCmd!, ct); + blackholed = true; + + // Observe at least one Bad tick during the outage. + var badDeadline = DateTime.UtcNow + TimeSpan.FromSeconds(30); + while (!sawBad && DateTime.UtcNow < badDeadline) + await Task.Delay(TimeSpan.FromMilliseconds(250), ct); + sawBad.ShouldBeTrue("the subscription should have observed the blackhole outage as a Bad tick"); + } + finally + { + if (blackholed) + { + // ALWAYS restore traffic once the blackhole started — never leave the shared sim dead. + await RunShellCommandAsync(stopCmd!, CancellationToken.None); + } + } + + // The loop must have SURVIVED the timeout outage and recovered on the same subscription. + await recoveredTcs.Task.WaitAsync(TimeSpan.FromSeconds(90), ct); + } + + private static async Task RunShellCommandAsync(string command, CancellationToken ct) + { + // Run through the login shell so an SSH/docker one-liner in the env var works verbatim. + using var proc = new Process + { + StartInfo = new ProcessStartInfo("/bin/sh", $"-c \"{command.Replace("\"", "\\\"")}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }, + }; + proc.Start(); + await proc.WaitForExitAsync(ct); + proc.ExitCode.ShouldBe(0, $"outage command failed: {await proc.StandardError.ReadToEndAsync(ct)}"); + // Give the network rule / pause a moment to take effect before the next poll tick. + await Task.Delay(TimeSpan.FromSeconds(1), ct); + } +} -- 2.52.0 From bdb7ed412dc626e18c6035998ea929a5153b3a63 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:09:03 -0400 Subject: [PATCH 13/13] =?UTF-8?q?docs(archreview):=20R2-01=20S7=20fault-pa?= =?UTF-8?q?th=20hardening=20complete=20=E2=80=94=20STAB-14/15=20fixed,=20S?= =?UTF-8?q?TAB-8=20seam=20documented=20(task=2012)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- archreview/05-protocol-drivers.md | 21 +++++++++++++++++++ ...2-01-s7-fault-hardening-plan.md.tasks.json | 5 +++-- archreview/plans/STATUS.md | 18 ++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/archreview/05-protocol-drivers.md b/archreview/05-protocol-drivers.md index 884cd5bf..455ea9a7 100644 --- a/archreview/05-protocol-drivers.md +++ b/archreview/05-protocol-drivers.md @@ -167,6 +167,13 @@ are the new STAB-14..17 below. ### 1. Stability +> **REMEDIATED by R2-01 (branch `r2/01-s7-fault-hardening`).** Connect-timeout now surfaces as a +> `TimeoutException` at the `EnsureConnectedAsync`/`InitializeAsync` source (not an escaping OCE), plus +> `when (token.IsCancellationRequested)` filters on the read/write ensure-wrappers (`:488`/`:1000`) and all +> three poll-loop OCE catches — so a non-teardown OCE degrades/backs-off instead of killing the loop. Unit +> guards T1–T4 (`S7DriverReconnectTests`, incl. the poll-loop-survival regression). Live gate +> `S7_1500ConnectTimeoutOutageTests` (env-gated SYN-blackhole, `deferred-live`). + **STAB-14 — High — NEW — S7's reconnect wrapper rethrows connect-timeout cancellation, and the poll loop's bare OCE catch then permanently kills the subscription.** `EnsureConnectedAsync` bounds the reopen with a linked @@ -192,6 +199,14 @@ degrade-to-`BadCommunicationError` path), and filter the poll loop's OCE catches on `ct.IsCancellationRequested`; add a fake-factory unit test where `OpenAsync` honours a token that only the `CancelAfter` fires. +> **REMEDIATED by R2-01 (branch `r2/01-s7-fault-hardening`).** `IsS7ConnectionFatal` broadened to the +> ISO-on-TCP framing surface (`TPKTInvalidException`/`TPDUInvalidException`/`WrongNumberOfBytesException` + +> `PlcException{WrongNumberReceivedBytes}`; deliberately NOT `InvalidDataException` — that is the driver's own +> config-mismatch decode backstop on a healthy socket). Cancellation observed mid-PDU on read + write now +> marks `_plcDead` before propagating; the probe restructured to classify + mark dead under `_gate` so the next +> tick reopens a lying/hung handle. Unit guards T5–T9 (`S7DriverReconnectTests`) + negative control +> `Read_does_not_reopen_on_a_data_address_error`. + **STAB-15 — High — NEW — S7's connection-fatal classification misses framing/desync errors and cancellation-during-I/O, reproducing Modbus's STAB-3 desync; and the probe never marks the handle dead.** @@ -340,6 +355,12 @@ S7's `ComputeBackoffDelay`, `S7Driver.cs:1447`) and a small connect-attempt throttle to the lazy-reconnect drivers (S7's `EnsureConnectedAsync` is now the natural home for a last-attempt timestamp). +> **SEAM DOCUMENTED by R2-01 (branch `r2/01-s7-fault-hardening`).** The S7 connect-throttle is left to +> R2-09 (fleet-wide reconnect-backoff), not implemented driver-locally. `EnsureConnectedAsync`'s `` +> now names itself as the single gate-serialized choke-point where a last-failed-attempt timestamp plugs in. +> (Note: R2-01's STAB-14/15 fixes already *reduce* dead-PLC cost — poll ticks that used to die now back off to +> `PollBackoffCap`, and connect-timeouts no longer escape as anomalous OCEs.) + **STAB-9 — Medium — `PollGroupEngine`'s `onError` sink is dead code: no driver passes it.** All five consumers construct the engine without the error callback (`ModbusDriver.cs:115`, `AbCipDriver.cs:133`, diff --git a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json index 93f78ee9..1435e5b4 100644 --- a/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-01-s7-fault-hardening-plan.md.tasks.json @@ -77,9 +77,10 @@ { "id": 12, "subject": "Close-out: whole-suite sweep + update archreview STATUS.md and 05 prior-finding table (STAB-14/15 FIXED, STAB-8 seam noted)", - "status": "pending", + "status": "completed", + "note": "S7 unit 246/246 green; S7 CLI 48/49 (1 pre-existing unrelated comment-scan failure, see deviations); solution BUILD clean (0 errors). Whole-solution dotnet test NOT run per controller memory-constraint (integration suites deferred to serial heavy pass).", "blockedBy": [5, 7, 8, 9, 10, 11] } ], - "lastUpdated": "2026-07-12" + "lastUpdated": "2026-07-13" } diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 0271f9e9..c56128a3 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -150,3 +150,21 @@ surface), 03/S2/S3 block-bridging, 03/P1 surgical adds (guard prerequisite in pl per-plan task counts, effort, and suggested execution order: [`00-INDEX.md`](00-INDEX.md) §"Round 2 plans". 193 bite-sized TDD tasks total, ~18–24 dev-days end-to-end; R2-01/02/03 (the re-review's new-and-sharp items) are ~2 days combined. + +**Round-2 execution progress:** + +- **R2-01 (05/STAB-14 + 05/STAB-15) — DONE.** S7 fault-path hardening implemented via 13 TDD tasks + on branch `r2/01-s7-fault-hardening` (all in `src/Drivers/.../S7Driver.cs` + `S7DriverReconnectTests.cs`). + STAB-14: connect-timeout now surfaces as `TimeoutException` (not an escaping OCE) at the + `EnsureConnectedAsync`/`InitializeAsync` source, plus `when (token.IsCancellationRequested)` filters on + the read/write ensure-wrappers + all three poll-loop OCE catches — subscription poll loops survive an + unreachable-host outage and recover. STAB-15: `IsS7ConnectionFatal` broadened to the ISO-on-TCP framing + surface (`TPKTInvalidException`/`TPDUInvalidException`/`WrongNumberOfBytesException` + + `PlcException{WrongNumberReceivedBytes}`; deliberately NOT `InvalidDataException`); cancellation observed + mid-PDU on read/write marks the handle dead before propagating; the probe restructured to classify + mark + dead under the gate so the next tick reopens. STAB-8 (S7 part): documented `EnsureConnectedAsync` as the + R2-09 connect-throttle seam (note-only, no throttle here). S7 unit suite 246/246 green (T1–T9 + pinning + + negative controls). **Live gate (task 11) `deferred-live`:** `S7_1500ConnectTimeoutOutageTests` authored + + skips cleanly offline; run it with env vars `S7_TIMEOUT_OUTAGE_START_CMD` / `S7_TIMEOUT_OUTAGE_STOP_CMD` + (SYN-blackhole start/stop, e.g. `docker pause`/`unpause` or `iptables … -j DROP`) on the 10.100.0.35 + s7_1500 fixture — the connect-*timeout* class the `docker restart` bounce test structurally cannot produce. -- 2.52.0