# Design + Implementation Plan — R2-01 S7 Fault-Path Hardening **Source report:** `archreview/05-protocol-drivers.md` (2026-07-12 re-review) **Plan verified against tree at:** `f6eaa267` (master, clean) **Scope:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/` (`S7Driver.cs`, `IS7Plc.cs`) + `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/` + `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/` **Action-list item:** #1 in `archreview/00-OVERALL.md` — "S7: filter connect-timeout OCE in the ensure-wrapper + poll-loop catches; broaden fatal classification (framing/cancel); mark handle dead in probe catch. Add fake-factory connect-timeout test" (05/STAB-14 + 05/STAB-15; STAB-8's S7 seam noted for R2-09). > Both findings are residuals of the Critical-3 fix (`25c0c6f6`, round-1 plan §A1/STAB-1, > `archreview/plans/05-protocol-drivers-plan.md`). STAB-14 is a **regression inside that fix** — > it re-opens the exact scenario class (transient outage permanently kills S7 subscriptions) > that Critical 3 was merged to close, just via connect-*timeout* instead of no-reconnect. > Modbus's twin defect (STAB-3, same desync class as STAB-15) is **out of scope** here — the > report's remediation order pairs it with STAB-15 conceptually, but it lives in > `ModbusTcpTransport.cs` and is tracked separately. --- ## Verification summary Every cited line was opened at `f6eaa267`. **Both findings CONFIRMED. Three citations drifted by ≤6 lines** (the report was written against the same commit but a few ranges anchor on doc comments rather than code); corrected refs below are used throughout this plan. | Report citation | Status at `f6eaa267` | Corrected ref | |---|---|---| | STAB-14 — `S7Driver.cs:488` ensure-wrapper `catch (OperationCanceledException) { throw; }` in `ReadAsync` | **CONFIRMED exact** — unconditional rethrow, no `when` filter | `:488` | | STAB-14 — `S7Driver.cs:1000` same wrapper in `WriteAsync` | **CONFIRMED exact** | `:1000` | | STAB-14 — `S7Driver.cs:1383, 1404` `PollLoopAsync` bare `catch (OperationCanceledException) { return; }` | **CONFIRMED exact** (initial-read catch `:1383`, tick catch `:1404`; the `Task.Delay` catch at `:1396-1397` is a third bare OCE-return) | `:1383, :1396-1397, :1404` | | STAB-14 — `S7Driver.cs:1206-1208` linked `CancelAfter(_options.Timeout)` in `EnsureConnectedAsync` | **CONFIRMED, drifted −2** — the `using var cts` / `CancelAfter` / `OpenAsync(cts.Token)` block is at `:1204-1206`; method spans `:1187-1218` | `:1204-1206` | | STAB-15 — `S7Driver.cs:1239-1251` `IsS7ConnectionFatal` | **CONFIRMED, drifted +1..3** — the cited range starts inside the xmldoc; the method body is `:1242-1252` (`MarkConnectionDeadIfFatal` at `:1226-1229`). Classifies only `SocketException`/`IOException`/`ObjectDisposedException` (inner-walking) + `PlcException{ErrorCode.ConnectionError}` — no framing types, no cancellation | `:1242-1252` | | STAB-15 — `S7Driver.cs:1542` probe bare `catch { }` never marks dead | **CONFIRMED exact** — `catch { /* transport/timeout/exception — treated as Stopped below */ }`; no `MarkConnectionDeadIfFatal` anywhere in `ProbeLoopAsync` (`:1516-1549`) | `:1542` | | STAB-8 (S7 part) — `S7Driver.cs:1188-1224` unthrottled per-call reconnect | **CONFIRMED, drifted** — `EnsureConnectedAsync` is `:1187-1218`; no attempt throttle, full `OpenAsync` bounded only by `_options.Timeout` per data call/probe tick while the PLC is down | `:1187-1218` | Additional load-bearing facts verified for the design: - **`ReadAsync` per-item loop swallows OCE today** — there is no per-item OCE catch; a mid-PDU cancellation falls into `catch (Exception ex)` (`S7Driver.cs:536-543`) → `MarkConnectionDeadIfFatal` (a no-op for OCE) → `BadCommunicationError`, and the half-read socket is **kept**. `WriteAsync`'s per-item loop *does* have an explicit OCE catch (`:1043-1048`) that rethrows — also without marking the handle dead. Both are STAB-15(b) sites. - **The driver throws its own `System.IO.InvalidDataException`** as the decode type-mismatch backstop in `ReinterpretRawValue` (`S7Driver.cs:973-975`) — a *config* fault on a healthy socket. This constrains the classifier design (below): blanket-classifying `InvalidDataException` as connection-fatal would churn a reopen on every read of a mis-authored tag. - **S7.Net 0.20.0 framing surface** (verified against the shipped assembly, `~/.nuget/packages/s7netplus/0.20.0`): `TPKTInvalidException`, `TPDUInvalidException`, `WrongNumberOfBytesException` are `public` classes with public parameterless ctors (directly constructible in tests), plus `PlcException` with `ErrorCode.WrongNumberReceivedBytes`. None are classified fatal today. - **The probe's own OCE handling is already correct for STAB-14** — `:1541` `catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; }` filters on the loop token, so a probe-*timeout* OCE falls to the bare catch and reads as `Stopped`. Its only gap is STAB-15(b): the bare catch never marks the handle dead. - **`InitializeAsync` has the same unfiltered connect pattern** (`:186-196`): a connect-timeout OCE escapes to the outer `catch (Exception ex)` → `Faulted` + rethrow — the *health* outcome is right, but the caller receives an OCE for what is a connection failure. Included as a consistency fix (low stakes; init callers treat any throw as init failure). - **Test infra:** `S7DriverReconnectTests` (`tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs`) holds the private `FakeS7PlcFactory`/`FakeS7Plc` (Options: `Timeout = 250ms`, probe disabled). `InternalsVisibleTo("ZB.MOM.WW.OtOpcUa.Driver.S7.Tests")` is in place (`ZB.MOM.WW.OtOpcUa.Driver.S7.csproj:26`). The live suite `S7_1500ReconnectTests` is double-gated on `Snap7ServerFixture` + `S7_RECONNECT_BOUNCE_CMD` and — as the report says — **structurally blind to STAB-14**: `docker restart` yields connection-*refused* (`SocketException`, immediate), never the SYN-blackhole connect-*timeout* the bug needs. --- ## Finding 1 — STAB-14 (High, regression) — connect-timeout OCE permanently kills S7 subscription polls **Restatement.** `EnsureConnectedAsync` bounds the reopen with a linked `CancelAfter(_options.Timeout)` CTS (`S7Driver.cs:1204-1206`). Against an unreachable-but-not-refusing host (pulled cable, powered-off PLC on a switched network, firewall DROP — the classic field outage Critical 3 targeted), `OpenAsync` throws `TaskCanceledException` when that internal timer fires. The read/write ensure-wrappers rethrow *any* OCE unconditionally (`:488`, `:1000`), so the timeout escapes the driver as OCE. In the poll loop, `catch (OperationCanceledException) { return; }` (`:1383`, `:1404`) treats every OCE as teardown and exits — no log, no health change, no `HandlePollFailure`. Server-initiated reads/writes self-heal on the next call (the Critical-3 fix working as designed), but **every subscription poll loop that ticks during the outage is silently and permanently dead until redeploy**. **Verification.** Confirmed at the corrected refs above. Traced end-to-end: poll tick → `PollOnceAsync` (`:1458`) → internal `ReadAsync` with the loop's `ct` → `_gate` → `EnsureConnectedAsync` → hang → internal `cts.CancelAfter` fires → `TaskCanceledException` (an OCE) → wrapper `:488` rethrows (the caller's `ct` is **not** cancelled) → `PollLoopAsync` tick catch `:1404` → `return`. Nothing distinguishes this from teardown. Also confirmed the unit suite can't currently see it: `FakeS7Plc.OpenAsync` never honours its token (returns synchronously or `Task.FromException`), so no existing test produces a timeout-OCE from open. **Root cause.** The Critical-3 fix introduced an *internal* timeout implemented as cancellation (`CancelAfter` on a linked CTS) but let the resulting OCE escape with caller-cancellation semantics. Downstream, the bespoke poll loop's bare OCE-catch (a CONV-1 fork divergence — `PollGroupEngine` never had this shape) turned "one timed-out reconnect attempt" into "loop exits forever". Two mistakes compound: timeout encoded as OCE at the source, and OCE interpreted as teardown without consulting the loop token. **Proposed design.** Fix at **three layers** — the source plus two defensive filters — so no single future edit can reintroduce the class: 1. **Source conversion (the root fix):** in `EnsureConnectedAsync`, catch the internal-timeout OCE and rethrow it as `TimeoutException`. A connect timeout *is* a connection failure, not a cancellation; converting at the source makes every caller (read wrapper, write wrapper, probe) handle it via their existing generic degrade paths with zero further changes, and puts a meaningful message in the logs/`_health.LastError` instead of "operation was canceled". 2. **Wrapper filters (belt):** `:488` and `:1000` become `catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; }` — caller cancellation still propagates; any *other* OCE (should no longer occur after #1, but e.g. an S7.Net-internal CTS in a future package bump) falls through to the existing `catch (Exception ex)` → whole-batch `BadCommunicationError` + `Degraded`. 3. **Poll-loop filters (suspenders):** `:1383`, `:1396-1397`, `:1404` become `catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; }`. A non-teardown OCE at `:1383`/`:1404` then falls into the existing `catch (Exception ex)` → `HandlePollFailure` (log + Degraded + capped backoff) — the loop lives. (The `Task.Delay` catch at `:1396-1397` can only see `ct`-triggered OCE, so its filter is purely for uniformity.) 4. **Consistency:** apply the same timeout→`TimeoutException` conversion to the identical connect block in `InitializeAsync` (`:186-196`), so init-time and reconnect-time timeouts surface the same exception type. *Alternatives considered:* - **`when` filters only, no source conversion** (the report's literal recommendation). Works, but leaves "timeout = OCE" encoded in the driver's exception contract: every current and future caller of `EnsureConnectedAsync` must remember the filter idiom (the probe already has it; a fourth caller wouldn't). Converting at the source fixes the taxonomy once. Rejected as *sole* fix; kept as layers 2-3 because defense-in-depth here is nearly free and the filters also cover OCEs born *below* `EnsureConnectedAsync`. - **Retry inside `EnsureConnectedAsync` on timeout.** Rejected — retry/backoff policy is exactly the fleet-wide primitive R2-09 owns (STAB-8); baking an ad-hoc retry here would pre-empt it. - **Move S7 onto `PollGroupEngine` now (CONV-1)** — the fork's OCE handling disappears with the fork. Right end-state, wrong scope: PollGroupEngine v2 (backoff + onError absorption) is the report's remediation item #5 and is a multi-driver change. This plan's two-line filters are the safe immediate stop-loss; they do not conflict with a later migration. **Implementation steps** (exact code for each small bit): 1. `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs`, `EnsureConnectedAsync` — replace the `try/catch` around `OpenAsync` (`:1202-1212`) with: ```csharp try { using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); 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 */ } throw; } ``` 2. Same file, `ReadAsync` `:488` and `WriteAsync` `:1000` — change ```csharp catch (OperationCanceledException) { throw; } ``` to ```csharp // 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; } ``` 3. Same file, `PollLoopAsync` — add `when (ct.IsCancellationRequested)` to the three OCE catches (`:1383`, `:1396-1397`, `:1404`), e.g. the tick catch: ```csharp catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } catch (Exception ex) // now also receives any non-teardown OCE → backoff, not death { consecutiveFailures++; HandlePollFailure(ex, consecutiveFailures, initial: false); } ``` 4. Same file, `InitializeAsync` `:184-196` — mirror step 1's `when (!cancellationToken.IsCancellationRequested)` → `TimeoutException` conversion around the initial `OpenAsync`. **Tests** (all in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs`, extending the existing fakes — see Task 0; the driver's `Options()` fixture keeps `Timeout = 250ms` so hang-until-cancelled opens cost 250 ms each): - **T1 (the mandated first-and-failing regression test)** `Read_degrades_batch_on_connect_timeout_instead_of_throwing` — connection #1 drops (`NextReadThrows = PlcException(ConnectionError)` → marks dead), factory set to hang-until-cancelled opens; the next `ReadAsync` **currently throws `TaskCanceledException`**; expected: returns a `BadCommunicationError` batch + `Degraded` health, no throw. **FAILS at `f6eaa267`.** - **T2** `Subscription_poll_loop_survives_a_connect_timeout_outage_and_recovers` — subscribe at 100 ms; drop the socket; let ≥1 poll tick hit a hanging reconnect (`OpenHangsUntilCancelledCount = 2`); then restore normal opens; assert a Good `OnDataChange` arrives *after* a Bad one within a 10 s deadline (`TaskCompletionSource` + `WaitAsync`). **FAILS at `f6eaa267`** (loop exits on the first timeout tick; no recovery event ever fires). - **T3** `Write_degrades_batch_on_connect_timeout_instead_of_throwing` — T1's shape through `WriteAsync`. **FAILS at `f6eaa267`.** - **T4 (pinning)** `Read_still_propagates_caller_cancellation_during_connect` — options `Timeout = 5s`, hang-until-cancelled open, caller CTS cancelled after 50 ms; assert `OperationCanceledException` propagates. **PASSES before and after** — pins the filter direction so the fix can't over-rotate into swallowing real cancellation. **Effort:** S. **Risk/blast-radius:** Low. Four small catch-clause edits on one file; the happy path is untouched. Behavior deltas: (a) connect-timeout now yields `TimeoutException`/Bad-batch instead of OCE — strictly the documented intent of the Crit-3 degrade path; (b) poll loops survive timeouts with backoff — the regression closed. Callers seeing `TimeoutException` where OCE escaped before: only the poll loop (fixed to want this) and `DriverHostActor` dispatch (treats both as capability failure). --- ## Finding 2 — STAB-15 (High) — fatal classification misses framing/desync + cancellation-during-I/O; probe never marks dead **Restatement.** `IsS7ConnectionFatal` (`S7Driver.cs:1242-1252`) classifies only `SocketException`/`IOException`/`ObjectDisposedException` (inner-walking) and `PlcException{ConnectionError}`. Two gaps: **(a)** a caller-token cancellation mid-PDU (server request deadline, or a dispatch-layer Polly timeout now that `CapabilityInvoker` wraps driver calls) abandons a half-read ISO-on-TCP response on the socket; OCE is not classified, the handle is kept, and the next call reads stale bytes. S7.Net then surfaces framing faults (`TPKTInvalidException`, `TPDUInvalidException`, `WrongNumberOfBytesException`, `PlcException{WrongNumberReceivedBytes}`) — none classified fatal either — so the single gated connection stays **permanently desynchronized** (the Modbus STAB-3 mode rebuilt in S7). **(b)** The probe loop swallows every failure (`:1542`) without marking the handle dead, so its reconnect-backstop role only works when `TcpClient.Connected` flips false on its own; a wire-dead or desynced handle that still claims `Connected` is re-probed and re-fails forever (contrast TwinCAT's probe, which force-recycles on failure). **Verification.** Confirmed. `IsS7ConnectionFatal` body at `:1242-1252` matches the report's description exactly. Read path: no per-item OCE catch — OCE lands in `catch (Exception ex)` (`:536-543`), `MarkConnectionDeadIfFatal` no-ops, socket kept. Write path: explicit per-item OCE rethrow at `:1043-1048`, also without marking. Probe: gate-held body `:1529-1539`, teardown OCE filter `:1541`, bare swallow `:1542`; `MarkConnectionDeadIfFatal` is absent from the whole loop. S7.Net exception taxonomy verified against the 0.20.0 assembly (all three framing types public, public parameterless ctors — unit-constructible). One nuance the report under-specifies: the driver *itself* throws `System.IO.InvalidDataException` for a declared-type/address-size mismatch (`:973-975`), so the report's suggestion to classify `InvalidDataException` as a framing fault must be narrowed (see design). **Root cause.** The Crit-3 classifier was written from S7.Net's *connection-error* surface (socket-level types + `ConnectionError`) without walking its *framing/parsing* surface, and without the "cancellation abandons an in-flight PDU on a single serialized stream" insight the Modbus STAB-3 analysis had already produced. The probe predates the dead-handle flag and was never taught to feed it. **Proposed design.** - **(a1) Broaden the classifier to the framing surface.** Add to the inner-walk of `IsS7ConnectionFatal`: `TPKTInvalidException`, `TPDUInvalidException`, `WrongNumberOfBytesException`, and `PlcException{ErrorCode.WrongNumberReceivedBytes}`. A framing violation on ISO-on-TCP means the stream position is untrustworthy; only a reopen recovers. **Deliberately NOT `System.IO.InvalidDataException`:** the driver's own decode backstop (`:973-975`) throws it for a *config* mismatch on a healthy socket — classifying it would churn a full reopen on every read of a mis-authored tag. Residual risk accepted: if some S7.Net path signals desync *only* via `InvalidDataException`, the desynced stream's very next PDU still lands in TPKT/TPDU/WrongNumber territory, which now tears down — self-heal within one extra failed call. - **(a2) Cancellation observed during wire I/O marks the handle dead before propagating.** Add a per-item OCE catch in `ReadAsync`'s loop (`_plcDead = true; throw;`) and add `_plcDead = true;` to `WriteAsync`'s existing per-item OCE catch (`:1043-1048`). Both run under `_gate`, honouring `_plcDead`'s documented gate-guarded discipline. Marking is deliberately *unconditional* on OCE here (not routed through the type classifier): whether any bytes moved is unknowable from outside, and the worst case of a false positive is one cheap reopen — versus permanent desync for a false negative. **Behavior change (read path):** caller cancellation mid-read now propagates as OCE instead of being converted to a `BadCommunicationError` snapshot — consistent with `WriteAsync`, with the pre-loop `_gate.WaitAsync(cancellationToken)`, and with "the caller cancelled; nobody consumes the batch". - **(b) Probe marks dead under the gate.** Restructure the gate-held probe body with an inner try/catch so classification happens while `_gate` is still held (keeping `_plcDead` gate-guarded — the current outer catch sits outside the gate): teardown OCE rethrows to the outer filter; a probe-*timeout* OCE (deadline fired mid-`ReadStatusAsync` PDU) marks dead unconditionally (same abandonment argument as (a2)); any other exception goes through `MarkConnectionDeadIfFatal`. Net effect: a desynced/hung handle whose `TcpClient.Connected` lies is disposed and reopened by the *next probe tick's* `EnsureConnectedAsync` — the probe becomes the real backstop the Crit-3 design intended (mirrors TwinCAT's probe-recycle). *Alternatives considered:* - **Classify OCE inside `IsS7ConnectionFatal`** instead of at the catch sites — rejected: the classifier is also called for exceptions that *wrap* an OCE thrown before any I/O, and the ensure-wrapper OCE (connect timeout) has no live handle to mark; explicit `_plcDead = true` at the three I/O catch sites is smaller and keeps the classifier a pure type-taxonomy function. - **Force-recycle (dispose + null `Plc`) directly in the probe catch** à la TwinCAT — rejected: S7 already has the exactly-once-dispose discipline centralized in `EnsureConnectedAsync` via `_plcDead`; a second dispose site would reintroduce the race the flag was built to prevent. - **Blanket-classify `InvalidDataException`** (report's literal text) — rejected as unsafe for the reason in (a1); revisit only if the decode backstop at `:973-975` is retyped first. **Implementation steps:** 1. `S7Driver.cs`, `IsS7ConnectionFatal` (`:1242-1252`) — extend the walk: ```csharp for (Exception? e = ex; e is not null; e = e.InnerException) { if (e is System.Net.Sockets.SocketException or System.IO.IOException or ObjectDisposedException) 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 // 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; ``` Update the method's xmldoc to match. 2. `ReadAsync` per-item loop — insert **before** `catch (NotSupportedException)` (`:511`): ```csharp 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-15). Mark dead // (we hold _gate) and propagate: the caller cancelled, nobody consumes the batch. _plcDead = true; throw; } ``` 3. `WriteAsync` per-item OCE catch (`:1043-1048`) — add `_plcDead = true;` above the `throw;` (extend the comment with the same rationale). 4. `ProbeLoopAsync` (`:1516-1549`) — restructure the gate-held section: ```csharp await _gate.WaitAsync(probeCts.Token).ConfigureAwait(false); try { try { var plc = await EnsureConnectedAsync(probeCts.Token).ConfigureAwait(false); await plc.ReadStatusAsync(probeCts.Token).ConfigureAwait(false); success = true; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } // teardown → outer filter 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. _plcDead = true; } catch (Exception ex) { // STAB-15(b): 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(); } ``` The existing outer `catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; }` (`:1541`) and bare `catch { }` (`:1542`) stay — the outer bare catch now handles only gate-*wait* timeouts (gate contention is not a connection fault; correctly not marked) and the rethrown teardown OCE exits via the outer filter after `finally` releases the gate. **Tests** (same file/fakes as Finding 1): - **T5** `Framing_faults_are_classified_connection_fatal_and_reopen` — `[Theory]` over `TPKTInvalidException`, `TPDUInvalidException`, `WrongNumberOfBytesException`, `PlcException(ErrorCode.WrongNumberReceivedBytes)` as `NextReadThrows`; assert the *next* read opens connection #2 and reads Good (`factory.Created.Count == 2`). **FAILS at `f6eaa267`** (count stays 1). Companion negative: `PlcException(ErrorCode.ReadData)` still does **not** reopen (already covered by `Read_does_not_reopen_on_a_data_address_error` — keep green). - **T6** `Cancelled_read_marks_handle_dead_and_next_read_reopens` — fake read hangs on its token; caller cancels at 50 ms; assert OCE propagates **and** the next (fresh-token) read creates connection #2. **FAILS at `f6eaa267`** (OCE → `BadCommunicationError`, same handle reused). - **T7** `Cancelled_write_marks_handle_dead_and_next_read_reopens` — same via a hanging write (OCE already propagates today; the reopen assertion is what fails). **FAILS at `f6eaa267`.** - **T8** `Probe_failure_on_connected_socket_marks_dead_and_next_tick_reopens` — probe enabled (`Interval = 50ms`, `Timeout = 250ms`); connection #1's `ReadStatusAsync` throws `SocketException` persistently while `IsConnected` stays true; assert `factory.Created.Count >= 2` and a `Running` host transition within a bounded window (the probe's own next tick reopens via `EnsureConnectedAsync`). **FAILS at `f6eaa267`** (fast path returns the same lying handle forever). Variant in the same test class: **T9** `Probe_timeout_marks_dead` — `ReadStatusAsync` hangs on its token instead of throwing; same assertions. **FAILS at `f6eaa267`.** **Effort:** S-M (the probe restructure is the only non-trivial edit). **Risk/blast-radius:** Low-Medium, all inside `S7Driver.cs`. Deltas: (a) framing faults now cost one reopen instead of permanent desync — reopen churn is bounded by the fault actually recurring; (b) read-path caller cancellation now propagates (contract-consistent; the only in-repo poll-loop caller handles OCE via the Finding-1 filters); (c) probe can now trigger reopens — bounded to one reopen per failed probe tick, and only when the failure classifies fatal or times out (a healthy-but-slow PLC that *answers* within `Probe.Timeout` is unaffected). --- ## Finding 3 — STAB-8, S7-specific part (Medium, note-only here) — unthrottled per-call reconnect; the R2-09 seam **Restatement.** While the PLC is down, every data call and every probe tick pays a full TCP connect attempt bounded only by `_options.Timeout` (`EnsureConnectedAsync`, `:1187-1218`) — no attempt throttle. The poll loop's capped backoff (`:1393-1397`, `ComputeBackoffDelay` `:1447`) shields subscriptions only; server-initiated reads/writes and the probe retry unthrottled. **Verification.** Confirmed at the corrected refs (report's `:1188-1224` drifted; method is `:1187-1218`). Note the Finding-1/2 changes slightly *reduce* dead-PLC cost already: poll ticks that previously died (stopping their contribution) now back off to `PollBackoffCap` (30 s), and timeout-OCEs no longer escape as anomalous exceptions. **Proposed design — no throttle implemented in this plan.** The connect-attempt throttle is a fleet-wide primitive (S7, AbCip, FOCAS, TwinCAT all need it) and belongs to **R2-09** (PollGroupEngine v2 / reconnect-backoff consolidation). This plan only **documents the seam**: `EnsureConnectedAsync` is the single choke-point every S7 reconnect flows through (data calls + probe), already gate-serialized, so R2-09's throttle is a last-failed-attempt timestamp checked at the top of its slow path — one field + one early-throw, no structural change. A `` paragraph on `EnsureConnectedAsync` names this so R2-09's implementer (and any interim reader wondering about dead-PLC connect churn) lands here. *Alternative — implement the throttle now:* rejected; a driver-local knob would immediately become fleet debt and prejudge R2-09's shape (shared policy vs per-driver options). **Implementation steps:** extend the `EnsureConnectedAsync` xmldoc `` (`:1180-1184`) with the seam note (exact text in Task 10). **Tests:** none (doc-only). **Effort:** S (trivial). **Risk:** none. --- ## Verification bar (house thesis: "unit-green ≠ wired") **Deterministic unit guards** — every behavioral claim above has a fake-factory unit test that fails at `f6eaa267` and passes after (T1-T3, T5-T9), plus two pinning tests (T4, existing data-address-error test) proving the fixes didn't over-rotate. The fakes drive the *exact* production code paths (real `S7Driver`, real gate, real poll/probe loops) — only the wire is faked, and the STAB-14 fakes specifically make `OpenAsync` honour its token, which is the property the live rig cannot simulate cheaply. **Live gate (env-gated, operator-run)** — the existing bounce test cannot cover STAB-14/15 because `docker restart` produces connection-*refused* (instant `SocketException`), never connect-*timeout*. The timeout variant needs the SYN-blackhole shape: **packets dropped, not rejected**. Design (Task 11), mirroring `S7_RECONNECT_BOUNCE_CMD`'s double-gate: - New `S7_1500ConnectTimeoutOutageTests` in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/`, triple-gated on the sim fixture + two env vars: - `S7_TIMEOUT_OUTAGE_START_CMD` — begins dropping traffic to the sim port, e.g. `ssh dohertj2@10.100.0.35 "sudo iptables -I INPUT -p tcp --dport 1102 -j DROP"` - `S7_TIMEOUT_OUTAGE_STOP_CMD` — removes the rule (`-D INPUT …`). Absent vars ⇒ clean `Assert.Skip` (safe offline on macOS; a `finally` always runs the stop command once start succeeded, so the shared sim is never left blackholed). - Body: subscribe against the live sim (`lmxopcua-fix up s7 s7_1500` on 10.100.0.35) → Good baseline `OnDataChange` → run start-cmd → observe ≥1 Bad tick (each reconnect attempt now times out rather than refusing) → run stop-cmd → assert a Good `OnDataChange` resumes **on the same subscription** within 90 s. This is precisely the outage class STAB-14 killed; before the fix this test would hang at the recovery assertion. - A `docker pause otopcua-python-snap7-s7_1500` / `unpause` pair is an acceptable simpler START/STOP command choice (paused container = ACKless blackhole for established flows and unanswered SYNs for new connects) — document both in the test's remarks; the env-var design deliberately leaves the mechanism to the operator. Run recipe for the operator (also in the test xmldoc): ```bash lmxopcua-fix up s7 s7_1500 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"' dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests --filter "Category=Reconnect" ``` --- ## Task breakdown TDD discipline throughout: the failing test lands (and is *observed* failing) before the code it guards. Filter strings assume the repo root. Tasks 0-5 are Finding 1 (STAB-14); 6-9 are Finding 2 (STAB-15); 10-12 close out. **Task 1 is the mandated connect-timeout regression test and must be run and seen RED before any driver edit.** ### Task 0 — Extend the reconnect-test fakes with token-honouring hang modes **Classification:** trivial **Estimated implement time:** 5 min **Parallelizable with:** nothing (everything depends on it) **Files:** - Modify: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs` Steps: 1. In `FakeS7Plc`: add `public bool OpenHangsUntilCancelled { get; set; }`, `public bool ReadHangsUntilCancelled { get; set; }`, `public bool WriteHangsUntilCancelled { get; set; }`, `public Exception? ReadStatusThrows { get; set; }` (persistent, not one-shot), `public bool ReadStatusHangsUntilCancelled { get; set; }`. In each corresponding method, when the flag is set: `await Task.Delay(System.Threading.Timeout.Infinite, cancellationToken);` (throws `TaskCanceledException` when the token fires) / `throw ReadStatusThrows` — placed *before* the existing one-shot `*ThrowsOnce` handling. Convert the affected methods to `async Task`. 2. In `FakeS7PlcFactory`: add `public int OpenHangsUntilCancelledCount { get; set; }` (each `Create` decrements it and sets `plc.OpenHangsUntilCancelled = true` while positive) and `public Exception? ReadStatusThrowsForNewConnections { get; set; }` (copied onto each created plc; lets T8 arm connection #1 only by clearing it after init). 3. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~S7DriverReconnectTests"` — expected: **PASS** (existing 4 tests unaffected; compile-only change). 4. Commit: `test(s7): extend reconnect fakes with token-honouring hang + probe-fault modes (R2-01 task 0)` ### Task 1 — RED: the connect-timeout regression tests (T1 + T3) — MUST FAIL on current code **Classification:** small **Estimated implement time:** 5 min **Parallelizable with:** — **Files:** - Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs` Steps: 1. Write `Read_degrades_batch_on_connect_timeout_instead_of_throwing` (T1, Finding 1 Tests) and `Write_degrades_batch_on_connect_timeout_instead_of_throwing` (T3): drop connection #1 via `NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped")` + one read to mark dead; set `factory.OpenHangsUntilCancelledCount = int.MaxValue`; assert the next `ReadAsync`/`WriteAsync` does **not** throw, returns non-zero statuses, and health is `Degraded`. 2. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~connect_timeout"` — expected: **FAIL ×2** (`TaskCanceledException` escapes `ReadAsync`/`WriteAsync`). Record the failure output. 3. No commit yet (red tests commit together with Task 2's green). ### Task 2 — RED: the poll-loop-survival test (T2) + caller-cancel pinning test (T4) **Classification:** small **Estimated implement time:** 5 min **Parallelizable with:** — (same file as Task 1; sequence after it) **Files:** - Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs` Steps: 1. Write `Subscription_poll_loop_survives_a_connect_timeout_outage_and_recovers` (T2): subscribe `["W0"]` at 100 ms; hook `OnDataChange` — set `sawBad` on `e.Snapshot.StatusCode != 0u`, complete a `TaskCompletionSource` on the first Good snapshot after `sawBad`; drop connection #1; `OpenHangsUntilCancelledCount = 2`; await the TCS with `WaitAsync(TimeSpan.FromSeconds(10))`. 2. Write `Read_still_propagates_caller_cancellation_during_connect` (T4): options with `Timeout = TimeSpan.FromSeconds(5)`; mark dead; `OpenHangsUntilCancelledCount = 1`; caller `CancellationTokenSource` with `CancelAfter(50)`; `Should.ThrowAsync`. 3. Run the two new tests — expected: **T2 FAIL** (10 s timeout — the loop died on the timeout tick), **T4 PASS** (pins existing correct behavior). 4. No commit yet. ### Task 3 — GREEN: `EnsureConnectedAsync` + `InitializeAsync` timeout→`TimeoutException` conversion **Classification:** standard **Estimated implement time:** 5 min **Parallelizable with:** — **Files:** - Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`EnsureConnectedAsync` `:1202-1212`; `InitializeAsync` `:184-196`) Steps: 1. Apply Finding 1 implementation step 1 (exact code above) and step 4 (the `InitializeAsync` mirror — filter on `!cancellationToken.IsCancellationRequested`). 2. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~S7DriverReconnectTests"` — expected: **ALL PASS** (T1/T2/T3 go green; T4 + the four pre-existing tests stay green). 3. Commit: `fix(s7): surface connect-timeout as TimeoutException, not OCE — poll loops no longer die on unreachable-host outages (STAB-14, R2-01)` ### Task 4 — Defensive `when` filters: ensure-wrappers + poll-loop catches **Classification:** small **Estimated implement time:** 5 min **Parallelizable with:** Task 5 **Files:** - Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`:488`, `:1000`, `:1383`, `:1396-1397`, `:1404`) Steps: 1. Apply Finding 1 implementation steps 2 and 3 (exact code above). These are belt-and-suspenders behind Task 3 — no reachable path produces a non-teardown OCE any more, so no new red test exists; the deterministic guards are T2 (loop survival) and T4 (cancellation direction), which must both stay green. 2. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests` — expected: **PASS** (whole project). 3. Commit: `fix(s7): filter OCE catches on the owning token in read/write wrappers + poll loop (STAB-14 defense-in-depth, R2-01)` ### Task 5 — Full S7 unit-suite regression sweep for Finding 1 **Classification:** trivial **Estimated implement time:** 3 min **Parallelizable with:** Task 4 (can be its verify step); listed separately as the finding-close gate **Files:** none (test run only) Steps: 1. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests` and `dotnet test tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests` — expected: **PASS**. 2. No commit (gate only; if anything fails, fix within Tasks 3-4 scope before proceeding). ### Task 6 — RED: framing-fault classification tests (T5) **Classification:** small **Estimated implement time:** 5 min **Parallelizable with:** Task 8 (different finding sub-part, same file — prefer sequencing) **Files:** - Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs` Steps: 1. Write `[Theory]` `Framing_faults_are_classified_connection_fatal_and_reopen` with `[MemberData]`/switch over four cases: `new TPKTInvalidException()`, `new TPDUInvalidException()`, `new WrongNumberOfBytesException()`, `new PlcException(ErrorCode.WrongNumberReceivedBytes, "short read")` (all public ctors, verified against S7netplus 0.20.0). Shape: set as `NextReadThrows`; first read → non-zero status, `Created.Count` still 1; second read → **Good** and `Created.Count == 2`, `Created[0].Disposed` true. 2. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~Framing_faults"` — expected: **FAIL ×4** (`Created.Count` stays 1). 3. No commit yet. ### Task 7 — GREEN: broaden `IsS7ConnectionFatal` **Classification:** small **Estimated implement time:** 5 min **Parallelizable with:** — **Files:** - Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`IsS7ConnectionFatal` `:1242-1252` + its xmldoc `:1231-1241`) Steps: 1. Apply Finding 2 implementation step 1 (exact code above), including the NOT-`InvalidDataException` comment and the xmldoc update naming the framing surface. 2. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~S7DriverReconnectTests"` — expected: **PASS** (T5 ×4 green; `Read_does_not_reopen_on_a_data_address_error` still green — the negative control). 3. Commit: `fix(s7): classify ISO-on-TCP framing/desync faults as connection-fatal (STAB-15a, R2-01)` ### Task 8 — RED→GREEN: cancellation-during-I/O marks the handle dead (T6 + T7) **Classification:** standard **Estimated implement time:** 5 min (tests) + 5 min (fix) — two sittings of ≤5 min **Parallelizable with:** — **Files:** - Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs` - Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`ReadAsync` per-item loop, insert before `:511`; `WriteAsync` OCE catch `:1043-1048`) Steps: 1. Write `Cancelled_read_marks_handle_dead_and_next_read_reopens` (T6): `Created[0].ReadHangsUntilCancelled = true`; caller CTS `CancelAfter(50)`; assert `Should.ThrowAsync` on the read; then a fresh-token read → Good, `Created.Count == 2`. Write `Cancelled_write_marks_handle_dead_and_next_read_reopens` (T7) with `WriteHangsUntilCancelled`. 2. Run `--filter "FullyQualifiedName~marks_handle_dead"` — expected: **FAIL ×2** (T6 additionally fails its throw assertion today — the read path converts OCE to a Bad snapshot; T7 fails the reopen assertion). 3. Apply Finding 2 implementation steps 2 and 3 (exact code above). 4. Re-run — expected: **PASS ×2**; then the whole `S7DriverReconnectTests` class — **PASS**. 5. Commit: `fix(s7): cancellation observed mid-PDU marks the connection dead before propagating (STAB-15a, R2-01)` ### Task 9 — RED→GREEN: probe marks the handle dead (T8 + T9) **Classification:** standard **Estimated implement time:** 5 min (tests) + 5 min (fix) — two sittings of ≤5 min **Parallelizable with:** — **Files:** - Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs` - Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`ProbeLoopAsync` `:1516-1549`) Steps: 1. Write `Probe_failure_on_connected_socket_marks_dead_and_next_tick_reopens` (T8): probe-enabled options (`Probe = new S7ProbeOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(50), Timeout = TimeSpan.FromMilliseconds(250) }`); after init, arm connection #1 with persistent `ReadStatusThrows = new SocketException()` (leave later connections healthy); poll `factory.Created.Count` under a 5 s deadline until `>= 2`, then assert a subsequent `GetHostStatuses()[0].State == HostState.Running` within a further bounded window. Write `Probe_timeout_marks_dead` (T9): same shape with `ReadStatusHangsUntilCancelled = true` on connection #1. 2. Run `--filter "FullyQualifiedName~Probe_"` (scoped to the reconnect class) — expected: **FAIL ×2** (`Created.Count` pinned at 1 — the lying handle is re-probed forever). 3. Apply Finding 2 implementation step 4 (exact probe restructure above). 4. Re-run — expected: **PASS ×2**; whole test project — **PASS**. 5. Commit: `fix(s7): probe failures mark the handle dead under the gate so the next tick reopens (STAB-15b, R2-01)` ### Task 10 — Doc: the R2-09 connect-throttle seam note on `EnsureConnectedAsync` **Classification:** trivial **Estimated implement time:** 3 min **Parallelizable with:** Tasks 6-9 **Files:** - Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`EnsureConnectedAsync` `` `:1180-1184`) Steps: 1. Append to the ``: `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.` 2. Build-only check (`dotnet build src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7`) — expected: **PASS**. 3. Commit: `docs(s7): mark EnsureConnectedAsync as the R2-09 connect-throttle seam (STAB-8 note, R2-01)` ### Task 11 — Env-gated live connect-timeout outage test (the STAB-14 live gate) **Classification:** standard **Estimated implement time:** 5 min (authoring; the live run is a separate operator action) **Parallelizable with:** Tasks 6-10 **Files:** - Create: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ConnectTimeoutOutageTests.cs` Steps: 1. Author per the Verification-bar design: `[Collection(Snap7ServerCollection.Name)]`, `[Trait("Category","Integration")]`, `[Trait("Category","Reconnect")]`, `[Trait("Device","S7_1500")]`; triple-gated skip on `sim.SkipReason` + `S7_TIMEOUT_OUTAGE_START_CMD` + `S7_TIMEOUT_OUTAGE_STOP_CMD`; reuse `S7_1500ReconnectTests.RunBounceCommandAsync`'s shell-exec shape (copy the private helper — or hoist it to the fixture if trivial); `try/finally` guarantees STOP runs once START succeeded. Body: subscribe → Good baseline → START (blackhole) → observe ≥1 Bad `OnDataChange` → STOP → await a Good `OnDataChange` on the same subscription within 90 s. Xmldoc carries the operator recipe (docker pause/unpause and iptables DROP variants) and the explicit rationale: `docker restart` = connection-refused; this test exists because STAB-14 only fires on connect-*timeout*. 2. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests --filter "FullyQualifiedName~ConnectTimeoutOutage"` locally (no env vars) — expected: **SKIP (cleanly)**. 3. Commit: `test(s7): env-gated live connect-timeout outage test — the STAB-14 outage class the bounce test cannot produce (R2-01)` 4. Note for the operator (not a CI step): run the recipe from the Verification-bar section against the 10.100.0.35 fixture (`lmxopcua-fix up s7 s7_1500`) before trusting the fix live. ### Task 12 — Close-out: whole-solution S7-adjacent sweep + bookkeeping **Classification:** trivial **Estimated implement time:** 5 min **Parallelizable with:** — **Files:** - Modify: `archreview/plans/STATUS.md` (mark R2-01 done, note the live-gate env vars) - Modify: `archreview/05-protocol-drivers.md` (prior-finding table: STAB-14/15 → FIXED with commit refs; STAB-8 row: note the documented S7 seam) Steps: 1. Run `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests` and a solution build — expected: **PASS**. 2. Update the two archreview files (working-tree convention — these carry uncommitted re-review state; follow whatever commit discipline the archreview docs branch is using at execution time). 3. Commit: `docs(archreview): R2-01 S7 fault-path hardening complete — STAB-14/15 fixed, STAB-8 seam documented` --- ## Effort & risk summary | Finding | Effort | Risk / blast radius | |---|---|---| | STAB-14 (Tasks 0-5) | **S** | Low — four catch-clause edits, one file; regression guarded by T1-T4 + live gate | | STAB-15 (Tasks 6-9) | **S-M** | Low-Medium — classifier + three catch sites + probe restructure, one file; guarded by T5-T9 + negative controls | | 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).