Merge R2-01 S7 fault-path hardening (arch-review round 2) [PR #429]
v2-ci / build (push) Successful in 3m37s
v2-ci / unit-tests (push) Failing after 6m49s

Findings 05/STAB-14 (High regression — connect-timeout OCE no longer silently
kills S7 subscriptions), STAB-15 (framing/desync + mid-PDU cancel fatal
classification), STAB-8 seam note. All prod changes in S7Driver.cs; T11 live
outage gate deferred. STATUS.md conflict resolved additively. Build clean.
This commit is contained in:
Joseph Doherty
2026-07-13 10:14:59 -04:00
7 changed files with 654 additions and 42 deletions
+21
View File
@@ -167,6 +167,13 @@ are the new STAB-14..17 below.
### 1. Stability
> **REMEDIATED by R2-01 (branch `r2/01-s7-fault-hardening`).** Connect-timeout now surfaces as a
> `TimeoutException` at the `EnsureConnectedAsync`/`InitializeAsync` source (not an escaping OCE), plus
> `when (token.IsCancellationRequested)` filters on the read/write ensure-wrappers (`:488`/`:1000`) and all
> three poll-loop OCE catches — so a non-teardown OCE degrades/backs-off instead of killing the loop. Unit
> guards T1T4 (`S7DriverReconnectTests`, incl. the poll-loop-survival regression). Live gate
> `S7_1500ConnectTimeoutOutageTests` (env-gated SYN-blackhole, `deferred-live`).
**STAB-14 — High — NEW — S7's reconnect wrapper rethrows connect-timeout
cancellation, and the poll loop's bare OCE catch then permanently kills the
subscription.** `EnsureConnectedAsync` bounds the reopen with a linked
@@ -192,6 +199,14 @@ degrade-to-`BadCommunicationError` path), and filter the poll loop's OCE
catches on `ct.IsCancellationRequested`; add a fake-factory unit test where
`OpenAsync` honours a token that only the `CancelAfter` fires.
> **REMEDIATED by R2-01 (branch `r2/01-s7-fault-hardening`).** `IsS7ConnectionFatal` broadened to the
> ISO-on-TCP framing surface (`TPKTInvalidException`/`TPDUInvalidException`/`WrongNumberOfBytesException` +
> `PlcException{WrongNumberReceivedBytes}`; deliberately NOT `InvalidDataException` — that is the driver's own
> config-mismatch decode backstop on a healthy socket). Cancellation observed mid-PDU on read + write now
> marks `_plcDead` before propagating; the probe restructured to classify + mark dead under `_gate` so the next
> tick reopens a lying/hung handle. Unit guards T5T9 (`S7DriverReconnectTests`) + negative control
> `Read_does_not_reopen_on_a_data_address_error`.
**STAB-15 — High — NEW — S7's connection-fatal classification misses
framing/desync errors and cancellation-during-I/O, reproducing Modbus's
STAB-3 desync; and the probe never marks the handle dead.**
@@ -340,6 +355,12 @@ S7's `ComputeBackoffDelay`, `S7Driver.cs:1447`) and a small connect-attempt
throttle to the lazy-reconnect drivers (S7's `EnsureConnectedAsync` is now
the natural home for a last-attempt timestamp).
> **SEAM DOCUMENTED by R2-01 (branch `r2/01-s7-fault-hardening`).** The S7 connect-throttle is left to
> R2-09 (fleet-wide reconnect-backoff), not implemented driver-locally. `EnsureConnectedAsync`'s `<remarks>`
> now names itself as the single gate-serialized choke-point where a last-failed-attempt timestamp plugs in.
> (Note: R2-01's STAB-14/15 fixes already *reduce* dead-PLC cost — poll ticks that used to die now back off to
> `PollBackoffCap`, and connect-timeouts no longer escape as anomalous OCEs.)
**STAB-9 — Medium — `PollGroupEngine`'s `onError` sink is dead code: no
driver passes it.** All five consumers construct the engine without the
error callback (`ModbusDriver.cs:115`, `AbCipDriver.cs:133`,
@@ -730,3 +730,7 @@ Steps:
| STAB-8 note (Task 10) | **S** (trivial) | None — doc only |
| Live gate + close-out (Tasks 11-12) | **S** | None locally (env-gated skip); live run is operator-driven |
| **Overall** | **S-M** (~2-3 h implementer wall-time) | Single-file production change (`S7Driver.cs`); no contract/DI/schema changes |
## Execution deviations (R2-01)
- **Pre-existing, out-of-scope CLI test failure (Task 5/12 gate).** `SubscribeCommandConsoleHandlerCommentTests.SubscribeCommand_explains_why_OnDataChange_uses_console_Output_synchronously` in `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests` FAILS at the branch base (`1676c8f4`) — the source-scanning assertion expects the literal phrase `"CliFx console"` in `SubscribeCommand.cs`, which commit `9cad9ed0` (docs: strip tracking-ID comments) removed. R2-01 never touches the CLI project (`git diff --stat 1676c8f4 HEAD -- src/Drivers/Cli tests/Drivers/Cli` is empty). Left as-is: not caused by, nor in scope for, S7 fault-path hardening. The rest of the S7 CLI suite is green (48/49). The S7 unit suite (`Driver.S7.Tests`) is fully green (238/238).
@@ -4,81 +4,83 @@
{
"id": 0,
"subject": "Extend reconnect-test fakes with token-honouring hang + probe-fault modes (FakeS7Plc/FakeS7PlcFactory)",
"status": "pending",
"status": "completed",
"blockedBy": []
},
{
"id": 1,
"subject": "RED: connect-timeout regression tests T1+T3 (read/write degrade instead of throwing OCE) — must FAIL at f6eaa267",
"status": "pending",
"status": "completed",
"blockedBy": [0]
},
{
"id": 2,
"subject": "RED: poll-loop-survival test T2 (subscription survives connect-timeout outage) + caller-cancel pinning test T4",
"status": "pending",
"status": "completed",
"blockedBy": [0, 1]
},
{
"id": 3,
"subject": "GREEN: EnsureConnectedAsync + InitializeAsync connect-timeout OCE -> TimeoutException conversion (STAB-14 root fix)",
"status": "pending",
"status": "completed",
"blockedBy": [1, 2]
},
{
"id": 4,
"subject": "Defensive when-filters on ensure-wrapper OCE rethrows (:488/:1000) + poll-loop OCE catches (:1383/:1396/:1404)",
"status": "pending",
"status": "completed",
"blockedBy": [3]
},
{
"id": 5,
"subject": "Finding-1 close gate: full S7 unit + CLI test-project sweep green",
"status": "pending",
"status": "completed",
"blockedBy": [4]
},
{
"id": 6,
"subject": "RED: framing-fault classification Theory T5 (TPKT/TPDU/WrongNumberOfBytes/PlcException-WrongNumberReceivedBytes reopen) — must FAIL",
"status": "pending",
"status": "completed",
"blockedBy": [0]
},
{
"id": 7,
"subject": "GREEN: broaden IsS7ConnectionFatal to the ISO-on-TCP framing surface (NOT InvalidDataException) + xmldoc (STAB-15a)",
"status": "pending",
"status": "completed",
"blockedBy": [6]
},
{
"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]
},
{
"id": 9,
"subject": "RED->GREEN: probe marks handle dead under the gate (T8 persistent probe fault + T9 probe timeout; ProbeLoopAsync inner catch restructure, STAB-15b)",
"status": "pending",
"status": "completed",
"blockedBy": [8]
},
{
"id": 10,
"subject": "Doc: R2-09 connect-throttle seam note on EnsureConnectedAsync <remarks> (STAB-8 S7 part, note-only)",
"status": "pending",
"status": "completed",
"blockedBy": [3]
},
{
"id": 11,
"subject": "Env-gated live connect-timeout outage test (S7_TIMEOUT_OUTAGE_START_CMD/STOP_CMD blackhole design; skips cleanly offline)",
"status": "pending",
"status": "deferred-live",
"note": "env-gated; runs in serial live pass (integration suite; serialized heavy pass). Test authored + committed; verified it SKIPs cleanly offline.",
"blockedBy": [5]
},
{
"id": 12,
"subject": "Close-out: whole-suite sweep + update archreview STATUS.md and 05 prior-finding table (STAB-14/15 FIXED, STAB-8 seam noted)",
"status": "pending",
"status": "completed",
"note": "S7 unit 246/246 green; S7 CLI 48/49 (1 pre-existing unrelated comment-scan failure, see deviations); solution BUILD clean (0 errors). Whole-solution dotnet test NOT run per controller memory-constraint (integration suites deferred to serial heavy pass).",
"blockedBy": [5, 7, 8, 9, 10, 11]
}
],
"lastUpdated": "2026-07-12"
"lastUpdated": "2026-07-13"
}
+1
View File
@@ -171,3 +171,4 @@ what the unit leg cannot; (c) migrate the 3 xunit v2 holdouts once Akka ships an
|---|---|---|---|---|
| **R2-06** | 06/S-11 (rank 7), 06/U-7 (rank 8), 06/C-7 | `r2/06-serverhistorian-failfast` (off `1676c8f4`) | `3a049a13``4640ecb1` (10 task commits) | **Code-complete, offline-verified.** S-11: fail-fast `ServerHistorianOptionsValidator` (consumer-gated on `ServerHistorian:Enabled` OR `AlarmHistorian:Enabled`, fails empty/non-http(s) Endpoint at startup) + `AddValidatedOptions` wiring + adapter defense-in-depth (`TryCreate`→named `InvalidOperationException`, no more raw `UriFormatException` crash-loop) + `Validate()` warning parity + alarm-only-mode startup warning. U-7: pinned Boolean→Int1 explicit-presence (`HasDataType`) + `HISTGW_BOOL_SANDBOX_TAG` live Boolean EnsureTags leg + retype probe (skip-clean offline) + upsert/retype migration note (`docs/Historian.md`). C-7: keep-populating decision + dead-`Quality` doc comments (0.2.0 C-002). **Live leg operator-gated (T12) — pending a VPN run on a 0.2.0 gateway** (records the observed in-place retype policy back into `docs/Historian.md`; also discharges 06/U-2's full documented run). |
| **R2-05** | 04/C-1 (High) | `r2/05-adminui-authz` (off `1676c8f4`) | `183b72b7``6c1f05cb` (5 commits) | **Code-complete, offline-verified.** All 38 routable AdminUI pages carry an explicit named-policy `@attribute` via new `AdminUiPolicies` constants (`Security/Auth/`). New `ConfigEditor` (Administrator+Designer) gates the 20-page config-authoring surface incl. the 8 driver pages that author live `ResilienceConfig` + `/api/script-analysis/*`; new `AuthenticatedRead` on the 16 read pages; `FleetAdmin`/`DriverOperator` unchanged (literals→constants). Anti-drift guard `PageAuthorizationGuardTests` (bans bare `[Authorize]`/`Roles=`/unknown-policy/unclassified) + `AdminUiPoliciesTests` semantics matrix. Corrections folded in: Reservations + ClusterRedundancy read-only (→`AuthenticatedRead`); Home had no attribute (now `AuthenticatedRead`). AdminUI.Tests 517/517, Security.Tests 80/80. **Live `/run` positive pass deferred (T15)** — docker-dev `DisableLogin=true` auto-admin can't observe deny; negative proof is CI-side (both guard tests). |
| **R2-01** | 05/STAB-14 (High regression, rank 1), 05/STAB-15 (rank 3), 05/STAB-8 seam | `r2/01-s7-fault-hardening` (off `1676c8f4`) | `fcdcf0e2``bdb7ed41` (13 task commits) | **Code-complete, offline-verified.** All prod changes in `S7Driver.cs`. STAB-14: connect-timeout surfaces as `TimeoutException` (not an escaping OCE) at the `EnsureConnectedAsync`/`InitializeAsync` source + `when(token.IsCancellationRequested)` filters on the read/write ensure-wrappers and all three poll-loop OCE catches — subscription poll loops survive an unreachable-host outage and recover (T2 regression RED→GREEN). STAB-15: `IsS7ConnectionFatal` broadened to the ISO-on-TCP framing surface (TPKT/TPDU/WrongNumberOfBytes/`PlcException{WrongNumberReceivedBytes}`, NOT `InvalidDataException`); mid-PDU cancel marks handle dead; probe classifies+marks-dead under the gate. STAB-8 (S7 part): `EnsureConnectedAsync` documented as the R2-09 connect-throttle seam (note-only). Driver.S7.Tests 246/246. **Live outage gate deferred (T11)**`S7_1500ConnectTimeoutOutageTests` (env `S7_TIMEOUT_OUTAGE_START_CMD`/`STOP_CMD`, SYN-blackhole) skips clean offline; the connect-*timeout* class the `docker restart` bounce can't produce. (Pre-existing unrelated CLI docs-scan test fails at base — out of scope.) |