From 8999c722a063c3238ab30c497bf8ee3a8e0232c9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:03:36 -0400 Subject: [PATCH] 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