From 33044061c33f3536fc4b6ab1ac346d1691334d01 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:01:20 -0400 Subject: [PATCH] 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