# Architecture Review 05 — Protocol Drivers **Date:** 2026-08-19 **Commit:** `12b0035a` (master, clean tree) **Updates:** the second update — round 1 ran 2026-07-08 at `9cad9ed0`, round 2 (re-review) 2026-07-12 at `f6eaa267`. ~292 commits touched this domain since round 2, including the R2 remediation waves (R2-01 S7 fault hardening, R2-09 PollGroupEngine v2 + S7 fork retirement, R2-11 TagConfig consolidation + #457 strict parsing), the 05/STAB-3/4/5/8/9/16/17 and CONV-4 targeted fixes, the #516 config-re-parse sweep, the #519/#517/#497 status-code work, the comms-loss-watchdog driver legs (#527), and **four brand-new drivers**: MTConnect (+ IWritable), MQTT incl. Sparkplug B (Driver.Mqtt + .Browser/.Contracts/.Transport), the SQL poll driver (Driver.Sql + .Browser/.Contracts/.Dialect), and the Calculation pseudo-driver. **Scope:** the twelve cross-platform protocol drivers and their satellites — `Driver.Modbus` (+ `.Addressing`, `.Contracts`, and the new RTU-over-TCP transport), `Driver.S7`, `Driver.AbCip`, `Driver.AbLegacy`, `Driver.TwinCAT`, `Driver.FOCAS`, `Driver.OpcUaClient` (+ `.Contracts`, `.Browser`, and the new `.Session` leaf), `Driver.Mqtt` (+ `.Browser`, `.Contracts`, `.Transport`), `Driver.MTConnect` (+ `.Contracts`), `Driver.Sql` (+ `.Browser`, `.Contracts`, `.Dialect`), `Driver.Calculation` (+ `.Contracts`), and `src/Drivers/Cli/*`. Galaxy and Historian.Gateway are out of scope (report 06). **Method:** every prior finding — open or claimed-closed — was re-verified against current code with file:line evidence; no remediation claim, commit message, or doc was trusted. The four new drivers were fresh-reviewed to the same adversarial standard as the incumbents (full driver reads, house failure-mode hunt: built-but-never-wired, optimistic success, fatal-vs-transient misclassification, teardown leaks on reconnect, fixture-shaped verification gaps). --- ## Prior-finding status (f6eaa267 → 12b0035a) | ID | One-liner | Status @ 12b0035a | |---|---|---| | STAB-1 | S7 no reconnect path | **REMAINS FIXED** — the R2-01 shape survives the fork retirement: `EnsureConnectedAsync` (`S7Driver.cs:1912-1954`), `MarkConnectionDeadIfFatal`, probe backstop. | | STAB-2 | TwinCAT native subs orphaned after reconnect | **REMAINS FIXED** — `NativeRegistration` replay intact; residuals STAB-16/17 now closed too (below). | | STAB-3 | Modbus transport desync after timeout / TxId mismatch | **CLOSED** (`98f48601`) — per-op timeout tears the socket down and normalizes as `ModbusTransportDesyncException.DesyncReason.Timeout` (`ModbusTcpTransport.cs:182-191`); TxId mismatch / truncated frame likewise (`:153-161, :176-181`); a well-formed exception PDU correctly does NOT tear down (`:165-172`). The new RTU-over-TCP transport ships the same discipline from day one — CRC/unit-mismatch/truncation → desync + teardown, per-op timeout → teardown (`ModbusRtuOverTcpTransport.cs:160-177`). | | STAB-4 | AbCip concurrent ops on shared libplctag handles | **CLOSED** (`e48609b0`+`00dcb547`) — per-runtime `SemaphoreSlim` via `DeviceState.GetRuntimeLock` taken on read (`AbCipDriver.cs:776`), RMW (`:856`), write (`:960`); the alarm projection routes through the driver's own locked `ReadAsync`/`WriteAsync` (`AbCipAlarmProjection.cs:21-24, :150, :231`), so the new bounded-parallel read window (`MaxConcurrentReads`, `:643-645`) composes with the locks instead of bypassing them. | | STAB-5 | FOCAS fixed-tree caches plain `Dictionary`s | **CLOSED** (`4231afb2`) — all four are `ConcurrentDictionary` (`FocasDriver.cs:1585, :1591, :1593, :1595`). But two *other* FOCAS dictionaries stayed plain — new STAB-32. | | STAB-6 | AbLegacy probe-shutdown race; S7 unsubscribe race | **PARTIAL** — the **S7 half is CLOSED** (fork retired; `PollGroupEngine.StopStateAsync` awaits the loop before disposing the CTS, `PollGroupEngine.cs:230-239`). The **AbLegacy half is STILL OPEN**: the probe task is still discarded at spawn (`_ = Task.Run(...)`, `AbLegacyDriver.cs:190`) and `ShutdownAsync` still cancels + disposes `ProbeCts` and the runtimes without awaiting loop exit (`:231-237`) — a winding-down loop can touch a disposed CTS and raise `OnHostStatusChanged` post-shutdown. AbCip's retained-`ProbeTask` fix still never propagated back. | | STAB-7 | `InitializeAsync` ignores `driverConfigJson` (Modbus, AbLegacy); TwinCAT empty-config quirk | **MOSTLY CLOSED** (#516 sweep `d32d89c3`/`5184a2e1`) — Modbus re-parses (`ModbusDriver.cs:326-327`), AbLegacy re-parses (`AbLegacyDriver.cs`, "#516" comment + `ParseOptions` in `InitializeAsync`), Sql re-parses all-or-nothing via `ApplyBinding` (`SqlDriver.cs:362-366`), FOCAS via the `_rebind` seam (`FocasDriver.cs:190-195`), OpcUaClient (`OpcUaClientDriver.cs:204-205`). **The TwinCAT empty-config quirk survives verbatim**: `if (parsed.Devices.Count > 0 \|\| parsed.RawTags.Count > 0) _options = parsed;` (`TwinCATDriver.cs:121-126`) — a contentful zero-device config is silently discarded, so a deploy that deliberately empties a TwinCAT driver keeps serving the old device set (FOCAS fixed the same problem with a shape check; TwinCAT's content check is the bug). | | STAB-8 | No reconnect backoff fleet-wide | **MOSTLY CLOSED** — engine-level capped-exponential failure backoff (`backoffCap: 30s`) is passed by all seven poll consumers, and per-device `ConnectionBackoff` connect throttles landed in AbCip (`AbCipDriver.cs:1147-1167`, single-flight cold connect under `ConnectGate` `:1121`), TwinCAT (`TwinCATDriver.cs:876-878, :909-913, :1147`), FOCAS (`FocasDriver.cs:1442-1473, :1548`). **The S7 leg alone is STILL OPEN**: `EnsureConnectedAsync` documents the seam and defers it (`S7Driver.cs:1896-1908` — "Deliberately NOT implemented driver-locally here") — and S7's engine backoff is unreachable anyway (new STAB-21), so a dead PLC still pays a full `_options.Timeout` connect attempt per read/write batch + probe tick. | | STAB-9 | `PollGroupEngine` `onError` sink dead — no driver passes it | **CLOSED** (`9213c037`/`b494d09d`) — all seven poll consumers pass `onError: HandlePollError` routing to health + logger (Modbus `ModbusDriver.cs:158+`, S7 `S7Driver.cs:67-79`, AbCip `AbCipDriver.cs:158+`, AbLegacy `AbLegacyDriver.cs:75+`, TwinCAT `TwinCATDriver.cs:77-81`, FOCAS `FocasDriver.cs:118-122`, Sql `SqlDriver.cs:227-232`), plus `backoffCap` and the `onOverrun` meter closure. The parameter remains optional in the engine (`PollGroupEngine.cs:129`) but has no bare consumer left. | | STAB-10 | AbLegacy never evicts failed tag handles | **CLOSED** (`a5a0f96d`) — `EvictRuntime` on non-zero status and transport exceptions on both read (`AbLegacyDriver.cs:315-316, :340-341`) and write (`:417-418, :442-443`) paths, mirroring AbCip (`:787-792`). | | STAB-11 | RMW locks don't cover direct parent writes; AbCip DINT / TwinCAT 4-byte assumptions | **STILL OPEN, all parts** — AbCip: bit-RMW takes `GetRmwLock(parentName)` (`AbCipDriver.cs:1054`) but a direct write to the parent tag goes through `GetRuntimeLock(def.Name)` only (`:960`) — different lock dictionaries, no mutual exclusion → lost update; RMW is still BOOL-within-**DINT** only (`:942-944, :1043-1044`). TwinCAT: RMW still reads/writes the parent as 4-byte `UDInt` (`TwinCATDriver.cs:341-359`; `AdsTwinCATClient.cs:102` admits the width assumption) — a 4-byte write-back to a 2-byte WORD parent is not harmless — and only the `Bool`+`BitIndex` branch takes `_bitRmwLocks` (`:333-368`), direct parent writes bypass it (`:370-373`). | | STAB-12 | Blocking/uncancellable teardown paths | **PARTIAL** — the TwinCAT probe-recycle compounding is **CLOSED** (`502a327f`): `RecycleClientAsync` waits on `ConnectGate` with the probe token (`TwinCATDriver.cs:1049-1052`). Still open: FOCAS's sync `Dispose()` close-PDU exchange on a `NetworkStream` with **no** timeout and no token (`Wire/FocasWireClient.cs:156-182`, `SendPdu`+`ReadPdu` at `:168-169`; zero `ReadTimeout`/`WriteTimeout` sets in the file) — a wedged CNC blocks Dispose indefinitely while holding `_lifetimeGate`; and TwinCAT's blocking SDK `Connect` ignoring token+timeout (`AdsTwinCATClient.cs:73-80`, self-documented as "the un-fixed root, carried" at `TwinCATDriver.cs:1050-1051`). | | STAB-13 | (Positive) write-outcome surfacing universal; OpcUaClient reconnect strongest | **STILL HOLDS** — OpcUaClient per-item verbatim passthrough + `BadTimeout` "outcome unknown" on post-dispatch cancellation (`OpcUaClientDriver.cs:813-839`); reconnect machine intact (`:1997-2133`). The four new drivers all return per-item `WriteResult`s with named constants visible to `StatusCodeParityTests` (Sql is structurally read-only — `SqlDriver.cs:17-20`). But see STAB-22 (S7 write-health ordering) and STAB-26 (MQTT QoS-0 optimism). | | STAB-14 | S7 connect-timeout OCE kills subscription polls (regression in Crit-3 fix) | **CLOSED** (R2-01 + PR #453) — connect timeout surfaces as `TimeoutException` at both connect sites (`S7Driver.cs:246, :1933-1942`); read/write ensure-wrappers rethrow OCE only on the caller's token (`:565, :1707`); the poll loop is now the shared engine whose OCE catches filter on the loop's own token (`PollGroupEngine.cs:266, :323` — the "STAB-14 engine guard" comment). The read-leg sibling: `S7OperationDeadline.RunAsync` wall-clocks **every** data-plane wire op — string/block/multi-var read, string/block write, probe (`IS7Plc.cs:142-177`) — via `WaitAsync` so control returns even when S7.Net ignores the token (`S7OperationDeadline.cs:48-58`). | | STAB-15 | S7 fatal classification misses framing/desync + cancel-mid-PDU; probe never marks dead | **CLOSED** — `IsS7ConnectionFatal` (`S7Driver.cs:1994-2017`) classifies `TimeoutException`, `TPKTInvalidException`/`TPDUInvalidException`/`WrongNumberOfBytesException`, `PlcException{ConnectionError, WrongNumberReceivedBytes}` fatal, walks inners, and deliberately excludes `InvalidDataException`; mid-PDU cancellation marks `_plcDead` unconditionally at all four wire paths (`:652-663, :925-931, :1044-1050, :1750-1758`); the probe classifies + marks dead **under `_gate`** (`:2167-2180`). | | STAB-16 | TwinCAT replay failure silent + never retried | **CLOSED** (`a06515ae`+`de297596`) — a failed intent is marked dead, publishes a Bad snapshot (`BadCommunicationError` via `OnUnavailable`, `TwinCATDriver.cs:962-970, :534-536`), degrades health, and is retried on the next successful probe tick (`RetryDeadRegistrationsAsync`, `:730, :989-1037`). Pinned by `TwinCATReconnectReplayTests`. Residual (info): the retry rides the probe loop, so `Probe.Enabled=false` silently disables STAB-16 recovery; default is enabled (`TwinCATDriverFactoryExtensions.cs:68`). | | STAB-17 | Unsubscribe racing replay leaks a live ADS notification | **CLOSED** (`082ee99b`) — both replay sites re-check ownership after `SwapHandle` and retract an ownerless fresh handle (`TwinCATDriver.cs:947-952, :1008-1014`); handle transfer is `Interlocked.Exchange` (`:679-697`). All interleavings traced to exactly-once disposal; pinned by `UnsubscribeDuringReplay_DisposesFreshHandle_NoOrphan`. | | PERF-1 | Five of seven drivers read one tag per wire round-trip | **PARTIAL** — **S7 closed** (span-grouped block reads, one PDU per contiguous block, `S7ReadSpanPlanner` + `ReadSpanAsync` `S7Driver.cs:608-619, :1037-1042`, on by default with `-1` opt-out validated at parse time; leftovers batch through multi-var PDUs chunked at 12 with a safe whole-chunk-falls-to-per-tag fallback `:932-938`); **AbCip improved** (bounded-parallel reads, `MaxConcurrentReads` default 8, `:643-645`) though still one CIP round-trip per tag; the engine gained a per-cycle batch callback consumed by Modbus/S7/AbCip/FOCAS (`IBatchSubscribable`). Still per-tag: **TwinCAT** (no ADS sum-read — sole occurrence is a doc citation, `AdsTwinCATClient.cs:116`), **FOCAS** (single-flight socket, `pmc_rdpmcrng` still fetches one value's width, `Wire/WireFocasClient.cs:302-317`), **AbLegacy** (no span coalescing). | | PERF-2 | AbCip whole-UDT read exists in two never-joined halves | **STILL OPEN** — the planner still relies on declaration-order offsets (`AbCipUdtReadPlanner.cs:27`), the fast path is still opt-in-and-dangerous (`AbCipDriverOptions.cs:72-79`), and the Template-Object true-offset cache still feeds discovery only. | | PERF-3 | S7 forked poll loop diffs arrays by reference | **CLOSED** — the fork is deleted (`85ef74ea`); the shared engine compares arrays structurally (`PollGroupEngine.cs:425-431`). | | PERF-4 | OpcUaClient serializes all traffic on one global gate | **STILL OPEN** — `_gate = new SemaphoreSlim(1,1)` (`OpcUaClientDriver.cs:120`) taken by every wire path (reads `:685`, writes `:773`, discovery `:907` — held across the *entire* recursive browse —, subs `:1254`, alarms `:1446-1520`, HistoryRead `:1724`, events `:1896`). | | PERF-5 | OpcUaClient discovery enrichment un-chunked Read + silent degrade | **STILL OPEN** — `pending.Count * 4` ReadValueIds in one call (`:1097-1109`); the wholesale catch registers everything Int32/ViewOnly with **no log line** (`:1117-1125, :1162-1172`); the ":932 the SDK chunks automatically" comment remains unsubstantiated. | | PERF-6 | Per-tick allocation churn (TwinCAT symbol re-parse) | **STILL OPEN** — `TwinCATSymbolPath.TryParse` per hot call (`TwinCATDriver.cs:264, :328, :520`; allocations in `TwinCATSymbolPath.cs:21-96`); FOCAS's `_parsedAddressesByTagName` fix never propagated. | | PERF-7 | Deadband only in Modbus | **STILL OPEN** — publish-suppression deadband remains Modbus-only (`ShouldPublish`, honoured on the batch path too — `_cycleBatchItems` buffers only what `ShouldPublish` passed, `ModbusDriver.cs:158-177`); MQTT re-implements unchanged-value suppression locally for Plain mode; the engine itself has none. | | CONV-1 | S7 forked the poll loop; fork diverged both ways | **CLOSED** (`85ef74ea`) — the fork is deleted; S7 is a normal `PollGroupEngine` consumer (`S7Driver.cs:67-79`); the engine absorbed the fork's backoff + failure-health (via `onError`) and kept the structural diff + drain-before-dispose it always had. (But see STAB-21: S7's absorbed backoff is unreachable.) | | CONV-2 | Three config-parse strictness tiers; silent enum defaulting ×6 | **CLOSED** (R2-11 + #457 + the #549/#551/#556/#559 authoring chain) — the six copy-pasted `ReadEnum`s are replaced by shared `TagConfigJson.TryReadEnumStrict` (present-but-invalid ⇒ reject ⇒ `BadNodeIdUnknown`); the four new drivers use the strict readers from day one (e.g. `SqlEquipmentTagParser.cs:49-50`); the Modbus probe parses the factory DTO shape incl. transport mode (`ModbusDriverProbe.cs:33-63`); Sql/Mqtt/MTConnect probes document DTO parity (`SqlDriverProbe.cs:12-16`). Residual: MTConnect's probe JSON options are *stricter* than its factory's (new CONV-9). | | CONV-3 | Four factories take no `ILoggerFactory` → NullLogger in production | **STILL OPEN — same four** — `S7DriverFactoryExtensions.cs:20`, `TwinCATDriverFactoryExtensions.cs:19`, `AbCipDriverFactoryExtensions.cs:21`, `FocasDriverFactoryExtensions.cs:37` all take `(DriverFactoryRegistry)` only; `DriverFactoryBootstrap.cs:171-188` calls them bare while the other **eight** get `loggerFactory`. Costlier every round: S7's reconnect/span logs, TwinCAT's replay-retry warnings, FOCAS's capability-gate + skipped-tag warnings all run blind in production, and FOCAS's `BuildClientFactory` uses the no-logger `WireFocasClientFactory` ctor even though a logger-taking ctor exists precisely because "the previous no-arg factory path discarded them" (`FocasDriverFactoryExtensions.cs:141`, `WireFocasClient.cs:409-414`). | | CONV-4 | `ResolveHost` mis-keys equipment-tag refs in every multi-device driver | **CLOSED** (`9262dd25`+`e69d3b3b`) — all resolve through `EquipmentTagRefResolver` to the parsed def's host: Modbus per-unit (`ModbusDriver.cs:226-235`, `{host}:{port}/unit{n}`; equipment tags now carry `unitId`), AbCip (`AbCipDriver.cs:604-610`), AbLegacy (`AbLegacyDriver.cs:621-626`), TwinCAT/FOCAS (same commit, resolver-backed). Per-host breaker isolation is real for equipment tags now. | | CONV-5 | Health-field publication idioms drift | **PARTIAL, still drifting** — conforming: Modbus (`ModbusDriver.cs:433-438`), Mqtt (`MqttDriver.cs:1486-1489`), MTConnect (`MTConnectDriver.cs:3205-3208`), FOCAS (converted — `FocasDriver.cs:157, :183` + `Volatile.Read` in `GetHealth`), AbLegacy (`volatile`, `:40-45`), and **Sql sets the new high bar** with a CAS retry loop that a lost-update race motivated (`SqlDriver.cs:836-845`, Driver.Sql-010). Still plain fields: S7 (`S7Driver.cs:181`), AbCip (`AbCipDriver.cs:113`), TwinCAT (`TwinCATDriver.cs:52`), OpcUaClient (`OpcUaClientDriver.cs:147`). | | CONV-6 | OpcUaClient.Contracts drags the OPC UA SDK | **CLOSED** (`b4a03b4b`) — `NamespaceMap` moved to the new `Driver.OpcUaClient.Session` leaf which carries the sole SDK ref; Contracts references only Core.Abstractions, pinned by `ContractsIsDependencyLightTests`. | | CONV-7 | CLI harness divergences | **STILL OPEN** — six harnesses unchanged: `ParseValue` copy-pasted ×6 (`*/Commands/WriteCommand.cs`), AbLegacy still validation-free, AbCip's `Timeout` init-setter still throws (`AbCipCommandBase.cs:44`). None of the four new drivers (nor OpcUaClient) has a CLI. | | CONV-8 | Dead config knobs + stale scaffold docs; no options `Validate()` | **PARTIAL** — `DisableFC23` is still a no-op but now honestly documented "Reserved / no-op" (`ModbusDriverOptions.cs:91-97`); AbCip `ConnectionSize` doc now claims "Honoured by the driver layer" (`AbCipDriverOptions.cs:120-122`) — unverified this round; stale scaffold docs remain (`AbLegacyDriver.cs:12` "ship in PRs 2 and 3", `OpcUaClientDriver.cs:13` "PR 66 ships"). Options validation improved materially where it matters: the Sql factory validates every knob incl. the inverted-deadline gate (`SqlDriverFactoryExtensions.cs:236-278`), S7 validates span options at parse time, R2-02 clamps resilience knobs. | | UNDER-1 | FOCAS advertises writes it cannot perform | **CLOSED (reshaped)** — discovery forces `ViewOnly` on every emitter (`FocasDriver.cs:692-702, :751, :771, :810`); the wire still refuses (`WireFocasClient.cs:71-73`) and `FIXED:` writes are refused pre-wire (`FocasDriver.cs:543-547`); deploy inspection warns on authored `writable:true` (`FocasTagDefinitionFactory.cs:250-251`). The internal write gate honours the key by design as the seam for a future writable backend — with the wire refusing, nothing misleads. | | UNDER-2 | OpcUaClient `UnsMappingTable` mandatory-but-unconsumed | **STILL OPEN** — validated mandatory (`OpcUaClientDriver.cs:411, :420`), consumed nowhere (repo-wide grep: option + validator + tests only). | | UNDER-3 | OpcUaClient alarm filter compares mismatched NodeId encodings | **STILL OPEN** — verbatim ordinal set of caller refs (`:1406`) vs session-relative `ns=N;…` rendering (`:1609-1610`); stored refs are `nsu=` (`:879`); production caller passes v3 refs (`DriverInstanceActor.cs:1146`); no `_namespaceMap` conversion on the path — any non-empty filter still drops every event. | | UNDER-4 | S7 authored-array/write/UInt32 gaps | **PARTIAL** — `ArrayCount` **closed** (`S7DriverOptions.cs:178-185`, factory `S7TagDefinitionFactory.ReadArrayCount :255-269`, discovery surfaces `IsArray`/`ArrayDim` `S7Driver.cs:2059-2068`, single block-read array path `:1227-1244`). Array *writes* still unsupported but now loud (init guard `:463-469`; `NotSupportedException` → `BadNotSupported` `:1805-1808`). `UInt32 → Int32` still lossy, now documented (`:2077-2080`). | | UNDER-5 | AbCip alarm-ack outcomes discarded | **STILL OPEN** — `_ = await _driver.WriteAsync(requests, …)` verbatim (`AbCipAlarmProjection.cs:150`); a failed ack is still invisible to operator/health/Part 9 caller. The discovery residuals also persist: with `EnableControllerBrowse` on, the multi-device `await foreach` has no per-device try/catch (`AbCipDriver.cs:1265+` — one unreachable PLC faults the whole `DiscoverAsync`). The #468/#539/#534 chain did land (controller-browse gate unblocked, authored EtherNet/IP port honoured — `BuildCreateParams` uses `ParsedAddress.Port`, `AbCipDriver.cs:1617`). | | UNDER-6 | Equipment-tag parsers lag the authored-tag feature set | **CLOSED** — R2-11 gave all six incumbents shared readers + the `writable` key (FOCAS forced read-only, see UNDER-1) + `Inspect()` warnings; FOCAS equipment tags run the capability-matrix pre-flight (`FocasDriver.cs:251-258`); the #549/#551 chain made authoring never accept what the runtime rejects, service-enforced. The four new drivers shipped with parser/authoring parity from the start. | | UNDER-7 | Live-risk assumptions documented but ungated | **PARTIAL** — FOCAS servo-load scaling and timer endianness are now **live-verified** ("Validated against a live 31i-B 2026-06-25", `WireFocasClient.cs:934-941, :917-924`). Still unverified: TwinCAT array read shape (`AdsTwinCATClient.cs:113-116`), Flat-mode `SubSymbols` (`:270-276`, gated only by `EnableControllerBrowse` defaulting false), FOCAS `cnc_getfigure` (`WireFocasClient.cs:391-395`, soft-degrades), AbLegacy array-decode assumptions, and the TwinCAT replay path (still fake-client-only — no TC3 fixture). | | UNDER-8 | Test coverage codec-heavy, failure-path-light | **SUBSTANTIALLY IMPROVED** — the four new drivers arrive with the best failure-path coverage in the subsystem (MQTT ~541 tests/27 files + 5 live-integration suites + a project-owned Sparkplug simulator; MTConnect ~408 tests incl. deterministic teardown/backoff/blocked-subscriber suites + a self-skipping cppagent integration; Sql ~263+54 unit + 3-dialect docker integration incl. a blackhole-timeout suite; Calculation 44). S7 gained deadline/migration/span-parity/live-outage suites. Remaining gaps: still **zero AbLegacy reconnect tests** (STAB-6 lives there), no S7/AbCip concurrency suites, no test pins S7's write-health ordering (STAB-22) or backoff engagement (STAB-21), no DTD-hardening test for MTConnect's probe parser (UNDER-11), and the Sql fixtures cover only text key columns / unbound topByTimestamp on Postgres (the STAB-24 blind spot). | | UNDER-9 | OpcUaClient 2,154-line monolith; PKCS#12-only identity | **STILL OPEN, one leg worse** — 2,230 lines now; `TransferSubscriptionsOnReconnect` is still never set (SDK default `false`) while the `OnReconnectComplete` doc-comment (`:2020-2022`) now affirmatively claims it defaults `true` — the reconnect path does not do what the driver believes; `BuildCertificateIdentity` still loads PKCS#12 only (`:559-560`) while its own comment promises a PEM fallback that does not exist (`:556-558`). | --- ## Architecture overview — what changed since round 2 **PollGroupEngine v2** (`src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs`, 484 lines) is the round's structural win: fixed-**rate** pacing with an overrun counter + `onOverrun` sink (`:37-56, :351-358`), capped-exponential failure backoff via `ConnectionBackoff` on the failure branch only (`:287-295`), the teardown-token-filtered OCE catches that make the S7 fork retirement safe (`:262-266, :321-323`), an optional per-cycle `onBatch` callback (raised once, after all per-tag callbacks, never empty, `:415-416`), a non-blocking `UnsubscribeAsync` that drains before disposing (`:191-239`), and structural array comparison (`:425-431`). Seven drivers consume it uniformly with `onError` + `backoffCap: 30s` + a meter-wired `onOverrun`. **The four new drivers** are, on conventions, the best code in the subsystem — each has a single config-parse authority shared by factory/probe/driver, strict enum reads, a Contracts-side `Inspect()` wired into the deploy gate, named status constants covered by the reflection `StatusCodeParityTests` (`tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/StatusCodeParityTests.cs` — scans every `Driver.*` dll; note its own warning that inline literals are invisible to it), `ISkippedTagReporter` (now implemented by **all twelve** drivers, #541), and health surfaces designed around the #527 comms-loss watchdog's conjunction (Sql's `IsAnswerEvidence` classification, `SqlDriver.cs:743-749`, is the most carefully reasoned `LastSuccessfulRead` semantics in the fleet). The Sql injection boundary is genuinely airtight in design: identifiers reach SQL text only through `ISqlDialect.QuoteIdentifier` (control/format-char rejection incl. the Trojan-Source class, `SqlServerDialect.cs:129-155`) *after* the §8.1 catalog gate has rewritten them to catalog spellings (`SqlDriver.cs:953-996` — polls resolve only against the validated table, `:319`), and all values are bound parameters (`SqlGroupPlanner.cs:17-26`). ### Per-driver conformance matrix (updated) | Aspect | Modbus | S7 | AbCip | AbLegacy | TwinCAT | FOCAS | OpcUaClient | Mqtt | MTConnect | Sql | Calculation | |---|---|---|---|---|---|---|---|---|---|---|---| | Subscription model | PollGroupEngine | PollGroupEngine (**fork deleted**) | PollGroupEngine | PollGroupEngine | native ADS + engine fallback | PollGroupEngine | MonitoredItems | **broker push** | **/sample stream pump** | PollGroupEngine | dependency-mux push | | onError→health / backoffCap | yes/yes | yes/yes (but see STAB-21) | yes/yes | yes/yes | yes/yes | yes/yes | n/a | n/a | n/a | yes/yes | n/a | | Batch subscribe (`IBatchSubscribable`) | yes | yes | yes | no | no | yes | no | no | no | no | no | | Connect-attempt throttle | transport backoff | **no (STAB-8 residual)** | ConnectionBackoff | evict-parity only | ConnectionBackoff | ConnectionBackoff | SessionReconnectHandler | bounded reconnect loop | bounded backoff | n/a (per-poll open) | n/a | | Config re-parse on Initialize (#516) | yes | yes | yes | yes | **empty-config quirk** | yes | yes | yes | yes | yes (all-or-nothing) | fallback bug (STAB-28) | | Factory `Register` takes `ILoggerFactory` | yes | **no** | **no** | yes | **no** | **no** | yes | yes | yes | yes | yes | | Health idiom | Volatile | **plain** | **plain** | volatile | **plain** | Volatile | **plain** | Volatile | Volatile | **CAS loop (best)** | derived | | Writes | per-item + RMW | per-item (health ordering: STAB-22) | per-item + RMW | per-item + RMW | per-item + RMW | refused at wire | verbatim passthrough | QoS-0 publish (STAB-26) | HTTP PUT, double-gated | **structurally read-only** | n/a | | `ISkippedTagReporter` | yes | yes | yes | yes | yes | yes | yes | yes | yes | yes (two-stage union) | yes | | CLI harness | yes | yes | yes | yes | yes | yes | no | no | no | no | no | --- ## Findings Severity scale unchanged: **Critical** = live data-loss/unavailability in a realistic field scenario; **High** = correctness or stability defect likely to bite; **Medium** = architectural debt / divergence with concrete failure modes; **Low** = hygiene. New IDs continue the existing numbering (STAB-18+, PERF-8+, CONV-9+, UNDER-10+). ### 1. Stability **STAB-18 — High — NEW — MQTT has no initial-push analogue of the poll engine's `forceRaise`, so a ref latched Bad recovers only when the plant value changes.** `DriverHostActor.FanCommsLossToDependencyMux` forces `BadNoCommunication` into the mux for every ref of a disconnected driver and documents its recovery contract as riding "a fresh subscription whose PollGroupEngine initial push runs with `forceRaise: true`" (`DriverHostActor.cs:1310-1318`). MQTT has no poll engine: `MqttSubscriptionManager.SubscribeAsync` (`MqttSubscriptionManager.cs:325-365`) and `SparkplugIngestor.SubscribeAsync` (`SparkplugIngestor.cs:470-506`) record handles and push nothing, nothing in production calls `IReadable.ReadAsync`, and Plain-mode delivery then actively suppresses the one message that would recover the ref — `Deliver` skips `Raise` when value+status match the driver-owned last-value cache (`MqttSubscriptionManager.cs:762-767`), which survives reconnects and redeploys by design. Concrete shapes: (a) after a #527 watchdog reconnect, the retained-seed replay carries the identical value → suppressed → scripted-alarm / virtual-tag inputs latched Bad by the #481 class never recover; (b) after a full address-space rebuild, nodes reseed `BadWaitingForInitialData` and an unchanged republish never re-raises — the exact "prod-inert after redeploy → Bad forever" class the v3 Batch 4 live gate already caught once at the host layer. Sparkplug shares shape (b) for quiet metrics (a rebuild triggers no rebirth). *Recommendation:* on (re)subscribe, republish the cached snapshots for the subscription's refs (the push-model `forceRaise`), or clear suppression memory per re-subscribe. **STAB-19 — High — NEW — Calculation launders dependency quality to Good, defeating the system's own comms-loss fan-out.** `CalculationDriver.OnDependencyValue` never reads its `statusCode` parameter (`CalculationDriver.cs:330-352`), and the feeding seam hardcodes `statusCode: 0u` behind a comment — "Quality is stripped at the mux boundary" — that is factually wrong (`DependencyConsumerMuxAdapter.cs:71-72`): `DependencyValueChanged` has carried `StatusCode` since #478/#519 and `DependencyMuxActor` forwards it (`DependencyMuxActor.cs:114, :150`). `DriverHostActor.FanCommsLossToDependencyMux` (`DriverHostActor.cs:1334-1359`) deliberately pushes `BadNoCommunication` + null value so dependents see comms loss — a calc tag receives that as an *arrived, Good, null*: (a) the warm-up gate is defeated (`_arrivedDeps.Add` at `:336` fires for a value that never arrived); (b) the common `(double)ctx.GetTag("x").Value` idiom NREs on the null → a spurious "script threw" storm — the exact noise class the `a836e0d9` warm-up gate was built to eliminate; (c) a null-tolerant script publishes a **Good** output computed from a dead input, and a slow-moving input that dies leaves the calc output stale-Good indefinitely. `CalculationEvaluator.BuildReadCache` (`:181-187`) additionally stamps every dep Good with eval-time timestamps. *Recommendation:* thread the real status through the adapter, hold the warm-up gate open for Bad arrivals, and degrade the calc output quality when an input is Bad. **STAB-20 — Medium — NEW — MTConnect: a Subscribe racing a lifecycle rebuild can orphan the /sample pump on a disposed client, permanently.** `SubscribeAsync` deliberately runs outside `_lifecycle` and starts the pump under `_subscriptionLock` (`MTConnectDriver.cs:1043-1049`); the rebuild path runs stop-pump → `TeardownCoreAsync` (disposes the client) → `StartCoreAsync` (`:551-555, :599-607`), and the actor invokes initialize on a fire-and-forget task (the driver's own #517 comment, `:2741-2743`) — so a Subscribe can land between stop and teardown, bind the old client, and then `StartCoreAsync`'s start is a no-op because a live pump exists (`:1809` `if (_pumpTask is { IsCompleted: false }) return;`). Every `SampleAsync` on the disposed client throws ODE → classified Transient → the stray pump reconnect-loops at max backoff against a dead object forever; reads keep succeeding (advancing `LastSuccessfulRead`), so the watchdog never trips and nothing self-heals until the next lifecycle call. *Recommendation:* generation-stamp the pump with its `AgentSession` and let `StartCoreAsync` supersede a pump from an older session. **STAB-21 — Medium — NEW — S7's poll-failure backoff is unreachable: the engine backs off only when the reader throws, and S7's reader never throws for device faults.** `consecutiveFailures` increments only in the engine's catch (`PollGroupEngine.cs:287-289, :324-329`); `S7Driver.ReadAsync` by design returns a fully-degraded batch on a failed (re)connect (`S7Driver.cs:566-571`) and absorbs every per-tag/span/multi-var fault into result stamps — its only throws are caller-cancellation OCE. With the PLC down, backoff never engages and each cycle immediately follows the previous one after a full `_options.Timeout` (default 5 s) connect attempt, per subscription, plus the probe every 5 s, all serialized on `_gate` — this is what makes the STAB-8 S7 residual bite. The ctor comment (`S7Driver.cs:63-66`) claiming the migration brought backoff to S7 overstates what is reachable, and `SustainedPollFailure_DegradesHealth` never exercises the branch. *Recommendation:* wire `ConnectionBackoff.ShouldAttempt/RecordFailure/RecordSuccess` into `EnsureConnectedAsync` under `_gate` (the FOCAS/TwinCAT/AbCip shape) — exactly where the code comment already says it belongs. **STAB-22 — Medium — NEW — S7 write-path health is per-item last-writer-wins, and the next read pass erases a write-side Faulted.** `WriteAsync` stamps `_health` inside each item's catch (`S7Driver.cs:1770-1772` Faulted PUT/GET-denied, `:1782, :1791` Degraded) with no outcome accumulator — item 2's transport Degraded overwrites item 1's operator-actionable Faulted in the same batch, the exact ordering bug the read pass was refactored to kill (`ReadPassOutcome`, `:703-711`, worst-state-wins with rank Healthy DisposeAsync().AsTask().GetAwaiter().GetResult();` (`FocasDriver.cs:1478`) — the exact pattern TwinCAT's own comment forbids for the OPC UA stack thread (`TwinCATDriver.cs:1081-1087`), compounded by STAB-12b's unbounded close-PDU. And `_devices` / `_tagsByRawPath` are plain `Dictionary`s (`FocasDriver.cs:47, :50`) read concurrently by data paths, `GetHostStatuses`, and the alarm scan while `ShutdownAsync`/`InitializeAsync` clear/repopulate them — TwinCAT hardened the same two fields to `ConcurrentDictionary` with an explicit rationale (`TwinCATDriver.cs:30-37`). **STAB-33 — Low — NEW — S7 residuals from the fresh pass.** (a) `InitializeAsync` overwrites `Plc` and `_probeCts`/`_probeTask` without disposing existing ones (`S7Driver.cs:264, :286-291`) — currently safe because every production path tears down first, but it violates the dispose-before-overwrite rule Modbus/MQTT now embody (`ModbusDriver.cs:298-319`) and is one refactor from live. (b) The probe loop reports `HostState.Stopped` on mere gate contention — an OCE from `_gate.WaitAsync` under the 2 s probe deadline falls into the bare catch (`:2185-2187`) and flaps host state while a long span read holds the gate. (c) `IsAccessDenied` classifies any `PlcException` whose message chain contains "not allowed" as PUT/GET-denied → latched Faulted (`:2028-2039`) — the broad arm widens the false-positive surface over the specific "Accessing object not allowed" pattern. **STAB-34 — Low — NEW — Calculation minor residuals.** One-stale-publish race across reinit for surviving tags (state-existence guard only, `CalculationDriver.cs:524`); `GetHealth` is Healthy whenever `_running` — a driver whose every evaluation fails stays Healthy with only Bad quality + script logs as evidence (defensible per CLAUDE.md, but there is no Degraded surface at all), and `_lastComputeUtc` (`DateTime?`, `:103`) is written under `_lock` but read lock-free (`:319-321`); value dedup uses `Equals(st.LastValue, value)` (`:551`) so array results republish every evaluation; the deploy-time Tarjan gate is the sole cycle defence (no runtime damping). **STAB-35 — Low — NEW — PollGroupEngine records a change as seen before delivering it, so a throwing subscriber loses that change permanently — and only Modbus defends its bridge.** `state.LastValues[tagRef] = current;` precedes `_onChange(...)` (`PollGroupEngine.cs:404-411`); an `OnDataChange` subscriber that throws unwinds `PollOnceAsync` mid-loop — the throwing tag's change is marked seen but never delivered (no re-raise until the next value change), later tags in the cycle are skipped, and for batch-wired drivers the cycle's `onBatch` never fires. Modbus wraps its bridge in try/catch precisely for this (`ModbusDriver.cs:158-177`, with the cross-cycle-contamination rationale); the other six consumers invoke `OnDataChange?.Invoke` unguarded. ### 2. Performance *(PERF-1/2/4/5/6/7 verdicts in the table above; the substantive change is S7's span + multi-var batching and AbCip's bounded-parallel reads.)* **PERF-8 — Medium — NEW — MQTT's `MaxPayloadBytes` bounds decode work only, and the Sparkplug path has no publish suppression.** `MqttClientOptionsFactory.BuildClientOptions` (`:63-68`) never calls `WithMaximumPacketSize`, so a multi-MB retained blob is fully received and buffered by MQTTnet before the handler's one-comparison refusal — and the in-code remark still calls the cap "a constructor knob, not yet an operator-facing one" (`MqttSubscriptionManager.cs:184-190`) when the options key has existed for some time, hiding that only the protocol-layer half is missing. Separately, Sparkplug ingest bypasses the Plain-mode unchanged-value suppression entirely (documented at `:759-761`), so every rebirth re-fans full birth-value sets downstream. ### 3. Conventions *(CONV-1/2/4/6 closed; CONV-3/7 open; CONV-5/8 partial — table above.)* **CONV-9 — Low — NEW — MTConnect's probe parses a stricter JSON dialect than its factory.** The probe's `JsonOptions` (`MTConnectDriverProbe.cs:96-101`) lack `ReadCommentHandling.Skip`/`AllowTrailingCommas` that the driver's have (`MTConnectDriver.cs:169-174`) — a config carrying a comment or trailing comma deploys fine but Test Connect reports "Config JSON is invalid": the inverse of the documented "reachable ≠ deployable" gap, and a violation of the probe-parity rule the fleet otherwise now follows. ### 4. Underdeveloped areas *(UNDER-1/6 closed; UNDER-2/3/5 open; UNDER-4/7/8/9 partial — table above.)* **UNDER-10 — Medium — NEW — S7's deploy-time `Inspect` does not cover the init-time config guards, so a clean-inspecting tag can fault the whole driver.** `S7TagDefinitionFactory.Inspect` warns only for blobs `FromTagConfig` rejects (`S7TagDefinitionFactory.cs:161-201`), but `FromTagConfig` accepts a `Float64`+`isArray` blob, a non-byte-anchored wide scalar, a mis-typed Timer/Counter, and a writable array — each of which then throws at driver init in `RejectUnsupportedTagConfigs` (`S7Driver.cs:407-469`, guards a–e), faulting the entire instance and taking every other tag on it to Bad — contradicting the mapper's own "one bad tag must not fail the whole driver init" philosophy three lines up (`:204-205`). This is the authoring-vs-deploy-gate drift class the repo has closed elsewhere (#535→#559), reopened for exactly the blobs with the worst blast radius. **UNDER-11 — Low — NEW — MTConnect deferred/ungated edges.** DTD hardening is real in the streams parser (`DtdProcessing.Prohibit`, `MTConnectStreamsParser.cs:131-138`) but the probe parser inherits it only via `XDocument` defaults and **no test feeds an entity-expansion document** — a refactor onto a permissive `XmlReader` passes every test. A credentialed `AgentUri` is carefully redacted but non-functional (`HttpClient` never converts URI userinfo to an Authorization header) and there are no TLS trust knobs. No host-resolution seam: default `SocketsHttpHandler` with infinite `PooledConnectionLifetime` means a DNS-repointed agent name is re-resolved only when a connection drops — a healthy long-lived `/sample` stream to a stale IP persists indefinitely. And an authorable `AccessLevel=ReadWrite` tag on a write-disabled instance advertises writable and honestly fails (`BadNotWritable` + revert) with no deploy-gate warning. An Agent write *refusal* (401/403) and unreachability also share `BadCommunicationError` (`MTConnectDriver.cs:86-89, :787-789`) where `BadUserAccessDenied` would tell the operator over the wire. **UNDER-12 — Low — NEW — Every valid MQTT Sparkplug tag materializes writable.** Sparkplug `CanWrite` is "binding tuple complete" (`MqttDriver.cs:648-650`), which every mapped tag satisfies, so read-only sensor metrics all advertise `SecurityClassification.Operate`. Defensible under Sparkplug command semantics and gated server-side by `WriteOperate`/ACLs — but Plain mode has a per-tag opt-in (`writeTopic` absent ⇒ ViewOnly) and Sparkplug has no equivalent opt-out. --- ## Maturity ratings | Dimension | Rating (1-5) | Prior | Justification | |---|---|---|---| | Stability | **3.5** | 3 | Every carried High from rounds 1–2 is now verifiably closed in code — STAB-3/4/5/14/15 (the desync/locking/classification class), STAB-16/17, STAB-9/10, most of 7/8/12 — with tests that pin the fixes, and the comms-loss-watchdog driver legs (Modbus/MQTT dispose-before-overwrite, honest `LastSuccessfulRead` semantics) landed. Held below 4 because the two new push drivers each ship one High of the classic house type (STAB-18 MQTT no-initial-push recovery gap; STAB-19 Calculation quality laundering — both "the signal exists end-to-end and is discarded at the last seam"), the AbLegacy shutdown race and STAB-11 RMW class persist untouched, and S7's backoff is dead code (STAB-21). | | Performance | **3** | 2 | Real batching finally landed where it was cheapest and hottest: S7 span-grouped block reads + multi-var leftovers (on by default, live-gated), AbCip bounded-parallel reads, engine-level per-cycle batch fan-out consumed by four drivers, fixed-rate pacing with an overrun meter. Still open: TwinCAT sum-read / FOCAS PMC ranges / AbLegacy spans (per-tag on serialized links), the AbCip UDT planner⇄template join, the OpcUaClient global gate + un-chunked enrichment, TwinCAT per-call symbol re-parse. | | Conventions | **4** | 3 | The template is now enforced rather than aspirational: one config-parse authority per driver (probe parity), strict enum parsing fleet-wide (#457), Contracts-side `Inspect()` deploy gates, reflection-guarded status constants (#497/#519), `ISkippedTagReporter` ×12, `DriverTypeNames` keying, and the four new drivers are exemplary citizens (MQTT and MTConnect arguably now define the template). Docked for the unchanged CONV-3 logger asymmetry (S7/TwinCAT/AbCip/FOCAS still run blind in production — more expensive every round), the four-way health-idiom drift, the CLI divergence, and CONV-9/UNDER-10 gate-drift residuals. | | Underdeveloped areas | **3.5** | 3 | UNDER-1 and UNDER-6 closed properly (forced-ViewOnly discovery + capability pre-flight + authoring parity), FOCAS's riskiest scalings live-verified, and the new drivers' deferrals are declared-and-loud rather than silent (MTConnect DATA_SET→`BadNotSupported`, Sql's reserved providers failing at construction with a naming message). Still carried: `UnsMappingTable` (advertised feature that does not exist), the OpcUaClient alarm-filter encoding mismatch, AbCip ack-outcome discard, the unverified-against-hardware TwinCAT set, AbLegacy's zero reconnect tests — and the OpcUaClient reconnect comment now actively misstates `TransferSubscriptionsOnReconnect`'s default. | --- ## Recommended remediation order 1. **STAB-18 + STAB-19 together — the "signal discarded at the last seam" pair.** MQTT needs a re-subscribe initial push (or suppression reset); Calculation needs the real `statusCode` threaded through `DependencyConsumerMuxAdapter` and honoured. Both defeat the host's comms-loss fan-out contract that the rest of the system relies on, and both are invisible to unit tests that never drive the reconnect/rebuild path. 2. **STAB-21 + STAB-8(S7) — wire `ConnectionBackoff` into `EnsureConnectedAsync`** (the comment already names the seam) so S7 stops paying a 5 s connect per batch against a dead PLC and the engine's backoff branch becomes reachable; add the missing backoff-engagement test. 3. **STAB-24 — Sql Postgres typed-parameter binding** + integer-key fixture legs; this is a wrong-guidance correctness bug on a shipped dialect, currently masked by the fixture. 4. **STAB-22 — S7 write-pass `ReadPassOutcome` sibling** (worst-state-wins accumulator on `WriteAsync`, and stop the read pass erasing write-side Faulted). 5. **STAB-20 — MTConnect pump generation stamp**; STAB-23 — FOCAS loop joins (port TwinCAT's retained-task shape); STAB-6 — the same fix finally applied to AbLegacy's probe loop. 6. **CONV-3 — one line per factory** (S7/TwinCAT/AbCip/FOCAS `Register(registry, loggerFactory)`): now a prerequisite for *four* recovery mechanisms being observable in production at all. 7. **STAB-25/26/27 — MQTT write honesty + claim backoff**; UNDER-10 — S7 Inspect/init-guard parity (the #535→#559 arc's method applies directly). 8. **STAB-11 — RMW lock unification** (route direct parent writes through the RMW lock; width-aware TwinCAT RMW) — unchanged three rounds running; either fix it or demote it to a documented limitation with a deploy-gate warning. 9. Sweep the carried one-liners: UNDER-2 delete the `UnsMappingTable` gate, UNDER-3 alarm-filter encoding, UNDER-5 ack outcomes + per-device browse fault isolation, PERF-5 chunk+log, UNDER-9 fix the two misleading comments (TransferSubscriptions, PEM), STAB-7 TwinCAT empty-config quirk, CONV-9 probe JSON parity.