Joseph Doherty
44f6408e1c
Every round-2 finding re-verified against code (73 closed / 31 partial / 97 carried); matrix 19/28 cells up, one down. New Critical 03/S14 (AdminUI FallbackPolicy 401s every mesh gRPC map on a login-enabled central; masked on every rig gate by DisableLogin=true). New Highs 03/S15, 05/STAB-19, 05/STAB-18, 03/S16 — the stale-Good family at four seams.
460 lines
52 KiB
Markdown
460 lines
52 KiB
Markdown
# 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<Degraded<Faulted
|
||
`:783-788`). Worse, the next successful read pass writes Healthy unconditionally
|
||
(`:686-692`), so a write-only PUT/GET denial (readable CPU, write-protected) is erased
|
||
within one poll interval. The read-pass fix's own rationale applies verbatim to writes,
|
||
unaddressed, and no test pins the ordering.
|
||
|
||
**STAB-23 — Medium — NEW — FOCAS `ShutdownAsync` never joins its three fire-and-forget
|
||
loops: narrow two-socket leak + generation overlap on Reinitialize.** `InitializeAsync`
|
||
starts probe / recycle / fixed-tree loops as `_ = Task.Run(...)` (`FocasDriver.cs:280,
|
||
:290, :308`) and `ShutdownAsync` (`:330-362`) only cancels + disposes the CTSes before
|
||
`state.DisposeClient()` and `_devices.Clear()`. A loop past its last cancellation
|
||
checkpoint can re-enter `EnsureConnectedAsync`, find the just-disposed client, and connect
|
||
a fresh `SynchronizedFocasClient` (two TCP sockets) that nothing ever disposes; after
|
||
`ReinitializeAsync` the stale-generation loop can briefly run alongside the new
|
||
generation's against the same CNC. TwinCAT closed exactly this hole by retaining and
|
||
awaiting `ProbeTask` (`TwinCATDriver.cs:167-168, :201-214`). Same fix, three loops.
|
||
|
||
**STAB-24 — Medium — NEW — Sql: text-bound parameters break numeric key/where columns on
|
||
Postgres, and the fixture cannot show it.** Both value-bearing fields are captured as text
|
||
(`SqlEquipmentTagParser.ReadScalar`, `SqlEquipmentTagParser.cs:130-138`) and bound as CLR
|
||
strings (`SqlPollReader.cs:401-408`); the reader's own comment asserts "the provider
|
||
coerces on the way in" (`:533-535`) — true for SqlClient (implicit conversion) and ODP.NET
|
||
(VARCHAR2→NUMBER), **false for Npgsql**: Postgres does not implicitly cast text↔integer in
|
||
comparisons, so `WHERE station_id = $1(text)` / `keycol IN ($1..)` against an
|
||
`integer` column fails with *operator does not exist* → the whole group returns
|
||
`GroupResult.Failed` → every member publishes `BadCommunicationError` on every poll, with
|
||
health guidance pointing the operator at "the table/column, the grant" (`SqlDriver.cs:664-676`)
|
||
— the wrong system. The design explicitly intends numeric key columns ("an INTEGER station
|
||
id, say"). The Postgres integration fixture uses a `varchar` key column and only the
|
||
unbound `topByTimestamp` wide-row selector
|
||
(`tests/.../Sql.IntegrationTests/Docker/postgres/seed-pg.sql:56-72`,
|
||
`PostgresReadTests.cs:278-280`) — the house fixture-shaped-verification failure mode.
|
||
*Recommendation:* bind with a provider type inferred from the catalog column type (the
|
||
gate already reads it), or cast the parameter in the emitted SQL for the Postgres dialect;
|
||
add integer-key legs to the Postgres/Oracle fixtures.
|
||
|
||
**STAB-25 — Medium — NEW — MQTT Sparkplug writes encode with the authored/default data
|
||
type, never the birth-declared type.** The read path resolves
|
||
`def.DataTypeAuthored ? def.DataType : birthType` (`SparkplugIngestor.cs:1238`), but the
|
||
write path consults only `def.DataType` (`MqttWriteEncoder.cs:209`). A Sparkplug tag
|
||
authored without `dataType` — the supported shape `UntilStable` discovery exists for —
|
||
defaults to `String` (`MqttTagDefinitionFactory.cs:235`), so a write to a birth-declared
|
||
`Int32` metric goes out as `Datatype=String`/`StringValue="42"` — and the encoder's own
|
||
remarks (`MqttWriteEncoder.cs:241-246`) name a silently-dropped command as "the failure
|
||
mode this whole feature must not have". *Recommendation:* resolve the write datatype
|
||
through the birth binding the way `PublishMetric` does.
|
||
|
||
**STAB-26 — Medium — NEW — MQTT writes are QoS-0 fire-and-forget yet return Good.**
|
||
`MqttWriteEncoder.WriteQos = 0` (`MqttWriteEncoder.cs:77`) for Plain and NCMD/DCMD;
|
||
`WriteAsync` returns `Good` when `PublishAsync` completes (`MqttDriver.cs:731-733`) —
|
||
at QoS 0 that means "bytes reached the socket", so during a half-open TCP session the
|
||
write vanishes, the driver reports success, and the server's write-outcome
|
||
self-correction (#5) never reverts. The optimistic posture is documented
|
||
(`MqttDriver.cs:677-691`) and Sparkplug NCMD arguably wants QoS 0, but for Plain-mode
|
||
`writeTopic` publishes QoS 1 would make Good mean "broker accepted" at trivial cost — and
|
||
the QoS is a non-configurable constant.
|
||
|
||
**STAB-27 — Medium — NEW — MQTT Sparkplug primary-host claim retries at 1 Hz forever with
|
||
no backoff.** `SparkplugPrimaryHost.RoleSampleInterval` is 1 s (`SparkplugPrimaryHost.cs:67`);
|
||
a broker that accepts CONNECT but refuses the STATE publish (an ACL without
|
||
`spBv1.0/STATE/#` — the driver's own docs call this "the ordinary way this happens",
|
||
`MqttDriver.cs:1357-1359`) produces a 1 Hz connect/teardown loop plus one warning per tick
|
||
(`SparkplugPrimaryHost.cs:335-343`) — ~86k warnings/day, no escalation. The ingest
|
||
connection's supervisor has exponential backoff; the claim loop has none.
|
||
|
||
**STAB-28 — Medium — NEW — Calculation's malformed-reinit fallback reverts to the
|
||
construction-time tag set.** `_options` is `readonly`; `ParseOptions`'s fallback
|
||
(`CalculationDriver.cs:236`) returns `RawTags = _options.RawTags` under the comment
|
||
"keeping the current tag table" — but after one successful delta the ctor set is stale, so
|
||
a second, malformed delta actively rebuilds back to the *spawn-time* tags
|
||
(`RebuildTagTable`, `:286`), silently un-deploying every tag added since. Latent (the host
|
||
emits well-formed merged config), but the fallback exists exactly for the case it
|
||
mishandles.
|
||
|
||
**STAB-29 — Medium — NEW — Calculation's `_pending` queue is unbounded with no per-tag
|
||
coalescing.** Every dep arrival enqueues one `PendingEvaluation` (with its own snapshot
|
||
dictionary) per change-triggered dependent (`CalculationDriver.cs:344-349`), drained by a
|
||
single pump at worst one per `RunTimeout` (2 s default). A fast-ticking dependency feeding
|
||
a slow script grows memory and publish latency without bound and publishes stale results
|
||
in trigger order. No cap, no coalesce, no queue-depth diagnostic — the timer path has both
|
||
an in-flight skip and a `SkippedTickCount` metric (`CalculationTimerScheduler.cs:62-87`);
|
||
the change path has neither.
|
||
|
||
**STAB-30 — Low — NEW — MQTT lifecycle residuals.** (a) `ShutdownAsync`'s wedged-gate
|
||
fallback lacks the `_disposed`-flag defence `DisposeAsync` has (`MqttDriver.cs:385-416` vs
|
||
`:999-1003`) — a wedged initialize can complete after shutdown returns and publish a live
|
||
connection on a driver the host believes stopped (self-heals on the next lifecycle call).
|
||
(b) A failed `ConnectAsync` in `InitializeCoreAsync` abandons the never-published
|
||
`MqttConnection` undisposed (`:992` vs the disposing sibling paths `:1001, :1021-1025`).
|
||
(c) A later subscription's stronger QoS on an already-subscribed topic is silently
|
||
deferred to the next reconnect (`MqttSubscriptionManager.cs:361, :554-572`). (d) A
|
||
transient failure of the *initial* SUBSCRIBE degrades tags to `BadCommunicationError` with
|
||
no retry until the connection drops (`:311-316, :581-651` — deliberate for permanent ACL
|
||
refusals, but it also covers a broker hiccup; contrast Sparkplug, where a refused group
|
||
filter faults the initialize into the resilience retry loop).
|
||
|
||
**STAB-31 — Low — NEW — TwinCAT sync `Dispose` can dispose `ConnectGate` under a live
|
||
probe loop.** `Dispose()` waits at most 2 s for `ProbeTask` (`TwinCATDriver.cs:1104-1107`)
|
||
then unconditionally `DisposeGate()` (`:1111`); a probe wedged in the blocking SDK
|
||
`Connect` (STAB-12c) then throws `ObjectDisposedException` from its next
|
||
`ConnectGate.WaitAsync` (`:1052, :993`), which the loop shields only as OCE (`:739-740`) —
|
||
the ODE escapes as an unobserved task exception and the loop dies silently. `ShutdownAsync`
|
||
(unbounded await) is safe; only sync `Dispose` has the hole.
|
||
|
||
**STAB-32 — Low — NEW — FOCAS: sync-over-async `Dispose` and two plain shared
|
||
dictionaries.** `public void Dispose() => 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.
|