dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity
Files
b94ea3d412a29bf095999e8c51b42e6de42aafb7
lmxopcua/archreview/plans/R2-01-s7-fault-hardening-plan.md
T

47 KiB
Raw Blame History

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:

    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

    catch (OperationCanceledException) { throw; }

    to

    // 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:

    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:

    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):

    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:

    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 <remarks> 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 <remarks> (: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):

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<OperationCanceledException>.
  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<OperationCanceledException> 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 <remarks> :1180-1184)

Steps:

  1. Append to the <remarks>: 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).
Reference in New Issue View Git Blame Copy Permalink