1891f5d6a7
Re-ran all seven domain reviews at master f6eaa267 (reports rewritten in
place, each with a prior-finding status table): all 4 round-1 Criticals
verified closed; new top findings are the S7 connect-timeout OCE regression
(05/STAB-14), the ResilienceConfig operator-authorable brick (01/S-6), and
a batch of resilience-seam Mediums. 00-OVERALL.md carries the updated
maturity matrix + 12-item action list.
Adds R2-01..R2-12 design/implementation plans (one per action item, house
format + bite-sized TDD task breakdowns + co-located .tasks.json; 193 tasks,
~18-24 dev-days). STATUS.md updated: round-1 topology marked historical
(all merged+pushed), re-review findings table + plan pointers added.
572 lines
69 KiB
Markdown
572 lines
69 KiB
Markdown
# Design + Implementation Plan — R2-09 Driver Fleet Carried Batch
|
||
|
||
- **Source report:** `archreview/05-protocol-drivers.md` (2026-07-12 re-review)
|
||
- **Overall action item:** #9 in `archreview/00-OVERALL.md` — "Driver fleet carried batch: Modbus fatal-classification, AbCip/FOCAS locking, `ResolveHost`, `onError`, TwinCAT replay retry + Bad-quality"
|
||
- **Plan verified against tree at:** `f6eaa267` (master, clean) — every anchor below re-opened and re-located in the working tree on 2026-07-12
|
||
- **Round-1 plan built on:** `archreview/plans/05-protocol-drivers-plan.md` Parts A (A3/A4/A5) and B (B1 PollGroupEngine v2, B3 ResolveHost-via-resolver, B4 ConnectionBackoff) — designs verified still valid and extended below
|
||
- **Scope:** `src/Drivers/` fleet findings — 05/STAB-3, 05/STAB-4, 05/STAB-5, 05/STAB-8 (fleet part), 05/STAB-9 + CONV-1 + PERF-3 + STAB-6-S7 (PollGroupEngine v2 batch), CONV-4 (`ResolveHost`), 05/STAB-16, 05/STAB-17, 05/STAB-12 (TwinCAT-compounded part only)
|
||
- **Scope boundary (do NOT duplicate):**
|
||
- **Plan R2-01** owns the S7-specific OCE/fatal-classification fixes (STAB-14 ensure-wrapper OCE filter at `S7Driver.cs:488/:1000`, STAB-15 framing/cancel fatal-classification + probe mark-dead, and the S7 `EnsureConnectedAsync` attempt-throttle wiring). This plan hands R2-01 two seams: the `ConnectionBackoff` primitive (sub-batch B3/B4) and the engine-level OCE hardening that makes the S7 fork retirement safe (B3). Sequencing notes inline.
|
||
- **Plan R2-11** owns `ReadEnum`/TagConfig parse-strictness consolidation (CONV-2/UNDER-6). B5 adds exactly one key (`unitId`) to `ModbusEquipmentTagParser` because per-host breaker keying is impossible without it; all other parser-parity work stays with R2-11.
|
||
|
||
---
|
||
|
||
## Verification summary
|
||
|
||
Every finding was re-opened at its cited file:line in the working tree at `f6eaa267`. **All 10 findings CONFIRMED; 2 anchors drifted (details below); 0 stale.**
|
||
|
||
| Finding | Cited anchor | Verified at `f6eaa267` |
|
||
|---|---|---|
|
||
| STAB-3 | `ModbusTcpTransport.cs:155-165, 233-235, 257-261` | **EXACT** — retry catch `:155-165` (only `IsSocketLevelFailure`), per-op linked CTS `CancelAfter` `:226-227`, TxId-mismatch `InvalidDataException` `:233-235`, truncated-length `:236-237`, `IsSocketLevelFailure` `:257-261`, `TearDownAsync` `:263-270` |
|
||
| STAB-4 | `AbCipDriver.cs:544-575, 614-637, 708-726` (prompt's "~548-579 +4" was conservative) | **EXACT to the report** — `ReadSingleAsync` critical section `:544-575`, `ReadGroupAsync` `:614-637`, `WriteAsync` Encode→Write→GetStatus `:708-726`; zero runtime locking (only `GetRmwLock` at `DeviceState:1243`, bit-RMW only); alarm-projection read loop `AbCipAlarmProjection.cs:230-232` hits the same handles; AbLegacy's guard confirmed at `AbLegacyDriver.cs:227-244` (read) / `:331-342` (write) / `_runtimeLocks` + `GetRuntimeLock` `:773-787` / dispose `:841-842` |
|
||
| STAB-5 | `FocasDriver.cs:1184-1194, 752-839, 912-945` | **EXACT** — plain `Dictionary` fields `LastFixedSnapshots :1184`, `LastTimers :1190`, `LastServoLoads :1192`, `LastSpindleLoads :1194`; loop writes `:700, :752, :793, :836-839, :862-863, :874`; reader-thread reads `:878, :912, :920, :945`; contrast `_parsedAddressesByTagName` already `ConcurrentDictionary` `:40` |
|
||
| STAB-9 (dead `onError`) | 5 ctor sites | **EXACT** — `ModbusDriver.cs:115`, `AbCipDriver.cs:133`, `AbLegacyDriver.cs:65`, `TwinCATDriver.cs:69`, `FocasDriver.cs:72` — all five pass `reader:`/`onChange:` only; `PollGroupEngine.cs` `onError` param `:59`, `ReportError` `:164-169` dead fleet-wide |
|
||
| CONV-1/PERF-3/STAB-6-S7 (S7 fork) | `S7Driver.cs:1373-1412, 1447, 1469, 1350-1354` | **EXACT** — fork `PollLoopAsync :1373-1412` (bare `catch (OCE) { return; }` `:1383, :1397, :1404`), `HandlePollFailure :1421-1437`, `ComputeBackoffDelay :1447-1456`, reference-equality diff `:1469`, `UnsubscribeAsync` cancel+dispose-without-drain `:1350-1354`. Engine already has everything the fork lacks: `StopState` drain `PollGroupEngine.cs:99-112`, structural array compare `:203-209`, `ObjectDisposedException` guard `:136` — and lacks what the fork has: backoff + failure→health |
|
||
| STAB-8 (fleet) | AbCip `:891-897`, FOCAS `:1089-1120`, TwinCAT `:706-721` | **CONFIRMED** — `EvictRuntime` `AbCipDriver.cs:891-897` + `EnsureTagRuntimeAsync :852-882` recreate at full Forward-Open cost per call; FOCAS `EnsureConnectedAsync :1089-1120` full two-socket reconnect per call, no throttle; TwinCAT `EnsureConnectedAsync :706-721` / `EnsureConnectedUnderGateAsync :731-775` connect per call, no throttle. Only Modbus transport (`ConnectWithBackoffAsync :179-208`) and the S7 poll loop have any backoff |
|
||
| CONV-4 (`ResolveHost`) | 5 sites | **EXACT** — `AbCipDriver.cs:483-488`, `AbLegacyDriver.cs:496-501`, `TwinCATDriver.cs:689-696` (falls back to `UnresolvedHostSentinel` `""`, `:686`), `FocasDriver.cs:1082-1087`, `ModbusDriver.cs:125-131` (`BuildSlaveHostName` `:134`, `ResolveUnitId` `:474`). All consult `_tagsByName` only. All four multi-device `TDef`s carry `DeviceHostAddress` and all four equipment parsers parse `deviceHostAddress` (AbCip parser `:39`, AbLegacy `:42`, TwinCAT `:30`, FOCAS `:32` — FOCAS defaults to `""` when absent). `ModbusEquipmentTagParser` has **no** `unitId` key (grep-confirmed) |
|
||
| STAB-16 | `TwinCATDriver.cs:801-816` | **CONFIRMED, minor drift** — the replay-failure catch is `:801-807` (log-only; `SwapHandle` runs only in the success path `:797`), summary log `:810-813`. Failed intent keeps its dead handle, `OnChange` is `Action<string, object?>` (no quality channel, `:575`), replay runs only from `EnsureConnectedUnderGateAsync :773` (i.e. only when a NEW client is installed — a healthy client never retries) |
|
||
| STAB-17 | `TwinCATDriver.cs:608-613, 786-803` | **EXACT** — `NativeRegistration.Dispose :607-613` is gate-free (registry `TryRemove :610`, handle exchange+dispose `:611-612`); replay snapshots intents `:786` and `SwapHandle`s `:797` with no ownership re-check, so a dispose between snapshot and swap leaves the fresh handle ownerless and the ADS notification live |
|
||
| STAB-12 (compounded part) | `TwinCATDriver.cs:835` | **DRIFTED −9** — the `ConnectGate.WaitAsync(CancellationToken.None)` is at `TwinCATDriver.cs:826` (method `RecycleClientAsync :824-840`); callers are the probe loop `:640` and `:648`, both holding a live `ct` |
|
||
|
||
**Drifted anchors (2):** STAB-12's `RecycleClientAsync` gate-wait `:835` → **`:826`**; STAB-16's cited span `:801-816` is really catch `:801-807` + summary log `:810-813` (same method, cosmetic). Everything else verified exact.
|
||
|
||
**Test infrastructure verified:** `PollGroupEngineTests.cs` (Core.Abstractions.Tests), `ModbusTcpReconnectTests.cs` with in-proc `FlakeyModbusServer` (TcpListener-based — extendable for stall/wrong-TxId/truncation), `AbLegacyRuntimeConcurrencyTests.cs` (the template for B2), `FakeAbCipTag.cs`, `FakeFocasClient.cs`, `TwinCATReconnectReplayTests.cs` (fake `ITwinCATClient`), `S7DriverReconnectTests.cs` + `S7DiscoveryAndSubscribeTests.cs` (fake `IS7PlcFactory`), env-gated `S7_1500ReconnectTests.cs`.
|
||
|
||
**Live gates:** docker fixtures on `10.100.0.35` via `lmxopcua-fix`: **modbus** (`:5020`, `lmxopcua-fix up modbus standard`; the `exception_injector` container can force write/FC failures — see the write-outcome memory recipe), **abcip** (`:44818`, `up abcip controllogix`), **s7** (`:1102`, `up s7 s7_1500`; bounce = `down`/`up` for reconnect recipes), **opcuaclient** (`:50000`). **TwinCAT, FOCAS, and AbLegacy have NO fixture** — the fake-client unit suites are authoritative there (accepted in the STAB-2 remediation and re-affirmed by UNDER-7).
|
||
|
||
---
|
||
|
||
# Sub-batch B1 — Modbus transport fatal-classification (STAB-3, High)
|
||
|
||
*Independently shippable. No dependencies.*
|
||
|
||
## STAB-3 — Modbus transport desynchronizes permanently after a response timeout or TxId mismatch
|
||
|
||
**Restatement:** a per-op timeout (`OperationCanceledException` from the linked `CancelAfter` CTS) or a framing violation (TxId mismatch / truncated header → `InvalidDataException`) is not classified socket-fatal, so the single-flight socket is kept with a half-read or still-in-flight response. The next transaction reads the stale bytes, throws on the TxId guard — also not socket-fatal — and the stream stays desynchronized forever (until a real socket error or the default-off idle-disconnect).
|
||
|
||
**Verification:** CONFIRMED at `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs`. `SendAsync`'s reconnect-retry catch (`:155`) filters on `IsSocketLevelFailure` (`:257-261` — only `EndOfStreamException`/`IOException`/`SocketException`/`ObjectDisposedException`). The per-op deadline is a linked CTS `CancelAfter(_timeout)` (`:226-227`) whose cancellation surfaces as OCE; the TxId guard throws `InvalidDataException` (`:234-235`); the length guard throws `InvalidDataException` (`:237`). None reach `TearDownAsync` (`:263-270`). Note the 05 report explicitly pairs this with S7's STAB-15 ("same defect class in two drivers") — the S7 side is **R2-01's**; only the Modbus transport is fixed here.
|
||
|
||
**Root cause:** the transport conflates "protocol-layer error, socket still coherent" (Modbus exception PDU — correctly kept) with "framing/timeout error, unknown bytes may be in flight — the socket is unusable". Timeout and framing violations require teardown regardless of exception type.
|
||
|
||
**Proposed design** (unchanged from round-1 plan A3, verified still correct): treat per-op timeout and any framing violation as **connection-fatal — tear down before propagating**, normalized as an `IOException` subclass so the existing retry classifier covers it with zero changes:
|
||
|
||
- New `ModbusTransportDesyncException : IOException` (carries the desync reason: `Timeout`, `TxIdMismatch`, `TruncatedFrame`).
|
||
- In `SendOnceAsync` (`:210-250`), wrap the write/read/parse body:
|
||
- `catch (OperationCanceledException) when (!ct.IsCancellationRequested)` — the linked timeout fired, not the caller ⇒ `await TearDownAsync()` ⇒ `throw new ModbusTransportDesyncException(...)`. **Caller cancellation (`ct.IsCancellationRequested`) propagates as OCE with NO teardown** — a legitimate shutdown must not look like a desync.
|
||
- `catch (InvalidDataException ide)` ⇒ `await TearDownAsync()` ⇒ `throw new ModbusTransportDesyncException(..., ide)`.
|
||
- Because `ModbusTransportDesyncException` **is** an `IOException`, `SendAsync`'s existing single-retry (`:155-165`) now reconnects and resends once when `_autoReconnect` is on; when `_autoReconnect` is off the socket is still torn down (mandatory — a desynced socket must never be reused) and the exception propagates to the driver's status mapping (`BadCommunicationError`).
|
||
|
||
*Alternative considered:* add `OperationCanceledException`/`InvalidDataException` to `IsSocketLevelFailure` — rejected: the retry catch runs teardown only when auto-reconnect is on, and OCE must be split caller-vs-timeout at the throw site anyway. The teardown-at-throw-site design covers auto-reconnect-off and keeps `IsSocketLevelFailure` a pure type test.
|
||
|
||
**Implementation steps:**
|
||
1. `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTransportDesyncException.cs` — new file, `sealed class ModbusTransportDesyncException : IOException`.
|
||
2. `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs` — wrap `SendOnceAsync` body per design (two catch clauses + teardown). No change to `SendAsync`, `IsSocketLevelFailure`, or `ConnectWithBackoffAsync`.
|
||
3. XML-doc the timeout-vs-caller-cancel distinction on `SendOnceAsync`.
|
||
|
||
**Tests** (extend `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusTcpReconnectTests.cs` — its `FlakeyModbusServer` (TcpListener, `:22-35`) gains scriptable behaviors):
|
||
- `SendAsync_AfterResponseTimeout_TearsDownAndReconnects` — server stalls past the transport timeout on request N, answers correctly on N+1; assert request N throws (Bad path), the socket was replaced (server sees a new connection), and N+1 succeeds with the CORRECT response (no stale-read).
|
||
- `SendAsync_AfterTxIdMismatch_TearsDownSocket` — server answers with a wrong TxId; assert `ModbusTransportDesyncException` (or successful single-retry when auto-reconnect on) and a fresh connection for the next call.
|
||
- `SendAsync_AfterTruncatedHeader_TearsDownSocket` — server sends a short/garbage header then closes write side.
|
||
- `SendAsync_CallerCancellation_DoesNotTearDown` — cancel the caller token mid-request; assert OCE propagates and the server does NOT see a reconnect on the next call.
|
||
- Live gate (optional, modbus fixture): the `exception_injector` rig scenario from the round-1 plan — force a non-responding unit, assert recovery on the next poll.
|
||
|
||
**Effort:** S. **Risk:** Low-Medium — the timeout-vs-caller-cancel filter is the one subtlety; an inverted filter turns every shutdown into a spurious reconnect (harmless but noisy) or every timeout into a permanent desync (the bug). The four unit tests pin both directions.
|
||
|
||
---
|
||
|
||
# Sub-batch B2 — Silent-corruption locking: AbCip runtime lock + FOCAS concurrent caches (STAB-4 + STAB-5, High)
|
||
|
||
*Independently shippable. No dependencies. Concurrency fixes — high-risk classification.*
|
||
|
||
## STAB-4 — AbCip runs concurrent operations on shared libplctag handles with no serialization
|
||
|
||
**Restatement:** `ReadSingleAsync`/`ReadGroupAsync`/`WriteAsync` execute Read→GetStatus→Decode / Encode→Write→GetStatus on cached `IAbCipTagRuntime` handles with zero locking, while the server read path, every poll-group loop, and the alarm-projection loop can hit the same handle concurrently. libplctag `GetStatus` returns the *last* operation's status → torn/cross-attributed values decoded with Good status. AbLegacy documents and guards this exact hazard; the fix never propagated.
|
||
|
||
**Verification:** CONFIRMED. Unlocked critical sections at `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs:544-575` (`ReadSingleAsync`: `EnsureTagRuntimeAsync`→`ReadAsync`→`GetStatus`→`DecodeValue/DecodeArray`), `:614-637` (`ReadGroupAsync`: same on the parent + `DecodeValueAt` per member), `:708-726` (`WriteAsync`: `EncodeValue`→`WriteAsync`→`GetStatus`). Handles are cached in `DeviceState.Runtimes` (`ConcurrentDictionary`, `:1226` — the *map* is safe, the *handles* are not). Concurrent callers confirmed: `AbCipAlarmProjection.RunPollLoopAsync` reads through `_driver.ReadAsync` (`AbCipAlarmProjection.cs:230-232`), plus `PollGroupEngine` loops (one per subscription) and server-path reads/writes. The AbLegacy template: `_runtimeLocks` `ConcurrentDictionary<string, SemaphoreSlim>` + `GetRuntimeLock` (`AbLegacyDriver.cs:773-787`), applied around the read sequence (`:227-244`) and the write sequence (`:331-342`), disposed at teardown (`:841-842`). The RMW path (`WriteBitInDIntAsync :787-818`) already serializes via `GetRmwLock` (`DeviceState:1238-1244`) but only RMW-vs-RMW, and it operates on a **separate** handle cache (`ParentRuntimes :1235`) — so the runtime lock and the RMW lock govern disjoint handle sets and do not need merging (the STAB-11 direct-parent-write-vs-RMW lost-update is a *device-data* race across two handles, explicitly out of this batch — carried under STAB-11).
|
||
|
||
**Root cause:** the AbLegacy fix never propagated to AbCip (report theme: fixes don't flow back to siblings). AbCip caches per-tag runtimes but never serializes operations on one.
|
||
|
||
**Proposed design** (round-1 plan A4, port AbLegacy's per-runtime operation lock — verbatim pattern):
|
||
- `DeviceState` gains `private readonly ConcurrentDictionary<string, SemaphoreSlim> _runtimeLocks` + `public SemaphoreSlim GetRuntimeLock(string tagName)` (mirror `AbLegacyDriver.cs:786-787`), disposed in `DisposeHandles()` (mirror `:841-842`).
|
||
- `ReadSingleAsync`: acquire `device.GetRuntimeLock(def.Name)` around the `ReadAsync`→`GetStatus`→`Decode*` sequence (`:545-573`); status-mapping/health/result-stamping can stay outside the lock (they touch no handle) but keeping the AbLegacy shape (compute `status`/`value` inside, act outside) is the cleanest port.
|
||
- `ReadGroupAsync`: acquire `device.GetRuntimeLock(parent.Name)` around `ReadAsync`→`GetStatus`→per-member `DecodeValueAt` (`:615-636`) — the members decode from the parent handle's buffer, so they belong inside the same critical section.
|
||
- `WriteAsync`: acquire `device.GetRuntimeLock(def.Name)` around `EncodeValue`→`WriteAsync`→`GetStatus` (`:709-712`).
|
||
- Eviction interplay: `EvictRuntime` (`:891-897`) removes+disposes the handle while the evicting caller still holds the lock; the lock entry is deliberately NOT removed — a fresh runtime under the same name reuses it (AbLegacy precedent, avoids a remove/recreate race on the lock map).
|
||
- Locks keyed per tag name ⇒ cross-tag parallelism preserved. *Alternative (per-device lock):* rejected — serializes independent tags, and AbLegacy's field-proven granularity is per-runtime.
|
||
|
||
**Implementation steps:**
|
||
1. `AbCipDriver.cs` `DeviceState` (`:1187-1281`): add `_runtimeLocks`, `GetRuntimeLock`, dispose in `DisposeHandles`.
|
||
2. Wrap the three op bodies (`ReadSingleAsync`, `ReadGroupAsync`, `WriteAsync`) in `WaitAsync(ct)`/`finally Release`.
|
||
3. XML-doc on `GetRuntimeLock` quoting the AbLegacy rationale ("a libplctag Tag handle is not safe for concurrent Read/GetStatus/DecodeValue").
|
||
|
||
**Tests:**
|
||
- New `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipRuntimeConcurrencyTests.cs` mirroring `AbLegacyRuntimeConcurrencyTests`: extend/derive from `FakeAbCipTag` with a reentrancy guard (an `Interlocked` in-op counter that records a violation when >1 thread is inside Read/GetStatus/Decode/Encode/Write on the same instance); hammer concurrent `ReadAsync` + `WriteAsync` + a group read against one tag; assert zero overlap violations and coherent statuses.
|
||
- Regression: full existing AbCip suite (subscription/read/write/RMW tests) must stay green — the lock must not deadlock the RMW path (RMW uses `ParentRuntimes` handles, never `Runtimes`, so no nesting occurs; assert with the existing `AbCipBoolInDIntRmwTests`).
|
||
- Live gate (optional): abcip fixture — concurrent subscribe + write on overlapping tags, values stay coherent.
|
||
|
||
**Effort:** S. **Risk:** Medium (concurrency change on the hot data path, but strictly additive locking with a field-proven sibling template; deadlock risk nil — single lock level, no nesting).
|
||
|
||
## STAB-5 — FOCAS fixed-tree caches are plain `Dictionary`s mutated across threads
|
||
|
||
**Restatement:** four per-device caches are written by the background fixed-tree loop while `ReadAsync` callers read them concurrently — undefined behaviour on `Dictionary` (corruption or throw on resize).
|
||
|
||
**Verification:** CONFIRMED at `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs`: `LastFixedSnapshots` (`Dictionary<string,double>`, `:1184`), `LastTimers` (`Dictionary<FocasTimerKind,FocasTimer> = []`, `:1190`), `LastServoLoads` (`Dictionary<string,double>`, `:1192`), `LastSpindleLoads` (`Dictionary<int,int> = []`, `:1194`). Writes from `FixedTreeLoopAsync` and its helpers (`:700, :752, :793, :836-839, :862-863, :874`); reads from `TryReadFixedTree` on `ReadAsync` callers' threads (`:878, :912, :920, :945`). The neighbouring `_parsedAddressesByTagName` already got the `ConcurrentDictionary` treatment (`:40`). All call sites use indexer-set and `TryGetValue` only — both supported by `ConcurrentDictionary` unchanged. (`LastProgramInfo`/`LastProgramAxisRef` are single reference-typed properties — atomic writes, not part of this finding.)
|
||
|
||
**Root cause:** partial application of the concurrent-collection fix.
|
||
|
||
**Proposed design** (round-1 plan A5): convert the four fields to `ConcurrentDictionary<,>`; `[]`/`new(...)` initializers become `new()`/`new(StringComparer.OrdinalIgnoreCase)`. *Alternative (immutable snapshot swap per loop pass):* cleaner isolation, more churn — rejected for the minimal type-only change matching the in-tree precedent at `:40`.
|
||
|
||
**Implementation steps:** change the four field types on `FocasDriver.DeviceState` (`:1184-1194`) + initializers. Zero call-site changes.
|
||
|
||
**Tests:** new `FocasFixedTreeConcurrencyTests.cs` in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/`: a stress test spinning writer iterations (mimic `PublishAxisSnapshot`-style keyed writes forcing dictionary growth/resize) against concurrent `TryGetValue` readers on the same `DeviceState`; N-thousand iterations, assert no exception. (On the plain `Dictionary` this reliably throws "operations that change non-concurrent collections must have exclusive access" / corrupts under resize.) No FOCAS fixture exists — this unit test is the authoritative gate.
|
||
|
||
**Effort:** S. **Risk:** Very low — type-only change, FOCAS-local.
|
||
|
||
---
|
||
|
||
# Sub-batch B3 — PollGroupEngine v2: backoff + OCE hardening + `onError` fleet wiring + S7 fork retirement (STAB-9, STAB-8-poll-part, CONV-1, PERF-3, STAB-6-S7 — High/Medium)
|
||
|
||
*The highest-leverage structural item. Contains concurrency-adjacent loop changes — high-risk classification for the S7 migration. Introduces the shared `ConnectionBackoff` primitive that B4 and plan R2-01 consume.*
|
||
|
||
## Verification of the pieces
|
||
|
||
- **Engine (`src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs`)** already has: the `onError` sink (`:59, :164-169`), `StopState` drain-before-CTS-dispose (`:99-112`), `ObjectDisposedException`-on-Delay guard (`:136`), structural array compare (`:203-209`), reader-contract validation (`:178-181`). It lacks: failure backoff (fixed `Task.Delay(state.Interval)` `:131`) and — newly relevant — its `PollLoopAsync` treats **any** OCE as teardown (`catch (OperationCanceledException) { return; }` at `:122, :132, :139`), the exact shape that turned S7's connect-timeout OCE into permanent subscription death (STAB-14). If S7 migrates onto the engine without this filter, STAB-14 reproduces inside the shared engine.
|
||
- **STAB-9** CONFIRMED: all five consumers construct without `onError` (`ModbusDriver.cs:115`, `AbCipDriver.cs:133`, `AbLegacyDriver.cs:65`, `TwinCATDriver.cs:69`, `FocasDriver.cs:72`) — a reader-contract violation or reader exception is silently swallowed fleet-wide.
|
||
- **CONV-1/PERF-3/STAB-6-S7** CONFIRMED: S7's fork (`S7Driver.cs:1373-1412`) has capped exponential backoff (`ComputeBackoffDelay :1447-1456`, cap `PollBackoffCap :1363` = 30 s) and failure→health (`HandlePollFailure :1421-1437`, threshold `:1371`, never-downgrade-Faulted nuance `:1432-1435`) that the engine lacks; and lacks the structural array diff (`Equals(lastSeen?.Value, current.Value)` `:1469` — every subscribed array tag fires every tick) and the drain-before-dispose (`UnsubscribeAsync :1350-1354` cancels + disposes the CTS without awaiting `PollTask`; the loop's `Task.Delay` has no ODE guard) that the engine has. `SubscribeAsync :1328-1345`, `SubscriptionState :1477-1492`, teardown drains at `:269-270`/`:1605-1606`.
|
||
- **STAB-8 poll-part** CONFIRMED: the engine polls at fixed cadence regardless of failures — a dead device pays full-cost reads every tick on Modbus/AbCip/AbLegacy/TwinCAT-fallback/FOCAS.
|
||
|
||
**Root cause (CONV-1):** template divergence — each side owns fixes the other needs. Single home = extend the engine with the fork's two genuine improvements, then delete the fork.
|
||
|
||
## Proposed design
|
||
|
||
**1. `ConnectionBackoff` primitive** — new `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/ConnectionBackoff.cs`:
|
||
- `public static TimeSpan ComputeDelay(TimeSpan baseInterval, int consecutiveFailures, TimeSpan cap)` — the S7 math ported verbatim (`S7Driver.cs:1447-1456`: shift-capped-at-30, tick overflow guard, saturate at cap; `failures <= 0 ⇒ baseInterval`).
|
||
- `public sealed class ConnectionBackoff` — the per-device attempt-throttle B4/R2-01 consume: ctor `(TimeSpan baseDelay, TimeSpan maxDelay)`; `bool ShouldAttempt(DateTime nowUtc)` (true when outside the current backoff window), `void RecordFailure(DateTime nowUtc)`, `void RecordSuccess()` (reset to zero — recovery must never be delayed). **Not internally synchronized** — documented contract: callers invoke it under their existing per-device gate (`ConnectGate`/`_gate`/probe lock), which every intended call site already holds. *Alternative (lock-free Interlocked state):* rejected as needless complexity while every consumer already serializes.
|
||
- This is the **seam handoff to R2-01**: S7's `EnsureConnectedAsync` attempt-throttle (STAB-8's "slightly worse on S7" leg) is wired by R2-01 using this type; B3 lands the primitive, B4 wires TwinCAT/FOCAS/AbCip.
|
||
|
||
**2. Engine v2 (`PollGroupEngine`)** — three additive changes, all preserving current consumer behavior by default:
|
||
- Ctor gains `TimeSpan? backoffCap = null`. `PollLoopAsync` tracks `consecutiveFailures` (reset on a successful `PollOnceAsync`, incremented in the catch), and the delay becomes `backoffCap is null ? state.Interval : ConnectionBackoff.ComputeDelay(state.Interval, consecutiveFailures, backoffCap.Value)`. Null cap ⇒ byte-identical current cadence.
|
||
- **OCE hardening:** the three `catch (OperationCanceledException) { return; }` sites (`:122, :132, :139`) gain `when (ct.IsCancellationRequested)`; a *non-caller* OCE thrown by the reader (a driver-internal timeout CTS — the STAB-14/STAB-15 class) falls through to the generic `when (!IsFatal(ex))` handler ⇒ `ReportError` + backoff + continue. The `Task.Delay` catch at `:132` keeps its bare form (Delay's OCE can only be the loop token) — only the two `PollOnceAsync` catches change. This is the guard that makes the S7 migration safe regardless of R2-01's landing order.
|
||
- Keep `onError` optional in the signature (fleet consumers all pass it after this batch; making it required is a breaking nicety deferred — noted, not done, to keep the change additive).
|
||
-
|
||
**3. `onError` wiring — all five consumers.** Each construction gains `onError: ex => { /* degrade health, log */ }` implementing the S7 nuance: log warning; set `Degraded` preserving `LastSuccessfulRead`; **never downgrade `Faulted`** (`_health.State != DriverState.Faulted` guard — S7Driver.cs:1432-1435). Per-driver health idiom follows each driver's existing convention (Modbus `WriteHealth` Volatile helper; AbCip/TwinCAT plain `_health` field; AbLegacy `volatile`; FOCAS Volatile) — CONV-5 idiom unification is explicitly NOT this batch.
|
||
- Also pass `backoffCap: TimeSpan.FromSeconds(30)` in all five (adopting S7's proven cap fleet-wide) — this closes the STAB-8 subscriber-side leg for the whole fleet.
|
||
|
||
**4. S7 fork retirement.** Replace `_subscriptions`/`SubscribeAsync`/`UnsubscribeAsync`/`PollLoopAsync`/`PollOnceAsync`/`HandlePollFailure`/`SubscriptionState`/`S7SubscriptionHandle`/`PollBackoffCap`/`PollFailureHealthThreshold` (`S7Driver.cs:1328-1499` + field `:59` + teardown drains `:269-270`, `:1605-1606`) with a `PollGroupEngine` instance constructed like the other five (reader `ReadAsync` — method-group reference, same OTOPCUA0001-clean shape the other five use; onChange forwards `OnDataChange`; onError = the extracted failure handler; `minInterval` default 100 ms — matches the fork's floor `:1335-1337`; `backoffCap` 30 s). `ComputeBackoffDelay` moves to `ConnectionBackoff.ComputeDelay` (S7's copy deleted; its unit asserts move to `ConnectionBackoffTests`). Teardown paths swap the manual drain for `_poll.DisposeAsync()`. This deletes PERF-3 (structural compare inherited), STAB-6-S7 (StopState drain inherited), and the fork's bare-OCE poll-death (superseded by the hardened engine catches).
|
||
|
||
**Sequencing vs R2-01:** independent in either order. If R2-01 lands first, its poll-loop OCE-filter edits (`:1383/:1397/:1404`) are simply deleted by this migration (its ensure-wrapper fixes at `:488/:1000` are on the read/write path and survive untouched). If B3 lands first, the engine's hardened OCE catch already prevents timeout-OCE from killing the loop, and R2-01's remaining scope is the wrapper rethrow filter + fatal classification + probe mark-dead. Cross-reference this in both PRs.
|
||
|
||
## Implementation steps
|
||
|
||
1. New `Core.Abstractions/ConnectionBackoff.cs` (static `ComputeDelay` + instance throttle) + `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/ConnectionBackoffTests.cs` (schedule parity with the S7 math: 0 failures ⇒ base, doubling, cap saturation, overflow guard; `ShouldAttempt` window semantics; `RecordSuccess` resets immediately).
|
||
2. `PollGroupEngine.cs`: `backoffCap` param + `consecutiveFailures` + `ConnectionBackoff.ComputeDelay`; OCE `when (ct.IsCancellationRequested)` filters on the two `PollOnceAsync` catches. Extend `tests/Core/.../PollGroupEngineTests.cs`.
|
||
3. Wire `onError` (+ 30 s `backoffCap`) into `ModbusDriver.cs:115`, `AbCipDriver.cs:133`, `AbLegacyDriver.cs:65`, `TwinCATDriver.cs:69`, `FocasDriver.cs:72` — each with a small private `HandlePollError(Exception)` following the driver's health idiom.
|
||
4. S7: construct the engine in the ctor; delete the fork block (`:1328-1499` minus what moves); route `SubscribeAsync`/`UnsubscribeAsync` to `_poll`; swap teardown drains for `_poll.DisposeAsync()`; delete `ComputeBackoffDelay` (now in `ConnectionBackoff`).
|
||
|
||
## Tests
|
||
|
||
- `ConnectionBackoffTests`: as step 1 (port the existing S7 backoff asserts — grep `ComputeBackoffDelay` under `tests/Drivers/.../S7.Tests`).
|
||
- `PollGroupEngineTests` additions: `Backoff_SustainedReaderFailures_StretchesCadenceToCap` + `Backoff_SuccessResetsToInterval` (virtual-time via short intervals + tick counting); `ReaderOperationCanceled_WithoutCallerCancellation_TreatedAsFailureNotTeardown` (reader throws `new OperationCanceledException()` with the caller token un-cancelled; assert the loop survives, `onError` fired, next tick polls again) — **this is the STAB-14-class regression guard at the engine level**; `CallerCancellation_StillExitsPromptly`.
|
||
- Per-driver `onError` tests (one per driver, in each driver's `.Tests`): reader forced to throw (e.g. contract-violation via a fake returning a short snapshot list, or a throwing fake client) ⇒ assert health transitions to `Degraded` with `LastSuccessfulRead` preserved, and a pre-existing `Faulted` is not downgraded.
|
||
- S7 migration guards (in `S7.Tests`): `ArrayTag_UnchangedBetweenPolls_DoesNotRefire` (PERF-3 regression — fresh array instances, identical contents, assert one initial push only); `RapidSubscribeUnsubscribe_NoObjectDisposedException` (STAB-6 regression); `SustainedPollFailure_DegradesHealth_WithBackoff` (parity with the deleted `HandlePollFailure`); full existing `S7DiscoveryAndSubscribeTests` + `S7DriverReconnectTests` stay green.
|
||
- Live gate: s7 fixture (`lmxopcua-fix up s7 s7_1500`, `:1102`) — subscribe, bounce the container, assert values resume (the existing env-gated `S7_1500ReconnectTests` recipe now also exercises the engine-based loop).
|
||
|
||
**Effort:** M (engine S; five wirings S; S7 migration M). **Risk:** Medium-High — the S7 migration touches its subscription surface and alters array publish cadence (intended, PERF-3); the engine OCE filter changes loop-exit semantics (guarded by the new engine tests). Blast radius otherwise bounded: null `backoffCap` consumers (none remain in-tree after step 3) and existing engine consumers keep byte-identical behavior until they opt in.
|
||
|
||
---
|
||
|
||
# Sub-batch B4 — Connect-attempt throttles for the lazy-reconnect drivers (STAB-8 fleet part, Medium)
|
||
|
||
*Depends on B3 step 1 (`ConnectionBackoff`). Independently shippable after that. S7 wiring explicitly excluded — R2-01 seam.*
|
||
|
||
## STAB-8 — unthrottled reconnect attempts against a dead device
|
||
|
||
**Restatement:** a dead device pays full reconnect cost on every data call / poll tick: TwinCAT a connect attempt per call, FOCAS a full two-socket reconnect per tick of each of three loops, AbCip a full `Tag` create + Forward Open per tag per tick via evict-recreate. (S7's new per-call reconnect is the same defect — **R2-01's wiring**, same primitive.)
|
||
|
||
**Verification:** CONFIRMED — TwinCAT `EnsureConnectedAsync`/`EnsureConnectedUnderGateAsync` (`TwinCATDriver.cs:706-775`) attempts a connect on every call while down; FOCAS `EnsureConnectedAsync` (`FocasDriver.cs:1089-1120`) unconditionally builds + connects a fresh `SynchronizedFocasClient`; AbCip `EnsureTagRuntimeAsync` (`AbCipDriver.cs:852-882`) re-creates + re-initializes (Forward Open) a runtime per call after eviction (`EvictRuntime :891-897`). B3 closes the *subscriber-side* cost (poll backoff stretches tick cadence); this closes the *connect-side* cost for server-initiated reads/writes and multi-loop drivers.
|
||
|
||
**Proposed design:** per-device `ConnectionBackoff` gate on the connect/create path, with the **probe loop as the recovery agent**:
|
||
- Data-path callers consult `backoff.ShouldAttempt(now)` before an actual connect/create attempt; inside the window they fail fast with the same exception type the failed connect would have thrown (drivers already map it to `BadCommunicationError` snapshots / write results) — no wire traffic.
|
||
- The **probe loop bypasses the throttle** (its own interval is already a bounded cadence) and drives `RecordFailure`/`RecordSuccess`. Success on any path (probe or a data call that squeaked through at a window boundary) resets the backoff instantly — recovery is never delayed beyond one probe interval, and typically not at all (first post-recovery data call inside an expired window succeeds and resets).
|
||
- Defaults: base 1 s, max 30 s (matches Modbus transport + S7 poll conventions). Not config-exposed in this pass (no options-surface churn; note for CONV-8's future options `Validate()` work).
|
||
|
||
Per driver:
|
||
- **TwinCAT:** `DeviceState` gains `ConnectionBackoff Backoff`; `EnsureConnectedUnderGateAsync` (under `ConnectGate` — the documented synchronization home) checks `ShouldAttempt` before building a client when a bypass flag is not set, records failure in its existing connect catch (`:759-767`) and success after `device.Client = client` (`:768`); `EnsureConnectedAsync` gains a `bypassBackoff` parameter the probe loop (`:631`) sets true.
|
||
- **FOCAS:** same shape on `FocasDriver.DeviceState` + `EnsureConnectedAsync` (`:1089`); the probe loop's `EnsureConnectedAsync` call passes bypass; the fixed-tree and recycle loops go through the throttled path (they're the per-tick hammer the finding cites).
|
||
- **AbCip:** `DeviceState` gains `ConnectionBackoff Backoff`; `EnsureTagRuntimeAsync` checks `ShouldAttempt` **only on the create path** (cache hits unaffected, `:855`); `RecordFailure` in the init catch (`:867-875` — only for transport-shaped failures: keep it simple and record on any `InitializeAsync` throw), `RecordSuccess` after a successful `TryAdd`; the probe loop bypasses (its own runtime create) and records success/failure. Eviction (`EvictRuntime`) does not touch the backoff — the next create attempt decides.
|
||
|
||
*Alternative — throttle inside `PollGroupEngine`:* rejected; B3's poll backoff already stretches subscription cadence, and the connect cost also burns on server-initiated reads which never pass through the engine.
|
||
|
||
**Implementation steps:**
|
||
1. (`ConnectionBackoff` instance API landed in B3 task 1.)
|
||
2. TwinCAT: field + `EnsureConnectedUnderGateAsync`/`EnsureConnectedAsync` bypass param + probe wiring.
|
||
3. FOCAS: field + `EnsureConnectedAsync` gate + probe bypass.
|
||
4. AbCip: field + `EnsureTagRuntimeAsync` create-gate + probe wiring.
|
||
5. Cross-reference comment in `S7Driver.EnsureConnectedAsync` pointing R2-01 at the primitive (comment only — no S7 behavior change here).
|
||
|
||
**Tests (all fake-client — TwinCAT/FOCAS have no fixture; AbCip's is optional):**
|
||
- Per driver: `DeadDevice_ConnectAttemptsFollowBackoffSchedule` — failing fake connect; N rapid data calls; assert the fake's connect/create invocation count matches the schedule (1 attempt + fail-fasts inside the window), not N.
|
||
- Per driver: `ProbeBypassesBackoff_AndSuccessResets` — device recovers; assert the next probe tick connects despite an open window and the following data call succeeds immediately (no residual delay).
|
||
- AbCip: `CacheHit_NeverConsultsBackoff` (healthy runtime reads unaffected).
|
||
|
||
**Effort:** M. **Risk:** Low-Medium — the failure mode to guard is *delayed recovery*; the probe-bypass + instant-reset design plus the reset tests pin it. Fail-fast inside the window changes dead-device error latency (faster Bad, less wire noise) — intended.
|
||
|
||
---
|
||
|
||
# Sub-batch B5 — `ResolveHost` via the resolver: per-host breaker isolation (CONV-4, Medium)
|
||
|
||
*Independently shippable. No dependencies.*
|
||
|
||
## CONV-4 — `ResolveHost` mis-keys equipment-tag references in every multi-device driver
|
||
|
||
**Restatement:** every `ResolveHost` consults only `_tagsByName` (authored names); an equipment-tag reference (raw TagConfig JSON) never matches and falls back to the first device's host — so per-host Polly bulkhead/circuit-breaker keys are wrong for the *primary* authoring model, and a broken device B trips device A's breaker. Now production-live via the CapabilityInvoker dispatch wiring (#10).
|
||
|
||
**Verification:** CONFIRMED at all five sites — `AbCipDriver.cs:483-488`, `AbLegacyDriver.cs:496-501`, `TwinCATDriver.cs:689-696` (fallback `UnresolvedHostSentinel = ""`), `FocasDriver.cs:1082-1087`, `ModbusDriver.cs:125-131` (per-unit host `BuildSlaveHostName :134`; `ResolveUnitId :474` = per-tag override or driver default). All four multi-device `TDef` records carry `DeviceHostAddress`, and all four equipment parsers populate it from the `deviceHostAddress` key (verified: AbCip `:39/:56`, AbLegacy `:42/:51`, TwinCAT `:30/:38`, FOCAS `:32/:34` — FOCAS coalesces a missing key to `""`). `ModbusEquipmentTagParser` parses **no** `unitId` (grep-confirmed), so Modbus equipment tags currently cannot key per-unit at all. `EquipmentTagRefResolver` (`Core.Abstractions/EquipmentTagRefResolver.cs`) already caches parses (positive and negative) — routing `ResolveHost` through it adds no per-call parse cost after first use.
|
||
|
||
**Root cause:** `ResolveHost` predates the equipment-tag path and never learned about `_parseRef`.
|
||
|
||
**Proposed design** (round-1 plan B3, confirmed): route through the resolver, derive the host from the parsed def:
|
||
- AbCip / AbLegacy / TwinCAT / FOCAS: `ResolveHost(ref)` becomes `_resolver.TryResolve(ref, out var def) && !string.IsNullOrEmpty(def.DeviceHostAddress) ? def.DeviceHostAddress : <current fallback>` (first-device / `UnresolvedHostSentinel` / `DriverInstanceId` — each driver keeps its exact current fallback; the empty-string guard covers FOCAS's `?? ""` default so an address-less equipment tag falls back rather than keying an `""` host that TwinCAT alone treats as a documented sentinel).
|
||
- Modbus: `ResolveHost` becomes `_resolver.TryResolve(ref, out var tag) ? BuildSlaveHostName(ResolveUnitId(tag)) : HostName`; and `ModbusEquipmentTagParser` gains an optional `unitId` (byte, absent ⇒ null ⇒ driver-level `_options.UnitId` via `ResolveUnitId`) so multi-unit equipment tags key their own breaker. **Scope note:** `unitId` only — the parser's other gaps (`writable`, `deadband`, address-grammar…) are UNDER-6 and the strictness tiers are **R2-11's**.
|
||
- *Optional resolver sugar* (`ResolveHost(ref, Func<TDef,string>)` overload on `EquipmentTagRefResolver`) — skipped: five one-line bodies don't justify a new generic seam; revisit if a sixth pattern appears.
|
||
|
||
**Implementation steps:**
|
||
1. Rewrite the four multi-device `ResolveHost` bodies (files/lines above).
|
||
2. `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs`: parse optional `unitId` into `ModbusTagDefinition.UnitId`; rewrite `ModbusDriver.ResolveHost`.
|
||
3. XML-doc each `ResolveHost`: the key is consumed by per-host resilience (bulkhead/breaker) keying — equipment tags MUST resolve their authored device.
|
||
|
||
**Tests:**
|
||
- Per multi-device driver (in each `.Tests` project, pattern: two configured devices A+B, an equipment-tag ref whose TagConfig JSON names device B): `ResolveHost_EquipmentTagRef_ReturnsItsOwnDeviceHost` — asserts B (today: A). Plus `ResolveHost_UnknownRef_KeepsCurrentFallback` (regression: fallback string unchanged per driver).
|
||
- FOCAS: equipment ref with no `deviceHostAddress` falls back (not `""`).
|
||
- Modbus: `EquipmentTag_WithUnitId_ResolvesPerUnitHostName` (`host:port/unitN`) + parser round-trip test in the existing `ModbusEquipmentTagParser` suite; absent `unitId` keeps driver-level behavior.
|
||
|
||
**Effort:** S-M. **Risk:** Low — corrects a mis-key; the visible change is that per-host breaker keys become per-device for equipment tags (strictly the intended semantics). Verify no dashboard/consumer grouped on the old first-device key (the `/hosts` status table keys on `HostConnectivityStatus.HostName`, which already reports per-device — this fix *aligns* the two).
|
||
|
||
---
|
||
|
||
# Sub-batch B6 — TwinCAT replay hardening (STAB-16 Medium, STAB-17 Low, STAB-12-compound Medium)
|
||
|
||
*Independently shippable. No dependencies. Concurrency fixes — high-risk classification. No TC3 fixture exists — the fake-`ITwinCATClient` suite (`TwinCATReconnectReplayTests`) is authoritative.*
|
||
|
||
## STAB-16 — replay failure is silent to subscribers and never retried while the client stays up
|
||
|
||
**Restatement:** `ReplayNativeRegistrationsAsync` logs and skips a failed intent (deliberately — one bad symbol must not abort recovery of the rest), but the failed registration keeps its dead handle, nothing can push a Bad snapshot (the `OnChange` shape has no quality channel), and nothing retries until the *next client swap*. A transient `AddNotificationAsync` failure (ADS notification-quota blip, symbol mid-redeploy) kills that tag's push feed with only a warning log.
|
||
|
||
**Verification:** CONFIRMED — catch at `TwinCATDriver.cs:801-807` (log-only; `SwapHandle` success-path-only `:797`); `OnChange` is `Action<string, object?>` building Good-only snapshots in the `SubscribeAsync` closure (`:477-479`); replay is invoked solely from `EnsureConnectedUnderGateAsync :773` (new-client installs only); the probe loop's success path (`:631-651`) never re-examines registrations.
|
||
|
||
**Root cause:** the replay commit treated per-intent failure as purely a logging concern; the registration's lifecycle has no "dead, needs re-registration" state.
|
||
|
||
**Proposed design:**
|
||
- **Dead-handle state:** `NativeRegistration` gains `public bool HasLiveHandle => Volatile.Read(ref _handle) is not null;` and `public ITwinCATNotificationHandle? MarkHandleDead() => Interlocked.Exchange(ref _handle, null);`. On replay failure: `reg.MarkHandleDead()` + dispose the returned (already-dead) handle best-effort.
|
||
- **Bad-quality surfacing:** `NativeRegistration` gains a second stored callback `Action OnUnavailable` (built in `SubscribeAsync` beside `OnChange`, closing over the same `(handle, reference)`): `() => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, new DataValueSnapshot(null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)))`. The replay-failure catch invokes it (defensively wrapped) and degrades `_health` to `Degraded` preserving `LastSuccessfulRead`. Subscribers see the tag go Bad instead of frozen-Good-stale.
|
||
- **Probe-tick retry:** new `RetryDeadRegistrationsAsync(DeviceState device, CancellationToken ct)`: under `ConnectGate`, if `device.Client is { IsConnected: true } client`, for each `reg` in `device.NativeRegistrations.Values` with `!reg.HasLiveHandle`, attempt `AddNotificationAsync` → `SwapHandle` → **ownership re-check (shared with STAB-17 below)**; on success, count + log (recovery of the value itself arrives via ADS's initial-notification push on register — same mechanism initial subscribe relies on). Called from `ProbeLoopAsync` after a successful probe (`success == true`, after `:640`'s block). Failures just stay dead for the next tick — bounded retry cadence = probe interval, no extra timers.
|
||
- *Alternative — retry inside a dedicated timer:* rejected; the probe loop is the existing per-device liveness cadence and already holds the right gate discipline.
|
||
|
||
## STAB-17 — unsubscribe racing a replay leaks a live ADS notification
|
||
|
||
**Restatement:** `NativeRegistration.Dispose` runs gate-free (registry remove + handle dispose). A concurrent replay has already snapshotted the intents (`:786`) and may complete `AddNotificationAsync` *after* the dispose; `SwapHandle` installs the fresh handle into a registration nobody owns — the ADS notification stays live on the new client for its lifetime.
|
||
|
||
**Verification:** CONFIRMED (`:607-613` vs `:786-798`). Narrow window, bounded blast radius (one orphan per race), but a genuine native-resource leak on the PLC side.
|
||
|
||
**Proposed design** (the report's recommended shape): **post-swap ownership re-check** — in `ReplayNativeRegistrationsAsync` (and the new `RetryDeadRegistrationsAsync`), after `reg.SwapHandle(newHandle)`: `if (!device.NativeRegistrations.ContainsKey(reg.Id)) { var orphan = reg.MarkHandleDead(); try { orphan?.Dispose(); } catch { } }` — the registration was unsubscribed mid-replay; retract the fresh notification immediately. *Alternative — take `ConnectGate` in `Dispose`:* rejected: `Dispose` is called from `UnsubscribeAsync` (server-facing, should stay non-blocking) and would wait behind a wedged connect — exactly the STAB-12 hazard class this same sub-batch is shrinking.
|
||
|
||
## STAB-12 (compounded part) — `RecycleClientAsync` waits the gate uncancellably
|
||
|
||
**Restatement:** the probe loop's `RecycleClientAsync` waits `ConnectGate` with `CancellationToken.None`, so a connect wedged under the gate (TwinCAT's blocking SDK `Connect`, `AdsTwinCATClient.cs:73-80` — the un-fixed root of STAB-12, carried) now also wedges the probe loop beyond cancellation.
|
||
|
||
**Verification:** CONFIRMED at `TwinCATDriver.cs:826` (drifted from cited `:835`). Both call sites (`:640`, `:648`) are in `ProbeLoopAsync` and hold `ct`.
|
||
|
||
**Proposed design:** `RecycleClientAsync(DeviceState device, CancellationToken ct)` — pass the probe token into `ConnectGate.WaitAsync(ct)`. Both call sites pass the loop `ct`; the `:648` site sits inside a `catch` block, so wrap its await in `try { … } catch (OperationCanceledException) { break-equivalent }` (return from the loop body iteration) so a shutdown mid-recycle exits the probe task cleanly instead of faulting it. The *root* blocking-`Connect` fix (`AdsTwinCATClient`) stays carried under STAB-12 — out of this batch.
|
||
|
||
## Implementation steps
|
||
|
||
1. `TwinCATDriver.cs` `NativeRegistration` (`:558-614`): add `OnUnavailable` ctor param + property, `HasLiveHandle`, `MarkHandleDead`.
|
||
2. `SubscribeAsync` (`:474-479`): build + pass the `OnUnavailable` closure.
|
||
3. `ReplayNativeRegistrationsAsync` (`:784-814`): failure catch → `MarkHandleDead` + `OnUnavailable` + `_health = Degraded(...)`; success path → post-swap ownership re-check (STAB-17); keep the per-intent isolation and tally log.
|
||
4. New `RetryDeadRegistrationsAsync`; call from `ProbeLoopAsync` after a successful probe.
|
||
5. `RecycleClientAsync` signature + both call sites + the in-catch OCE wrap.
|
||
|
||
## Tests (extend `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReconnectReplayTests.cs` — fake client authoritative)
|
||
|
||
- `ReplayFailure_EmitsBadQuality_AndDegradesHealth` — fake client fails `AddNotificationAsync` for one symbol during replay; assert an `OnDataChange` with `BadCommunicationError` fired for that reference, health `Degraded` (`LastSuccessfulRead` preserved), sibling intents replayed.
|
||
- `ReplayFailure_RetriedOnNextSuccessfulProbeTick` — after the failed replay, drive a probe tick with the fake now accepting registrations; assert `AddNotificationAsync` re-invoked with the identical intent, a subsequent fake push reaches `OnDataChange` with Good, and no duplicate registration exists.
|
||
- `UnsubscribeDuringReplay_DisposesFreshHandle_NoOrphan` — fake client blocks `AddNotificationAsync` on a test-controlled gate; start replay, `UnsubscribeAsync` mid-flight, release the gate; assert the freshly-created handle was disposed and the fake's live-notification count is zero (fake tracks adds/disposes).
|
||
- `RecycleClient_HonorsCancellation` — wedge the gate (hold `ConnectGate` from the test), cancel the probe token; assert `RecycleClientAsync` throws/returns promptly and the probe task completes (no uncancellable hang; bound with `WaitAsync(seconds)` so a regression fails rather than hangs the suite).
|
||
- Regression: the existing 4 replay tests stay green.
|
||
|
||
**Effort:** M. **Risk:** Medium — concurrency around `ConnectGate`/`Interlocked` handle state; mitigated by the fake-client suite and by keeping every new mutation inside the existing gate discipline (retry under gate; ownership re-check is lock-free `Interlocked` + registry read, matching the existing swap idiom).
|
||
|
||
---
|
||
|
||
# Task breakdown
|
||
|
||
Rules: tasks are bite-sized (≤ ~5 min of implementation each — TDD steps split test-writing from implementation); **sub-batch boundaries are PR boundaries** (one branch + one merge per sub-batch: `fix/r2-09-b1-modbus-desync`, `fix/r2-09-b2-abcip-focas-locks`, `fix/r2-09-b3-pollengine-v2`, `fix/r2-09-b4-connect-backoff`, `fix/r2-09-b5-resolvehost`, `fix/r2-09-b6-twincat-replay`). B1/B2/B5/B6 are mutually independent and parallelizable; B4 depends on B3.1-3.2 only; within a sub-batch, tasks are sequential unless noted. Parallel implementers on this shared tree must worktree-isolate (house rule).
|
||
|
||
## B1 — Modbus fatal-classification *(sub-batch boundary — PR 1)*
|
||
|
||
### Task B1.1 — Failing transport-desync tests
|
||
**Classification:** test-first (low-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** B2.*, B3.*, B5.*, B6.*
|
||
**Files:** `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusTcpReconnectTests.cs`
|
||
1. Extend `FlakeyModbusServer` with scriptable per-request behaviors: stall (no reply), wrong-TxId reply, truncated header.
|
||
2. Add the four tests from the B1 design (`…ResponseTimeout_TearsDownAndReconnects`, `…TxIdMismatch…`, `…TruncatedHeader…`, `…CallerCancellation_DoesNotTearDown`).
|
||
3. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests --filter "FullyQualifiedName~ModbusTcpReconnectTests"` → **FAIL** (first three; caller-cancel may pass — keep as regression pin).
|
||
4. Commit: `test(modbus): failing repro for STAB-3 timeout/framing stream desync`.
|
||
|
||
### Task B1.2 — Teardown-before-propagate in `SendOnceAsync`
|
||
**Classification:** transport fault-path (medium-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTransportDesyncException.cs` (new), `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs`
|
||
1. Add `ModbusTransportDesyncException : IOException`.
|
||
2. Wrap `SendOnceAsync` body: OCE `when (!ct.IsCancellationRequested)` + `InvalidDataException` ⇒ `await TearDownAsync()` ⇒ throw desync exception.
|
||
3. Same filter as B1.1 step 3 → **PASS** (all four).
|
||
4. Commit: `fix(modbus): classify per-op timeout + framing violations connection-fatal (05/STAB-3)`.
|
||
|
||
### Task B1.3 — Full Modbus suite + PR
|
||
**Classification:** verification
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** —
|
||
**Files:** —
|
||
1. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests` and `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests` → green.
|
||
2. Optional live gate: modbus fixture + `exception_injector` non-responding-unit scenario.
|
||
3. Merge PR 1.
|
||
|
||
## B2 — AbCip lock + FOCAS caches *(sub-batch boundary — PR 2)*
|
||
|
||
### Task B2.1 — Failing AbCip concurrency test
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** B1.*, B2.4, B3.*, B5.*, B6.*
|
||
**Files:** `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipRuntimeConcurrencyTests.cs` (new), `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs`
|
||
1. Add a reentrancy-violation counter to `FakeAbCipTag` (Interlocked in-op depth; record violation when >1).
|
||
2. Write the hammer test (concurrent Read + Write + group-read on one tag; assert zero violations) — mirror `AbLegacyRuntimeConcurrencyTests`.
|
||
3. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests --filter "FullyQualifiedName~AbCipRuntimeConcurrencyTests"` → **FAIL**.
|
||
4. Commit: `test(abcip): failing repro for STAB-4 unserialized shared-handle ops`.
|
||
|
||
### Task B2.2 — `GetRuntimeLock` on AbCip `DeviceState`
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** —
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs` (DeviceState `:1187-1281`)
|
||
1. Port `_runtimeLocks` + `GetRuntimeLock` + dispose-in-`DisposeHandles` from `AbLegacyDriver.cs:773-787, 841-842`, with the hazard doc-comment.
|
||
2. Commit: `feat(abcip): per-runtime operation lock on DeviceState (AbLegacy pattern)`.
|
||
|
||
### Task B2.3 — Wrap the three AbCip op bodies
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs` (`:544-575`, `:614-637`, `:708-726`)
|
||
1. `WaitAsync(ct)`/`finally Release` around ReadSingle / ReadGroup / Write critical sections.
|
||
2. B2.1's filter → **PASS**; then full AbCip suite (`dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests`) → green (RMW suite proves no nesting/deadlock).
|
||
3. Commit: `fix(abcip): serialize Read/GetStatus/Decode + Encode/Write/GetStatus per runtime (05/STAB-4)`.
|
||
|
||
### Task B2.4 — Failing FOCAS cache-concurrency test
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** B2.1-B2.3 (different driver)
|
||
**Files:** `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeConcurrencyTests.cs` (new)
|
||
1. Stress test: writer thread grows/updates the four `DeviceState` caches (force resizes) vs concurrent `TryGetValue` readers; assert no exception over N iterations.
|
||
2. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests --filter "FullyQualifiedName~FocasFixedTreeConcurrencyTests"` → **FAIL** (flaky-fail is expected on UB — loop iterations high enough to be deterministic in practice).
|
||
3. Commit: `test(focas): failing repro for STAB-5 cross-thread Dictionary mutation`.
|
||
|
||
### Task B2.5 — Convert the four FOCAS caches
|
||
**Classification:** concurrency (high-risk, type-only)
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** —
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs` (`:1184-1194`)
|
||
1. Four fields → `ConcurrentDictionary<,>`; initializers adjusted. No call-site edits.
|
||
2. B2.4's filter → **PASS**; full FOCAS suite green.
|
||
3. Commit: `fix(focas): ConcurrentDictionary for fixed-tree caches (05/STAB-5)`. Merge PR 2.
|
||
|
||
## B3 — PollGroupEngine v2 + S7 fork retirement *(sub-batch boundary — PR 3)*
|
||
|
||
### Task B3.1 — `ConnectionBackoff` primitive + tests
|
||
**Classification:** shared primitive (medium-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** B1.*, B2.*, B5.*, B6.*
|
||
**Files:** `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/ConnectionBackoff.cs` (new), `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/ConnectionBackoffTests.cs` (new)
|
||
1. Write `ConnectionBackoffTests` first (schedule parity with `S7Driver.ComputeBackoffDelay` — port the S7 asserts; `ShouldAttempt`/`RecordFailure`/`RecordSuccess` window + instant-reset semantics). `dotnet test tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests --filter "FullyQualifiedName~ConnectionBackoffTests"` → **FAIL** (doesn't compile → add type) → implement → **PASS**.
|
||
2. Commit: `feat(core): ConnectionBackoff primitive — shared capped-exponential backoff + attempt throttle (05/STAB-8; seam for R2-01 S7 wiring)`.
|
||
|
||
### Task B3.2 — Engine backoff + OCE hardening (tests first)
|
||
**Classification:** concurrency (high-risk — shared loop semantics)
|
||
**Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
**Parallelizable with:** — (after B3.1)
|
||
**Files:** `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs`
|
||
1. Add `Backoff_SustainedReaderFailures_StretchesCadenceToCap`, `Backoff_SuccessResetsToInterval`, `ReaderOperationCanceled_WithoutCallerCancellation_TreatedAsFailureNotTeardown`, `CallerCancellation_StillExitsPromptly`. Filter `FullyQualifiedName~PollGroupEngineTests` → **FAIL**.
|
||
2. Implement: `backoffCap` ctor param, `consecutiveFailures`, `ConnectionBackoff.ComputeDelay`, `when (ct.IsCancellationRequested)` on the two `PollOnceAsync` OCE catches (`:122, :139`). → **PASS** (plus all existing engine tests).
|
||
3. Commit: `feat(core): PollGroupEngine v2 — failure backoff + caller-token-filtered OCE exit (CONV-1/STAB-8; STAB-14-class engine guard)`.
|
||
|
||
### Task B3.3 — `onError` + backoffCap wiring: Modbus, AbCip, AbLegacy
|
||
**Classification:** driver wiring (medium-risk)
|
||
**Estimated implement time:** 5 min per driver (tests-first each)
|
||
**Parallelizable with:** B3.4
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs:115`, `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs:133`, `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs:65` + one test per driver's `.Tests` project
|
||
1. Per driver: failing test `PollReaderFailure_DegradesHealth_PreservesLastSuccessfulRead_NeverDowngradesFaulted` (e.g. `--filter "FullyQualifiedName~PollErrorHealthTests"` per project) → **FAIL**.
|
||
2. Add `HandlePollError(Exception)` (driver's own health idiom) + pass `onError:` and `backoffCap: TimeSpan.FromSeconds(30)`. → **PASS**.
|
||
3. Commit per driver or one commit: `fix(drivers): wire PollGroupEngine onError→health + 30s poll backoff (05/STAB-9) [modbus,abcip,ablegacy]`.
|
||
|
||
### Task B3.4 — `onError` + backoffCap wiring: TwinCAT, FOCAS
|
||
**Classification:** driver wiring (medium-risk)
|
||
**Estimated implement time:** 5 min per driver
|
||
**Parallelizable with:** B3.3
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs:69`, `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs:72` + tests
|
||
1. Same TDD shape as B3.3.
|
||
2. Commit: `fix(drivers): wire PollGroupEngine onError→health + 30s poll backoff (05/STAB-9) [twincat,focas]`.
|
||
|
||
### Task B3.5 — S7 migration guard tests (failing)
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7PollEngineMigrationTests.cs` (new)
|
||
1. Write `ArrayTag_UnchangedBetweenPolls_DoesNotRefire` (PERF-3), `RapidSubscribeUnsubscribe_NoObjectDisposedException` (STAB-6-S7), `SustainedPollFailure_DegradesHealth_WithBackoff` against the fake `IS7PlcFactory` seam.
|
||
2. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~S7PollEngineMigrationTests"` → **FAIL** (array test fails on the fork's `Equals` diff; unsub test fails/flakes on the disposed CTS).
|
||
3. Commit: `test(s7): failing guards for the poll-fork retirement (PERF-3, STAB-6)`.
|
||
|
||
### Task B3.6 — Retire the S7 fork onto the engine
|
||
**Classification:** concurrency (high-risk — subscription surface rewrite)
|
||
**Estimated implement time:** 3 × 5 min (ctor+subscribe/unsubscribe swap; fork deletion + teardown swap; `ComputeBackoffDelay` relocation)
|
||
**Parallelizable with:** —
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (`:59`, `:269-270`, `:1328-1499`, `:1605-1606`), S7 backoff test relocation into `ConnectionBackoffTests`
|
||
1. Construct `_poll = new PollGroupEngine(reader: ReadAsync, onChange: …, minInterval: 100ms, onError: HandlePollError, backoffCap: 30s)`; route `SubscribeAsync`/`UnsubscribeAsync` through it; delete `PollLoopAsync`/`PollOnceAsync`/`HandlePollFailure`/`SubscriptionState`/`S7SubscriptionHandle`/`_subscriptions`/`PollBackoffCap`/`PollFailureHealthThreshold`/`ComputeBackoffDelay`; swap both teardown drains for `_poll.DisposeAsync()`.
|
||
2. B3.5's filter → **PASS**; full S7 suite (`dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests`) → green, incl. `S7DriverReconnectTests` + `S7DiscoveryAndSubscribeTests`.
|
||
3. Commit: `refactor(s7): retire the bespoke poll fork onto PollGroupEngine v2 (CONV-1; fixes PERF-3 + STAB-6-S7; deletes the fork's bare-OCE poll-death — cross-ref plan R2-01)`.
|
||
|
||
### Task B3.7 — Cross-solution verification + PR
|
||
**Classification:** verification
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** —
|
||
1. `dotnet test tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests` + all five driver unit suites → green.
|
||
2. Live gate (recommended for the S7 leg): s7 fixture bounce recipe (`lmxopcua-fix down s7 && lmxopcua-fix up s7 s7_1500`) with the env-gated `S7_1500ReconnectTests`.
|
||
3. Merge PR 3. Note in the PR body: R2-01 sequencing paragraph from the B3 design.
|
||
|
||
## B4 — Connect-attempt throttles *(sub-batch boundary — PR 4; blocked by B3.1-B3.2 only)*
|
||
|
||
### Task B4.1 — TwinCAT connect throttle (tests first)
|
||
**Classification:** concurrency (high-risk — connect-path gating)
|
||
**Estimated implement time:** 5 min (tests) + 5 min (impl)
|
||
**Parallelizable with:** B4.2, B4.3
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs` (`DeviceState`, `EnsureConnectedAsync :706`, `EnsureConnectedUnderGateAsync :731`, probe `:631`), `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATConnectBackoffTests.cs` (new)
|
||
1. Failing tests: `DeadDevice_ConnectAttemptsFollowBackoffSchedule`, `ProbeBypassesBackoff_AndSuccessResets` (filter `FullyQualifiedName~TwinCATConnectBackoffTests`) → **FAIL** → implement per B4 design → **PASS**.
|
||
2. Commit: `fix(twincat): per-device connect-attempt backoff, probe-driven recovery (05/STAB-8)`.
|
||
|
||
### Task B4.2 — FOCAS connect throttle (tests first)
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min + 5 min
|
||
**Parallelizable with:** B4.1, B4.3
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs` (`DeviceState`, `EnsureConnectedAsync :1089`, probe loop), `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConnectBackoffTests.cs` (new)
|
||
1. Same TDD shape (fake `IFocasClientFactory` counting connects). FAIL → implement → PASS.
|
||
2. Commit: `fix(focas): per-device connect-attempt backoff across the three loops (05/STAB-8)`.
|
||
|
||
### Task B4.3 — AbCip runtime-create throttle (tests first)
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min + 5 min
|
||
**Parallelizable with:** B4.1, B4.2
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs` (`DeviceState`, `EnsureTagRuntimeAsync :852`, probe loop), `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipConnectBackoffTests.cs` (new)
|
||
1. Tests: schedule on the create path, `CacheHit_NeverConsultsBackoff`, probe bypass + reset. FAIL → implement → PASS.
|
||
2. Commit: `fix(abcip): throttle evict-recreate Forward-Open attempts per device (05/STAB-8)`.
|
||
|
||
### Task B4.4 — S7 seam note + PR
|
||
**Classification:** docs/verification
|
||
**Estimated implement time:** 3 min
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (comment on `EnsureConnectedAsync`)
|
||
1. Comment pointing R2-01 at `ConnectionBackoff` for the S7 attempt-throttle. Full three driver suites green. Merge PR 4.
|
||
|
||
## B5 — `ResolveHost` via resolver *(sub-batch boundary — PR 5)*
|
||
|
||
### Task B5.1 — Failing `ResolveHost` tests (4 multi-device drivers)
|
||
**Classification:** correctness (medium-risk)
|
||
**Estimated implement time:** 2 × 5 min
|
||
**Parallelizable with:** B1.*, B2.*, B3.*, B6.*
|
||
**Files:** one test file (or additions) per `.Tests` project: AbCip, AbLegacy, TwinCAT, FOCAS
|
||
1. Per driver: `ResolveHost_EquipmentTagRef_ReturnsItsOwnDeviceHost` (two devices, equipment ref naming device B → expect B) + `ResolveHost_UnknownRef_KeepsCurrentFallback`; FOCAS adds the empty-`deviceHostAddress` fallback case. Suggested shared name: `--filter "FullyQualifiedName~ResolveHostTests"` per project → **FAIL** (4×).
|
||
2. Commit: `test(drivers): failing repro for CONV-4 equipment-tag ResolveHost mis-keying`.
|
||
|
||
### Task B5.2 — Rewrite the four `ResolveHost` bodies
|
||
**Classification:** correctness (medium-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** B5.3
|
||
**Files:** `AbCipDriver.cs:483-488`, `AbLegacyDriver.cs:496-501`, `TwinCATDriver.cs:689-696`, `FocasDriver.cs:1082-1087`
|
||
1. Route through `_resolver.TryResolve` + empty-host guard; keep per-driver fallbacks. B5.1 filters → **PASS**.
|
||
2. Commit: `fix(drivers): ResolveHost resolves equipment-tag refs via EquipmentTagRefResolver — per-host breaker isolation (CONV-4)`.
|
||
|
||
### Task B5.3 — Modbus `unitId` + `ResolveHost`
|
||
**Classification:** correctness (medium-risk)
|
||
**Estimated implement time:** 5 min (test) + 5 min (impl)
|
||
**Parallelizable with:** B5.2
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs`, `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs:125-131`, Modbus parser + driver tests
|
||
1. Failing tests: parser `unitId` round-trip; `EquipmentTag_WithUnitId_ResolvesPerUnitHostName`; absent-`unitId` regression. → **FAIL** → implement → **PASS**. (Scope: `unitId` ONLY — R2-11 owns parser strictness/parity.)
|
||
2. Commit: `fix(modbus): equipment tags carry unitId; ResolveHost keys per-unit via the resolver (CONV-4)`.
|
||
|
||
### Task B5.4 — Suites + PR
|
||
**Classification:** verification
|
||
**Estimated implement time:** 3 min
|
||
1. All five driver unit suites green. Merge PR 5.
|
||
|
||
## B6 — TwinCAT replay hardening *(sub-batch boundary — PR 6)*
|
||
|
||
### Task B6.1 — Failing test: replay failure surfaces Bad + degrades health
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** B1.*, B2.*, B3.*, B5.*
|
||
**Files:** `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReconnectReplayTests.cs`
|
||
1. `ReplayFailure_EmitsBadQuality_AndDegradesHealth` per B6 design. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests --filter "FullyQualifiedName~TwinCATReconnectReplayTests"` → **FAIL**.
|
||
2. Commit: `test(twincat): failing repro for STAB-16 silent replay failure`.
|
||
|
||
### Task B6.2 — Dead-handle state + `OnUnavailable` + replay-catch surfacing
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 2 × 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs` (`NativeRegistration :558-614`, `SubscribeAsync :474-479`, `ReplayNativeRegistrationsAsync :784-814`)
|
||
1. `HasLiveHandle`/`MarkHandleDead`/`OnUnavailable` + closure in `SubscribeAsync` + failure-catch: mark dead, emit Bad, degrade health.
|
||
2. B6.1's filter → **PASS**. Commit: `fix(twincat): replay failure marks the registration dead, publishes Bad quality, degrades health (05/STAB-16)`.
|
||
|
||
### Task B6.3 — Probe-tick retry of dead registrations (tests first)
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min + 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** same driver file (`ProbeLoopAsync :622-656`, new `RetryDeadRegistrationsAsync`), same test file
|
||
1. Failing `ReplayFailure_RetriedOnNextSuccessfulProbeTick` → implement `RetryDeadRegistrationsAsync` (under `ConnectGate`, re-register `!HasLiveHandle` intents, tally log) + probe-success call site → **PASS**.
|
||
2. Commit: `fix(twincat): retry failed replay intents on the next successful probe tick (05/STAB-16)`.
|
||
|
||
### Task B6.4 — STAB-17 ownership re-check (tests first)
|
||
**Classification:** concurrency (high-risk)
|
||
**Estimated implement time:** 5 min + 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** same driver file (`ReplayNativeRegistrationsAsync` success path + `RetryDeadRegistrationsAsync`), same test file (+ fake-client live-notification tracking)
|
||
1. Failing `UnsubscribeDuringReplay_DisposesFreshHandle_NoOrphan` (gate-blocked fake `AddNotificationAsync`) → implement the post-`SwapHandle` `NativeRegistrations.ContainsKey` re-check + orphan dispose in both replay paths → **PASS**.
|
||
2. Commit: `fix(twincat): retract the fresh ADS notification when unsubscribe raced the replay (05/STAB-17)`.
|
||
|
||
### Task B6.5 — Cancellable `RecycleClientAsync` (tests first)
|
||
**Classification:** fault-path (medium-risk)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** —
|
||
**Files:** same driver file (`RecycleClientAsync :824-840`, probe call sites `:640, :648`), same test file
|
||
1. Failing `RecycleClient_HonorsCancellation` (held gate + cancelled token; bounded wait so regression fails, not hangs) → thread `ct` through `ConnectGate.WaitAsync` + OCE-wrap the in-catch call site → **PASS**.
|
||
2. Commit: `fix(twincat): probe recycle honors cancellation — no uncancellable gate wait (05/STAB-12 compounded part)`.
|
||
|
||
### Task B6.6 — Full TwinCAT suite + PR
|
||
**Classification:** verification
|
||
**Estimated implement time:** 3 min
|
||
1. `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests` → green (incl. the pre-existing 4 replay tests). No TC3 fixture — this suite is the authoritative gate (UNDER-7 noted).
|
||
2. Merge PR 6.
|
||
|
||
---
|
||
|
||
# Sequencing & effort roll-up
|
||
|
||
| PR | Sub-batch | Findings | Effort | Risk | Depends on | Live gate |
|
||
|---|---|---|---|---|---|---|
|
||
| 1 | B1 Modbus fatal-classification | STAB-3 | S | Low-Med | — | modbus fixture (+`exception_injector`), optional |
|
||
| 2 | B2 AbCip lock + FOCAS caches | STAB-4, STAB-5 | S-M | Med (concurrency) | — | abcip fixture optional; FOCAS unit-authoritative |
|
||
| 3 | B3 PollGroupEngine v2 + S7 fork retirement (+`ConnectionBackoff`) | STAB-9, CONV-1, PERF-3, STAB-6-S7, STAB-8-poll | M | Med-High | — (coordinate with R2-01) | s7 fixture bounce, recommended |
|
||
| 4 | B4 connect-attempt throttles (TwinCAT/FOCAS/AbCip) | STAB-8-fleet | M | Low-Med | B3.1-B3.2 | none needed (fake-client) |
|
||
| 5 | B5 `ResolveHost` via resolver (+Modbus `unitId`) | CONV-4 | S-M | Low | — | none needed |
|
||
| 6 | B6 TwinCAT replay hardening | STAB-16, STAB-17, STAB-12-part | M | Med (concurrency) | — | none exists — unit-authoritative |
|
||
|
||
**Overall effort:** ≈ 5.5–7 dev-days across 6 independently shippable PRs (B1/B2/B5/B6 parallelizable from day one; B4 unblocks after B3's first two tasks).
|
||
|
||
**Explicit hand-offs:** `ConnectionBackoff` → **R2-01** (S7 `EnsureConnectedAsync` throttle + the STAB-14/15 wrapper/classification fixes; B3.6's fork deletion removes the poll-loop half of STAB-14's blast surface in whichever order the two plans land). Parser strictness / `ReadEnum` / remaining equipment-parser parity → **R2-11**. Carried and untouched here: STAB-10, STAB-11, full STAB-12 (FOCAS close-PDU + TwinCAT blocking `Connect`), CONV-3 (`ILoggerFactory` — worth an early ride-along in any of these PRs per the report, but owned elsewhere), PERF-1/2, CONV-5 idioms.
|