From ad2b5c4044946641dd3f23afb8f23052b9cb29cf Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:04:58 -0400 Subject: [PATCH] docs: add 2026-10-21 system review + remediation plan Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/reviews/2026-10-21/00-SUMMARY.md | 117 ++++ docs/reviews/2026-10-21/01-opcuaserver.md | 175 ++++++ docs/reviews/2026-10-21/02-runtime-cluster.md | 308 ++++++++++ .../03-host-controlplane-security-config.md | 377 ++++++++++++ docs/reviews/2026-10-21/04-adminui.md | 198 ++++++ docs/reviews/2026-10-21/05-core.md | 124 ++++ docs/reviews/2026-10-21/06-drivers-session.md | 297 +++++++++ docs/reviews/2026-10-21/07-drivers-plc.md | 277 +++++++++ .../2026-10-21/08-historians-clients.md | 226 +++++++ .../plans/2026-10-22-review-remediation.md | 565 ++++++++++++++++++ 10 files changed, 2664 insertions(+) create mode 100644 docs/reviews/2026-10-21/00-SUMMARY.md create mode 100644 docs/reviews/2026-10-21/01-opcuaserver.md create mode 100644 docs/reviews/2026-10-21/02-runtime-cluster.md create mode 100644 docs/reviews/2026-10-21/03-host-controlplane-security-config.md create mode 100644 docs/reviews/2026-10-21/04-adminui.md create mode 100644 docs/reviews/2026-10-21/05-core.md create mode 100644 docs/reviews/2026-10-21/06-drivers-session.md create mode 100644 docs/reviews/2026-10-21/07-drivers-plc.md create mode 100644 docs/reviews/2026-10-21/08-historians-clients.md create mode 100644 docs/superpowers/plans/2026-10-22-review-remediation.md diff --git a/docs/reviews/2026-10-21/00-SUMMARY.md b/docs/reviews/2026-10-21/00-SUMMARY.md new file mode 100644 index 00000000..f02b2ea0 --- /dev/null +++ b/docs/reviews/2026-10-21/00-SUMMARY.md @@ -0,0 +1,117 @@ +# OtOpcUa system review — merged summary + +Master @ `d51d35bd`, 2026-10-21/22. 8 parallel Fable reviewers, read-only, every finding cited to file:line in the per-slice reports: + +| # | Slice | Report | Findings | +|---|---|---|---| +| 1 | OpcUaServer | `01-opcuaserver.md` | ~10 | +| 2 | Runtime + Cluster | `02-runtime-cluster.md` | 21 | +| 3 | Host / ControlPlane / Security / Config | `03-host-controlplane-security-config.md` | 19 | +| 4 | AdminUI | `04-adminui.md` | 24 | +| 5 | Core libraries | `05-core.md` | 12 | +| 6 | Session drivers (Galaxy, OpcUaClient, MQTT, MTConnect, Sql) | `06-drivers-session.md` | ~25 | +| 7 | PLC drivers (AB, Modbus, S7, TwinCAT, FOCAS, Calculation) | `07-drivers-plc.md` | 46 | +| 8 | Historians + clients + analyzers | `08-historians-clients.md` | 20 | + +## Cross-cutting themes (fix the pattern, not N instances) + +1. **The host never disposes a driver — only `ShutdownAsync`** (`DriverInstanceActor.cs:1550`, `DiscoveryDriverBrowser.cs:128`). Any driver whose teardown lives in `Dispose[Async]` leaks per respawn / per browse: + Galaxy (whole runtime + mxaccessgw session — **Critical**), Calculation (collectible script ALCs), OpcUaClient (logger factory, gate, CTS). Fix once in the runtime (dispose after shutdown) and audit each driver's `ShutdownAsync`. +2. **Timed-out scripts leak a thread-pool thread per evaluation** — found independently by 3 reviewers: `TimedScriptEvaluator` (scripted alarms + virtual tags, `TimedScriptEvaluator.cs:78`, `RoslynVirtualTagEvaluator.cs:158`) and `CalculationEvaluator.cs:187`. One infinite-loop script trends the node to pool starvation while health stays Healthy. Needs a quarantine/breaker after N timeouts plus a cap on outstanding orphaned evaluations. The sandbox also allows `System.Threading` (blocking primitives). +3. **The health/watchdog contract is wrong in both directions across drivers.** + - *Refusals counted as comms faults, which triggers 90 s reconnect loops on a reachable device:* AbCip H1/F9, AbLegacy F5, Modbus H4, S7 H4, TwinCAT `NotSupportedException` write. + - *Real outages that never degrade:* TwinCAT dead AMS router, TwinCAT native mode (`LastSuccessfulRead` never advances), FOCAS FIXED-only, Galaxy EventPump death, OpcUaClient alarm ack swallowing errors, MQTT failed SUBSCRIBE. + - *Device blackout from a probe-tag typo:* AbCip F1. +4. **A plain `Dictionary` is mutated during re-init while the poll loop reads it:** FOCAS `_devices`, AbCip device tables, AbLegacy `_devices` + `_tagsByRawPath`. The swap-by-reference fix already exists for some tables; apply it to all. +5. **One dead or slow device stalls every device in the driver** because reads are serial with no per-device backoff: AbLegacy (no backoff at all, can trip the watchdog on a healthy device), FOCAS, TwinCAT all-or-nothing native subscribe, S7 span-wide failure. +6. **Array writes are advertised writable but unimplemented** in AbCip, AbLegacy and TwinCAT: scalar encode, stale-element rewrite, or a cast exception reported as `BadTypeMismatch` + `Degraded`. +7. **Config knobs bound or authored but never consumed:** MQTT `birthObservationWindowSeconds`, Galaxy `DiscoverPageSize`, `ProbeTimeoutSeconds` (Galaxy, OpcUaClient), Modbus Family/MELSEC/DisableFC23, `TelemetryDialOptions.CallTimeoutSeconds`, plus `/auth/token` JWTs that nothing validates. +8. **Mailbox-blocking `ReceiveAsync` in singletons and driver actors:** AdminOps (a 60 s Test Connect stalls fleet-wide acks and deploys), DriverInstanceActor writes/subscribes (spurious write timeouts and reverts), and untimed teardown waits in PostStop / alarm-engine Dispose. + +## Ranked top findings + +### Critical / High +| # | Finding | Where | +|---|---|---| +| 1 | Galaxy `ShutdownAsync` is a no-op, so every respawn/deploy/browse leaks a full runtime + gateway session | `GalaxyDriver.cs:735` | +| 2 | Uncapped raw HistoryRead (`NumValuesPerNode=0`) materialises the whole window on all 3 backends; reachable anonymously with ACL off | `HistoryPaging.cs:116`, `SqlServerHistorianDataSource.cs:99`, QuestDb `:101`, Gateway `:213` | +| 3 | Deploy pipeline wedges permanently (`Dispatching` with 0 node rows → `0==0` deadline branch never clears; every later deploy refused) | `ConfigPublishCoordinator.cs:163-200,355` | +| 4 | OPC UA SDK start failure swallowed; node with 4840 unbound stays Ready, Primary-eligible, and acks Applied | `OtOpcUaServerHostedService.cs:163` | +| 5 | Script timeout thread leak (theme 2) | 3 sites | +| 6 | `ScriptedAlarmHostActor` restart reuses its own disposed engine, so scripted alarms die silently on that node | `ScriptedAlarmHostActor.cs:676` | +| 7 | Driver-dispose gap leaks Calculation script ALCs on every calc edit (theme 1) | `DriverInstanceActor.cs:1550` | +| 8 | Removed tags resurrected as root-parented phantoms by a racing publish, poisoning a later re-add | `OtOpcUaNodeManager.cs:941` | +| 9 | New subscriptions bind to dead `NodeState` after rebuild/remove (SDK `MonitoredNodes` map never cleared): `Good` on create, never data | `OtOpcUaNodeManager.cs` vs SDK `MonitoredNodeMonitoredItemManager.cs:75` | +| 10 | Galaxy spawns a new `DeployWatcher` per re-init without stopping the old one | `GalaxyDriver.cs:631` | +| 11 | Galaxy per-platform ScanState probing structurally dead (handles never registered) | `PerPlatformProbeWatcher.cs:125` | +| 12 | AdminUI script >32 KB silently stops syncing and Save persists stale source (SignalR 32 KB default) | `monaco-init.js:213`, no `AddHubOptions` | +| 13 | Gateway capped events read breaks out of the gRPC stream, so the gateway evicts its pooled session every time | `HistorianGatewayClientAdapter.cs:111` | +| 14 | AbCip probe-tag typo, then permanent device blackout (Test Connect shows green) | `AbCipDriver.cs:666-701` | +| 15 | TwinCAT native subscribe all-or-nothing: 0 data + 2k register/delete per 30 s, health Healthy | `TwinCATDriver.cs:690-721` | +| 16 | TwinCAT native `LastSuccessfulRead` never advances, so one bad write forces a full reconnect; a dead router never degrades | `TwinCATDriver.cs:540,889` | +| 17 | FOCAS `_devices` plain Dictionary cleared under 4 live readers | `FocasDriver.cs:46,412` | +| 18 | FOCAS macro/param `(short)` narrowing: `MACRO:99999` reads var −31073 at Good | `WireFocasClient.cs:408` | +| 19 | AbLegacy strictly serial, no backoff: one dead device stalls all and trips the watchdog on healthy ones | `AbLegacyDriver.cs:375-479` | +| 20 | Modbus transport never self-heals after 2 consecutive socket failures | `ModbusTcpTransport.cs:66` | +| 21 | Modbus deadband swallows Good-after-Bad recovery at the same value, so the node stays Bad | `ModbusDriver.cs:236` | +| 22 | S7 one mis-authored tag faults the whole driver at init (10 s retry loop); no editor or deploy-gate check | S7 H3 | +| 23 | Calculation compile-error script recompiled every trigger under the driver-wide gate | `CompiledScriptCache.cs:71` | + +### Medium highlights +- **Performance:** + - OpcUaServer: rebuild O(k²) per folder under `Lock`; equipment-subtree removal O(M·N); full events reconcile on every condition removal. + - Runtime: only 4 drivers implement `IBatchSubscribable`; historization fsync per burst caps throughput; the deploy apply runs on the single pinned OPC UA thread. + - AbCip: `MaxConcurrentReads=8` caps native MSP packing at 8 services per round trip (~12× more round trips than needed). + - QuestDB: write = 3 RTT + a 10 ms WAL poll per group. + - Alarm-history writers: 1 event per round trip on all backends. + - AdminUI: per-keystroke full Roslyn compilation with no cancellation; `/hosts` full re-render per health publish; `/raw` tree not virtualised. + - MTConnect: `/current` fetched and fully parsed per read. + - OpcUaClient: a single `_gate` serialises everything. +- **Leaks:** + - Scripted-alarm `_valueCache` never pruned on reload. + - Deployment artifact blobs and audit tables have no retention. + - OpcUaClient half-created subscription per subscribe retry. + - AdminUI driver-status stores never evict (ghost `/hosts` rows). +- **Correctness:** + - `StartDeployment` reads config twice with no transaction (the validated set ≠ the shipped artifact). + - One duplicate audit event drops the whole 500-event batch. + - API-key pepper not validated at start. + - Pre-auth DoS: distinct unknown key IDs each cost a SQL query under the SessionManager lock. + - Gateway `TimeAverage`/`Total` return wrong values at Good. + - Gateway writer relabels Local timestamps as UTC. + - Value writers swallow OCE (the recorder's shutdown guard is dead). + - FOCAS `String` type publishes numbers at Good. + - AB/TwinCAT bit-in-word RMW hardcodes a 32-bit parent. + - AbCip BOOL-array element read decodes the wrong bit. + - AdminUI single-click deletes without confirmation on the equipment page and ScriptEdit; Monaco offers `ctx.SetVirtualTag`, which the runtime drops. + +## Checked and fine (don't re-chase) +- **Monitored items and continuation points:** monitored-item / event-item map lifecycle and continuation-point bounds. +- **Auth:** ACL decision cache; the gRPC auth interceptors; ApiKey constant-time compare. +- **Persistence and SQL:** DbContext lifetimes + `AsNoTracking`; all dynamic SQL identifiers regex-validated (no injection); historization outbox disk-backed and bounded. +- **Script and dependency engine:** collectible-ALC unload (when disposal actually happens); DependencyGraph Tarjan. +- **Transport and wire layers:** + - The whole `Ab.Transport` layer and native lease balance. + - ADS handle lifetimes. + - FOCAS wire parsers. + - Modbus desync recovery. + - MQTT teardown + Sparkplug cache bounds. + - MTConnect stream handling. + - SQL driver disposal/injection hygiene. +- **Actor and polling plumbing:** `PollGroupEngine`; actor timer/DPS subscription cleanup. + +## Cross-repo corroboration (mxaccessgw session, 2026-10-22) +The gateway review reports two things that bear on the Galaxy session leak (finding #1): +- **Leaked sessions are held for a long time.** A session that never calls `StreamEvents` is held for the full `DefaultLeaseSeconds` (1800 s). Detach-grace only starts after a subscriber has attached at least once, `GatewaySession.ExtendLease` has no callers, and there is no renew RPC. So each leaked GalaxyDriver session pins an x86 worker for 30 min, and about 64 respawns in that window would use up `MaxSessions`. +- **The open-sessions gauge drifts upward.** Sessions closed by the timed sweep never decrement `mxgateway.sessions.open` (`TryBeginCloseIfExpired` sets `_closeStarted`, so `CloseSessionCoreAsync` skips `SessionClosed()`). + +This makes #1 a fleet-availability risk, not just a leak. + +## Suggested remediation order +1. Theme 1 (runtime disposes drivers) + Galaxy `ShutdownAsync`/DeployWatcher (#1, #7, #10). Small change, removes the only Critical. +2. HistoryRead server-side cap/paging (#2). +3. Deploy-pipeline wedge + OPC UA start-failure health (#3, #4). +4. Script timeout quarantine (theme 2, #5, #23) + scripted-alarm restart (#6). +5. Node-manager phantom/stale-MonitoredNode pair (#8, #9). Needs a live gate, since unit tests won't show it. +6. Driver health-contract sweep (theme 3) + Dictionary swaps (theme 4) as one batch per driver family. +7. AdminUI 32 KB hub limit (#12). One-liner. diff --git a/docs/reviews/2026-10-21/01-opcuaserver.md b/docs/reviews/2026-10-21/01-opcuaserver.md new file mode 100644 index 00000000..9da762cd --- /dev/null +++ b/docs/reviews/2026-10-21/01-opcuaserver.md @@ -0,0 +1,175 @@ +# Review 01 — `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer` (master @ d51d35bd) + +Scope: `OtOpcUaNodeManager` (6,988 lines), `AddressSpaceApplier`, `AddressSpaceComposer`, `AddressSpacePlan`/`Planner`, +`AddressSpaceChangeClassifier`, `SdkAddressSpaceSink`, `OtOpcUaSdkServer`, `OpcUaApplicationHost`, `Security/*`, +`HistoryContinuationStore`, the HistoryRead helpers (`ReverseWalk`, `ReverseRawReader`, `ReverseEventsReader`, +`HistoryBounds`, `HistoryPaging`, `RawPageLimit`), `AdvertisedAggregateFunctions`, `SdkServiceLevelPublisher`. + +SDK behaviour claims below were verified against the pinned SDK sources +(`~/Desktop/OpcUaTestServer/external/UA-.NETStandard`, 1.5.378 line): `NodeState.cs`, +`Diagnostics/CustomNodeManager.cs`, `NodeManager/MonitoredItem/MonitoredNodeMonitoredItemManager.cs`, +`NodeManager/MasterNodeManager.cs`. All paths are absolute; line numbers are from the current master. + +--- + +## 1. Memory / resource leaks + +### L1. Lazy `CreateVariable` resurrects removed (or never-declared) NodeIds as orphan variables under the shared root — and then poisons a later re-add +- **Severity:** High · **Confidence:** High (mechanism) / Medium (trigger frequency) +- **Location:** `/Users/dohertj2/Desktop/OtOpcUa/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs:937-945` (`WriteValueLocked`), `:1059-1071` (`WriteAlarmCondition` fallback), `:6964-6986` (`CreateVariable`), `:2974-2975` (`EnsureVariable` early return) +- **Evidence:** + ```csharp + if (!_variables.TryGetValue(key, out var variable)) + { + variable = CreateVariable(nodeId, ns); // parented under _root, DataType=BaseDataType, CurrentRead only + _variables[key] = variable; + } + ``` + and `EnsureVariable`: `if (_variables.ContainsKey(key)) return;` +- **Failure scenario:** `WriteStatuses` was deliberately made non-creating because "the caller's per-driver ref index can legitimately name a node a surgical remove has just dropped" (`:825-828`). `WriteValues`/`WriteValue` still create. So a driver publish (or the UNS fan-out) that races a surgical `RemoveVariableNode` re-creates `s=` (and `s=`) as a flat, read-only, `BaseDataType` variable **directly under `OtOpcUa`**, browsable, with no `OnWriteValue`, no historian registration, no `_arrayOnScalarRefused`. It stays until the next *full* rebuild — surgical deploys made that rare. Worse: if the same tag is later re-added surgically, `EnsureVariable` early-returns on the phantom, so the "re-added" tag permanently keeps the phantom's wrong parent/DataType/writability. The planner never names the phantom, so no surgical remove ever reaches it. +- **Suggested fix:** Make `WriteValueLocked` non-creating (mirror `WriteStatusLocked`: unknown key ⇒ skipped, counted, Debug-logged). The create-on-first-write path predates `EnsureVariable` and has no legitimate production producer left. If some test relies on it, gate it behind an explicit `AllowLazyNodeCreation` that defaults off. + +### L2. Stale `MonitoredNode2` survives a rebuild / remove+recreate: new subscriptions bind to the dead `NodeState`, and ACL "restore" re-attaches to dead nodes +- **Severity:** High · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:3471-3537` (`RebuildAddressSpace`), `:3573-3607` (`RemoveVariableNode`), `:4801` / `:4889` (revalidation restore). SDK: `MonitoredNodeMonitoredItemManager.cs:75-83`. +- **Evidence (SDK):** + ```csharp + if (!MonitoredNodes.TryGetValue(handle.Node.NodeId, out MonitoredNode2 monitoredNode)) { ... new MonitoredNode2(m_nodeManager, cachedNode); } + handle.Node = monitoredNode.Node; // <-- the OLD NodeState + ``` + `RebuildAddressSpace`/`RemoveVariableNode` detach nodes from `PredefinedNodes` and the parent, but never touch the manager's `MonitoredNodes` map. `DeleteMonitoredItem` only drops the entry when its item count hits zero. +- **Failure scenario:** Client A (e.g. ScadaBridge) holds an item on `Plant/Modbus/dev1/Speed`. A deploy rebuilds (or surgically removes + re-adds) that node. A's item goes silent (known). Now ANY session — A after reconnecting, or a UaExpert operator — creates a new item on the same NodeId: the SDK reuses the existing `MonitoredNode2` and sets `handle.Node` to the **dead** NodeState, so the new item never receives a value either, with `Good` create status. Only when *every* session has deleted its items on that NodeId does a fresh create work. Same class: `RevalidateMonitoredItems` restore does `tracked.Handle.MonitoredNode?.Add(item)` and `RevalidateEventItems` calls `SubscribeToEvents(SystemContext, tracked.Handle.Node, …)` — after a rebuild both re-attach to dead nodes. Memory-wise each live item pins its dead subtree via `LiveItem.Handle` (bounded by item count, but it's the whole detached subtree). +- **Suggested fix:** In `RebuildAddressSpace`, `RemoveVariableNode`, `RemoveAlarmConditionNode`, `RemoveEquipmentSubtree`: for each removed NodeId, `MonitoredNodes.TryGetValue(nodeId, out var mn)` → queue `BadNodeIdUnknown` to each `mn.DataChangeMonitoredItems`, then `MonitoredNodes.Remove(nodeId)` (and drop the matching `_liveItems`/`_eventItems` entries). For a rebuild that recreates the same NodeId you could instead re-point the `MonitoredNode2` to the new `NodeState` — but removing it and letting clients re-create is simpler and matches the documented "rebuild kills subscriptions" contract while making re-create actually work. + +### L3. `OnEquipmentTagWrite` continuation never observes a faulted write task +- **Severity:** Low · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:2202-2209` +- **Evidence:** `var outcome = t.IsCompletedSuccessfully ? t.Result : new NodeWriteOutcome(false, "write dispatch faulted");` — `t.Exception` is never read on the faulted branch. +- **Failure scenario:** Every faulted `WriteAsync` (gateway Ask timeout/exception) raises `TaskScheduler.UnobservedTaskException` at finalization; with a handler that logs, each failed device write becomes a late, context-free error line; the real reason is never logged here either (`outcome.Reason` is the constant string). +- **Suggested fix:** On the non-success branch read `t.Exception?.GetBaseException()` into the reason and log it (the pattern `AddressSpaceApplier.ProvisionHistorizedTags:727-733` already uses). + +### L4. Legacy `bool[2]` placeholder path is reachable in production and transiently shadows the equipment folder in `PredefinedNodes` +- **Severity:** Medium · **Confidence:** Medium +- **Location:** `OtOpcUaNodeManager.cs:1059-1071`; producer `AddressSpaceApplier.cs:100-103` (`SafeWriteAlarmCondition(eq.EquipmentId, RemovedConditionState, …)`). +- **Evidence:** The applier writes `RemovedConditionState` against `eq.EquipmentId` for every removed equipment; an equipment id is a *folder*, never a condition, so `_alarmConditions` misses and the "legacy two-element placeholder … (and the existing unit tests)" branch runs `CreateVariable(alarmNodeId, ns)` + `AddPredefinedNode`. SDK `AddPredefinedNode` is `PredefinedNodes.AddOrUpdate(...)` (`CustomNodeManager.cs:586`), so the phantom variable **replaces the equipment folder's `PredefinedNodes` entry** (same realm + same `s=` id) until the folder loop in `RemoveEquipmentSubtree` (or the rebuild) removes it. +- **Failure scenario:** During a PureRemove/Rebuild apply a concurrent Read/Browse of the equipment folder resolves to a `bool[2]` variable. The phantom is cleaned up by the same apply on every path I traced (InScope matches `key.Id == equipmentNodeId`; an early `false` ratchets to rebuild), so it is transient — but it is a production path running through a self-described test fallback, and it does one needless `ClearChangeMasks` per removed equipment. +- **Suggested fix:** Make `WriteAlarmCondition` a logged no-op for unknown ids (as `WriteAlarmQuality` already is), and drop the applier's equipment-id condition write (an equipment never has a condition node). + +### L5. (bounded, note) `_notifierFolders` / `_eventNotifierSources` not demoted on `RemoveAlarmConditionNode` +- **Severity:** Low · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:3637-3664` +- Documented and bounded by folder count; a raw device/group folder promoted for a native alarm stays a root notifier (and an advertised event-history source) after its last condition is removed until a rebuild. Harmless except `HistoryReadEvents` on it answers from the historian for a folder with no alarms. No action needed beyond awareness. + +--- + +## 2. Unimplemented / stubbed / dormant + +### U1. `HistoryReadEvents` projects `EventType`, `SourceNode`, `ConditionName` (and every other field) as `Variant.Null` +- **Severity:** Low · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:5942-5957` +- Only `EventId/SourceName/Time/ReceiveTime/Message/Severity` are populated. A client whose select clause includes `EventType` (most generic alarm viewers) gets nulls; a where-clause on `OfType` cannot be honoured. Documented as "spec-conformant", but it is a functional gap for historical alarm browsing. Fix: `HistoricalEvent` would need to carry `EventType`/`ConditionName`; map `SourceNode` from the source name when resolvable. + +### U2. Equipment-tag materialisation + its surgical branch are dead code in v3 +- **Severity:** Low · **Confidence:** High +- **Location:** `AddressSpaceComposer.cs:590-592` (`EquipmentTags = Array.Empty()` always), `AddressSpaceApplier.cs:197-215` (surgical `ChangedEquipmentTags` loop), `:1098-1161` (`MaterialiseEquipmentTags`), `AddressSpaceChangeClassifier.cs:162-172`. +- The composer never emits equipment tags, so the applier's `MaterialiseEquipmentTags` pass, the `UnsEquipmentVar`/alarm-under-equipment branch and `TagDeltaIsSurgicalEligible` are unreachable in production (kept "structurally empty"). Not a bug; it is maintained, tested dead weight that still participates in `IsEmpty`/classification. Candidate for removal. + +### U3. Historian capability bits are fixed at promotion/materialise time +- **Severity:** Low · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:2831-2848` (`EnsureFolderIsEventNotifier` registers `_eventNotifierSources` only if `_historianDataSource is not NullHistorianDataSource` *now*), `:2987` / `:3084` (`HistoryWrite` bit depends on `_historianDataUpdater` *now*). +- Documented. Consequence: a `HistorianDataSource`/`HistorianDataUpdater` assigned after the first apply leaves existing folders without event history / HistoryUpdate until a full rebuild, and `SetHistorianDataSource(null)` later does **not** remove the HistoryRead bit from already-promoted folders (they then answer `BadHistoryOperationUnsupported`). Acceptable given the Host ordering, but there is no runtime guard that the ordering held. + +### U4. Native conditions: Confirm / shelve / Enable / Disable ⇒ `BadNotSupported`; HistoryUpdate events/structure arms not overridden +- **Severity:** Low · **Confidence:** High +- **Location:** `:1496-1540`, base `HistoryUpdate` dispatch (not overridden for `UpdateEventDetails`/`UpdateStructureDataDetails`). +- Both are deliberate and documented; listed so the parent does not re-investigate. + +### U5. Test seams present in production type (not dormant DI, just noting) +- `HistoryContinuationStore` internal setter + `InMemoryHistoryContinuationStore` (`HistoryContinuationStore.cs:104-158`), `SessionAclDecisionCache.GetOrAdd` ("production has no caller"), `AllEventsMonitoringConditionVisits`, `TrackLiveMonitoredItem` internal. All default to the production implementation; none alters behaviour. Fine. + +--- + +## 3. Performance limiters + +### P1. `RebuildAddressSpace` is O(k²) per folder because `NodeState.RemoveChild` is a linear scan + `RemoveAt` +- **Severity:** Medium · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:3475-3479`, `:3495-3499`, `:3513-3517`; SDK `NodeState.cs:4247-4265`. +- **Evidence:** `foreach (var v in _variables.Values) { v.Parent?.RemoveChild(v); PredefinedNodes?.Remove(v.NodeId); }` and SDK `RemoveChild` walks `m_children` with `ReferenceEquals` then `RemoveAt(ii)` (element shift). Removing all k children of one folder, in map order, costs ~k²/2 compares + shifts, all under `Lock`. +- **Failure scenario:** A device folder with 10k tags ≈ 50M compares under the manager lock on every full rebuild (alarm-bearing delta, container rename, etc.), stalling every Read/Browse/publish for the duration; UNS equipment folders with many references multiply it. `ForgetPredefinedSubtree` also allocates one `List` per node recursively. +- **Suggested fix:** Detach whole subtrees instead of leaves: remove top-level folders from `_root` once (children go with them), then clear the maps and `PredefinedNodes` keys in bulk; or iterate `folder.GetChildren` and build a new child list. Keep `PredefinedNodes.Remove` per id (dictionary, O(1)). + +### P2. `RemoveEquipmentSubtree` scans every map on every call ⇒ O(M·N) for M removed equipment +- **Severity:** Medium · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:3685-3752` +- **Evidence:** `_variables.Keys.Where(InScope).ToList()`, same for `_alarmConditions`, `_notifierFolders`, `_eventNotifierSources`, `_folders`, plus `foreach (var wiring in _alarmNotifierWiring.Values) wiring.Folders.Where(...)` — each a full scan with `StartsWith` per key, under `Lock`. +- **Failure scenario:** A PureRemove deploy that drops 200 equipment on a 50k-node server does 200 × ~6 full-map scans ≈ 60M key comparisons under the lock, on the pinned publish thread, serialising all SDK service calls. +- **Suggested fix:** Walk the equipment `FolderState`'s children (the SDK tree already has them) to enumerate descendants, or maintain a per-equipment child index populated by `EnsureVariable`/`EnsureFolder`/`MaterialiseAlarmCondition`. + +### P3. `ReconcileAllEventsMonitoring` runs a full O(C) reconcile on every `RemoveAlarmConditionNode` and every all-events (un)subscribe +- **Severity:** Medium · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:3659`, `:3714`, `:4240`, `:4298-4322` +- **Evidence:** Builds a new `HashSet` over `_alarmConditions.Values` plus `.Where().ToList()` each call. The materialise side was already made incremental ("made a deploy of N alarms O(N²) under Lock") but the removal side was not. +- **Failure scenario:** Surgically removing M alarm-bearing raw tags or scripted alarms costs O(M·C) under `Lock`; a client that creates/modifies many `i=2253` items (e.g. per-subscription) pays O(C) per call. +- **Suggested fix:** Add `LowerAllEventsMonitoring(condition)` (mirror of `RaiseAllEventsMonitoring`) for the two surgical removal sites; keep the full reconcile for rebuild and all-events (un)subscribe. + +### P4. HistoryRead/HistoryUpdate arms block SDK request threads (sync-over-async) with a 5 s admission wait +- **Severity:** Low · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:5997-6014` (`RunBounded` → `Task.WhenAll(...).GetAwaiter().GetResult()`), `:6065-6080` (`gate.Wait(≤5 s)`). +- Documented and bounded (16 batches × 4 nodes), and HistoryRead is not under `Lock`. Under a slow historian (deadline 60 s) up to 16 SDK request threads sit blocked for a minute while 16 more block 5 s each before fast-failing; with `MaxRequestThreadCount = 100` this is survivable but eats 1/3 of request capacity. Consider `MaxConcurrentHistoryReadBatches` default lower on small nodes, or an async `HistoryReadAsync` override if the SDK version exposes one (`IAsyncNodeManager` exists in this SDK line). + +### P5. `WireAlarmNotifiers` / SDK notifier lists are linear per folder +- **Severity:** Low · **Confidence:** High +- **Location:** `:1723-1760`; SDK `NodeState.AddNotifier` (`NodeState.cs:2385-2410`, linear dedup). +- An equipment folder referencing K native alarms costs O(K²) `ReferenceEquals` on wiring plus per-call `ToHashSet`/`Where().ToList()`/`Any` allocations. Bounded by K (tens–hundreds); only matters on rebuild of alarm-heavy equipment. + +### P6. Hard-coded SDK request quotas +- **Severity:** Low · **Confidence:** High +- **Location:** `OpcUaApplicationHost.cs:435-441` — `MinRequestThreadCount = 5, MaxRequestThreadCount = 100, MaxQueuedRequestCount = 200`, `TransportQuotas = new TransportQuotas()` (defaults). +- Not configurable from `appsettings`; a burst above 200 queued requests is refused by the SDK. Fine for current fleet sizes; flagging as the ceiling. + +--- + +## 4. Other correctness / concurrency hazards + +### H1. Removed-tag resurrection + re-add poisoning (see L1) — correctness, not just leak +Already covered; the correctness half (a re-added tag silently inherits the phantom's shape) is the more serious consequence. + +### H2. `AddPredefinedNode` is AddOrUpdate: any id collision between a variable and a folder in the same realm silently replaces the folder's lookup entry +- **Severity:** Medium · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:2901`, `:3014`, `:1559`, `:6984`; SDK `CustomNodeManager.cs:586`. +- `_variables`, `_folders`, `_alarmConditions` are separate maps keyed by `(realm, id)`, so nothing prevents the same `(realm, id)` existing as both a folder and a variable (L4 shows a production producer). The SDK dictionary keeps whichever was added last. Suggest a cross-map guard in `EnsureFolder`/`EnsureVariable`/`MaterialiseAlarmCondition` (`ResolveNodeState(key) is not null ⇒ log + return`). + +### H3. `ConditionRefresh` (ACL path) walks notifier subtrees outside `Lock` +- **Severity:** Low · **Confidence:** High +- **Location:** `OtOpcUaNodeManager.cs:4646-4651` +- Only the `RootNotifiers` copy is taken under `Lock`; `notifier.ConditionRefresh(...)` then walks children while a rebuild can be mutating child lists. **The SDK base does exactly the same** (`CustomNodeManager.cs:3488-3515`), and `NodeState` guards its child list with `m_childrenLock`, so this is parity with the SDK rather than a regression. Listed so it is not re-raised. + +### H4. `RemainingShelveMs` and condition timestamps use `DateTime.UtcNow`, not `Clock` +- **Severity:** Low · **Confidence:** High +- **Location:** `:1087`, `:1452`, `:1514`, `:1926`, `:2621-2626`, `:4778`. +- Inconsistent with `HistoryUpdate`/`EventHorizon`, which use the injected `TimeProvider`; purely a testability/consistency nit. + +### H5. `OnEquipmentTagWrite` relies on `gateway.WriteAsync` never throwing synchronously +- **Severity:** Low · **Confidence:** Medium +- **Location:** `:2202` +- A synchronous throw escapes the handler while the SDK holds `Lock` in `Write`; the SDK's per-node `try` (`CustomNodeManager.cs` Write, around line 1885) converts it to a Bad result, so the lock is released — no deadlock, but the optimistic/prior bookkeeping is skipped and no audit event is raised. Wrap the call in try/catch → treat as failed outcome. + +--- + +## 5. Checked and fine (do not re-investigate) + +- **`_liveItems` / `_eventItems` / `_allEventsItems` lifecycle.** `OnMonitoredItemDeleted` removes data items; `MasterNodeManager` routes every delete/transfer of event items through `SubscribeToEvents(…, unsubscribe:true)` / `SubscribeToAllEvents(…, true)` (`MasterNodeManager.cs:3329-3345`, `:3528-3545`), which the overrides handle. Not session-leaking. +- **`DataPlaneAuthority` event handler** is detached in the setter and in `Dispose` (`:448-454`, `:488-500`). Correct. +- **`SessionAclDecisionCache`** is doubly bounded (20k/shard, 100k global) with sealed-shard counters; sharded by key, cleared on generation advance. No unbounded growth. +- **`HistoryContinuationStore`** production path is the SDK per-session store (capped at `MaxHistoryContinuationPoints`, disposed with the session). `InMemoryHistoryContinuationStore` is test-only and capacity-bounded. +- **`CreateDeadline()` CTS** is `using`-disposed per arm; `RunBounded`'s `SemaphoreSlim` is `using`-disposed; the process-wide `_historyReadBatchLimiter` is never disposed but holds no OS handle. +- **`_rebuildCarryOver` / `_restoredConditionReports`** cleared at `CompleteAddressSpaceApply`, which the publish actor calls in a `finally` (`OpcUaPublishActor.cs:500`). Bounded. +- **`AddRootNotifier` cost**: SDK copies `Server.EventManager.GetMonitoredItems()` per newly-promoted folder and calls `SubscribeToEvents(…, true)` for all-events items; the protected override short-circuits those (`:4257-4261`). O(E) per folder on first promotion only. +- **`WriteValues` chunking** (512 per `Lock` take) is a deliberate, documented trade-off; `ClearChangeMasks` under `Lock` is the SDK pattern. +- **Alarm method handlers under `Lock`** (`ReportAfterMethod`, `IsNativeAlarmNode`) — `Lock` is a reentrant `Monitor`; the router delegates are fire-and-forget by contract. +- **Revalidation lock ordering**: `QueueValue` (item lock) is taken *outside* `Lock`, then `Lock` is taken for attach/detach — no nested inversion with the publish path (`Lock` → item lock). +- **`AdvertisedAggregateFunctions.Apply`** takes the Diagnostics node manager's `Lock` from the `HistorianDataSource` setter — called from host start, never while our `Lock` is held. +- **`OpcUaApplicationHost.DisposeAsync`** detaches `ImpersonateUser` and stops the application; the hosted service stops/disposes the server (`OtOpcUaServerHostedService.cs:357-365`). +- **Impersonation `GetAwaiter().GetResult()`** is irreducible (SDK sync callback under `SessionManager` lock) and the API-key authenticator does no I/O; documented. +- **`AddressSpacePlanner.DiffById`** is dictionary-based O(n); `AddressSpaceComposer` builds all lookups as dictionaries (no quadratic joins found). `BuildEquipmentFolderPaths` is computed twice per compose (once for refs, once for raw) — trivial. +- **`ReverseWalk`** has explicit termination arguments (segment always narrows; `granularity` floor), a `LookAhead`/`Floor` clamp, and the tie-cluster budget throws rather than looping. diff --git a/docs/reviews/2026-10-21/02-runtime-cluster.md b/docs/reviews/2026-10-21/02-runtime-cluster.md new file mode 100644 index 00000000..dad511eb --- /dev/null +++ b/docs/reviews/2026-10-21/02-runtime-cluster.md @@ -0,0 +1,308 @@ +# Review 02 — Runtime + Cluster (Akka actors, deploy apply/acks, redundancy gates, mesh/config/telemetry transports, LocalDb) + +Scope: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/**` and `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/**` at master `d51d35bd`. +Production code only. Every finding below was verified by reading the cited lines; drivers / Core / ControlPlane were +read only where a Runtime behaviour depends on them. + +Severity scale: Critical / High / Medium / Low. Confidence: High = mechanism verified in code; Medium = mechanism verified, +magnitude or trigger frequency estimated. + +--- + +## 1. Memory / resource leaks + +### L1 — `IDriver` instances are never disposed; the runtime only ever calls `ShutdownAsync` (Calculation leaks compiled-script ALCs; OpcUaClient leaks a LoggerFactory + gate + CTS per respawn) +- **Severity:** High (Calculation) / Low (OpcUaClient) · **Confidence:** High (never-disposed), Medium (growth rate) +- **Location:** + - `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs:1527-1555` (`PostStop`) + - `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs:3759-3775` (`StopChild`), `:3811-3827` (`HandleRestartDriver`) + - `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriver.cs:11` (no `IDisposable`/`IAsyncDisposable`) + - `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:323-330` vs `:719-727` + - `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs:3326-3367` +- **Evidence:** the only teardown the runtime performs on a stopped child is + ```csharp + try { _driver.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult(); } // PostStop + ``` + No code path in Runtime/Host/Core.Resilience ever does `(driver as IDisposable)?.Dispose()` (grep over those trees found none). + `CalculationDriver.ShutdownAsync` only stops timers and clears `_pending`; `_evaluator.Dispose()` lives exclusively in + `CalculationDriver.Dispose()` (line 726). `CalculationEvaluator._cache` is a `CompiledScriptCache` that owns "a collectible + `AssemblyLoadContext` per evaluator" and unloads them only from its `Dispose()`/`Clear()` (`CompiledScriptCache.cs:130-135`). + `OpcUaClientDriver.ShutdownAsync` does detach the cert-validation handler, but the per-instance `SdkTelemetry.ForLogger(...)` + `LoggerFactory` (`SdkTelemetry.Release`, line 3367/3395), `_gate` and `_sessionWorkCts` are released only in `Dispose`/`DisposeAsync`. +- **Failure scenario:** `DriverSpawnPlanner` respawns a driver on ANY `DriverConfig` change (`DriverSpawnPlan.cs:62-68`), and a + Calculation driver's merged config folds in its tag/script set, so every edit to a calculation tag or script = one abandoned + `CalculationEvaluator` holding every compiled script assembly it ever loaded. Same on operator Restart and on driver removal. + On a calc-heavy node a day of authoring leaves N loaded script ALCs pinned by an unreachable-but-unloaded context; JIT code + + metadata are never reclaimed. OpcUaClient leaks one `LoggerFactory` (with its `ForwardingLoggerProvider`) + semaphore + CTS per respawn. +- **Fix:** in `DriverInstanceActor.PostStop`, after `ShutdownAsync`, do `if (_driver is IAsyncDisposable ad) await-ish ad.DisposeAsync(); else if (_driver is IDisposable d) d.Dispose();` + — but ONLY when the actor is being stopped for good, not on a supervision restart (the Props closure re-uses the same driver + object; see H2). Override `PreRestart` to skip the dispose, or have the host pass an "owned" flag. Alternatively make + `IDriver : IAsyncDisposable` and dispose in `StopChild`/`HandleRestartDriver` after `Context.Stop` via a watch. + +### L2 — `DriverHostActor._lastReprovisionUtc` is never pruned +- **Severity:** Low · **Confidence:** High +- **Location:** `DriverHostActor.cs:531` (declaration), `:3897` (write); `StopChild` (`:3759`) never removes. +- **Evidence:** `_lastReprovisionUtc[msg.DriverInstanceId] = now;` with no `Remove` anywhere. The comment says "bounded by the number + of driver children" but it is bounded by the number of *distinct ids ever reprovisioned*, which grows with every driver + that was removed after crashing. ~16 bytes/entry; cosmetic unless ids are generated per deploy (they are stable GUID-ish ids). +- **Fix:** `_lastReprovisionUtc.Remove(driverInstanceId)` in `StopChild`. + +### L3 — `DependencyMuxTagUpstreamSource._cache` retains every RawPath ever pushed, across deploys +- **Severity:** Low · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/DependencyMuxTagUpstreamSource.cs:45-48` +- **Evidence:** `// Intentionally never pruned` — `_cache[path] = snapshot` with no eviction. The engine re-registers mux interest per + apply, so renamed/removed tags stop being pushed, but their last snapshot (boxed value + two DateTimes) stays forever. +- **Failure scenario:** a site that renames raw paths often (RawTree rename is a surgical remove+add) accumulates one stale snapshot + per retired path for the process lifetime. Bounded by churn, not by live config. +- **Fix:** on `ScriptedAlarmHostActor.OnAlarmsLoaded`, prune `_cache` to the loaded dep-ref union (add a `RetainOnly(IReadOnlySet)`). + +### L4 — `TelemetryLocalHub._evicted` grows per removed driver id +- **Severity:** Low · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Telemetry/TelemetryLocalHub.cs:47, 77-89, 92-98` +- **Evidence:** `Evict` adds to `_evicted`; only `Readmit` (re-spawn of the SAME id) removes. A driver removed for good keeps its id in + the set forever. Bounded by distinct removed ids; a few bytes each. Noted because the tombstone is load-bearing (#609) and cannot + simply be cleared on a timer without re-opening the late-write resurrection. +- **Fix:** acceptable as is; if desired, stamp eviction time and sweep entries older than the longest plausible in-flight capability + call (minutes), since the tombstone only needs to outlive the stopping child. + +--- + +## 2. Unimplemented / stubbed / dormant + +### U1 — `GetDiagnostics` → `NodeDiagnosticsSnapshot` is a stub payload AND the only client has no caller +- **Severity:** Medium · **Confidence:** High +- **Location:** `DriverHostActor.cs:2499-2517`; `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Clients/FleetDiagnosticsClient.cs:35`; + `AdminUI/Clients/ServiceCollectionExtensions.cs:17` +- **Evidence:** + ```csharp + new DriverInstanceDiagnostics(DriverInstanceId: Guid.Empty, Name: kv.Key, + State: kv.Value.Stubbed ? "Stubbed" : "Spawned", ConnectedDevices: 0, FaultedDevices: 0, LastChangeUtc: DateTime.UtcNow) + ``` + Connectivity, device counts and change time are placeholders regardless of the child's actual state. `IFleetDiagnosticsClient` is + registered in AdminUI DI but no `.razor`/`.cs` resolves it (grep: only the registration + the client itself). The message is handled + in all three behaviours so it looks live, but the data is fake and nobody reads it. `RunningFromCache` is the one real field. +- **Fix:** either delete `GetDiagnostics`/`NodeDiagnosticsSnapshot`/`IFleetDiagnosticsClient` (the `/hosts` page already gets real + state over the telemetry channels) or populate it from the child health snapshots. Leaving a plausible-looking stub is the + "dormant DI registration" class the review asked for. + +### U2 — `TelemetryDialOptions.CallTimeoutSeconds` is bound and defaulted but consumed nowhere +- **Severity:** Low · **Confidence:** High +- **Location:** `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs:75-76` +- **Evidence:** `public int CallTimeoutSeconds { get; set; } = 30;` — zero references outside the options file (validators included; the + only other `CallTimeout*` hits are the unrelated Galaxy gateway option). The telemetry dialer therefore runs its server-streaming + call with no deadline from this knob, and an operator setting `TelemetryDial:CallTimeoutSeconds` changes nothing. +- **Fix:** wire it into the dialer's `CallOptions`/keep-alive, or delete it and the doc row in `docs/Configuration.md`. + +### U3 — `HistorianAdapterActor.Receive` has no production sender +- **Severity:** Low · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/HistorianAdapterActor.cs:63-66` +- **Evidence:** comment says "kept for a future engine→historian path"; `grep "new AlarmHistorianEvent("` over `src` finds no producer + (the adapter's own `Translate` is target-typed `new(`). The handler carries a WARNING about double-writes if anyone ever uses it. +- **Fix:** delete the receive (and the warning) — a latent double-historize trap with no current benefit. + +### U4 — `DependencyMuxActor.OnAttributeValuePublished` is a documented test-only seam kept in the production actor +- **Severity:** Low · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyMuxActor.cs:55, 106-123`; `DriverInstanceActor.cs:110-117` +- **Evidence:** "No production sender remains" / "treat it as dead in production". Allocates a per-subscriber single-item batch. + Harmless, but it is a message with a handler and no sender, which the review asked to flag. +- **Fix:** move the per-value shape to a test helper that wraps it in a one-item `AttributeValueBatchPublished`. + +### U5 — `DriverHostActor` constructor keeps an unused `ILoggerFactory? loggerFactory` positional parameter +- **Severity:** Low (nit) · **Confidence:** High +- **Location:** `DriverHostActor.cs:639-640, 705, 840` +- **Evidence:** "Retained for constructor-signature stability; unused". Given the file's own warning that the positional Props list is + a mis-binding hazard (`:727-731`), dead positional slots raise that risk for no benefit. + +--- + +## 3. Performance limiters + +### P1 — Per-tag drivers pay a 3-hop actor chain and one SDK sink lock per changed tag; only 4 of 12 drivers batch +- **Severity:** Medium · **Confidence:** High +- **Location:** `DriverInstanceActor.cs:975-990` (handler choice), `:1384-1390` (`OnDataChangeForward`), + `DriverHostActor.cs:1602-1644` (`ForwardBatchToMux`), `OpcUaPublishActor.cs:402-423`; + `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs:404-416` +- **Evidence:** `IBatchSubscribable` is implemented by Modbus, FOCAS, S7 and AbCip only (grep). TwinCAT, AbLegacy and Sql use the same + `PollGroupEngine` but expose only `ISubscribable`, so the engine's per-tag `_onChange(...)` fires `self.Tell(new DataChangeForward(..))` + once per changed tag; each becomes a 1-item `AttributeValueBatchPublished` → host → `_dependencyMux.Tell` + 1-item + `AttributeValueBatchUpdate(List)` → `_sink.WriteValues` (one lock acquisition each). Galaxy, MQTT, MTConnect and OpcUaClient are + per-tag by nature too. +- **Failure scenario:** a 2k-tag TwinCAT/Sql device whose values all move per 1 s cycle = ~6k actor messages/s + 2k node-manager lock + round-trips/s on the pinned OPC UA thread, versus 3 messages + 1 lock for a Modbus device of the same size. That thread is also + every client's Publish path. +- **Fix:** implement `IBatchSubscribable` on the three remaining `PollGroupEngine` drivers (the engine already builds the batch; + it is a one-line event forward), and consider a time-boxed coalescer (e.g. 10–20 ms) in `DriverInstanceActor` for the push drivers. + +### P2 — `DriverInstanceActor` suspends its mailbox for the whole device round-trip on writes/subscribes (serialized writes, head-of-line blocking of data changes) +- **Severity:** Medium · **Confidence:** High +- **Location:** `DriverInstanceActor.cs:520-523, 542` (`ReceiveAsync`, ``, ``, ``), + `:858-901` (5 s write bound), `:948-1011` (10 s subscribe + 5 s unsubscribe), `:1296-1355` (10 s alarm unsub + 10 s sub) +- **Evidence:** Akka `ReceiveAsync` suspends the actor until the task completes. A write holds the actor up to 5 s; a data + (re)subscribe up to 15 s; an alarm re-subscribe up to 20 s. Meanwhile every `DataChangeForward`/`DataChangeBatchForward` and every + further `WriteAttribute` queues behind it. The host's `Ask` for a write is 8 s (`DriverHostActor.cs:2176-2177`), so the SECOND of two + concurrent operator writes to a slow device can time out at the host ("write timeout") while the device actually accepted it later. +- **Failure scenario:** operators issuing several writes to a Galaxy/OPC UA upstream with ~2 s latency get spurious "write timeout" + reverts (the optimistic-write self-correction then reverts a value the device DID take); value publishes stall for the duration. +- **Fix:** run writes via `PipeTo` (capture `Sender`, don't suspend the mailbox) with a per-driver in-flight cap, keeping + `ReceiveAsync` only for the subscribe/unsubscribe pair that must serialize against `_subscriptionHandle`. + +### P3 — `ContinuousHistorizationRecorder` performs one awaited fsync per ingested burst on the actor thread (serial fsync cap; unbounded mailbox backlog) +- **Severity:** Medium · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs:308-309, 463-466`; + `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Recorder/FasterLogHistorizationOutbox.cs:158-169` +- **Evidence:** `ReceiveAsync(OnValueBatchAsync)` → `await _outbox.AppendBatchAsync(entries, ...)`; under the default + `CommitMode = PerEntry` that is `_log.CommitAsync` (fsync) before the handler returns. Ingest throughput is therefore + `1 / fsync_latency` bursts/s regardless of core count, and the mux keeps `Tell`ing into an unbounded mailbox. +- **Failure scenario:** 50 drivers × 1 Hz poll on a disk with 10 ms fsync = 500 ms/s busy; at 100+ bursts/s (many drivers, or MQTT push) + the mailbox grows without bound and heap climbs until the input stops. Nothing meters mailbox depth. +- **Fix:** default `CommitMode` to `Periodic` (100 ms) for production, or move the append off the mailbox (pipe the commit outcome back, + like the drain) with a bounded in-flight count; expose mailbox depth via `GetStatus`. + +### P4 — `DriverInstanceActor.PostStop` blocks a dispatcher thread on an unbounded `ShutdownAsync` +- **Severity:** Medium · **Confidence:** High +- **Location:** `DriverInstanceActor.cs:1550` +- **Evidence:** `_driver.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult();` — no token, no `WaitAsync(timeout)`. Runs on the + default dispatcher for every `Context.Stop(child)` (deploy removal, Restart, respawn-on-config-change) and inside `DriverHostActor`'s + `PreRestart` child-stop cascade. +- **Failure scenario:** a driver whose shutdown waits on a dead socket/COM proxy (OpcUaClient subscription deletes against a half-dead + session, Galaxy gateway stream close, FOCAS handle release) pins a fork-join thread for its whole wait; a few such stops starve the + default dispatcher, and `CoordinatedShutdown` / a `DriverHostActor` restart (which waits for all children to terminate before + `PostRestart`) hangs for the sum of them. +- **Fix:** `ShutdownAsync(cts.Token).WaitAsync(TimeSpan.FromSeconds(5))` with a logged timeout; or stop children via a two-phase + "ShutdownRequested → PipeTo → PoisonPill" protocol so no actor thread blocks. + +### P5 — `OpcUaPublishActor` runs the entire deploy apply (parse, plan, materialise) on the single pinned OPC UA thread; legacy path also does a SQL read there +- **Severity:** Medium (by design, worth a note) · **Confidence:** High +- **Location:** `OpcUaPublishActor.cs:483-725` (`HandleRebuild`/`HandleRebuildCore`), `:942-977` (`LoadArtifact`/`LoadLatestArtifact`) +- **Evidence:** the actor is pinned (`opcua-synchronized-dispatcher`) so a rebuild of a 50k-node composition serialises every live + `AttributeValueBatchUpdate` and (because the node manager lock is the SDK's) every client Publish behind it. The surgical-deploy work + bounds the common case, but `MaterialiseHierarchy` still runs in full each apply and a classifier miss is a full rebuild. The + `_dbFactory`-backed `LoadArtifact` branch (only reachable for a `RebuildAddressSpace` without bytes — no production caller today) would + also block that thread on central SQL. +- **Fix:** parse + plan (`DeploymentArtifact.Open`, `ParseComposition`, `AddressSpacePlanner.Compute`) off-thread via `PipeTo`, keeping only + the sink materialisation on the pinned thread; delete the bytes-less `LoadArtifact` branch or route it through the host. + +### P6 — `VirtualTagActor` publishes a cluster-wide DPS `ScriptLogEntry` on EVERY failed evaluation (no rate limit) +- **Severity:** Medium · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs:353-356, 375-399` +- **Evidence:** `OnEvaluationFailed` → `PublishLog(level, reason)` unconditionally ("Always meters + publishes the per-failure + script-log entry"), then `DistributedPubSub...Publish(ScriptLogsTopic, entry)` + `_telemetryHub.Emit`. The Bad-status publish is + deduped once per transition; the log line is not. +- **Failure scenario:** a script that throws on every input (e.g. a null-dereferencing cast after a device started sending nulls) + on a 1 Hz dependency emits 1 DPS message/s per virtual tag across the mesh and into every telemetry subscriber; 200 such tags = + 200 msg/s of identical text into the AdminUI `/script-log` bridges and the gRPC telemetry stream (bounded channels drop-oldest, + so real entries are shed). +- **Fix:** rate-limit per vtag (first N per minute, then a summary count), mirroring `SdkTelemetry`'s 20/min window in the OpcUaClient driver. + +### P7 — `DbHealthProbeActor` runs a synchronous EF query on its actor thread with no explicit command timeout +- **Severity:** Low · **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Health/DbHealthProbeActor.cs:52-65` +- **Evidence:** `db.Deployments.AsNoTracking().Take(1).ToList()` every 5 s; relies on SqlClient's implicit 15 s connect / 30 s command + timeouts. `OpcUaPublishActor` Asks it with a 1 s timeout and fail-safe-demotes, so the consumer is fine; the cost is a dispatcher + thread held for up to 30 s per tick during a SQL hang, and ticks queue behind each other. +- **Fix:** `Task.Run` + `PipeTo`, or `SetCommandTimeout(2s)` like `DriverHostActor.LoadArtifactBlob` does. + +--- + +## 4. Other hazards (concurrency / lifecycle) + +### H1 — A supervision restart of `ScriptedAlarmHostActor` disposes the shared `ScriptedAlarmEngine`; the restarted actor then fails every `LoadAsync` forever (scripted alarms on the node silently die) +- **Severity:** High · **Confidence:** High (mechanism), Medium (trigger frequency) +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs:155-163` (Props captures the engine), + `:676-687` (`PostStop` disposes it), `:406-429` (documented "restarts the actor" policy), `DriverHostActor.cs:1176-1183` (engine built once); + `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:239-241, 1153-1158` +- **Evidence:** + ```csharp + // PostStop + _cts.Cancel(); _engine.OnEvent -= _onEngineEvent; _engine.Dispose(); _cts.Dispose(); + ``` + Akka's default `PreRestart` calls `PostStop()`, then re-creates the actor from the SAME `Props` — i.e. around the same, now-disposed, + `engine` and `upstream`. `ScriptedAlarmEngine.Dispose` sets `_disposed`, kills the shelving timer and `UnsubscribeFromUpstream()`; + `LoadAsync` then throws `ObjectDisposedException` (`:241`) and every reactive path early-returns on `_disposed`. The host's own doc + (`:406-416`) states that an exception escaping `OnDependencyChanged` "restarts the actor" and calls that acceptable because "the next + delivery re-converges" — it cannot, because the engine is dead. The new incarnation re-subscribes `OnEvent` (ctor) and keeps piping + `Status.Failure` → `OnLoadFailed` ("engine LoadAsync failed — alarms not (re)loaded") on every deploy until `DriverHostActor` itself restarts. +- **Failure scenario:** any throw inside a `Receive` of this actor — `_upstream.Push` argument validation, a `ToSnapshot`/`ToDefinition` + mapping surprise, `ReassertConditionNodes` → `_engine.GetProjections()` — and the node's scripted alarms stop evaluating with only a + Warning per deploy as evidence; condition nodes stay at their last state (no Bad quality, no `/alerts` row), which reads as "no alarms". +- **Fix:** either (a) override `PreRestart` to NOT dispose (detach `OnEvent` only; dispose in a real stop via a flag) and re-subscribe in + `PostRestart`, or (b) make the engine owned per incarnation: have `DriverHostActor` pass a factory `Func<(upstream, engine)>` and build + them in the actor ctor, so a restart gets a fresh engine. Add a test: throw from a `DependencyValueBatch` handler, then assert a later + `ApplyScriptedAlarms` loads cleanly. + +### H2 — `DriverInstanceActor.InitializeAsync` lets reconnect attempts overlap on the same driver object (concurrent `ShutdownAsync` + `InitializeAsync`) +- **Severity:** Medium · **Confidence:** High +- **Location:** `DriverInstanceActor.cs:743, 791` (`RetryConnect` every `_reconnectInterval` = 10 s), `:807-856` (`Task.Run`, generation token), + `src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResilienceOptions.cs:88, 100, 112` (Initialize timeouts 5 / 8 / 15 s) +- **Evidence:** the periodic timer fires regardless of whether the previous attempt is still running; `_initGeneration` only discards the + RESULT of an older attempt. Each attempt first runs `driver.ShutdownAsync(CancellationToken.None)` (unbounded, unwrapped) and then the + invoker-bounded `InitializeAsync`. With Tier C's 15 s Initialize timeout (> 10 s cadence), or `NullDriverCapabilityInvoker` (no timeout — + tests/harness graphs), attempt N+1's `ShutdownAsync` runs while attempt N's `InitializeAsync` is still inside the driver. +- **Failure scenario:** exactly the dispose-before-overwrite race the drivers' rule file warns about (`.claude/rules/drivers.md`, + "MUST tear down before re-initialising"): a transport field is overwritten by attempt N while attempt N+1's teardown is disposing the one + it read a moment earlier — a leaked socket or an `ObjectDisposedException` inside the driver per overlap, for as long as the device is down. +- **Fix:** gate the retry on "no attempt in flight" (a `_initInFlight` bool cleared by the piped `InitializeSucceeded/Failed`), and bound + the teardown (`ShutdownAsync(ct).WaitAsync(5 s)`). + +### H3 — `ScriptedAlarmHostActor.OnAlarmCommand` and `ContinuousHistorizationRecorder.GetStatus` suspend their mailbox on unbounded awaits +- **Severity:** Low · **Confidence:** High +- **Location:** `ScriptedAlarmHostActor.cs:581-648` (`CancellationToken.None` on every engine op), `ContinuousHistorizationRecorder.cs:313-320` +- **Evidence:** both are `ReceiveAsync`; the engine ops end in `IAlarmStateStore.SaveAsync` (LocalDb/SQLite) and `GetStatus` in + `_outbox.CountAsync`. A hung SQLite file lock (replication snapshot, disk full) stalls dependency-value ingestion / status replies for + the duration with no timeout. +- **Fix:** linked token with a timeout (10 s) and log on cancel. + +### H4 — `HistorianAdapterActor` fans every alert into an unbounded number of concurrent SQLite inserts +- **Severity:** Low · **Confidence:** High +- **Location:** `HistorianAdapterActor.cs:66, 79, 159-170` +- **Evidence:** `_ = EnqueueAsync(Translate(t))` per transition — no in-flight cap. Each catches and logs, so nothing is lost silently, but an + alarm storm (a native source re-raising hundreds of conditions on reconnect) spawns hundreds of concurrent `ExecuteAsync` against one + SQLite connection pool, where they serialise on the file lock and hold pool threads. +- **Fix:** a `Channel` drained by one loop, or a bounded `SemaphoreSlim`. + +### H5 — `DriverHostActor` `Stale` behaviour has no receive for driver value/alarm bursts or a late `ArtifactForApply` +- **Severity:** Low · **Confidence:** High (unreachable in practice today) +- **Location:** `DriverHostActor.cs:2448-2497` +- **Evidence:** `Stale()` registers neither `AttributeValueBatchPublished`, `AttributeAlarmPublished`, `ArtifactForApply` nor `ApplyWedged`, + so they would hit `Unhandled` (dead-letter log noise, dropped). It is only entered from `Bootstrap`'s catch before any child is spawned, + so no child exists to send them — but `BeginDirectApply`'s orphan replay in the same try block has already `Become(Applying)` + + started a `Task.Run` pipe-back before the catch could run; nothing between them throws today, which is the same "by accident of + ordering" argument the file itself rejects for the cache pipe-back (`:3159-3166`). +- **Fix:** register the four receives in `Stale` as drops, like the others. + +--- + +## 5. Checked and fine (looked like leaks/hazards, are bounded) + +- **`DriverHostActor._pendingAcks` / apply-outcome timers** — every entry is removed by `RebuildCompleted`, `ApplyOutcomeTimedOut` or + `DropPendingAck`; timer keyed per correlation (`:2778-2800`). Bounded by in-flight applies (one at a time). +- **Stash growth in `Applying`** — bounded by distinct deployment ids via `_deferredDispatches` (`:1529-1546`), cleared in `BecomeSteady`. +- **Routing maps (`_nodeIdByDriverRef`, `_driverRefByNodeId`, alarm maps, `_desiredByDriver`, `_valueRefsByDriver`)** — clear-and-rebuild per + apply (`:3485-3565`); `NativeAlarmProjector.RetainOnly` prunes removed conditions (`:3550`); `_commsLatched` prunes to empty (`:1839-1844`). +- **DPS / cluster subscriptions** — the DPS mediator watches subscribers and removes them on `Terminated`; `DriverHostActor.PostStop` + unsubscribes its cluster subscriptions (`:1080-1088`); `ClusterRoleInfo.SubscriberActor.PostStop` unsubscribes (`ClusterRoleInfo.cs:276`). + EventStream subscriptions are keyed by actor ref and dropped on termination. +- **`IDriver` event handlers** — data/batch/alarm/rediscovery/host-status/connection-loss handlers are all detached in `PostStop` + (`DriverInstanceActor.cs:1544-1549`), so a respawn around the same driver object does not accumulate dead-`Self` closures. +- **Child actor lifecycle** — `VirtualTagHostActor` watches + stops children and prunes on `Terminated`; `PeerProbeSupervisor` reconciles + against the snapshot with generation-suffixed names; `DependencyMuxActor` watches subscribers and unregisters on `Terminated`; + `DriverHostActor` uses generation-suffixed child names so stop+respawn never collides. +- **`DriverInstanceActor._faultTimestamps`** — pruned to the 5-minute window on every health publish (`:1425-1431`). +- **gRPC channels** — one `GrpcChannel` per endpoint, cached, disposed by DI at host shutdown (`GrpcApplyAckSender`, `GrpcDeploymentArtifactFetcher`); + per-endpoint deadlines (`AckTimeoutSeconds` 10 s, `FetchTimeoutSeconds` 30 s) are positive by default and validated. +- **LocalDb artifact cache** — newest-2 retention per cluster inside the store transaction, idempotent re-store skip (`LocalDbDeploymentArtifactCache.cs:139-166, 204-235`). +- **`ParsedDeploymentArtifact` / `JsonDocument`** — opened once per apply, disposed in `HandleArtifactForApply`'s `finally` and in + `OpcUaPublishActor.HandleRebuildCore`'s `using`; the cache continuation captures only scalars (`DriverHostActor.cs:3207-3231`). +- **`TelemetryLocalHub` channels** — bounded `DropOldest`; `Detach` removes the subscriber and completes the writer; `Emit` never blocks. +- **`ContinuousHistorizationRecorder` drain** — single-flight (`_draining`), bounded parallelism (`SemaphoreSlim(_drainConcurrency)`), + outbox capacity 500k drop-oldest; `_tagWriteFailures` entries are cleared on the tag's next success and bounded by tag count. +- **`ConfigureAwait(false)` + `Self`/`Sender`** — every such site captures `Self`/`Sender` before the await + (`ContinuousHistorizationRecorder.IngestAsync:410`, `GetStatus:317`, `DriverHostActor.HandleRouteNodeWrite:2175`, `HandleSubscribeAsync:955-956`); + `OpcUaPublishActor.OnHealthTick`'s `ContinueWith` only constructs a record. No regression of the #491 class found. +- **`DriverHostActor` restart** — default `PreRestart` stops all children and Akka waits for their termination before `PostRestart`, so the + re-run `PreStart` (`SpawnVirtualTagHost`/`SpawnScriptedAlarmHost` with fixed names) cannot hit an actor-name collision; a NEW + `ScriptedAlarmEngine` is built, so H1 applies only to the alarm host's own restart. +- **`DriverHostActor.DriverMemberCount()` on the per-write / per-transition gate path** — `Cluster.State` is the cached read-view snapshot + and the LINQ count is over a 2–6 member set; negligible. +- **Unreachable-member set (`_unreachableNodes`)** — removed members deliberately retained (documented #672 reasoning); bounded by + addresses ever seen in a pair. diff --git a/docs/reviews/2026-10-21/03-host-controlplane-security-config.md b/docs/reviews/2026-10-21/03-host-controlplane-security-config.md new file mode 100644 index 00000000..1eb5d5d0 --- /dev/null +++ b/docs/reviews/2026-10-21/03-host-controlplane-security-config.md @@ -0,0 +1,377 @@ +# Review 03 — Host / ControlPlane / Security / Configuration + +Repo: `/Users/dohertj2/Desktop/OtOpcUa` @ `d51d35bd` (master). Read-only audit of production code in +`src/Server/ZB.MOM.WW.OtOpcUa.Host`, `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane`, +`src/Server/ZB.MOM.WW.OtOpcUa.Security`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration`. +Every finding below was verified by reading the cited code. Line numbers are from the files as checked in. + +Severity: Critical / High / Medium / Low. Confidence: High / Medium. + +--- + +## 1. Memory / resource leaks + +### 1.1 Runaway virtual-tag script leaks one spinning thread-pool thread per evaluation, with no breaker +- **Severity:** High **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/Engines/RoslynVirtualTagEvaluator.cs:155-160`; + `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/TimedScriptEvaluator.cs:17-25`; + `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagHostActor.cs` (no timeout bookkeeping — only line 224 mentions timeouts). +- **Evidence:** + ```csharp + var timed = new TimedScriptEvaluator(evaluator, _runTimeout); + var raw = timed.RunAsync(context).GetAwaiter().GetResult(); // RoslynVirtualTagEvaluator.cs:158-159 + ``` + `TimedScriptEvaluator` remarks: *"when a script times out, the underlying ScriptRunner task continues + running on a thread-pool thread … In the CPU-bound-infinite-loop case that's effectively 'leaked' … + the orphan thread is rare."* +- **Failure scenario:** The "rare" claim only holds per evaluation. A change-triggered or timer-triggered + virtual tag whose script spins (e.g. `while(true)`, or a `ctx.GetTag` loop that never converges) + is re-evaluated on every upstream change; each evaluation times out after 2 s and leaves a thread + burning 100 % CPU forever. `VirtualTagHostActor` does nothing on a `Failure("timed out…")` result + except publish Bad quality — no quarantine, no back-off, no consecutive-timeout cap. A 1 Hz input + produces ~1 orphaned core per second; the node is CPU-starved (Akka dispatchers, OPC UA publish + thread, SQL) within minutes and only a process restart recovers it. A Designer-role user can author + this through the AdminUI. +- **Suggested fix:** Track consecutive timeouts per `virtualTagId` in `VirtualTagHostActor`/evaluator and + quarantine the tag (stop evaluating, publish Bad, log Error, surface on `/uns`) after N (e.g. 3) + timeouts until the script is edited. Longer term: cooperative `CancellationToken` polling injected + into compiled scripts, or an out-of-process runner. + +### 1.2 `Deployment` / `NodeDeploymentState` / `ConfigEdit` / `ConfigAuditLog` grow without bound — every deploy stores a full artifact blob forever +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs:338-357`; + `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/OtOpcUaConfigDbContext.cs:720` (`ArtifactBlob varbinary(max)`); + no `Deployments.Remove`/`ExecuteDelete`/retention anywhere in `src` (grep verified). +- **Evidence:** + ```csharp + db.Deployments.Add(new Deployment { …, ArtifactBlob = artifact.Blob }); // AdminOperationsActor.cs:338-345 + ``` +- **Failure scenario:** Each `StartDeployment` persists the gzip'd snapshot (tens of KB to MBs on a 50k-tag + fleet) plus a `ConfigEdit` marker and one `NodeDeploymentState` row per node. Nothing prunes any of + them. Over a year of routine edits the `Deployment` table alone is GBs of blobs; `Deployments.razor` + projects the blob away (fine), but `ClusterOverview.razor:135` and + `ConfigPublishCoordinator.PreStart` (`ConfigPublishCoordinator.cs:164-167`) each materialise the full + newest row including the blob. The `IX_Deployment_Status` index keeps the in-flight query cheap, so + this is capacity/backup growth rather than an immediate hot-path cost. +- **Suggested fix:** Retention job on the admin singleton (keep last N sealed + anything referenced by a + `NodeDeploymentState` newer than X days; null out `ArtifactBlob` on older rows rather than deleting, + so the audit row survives). Project `ArtifactBlob` away in the two full-row reads. + +### 1.3 `MeshGrpcCommandDialer` channel cache never evicts a stale endpoint +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/MeshGrpcCommandDialer.cs:93, 194-199`. +- **Evidence:** `_channels.GetOrAdd(endpoint, GrpcChannel.ForAddress)` keyed by the `http://{Host}:{GrpcPort}` + string; only `Dispose()` removes entries. +- **Failure scenario:** Every `ClusterNode.Host`/`GrpcPort` edit (host rename, port move, IP→DNS migration) + leaves the old `GrpcChannel` — and its idle HTTP/2 connection/socket — alive for the life of central. + Bounded by the number of distinct endpoints ever configured, so it is a slow leak, not a flood. +- **Suggested fix:** On `ContactsLoaded`, dispose channels whose endpoint is no longer in the target set + (the actor already knows the set), or use `SocketsHttpHandler.PooledConnectionIdleTimeout`. + +### 1.4 `OtOpcUaServerHostedService.StopAsync` detaches every sink but leaves the SDK listening until host dispose +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs:448-476` (StopAsync) + vs `482-490` (DisposeAsync → `_appHost.DisposeAsync()` → `ApplicationInstance.StopAsync()`). +- **Evidence:** `StopAsync` nulls the write gateway, historian, authority and authorizer and returns + `Task.CompletedTask`; the only `ApplicationInstance.StopAsync()` is in `DisposeAsync`. +- **Failure scenario:** Between `IHostedService.StopAsync` and host disposal (Akka coordinated shutdown, + other hosted services' StopAsync) the OPC UA endpoint still accepts and activates sessions and serves + the last-written address-space values with the authority source set to empty — reads/subscribes + succeed on stale data, writes get "unavailable". Clients see a server that is up but inert instead of + a clean `BadServerHalted`/connection refusal that would trigger their redundancy failover sooner. +- **Suggested fix:** Call `_appHost.DisposeAsync()` (or an explicit `StopAsync`) from `StopAsync` + *after* the detaches, making `DisposeAsync` idempotent. + +### 1.5 `CookieAuthenticationStateProvider` — dead type with a fire-and-forget ping loop +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Security/Blazor/CookieAuthenticationStateProvider.cs:40`. +- **Evidence:** `_pingLoop ??= Task.Run(() => PingLoopAsync(_cts.Token));` — and no registration or + reference to the type anywhere else in `src` (grep: only its own file). +- **Failure scenario:** Not reachable today, so no live leak. If someone registers it as the circuit + `AuthenticationStateProvider`, the injected `HttpClient` has no `BaseAddress` server-side (the repo's + own memory note "Blazor Server self-HubConnection" documents this trap), `GetAsync("/auth/ping")` + throws, the loop logs once and dies, and the circuit never learns its cookie expired. +- **Suggested fix:** Delete it (see 2.5), or wire it properly with a `BaseAddress` and register it. + +--- + +## 2. Unimplemented / stubbed / dead features + +### 2.1 A failed OPC UA SDK start is swallowed; no health check reflects it; the node stays "Ready", stays Primary-eligible, and acks deploys as Applied +- **Severity:** High **Confidence:** High (swallow + no health probe) / Medium (clean-ack claim) +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs:163-182`; + `src/Server/ZB.MOM.WW.OtOpcUa.Host/Health/HealthEndpoints.cs:34-96` (checks: configdb, akka, + cluster-primary, localdb-replication — nothing for the OPC UA endpoint; the only custom + `IHealthCheck` in Host is `LocalDbReplicationHealthCheck`); + `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs:5-8` (falls back to `NullOpcUaAddressSpaceSink`). +- **Evidence:** + ```csharp + catch (Exception ex) + { + _logger.LogError(ex, "OtOpcUaServerHostedService: SDK start failed; OpcUaPublishActor writes will continue to no-op"); + // Don't rethrow — the rest of the host (admin UI, driver actors, etc.) can still boot. + return; + } + ``` +- **Failure scenario:** Port 4840 already bound, PKI directory unwritable, cert validation failure, or a + bad `OpcUa:*` override on a driver-only site node: the process logs one Error and then reports + `/health/ready` 200, joins the Akka mesh, is elected driver Primary by age, publishes ServiceLevel + through the still-`Null` publisher (so the peer is not promoted), and every deploy applies through + the Null sink and acks `Applied` (the Null sink's surgical methods return false → `SafeRebuild()` → Null + `RebuildAddressSpace()` no-op — the ack path was not traced further, hence Medium on that clause). + Clients get connection-refused from the node the fleet says is primary and healthy. The comment's + rationale ("admin UI can still boot") does not apply to a driver-only node, which has no AdminUI. +- **Suggested fix:** Add an `IHealthCheck` (tags Ready+Active) that reports Unhealthy while + `_server?.NodeManager is null` after StartAsync ran; on a driver-only node (no admin role) + rethrow so `Program.cs`'s fatal handler exits non-zero and the supervisor restarts; feed the + "SDK not started" fact into the ServiceLevel calculator so the peer takes Primary. + +### 2.2 Deploy pipeline can wedge permanently: a `Deployment` stuck in `Dispatching` with no `NodeDeploymentState` rows blocks every future deploy +- **Severity:** High **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Coordinators/ConfigPublishCoordinator.cs:163-200` + (PreStart recovery), `202-248` (HandleDispatch), `355-382` (HandleDeadline); + `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs:193-206, 338-359`. +- **Evidence:** + ```csharp + // PreStart: recovers Dispatching OR AwaitingApplyAcks rows + var inflight = db.Deployments.Where(d => d.Status == DeploymentStatus.Dispatching || d.Status == DeploymentStatus.AwaitingApplyAcks)… + _expectedAcks = nodeStates.Select(…).ToHashSet(NodeIdComparer); // EMPTY when HandleDispatch never ran + … + // HandleDeadline: + if (_acks.Count == _expectedAcks.Count) { /* Race … Already sealed/failed elsewhere. */ return; } // 0 == 0 + ``` + `AdminOperationsActor` writes `Status = Dispatching` and `SaveChangesAsync()` **before** `_coordinator.Tell(...)` + (lines 338-359) and refuses any new deploy while a `Dispatching`/`AwaitingApplyAcks` row exists (193-206). +- **Failure scenario:** Any of: (a) `HandleDispatch` throws before its `SaveChanges` (transient SQL fault + seeding `NodeDeploymentState`) → actor restarts, message dropped; (b) the process dies between the + AdminOps `SaveChangesAsync` and the coordinator handling the Tell; (c) the singleton proxy's buffered + `DispatchDeployment` is lost during a singleton hand-over. In all three the row stays `Dispatching` + with zero `NodeDeploymentState` rows. `PreStart` recovers it with an empty expected set, the deadline + fires, `HandleDeadline` hits the `0 == 0` "race" branch and returns **without** updating the row or + clearing `_current`. From then on every `StartDeployment` is rejected with + `AnotherDeploymentInFlight` until someone hand-edits SQL. No log line names the condition. +- **Suggested fix:** In `PreStart`, treat a recovered deployment with an empty expected set as a failed + dispatch: re-run `DiscoverDriverNodes` + seed rows + re-dispatch, or mark it `TimedOut`/`Failed` and + `ResetForNext()`. In `HandleDeadline`, only take the "race" branch when `_expectedAcks.Count > 0`. + Consider moving the `Dispatching` row write into the coordinator so the row and the expected set are + always created together. + +### 2.3 `Security:ApiKeys:Pepper` is never validated at startup — an unset pepper silently rejects every key +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs:164-168` (plain `Configure`, no validator); + `src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs:114-127`; + `src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs` (`Pepper = string.Empty` default). +- **Evidence:** Program.cs comment: *"getting this wrong on either side is a silent authentication + failure rather than a startup error"* — acknowledged but not closed. The authenticator discovers it + only when `ApiKeySecret.Verify` throws `ArgumentException` on the first activation, and then only logs. +- **Failure scenario:** A new site node deployed without `Security__ApiKeys__Pepper` (or with a different + pepper from central) boots green, passes health, applies deployments, and denies every machine client + with the same opaque "Invalid API key." until someone reads the log. On a fused central node a wrong + pepper also makes the AdminUI *mint* keys that can never verify anywhere. +- **Suggested fix:** `AddValidatedOptions` requiring a non-blank + pepper (minimum length) whenever `hasDriver || hasAdmin`; every other security section in this file + already uses `ValidateOnStart`. + +### 2.4 JWT issuance is a dead end: `/auth/token` mints tokens nothing validates, and the signing-key check fires only on first use +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs:39` (`/auth/token`); + `src/Server/ZB.MOM.WW.OtOpcUa.Security/Jwt/JwtTokenService.cs:66` (ctor throws), `106`, `139` + (`TryValidate`/`BuildValidationParameters` — no callers outside the file; no `AddJwtBearer` anywhere). +- **Evidence:** grep over `src` for `TryValidate|BuildValidationParameters|JwtTokenService` finds only + the registration, `IssueToken`, and the type itself. +- **Failure scenario:** The endpoint is an authenticated-only surface that hands out HS256 bearer tokens + no endpoint in the product accepts. With `Security:Jwt:SigningKey` absent (the Host `appsettings*.json` + carry none; only docker-dev sets it) the singleton's constructor throws `InvalidOperationException` + on the first `/auth/token` call → HTTP 500 instead of a startup failure. +- **Suggested fix:** Either delete `/auth/token` + `JwtTokenService`, or finish the feature + (`AddJwtBearer` using `BuildValidationParameters()`) and validate `JwtOptions` with `ValidateOnStart`. + +### 2.5 Registered-but-never-resolved: `IAuditActorAccessor`; never-registered: `CookieAuthenticationStateProvider` +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Security/ServiceCollectionExtensions.cs:56` + (`TryAddScoped()`); `src/Server/ZB.MOM.WW.OtOpcUa.Security/Audit/AuditActor.cs`; + `src/Server/ZB.MOM.WW.OtOpcUa.Security/Blazor/CookieAuthenticationStateProvider.cs`. +- **Evidence:** grep over `src` (cs + razor) for `IAuditActorAccessor|AuditActor.Resolve` → only the + registration and the two Security files; `CookieAuthenticationStateProvider` → only its own file. +- **Failure scenario:** Dead surface that reads as if audit rows are attributed to the HTTP principal. + Whoever eventually wires `HttpAuditActorAccessor` into a Blazor circuit will get `HttpContext == null` + after the first render and every audit row stamped `"system"`. +- **Suggested fix:** Delete both, or resolve the actor from `AuthenticationStateProvider` in the AdminUI + and actually consume `IAuditActorAccessor` where `AuditEvent.Actor` is built. + +### 2.6 `AuditWriterActor` documents per-row dedup on the DB unique index, but one constraint hit drops the whole 500-event batch +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Audit/AuditWriterActor.cs:17-21` (claim) vs `86-117` (behaviour). +- **Evidence:** + ```csharp + // "a retry of an already-flushed batch hits the constraint and we drop the duplicate insert + // without losing the rest of the batch" (lines 19-21) + … + using var db = _dbFactory.CreateDbContext(); + foreach (var evt in snapshot) db.ConfigAuditLogs.Add(…); + db.SaveChanges(); // one statement batch + … + catch (Exception ex) { _log.Error(ex, "AuditWriter flush failed; {Count} events dropped", snapshot.Count); } + ``` +- **Failure scenario:** A single duplicate `EventId` (any at-least-once producer — the audit seam is + `Tell`-based through a singleton proxy that re-delivers across hand-over) makes `SaveChanges` throw + `DbUpdateException` for the whole batch; up to 499 unrelated audit events are discarded and the + Error line blames "flush failed". Audit is best-effort by contract, but the contract the comment + states is not the one implemented. +- **Suggested fix:** Catch `DbUpdateException` with SQL error 2601/2627, remove the offending entry + from the change tracker and retry once; or pre-filter with `WHERE EventId IN (...)`; or fix the comment. + +### 2.7 `AuthEndpoints.LoginAsync` swallows the LDAP exception with no log +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs:72-79`. +- **Evidence:** `catch (Exception) { return Results.StatusCode(503); }` — no logger call. +- **Failure scenario:** During a directory outage every login returns 503 and the server log contains + nothing from this path; the operator has to infer the cause from the GLAuth/AD side. +- **Suggested fix:** Log at Warning with the exception (the role-mapper branch a few lines down already does). + +--- + +## 3. Performance limiters + +### 3.1 The admin singleton's mailbox is suspended for the whole of every `ReceiveAsync` — a 60 s Test-Connect or a large deploy blocks AdminUI alarm acks, restarts and the next deploy +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs:77-82, 387-432`. +- **Evidence:** + ```csharp + ReceiveAsync(HandleStartDeploymentAsync); + ReceiveAsync(HandleTestDriverConnectAsync); // awaits probe.ProbeAsync up to Math.Clamp(timeout,1,60) s + Receive(HandleAcknowledgeAlarm); + ``` + Akka.NET `ReceiveAsync` suspends the mailbox until the returned Task completes. +- **Failure scenario:** One operator pressing Test Connect against an unreachable PLC (TCP connect + black-hole, default 60 s clamp) stalls every other admin operation fleet-wide — including + `AcknowledgeAlarmCommand`/`ShelveAlarmCommand` from the AdminUI alarm page — for up to a minute. + `HandleStartDeploymentAsync` on a 50k-tag DB (two full snapshots + validation + gzip) has the same + effect for seconds. +- **Suggested fix:** Run the probe on a child/`Task.Run` + `PipeTo` (capture `Sender`), or move + `TestDriverConnect` out of the singleton entirely (it needs no DB and no coordination). Same for the + deploy compose: do the DB work off-actor and only serialize the "is one in flight" check. + +### 3.2 `StartDeployment` reads the entire config DB twice with no transaction — validation and the composed artifact can diverge +- **Severity:** Medium **Confidence:** High +- **Location:** `AdminOperationsActor.cs:218` (`DraftSnapshotFactory.FromConfigDbAsync` — 15 tables), + `:231-239` (clusters + nodes again), `:313` (scripts again), `:334` (`ConfigComposer.SnapshotAndFlattenAsync` — 17 tables); + `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/ConfigComposer.cs:39-64`; + `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftSnapshotFactory.cs:7-37`. +- **Evidence:** Both loaders are sequential `AsNoTracking().ToListAsync()` per table on the same + context with no `BeginTransaction(IsolationLevel.Snapshot)`; the artifact is composed from a second + read taken after validation returned. +- **Failure scenario:** (a) Cost: ~32 sequential round trips and double materialisation of every row + per deploy — on the admin singleton's thread (see 3.1). (b) Correctness: a `/uns` or `/raw` save that + lands between the validator read and the composer read ships an artifact the `DraftValidator` never + saw (e.g. an effective-name collision or a cross-cluster ACL that the gate exists to stop). +- **Suggested fix:** Load once (`DraftSnapshot` already holds nearly every table the composer needs — + add `ServerClusters`/`ClusterNodes`/`ApiKeys` to it and compose from the snapshot), inside a + `SNAPSHOT`/`SERIALIZABLE` read transaction. Project `Script.SourceCode` once instead of three times. + +### 3.3 Pre-authentication SQL round trip per *distinct* unknown key id, serialized under the SDK `SessionManager` lock +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs:46-50, 104-122, 180-184`; + `src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs:101-119`; + `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs:184-190` (sync bridge under the SDK lock). +- **Evidence:** The negative cache is keyed by `apiKeyId`; its own remark says the ceiling makes *"a flood + of unknown ids … not become a SQL flood"*, but `Store` only clears wholesale at 1024 entries and a + flood of **distinct** ids misses every time. Each miss = one `SingleOrDefaultAsync` bounded by + `StoreTimeoutMs` (2 s) while the SDK holds its SessionManager-wide lock. +- **Failure scenario:** Any client that can reach 4840 on an admin/fused node — no credential needed, + only the `apikey` username and a well-formed `otua__` password — can hold the global + activation lock for one indexed query per attempt, ~1-2 ms normally and up to 2 s during SQL + slowness, starving every legitimate activation (anonymous and X.509 included). The ACL path on a + driver-only node (`ArtifactApiKeyStore`) is immune. +- **Suggested fix:** Rate-limit/fail-fast per remote endpoint before the store call, or move the ConfigDb + key set into memory (the artifact snapshot already carries it on every node — the fused node could + read `IDataPlaneAuthoritySource` first and only fall through to SQL on a miss that the snapshot + cannot answer), or prefix-check `keyId` against an in-memory bloom/set refreshed on `StoreCacheSeconds`. + +### 3.4 Central's inbound-ack gate blocks a gRPC thread on a synchronous secret-store read after every contact refresh +- **Severity:** Low **Confidence:** High +- **Location:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/IMeshPskProvider.cs:316-353` + (`GetAwaiter().GetResult()` under a `Lazy` ExecutionAndPublication lock, 3 s bound); + `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Communication/CentralCommunicationActor.cs:782-783` + (`InvalidateAll()` on **every** refresh, default every 60 s). +- **Evidence:** The provider's remarks call the block acceptable because it *"happens at most once per + cluster per process"*; the actor's remarks say the cache is dropped *"on every contact refresh"*. Both + are true; together they mean once per cluster per minute, and the first ack/dial per cluster after + each refresh pays it on a gRPC request thread (or, for dials, a thread-pool thread). +- **Failure scenario:** With a slow secrets store (SQLite on a busy disk, or a future networked KEK) + every deploy's first ack per cluster waits up to 3 s and, past the bound, is denied `PermissionDenied` + — a transient, confusing authentication failure on an otherwise correct key. +- **Suggested fix:** Refresh the per-cluster keys asynchronously from the actor's `ContactsLoaded` + handler and swap the cache atomically, instead of invalidating and re-reading on the request path. + +### 3.5 `ConfigPublishCoordinator.PreStart` materialises the full newest `Deployment` row including `ArtifactBlob` +- **Severity:** Low **Confidence:** High +- **Location:** `ConfigPublishCoordinator.cs:164-167` — `.OrderByDescending(d => d.CreatedAtUtc).FirstOrDefault()` with no `Select`. +- **Failure scenario:** One blob read on every singleton start/hand-over; harmless today, grows with 1.2. +- **Suggested fix:** `.Select(d => new { d.DeploymentId, d.CreatedAtUtc })`. + +--- + +## 4. Other hazards + +### 4.1 Admin operations echo raw exception text to the UI +- **Severity:** Low **Confidence:** High +- **Location:** `AdminOperationsActor.cs:375-384, 423-431, 459-462, 489-492` (`Message: ex.Message`). +- **Failure scenario:** A `SqlException`/`UriFormatException` message (connection string fragments, host + names) reaches any `ConfigEditor`-role browser session. Low because the audience is already + authenticated admin staff. +- **Suggested fix:** Return a fixed message + correlation id; keep the detail in the log. + +### 4.2 Fail-open window on the mesh known-cluster gate at central boot (documented, bounded) +- **Severity:** Low **Confidence:** High +- **Location:** `CentralCommunicationActor.cs:216-230, 286-293, 755-793`; `IMeshPskProvider.cs:63-69, 199-201`. +- **Evidence:** Until the first successful `ClusterNode` load, `_knownClusters` is null and `TryGetPsk` + consults the secret store for **any** `x-otopcua-cluster` value. A 15 × 2 s bootstrap retry and the + 60 s periodic refresh heal it once SQL answers. +- **Failure scenario:** During a central boot with ConfigDb down, an unauthenticated prober can drive + one bounded secret-store read + warning log per request. Keys are still required; this is only the + cost/oracle the gate exists to remove. Acceptable, but worth knowing it is "fail-open until SQL". + +### 4.3 `ActiveNodeHealthCheck`/Primary election and the OPC UA endpoint are decoupled (see 2.1) +Covered under 2.1 — listed here because the consequence is an availability hazard rather than a stub. + +--- + +## 5. Checked and fine + +- **DbContext lifetimes.** Every consumer in the slice takes `IDbContextFactory` + and creates a short-lived (pooled) context per call; read paths use `AsNoTracking()` (47 sites); + the only scoped `OtOpcUaConfigDbContext` registration (`Configuration/ServiceCollectionExtensions.cs:41`) + exists for DataProtection's key repository and returns the context to the pool at scope end. +- **`BoundedApiKeyStore` deadline race.** Abandoned lookups are observed (`ObserveAbandoned`), never cached, + CTSs disposed on both arms; faults/timeouts never cached. The negative-cache amplification (3.3) is the + only gap. +- **`ApiKeyOpcUaUserAuthenticator`.** Uniform deny message, anonymous-principal key refused, catch-all deny; + one HMAC per activation — no per-request crypto beyond that. +- **gRPC auth interceptors (5).** All fail closed on an unset key, use `FixedTimeEquals`, scope by method + prefix, single public ctor; `CentralCommandAuthInterceptor` returns one status text for all denials. +- **`MeshPskProvider.TryGetPsk` Lazy-exception handling.** A throwing logger cannot poison the cache for the + process lifetime (entry evicted, fail-closed). +- **`CentralCommunicationActor` lifecycle.** `_lifecycle` CTS cancelled + disposed in `PostStop`, token reads + guarded by `ObjectDisposedException`; per-dial results piped as messages; nothing captured off-thread. +- **`AclRevalidationTimer`** — created last in `StartAsync`, disposed first in `StopAsync` and in + `DisposeAsync`, dispose-before-overwrite on restart, tick never throws. +- **`ActorSystemTerminationWatchdog`, `DriverResilienceStatusPublisherService`, `SecretReplicationStarter`, + `ClusterNodeAddressReconcilerActor`, `RedundancyStateActor`** — timers/subscriptions are cleaned up in + `PostStop`/`StopAsync`; periodic loops catch and continue; nothing resolves the `ActorSystem` at + construction. +- **`TelemetryStreamGrpcService`** — static `_activeStreams` cap is paired on every path; stream lifetime + bounded at 4 h; subscription disposed by `using`. +- **`TelemetryStreamClient` / `MeshGrpcCommandDialer`** — owned `GrpcChannel`s disposed; keep-alive pings set. +- **`Program.cs`** — `IHistorianBackendProvider` registered via factory (so the container disposes it); + Kestrel re-bind computed exactly once; HTTPS surface refuses takeover rather than silently dropping + TLS; startup failures exit non-zero; `HostAbortedException` passes through. +- **`DraftValidator`** — purely in-memory over the snapshot; `GroupBy/First` guards against duplicate ids + throwing mid-pass; ACL scope rules mirror the picker (`NodeAclScopeIdUnexpected` etc.). +- **`LdapGroupRoleMappingService`, `ClusterAuditQuery`** — context per call, `AsNoTracking`, parameterised `Contains`. +- **`AlarmSfLegacyMigrator`** — read-only legacy open, single transaction, rename only after commit, + `INSERT OR IGNORE` with payload-derived ids makes a re-run idempotent. +- **`OtOpcUaLdapAuthService`** — singleton over a stateless library client; `Enabled` wins over `DevStubMode`; + plaintext transport refused unless `AllowInsecure`. +- **`AuthEndpoints`** — open-redirect guard on `returnUrl`, role-mapper fault fails closed to zero roles + (documented and tested). diff --git a/docs/reviews/2026-10-21/04-adminui.md b/docs/reviews/2026-10-21/04-adminui.md new file mode 100644 index 00000000..68e6b495 --- /dev/null +++ b/docs/reviews/2026-10-21/04-adminui.md @@ -0,0 +1,198 @@ +# AdminUI review — `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI` (master @ d51d35bd) + +Scope: Blazor Server AdminUI — `/raw` tree, `/uns`, equipment page, live-status panels, SignalR bridges + in-process stores, ScriptAnalysis (Roslyn) endpoints + Monaco glue, driver browsers / address pickers, CSV import/export. Production code only. Every finding below was verified by reading the cited code; read-only review, nothing edited. + +Overall: this slice is in good shape. The hard Blazor-Server lifecycle problems (timer drain, event unsubscribe, superseded-modal-open races, N+1 queries, Razor string slicing) have clearly been worked through already. What remains is mostly **scale limiters** (whole-tree renders, per-keystroke Roslyn compiles, unbounded fan-out) and a few **stubs still wired into the live UI**. One finding is a real correctness hazard (#P1). + +--- + +## Memory / Resource leaks + +### L1. Driver-status / resilience snapshot stores never evict — ghost drivers on `/hosts` until process restart +- **Severity:** Medium · **Confidence:** High +- **Location:** `Hubs/InMemoryDriverStatusSnapshotStore.cs:6-17`, `Hubs/InMemoryDriverResilienceStatusStore.cs:6-18`, consumer `Hosts/HostsDriverView.cs` (`Build` iterates `snapList`, i.e. `DriverStore.GetAll()`), `Components/Pages/Hosts.razor:452-453`. +- **Evidence:** + ```csharp + private readonly ConcurrentDictionary _byInstance = new(); + public void Upsert(DriverHealthChanged snapshot) { _byInstance[snapshot.DriverInstanceId] = snapshot; ... } + public IReadOnlyCollection GetAll() => _byInstance.Values.ToArray(); + ``` + There is no `Remove`/TTL; `HostsDriverView.Build` derives the driver rows from the snapshot list, not from `DriverInstances`. +- **Failure scenario:** delete (or delete+recreate under a new id) a driver instance; its last `DriverHealthChanged` stays in the singleton forever. `/hosts` keeps rendering it as a frozen "stale" row (name/type fall back to id since the config join misses), and `HostsDriverView.Build` cost grows with every id ever seen since the admin node started. Long-lived admin nodes on a churny fleet accumulate. +- **Fix:** evict entries whose `DriverInstanceId` is absent from the config load Hosts already performs every 5 s (`_instances`), or add a `Remove` driven by the driver-removal / deploy message, or expire by `PublishedUtc` (e.g. > 10 min). + +### L2. `DriverTestConnectButton` — `System.Timers.Timer` with an `async void` Elapsed handler, never drained +- **Severity:** Low · **Confidence:** Medium +- **Location:** `Components/Shared/Drivers/DriverTestConnectButton.razor:66-74, 82` +- **Evidence:** + ```csharp + _clearTimer = new System.Timers.Timer(30_000) { AutoReset = false }; + _clearTimer.Elapsed += async (_, _) => { _result = null; await InvokeAsync(StateHasChanged); }; + ... + public void Dispose() => _clearTimer?.Dispose(); + ``` +- **Failure scenario:** `Timers.Timer.Dispose` does not wait for an in-flight callback, and an `async void` lambda's post-await exception (component/circuit already gone) cannot be observed by the timer's exception suppression — it surfaces as an unhandled thread-pool exception. The sibling components (`DriverStatusPanel`, `Alerts`) explicitly moved to `System.Threading.Timer` + `DisposeAsync` for this reason; this one was left behind. +- **Fix:** mirror `DriverStatusPanel.ShowOpResult` (`System.Threading.Timer`, `IAsyncDisposable`, token check for stale fires). + +### L3. `AlarmsHistorian` page — sync timer dispose, no overlap guard, blanket `catch {}` +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Pages/AlarmsHistorian.razor:58-74, 87` +- **Evidence:** `_timer = new Timer(_ => _ = InvokeAsync(RefreshAsync), ...)` … `public void Dispose() => _timer?.Dispose();` and `RefreshAsync` wraps the `Ask` in a bare `catch { }`. +- **Failure scenario:** after dispose a queued tick still issues an actor `Ask` (2 s timeout) against a gone component; every failure (including "this role has no historian actor") is swallowed and the page sits on "Loading…" forever — the comment admits the role-gated message "lands when we add role gating to the UI". +- **Fix:** copy the `Fleet.razor` pattern (`IAsyncDisposable`, `_disposed`, overlap guard, cancel CTS) and surface the "no historian on this role" state. + +### L4. Browse sessions can be orphaned when a picker is disposed mid-`OpenAsync` (bounded by the reaper) +- **Severity:** Low · **Confidence:** High +- **Location:** all `Components/Shared/Drivers/Pickers/*AddressPickerBody.razor` `OpenBrowseAsync` (e.g. `GalaxyAddressPickerBody.razor:146-157`) and `RawBrowseModal.razor` `OpenBrowseAsync`; disposal at e.g. `GalaxyAddressPickerBody.razor:217-225`. +- **Evidence:** `var result = await BrowserService.OpenAsync(...); if (result.Ok) _token = result.Token;` — if the hosting modal is closed/navigated away while the open is in flight, `DisposeAsync` has already run with `_token == Guid.Empty`, then the token lands on a dead component and nobody calls `CloseAsync`. +- **Failure scenario:** an OPC UA client session / MQTT connection / SQL connection stays open against the plant device for up to `IdleTtl` (2 min) + one reaper tick (30 s). Harmless at low volume; a flapping operator can hold several at once. +- **Fix:** in `OpenBrowseAsync`, after the await check a `_disposed` flag and immediately `CloseAsync(result.Token)`. + +### L5. In-process stores invoke subscriber delegates synchronously on the bridge actor thread; one throwing subscriber aborts delivery and (for two bridges) restarts the actor +- **Severity:** Low · **Confidence:** High +- **Location:** `Hubs/InMemoryDriverStatusSnapshotStore.cs:13`, `Hubs/IInProcessBroadcaster.cs` (`Publish => Received?.Invoke(item)`), `Hubs/AlertSignalRBridge.cs` `ForwardAsync` (the `_broadcaster.Publish(msg)` sits **outside** the `try`), `Hubs/DriverResilienceStatusBridge.cs` (`Receive<…>(msg => _store.Upsert(msg))`, no catch). +- **Failure scenario:** every open circuit's handler runs inline on the actor thread (`InvokeAsync` is fire-and-forget so it is cheap, but any synchronous throw — e.g. a handler on a circuit whose renderer is mid-teardown — stops the multicast at that subscriber; remaining panels miss the update, and the alert/resilience bridge actor restarts and re-subscribes to DPS). It also means the DPS bridge's throughput is coupled to the number of open admin browser tabs. +- **Fix:** iterate `GetInvocationList()` with per-subscriber try/catch (or post to a channel), and move `Publish` inside the `try` in `AlertSignalRBridge`. + +--- + +## Unimplemented / stubbed + +### S1. `ctx.SetVirtualTag` is advertised by the editor but documented (by the editor itself) as inert at runtime +- **Severity:** Medium · **Confidence:** High +- **Location:** `ScriptAnalysis/ScriptAnalysisService.cs:349-353` (hover text), `TryGetTagPathLiteral` offers path completion for `SetVirtualTag` (`:295-300`). +- **Evidence:** `"⚠ Cross-tag SetVirtualTag writes are currently dropped in single-tag mode (the cascade engine is not wired into the host); this write will not take effect at runtime."` +- **Failure scenario:** an author gets completions + green diagnostics for a call that silently does nothing when deployed. Only a hover reveals it. +- **Fix:** either wire the cascade path or emit an `OTSCRIPT_*` warning marker from `Diagnose` for `ctx.SetVirtualTag(...)` so the Problems list shows it, not just hover. + +### S2. Inlay-hints endpoint is a no-op, but the JS still POSTs the whole script to it +- **Severity:** Low · **Confidence:** High +- **Location:** `ScriptAnalysis/ScriptAnalysisService.cs:468-472` (`InlayHints(...) => new(Array.Empty())`), `ScriptAnalysisEndpoints.cs:24`, `wwwroot/js/monaco-init.js:104-121` (`registerInlayHintsProvider` → `fetch("/api/script-analysis/inlay-hints", { body: model.getValue() })`). +- **Failure scenario:** Monaco re-requests inlay hints on content/viewport changes, so every edit costs an extra authenticated POST carrying the full source, for an always-empty answer. +- **Fix:** drop the provider registration (and endpoint) until the feature exists. + +### S3. `MonacoEditor.MarkersChanged` / `RevealLineAsync` ("problems panel") have no consumer; marker array is still round-tripped JS→.NET after every diagnostics pass +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Shared/MonacoEditor.razor:39-44, 129-131, 134-138` ("The marker DTO is not modelled yet — typed as object[] until a later task…"); `monaco-init.js:209` (`dotNetRef.invokeMethodAsync("OnMarkersChanged", markers)`); `grep MarkersChanged` finds no consumer in `ScriptEdit.razor` / `VirtualTagModal.razor`. +- **Fix:** either build the problems panel or remove the callback + JS invoke. + +### S4. Dead service surface: `IUnsTreeService.LoadTagsForEquipmentAsync` / `EquipmentTagRow` / `tagCounts` +- **Severity:** Low · **Confidence:** High +- **Location:** `Uns/UnsTreeService.cs` (`LoadTagsForEquipmentAsync` returns `Array.Empty()`; `LoadStructureAsync` builds an always-empty `tagCounts`), `Uns/EquipmentChildRows.cs:7`. No production caller. +- **Fix:** delete the member, record and the zero-count plumbing (v3 retired the binding). + +### S5. Deploy API `202 Accepted` Location points at a route that does not exist +- **Severity:** Low · **Confidence:** High +- **Location:** `Api/DeployApiEndpoints.cs:86-88` — `Results.Accepted($"/api/deployments/{r.DeploymentId?.Value}", …)`; the only route mapped is `MapPost("/api/deployments")`; no `MapGet` anywhere in `src/Server`. +- **Failure scenario:** a REST client that follows the `Location` for status polling gets a 404. +- **Fix:** add a GET that reads `Deployments`/`NodeDeploymentStates` for the id, or return `Accepted(value)` without a location. + +### S6. Equipment CSV import is a naive comma split (no RFC-4180), unlike the raw-tag CSV import +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Shared/Uns/ImportEquipmentModal.razor` `ParseCsv` — `// NOTE: simple comma split — no RFC4180 quoting; values with commas are not supported.` +- **Failure scenario:** a quoted `Manufacturer` containing a comma silently shifts every later column (Model/SerialNumber…) with no error. The raw-tag importer (`CsvParser` + `CsvColumnMap`) already does this correctly. +- **Fix:** reuse the raw CSV parser. + +### S7. Dev-only demo page ships in the production assembly +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Pages/Dev/ContextMenuDemo.razor:1-3` — `@page "/dev/context-menu-demo"`, `[Authorize(Policy = AuthenticatedRead)]`. +- **Fix:** `#if DEBUG` the route or move it to a test host. + +--- + +## Performance limiters + +### P1. Monaco ships the full editor text JS→.NET on every keystroke; Blazor Server's 32 KB `MaximumReceiveMessageSize` is never raised +- **Severity:** High · **Confidence:** Medium (limit + code path verified; failure mode on exceed is per ASP.NET Core SignalR behaviour) +- **Location:** `wwwroot/js/monaco-init.js:213-214` (`editor.onDidChangeModelContent(() => dotNetRef.invokeMethodAsync("OnValueChanged", editor.getValue()).catch(function () {}))`), `Components/Shared/MonacoEditor.razor:119-127`, `EndpointRouteBuilderExtensions.cs:56` (`AddRazorComponents().AddInteractiveServerComponents()` — no `AddHubOptions`; `grep MaximumReceiveMessageSize src` → nothing). +- **Failure scenario:** (a) cost: each keystroke transfers O(script size) over the circuit — a 20 KB script typed at 5 keys/s is 100 KB/s per author, before the separate `fetch` calls below. (b) correctness: once the script exceeds 32 KB (paste a large script), the JS→.NET invoke exceeds the hub's receive limit; the failure is swallowed by `.catch(function () {})`, so `_lastSentValue`/`Value` silently stop tracking the editor and **Save persists stale source** (ScriptEdit `SubmitAsync` reads `_form.SourceCode`, which only `ValueChanged` updates). SignalR may also tear the circuit down on the oversize frame. +- **Fix:** read the text back on demand (`MonacoBlazor.getValue` already exists) at Save / blur instead of per keystroke, or send `onDidChangeModelContent` *changes* and apply them server-side; and raise `HubOptions.MaximumReceiveMessageSize` (or document the cap). + +### P2. Roslyn: a full `CSharpCompilation` + semantic model per request, per keystroke, with no cancellation anywhere +- **Severity:** Medium · **Confidence:** High +- **Location:** `ScriptAnalysis/ScriptAnalysisService.cs:60-67` (`Analyze` → `CSharpCompilation.Create(...)` every call), callers `CompleteAsync:220`, `Hover:323`, `SignatureHelp:422`, `Diagnose:97`; `ScriptAnalysisEndpoints.cs:19-24` (handlers do not take `CancellationToken`/`RequestAborted`); `monaco-init.js:32-72` (`quickSuggestions.strings = true`, providers ignore Monaco's cancellation token, no `AbortController`). +- **Evidence:** `var compilation = CSharpCompilation.Create("ScriptAnalysis", new[] { tree }, Sandbox.References, CompileOptions); return (tree, compilation, compilation.GetSemanticModel(tree), …)`. +- **Failure scenario:** typing inside `ctx.GetTag("…")` fires a completion request per character plus hover plus signature help plus the 500 ms diagnostics; each is a fresh compile against the full sandbox reference set on the admin node's thread pool, and an abandoned request runs to completion. Two or three concurrent authors on a big script make the admin node (which also hosts the control-plane actors) visibly busy. +- **Fix:** keep one `CSharpCompilation` per scoped service and use `compilation.ReplaceSyntaxTree` (cheap incremental re-bind); plumb `HttpContext.RequestAborted` through; honour Monaco's `CancellationToken` + `AbortController` in the providers; consider server-side debounce on completions. + +### P3. `ScriptTagCatalog.GetPathsAsync` rebuilds a HashSet of every fleet tag path and sorts on every completion keystroke +- **Severity:** Medium · **Confidence:** High +- **Location:** `ScriptAnalysis/IScriptTagCatalog.cs` `GetPathsAsync` (`new HashSet(entries.Select(e => e.Path), OrdinalIgnoreCase)` → `Where(StartsWith)` → `OrderBy` → `Take(200)`), `GetTagInfoAsync` (linear `FirstOrDefault` per hover). +- **Failure scenario:** the 5 s snapshot cache fixed the DB side, but the in-memory side is still O(n log n) allocation per keystroke; on a 50k-tag fleet that is tens of MB of garbage per typed character. +- **Fix:** materialise a sorted, de-duplicated `string[]` (and a path→info dictionary) inside the `Snapshot`; prefix-search with `Array.BinarySearch`. + +### P4. `/hosts` rebuilds every cluster group and re-renders the whole page on every driver health publish +- **Severity:** Medium · **Confidence:** High +- **Location:** `Components/Pages/Hosts.razor:478-479` (`OnSnapshotChanged(DriverHealthChanged _) => InvokeAsync(() => { RebuildDriverGroups(); StateHasChanged(); })`), `Hosts/HostsDriverView.Build` (full re-projection: `ToList` + per-cluster `Where` scans), publish cadence documented at `Hosts.razor:301-302` ("5 s publish interval"). +- **Failure scenario:** N enabled drivers ⇒ N full rebuilds + N full-page diffs every 5 s per open `/hosts` tab (200 drivers ≈ 40/s), on top of the page's own 5 s tick. Each `HostsDriverRow` ctor also re-sorts `Diagnostics` and `HostStatuses`. +- **Fix:** set a dirty flag in `OnSnapshotChanged` and let the existing 5 s `TickAsync` do the rebuild (or throttle to ≤1 rebuild/s), and rebuild only the affected cluster. + +### P5. `RawTree` re-creates every node's context-menu list per render and re-renders a `ContextMenu` child per row; no virtualization +- **Severity:** Medium · **Confidence:** High +- **Location:** `Components/Shared/Raw/RawTree.razor:107-137` (`RenderNode` → `var items = MenuFor(node)` → `new List { new() { … OnClick = () => … } }` per node per render), `:129` (`` — `Items` is a reference-type parameter, so Blazor treats it as changed on every render), `grep Virtualize` → unused anywhere in AdminUI; `Uns/RawTreeService.cs` `LoadDeviceChildrenAsync` / `LoadTagGroupChildrenAsync` return every tag of the device/group. +- **Failure scenario:** expand a device with a few thousand root tags: every spinner toggle, checkbox click (picker mode `ToggleTagSelection` calls `StateHasChanged`), or modal open/close re-renders thousands of rows, allocates thousands of menu lists + closures, and re-renders thousands of `ContextMenu` children. Same shape in `DriverBrowseTree` (`RenderNode` per browse node) and `UnsTree`. +- **Fix:** cache the per-node menu on `RawNode` (invalidate on `Enabled` change), add `ShouldRender`/`@key` discipline, or wrap each expanded level in ``; page tags from the service (e.g. 500 per level with a "show more"). + +### P6. Whole CSVs are rendered into the circuit: import review grid has no paging, export embeds the file twice in the DOM +- **Severity:** Medium · **Confidence:** High +- **Location:** `Components/Shared/Raw/RawCsvImportModal.razor:85` (`@foreach (var row in _result.Rows)` — every parsed row of a file up to the 16 MB cap at `:261`), `Components/Shared/Raw/RawCsvExportModal.razor:46` (``) and `:54` (`href="@_dataUri"` = base64 of the same text) with both strings retained in `_csv`/`_dataUri` while the modal is open. +- **Failure scenario:** exporting a 50k-tag device produces a multi-MB CSV; the render batch pushed over SignalR is ≈2.3× that size (textarea + base64) and the DOM holds it; importing a large file renders 100k+ ``s in one go and recomputes `OkCount/ErrorCount/RemainingCount` (three full scans) on every render. +- **Fix:** `` the review grid; stream the export via `IJSRuntime` + `DotNetStreamReference` or a download endpoint; show a bounded preview (first N lines) in the textarea. + +### P7. Alerts / ScriptLog: one `StateHasChanged` per received event; list re-filtered per render +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Pages/ScriptLog.razor:128-134` (`OnEntry` → `InvokeAsync(() => { _rows.Insert(0, entry); …; StateHasChanged(); })`), `:95-110` (`VisibleRows` filters + `ToList()` on every render), `Components/Pages/Alerts.razor:279-286`. +- **Failure scenario:** a chatty script (`log` per evaluation per tag) drives one full-page render per line on every open `/script-log` tab; `Insert(0)` on a `List` of 500 is a memmove per line. +- **Fix:** batch via a 100–250 ms coalescing timer; use a ring buffer / `LinkedList`. + +### P8. `/scripts` list loads every script *with its full `SourceCode`* +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Pages/Scripts.razor:117-123` (`db.Scripts.AsNoTracking().OrderBy(s => s.Name).ToListAsync()` — full entity), source only needed for the on-demand "view source" dialog (`FindSourceScript`). +- **Fix:** project `{ScriptId, Name, Language, SourceHash, RowVersion}` for the list; fetch `SourceCode` when the dialog opens. + +### P9. Diagnostics responses are not sequenced — a slow older response can overwrite newer markers +- **Severity:** Low · **Confidence:** High +- **Location:** `wwwroot/js/monaco-init.js:204-212` (`scheduleDiagnostics` debounces the *start* but `fetchDiagnostics` results are applied whenever they arrive). +- **Failure scenario:** type, pause 500 ms (request A, slow — the admin node is busy per P2), type again, pause (request B, fast): B's markers land, then A's stale markers overwrite them; the editor shows errors for code that no longer exists until the next edit. +- **Fix:** generation counter captured before the fetch, ignore results whose generation != current; add `AbortController`. + +--- + +## Other hazards + +### H1. Destructive actions with no confirmation step +- **Severity:** Medium · **Confidence:** High +- **Location:** `Components/Pages/Uns/EquipmentPage.razor:243, 298` (per-row Delete buttons bound directly to `DeleteVirtualTag` / `DeleteAlarm`, which delete immediately at `:447-456`, `:492-501`); `Components/Pages/ScriptEdit.razor:72, 145-160` (`DeleteAsync` deletes the script on click). Contrast: `/uns` and `/raw` route every delete through a confirm modal (`GlobalUns.razor` `_confirmNode`, `RawTree.razor` `ShowConfirm`). +- **Failure scenario:** a mis-click on a dense row deletes a scripted alarm / virtual tag (and, after the next deploy, its OPC UA node and any ScadaBridge subscription to it) with no undo. +- **Fix:** reuse `RawConfirmDialog`. + +### H2. Modal parameter dictionaries and `EventCallback`s are re-created on every render +- **Severity:** Low · **Confidence:** High +- **Location:** `Components/Shared/Drivers/DriverConfigModal.razor` (`_formParameters => new() { … EventCallback.Factory.Create(...) }`), `DeviceModal.razor` (`_deviceFormParameters`), `RawTagModal.razor` `BuildEditorParameters()`. +- **Failure scenario:** `DynamicComponent` sees new parameter values every render, so the hosted driver/device/tag-config form runs `OnParametersSet` on every parent render. Correctness is protected only by each form's `_lastParsed` guard (verified present in all 12 driver forms + 9 device forms); any future form that forgets the guard will clobber in-progress edits on the first live-status push. This is the exact bug class the `MqttDriverForm` note describes. +- **Fix:** build the dictionary once per open (in `LoadAsync`) and cache the callbacks in fields. + +### H3. `monaco.editor.setTheme` is global +- **Severity:** Low · **Confidence:** High +- **Location:** `monaco-init.js:218-221` (comment acknowledges it) + `MonacoEditor.razor:158-162`. +- **Failure scenario:** `VirtualTagModal` and the host page can each mount an editor; toggling theme on one flips the other, and the per-component `_dark` flags drift from reality. Cosmetic. + +--- + +## Checked-and-fine (looked suspicious, verified OK) + +- **Timer/event lifecycle on the live pages** — `Fleet`, `Hosts`, `DriverStatusPanel`, `Alerts`, `ScriptLog`, `PageErrorBoundary`, `Certificates` all unsubscribe singleton events, drain `System.Threading.Timer` via `DisposeAsync`, cancel a page CTS, and guard `StateHasChanged` after teardown (`_disposed`, `_tickRunning`). `ToastNotification` binds auto-dismiss delays to a disposal CTS. +- **Browse pipeline** — `BrowseSessionRegistry` + `BrowseSessionReaper` (2 min idle TTL, 30 s tick, drain on shutdown), 20 s per-call timeout, "Browse" buttons hidden while `_token != Guid.Empty` (no double-open leak), `RawBrowseModal.CloseAsync` always closes the session, `RefreshTree` re-keys the tree without opening a second session, rebirth is authorized server-side and re-probed before publish. +- **EF query shapes** — `RawTreeService` / `UnsTreeService` / `ScriptTagCatalog` / `AclScopeService` use `AsNoTracking` + projections and batch child counts with `GroupBy`/`Contains` (no N+1 found); rename script scan runs server-side in batches; `ImportTagsAsync` / `ImportEquipmentAsync` pre-load lookup sets and `SaveChanges` once; `Deployments` pages `Take(50)`, `ClusterAudit` pages 200. +- **Razor string slicing (#504 class)** — every `[..N]` is either guarded by a `Length > N` check or applied to a fixed-width `Guid.ToString("N")`; DB values go through `DisplayText.Abbreviate`. +- **Superseded-open races** — `RawBrowseModal`, `RawCsvImportModal`, `RawCsvExportModal`, `DeviceModal`, `DriverConfigModal`, `RawTagModal`, `VirtualTagModal` all carry an open-generation / loaded-key guard so a late await cannot install another target's state. +- **Caches** — `ScriptTagCatalogSnapshotCache` (5 s TTL, single-flight gate, keeps old snapshot on failure) and `DeploymentDriftProbe` (watermark-keyed, single-flight) are bounded single-entry caches, not growing maps. No static mutable state in components (only `JsonSerializerOptions`). +- **Roslyn references** — `Sandbox.References` / `Preamble` are process-static and shared; no `AdhocWorkspace`/`MetadataReference` is created per request (the per-request cost is the compilation itself, see P2). +- **JS interop** — the single `DotNetObjectReference` (`MonacoEditor`) is disposed; `MonacoBlazor.dispose` is called under `JSDisconnectedException` handling; prerender `InvalidOperationException` is handled. +- **TelemetryDialSupervisor** — the `Task.Run` dial loop only `Tell`s `Self`; `Dialer.Cancel()` cancels **and disposes** the CTS; reconnect timer per node is replaced, not stacked. +- **Auth on the push/analysis surfaces** — `AlertHub`/`ScriptLogHub`/`DriverStatusHub` are `[Authorize]`; `/api/script-analysis/*` requires `ConfigEditor`; `/api/deployments` is API-key gated with constant-time compare. +- **Driver-type coverage** — all 12 `DriverTypeNames` have a driver form (`DriverFormMaps`), tag-config editor + validator; Sql/MTConnect/Calculation deliberately have no device form (`SingleConnectionDriverTypes`), so nothing is "registered but unauthorable" in this slice. +- **`GlobalUns` "Delete for this node kind is not yet available"** — unreachable; `UnsTree.RenderActions` never renders Delete for Tag/VirtualTag. +- **`ClusterNamespaces.razor` placeholder** — intentional redirect stub so old links don't 404. +- **`InputFile`** — streamed with a 16 MB cap (not subject to the 32 KB hub limit in P1). +- **`TwinCATDriverForm` "PLC diagnostics… not yet live-gated"** — persisted and consumed by the driver; the note is about test coverage, not a dangling option (driver slice). diff --git a/docs/reviews/2026-10-21/05-core.md b/docs/reviews/2026-10-21/05-core.md new file mode 100644 index 00000000..9a7b9575 --- /dev/null +++ b/docs/reviews/2026-10-21/05-core.md @@ -0,0 +1,124 @@ +# Core slice review (Commons, Core, Core.Abstractions, Core.Scripting[.Abstractions], Core.ScriptedAlarms, Core.VirtualTags, Core.AlarmHistorian) + +Reviewer 05. Read-only audit of production code. master @ d51d35bd. + +Scope note: this slice is unusually mature — collectible-ALC script unloading, per-generation +resilience-pipeline keys, retire/reinstate tombstoning, and the scripted-alarm drain machinery have +all clearly been through multiple hardening rounds, and most of the "obvious" leaks are already +closed and commented. The findings below are the residue. + +--- + +## Memory / Resource leaks + +### 1. `ScriptedAlarmEngine._valueCache` is never pruned across reloads — unbounded growth per deploy +- Severity: Medium · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:163` (field), `:260-266` (LoadAsync clears), `:547-549` (OnUpstreamChange write) +- Evidence: `LoadAsync` clears `_alarms`, `_alarmsReferencing`, `_scratchByAlarmId`, `_lastQualityBucketByAlarmId`, `_compileCache` — but **not** `_valueCache`. It then re-seeds only the *currently* referenced paths (`foreach (var path in _alarmsReferencing.Keys) _valueCache[path] = ...`). `OnUpstreamChange` also writes `_valueCache[path] = value;` unconditionally (even before the `_disposed` check). +- Failure scenario: each deploy/reload whose alarm set drops a referenced tag leaves that tag's entry in `_valueCache` forever. Over many deploys with churning tag sets, `_valueCache` accumulates stale keys that nothing ever reads or evicts — a slow leak on a long-lived process. It also carries dead entries into `TryLookup` (message-template resolution) and `WorstInputStatus` snapshots, though those only read the referenced subset so the effect is memory, not correctness. +- Suggested fix: clear `_valueCache` in `LoadAsync` alongside the other maps (it's re-seeded immediately after), or intersect it with `_alarmsReferencing.Keys` at the end of load. + +### 2. A timed-out script leaks one thread-pool thread *per evaluation*, and alarm re-eval makes that unbounded +- Severity: Medium (High impact if a script loops) · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/TimedScriptEvaluator.cs:78-97`; consumed at `ScriptedAlarmEngine.cs:770` +- Evidence: `RunAsync` does `var runTask = Task.Run(() => _inner.RunAsync(context, ct)); await runTask.WaitAsync(Timeout, ct);`. The class XML doc states the orphaned ScriptRunner thread "is tied up until the runtime decides to return, which it may never do" and calls the orphan "rare". +- Failure scenario: a CPU-bound (or blocking — see Finding 10) predicate times out; the engine logs and returns `seed`, releasing `_evalGate`. The *next* upstream change re-evaluates the same alarm, spawning *another* `Task.Run` that also hangs. A single accidental `while(true){}` in one authored predicate therefore leaks one pool thread per change event, not one total — trending toward thread-pool starvation across the whole server, not just that alarm. The "rare orphan" framing understates the per-evaluation accumulation. +- Suggested fix: out of scope to fully fix in-process, but at minimum gate re-evaluation of an alarm whose predicate has already timed out (circuit-break the alarm after N consecutive timeouts) so one bad script cannot spawn unbounded orphan threads. Document the accumulation in the class remarks. + +### 3. `ScriptedAlarmEngine.Dispose` drains in-flight tasks with no timeout — can hang shutdown +- Severity: Medium · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:1168-1180` +- Evidence: `Task[] toAwait; ... Task.WhenAll(toAwait).GetAwaiter().GetResult();` — a synchronous, **un-timed** block. The drained tasks (`DrainDirtyAsync`, `ShelvingCheckAsync`) run with `CancellationToken.None`, and each contains `await _store.SaveAsync(..., ct)`. +- Failure scenario: if the injected `IAlarmStateStore.SaveAsync` (LocalDb/SQL-backed in production) blocks — connection stall, lock, replication hiccup — the background task never completes, so `Dispose` (and the host shutdown that calls it) hangs indefinitely. Contrast `PollGroupEngine.DisposeAsync` (`:449`) which bounds the identical pattern with `.WaitAsync(TimeSpan.FromSeconds(5))`. +- Suggested fix: bound the drain with `.WaitAsync(someTimeout)` like PollGroupEngine does, and/or pass a cancelable token to the background tasks that Dispose can trip. + +### 4. Persisted alarm `Comments` list grows unbounded and is re-serialized whole on every transition +- Severity: Low · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/Part9StateMachine.cs:334-336`; persisted via `ScriptedAlarmEngine` `SaveAsync` of the full `AlarmConditionState` +- Evidence: `AppendComment` only ever `existing.Add(...)`; nothing trims. Every ack/confirm/shelve/unshelve/AddComment/AutoUnshelve appends, and each transition persists the entire `AlarmConditionState` (comments included) and replicates it to the pair peer. +- Failure scenario: a long-lived, frequently-operated alarm accumulates an ever-growing audit list; each `SaveAsync` re-serializes the whole list (O(n) per transition, O(n²) over the alarm's life) and replicates it. GxP retention means the data must be kept, but keeping it *inline in the condition row* makes every write cost grow with history. +- Suggested fix: this is a design trade-off, not a bug — note it. If it bites, move the append-only audit trail to its own table keyed by alarm id so the condition row stays fixed-size. + +--- + +## Unimplemented / stubbed + +### 5. No true stubs found in the slice +- Severity: — · Confidence: High +- Every `NotSupportedException` / `NotImplementedException` hit is a *deliberate, contractual* default or guard, not a gap: + - `IHistorianDataSource.ReadModifiedAsync` / reverse-events defaults throw `NotSupportedException` by design (backend opt-in), documented at `Core.Abstractions/Historian/IHistorianDataSource.cs:205,275`. + - `GenericDriverNodeManager.cs:68` throws `NotSupportedException` when a driver lacks `ITagDiscovery` — correct. + - `HistoryAggregateNotSupportedException` (`HistoryReadContracts.cs:74`) is a typed signal, not a stub. + - `AlarmPredicateContext.SetVirtualTag` (`AlarmPredicateContext.cs:55`) throws by design (predicates must be pure). +- No TODO/FIXME/HACK in slice production code. No dead options I could find unconsumed. + +### 6. Empty catch blocks are all justified, with one worth a glance +- Severity: Low · Confidence: Medium +- Location: `ScriptedAlarmSource.cs:101` `try { OnAlarmEvent?.Invoke(...); } catch { }` +- Evidence: subscriber exceptions are swallowed silently (no log) in the `IAlarmSource` fan-out, unlike the engine's own `FireEvent` (`ScriptedAlarmEngine.cs:975`) which logs. The other empty catches in the slice (PollGroupEngine CTS cancel/drain, DriverHost best-effort shutdown, `s.Dispose()` on unsubscribe) are standard best-effort teardown and fine. +- Failure scenario: a throwing alarm subscriber (e.g. the OPC UA alarm-source adapter) is silently dropped with no diagnostic — a lost alarm event with no trace. Low because the inner engine path does log. +- Suggested fix: log at warning in the `ScriptedAlarmSource` catch, matching `FireEvent`. + +--- + +## Performance limiters + +### 7. `LocalDbStoreAndForwardSink.GetStatus()` runs a synchronous `COUNT(*)` on every call +- Severity: Low · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/LocalDbStoreAndForwardSink.cs:520-532` +- Evidence: `GetStatus` opens a fresh connection and runs `SELECT COUNT(*) ... WHERE dead_lettered = 1` synchronously (`ExecuteScalar`) each time. The queue depth itself is the cached `_queuedRowCount`, but dead-letter depth is always a live scan. +- Failure scenario: `GetStatus` feeds the Admin UI `/alarms/historian` page and `/healthz`. A health probe or dashboard polling it on a short interval runs a table scan each tick on the shared LocalDb file. Minor at normal queue sizes; grows with dead-letter backlog. +- Suggested fix: maintain a cached dead-letter counter the way the queued counter is cached, resyncing on the same periodic cadence, or index-scan only. + +### 8. Sync-over-async blocking teardown in `PollGroupEngine.Unsubscribe` +- Severity: Low · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs:225` (`task.Wait(DrainTimeout)`) +- Evidence: the sync `Unsubscribe` blocks the caller up to 5 s draining the loop. The code itself documents this and offers `UnsubscribeAsync` as the preferred path ("prefer `UnsubscribeAsync` on any path that runs inside an actor mailbox turn"). +- Failure scenario: a caller that uses the sync overload from an actor turn or request pipeline stalls that thread for up to 5 s. Already called out in the XML doc; flagging so consumers are audited to prefer the async overload. +- Suggested fix: none in this file — the async path exists; ensure callers use it (out-of-slice). + +### 9. `ScriptLogTopicSink` / `ScriptLogCompanionSink` call `RenderMessage()` per event on the hot path +- Severity: Low · Confidence: Medium +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ScriptLogTopicSink.cs:64`, `ScriptLogCompanionSink.cs:57` +- Evidence: every forwarded script log event is eagerly rendered to a string (`logEvent.RenderMessage()`) and, in the topic sink, published onto the Akka DPS bus. The topic sink's floor is `Information` by default. +- Failure scenario: a script that logs at Information in its evaluation body (which runs per upstream change) fans a rendered string onto the cluster bus on every evaluation. The min-level discard protects below the floor, but Information+ is forwarded verbatim. +- Suggested fix: fine as-is given the min-level gate; consider a higher default floor for the topic sink, or rate-limiting, if a chatty script is observed saturating the bus. + +--- + +## Other hazards + +### 10. Script sandbox allows `System.Threading` blocking/synchronization primitives and `Type.GetType`/`RuntimeHelpers` +- Severity: Medium · Confidence: Medium +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs:60-80` (namespace deny-list), `:116-151` (type deny-list) +- Evidence: the namespace deny-list covers `System.Threading.Tasks` but **not** `System.Threading` itself (only `Thread`/`ThreadPool`/`Timer` are denied type-granularly). So `SemaphoreSlim`, `Monitor`/`lock`, `ManualResetEventSlim`, `SpinWait`, `Mutex`, and `EventWaitHandle` are all reachable. Separately, `System.Type.GetType(string)` (on the allowed `System.Type`) and `System.Runtime.CompilerServices.RuntimeHelpers` (namespace not denied; only `...CompilerServices.Unsafe` is type-denied) resolve. +- Failure scenario: (a) a blocking primitive (`new SemaphoreSlim(0).Wait()`, `SpinWait.SpinUntil(() => false)`, a named `Mutex`) blocks the script's pool thread past the timeout — same orphan-thread leak as Finding 2, and a named `Mutex`/`EventWaitHandle` touches a *system-global* kernel object. (b) `Type.GetType("System.IO.File")` / `RuntimeHelpers.GetUninitializedObject(...)` obtain a forbidden type/object without a `typeof`/cast the pass-2 `TypeSyntax` walk would catch. I could **not** demonstrate a full escape — pass-2 catches the obvious follow-ups (declaring the reflection result via `var` resolves to a `System.Reflection` type, and `MethodInfo.Invoke` / casting to a forbidden type are denied) — so this is a hardening gap, not a proven break. +- Suggested fix: add `System.Threading` to `ForbiddenNamespacePrefixes` with a type-granular allow-list for the genuinely-needed sync-free primitives (`CancellationToken` is the only one scripts legitimately touch, and scripts are synchronous so even that is marginal); add `System.Type` member `GetType`/`GetMethod`/… handling and `System.Runtime.CompilerServices.RuntimeHelpers` to the type deny-list. + +### 11. Scripts can allocate unbounded memory / the sandbox has no memory budget +- Severity: Low · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ScriptSandbox.cs:30-35` (class remarks acknowledge it) +- Evidence: the sandbox comment states "cannot prevent a script from allocating unbounded memory or spinning in a tight loop — those are budget concerns, handled by the per-evaluation timeout". The timeout bounds wall-clock but not allocation; a `new byte[int.MaxValue]`-style allocation or a growing collection in a loop can OOM the whole in-process server before the 250 ms timeout fires. +- Failure scenario: an authored script (buggy or hostile) allocates aggressively and OOMs the server. Only mitigated by operators authoring sane scripts and the preview harness. +- Suggested fix: documented trade-off; a true fix needs an out-of-process runner (noted as a v3 concern). Flagging for awareness. + +### 12. `ScriptedAlarmEngine.OnUpstreamChange` writes `_valueCache` before the disposed check +- Severity: Low · Confidence: High +- Location: `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:549-550` +- Evidence: `_valueCache[path] = value;` executes on line 549; `if (_disposed) return;` is line 550. So a late push after Dispose still mutates `_valueCache` (which Dispose never clears). +- Failure scenario: benign in practice — subscriptions are torn down before Dispose returns, and the dict is dropped with the engine. Combines with Finding 1 only as another un-cleared-cache note. +- Suggested fix: move the write below the `_disposed` guard; or accept it (harmless). + +--- + +## Checked and fine (notable) + +- **Collectible-ALC script lifecycle** (`ScriptEvaluator.cs`, `CompiledScriptCache.cs`): every compile loads into a per-evaluator collectible `AssemblyLoadContext`; `Clear`/`Dispose` unload them; failed loads/binds `alc.Unload()` before rethrow; `ScriptedAlarmEngine.LoadAsync` and `Dispose` both route through the cache's `Clear`/`Dispose` and the engine explicitly clears `_alarms` so the evaluator delegates aren't rooted. The "defeated the fix by compiling directly" regression is documented and closed. Genuinely well handled. +- **`ScriptSandbox` reference caches** (`PinnedReferences`, `BclReferences`): static, process-constant, bounded by the BCL assembly set — intended perma-cache, not a leak. +- **`DependencyGraph`** (`Core.VirtualTags`): cycle detection is iterative Tarjan (no stack blow-up), topo-rank is cached and invalidated on mutation (no per-event O(V+E)), shared `EmptySet` avoids per-call allocation. Not thread-safe (plain `Dictionary`), but it's a publish/compose-time single-threaded structure — correct by its usage contract. +- **Resilience pipeline cache** (`DriverResiliencePipelineBuilder`, `CapabilityInvoker`, `DriverResilienceStatusTracker`, `DriverCapabilityInvokerFactory`): lock-free `ConcurrentDictionary` hot path, generation-keyed to prevent stale-child re-cache, retire/reinstate tombstoning to stop removed-instance snapshot resurrection, clamp-before-build so a pipeline build never throws. Thorough. +- **`CompiledScriptCache`**: `ConcurrentDictionary>` with `ExecutionAndPublication` (compile-once), value-scoped `TryRemove` so a concurrent re-add isn't evicted. No capacity bound, but documented as operator-/config-bounded; fine for now. +- **`LocalDbStoreAndForwardSink` capacity**: default 1M cap with drop-oldest eviction + evicted-count telemetry, periodic counter resync against replication drift, idempotent `ON CONFLICT DO NOTHING` inserts, poison-row dead-lettering with retention purge. Store-and-forward growth is capped. Good. +- **`ApiKeySecret`**: fails closed on empty pepper, constant-time compare, CSPRNG secret, hex key-id to dodge the base64 `/`→split hazard. Solid. +- **`DataPlaneAuthorityState`**: monotonic generation counter (not hash-derived — the comment explains why that would silently serve stale ACLs), trie built-and-installed before snapshot publish, Changed handlers invoked outside the lock and per-handler try/caught. Careful. +- **`PollGroupEngine`**: fixed-rate pacing, capped-exponential backoff on failure, fatal-exception classification that tears down vs. swallow-and-retry, deterministic teardown that awaits the loop before disposing the CTS, structural array comparison to avoid spurious change events. Mature. diff --git a/docs/reviews/2026-10-21/06-drivers-session.md b/docs/reviews/2026-10-21/06-drivers-session.md new file mode 100644 index 00000000..bbe6accb --- /dev/null +++ b/docs/reviews/2026-10-21/06-drivers-session.md @@ -0,0 +1,297 @@ +# Review 06 — Drivers (session slice): Galaxy, OpcUaClient, Mqtt, MTConnect, Sql + +Repo: `/Users/dohertj2/Desktop/OtOpcUa` @ `d51d35bd` (master). Production code only (`src/Drivers/...`). Read-only audit; every finding was verified by reading the cited lines (fork-sourced MQTT/MTConnect/SQL findings were spot-checked by the lead against the cited lines). Line numbers are as of this commit. Prefix key: **G** Galaxy, **O** OpcUaClient, **M** MQTT, **T** MTConnect, **S** Sql. + +Host context that shapes several findings (verified outside the slice): + +- **The runtime never disposes a driver instance.** `DriverInstanceActor.PostStop` calls only `_driver.ShutdownAsync()` (`src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs:1550`); `DriverHostActor.StopChild` only stops the actor (`DriverHostActor.cs:3759-3775`); `grep -rn "is IAsyncDisposable\|as IDisposable" src/Server/ZB.MOM.WW.OtOpcUa.Runtime` is empty. The `/raw` browse picker (`src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/DiscoveryDriverBrowser.cs:128-150`) builds a throwaway driver per capture and also only calls `ShutdownAsync`. **`ShutdownAsync` is therefore the only teardown a driver ever gets.** +- The comms-loss watchdog's recovery is `ShutdownAsync` → `InitializeAsync` on the SAME driver object, retried every 10 s while `Reconnecting` (`DriverInstanceActor.cs:808-855`). + +--- + +## 1. Memory / resource leaks + +### G-1. `GalaxyDriver.ShutdownAsync` is a no-op — the whole gRPC runtime outlives the driver on every respawn, removal and `/raw` browse +- **Severity: Critical** · **Confidence: High** +- Location: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/GalaxyDriver.cs:735-741` (ShutdownAsync); `:1517-1568` (DisposeAsync, where teardown actually lives). +- Evidence: + ```csharp + public Task ShutdownAsync(CancellationToken cancellationToken) + { + if (_disposed) return Task.CompletedTask; + _logger.LogInformation("GalaxyDriver {InstanceId} shutting down", _driverInstanceId); + _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + return Task.CompletedTask; + } + ``` + Everything `BuildProductionRuntimeAsync` (`:390-437`) creates — `_ownedMxClient` (gRPC channel), `_ownedMxSession` (a SECOND `MxGatewayClient` + MX session + `Register` handle, `Runtime/GalaxyMxSession.cs:85-87`), the `EventPump` (two background tasks draining `StreamEvents`), `GatewayGalaxyAlarmFeed` (background `StreamAlarms` loop that reconnects forever, `Runtime/GatewayGalaxyAlarmFeed.cs:102-148`), `ReconnectSupervisor`, `PerPlatformProbeWatcher`, `DeployWatcher` (`WatchDeployEvents` stream loop) and `_ownedRepositoryClient` — is released only in `DisposeAsync`, which nothing in the host calls. +- Failure scenario: (a) a config change is a respawn (new `GalaxyDriver` from the factory; the old child only gets `ShutdownAsync`) → the old instance keeps its MX session registered on mxaccessgw, keeps draining `StreamEvents`/`StreamAlarms`, keeps the deploy-watch stream open and its supervisor loop alive — permanently, one set per deploy. (b) Every AdminUI `/raw` browse of a Galaxy device (`DiscoveryDriverBrowser.CaptureAsync` → `InitializeAsync` → `DiscoverAsync` → `ShutdownAsync`) leaks a full runtime: two gRPC channels, an MX session, both streams, a 50k-slot bounded channel. The gateway accumulates live sessions and STA-pumped advises; OtOpcUa accumulates threads and channels. +- Suggested fix: make `ShutdownAsync` do the device-state half of `DisposeAsync` (stop pump/alarm feed/deploy watcher/probe watcher/supervisor, dispose session + clients, null the seams so a later `InitializeAsync` rebuilds) and keep `DisposeAsync` as the idempotent final teardown. The `#630` contract already puts device state under `InitializeAsync`/`ShutdownAsync` for the other eleven drivers. + +### G-2. Every `InitializeAsync` on a live `GalaxyDriver` creates a new `DeployWatcher` without stopping the previous one +- **Severity: High** · **Confidence: High** +- Location: `GalaxyDriver.cs:631-654` (`StartDeployWatcherAsync`), called unconditionally from `InitializeAsync` `:377`. +- Evidence: + ```csharp + var source = new GatewayGalaxyDeployWatchSource(_ownedRepositoryClient); + _deployWatcher = new DeployWatcher(source, _logger); // previous watcher dropped, still running + _deployWatcher.OnRediscoveryNeeded += (_, args) => OnRediscoveryNeeded?.Invoke(this, args); + var startTask = _deployWatcher.StartAsync(CancellationToken.None); + ``` + `InitializeAsync` (`:357-382`) skips `BuildProductionRuntimeAsync` on a second call (seams non-null) but always runs this; `DeployWatcher.RunLoopAsync` (`Browse/DeployWatcher.cs:133-235`) runs until cancelled. +- Failure scenario: the watchdog's recovery is `ShutdownAsync` (no-op, G-1) + `InitializeAsync`. While mxaccessgw is down the supervisor reports `Degraded` with a frozen `LastSuccessfulRead` (G-11), so the host trips every ~90 s and each trip spawns another watch-stream loop with its own 1–30 s backoff. When the gateway returns, N watchers each raise `OnRediscoveryNeeded` per real deploy and N streams stay open for the process lifetime. +- Suggested fix: stop/dispose the existing watcher at the top of `StartDeployWatcherAsync` (or early-return when one exists), and stop it in `ShutdownAsync` (G-1). + +### O-1. OpcUaClient `SubscribeAsync` leaves a half-created subscription on the session when `CreateItemsAsync` (or `CreateAsync`) throws — one per host retry +- **Severity: Medium** · **Confidence: High** +- Location: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs:1781-1826`. +- Evidence: + ```csharp + session.AddSubscription(subscription); + await subscription.CreateAsync(cancellationToken).ConfigureAwait(false); + ... + item.Notification += notifHandler; + ... + await subscription.CreateItemsAsync(cancellationToken).ConfigureAwait(false); + _subscriptions[id] = new RemoteSubscription(subscription, handle, itemHandlers); + ``` + No `catch`: on a throw the SDK `Subscription` stays in `session.Subscriptions` (created server-side if `CreateAsync` succeeded) with every `MonitoredItem.Notification` closure still attached, untracked by `_subscriptions`, so neither `UnsubscribeAsync` nor `ShutdownAsync` deletes it. The alarm path has exactly this cleanup (`:2204-2226`: "a half-built subscription left on the session would accumulate one per retry"); the data path does not. +- Failure scenario: a server that accepts `CreateSubscription` but rejects `CreateMonitoredItems` (e.g. `BadTooManyMonitoredItems`, transient timeout) makes `DriverInstanceActor`'s health-tick reconcile retry every tick; each retry adds a live, publishing subscription on the upstream server until the next re-initialise disposes the session — and servers with per-session caps then refuse the real subscribe. +- Suggested fix: wrap `:1789-1824` in `try/catch` mirroring the alarm path — detach handlers, bounded `session.RemoveSubscriptionAsync(subscription)`, rethrow. + +### S-1. SQL key-value and where-pair plans have no row cap; an ambiguous selector materialises every matching row on every poll +- **Severity: Medium** · **Confidence: High** +- Location: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs:233-238` (key-value `SELECT … WHERE key IN (…)`, no limit), `:271-281` (where-pair `… WHERE col = @w`, no limit); `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs:445-452`: + ```csharp + var rows = new List(); + while (await reader.ReadAsync(cancellationToken).ConfigureAwait(false)) { ... rows.Add(row); } + ``` + `SelectWideRow` (`:552-560`) explicitly accepts that "an ambiguous selector materialises every matching row for one poll". +- Failure scenario: a tag authored against a log/history table (`keyColumn=TagName` on a 50M-row samples table, or `whereColumn=Line` on an events table) pulls every matching row into memory **every poll** (default 5 s). Memory spikes to the match-set size, GC churn, and `commandTimeout` is the only brake; nothing refuses this at deploy time and the only signal is a once-per-minute contract warning. +- Suggested fix: cap the read loop (stop after N rows, publish a configuration-error status and warn when the cap is hit), or emit dialect `TOP/LIMIT N+1` so ambiguity is still observable without unbounded materialisation. + +### M-1. A failed MQTT SUBSCRIBE (no connection drop) returns a live handle and drops the topic from the re-subscribe set — tags stay dark behind a Healthy connection +- **Severity: Medium** · **Confidence: High** (state-kept-wrong after failure; same family as a leak) +- Location: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs:667-684` (transport throw → `DegradeTopic` → `return 0`), `:719-721` (`_subscribedTopics.TryRemove(topic, out _)`), `:417-418` (`SubscribeAsync` returns the handle regardless), `:489-499` (`OnReconnectedAsync` re-subscribes `_subscribedTopics` only). +- Failure scenario (plain mode): the SUBSCRIBE overruns its deadline or the broker transiently refuses a filter. The topic leaves `_subscribedTopics`, its tags go `BadCommunicationError`, but `SubscribeAsync` still returns a handle so `DriverInstanceActor` considers the subscription live and never re-subscribes; the next reconnect re-SUBSCRIBEs only `_subscribedTopics`, which no longer holds it. Connection state stays `Connected`/Healthy and `LastMessageUtc` advances from other topics, so the watchdog never trips. Dark until a redeploy. +- Suggested fix: keep failed filters in a pending set that `OnReconnectedAsync`/the next `SubscribeAsync` retry, or throw from `SubscribeAsync` on a transport failure so the actor's reconcile re-subscribes. + +### T-1. MTConnect: a late (superseded/abandoned) pump publishes its stale cursor onto the NEW session — the doc says it is dropped, the code has no generation guard +- **Severity: Low** · **Confidence: Medium** +- Location: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:2011-2020` (pump `finally { PublishAgentCursor(instanceId, cursor); }`, comment "dropped if a lifecycle change has already installed a newer session"), `:2491-2508` (`PublishAgentCursor` only early-returns on `session is null` or an identical `(InstanceId, NextSequence)`; it CAS-writes onto whatever session is installed — nothing checks `_sessionGeneration`), `:1789-1811`, `:2914-2922`. +- Failure scenario: a pump retired into `_supersededPumps` or abandoned after `TeardownWait` finishes after `StartCoreAsync` installed the new session; the next pump start reads the stale `InstanceId`, treats the first chunk as an Agent restart → `ObservationIndex.Clear()` + Bad flicker on every subscribed tag + a needless re-baseline (`:2061-2081`). Bounded (one flicker), no leak. +- Suggested fix: stamp the pump with `_sessionGeneration` and have `PublishAgentCursor` refuse when it differs (or compare `ReferenceEquals(session.Client, client)`). + +### Low-severity leak nits (all High confidence) +- **G-3** `ReconnectSupervisor.ReportTransportFailure` (`Runtime/ReconnectSupervisor.cs:127-141`) allocates a new `_loopCts` per recovery cycle and disposes only the last one in `Dispose()`. Bounded by outage count. +- **O-2** `OpcUaClientDriver.InitializeAsync` (`:521-568`): the catch closes `Session` (the property, still null) rather than the local `session`, so a throw in `:544-556` (`AttachDiagnostics`/`NamespaceMap.FromSession`/`RecordSkippedTags`) orphans an activated session with its keep-alive handler until GC. Rare. +- **O-3** `SdkTelemetry.ForLogger` (`OpcUaClientDriver.cs:85`, `SdkTelemetry.cs:28-40`) creates an `ILoggerFactory` per driver construction, released only in `Dispose`/`DisposeAsync` (`:3365`, `:3395`) — which the host never calls; `_gate` and `_sessionWorkCts` likewise. One small object set per respawn, unbounded count. +- **M-2** `LastValueCache` (`MqttSubscriptionManager.cs:398`, `Sparkplug/SparkplugIngestor.cs:553`, `LastValueCache.cs:42-46`) gains an entry per non-authored reference ever subscribed and `Register` never prunes `Values`; bounded by the union of every RawPath authored across redeploys. +- **M-3** A failed initial connect leaves the new `MqttConnection`/`IMqttClient` undisposed (`MqttDriver.cs:967-986`, catch at `:1029` rethrows without disposing; contrast `EstablishAsync` at `:1011-1019`). Only one stale connection is held at a time (`_transport` is replaced each attempt), no socket. +- **S-2** A timed-out SQL group keeps its connection + `maxConcurrentGroups` slot until the provider returns (`SqlPollReader.cs:275-335`, `:345-357`); documented and bounded (default 4), but the bound is per driver, not per poll — pool sizing should know. +- **S-3** `SqlPollReader._gate` is never disposed and a new reader is built per `ApplyBinding` (`SqlPollReader.cs:53-59`, `SqlDriver.cs:264-273`); managed-only today. + +--- + +## 2. Unimplemented / stubbed + +### G-4. Galaxy per-platform `ScanState` host probing is structurally dead: the probe watcher's handles are never registered in `SubscriptionRegistry`, so `EventPump` drops every probe event +- **Severity: High** · **Confidence: High** +- Location: `Health/PerPlatformProbeWatcher.cs:125-150` (subscribes via `_subscriber.SubscribeBulkAsync`, stores handles only in its own `_itemHandlesByPlatform`), `Runtime/EventPump.cs:314-317`, `GalaxyDriver.cs:1465-1489`. +- Evidence: + ```csharp + // EventPump.DispatchDataChange + var subscribers = _registry.ResolveSubscribers(ev.ItemHandle); + if (subscribers.Count == 0) return; // stale event after unsubscribe — drop quietly + ``` + `SubscriptionRegistry.Register` is called only from `ReadViaSubscribeOnceAsync` (`:959`) and `SubscribeAsync` (`:1110`) — never for the watcher's handles. `OnPumpDataChange` (`:1480-1488`), the only caller of `OnProbeValueChanged`, is reached only via the pump's fan-out, so a `*.ScanState` event never gets there. The only test (`tests/.../Health/PerPlatformProbeWatcherTests.cs`) drives `OnProbeValueChanged` directly. +- Also: after a reconnect (`ReopenAsync` → `GalaxyMxSession.RecreateAsync`) the watcher's handles are dead and nothing re-subscribes them — `SyncPlatformsAsync` is called only from `DiscoverAsync` (`:800`); `ReplayAsync` rebinds only `_subscriptions`. +- Failure scenario: `IHostConnectivityProbe.GetHostStatuses()` for Galaxy only ever carries the synthetic transport entry; per-$WinPlatform/$AppEngine Running/Stopped on `/hosts` never updates. Each discovery also leaves N `ScanState` items advised on the gateway for nothing (released only by `Dispose`, G-1). +- Suggested fix: register the probe handles in the registry under a reserved subscription id (and `Rebind` them in `ResubscribeAllAsync`), or route raw `MxEvent`s to the watcher by item handle before the registry lookup. + +### M-4. `sparkplug.birthObservationWindowSeconds` is authored, range-checked and persisted by the AdminUI but consumed by nothing +- **Severity: Medium** · **Confidence: High** +- Location: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs:222-227` (`BirthObservationWindowSeconds = 15`, documented as the browse/discovery birth-collection window). `grep -rn BirthObservationWindowSeconds src` finds no reader outside Contracts and the AdminUI form (`MqttDriverForm.razor:222`, `MqttDriverFormModel.cs:168,274,376`); `MqttDriverBrowser.OpenBudget` uses `ConnectTimeoutSeconds` clamped 5–30 s (`MqttDriverBrowser.cs:188-189`). +- Failure scenario: an operator raising the window for slow births sees nothing change; discovery stability is governed by the host's `UntilStable` loop and the browse window by `ConnectTimeoutSeconds`. +- Suggested fix: feed it into `MqttDriverBrowser.OpenBudget`/the UntilStable cadence, or remove it from the contract and form. + +### G-5. `Repository.DiscoverPageSize` has an AdminUI field and is parsed by the factory, but nothing consumes it +- **Severity: Low** · **Confidence: High** +- Location: `GalaxyDriverFactoryExtensions.cs:94`, `Contracts/GalaxyDriverOptions.cs:114`, `Browse/GatewayGalaxyHierarchySource.cs:24-25` (`_client.DiscoverHierarchyAsync(cancellationToken)` — no page size), AdminUI `GalaxyDriverForm.razor:94`. +- Suggested fix: thread it into `DiscoverHierarchyAsync` or drop the field. + +### G-6 / O-4. `ProbeTimeoutSeconds` exists on both `GalaxyDriverOptions` and `OpcUaClientDriverOptions` and is consumed nowhere +- **Severity: Low** · **Confidence: High** +- Location: `GalaxyDriver.cs:732` (only use: equality compare); `OpcUaClientDriverOptions.ProbeTimeoutSeconds` has 0 references in driver, browser, session or any host project. Both probes take the caller's `timeout` argument instead. + +### O-5. OpcUaClient AdminUI browse session's `AttributesAsync` is a stub; Certificate auth is refused "in v1" +- **Severity: Low** · **Confidence: High** +- Location: `OpcUaClient.Browser/OpcUaClientBrowseSession.cs:56-61` (`Array.Empty()`), `OpcUaClientDriverBrowser.cs:61-63`. +- Impact: no DataType/access info in the live picker side-panel (the `/raw` browse-commit via `DiscoveryDriverBrowser` does enrich); a certificate-authenticated upstream cannot be browsed from the AdminUI. + +### M-5. Sparkplug array metrics are refused ("not supported in v1") yet the browser still offers them as bindable +- **Severity: Low** · **Confidence: High** (documented decision) +- Location: `SparkplugIngestor.cs:1274-1285` (`if (binding.DataType.IsSparkplugArray()) { WarnOnce(...); return; }`; DataSet/Template/PropertySet at `:1287-1302`); `MqttBrowseSession.cs:387-392` lists the leaf with `IsArray: true`. +- Impact: an operator can commit a tag the ingestor never feeds — warned once, then `BadWaitingForInitialData` forever. +- Suggested fix: mark array leaves not-bindable in `DescribeMetric` as `mapped is null` already is. + +### T-2. MTConnect Agent-restart detection lives only on the `/sample` pump; `ReadAsync` discards `instanceId` +- **Severity: Low** · **Confidence: High** +- Location: `MTConnectDriver.cs:716-738` (`ReadAsync` ignores `current.InstanceId`), `:2061-2081` (only `chunk.InstanceId != instanceId` check), `:1687-1711` (`AnnounceAgentRestart`). +- Impact: a driver with no active subscription (browse-only) keeps a cached `/probe` model across an Agent restart and `OnRediscoveryNeeded` never fires. Production always subscribes, so browse-only edge. + +### S-4. SQL: `SqlTagModel.Query` is on the enum but unsupported end-to-end; `MySql`/`Odbc` providers are reserved; `allowWrites` is parsed and ignored +- **Severity: Low** · **Confidence: High** (all documented deferrals that fail loudly or warn once) +- Location: `Driver.Sql.Contracts/SqlEquipmentTagParser.cs:75-76`, `SqlGroupPlanner.cs:336-339`; `SqlDriverFactoryExtensions.cs:214-225`, `SqlDriverProbe.cs:196-211`, `SqlDriverBrowser.cs:135-141`; `SqlDriverFactoryExtensions.cs:151-161` + `SqlDriver.cs:30-31` (no `IWritable`). +- Note: a `model: "Query"` tag is silently skipped (ledger + `BadNodeIdUnknown`) rather than refused at the deploy gate. + +### Dead parsed surface (Low, High confidence) +- **T-3** `MTConnectObservation.SampleRate` parsed per observation (`MTConnectStreamsParser.cs:424-428`) and never read; `MTConnectTagDefinition.MtType/MtSubType/Units` carried but only `MtCategory` is read (`MTConnectDriver.cs:3493`). +- **S-5** `IS_NULLABLE` projected by every dialect's `ListColumnsSql` (`SqlServerDialect.cs:107-109`, `PostgresDialect.cs:141-144`, `OracleDialect.cs:184-187`) and never read by `SqlBrowseSession.cs:359-371` / `SqlCatalogLoader.cs:117-124`. + +--- + +## 3. Performance limiters + +### T-4. Every MTConnect `ReadAsync` batch fetches + parses the whole device `/current` and builds a fresh FrozenDictionary index, even while the pump holds a live one +- **Severity: Medium** · **Confidence: High** +- Location: `MTConnectDriver.cs:718-732`; `MTConnectObservationIndex.cs:149-164` (`ToFrozenDictionary` per construction). +- Evidence: `var current = await BoundedAsync(client.CurrentAsync, …); var snapshot = new MTConnectObservationIndex(binding.IndexTags); snapshot.Apply(current, baseline: true);` +- Failure scenario: OPC UA clients polling with Read (no subscription) against a 2–5k DataItem Agent cost one full XML transfer + parse + index build per Read call regardless of batch size; a few such clients make the Agent the bottleneck. The single-writer rationale (`:654-671`) forbids writing the shared index, not reading it. +- Suggested fix: read through `ObservationIndex` while `_pumpTask` is live and `!_streamDegraded`, keep `/current` as the fallback; or prebuild `binding.IndexTags` as a FrozenDictionary on `TagBinding`. + +### T-5. MTConnect subscription throughput is hard-capped at `SampleCount / SampleIntervalMs` (default 1000 obs/s) with no lag detection or metric +- **Severity: Medium** · **Confidence: Medium** +- Location: `MTConnectAgentClient.cs:154-156` (`&interval=…&count=…`), `MTConnectDriver.cs:2083-2091` (gap → re-baseline), `:1961-1978`. +- Failure scenario: a machine producing more observations/s than `count/interval` makes the cursor fall steadily behind the Agent ring buffer; every buffer/rate seconds the driver warns, re-baselines via `/current` (losing intermediate transitions) and reopens — forever, every delivered value backlog-stale. Nothing surfaces the lag and nothing adapts `count`. +- Suggested fix: counter/Warning when chunks repeatedly carry exactly `SampleCount` observations or `expected - chunk.FirstSequence` shrinks toward zero; document the cap next to `SampleCount`. + +### G-7. `GalaxyDriver.DiscoverAsync` downloads the full Galaxy hierarchy twice per discovery +- **Severity: Medium** · **Confidence: High** +- Location: `GalaxyDriver.cs:787-800`: `discoverer.DiscoverAsync(...)` (which calls `_source.GetHierarchyAsync`, `Browse/GalaxyDiscoverer.cs:68`) then `await source.GetHierarchyAsync(cancellationToken)` again just to select platforms. +- Failure scenario: two full `DiscoverHierarchyAsync` sweeps (every object + attribute) per browse/deploy on a large galaxy — and the second feeds the dead probe path (G-4). +- Suggested fix: have `GalaxyDiscoverer` expose the hierarchy it fetched. + +### O-6. One `SemaphoreSlim _gate` serialises OpcUaClient reads, writes, subscribes, acks AND the full recursive discovery browse +- **Severity: Medium** · **Confidence: High** +- Location: `OpcUaClientDriver.cs:1213` (Read), `:1302` (Write), `:1436-1466` (DiscoverAsync holds the gate across `BrowseRecursiveAsync` + enrichment), `:1781`, `:2195`, `:2301`. +- Failure scenario: a stalled write (up to `_options.Timeout`) blocks every read and subscription create on that driver; recovery/reconcile reads queue behind it. Discovery on the runtime instance would block the data plane for the browse duration (latent today — discovery runs on the throwaway `DiscoveryDriverBrowser` instance). +- Suggested fix: separate read/write gating from subscription management; don't hold the gate across the whole recursive browse. + +### S-6. SQL per-poll warning spam on a persistently failing or frozen group is not rate-limited +- **Severity: Low** · **Confidence: High** +- Location: `SqlPollReader.cs:345-357` (`TimedOut` warns every poll per group), `:411-419` (`QueryAsync` warns with the full `plan.SqlText` every poll per failed group); only contract warnings go through `ShouldWarnAboutContract()` (`:714-721`). +- Failure scenario: a dropped table or paused server with 20 groups at 1 s = 20 warnings/s with SQL text, indefinitely. +- Suggested fix: reuse the rate-limiter per plan, or log once per state transition. + +### S-7. SQL query plans are re-planned and the slot dictionary re-allocated on every poll +- **Severity: Low** · **Confidence: Medium** +- Location: `SqlPollReader.cs:167-182` (`Dictionary>` keyed by a 12-field positional record, rebuilt per `ReadAsync`), `:206` (`SqlGroupPlanner.Plan` per poll) vs `SqlQueryPlan.cs:68-70` ("safe to cache and reuse across polls" — nothing caches). +- Suggested fix: cache plans per subscription reference list, invalidated on `ApplyCatalogGateAsync` swap. + +### Serial write paths (Low, High confidence) +- **G-8** Galaxy writes are strictly serial, three round-trips on first touch (`Runtime/GatewayGalaxyDataWriter.cs:124-131`, `:152-183`: AddItem → AdviseSupervisory → Write); acks serial by design (`GalaxyDriver.cs:1364-1381`). +- **M-8** MQTT `WriteAsync` publishes serially, one awaited QoS 1 round-trip per request bounded by `ConnectTimeoutSeconds` (`MqttDriver.cs:695-749`, `MqttConnection.cs:1172`) — 50 writes × up to 15 s on a stalled broker. +- **T-6** MTConnect writes are one POST per slot (`MTConnectDriver.cs:804-878`) though the Agent PUT surface accepts several `id=value` pairs. + +### Other Low perf items (High confidence) +- **M-6** Wildcard-authored MQTT topics are matched linearly against every inbound message on MQTTnet's dispatcher thread (`MqttSubscriptionManager.cs:586-592`); concrete topics are a dictionary lookup. Fine for hundreds of wildcard tags, not thousands. +- **M-7** `TryDecodeUtf8` allocates `new UTF8Encoding(false, true)` per Raw/Scalar delivery (`MqttSubscriptionManager.cs:1374`); `MqttBrowseSession.cs:150` already uses a static. +- **T-7** The MTConnect connectivity probe parses the full `/probe` device model every 5 s and discards it (`MTConnectDriver.cs:1496-1504`, `MTConnectAgentClient.cs:169-180`); documented trade-off (`:1456-1462`). `ResponseHeadersRead` + status check would do. +- **O-7** `OnDataChange` is invoked under the driver-wide `_deliveryLock` (`OpcUaClientDriver.cs:368-380`, `:1888-1912`) — deliberate; acceptable only while every subscriber just `Tell`s. + +--- + +## 4. Other hazards + +### G-9. The comms-loss watchdog's re-init does nothing for Galaxy (no teardown, no rebuild); recovery rests entirely on the in-driver supervisor +- **Severity: Medium** · **Confidence: High** +- Location: `GalaxyDriver.cs:366-369` (`if (_subscriber is null && _dataWriter is null && _hierarchySource is null) BuildProductionRuntimeAsync`), `:735-741`. +- Evidence: after the first init the seams are non-null, so a watchdog-driven `ShutdownAsync`+`InitializeAsync` neither tears down nor reconnects — it sets `_health = Healthy` (`:381`, masked by `GetHealth()` preferring `_supervisor.IsDegraded`) and spawns another DeployWatcher (G-2). If the supervisor loop ends with a dead session (e.g. `ReopenAsync` returning early on `_ownedMxSession is null`, `:448`, then `Replaying`→`Healthy`), the host has no lever short of a respawn. +- Suggested fix: as G-1 — make `ShutdownAsync` release the runtime so the host's re-init genuinely rebuilds. + +### O-8. OpcUaClient `AcknowledgeAsync` swallows non-cancellation transport failures, so the host reports the Part 9 ack as applied +- **Severity: Medium** · **Confidence: High** +- Location: `OpcUaClientDriver.cs:2384-2391`: + ```csharp + catch (Exception ex) + { + _logger.LogWarning(ex, "...AcknowledgeAsync failed; acknowledgements may not have been applied."); + } + ``` + A `ServiceResultException` from `session.CallAsync` (faulted session, closed channel) returns normally; the local condition is acked while upstream still shows it unacknowledged. The next `ConditionRefresh` dedup will re-report it as unacked, but the operator received success. +- Suggested fix: rethrow (or return per-ack failures) so the OPC UA `Acknowledge` call fails with the transport status. + +### G-11. Galaxy `LastSuccessfulRead` never advances on data — stamped only at init and on supervisor `Healthy` +- **Severity: Low** · **Confidence: High** +- Location: `GalaxyDriver.cs:381, 593-597, 661, 739`; no write from `OnPumpDataChange`/`ReadViaSubscribeOnceAsync`. +- Failure scenario: the watchdog's conjunction degenerates to "non-Healthy for 90 s" for Galaxy, so any supervisor `Degraded` lasting 90 s trips the host re-init even while values still flow on the old stream; with G-2 that is one leaked DeployWatcher per 90 s. +- Suggested fix: stamp (throttled) from `OnPumpDataChange`, as OpcUaClient's `RecordLivenessEvidence` does. + +### G-10. `EventPump` dispatch loop dying leaves the read loop alive and health `Healthy` — clean termination evading the watchdog +- **Severity: Low** · **Confidence: Medium** +- Location: `Runtime/EventPump.cs:276-295`. +- Evidence: `DispatchLoopAsync` catches any exception, logs "events past this point will be lost until restart" and exits; `RunAsync` keeps draining into the bounded channel until it fills, then only counts drops. Nothing calls `NotifyStreamFault`; `GetHealth()` stays `Healthy` and (G-11) the stamp never moves anyway. `Dispatch` isolates handler exceptions, so the remaining throw sites are `StatusCodeMap.FromMxStatus`/`ToSnapshot` — hence Medium confidence on reachability. +- Suggested fix: treat a dispatch-loop exit while not cancelled as a stream fault, as the read loop does for a server-completed stream (`:173-188`). + +### T-8. MTConnect logs and surfaces the raw `AgentUri` (which may carry `user:pass@`) while its own probe deliberately redacts it +- **Severity: Low** · **Confidence: Medium** +- Location: `MTConnectDriver.cs:1396` (`HostName => _options.AgentUri`), log lines at `:852, 1215, 2064, 2086, 2136, 2757, 2790`; contrast `MTConnectDriverProbe.cs:227-243, 251-256` (`RedactedDisplay`, "ex.Message is an open door for a credential leak"). +- Suggested fix: compute a redacted display URI once at `StartCoreAsync` and log/expose that. + +### S-8. SQL factory quotes the JSON parser message on a malformed config; the probe and browser withhold it for credential hygiene +- **Severity: Low** · **Confidence: Medium** +- Location: `SqlDriverFactoryExtensions.cs:1002-1006` (`$"... is not valid JSON: {ex.Message}"`) vs `SqlDriverProbe.cs:84-95`, `SqlDriverBrowser.cs:292-303` (message withheld "because the malformed text may carry a connection string"). The deployed config should carry only `connectionStringRef`, so this bites only on a pasted literal — but the message then reaches `InitializeFailed` → health `LastError` → `/hosts`. + +### O-9. OpcUaClient `ShutdownAsync` can take N×5 s against a half-dead upstream and blocks the actor's `PostStop` +- **Severity: Low** · **Confidence: High** +- Location: `OpcUaClientDriver.cs:1121-1135` (serial best-effort `DeleteAsync` per subscription, each bounded by 5 s, skipped only when `_sessionLost == 1`), `DriverInstanceActor.cs:1550` (`ShutdownAsync(...).GetAwaiter().GetResult()`). +- Suggested fix: one bounded budget for the whole teardown, or delete concurrently. + +### Low hazards, documented / bounded (High confidence) +- **M-10** `MqttDriver.ShutdownAsync`/`DisposeAsync` fall back to an ungated `TeardownAsync` after `ConnectTimeoutSeconds` (`MqttDriver.cs:402-409`, `:929-935`); the post-connect disposed re-check (`:993-997`) covers `DisposeAsync` only, so a wedged `InitializeCoreAsync` completing after a timed-out `ShutdownAsync` can publish one stray live connection — the actor's own Shutdown→Initialize sequence tears it down next time (bounded to one). +- **M-11** `MqttSubscriptionManager.EstablishAsync` with no transport records topics as subscribed and returns success (`:646-665`); unreachable today (`AttachTo` always precedes connect, `MqttDriver.cs:972-979`) but a silent "subscribed, receives nothing" if the ordering ever changes. +- **O-10** `ConnectionLost` raised before the host processed `InitializeSucceeded` is swallowed — documented known gap (`OpcUaClientDriver.cs:140-145`). +- **S-9** A stale SQL poll outliving the 5 s dispose wait (operationTimeout 15 s) can call `ObservePollOutcome` during the next `InitializeAsync`; `UpdateHealthUnlessFaulted` guards only `Faulted`, so a late Good read can briefly flip `Initializing`→`Healthy` (cosmetic; next `WriteHealth` overrides). +- **S-10** The SQL catalog gate rejects rather than reloads on schema drift (`SqlDriver.cs:929-972`, `:898-901`): a column added after init stays `BadNodeIdUnknown` until a reinit. Deliberate (#523 no-I/O-on-healthy rule); listed so operators know the surface is the skip ledger. + +--- + +## 5. Checked and fine + +**Galaxy** +- `ReadViaSubscribeOnceAsync` (`GalaxyDriver.cs:856-1009`): subscribe-once reads are the only MxAccess read primitive; handles unsubscribed in `finally`; internal 30 s deadline covers `CancellationToken.None`; a `SubscribeBulk` completing after the deadline is unsubscribed by continuation. +- `SubscriptionRegistry` prunes the reverse map on `Remove`/`Rebind`; nothing grows past the live subscription set. +- `EventPump` channel is bounded (50 000 default, configurable) with explicit `TryWrite` + `EventsDropped`; a server-completed stream is treated as a fault (`EventPump.cs:173-188`). Replay-gap handling is unreachable by design (`afterWorkerSequence: 0`, `GatewayGalaxySubscriber.cs:161`) and documented. +- `SubscribeAlarmsAsync` collapses to a single handle (`:1310-1322`); the per-reconnect handle growth it describes is closed. +- `GatewayGalaxyDataWriter` handle/advise caches are cleared on reconnect (`InvalidateHandleCaches`, `GalaxyDriver.cs:453`) and bounded by the written tag set; the fail-closed advise path forgets a failed handle. +- `GalaxyBrowseSession`/`GalaxyDriverBrowser`, `GalaxyDriverProbe`: client ownership transfers into the session and is disposed on `DisposeAsync`/`finally`; open path disposes on failure. + +**OpcUaClient** +- `ReleaseSessionAsync` (`:1055-1095`) detaches keep-alive + diagnostics handlers, bounds `CloseAsync` by 5 s, skips the wire close when the session is known lost, and re-arms every outage latch; `InitializeAsync` calls it first, so a re-init cannot orphan the previous session. +- Comms-loss contract (#527/#670/#676): bad keep-alive → `Degraded` with the stamp preserved; good keep-alives/notifications advance it only while no subscription is latched lost; definitive statuses raise `ConnectionLost` once per session; `PublishCommsLoss` fans `BadNoCommunication` with value retained; recovery dedup is `(value, status)` (`IsReconcileRepair`). +- `_lastValues`/`_lastGoodSourceTicks`/`_deliverySeq`/`_awaitingRecovery`/`_replayGuard` are keyed by authored RawPath and cleared in `ShutdownAsync`; bounded even though `UnsubscribeAsync` does not prune them. +- `CancelSessionWork` deliberately does not dispose the old CTS (documented race with the keep-alive thread); no timer attached. +- Discovery releases continuation points on cancel (`:1533-1550`); `MaxDiscoveredNodes`/`MaxBrowseDepth` bound the browse. Browser session/`OpcUaClientDriverBrowser`/probe dispose on every path. + +**MQTT** +- Teardown-before-rebuild still holds: `InitializeCoreAsync` calls the idempotent `TeardownAsync()` first (`MqttDriver.cs:955`), which reaches all three disposable fields — `_primaryHost`, `_hostProbeCts`, `_connection` (`:1381-1405`); no new disposable field escaped it. +- `MqttConnection` reconnect loop does not leak per attempt: one `IMqttClient` created once (`:718-724`), per-attempt CTSs `using`-scoped (`:689-693`), single supervisor task (`??=`, `:909`) awaited bounded on dispose (`:498-509`). +- Sparkplug caches are bounded by configuration, not wire: unauthored edge nodes dropped before decode (`SparkplugIngestor.cs:736-745`); `Births`, `_trackers`, `_lastRebirthTicks`, `_nodeBirthSeen`, `_metadataRebirths` pruned to authored nodes on `Register` (`:421-454`) and cleared on session reset; NDEATH/DDEATH evict (`:1076`, `:1091`); `_birthSignatures` pruned to `_authoredScopes` (`MqttDriver.cs:1221-1227`); `_warned` capped at 1024 (`:1912-1922`); `_pendingRebirths` self-prunes (`:1803-1810`). Rebirth storms gated (10 s per-node debounce, 3-attempt metadata cap, signature gate on rediscovery). +- Comms-loss contract: `GetHealth()` derives state from `MqttConnection.State` and `LastSuccessfulRead` from `LastMessageUtc` (`MqttDriver.cs:421-440`); `ConnectionLost` fires on the `Connected→Reconnecting` edge (`MqttConnection.cs:976-992`); both ingest paths stale to `BadNoCommunication` (`MqttSubscriptionManager.cs:269-282`, `SparkplugIngestor.cs:366-379`); plain-mode suppression keyed on `(status, value)` (`:824-828`); every (re)subscribe re-pushes cached snapshots. +- Inbound handling runs synchronously on MQTTnet's dispatcher with one JSON parse per message shared across matched tags; back-pressure is MQTTnet's per-client serialisation; the only handler `Tell`s. +- Browse session: atomic node-cap reservation, bounded payload inspection (512 B / 1 MiB birth), client disposed on `DisposeAsync` and on open failure. Primary host: separate client id, Will + explicit OFFLINE, claim backoff 2→30 s, connection disposed when ONLINE publish fails. + +**MTConnect** +- Comms-loss contract: stream failure → `Degrade(error)` preserving `LastSuccessfulRead` (`:3152-3159`); every chunk incl. heartbeats → `PublishHealthy(now)` (`:2102-2103`); a quiet enumeration end under a live token is reported as `MTConnectStreamEndedException`/Transient, never idle (`:2106-2117`, `MTConnectAgentClient.cs:237-243`) — no clean-termination evasion; `/current` and `/sample` own separate degraded flags. +- UNAVAILABLE → `BadNoCommunication`, value null (`MTConnectObservationIndex.cs:421-425`); conditions stay active with quality-only Bad (`MTConnectConditionAlarmMapper.cs:90-97`), as the rules file specifies. +- instanceId/sequence: restart checked before gap (`:2055-2081`); gap and OUT_OF_RANGE both re-baseline (`:2127-2141`); backoff resets only when the stream delivered (`:1975-1990`). +- Re-init without teardown (#630): `InitializeAsync` stops pump + probe and disposes the old client before building (`:565-574`); a client built in a failed attempt is disposed (`:2783`). `MTConnectAgentClient` disposed on every teardown path; `HttpResponseMessage`/streams `using`/`await using` (`MTConnectAgentClient.cs:175-191, 208-214`); heartbeat watchdog bounds every body read, buffer capped at 32 MiB, DTD prohibited. +- `_supersededPumps` is pruned (`:1742-1754`); index/tracker/told-record maps are bounded by the Agent's DataItem count. Streams parser is forward-only `XmlReader` with a single-string fast path; no JSON on the hot path. + +**SQL** +- Every `DbConnection`/`DbCommand`/`DbDataReader` path uses `await using` (`SqlPollReader.cs:371-405`, `SqlDriver.cs:982-989`, `:1086-1098`, `SqlCatalogLoader.cs:153-203`, `SqlBrowseSession.cs:396-404`); the browser disposes on open failure and hands ownership to the session otherwise. +- CTS/slot ownership on timeout paths is transferred to the work task and disposed in its `finally` (`SqlPollReader.cs:282-316`, `SqlDriver.cs:1042-1060`); `AbandonedTask.Detach` observes faults. +- Connection string never logged: only `Endpoint` is rendered (`SqlDriver.cs:1114-1128`); health/host messages use exception type names; browser redacts and drops inner exceptions. +- SQL injection surface closed: values bound everywhere; identifiers rewritten to catalog spellings by the allow-list gate before quoting (`SqlCatalogGate.cs:142-151`) then `QuoteIdentifier` with doubling + control-char rejection in all three dialects; Oracle `BindByName` set at every site; >2-part names rejected. +- Comms-loss contract: `ObservePollOutcome` (`SqlDriver.cs:606-675`) advances the stamp on any "database answered" code and degrades on timeouts; all-`BadTimeout` freezes it → watchdog → `ShutdownAsync` (`_poll.DisposeAsync`, 5 s cap) → `InitializeAsync` → re-subscribe; a query-failure-only outage is `Degraded` with the clock advancing, so no reconnect loop. Groups run in parallel under `Task.WhenAll` bounded by `maxConcurrentGroups`; key-value groups chunk at dialect bind caps; `CommandTimeout` floored at 1 s; factory enforces `operationTimeout > commandTimeout`. diff --git a/docs/reviews/2026-10-21/07-drivers-plc.md b/docs/reviews/2026-10-21/07-drivers-plc.md new file mode 100644 index 00000000..d5fc13c9 --- /dev/null +++ b/docs/reviews/2026-10-21/07-drivers-plc.md @@ -0,0 +1,277 @@ +# Review 07 — PLC drivers (AbCip, AbLegacy, Ab.Transport, Ab.LibplctagShared, Modbus, S7, TwinCAT, FOCAS, Calculation, Cli) + +Repo: `/Users/dohertj2/Desktop/OtOpcUa` @ `d51d35bd` (master). Production code only. Every finding below was verified by reading the cited lines; sub-reviewer candidates that could not be confirmed were dropped. Paths are relative to `src/Drivers/` unless absolute. + +Legend: **Sev** Critical/High/Medium/Low · **Conf** High/Medium. + +--- + +## 1. Memory / resource leaks + +### L1. Calculation: a script that overruns `RunTimeout` leaks one thread-pool thread per trigger, unbounded +**Sev High · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs:186-187`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/TimedScriptEvaluator.cs:78-81`, `ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationTimerScheduler.cs:66-80` +```csharp +var timed = new TimedScriptEvaluator(evaluator, _runTimeout); +var raw = timed.RunAsync(context).GetAwaiter().GetResult(); // no token passed +// TimedScriptEvaluator: var runTask = Task.Run(() => _inner.RunAsync(context, ct), ct); await runTask.WaitAsync(Timeout, ct) +``` +The timeout *abandons* the `Task.Run` thread (nothing can interrupt a synchronous Roslyn delegate). The timer group's `InFlight` flag is cleared in `finally` once `_evaluate` returns — i.e. as soon as the 2 s timeout fires — so a CPU-bound script (`while(true)`, a large array loop) on a 1 s timer leaks one pool thread per tick, forever; change-triggered tags do the same per input change. Health stays `Healthy` (`CalculationDriver.cs:341`), so nothing alerts until the pool is exhausted and unrelated work (gRPC, other drivers' `Task.Run`) stalls. +**Fix:** track the in-flight runner per tag and refuse to start another while it runs (publish `BadInternalError` "previous evaluation still running" + a counter); pass a `CancellationToken` so awaiting scripts can be cancelled; longer-term, out-of-process runner. + +### L2. Calculation: `ShutdownAsync` never clears compiled scripts — collectible ALCs of every previous generation survive each respawn +**Sev Medium · Conf High** — `CalculationDriver.cs:323-330` (ShutdownAsync), `:719+` (Dispose is the only caller of `ClearCompiledScripts` besides AdoptConfig `:257`), `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs:1550` (PostStop calls only `ShutdownAsync`) +Nothing in the runtime disposes an `IDisposable` driver; every config change is a respawn (#665), so each edit leaves the old generation's ALCs + delegates alive until GC finalises the driver graph — the "accreting ALCs" the `IScriptCacheOwner` contract exists to prevent, just deferred to GC timing. The same applies to the sync `Dispose()` on AbCip/Modbus/FOCAS (`AbCipDriver.cs:1900`, `ModbusDriver.cs:1761`, `FocasDriver.cs:1873`) — dead code in production. +**Fix:** call `_evaluator.ClearCompiledScripts()` from `ShutdownAsync` (idempotent), or dispose `IDisposable` drivers from `PostStop`. + +### L3. Modbus: `ConnectAsync` overwrites a failed `TcpClient` without disposing it; the reconnect backoff loop runs *inside* the transport gate +**Sev Medium · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusSocketLifecycle.cs:84-92, 102-131`, `ModbusTcpTransport.cs:68-101` +```csharp +_client = new TcpClient(target.AddressFamily); // previous failed _client dropped, never Dispose()d +... +if (delay > TimeSpan.Zero) await Task.Delay(delay, ct); // inside SendAsync's _gate; up to 10 attempts, 30 s cap +``` +A connection-refused/reset device makes one poll's `SendAsync` hold `_gate` for ≈ 81 s (0 + 0.2 + 0.4 … 30 s × 10), leaking up to 9 `TcpClient`s per loop (finalizer-reclaimed only). Every other read, every `WriteAsync` (the actor's 5 s write CTS cancels the gate wait → `BadInternalError`) and the probe loop (2 s CTS → host "Stopped") queue behind the one dead socket. `Dns.GetHostAddressesAsync` (`:79`) is outside the per-op `CancelAfter`. +**Fix:** `_client?.Dispose()` before reassignment; cap the in-gate reconnect to one attempt and leave multi-attempt backoff to the driver/watchdog; put DNS under the linked timeout CTS. + +### L4. AbCip native: a deterministic controller refusal other than Unsupported/TypeMismatch re-creates the tag runtime on every poll (lease acquire/release churn; Forward Open/Close churn when it is the device's only tag) +**Sev Medium · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs:1009-1021` (generic catch → `EvictRuntime` + `Degraded`), `:1475-1488` (cold create catch), `AbCipDriverProbe.cs:189` (`IsReachableStatus => Ok or NotFound`), `NativeAbCipTagRuntime.cs:394-401` (Dispose → `ReleaseInBackground`), `ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs:106-138` (last lease closes the connection) +A tag the controller answers `NotFound` (0x04/0x05 — a renamed tag) never gets cached: each poll allocates a runtime, acquires a lease, reads, releases. If it is the only tag on that `DeviceKey`, each release is the last lease → Forward Close + UnRegisterSession, and the next poll TCP-connects + RegisterSession + Forward Opens again (throttled to ≥ 1 s by `ForwardOpenThrottle`). See H1 for the health/backoff side. +**Fix:** cache the runtime with its refusal status (the `TypeMismatch` branch at `:1457-1467` already does exactly this) for `NotFound`/`OutOfBounds`/`NotAllowed`, and treat them as per-tag Bad with no eviction. + +### L6. FOCAS: handle-recycle loop disposes the device client outside `ConnectGate` — orphans a fresh connection or kills a poll mid-read +**Sev Medium · Conf High** (opt-in feature, default off) — `ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs:1665` (`state.DisposeClient()` with no gate), `:2024-2028` (`Client?.Dispose(); Client = null;` non-atomic), `:1849-1850` (`EnsureConnectedAsync` installs under the gate), `:1814` (fast path reads `Client` without it) +Interleaving (a): recycle reads the old client, `EnsureConnected` installs a fresh connected one, recycle nulls it — the fresh client (two TCP sockets) is never disposed. (b): recycle disposes the client a poll is mid-read on → `ObjectDisposedException` → `BadCommunicationError` + Degraded for the rest of that poll, every recycle interval. The driver's own comment says the gate exists because unserialised dispose/install "disposed each other's client". +**Fix:** take `device.ConnectGate` in the recycle loop and dispose/null under it. + +### L7. AbLegacy: per-key `SemaphoreSlim` tables grow with every distinct resolved indirect address (bounded) +**Sev Low · Conf Medium** — `ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs:907-917` (`resolvedName = $"{def.Name}=>{concreteText}"`), `:1109`. Each index value an indirect `N7:[N7:0]` tag sweeps through leaves one semaphore in `_runtimeLocks` and one in `_creationLocks` until `DisposeRuntimes`. Ceiling = file size (≤ 256 words × ≤ 1000 files). + +### L8. TwinCAT lifecycle nits +**Sev Low · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs:234` (`ShutdownAsync` awaits the probe task unbounded while `:197`/`:1445` bound it — a probe wedged in the synchronous `AdsClient.Connect`, `AdsTwinCATClient.cs:80`, hangs the actor's teardown-first reconnect); `AdsTwinCATClient.cs:36-41` (notification channel drops silently at 50k, no counter); `:646-651` (`NotificationRegistration.Dispose` removes from `_notifications` but not the queue, so a callback can fire after unsubscribe); in-place `InitializeAsync` never clears `_tagsByRawPath` (`:147-163`) and `_nativeSubs` keep references to disposed `DeviceState`s (only reachable via a direct re-init with changed config — the runtime respawns, #665). + +### L5. Ab.Transport: per-frame allocations on the hot path (low) +**Sev Low · Conf High** — `Ab.Transport/Eip/EncapsulationClient.cs:283-292` allocates `head` + `frame` per reply; `Wire/CipWriter.cs` allocates a 256-byte buffer + `ToArray()` per request; `Logix/LogixTagClient.cs:62-63` builds a `List` then `ToArray()`. Modbus transports likewise allocate PDU/ADU/response arrays per request (`ModbusTcpTransport.cs`, `ModbusRtuOverTcpTransport.cs`), and `NormalizeWordOrder` copies even for BigEndian. At PLC poll rates this is Gen0 noise; only matters at thousands of PDUs/s. Not a defect. + +--- + +## 2. Unimplemented / stubbed + +### U1. Modbus: `Family`, `MelsecSubFamily`, `DisableFC23` are authored in the AdminUI but nothing consumes them; the whole `Driver.Modbus.Addressing` project has no production caller +**Sev Medium · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs:87-93`, `ModbusDriverOptions.cs:91-97` (DisableFC23 documented no-op), `ModbusTagDefinitionFactory.cs:34-95` (reads numeric `address` + `region` only); `grep` finds zero references to `ModbusAddressParser`/`ParsedModbusAddress`/`DirectLogicAddress`/`MelsecAddress` outside the Addressing assembly. +The "Family" dropdown (DL205 octal V-memory, MELSEC hex X/Y) changes nothing at runtime; an operator who picks MELSEC and authors `D100` gets `BadNodeIdUnknown`. The only region/type cross-check (`ModbusAddressParser.cs:176-180`) is therefore never run (see H5). +**Fix:** route the editor's address field through `ModbusAddressParser.Parse(text, Family, SubFamily)` and persist the resolved region/offset, or remove the three fields until they do something. + +### U2. Modbus: block-read coalescing is OFF by default and never covers coils, discrete inputs, `BitInRegister` or `String` +**Sev Medium · Conf High** — `ModbusDriver.cs:476-478`, `:904-913`; `ModbusDriverOptions.cs:117` (`MaxReadGap = 0` = disabled) +```csharp +var coalesced = _options.MaxReadGap > 0 ? await ReadCoalescedAsync(...) : new HashSet(); +if (tag.Region is not (HoldingRegisters or InputRegisters)) continue; // coils/DI never coalesce +if (tag.DataType is String or BitInRegister) continue; +``` +Default-configured device with 200 holding registers = 200 FC03 round-trips per poll; 16 `BitInRegister` tags on one status word = 16 reads of the *same* register; 64 adjacent coils = 64 FC01s. `0` means "disabled" rather than "adjacent only". +**Fix:** treat `MaxReadGap >= 0` as enabled with gap 0 = adjacent, default ~8; add FC01/FC02 bit-block coalescing; fold same-register `BitInRegister` tags into one read. + +### U3. Modbus: `String[]` and `BitInRegister[]` tags read fine and are advertised Operate-writable, but every write fails `BadInternalError` +**Sev Low · Conf High** — `ModbusDriver.cs:655-673` (decode supports both), `:1177-1182, 1295-1301, 1314` (`ElementByteCount` throws for String; `Convert.ToBoolean(bool[])` → `InvalidCastException`). **Fix:** refuse at `FromTagConfig` or return `BadNotSupported`. + +### U4. S7: Bool arrays ignore the parsed bit offset +**Sev Low · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs:1340-1356, 1643-1649`. `ReadArrayAsync` reads `count` bytes at `ByteOffset`; `DecodeArrayBlock` takes `block[i] & 0x01`. `DB1.DBX0.3` with `arrayLength: 8` returns bit 0 of bytes 0..7, silently. **Fix:** reject non-zero bit offsets on Bool arrays at authoring/init, or decode bit-packed. + +### U5. Calculation: a change-triggered tag with zero literal `ctx.GetTag` reads never evaluates and nothing warns +**Sev Medium · Conf High** — `CalculationDriver.cs:178-186` (`_changeDependents` from `DependencyRefs` only), `:354-359`; `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs:395-456` (only Missing/NotFound/Cycle). `ctx.GetTag("Plant/" + line + "/Temp")` or `return ctx.Now.Hour;` with the default `changeTriggered:true` stays `BadWaitingForInitialData` forever while the driver reports Healthy. **Fix:** deploy-gate `CalculationNoEffectiveTrigger` when change-triggered && deps.Count == 0 && no timer; or evaluate once at init. + +### U6. AbLegacy native: PLC-5 writes are not shipped, PLC-5 reads are N-file only, PLC-5 sub-elements refused (documented, surfaced correctly) +**Sev Low · Conf High** — `Ab.Transport/Pccc/PcccServices.cs:16-19` (`Plc5WriteVerified = false`), `:112-114`; `ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs:150, 261-263`. All surface as `AbStatus.Unsupported` → `BadNotSupported` with no health impact, and `Native` is opt-in for AbLegacy. Noted as a capability gap, not a defect. + +### U8. FOCAS: `FocasDataType.String` is authorable but every read path publishes a number at Good +**Sev Medium · Conf High** — editor lists every enum member (`AdminUI/Components/Shared/Uns/TagEditors/FocasTagConfigEditor.razor:18`); `ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasConstants.cs:116` (`_ => FocasPmcDataType.Byte`), `FocasPmcCoalescer.cs:135` (`_ => (object)raw`, a `long`), `Wire/WireFocasClient.cs:392-393` (PARAM default reads Int32), `:456-458` (MACRO returns `double`). A String-typed OPC UA node receives a boxed `long`/`int`/`double` at Good. **Fix:** reject `String` in `FocasTagDefinitionFactory.FromTagConfig`/capability matrix and drop it from the editor. + +### U9. TwinCAT: array writes unimplemented but advertised writable; string writes bypass the codec +**Sev Medium · Conf High** — `TwinCATDriver.cs:521-524` (`def.ArrayLength` ignored), `AdsTwinCATClient.cs:908-925` (`ConvertForWrite` has no array case → `Convert.ToInt32(object)` → `InvalidCastException` → reported `BadTypeMismatch` at `:532-535` not `BadNotSupported`), `:586-590` (`DiscoverAsync` advertises arrays `Operate`); strings go to `_client.WriteValueAsync(symbolPath, string)` (`:472-473`) with no length check against `STRING(n)` and host-ANSI encoding the read side avoids (`TwinCATStringCodec.cs:7-10`). **Fix:** implement raw-bytes array/string writes mirroring the codecs, or refuse `BadNotSupported` and advertise `ViewOnly`. + +### U10. AbCip/AbLegacy: array writes — config/UI allow them, both drivers encode a scalar +**Sev Medium · Conf Medium** — `AbCipDriver.cs:1191-1255` (no array branch; `WriteWidthRefusal` skips arrays `:1739`), `NativeAbCipTagRuntime.cs:319-384` (`Convert.ToInt32(value)`), `:236` (`client.WriteAsync(path, type, null, 1, buffer)`); `LibplctagTagRuntime.cs:228-290`; `NativeAbLegacyTagRuntime.cs:287-302, 418-449`. An `int[]` → `InvalidCastException` → `BadTypeMismatch` **and** Degraded (`:1281-1288`); a scalar to an array tag writes element 0 natively, while libplctag `SetInt32(0,…)` resends the whole last-read buffer (stale elements 1..n-1). Medium confidence only because the server-side array-write gate was not traced end to end. **Fix:** implement array encode (elements = count) or refuse `IsArrayTag(def)` writes with `BadNotSupported` before any runtime exists. + +### U11. AbCip: BOOL-array element reads decode "low byte of the DWORD ≠ 0", not the addressed bit +**Sev Medium · Conf Medium** — `NativeAbCipTagRuntime.cs:440-442` (`IsTypeMismatch` exempts authored Bool answered `0xD3`), `:279` (`Bool => bitIndex is int bit ? GetBit(bit) : Span(offset, 1)[0] != 0`), `LibplctagTagRuntime.cs:121-123` (`GetInt8(offset) != 0`); `AbCipDriver.cs:956-958` (width check skipped for Bool). `BoolArr[n]` reads Good with "any of bits 0-7 set", subscript ignored; the write side already refuses this shape (`WriteWidthRefusal` → `BadNotSupported`). No capture/test covers which DWORD the controller returns. **Fix:** decode bit `n % 32` of element `n / 32`, or return `BadTypeMismatch` as the write path does. + +### U12. TwinCAT native subscribe silently drops unresolvable references +**Sev Low · Conf High** — `TwinCATDriver.cs:669-670` (`continue` on resolve/device miss) vs the poll path's `BadNodeIdUnknown` (`:282-290`). The node keeps whatever status the server seeded. **Fix:** push a `BadNodeIdUnknown` snapshot before `continue`. + +### U7. CLI gaps (low) +**Sev Low · Conf High** — `Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:60-66` writes from the poll thread with no lock/try-catch (every other CLI does); AbCip/AbLegacy `probe`/`read`/`write` print a stack trace on Ctrl-C (no `OperationCanceledException` arm); `Driver.Cli.Common/SnapshotFormatter.cs:107-114` renders arrays as `System.Int32[]`. + +--- + +## 3. Performance limiters + +### P1. S7: a read pass keeps issuing wire ops on a handle already marked dead; each pays the full 5 s deadline, and health is written only at the end of the pass +**Sev High · Conf High** — `S7Driver.cs:707-716` (span loop), `:741-765` (per-tag loop), `:758, 1037, 1156` (`_plcDead = true` set), `:2310-2313` (`MarkConnectionDeadIfFatal`), `:787-800` (single health write in `finally`) +No loop checks `_plcDead` between items. On a frozen-but-established peer (cable pull, firewall DROP — the case `S7OperationDeadline` exists for) the first span times out after `Timeout` and marks the handle dead; the remaining N spans/tags each still go to the wire and each wait another 5 s. A 20-span pass ≈ 100 s under `_gate`, and `Degraded` is not visible until the pass ends — so the #527 watchdog's 90 s clock starts only then: detection ≈ pass length + 90 s. The probe loop cannot help (same gate). +**Fix:** once `_plcDead` flips inside a pass, stamp the remaining items `BadCommunicationError` without I/O, record the failure, exit the pass. + +### P2. S7: one bad member fails its entire span every poll — no per-tag fallback (the planner's stated invariant is false) +**Sev High · Conf High** — `S7Driver.cs:1138-1165`; `S7ReadSpanPlanner.cs:11-16` +```csharp +catch (Exception ex) { var status = RecordReadFailure(ex, outcome); + foreach (var member in span.Members) results[member.OriginalIndex] = new DataValueSnapshot(null, status, null, now); return; } +``` +A span's block is the union of its members' ranges; if one member runs past the DB's actual length or over-declares `stringLength` (width = `StringLength + 2`, `:1430`), the CPU refuses the whole block and every neighbour within the 220-byte span is `BadDeviceFailure` + Degraded on every poll. The multi-var path (`:1040-1047`) and Modbus's coalescer (`ModbusDriver.cs:980-984`, auto-prohibit + per-tag fallback) both fall back; the span path does not. +**Fix:** on a non-connection-fatal span `PlcException`, re-read members individually for that pass and remember the span as "do not group". + +### P3. Modbus: transport never self-heals after a second consecutive socket failure — every later call fails fast until the 90 s watchdog tears down all subscriptions +**Sev High · Conf High** — `ModbusTcpTransport.cs:66, 87-97`; `ModbusRtuOverTcpTransport.cs:94, 116-127`; `ModbusSocketLifecycle.cs:147-154` +```csharp +if (_life.Stream is null) throw new InvalidOperationException("Transport not connected"); // :66, BEFORE the gate and the retry arm +catch (Exception ex) when (_life.AutoReconnect && ModbusSocketLifecycle.IsSocketLevelFailure(ex)) // :87, single retry +``` +A per-op timeout tears the socket down; the one retry reconnects and resends; if that also fails, `_stream` stays null and `:66` throws before `:87` can ever reconnect. Recovery is exclusively the comms-loss watchdog (Degraded + 90 s frozen) → `ShutdownAsync` → `_poll` disposed (all subscriptions) → re-init → re-subscribe. Two lost replies in a row from a slow device cost ≥ 90 s of Bad data plus full subscription churn. S7 reopens lazily on the next call. +**Fix:** when `_life.Stream is null && AutoReconnect`, attempt a backoff-throttled connect inside the gate instead of throwing. + +### P4. Calculation: a script with a compile error is re-fed to Roslyn on every trigger, under the driver-wide `_evalGate`, and floods both logs +**Sev High · Conf High** — `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/CompiledScriptCache.cs:71-80` (faulted `Lazy` evicted), `CalculationEvaluator.cs:158-161`, `CalculationDriver.cs:518` (`lock (_evalGate)` around the run), `:596` (`PublishScriptLog` on every failure) +Source never changes between triggers (fixed in the artifact), so each trigger of a broken tag is a full `CSharpCompilation.Create` + diagnostics serialised against every other calc tag on the driver; one broken tag on a 250 ms input starves the rest. The node publish is correctly once-per-transition, but the host Warning and script-log entry fire every time. +**Fix:** negative-cache compile failures by source hash (`ClearCompiledScripts` already drops it on redeploy); gate the Warning/script-log on the Good→Bad transition. + +### P5. Calculation: sync-over-async on a pool thread with a nested `Task.Run` manufactures false timeouts under load +**Sev Medium · Conf High** — `CalculationDriver.cs:494` (pump is `Task.Run`), `CalculationTimerScheduler.cs:53` (Timer callbacks are pool threads), `CalculationEvaluator.cs:187`. The `WaitAsync` clock starts before the inner item is scheduled, so pool starvation (which L1 causes) is charged to the script: "script timed out after 2.0s" → `BadInternalError` for a script that never ran. **Fix:** make the pump async and `await timed.RunAsync`, or run inline with a watchdog. + +### P7. AbLegacy: reads are strictly serial with no per-device backoff — one dead device stalls every device in the driver and can trip the watchdog against a healthy one +**Sev High · Conf High** — `ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs:375-479` (plain `for` + `await runtime.ReadAsync` at `:425`), `:443, 474` (failed read evicts the handle), `:1101-1128` (`EnsureTagRuntimeAsync` re-inits with a full `Timeout` each), no `ConnectionBackoff`/`ConnectGate` (grep: none); `NativeAbLegacyTagRuntime.cs:127` (`AllowPacking = false`) +The host subscribes all of a driver's refs in one `PollGroupEngine` loop (`DriverHostActor.cs:3631`). With device B down and 50 tags, each poll pays 50 × `Timeout` (2 s) serially ≈ 100 s; device A's tags are read once per ~100 s. Per-tag health writes (`:446/462/477`) flip Degraded/Healthy and `LastSuccessfulRead` only advances on an A success, so a cycle > 90 s trips the comms-loss watchdog against *reachable* device A, and the re-init restarts the same cycle. Native transport does not help (no MSP packing; adjacent N/F/B words never coalesced although the array path proves the transport can). AbCip has a milder form: cold waiters hold a window slot while queued on `ConnectGate` (`AbCipDriver.cs:797 → 1431`), so a dead device's backoff-expiry attempt can occupy all 8 slots for `Timeout` once per ≤ 30 s. +**Fix:** per-device `ConnectionBackoff` + single-flight connect gate (copy AbCip), bounded-parallel window across devices, adjacent-element coalescing per device. + +### P8. AbCip native: `MaxConcurrentReads` (default 8) caps Multiple Service Packet coalescing at 8 services per round trip +**Sev Medium · Conf High** — `AbCipDriver.cs:779-784, 797` (window slot taken before `runtime.ReadAsync` → `scheduler.SendAsync`), `AbCipDriverOptions.cs:121` (`= 8`, documented for libplctag per-handle concurrency); `Ab.Transport/Cip/RequestScheduler.cs` `TakeBatch`/`PumpAsync` pack only what is queued when the pump wakes. A 4002-byte Large Forward Open can carry ~100+ DINT reads per packet (the scheduler budgets request + reply bytes), so a 1000-tag CompactLogix pays ~125 round trips per poll instead of ~10. +**Fix:** on `AbCipTransport.Native` make the window effectively unbounded (≥ 64) — the scheduler's reply budget already prevents over-packing. + +### P9. FOCAS: one slow CNC stalls every device in the subscription; the per-device gate is shared by three loops +**Sev Medium · Conf High** — `FocasDriver.cs:565-651` (sequential walk of the whole batch), `SynchronizedFocasClient.cs:143-156` (gate shared with the fixed-tree loop — 9-block `cnc_rddynamic2` per axis every 250 ms — and the alarm loop). A dead device is cheap (backoff → `ConnectionThrottledException`), but a *slow* device (TCP-accept-then-stall, the live-observed 31i-B case) costs `Timeout` (2 s) per tag per poll during which no other device is read. Per-poll nit: `TryReadFixedTree` builds `HostAddress + "/"` for every device for every reference (`:1515`), run twice per poll (`:681`, `:577`). +**Fix:** bucket by device and read devices concurrently (`Task.WhenAll`, each serialised by its own gate); precompute the prefix on `DeviceState`. + +### P10. TwinCAT: a Warning per bad tag per poll with no once-per-tag dedup +**Sev Medium · Conf High** — `TwinCATDriver.cs:353-362` (`CompleteRead` logs `LogWarning` for every non-Good status every poll). 50 typo'd symbols on a 500 ms poll = 100 warnings/s indefinitely (AbCip and FOCAS warn once). **Fix:** a "warned" set keyed by reference, cleared on Good or re-init. + +### P11. AbCip: UDT read planner recomputes member layout per member per poll +**Sev Low · Conf High** — `AbCipUdtReadPlanner.cs:51-83` (`AbCipUdtMemberLayout.TryBuild(parent.Members)` at `:70` for every member reference every poll — O(members²) per parent per tick, allocating a dictionary each time); `AbCipDriver.cs:780-784` allocates a `SemaphoreSlim`, `Task[]`, `ReadOutcome[]` + closures per poll. Only material with `EnableDeclarationOnlyUdtGrouping` on and large groups. **Fix:** cache the layout per parent definition. + +### P6. Modbus: region/type compatibility is not enforced — with coalescing ON, one `HoldingRegisters`+`Bool` tag kills the whole subscription's poll cycle every tick +**Sev Medium · Conf High** — `ModbusDriver.cs:931` (`RegisterCount(tag)` outside any try), `:1429-1436` (`throw new InvalidOperationException($"Non-register data type …")`), `ModbusTagDefinitionFactory.cs:52-65` (no cross-check). `RegisterCount` throws out of `ReadCoalescedAsync` → out of the reader → `PollGroupEngine` returns no snapshots for any tag and backs off to 30 s, forever. With coalescing off the same tag reads `BadCommunicationError` + Degraded. Conversely `Coils` + `Int16` silently returns a `bool` into an Int32 node. +**Fix:** reject region/type mismatches in `FromTagConfig`; wrap the per-tag planner math so a bad tag goes to the per-tag path. + +--- + +## 4. Other hazards + +### H1. AbCip: deterministic controller refusals (NotFound / OutOfBounds / NotAllowed / unmapped status) latch `Degraded`, and the non-NotFound ones open the device-wide 1→30 s backoff window that fails every sibling cold create +**Sev Medium · Conf High** — `AbCipDriver.cs:1009-1021` (generic catch → `ReadOutcome.Degraded`), `:1442-1444` (`ConnectionThrottledException` for siblings), `:1475-1488` (`RecordFailure` unless `IsReachableStatus`), `AbCipDriverProbe.cs:189` (`Ok or NotFound` only), `:1937` (`new ConnectionBackoff(1 s, 30 s)`); `Ab.Transport/AbStatusCodes.cs:9-23` (0x1C/0x20/0xFF-0x2104… → `BadReply` → `BadCommunicationError`) +`drivers.md` says only Unsupported/TypeMismatch are "refusals, not comms faults". A tag authored `Arr[999]` (0xFF/0x2105 → `OutOfBounds`) is answered by a working session but records a *Forward-Open failure*, so any sibling needing a cold create (new subscription, or evicted after a transient) fails fast for up to 30 s with `BadCommunicationError`. Health latches Degraded while the tag exists; if it is the device's only tag, `LastSuccessfulRead` freezes and the #527 watchdog re-inits the driver every 90 s against a reachable controller. +**Fix:** widen `IsReachableStatus` to every status that proves the controller answered (`NotFound, OutOfBounds, NotAllowed, TooLarge/TooSmall, TypeMismatch, Unsupported`); treat them as per-tag Bad with no health change (as the TypeMismatch branch does); map 0x1C/0x20/0xFF-0x2104 explicitly instead of to `BadReply`. + +### H2. Modbus: deadband swallows the Good-after-Bad recovery publish (value-only dedup — the exact #527 trap `drivers.md` warns about) +**Sev High · Conf High** — `ModbusDriver.cs:236-259` +```csharp +if (snapshot.Value is null) return true; // Bad passes through, baseline NOT touched +... +if (Math.Abs(newD - oldD) < tag.Deadband.Value) return false; // status never consulted +``` +Tag publishes 10.0 (baseline 10.0) → comms drop → Bad published, baseline kept → device returns holding 10.0 → engine fires onChange (status differs) → `|10−10| < deadband` → **false**: the Good recovery is never published and the node stays Bad until the value drifts past the deadband. Deadband is only authorable via raw TagConfig JSON today (`ModbusTagDefinitionFactory.cs:80-82`), which narrows exposure, not the defect. +**Fix:** drop the baseline (`_lastPublishedByRef.TryRemove`) when returning true for a null/Bad snapshot, or baseline on `(value, status)`. + +### H3. S7: one mis-authored tag faults the whole driver at init and the actor retries the same init every 10 s forever; neither the editor nor the deploy gate catches the throwing cases +**Sev High · Conf High** — `S7Driver.cs:270` (`RejectUnsupportedTagConfigs()` throws), `:313-316` (`S7AddressParser.Parse` throws `FormatException`), `:336-348` (catch → `Faulted` → rethrow); `S7TagDefinitionFactory.cs:59-64` accepts any non-blank address; `DriverInstanceActor.cs:743` retries. The comment at `:245-246` says "one bad tag must not fail the whole driver init". A typo'd `DB1.DBZ0`, `DB1.DBW0` typed `Float64`, or `T5` typed `Int32` darkens every tag on that PLC. Modbus skips the tag (`ModbusDriver.cs:337-345`). CSV import / API blobs bypass the editor entirely. +**Fix:** convert init guards + parse pass into per-tag skips (Modbus pattern) and run `S7AddressParser.TryParse` + guards inside `FromTagConfig`/`Inspect` so the deploy gate refuses them. + +### H4. Modbus and S7: protocol refusals degrade driver health; all-tags-refused produces a 90 s reconnect/resubscribe loop +**Sev Medium · Conf High** — `ModbusDriver.cs:507-511` (`ModbusException` 01/02/03 → `Degraded`), `:492` vs `:510` (health is last-writer-wins per tag, so it flaps with tag order — `drivers.md`'s "latches Degraded forever" is wrong); `S7Driver.cs:1278-1285` (`PlcException` incl. `ReadData` bad DB/address → `Degraded`). When every tag of a driver refuses (wrong DB number, wrong register map, gateway 0x0B for an offline RS-485 slave), `LastSuccessfulRead` freezes AND health is Degraded → the #527 conjunction trips every 90 s against a perfectly reachable device. +**Fix:** map Modbus 01/02/03 and S7 non-fatal `ReadData`/`WriteData` to per-tag Bad + once-per-tag Warning with no health write (the AbCip rule); keep 04/05/06/0A/0B and connection-fatal codes on Degraded. + +### H5. S7: no driver-side lifecycle gate — overlapping `InitializeAsync` calls can leak an opened PLC connection and orphan a probe loop +**Sev Medium · Conf Medium** — `S7Driver.cs:226-349` (no gate around the body; only `_gate` at `:150` for I/O), `:304` (`Plc = plc`), `:328-333` (`_probeCts/_probeTask` assigned); `DriverInstanceActor.cs:743, 807-855` (`RetryConnect` every 10 s with `_initGeneration` dropping only the *result*). `ShutdownAsync` drains (≤ 5 s poll + ≤ 5 s probe + ≤ 5 s gate) + connect (≤ 5 s) can exceed 10 s, so attempt B starts while A is in `OpenAsync`; A's `Plc` and probe loop are then never released (only the latest `_probeCts` is cancelled). Modbus has `_lifecycleGate` (`ModbusDriver.cs:91-101`) for exactly this. +**Fix:** add the Modbus `_lifecycleGate` pattern around Initialize/Shutdown/Reinitialize. + +### H6. Calculation: a driver Restart leaves every calc tag `BadWaitingForInitialData` until each input happens to change +**Sev Medium · Conf High** — `CalculationDriver.cs:87-88, 354, 475`; `DriverHostActor.cs:3811-3826` (Restart = new instance, only the restarted child re-subscribes); `DependencyMuxActor.cs:43-88` (no last-value replay). Deploy is fine (poll drivers `forceRaise` on re-subscribe); Restart has no re-push, and values arriving before `_running` are dropped (`:354`). **Fix:** mux retains last `(value,status,ts)` per ref and replays on `RegisterInterest`; record inputs regardless of `_running`. + +### H10. AbCip: a missing/typo'd probe tag turns into a permanent device blackout — the probe loop records every exception as a Forward-Open failure +**Sev High · Conf High** — `AbCipDriver.cs:666-679` (`backoffVerdict = ex is AbCipTagConfigurationException ? null : false;` — every other exception incl. `AbTransportException(NotFound)`), `:700-701` (`RecordFailure` every `Probe.Interval` 5 s), `:1442-1444` (data path fails fast while the window is open), `:1482-1485` (the data path *does* classify `NotFound` as reachable — the probe does not), `AbCipDriverProbe.cs:146-151, 164-167` (Test Connect reports the same situation green: "controller reachable; probe tag not found") +With `RecordFailure` every 5 s, `ComputeDelay` saturates at 30 s so `_nextAttemptUtc` is always ≥ 25 s ahead and `ShouldAttempt` is **never** true: every cold runtime create throws `ConnectionThrottledException` → `BadCommunicationError` + Degraded for every tag not already hot, and after any single eviction a tag never comes back. Scenario: `Probe.ProbeTagPath` typo, or the probe tag deleted by a PLC download. Host `Stopped`, driver Degraded, all new/evicted tags dead indefinitely; a node restart does not help. +**Fix:** classify `ate.Status` with `IsReachableStatus` in the probe catch — `NotFound` ⇒ verdict `true` (the session answered), one config Warning, and decide `HostState` separately from the backoff verdict. + +### H11. TwinCAT native subscribe is all-or-nothing: one bad symbol or one unreachable device yields zero data for the whole driver, with register/delete churn every 30 s and health still `Healthy` +**Sev High · Conf High** — `TwinCATDriver.cs:690-706` (`TwinCATNotificationRefusedException` is the *only* per-tag catch), `:710-721` (any other exception: dispose every registration + rethrow); `AdsTwinCATClient.cs:540-542` (unknown symbol → `InvalidOperationException`), `:584-586`, `:1214-1216`/`:1240-1248` (device offline / backoff → connect exception); `TwinCATDriverOptions.cs:43` (`UseNativeNotifications = true` default) +The host bulk-subscribes every tag in one call, so one typo'd `symbolPath` or one dead PLC in a multi-device driver tears down every notification already registered and fails the subscription; the health-tick reconcile retries every 30 s — a 2,000-tag driver issues 2,000 `AddDeviceNotification` + 2,000 `DeleteDeviceNotification` per 30 s forever, serving nothing, with `_health` untouched so the watchdog never fires. The poll fallback handles the same inputs per tag. +**Fix:** catch `Exception` around `RegisterNotificationAsync`: one-off `BadNodeIdUnknown`/`BadCommunicationError` snapshot, store the intent as a dead registration so `RetryDeadRegistrationsAsync` picks it up; short-circuit a device-level connect failure for the rest of that device's tags. + +### H12. TwinCAT native mode: `LastSuccessfulRead` never advances, so one Degraded write latches a full driver reconnect; a router-level failure never degrades at all +**Sev High · Conf High** — `TwinCATDriver.cs:681-683` (notification callback publishes Good without touching `_health`), `:889-943` (probe only moves `HostState`), `:176` (`LastSuccessfulRead` = init stamp); debits: `:540-544` (any write exception incl. throttled connect → Degraded), `:527-531` (`NotSupportedException` write → Degraded — the #611 "refusal is not a comms fault" case), `:1294-1310` (replay failure, incl. a correctly-isolated type-mismatch tag re-attempted on every reconnect); `DriverInstanceActor.cs:637-646` (silence measured from `max(LastSuccessfulRead, _connectedSinceUtc)`) +On a connection older than 90 s, the first health tick after one Degraded write self-sends `DisconnectObserved` → `ShutdownAsync` + re-init + resubscribe: every ADS notification on every device deleted and re-registered because one write timed out. Inverse: an unreachable AMS router (`client.ConnectAsync` throws `:1234-1248`) is caught by the probe (`:920-928`) and never writes `_health` — values stay stale-Good, driver `Healthy` indefinitely. +**Fix:** stamp `LastSuccessfulRead` (and restore Healthy) from the notification dispatch path (throttled `Volatile.Write` per batch) and from a successful probe; write Degraded with `LastSuccessfulRead` preserved after N consecutive probe/connect failures; drop the health write from the `NotSupportedException` branch. + +### H13. FOCAS: `_devices` is a plain `Dictionary` cleared and refilled in place under a live poll loop, the fixed-tree loop, the alarm loop and the actor health tick +**Sev High · Conf High** — `FocasDriver.cs:46` (`Dictionary`), `:412` (`_devices.Clear()` in `ReleaseDeviceStatesAsync`), `:225` (refill in `InitializeAsync`); readers `:595` (`ReadAsync` TryGetValue), `:1513` (`foreach (var state in _devices.Values)` — "Collection was modified"), `:1145` (`GetHostStatuses` from the actor), `:1779` (alarm loop). The driver explicitly keeps the poll engine alive across re-init (`:49-50`) and build-and-swaps the *tag* tables for that reason, but not `_devices`. TwinCAT uses `ConcurrentDictionary` (`TwinCATDriver.cs:31-34`). Same class, lower exposure: AbCip `_devices`/`_devicesByRoutingKey` (`AbCipDriver.cs:101, 106`, cleared `:529-530` vs read `:901` outside the try) and AbLegacy `_devices` + `_tagsByRawPath` (`AbLegacyDriver.cs:31, 35`, `.Clear()` at `:259, 343` while the resolver reads it) — AbCip already swaps `_tagsByRawPath` by reference (`:532`) for exactly this reason. +**Fix:** `ConcurrentDictionary`, or build-and-`Volatile.Write`-swap like `_tagsByRawPath`. + +### H14. FOCAS: macro/parameter numbers are narrowed to `short` after the capability matrix admitted up to 99999 +**Sev High · Conf High** — `Wire/WireFocasClient.cs:408` (`_wire.ReadMacroAsync((short)address.Number, …)`), `:377-393` (parameters likewise); `FocasCapabilityMatrix.cs:59-65` (30i/31i/32i macro range `(0, 99999)`; `Unknown` → `int.MaxValue`); the wire format already carries `Int32` (`FocasWireProtocol.cs:285-288`). `MACRO:99999` → `(short)99999 == -31073`: the CNC is asked for a different variable and the result is published Good under the authored tag. +**Fix:** widen the adapter signatures to `int`, or reject `Number > short.MaxValue` at init into the skipped-tag ledger. + +### H15. FOCAS: a driver serving only `FIXED:` tags never leaves `Healthy` when its CNC disappears +**Sev Medium · Conf High** — `FocasDriver.cs:1243-1250, 1385-1391` (fixed-tree loop swallows every failure at Debug, never touches `_health`), `:1181-1185` (probe likewise), `:1544-1546` (deliberately does not credit), `:611-621` (`ReadAsync` returns the fixed-tree snapshot without touching health), `:1529-1530` (values go Bad after `FixedTreeStaleAfter` ≥ 30 s). `/hosts` shows a healthy driver with every node Bad; the #527 watchdog can never fire (the loops do self-reconnect, so recovery works — the health surface lies). Identity/Axes/Program/Status are the typical FOCAS deployment shape. +**Fix:** after a successful axis pass write Healthy + `LastSuccessfulRead`; after a failed tick past `FixedTreeStaleAfter` write Degraded preserving it. + +### H16. AbLegacy: a deterministic refusal (array of String/Timer/Counter/Control) latches Degraded and churns handles every poll +**Sev Medium · Conf High** — `AbLegacyDriver.cs:471-478` (generic catch → `EvictRuntime` + `BadCommunicationError` + Degraded), `:597-601` (write path same), `NativeAbLegacyTagRuntime.cs:411-414` / `LibplctagLegacyTagRuntime.cs:200-206` (`DecodeArray` throws `NotSupportedException`; comment "the driver rejects them before reaching here" has no corresponding check); `AbLegacyTagDefinitionFactory.cs:73-79` accepts `isArray:true` with any `dataType`. Every poll: create handle (one init read), decode throws, dispose, Bad, Degraded. **Fix:** catch `NotSupportedException` → `BadNotSupported`, keep the handle, no health write; reject the combination in `FromTagConfig`/`Validate`. + +### H17. AbCip: bit-in-word write hardcodes a DINT parent; INT/SINT hosts fail as comms faults +**Sev Medium · Conf High** — `AbCipDriver.cs:1358, 1363` (`DecodeValue(AbCipDataType.DInt)` / `EncodeValue(DInt)` on the parent), `:1385` (parent runtime created with no `AuthoredType`). Logix allows `.n` on SINT/INT/DINT; reads of `Flags.3` on an INT host work, but the write's `DecodeValue(DInt)` on a 2-byte buffer throws `OutOfBounds` → generic catch `:1322-1331` → `EvictRuntime` (the bit tag's, not the parent's), `BadCommunicationError`, Degraded. **Fix:** pick the parent width from `GetSize()`/CIP type code; map mismatch to `BadTypeMismatch` with no health write. + +### H18. TwinCAT: BOOL-within-word write is an RMW at a hardcoded 32-bit width with no parent check +**Sev Medium · Conf Medium** — `TwinCATDriver.cs:492-511` (reads/writes the parent as `UDInt`); the notification path validates the bit against the real width (`AdsTwinCATClient.cs:522-531`, `TwinCATBitAccess.CheckParent`) but the write path does neither. A bit beyond a 16-bit parent is written into neighbouring bytes or rejected; a signed parent with the sign bit set reads back ≥ 2³¹ and a checked conversion → `OverflowException` → `BadOutOfRange` for *any* bit write. Writes were outside the TwinCAT live gate. **Fix:** resolve the parent via `DescribeSymbolAsync`, `CheckParent`, RMW on raw bytes of `parent.ByteSize`. + +### H19. AbCip write-side health: client conversion errors latch Degraded; a good write overwrites Degraded with Healthy +**Sev Medium · Conf High** — `AbCipDriver.cs:1272-1296` (`FormatException`/`InvalidCastException`/`OverflowException` → Degraded; nothing touched the wire), `:1186-1187, 1254` (`Healthy` on success, no worst-wins — undoing a Degraded the poll loop just recorded; the batch read path's `ApplyBatchHealth :858-882` exists to avoid exactly this). AbLegacy maps the same three to `BadTypeMismatch`/`BadOutOfRange` with no health write (`:602-609`). Operator surface only (`LastSuccessfulRead` still advances on reads). **Fix:** drop the health writes from those catches and the success branches. + +### H20. FOCAS wire: `ParseAlarms` layout heuristic is ambiguous at 19 legacy records +**Sev Low · Conf High** — `Wire/FocasWireClient.cs:1114-1121` picks the 76-byte layout whenever `Length % 76 == 0`; a legacy 80-byte reply of exactly 19 records (1,520 B) also satisfies it and decodes as 20 garbage vendor records. + +### H21. AbCip `_health` is not `volatile` (sibling says it is load-bearing) +**Sev Low · Conf High** — `AbCipDriver.cs:136` vs `AbLegacyDriver.cs:41-46` (whose comment explains why the actor-thread reader needs the barrier). Reference assignment is atomic and the reader crosses method calls, so fine in practice. + +### H7. Modbus: `WriteAsync` maps transport failures to `BadInternalError`; S7 probe reports the host `Stopped` on gate contention +**Sev Low · Conf High** — `ModbusDriver.cs:1135-1146` (only `InvalidDataException` → `BadCommunicationError`; desync `IOException`, `SocketException`, timeout OCE → `BadInternalError`; encode `OverflowException` → `BadInternalError` not `BadOutOfRange`); `S7Driver.cs:2499-2546` (2 s `gateCts` around `_gate.WaitAsync` → `TransitionTo(Stopped)` on a long read pass), `S7DriverProbe.cs:74-76` ignores config `TimeoutMs`. + +### H8. Calculation: array-valued results never dedup +**Sev Low · Conf High** — `CalculationDriver.cs:615` `Equals(st.LastValue, value)` is reference equality for arrays: a `double[]`-returning timer script republishes every tick through the dual-namespace fan-out and the historian. **Fix:** `StructuralComparisons.StructuralEqualityComparer` for `Array`. + +### H9. Modbus: `SendOnceAsync` indexes the response PDU without a length check +**Sev Low · Conf High** — `ModbusTcpTransport.cs:157-171`. `respLen == 1` → `respPdu[0]` throws `IndexOutOfRangeException` (not an `IOException`, so no teardown/retry) → `BadCommunicationError` + Degraded for a malformed reply. **Fix:** treat `respLen < 2` (`< 3` for exception frames) as a `TruncatedFrame` desync. + +--- + +## 5. Checked and fine (do not re-chase) + +**Ab.Transport (read in full by me):** +- `EncapsulationClient`: single-flight gate; caller cancel never faults the socket; every timeout finite (`ValidateTimeout`); `Fault()` closes the socket; `DisposeAsync` UnRegisters with a 500 ms budget; `_gate` deliberately never disposed (abandoned exchange may release late). +- `CipConnection`: one shared open with `OpenBudget`; predecessor-closed chaining; one-shot keep-alive timer re-armed after each tick; Forward Close on dispose (1 s budget); `RetireClosedByTargetAsync` on 0x0107/0x0203 so a target-dropped class-3 connection reopens. +- `RequestScheduler`: one pump per device (no cross-device stall); waiter tokens never reach the wire; MSP packing budgeted on request **and** expected-reply bytes; cancellation registrations disposed exactly once. +- `DeviceSessionPool`: ref-counted leases, close chain prevents overlapping connections; `ForwardOpenThrottle` ≥ 1 s per device, never pruned (bounded by device count, documented). +- `LogixTagClient`: fragmented read capped at 2 MiB; struct-handle change mid-read detected; fragmented write chunk alignment handles wide elements. +- Decoders are bounds-checked: `CipReader.Take`, `MultipleServicePacket.Parse` (offset range), `EncapsulationCodec.ParseReply` (length/items), `PcccServices.ParseReply` (TNS before STS). +- `CipPath.EncodeRoute` rejects malformed links/odd pairs; `DeviceKey` canonicalises the route. + +**Modbus:** #527 dispose-before-overwrite in `InitializeCoreAsync` (`:307-319`) intact under `_lifecycleGate`; desync recovery tears the socket down before throwing (TCP `:176-191`, RTU `ModbusRtuFraming.cs:125-165`) and a coherent exception PDU does not; `BitInRegister` RMW serialised per register via `_rmwLocks` (bounded 65 536, no OS handle); `NormalizeWordOrder` correct for 8-byte types; block reads validate byte count before slicing; auto-prohibition bisection bounded and cleared on teardown. + +**S7:** `EnsureConnectedAsync` under `_gate` with `ConnectionBackoff` (one attempt per window), dead handle disposed once, failed `OpenAsync` handle disposed; worst-state-wins health with `AnySuccess` advancing `LastSuccessfulRead`; access-denied → sticky Faulted + `BadNotSupported` without reconnect churn; value-range/encode errors → `BadOutOfRange`/`BadTypeMismatch` with no health impact; multi-var chunk 12 safe for 240-byte PDU with per-tag fallback; `S7OperationDeadline` bounds every async op; Bool/wide arrays and Timer/Counter writes refused up front; shutdown drains bounded; TSAP/CPU map/codecs refuse rather than clamp. + +**Calculation:** timer `Timer`s + CTS disposed in scheduler `Dispose`, `StopTimers()` at the top of every lifecycle entry (no stacking on re-init); lock order `_evalGate → _lock` honoured, publishes fire outside both; `(value,status)` dedup on the recovery path correct (`:615`, `MarkInputsBad :639` per sub-code); `ClearCompiledScripts` vs in-flight run handled by the single retry on `ObjectDisposedException`; `AdoptConfig` clears `_pending` and drops removed tags' state; all input-cache access under `_lock`. + +**Cli / Contracts:** all five subscribe commands unsubscribe in `finally` and reach `ShutdownAsync`; CLI flags land (`--transport`, `--poll-only`, `--disable-reconnect`); the only Options property no driver reads is `ProbeTimeoutSeconds` (consumed by AdminUI Test Connect); all six factories use `TryReadEnumStrict`; S7 `NotSupportedException`s at `S7Driver.cs:1826-1830/2208-2212` are unreachable guards on the narrow path. + +**AbCip / AbLegacy:** native lease balance — every acquire has a release on every path (`NativeAbCipTagRuntime` init `:131-134` / `Dispose :398`; `EnsureTagRuntimeAsync` disposes on every init failure `:1465, 1472, 1477, 1496`; probe loop `:674, 715`; `AbCipDriverProbe` `finally :178`; `NativeAbLegacyTagRuntime` ctor acquire / `InitializeOrKeepRefusedAsync :1069-1073`; enumerator + template reader `finally`); `ConnectionLease.DisposeAsync` idempotent. `LibplctagOrphanRelease` wraps Initialize/Read/Write on both runtimes and the listing/template readers. AbCip `_tagsByRawPath`/`_declaredTags` swap by reference with `volatile` (`:109, 112, 461-462, 532`); `DisposeHandles` disposes runtimes, parent runtimes and both semaphore tables (`:2057-2069`); `ConnectGate` intentionally not disposed (`:1951-1958`). Alarm projection subscriptions retired out-of-band with CTSs disposed; told-records pruned by `RetainOnly` per subscribe; warned-once sets bounded by the authored set. `AbCipTemplateCache` never pruned — bounded by the controller's UDT count, staleness documented. AbCip read/write refusal classification (TypeMismatch, Unsupported, config exception, `NotSupportedException` on write) keeps the handle and leaves health alone; batch read health is worst-wins with `LastSuccessfulRead` advancing on any success. AbLegacy `WriteRefusal` before runtime creation (`:546-550`) + `LibplctagLegacyName` consistent across transports. Native STRING decode clamps LEN and handles the 88-byte padding; bit access and `Span` bounds-checked. `GetHostStatuses()` from the actor is try/catch-guarded (`CurrentHostStatuses`). `IRediscoverable` absent on AbCip is a documented gap. + +**TwinCAT / FOCAS:** ADS notification/variable handle lifetimes are balanced — `NativeRegistration.Dispose` deletes the AMS notification (`TwinCATDriver.cs:875-880` → `AdsTwinCATClient.cs:646-651`), replay swaps and disposes the old handle (`:1281-1282`), replay and probe-retry retract an orphan created by a racing unsubscribe (`:1286-1291, 1348-1353`), `ShutdownAsync`/`Dispose` dispose registrations before clients, `AdsTwinCATClient.Dispose` detaches the event, stops the dispatch loop and disposes the `AdsClient` (`:821-835`); `ReadRawAsync` variable handles deleted in `finally` (`:421-434`); failed connect disposes its client (`:1240-1248`); stale client disposed before replacement (`:1219-1224`); `RecycleClientAsync` runs under the gate. Sum-read classification matches #645. Codecs: `TwinCATStringCodec` NUL-terminates with 1252/UTF-8 by target; `TwinCATArrayCodec` throws on a short buffer and the notification path drops rather than publishes (`:612-615`); `TwinCATTemporalCodec.Encode` range-checks. FOCAS re-init/shutdown: all three loops cancelled + bounded-joined (5 s), CTSs disposed, client disposed; alarm projection paused not re-created; tag tables built fresh and swapped (`:238-318`); connect serialised per device with a `ReferenceEquals` guard (`:1855-1865`). FOCAS alarm health contract (#647) implemented as documented (`:485-499`). FOCAS wire parsers bounds-check via `RequireLength`; a mid-request failure closes both sockets; `ReadExactlyAsync` disposes the stream on cancellation; `SynchronizedFocasClient` pairs the gate with a per-call timeout. PMC coalescing memo bounded by the tag set. `AdsTwinCATClient.ConnectAsync` ignoring the token for the blocking SDK `Connect` is documented as the un-fixed root. Unverified assumption worth a wire capture: `DescribeSymbolAsync` per read relies on the Beckhoff symbol cache making `ReadSymbolAsync` local after the first call (`AdsTwinCATClient.cs:368-372`). + +**PollGroupEngine (shared):** one task per subscription; only the loop's own token exits; cancel + join (≤ 5 s) before CTS dispose; `LastValues` per subscription; reader-contract violation throws loudly. + +**Runtime seam:** `DriverInstanceActor.PostStop` calls `ShutdownAsync` only — the sync `IDisposable.Dispose()` overloads (`GetAwaiter().GetResult()`) on AbCip/Modbus/FOCAS have no production caller. diff --git a/docs/reviews/2026-10-21/08-historians-clients.md b/docs/reviews/2026-10-21/08-historians-clients.md new file mode 100644 index 00000000..b5fff4f0 --- /dev/null +++ b/docs/reviews/2026-10-21/08-historians-clients.md @@ -0,0 +1,226 @@ +# Review 08 — Historian backends (Gateway / SqlServer / QuestDb / Sql.Common), Client.*, Analyzers + +Repo: `/Users/dohertj2/Desktop/OtOpcUa` @ `d51d35bd` (master). Read-only review; every finding below was verified by reading the cited code. Line numbers are from the files as of this commit. + +Severity: Critical / High / Medium / Low. Confidence: High = the code path is unambiguous; Medium = depends on runtime/SDK/gateway behaviour I could not execute here but which the repo's own comments or the package docs assert. + +--- + +## 1. Memory / resource leaks + +### 1.1 Uncapped raw HistoryRead (`NumValuesPerNode = 0`) materialises a tag's whole window in memory on every backend +- **Severity:** High **Confidence:** High +- **Location:** + - `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer/SqlServerHistorianDataSource.cs:99-126` + - `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.QuestDb/QuestDbHistorianDataSource.cs:101-129` + - `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayHistorianDataSource.cs:213-216, 285-305` and `Mapping/SampleMapper.cs:69-75` + - Node-manager side (outside slice, confirms there is no paging): `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/HistoryPaging.cs:116` ("`NumValuesPerNode == 0` … never page; return everything in one shot"); SqlServer/QuestDb report `MaxRawValuesPerRead => 0` (SqlServerHistorianDataSource.cs:324, QuestDbHistorianDataSource.cs:365). +- **Evidence:** + ```csharp + // SqlServer + var top = maxValuesPerNode == 0 ? int.MaxValue : (int)Math.Min(maxValuesPerNode, int.MaxValue); + ... + $"SELECT TOP (@top) {SampleColumns} FROM {_conn.Q("Sample")} WHERE TagId = @id AND {where} ORDER BY SourceTimeUtc {order}" + while (await reader.ReadAsync(...)) samples.Add(new DataValueSnapshot(...)); // List, unbounded + // Gateway + if (cap == 0) { var all = await PassAsync(int.MaxValue) ... return all.Result.Samples; } // List + var snapshots = SampleMapper.ToSnapshots(samples); // second full copy + ``` +- **Failure scenario:** Any admitted session (reads are ungated for Anonymous with `Security:Acl:Enabled=false`, the default) issues HistoryRead-Raw with `NumValuesPerNode = 0` over a wide window on a dense tag (a year of 1 Hz = 31.5M rows). SqlServer streams the clustered-key seek into a `List` (boxed value + two DateTimes + status ≈ 100+ B each ⇒ several GB), the Gateway path builds the protobuf list *and* the snapshot list, then the SDK encodes the whole response. `MaxTimeWeightedRawRows` caps only processed reads; `HistoryReadDeadline` (60 s) is time, not rows, and a key seek fills GBs in well under 60 s. The server's `TransportQuotas` are SDK defaults (`OpcUaApplicationHost.cs:472`), so the oversized response is rejected *after* the allocation. With `MaxConcurrentHistoryReadBatches = 16` this is a trivial memory-exhaustion vector, and it also works on the Gateway backend when the gateway is in streaming mode (the default `AllowStreamingBeyondCeiling`). +- **Suggested fix:** Treat `0` as "server-chosen page size" (Part 11 allows a server to return fewer values plus a continuation point): add a `ServerHistorian:MaxRawValuesPerRead`-style hard cap (advertise it as `HistoryServerCapabilities/MaxReturnDataValues`) and route `N = 0` through the existing `RawPageLimit` paging. Independently, bound the backends' `TOP/LIMIT`/`max_values` by that cap so a misbehaving caller cannot bypass the node manager. + +### 1.2 Gateway events read drops `maxEvents` on the wire and abandons the gRPC stream at the cap +- **Severity:** High **Confidence:** Medium (the eviction behaviour is asserted by the repo's own comments and the gateway's `HistorianRpcFaults`, not re-verified here) +- **Location:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/HistorianGatewayClientAdapter.cs:111-124`; `GatewayHistorianDataSource.cs:576-598`; contrast `RawAggregateReader.cs:18-24` and `GatewayHistorianDataSource.cs:202-204`. +- **Evidence:** + ```csharp + // Adapter: maxEvents is accepted and never forwarded (the 0.3.0 package API has no max parameter) + public IAsyncEnumerable ReadEventsAsync(string? sourceName, DateTime startUtc, DateTime endUtc, int maxEvents, CancellationToken ct) + { ... return _inner.ReadEventsAsync(startUtc, endUtc, filter, ct); } + // Data source: breaks out of the server stream once the cap is full + if (hasCap && collected.Count == maxEvents) { truncated = true; break; } + ``` + while the raw path documents: "Every stream is consumed to completion (never broken out of — a cancelled gateway stream evicts its pooled historian session)" and `RawAggregateReader` says "the gateway currently classifies that cancellation as a connection fault and evicts its shared historian session (HistorianGateway `HistorianRpcFaults.IsConnectionFault`: any `RpcException` ⇒ true)". +- **Failure scenario:** Every capped HistoryReadEvents that has more than `NumValuesPerNode` events in its window (the normal case for a consumer paging alarm history, and for the #691.4 reverse walk-back which issues many small capped reads) cancels the server stream mid-way ⇒ the gateway evicts its historian session ⇒ the next read of *any* kind pays a reconnect, and concurrent raw reads sharing that session fail. Uncapped reads (`int.MaxValue`) additionally stream the entire window regardless of what the client asked for. +- **Suggested fix:** Drain the stream to completion with a bounded budget like the raw path (count past the cap without collecting), and add a `max_events` field to the gateway RPC so the gateway stops streaming at the cap; until then at least set `EventReadHorizon` guidance in docs. + +### 1.3 `FasterLogHistorizationOutbox.PeekBatchAsync` holds a monitor across FasterLog disk reads, blocking the recorder actor's appends +- **Severity:** Low **Confidence:** High +- **Location:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/Recorder/FasterLogHistorizationOutbox.cs:215-252` (peek) and `:147-156` (append index under the same lock); drain runs off-actor via `DrainOnceAsync(...).PipeTo` at `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs:696`. +- **Evidence:** + ```csharp + lock (_state) + { + ... + using FasterLogScanIterator iter = _log.Scan(_nextScanAddress, _log.TailAddress, recover: false); + while (batch.Count < max && iter.GetNext(out byte[] bytes, out int len, out long address, out long next)) + ``` +- **Failure scenario:** `FasterLog` is built with default settings (`new FasterLogSettings { LogDevice = _device }`, line 76), i.e. an ~8 MB in-memory window; `Capacity` defaults to 500 000 entries (≈35 MB on disk). Once the backlog's head has aged out of memory (any multi-minute gateway outage), `GetNext` reads pages from the device synchronously while `_state` is held; the actor thread's `AppendBatchAsync` then blocks on `lock (_state)` for the duration of the read — a sync block on an Akka dispatcher thread, once per 2 s drain pass, exactly during outage recovery. +- **Suggested fix:** Scan outside the lock (snapshot `_nextScanAddress` + the live-list cursor under the lock, iterate unlocked, re-take the lock only to flag/drop unreadable head records), or size `MemorySizeBits` to the configured capacity. + +### 1.4 Client raw history read leaves the server continuation point unreleased when stopped by the client cap +- **Severity:** Low **Confidence:** High +- **Location:** `src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs:175-206` vs the modified-read path `:263-272`. +- **Evidence:** `while (continuationPoint != null && continuationPoint.Length > 0 && allValues.Count < maxValues);` then `return allValues;` — no `releaseContinuationPoints` call; `HistoryReadModifiedAsync` explicitly releases ("A read stopped by the client's own cap leaves a server cursor behind; release it."). +- **Failure scenario:** A page that returns exactly `maxValues` (the common case on a dense tag) carries a continuation point the client never resumes or releases. Bounded in practice: the server's store is per-session and capped at `MaxHistoryContinuationPoints` (100, oldest-evicted, `OpcUaServer/HistoryContinuationStore.cs:50-55`), so the Client.UI's long-lived session just cycles 100 slots; the CLI disconnects immediately. +- **Suggested fix:** Mirror the modified path's release call. + +### 1.5 Client subscription adapters delete server-side but never remove the `Subscription` from the SDK `Session` +- **Severity:** Low **Confidence:** Medium +- **Location:** `src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs:321-333` (`_session.AddSubscription(subscription)`), `DefaultSubscriptionAdapter.cs:104-117, 122-133` (`_subscription.DeleteAsync(true, ct)` only). +- **Failure scenario:** `OpcUaClientService.SubscribeAlarmsAsync/UnsubscribeAlarmsAsync` and `MainWindowViewModel.MonitorAlarmsForSelectedNodeAsync` create a fresh SDK `Subscription` per cycle; each is deleted on the server but stays in `Session.Subscriptions` until the session is disposed, so a UI session that re-targets alarms repeatedly accumulates dead `Subscription` objects (and anything the SDK does over `Session.Subscriptions`, e.g. transfer/recreate on its own reconnect, iterates them). +- **Suggested fix:** Call `_session.RemoveSubscription(subscription)` after a successful delete (the adapter would need the session reference), or create subscriptions through `Session.RemoveSubscription`-aware helpers. + +--- + +## 2. Unimplemented / stubbed / silently wrong + +### 2.1 Gateway native `TimeAverage` / `Total` are served with **Good** status although the repo documents the values are wrong until `historiangw#27` +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/NativeAggregatePlan.cs:39-43`; served via `GatewayHistorianDataSource.cs:338-343, 350-387`; advertised in `ServedAggregates` (`:40-43`). +- **Evidence:** + ``` + ⚠️ Today historiangw#27 (the interval reaches AVEVA in ticks where it reads milliseconds) makes AVEVA answer ONE + Good row per request — the whole-intervals read lands on its earliest interval with a whole-span value and the rest + are absent — in both directions. + ``` + (`.claude/rules/historian.md` agrees: "until historiangw#27 lands both directions return Good-status wrong values".) +- **Failure scenario:** A client asks for 24 hourly `TimeAverage` intervals and receives one Good, whole-day value stamped on the first interval and 23 absent intervals; nothing on the wire says the answer is degraded. ScadaBridge or a report will store it as truth. +- **Suggested fix:** Until the gateway fix ships, either drop `TimeAverage`/`Total` from `ServedAggregates` (⇒ `BadAggregateNotSupported`, consistent with the other five time-weighted aggregates on this backend) or mark every row `UncertainDataSubNormal` + log once. Gate the native path behind a config flag that defaults off until #27 is live-verified. + +### 2.2 Gateway backend capability gaps vs. SqlServer/QuestDb (documented, but a consumer-visible matrix) +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway/GatewayHistorianProvider.cs:54` (`CreateUpdater => null`), `GatewayHistorianDataSource.cs:40-43` (12 of 17 aggregates), `:125-126` and `:556-562` (reverse raw/events `NotSupportedException`), `ReadModifiedAsync` not overridden (interface default ⇒ unsupported), no `CreateRetention`, `GatewayHistorianValueWriter.cs:28-38` (quality discarded by the gateway — "C-002"), `Mapping/HistorianTypeMapper.cs` (String/DateTime not provisioned). +- **What a client sees on Gateway only:** HistoryUpdate ⇒ `BadHistoryOperationUnsupported`; `IsReadModified` ⇒ unsupported; `TimeAverage2/Total2/Interpolative/PercentGood/PercentBad` ⇒ `BadAggregateNotSupported`; every stored sample reads back Good (comms-loss markers are dropped, driver Bad statuses are lost); reverse/end-only raw and events reads are emulated by forward walks (`ReverseRawReader`/`ReverseEventsReader`) that cost many round trips on sparse tags. +- **Suggested fix:** Nothing to "implement" in this repo for most of it (gateway-side), but publish the matrix in `docs/Historian.md` and have the AdminUI backend picker show it; the quality loss in particular deserves a startup Warning when `ContinuousHistorization` is enabled on Gateway. + +### 2.3 Client CLI/UI offer `StandardDeviation`, which no backend serves, and omit six aggregates that are served +- **Severity:** Low **Confidence:** High +- **Location:** `src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs` (12 members incl. `StandardDeviation`); `Client.CLI/Commands/HistoryReadCommand.cs:50, 202-221`; `Client.UI/ViewModels/HistoryViewModel.cs:47-62`. Served set: `Sql.Common/AggregateFamily.cs:50-51` (`FamilyOf != null` ⇒ 17: Average/Min/Max/Count/Range/Start/End/Delta/Min-MaxActualTime/TimeAverage/TimeAverage2/Total/Total2/Interpolative/PercentGood/PercentBad). +- **Failure scenario:** `historyread --aggregate stddev` always returns `BadAggregateNotSupported`; `Range`, `Delta`, `MinimumActualTime`, `MaximumActualTime`, `PercentGood`, `PercentBad` cannot be requested from the project's own client at all, so the gate recipes in `docs/Historian.md` cannot be run with Client.CLI. +- **Suggested fix:** Align `AggregateType` + `AggregateTypeMapper` with `AggregateFamilies.Served`. + +### 2.4 `ProbeAsync` / `GetConnectionStatusAsync` exist on the gateway seam but health is derived from read outcomes only +- **Severity:** Low **Confidence:** High +- **Location:** `IHistorianGatewayClient.cs:106, 111`; `GatewayHistorianDataSource.cs:616-638` (`ProcessConnectionOpen: _totalSuccesses > 0 && _consecutiveFailures == 0`). +- **Failure scenario:** A freshly booted node with a healthy gateway reports the historian as closed until some client performs a HistoryRead; a node that only *writes* (continuous historization) never flips it. Operators reading `/hosts` get a false negative. +- **Suggested fix:** Use `ProbeAsync` on a slow timer (or on `GetHealthSnapshot` with a cached result) now that the seam carries it. + +### 2.5 Two Gateway read arms record a caller cancellation as a backend failure +- **Severity:** Low **Confidence:** High +- **Location:** `GatewayHistorianDataSource.cs:607-612` (`ReadEventsAsync`) and `:519-524` (`ReadAtTimeNativeAsync`) lack the `catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) throw;` that `ReadRawAsync`/`ReadProcessedAsync`/`ReadAtTimeAsync` have (`:141-144, 377-380, 484-487`). +- **Failure scenario:** A `HistoryReadDeadline` expiry or client abort during an events read bumps `TotalFailures`/`ConsecutiveFailures` and flips `ProcessConnectionOpen` to false ⇒ health alarms on a healthy gateway. + +--- + +## 3. Performance limiters + +### 3.1 QuestDB live-write path: three round trips plus a 10 ms-poll visibility wait per tag group per drain pass +- **Severity:** Medium **Confidence:** High +- **Location:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.QuestDb/QuestDbHistorianValueWriter.cs:91-102, 128-143`; `QuestDbWalVisibility.cs:49-91`. +- **Evidence:** + ```csharp + await _catalog.EnsureAsync(tag, dataType, ct); // cached after first + var present = await ExistingTicksAsync(tag, rows, ct); // SELECT ts ... per cluster + for (...) await InsertAsync(...); // multi-row INSERT + await _wal.WaitAsync(_conn.SampleTable, ct); // SELECT sequencerTxn, then poll writerTxn every 10 ms ≤ 5 s + ``` +- **Failure scenario:** With `DrainConcurrency = 8` and WAL apply lagging under load, up to eight pollers each issue ~100 `wal_tables()` queries/s (≈800/s) against the same database that is already behind — the wait amplifies load exactly when the DB is slowest, and every tag group pays ≥3 round trips, so a 5 000-tag deployment at 1 s cadence spends `⌈5000/8⌉ × (3 RTT + apply latency)` per pass and falls behind the 2 s cadence. The existence pre-read is only needed for first-wins idempotency across redeliveries, and the visibility wait only so that an *immediate* redelivery sees the rows. +- **Suggested fix:** Wait once per drain pass (expose a `FlushAsync`/pass-end hook on `IHistorianValueWriter`, or let the recorder call `WaitAsync` once after `Task.WhenAll`), back the poll off exponentially (10→50→200 ms), and consider a per-pass existence read keyed by the outbox batch rather than per tag. + +### 3.2 All three alarm-history writers insert one event per round trip, serially +- **Severity:** Medium **Confidence:** High +- **Location:** `Gateway/GatewayAlarmHistorianWriter.cs:66-78` (one `SendEventAsync` per event); `SqlServer/SqlServerAlarmHistorianWriter.cs:55-59, 76-89` (new pooled connection + single-row `INSERT … WHERE NOT EXISTS` per event); `QuestDb/QuestDbAlarmHistorianWriter.cs:64-69, 89-105`. +- **Failure scenario:** The `LocalDbStoreAndForwardSink` drains `BatchSize` events per tick; an alarm storm of 10 000 events drains at `10 000 × RTT` (≥ 10–20 s on SQL, minutes over a WAN to the gateway), and the Primary-only drain means the backlog sits in LocalDb meanwhile. The per-event design exists to isolate poison events, but SqlServer already has the TVP + `OUTPUT` machinery to get per-row outcomes from one statement. +- **Suggested fix:** SqlServer: one `INSERT … SELECT FROM @events WHERE NOT EXISTS …` with `OUTPUT inserted.EventId` and map outcomes by id (fall back to per-row only on a data error). QuestDb: multi-row INSERT (DEDUP key already converges duplicates). Gateway: needs a batch `SendEvents` RPC. + +### 3.3 SQL Server retention issues O(tags) `DELETE TOP` statements per purge even when nothing is eligible +- **Severity:** Low **Confidence:** High +- **Location:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer/SqlServerHistorianRetention.cs:89-107, 134-149`. +- **Evidence:** `SELECT TagId FROM Tag` then `foreach (var id in tagIds) { DeleteLoopAsync(Sample…); if (trail) DeleteLoopAsync(SampleModified…); }` — the loop always runs at least one statement per table per tag, and the applock is held for the whole pass. +- **Failure scenario:** 50 000 historized tags ⇒ ≥100 000 statements per hourly purge (plus the full `Event` scan) on a database that has nothing to delete, serialised on one connection while other nodes' purges skip. +- **Suggested fix:** Pre-filter: `SELECT TagId FROM Tag t WHERE EXISTS (SELECT 1 FROM Sample s WHERE s.TagId = t.TagId AND s.SourceTimeUtc < @cutoff)` (a key seek per tag inside one statement), or keep a per-tag `OldestSampleUtc` high-water mark. + +### 3.4 QuestDB `event.source` and `sample_modified.tag` symbols carry no index +- **Severity:** Low **Confidence:** High +- **Location:** `QuestDb/QuestDbTables.cs:73-77` (`source SYMBOL`), `:95-99` (`tag SYMBOL`), vs the sample table's `tag SYMBOL INDEX` added for #683 (`:64-67`, `:116`). +- **Failure scenario:** `ReadEventsAsync` with a `sourceName` filter (`QuestDbHistorianDataSource.cs:402-407`) and every `ReadModifiedAsync` (`:183-185`) scan all partitions in the window rather than seeking; the #683 gate measured the same shape costing seconds on the sample table. +- **Suggested fix:** Add `INDEX` on both symbols in the DDL plus the same idempotent `ALTER … ADD INDEX` migration the bootstrapper already runs for `sample.tag`. + +### 3.5 `GatewayTagPresenceCache` adds a catalog-browse RPC before HistoryReads, keyed by a glob filter +- **Severity:** Low **Confidence:** Medium +- **Location:** `Gateway/GatewayTagPresenceCache.cs:16-32` (60 s positive / 5 s negative TTL, 4 096 entries); `HistorianGatewayClientAdapter.cs:85-93` (`BrowseTagNamesAsync(tag, ct)` drained fully). Package doc (0.3.0 XML): `BrowseTagNamesAsync(filter)` — "Tag name filter pattern accepted by the historian (e.g. `Sys*`)". +- **Failure scenario:** A consumer sweeping history for N tags pays N extra streaming RPCs per minute of sweep; with more than 4 096 distinct tags the LRU-less eviction (`_entries.Keys.First()`) thrashes. A historian tag name containing `*`/`?` (legal in AVEVA) streams every matching catalog entry on each check. +- **Suggested fix:** Use the package's `GetTagMetadataAsync` (exact lookup) now that the seam exists, lengthen the positive TTL, or drop the pre-check and treat an empty raw read as "no data" (which it already does). + +### 3.6 Client: N+1 browse in `BrowseAsync`, and O(n) scan per notification in the UI +- **Severity:** Low **Confidence:** High +- **Location:** `Client.Shared/OpcUaClientService.cs:223-242` (`HasChildrenAsync` ⇒ one Browse per Object child); `Client.UI/ViewModels/SubscriptionsViewModel.cs:61-73` (`foreach (var item in ActiveSubscriptions) if (item.NodeId == e.NodeId)` on the UI thread per notification). +- **Failure scenario:** `subscribe --recursive` / the UI's recursive subscribe over a 50 000-node raw tree issues ~50 000 Browse calls to find children; 2 000 subscribed items at 1 Hz = 4 M string compares/s on the Avalonia UI thread. +- **Suggested fix:** Browse with `includeSubtypes`/`resultMask` carrying `NodeClass` only and infer `HasChildren` lazily on expand; index `ActiveSubscriptions` by NodeId in a dictionary. + +--- + +## 4. Other hazards + +### 4.1 Every production `IHistorianValueWriter` swallows `OperationCanceledException`, so the recorder's shutdown guard is dead code +- **Severity:** Medium **Confidence:** High +- **Location:** `Gateway/Recorder/GatewayHistorianValueWriter.cs:112-120`; `SqlServer/SqlServerHistorianValueWriter.cs:93-97`; `QuestDb/QuestDbHistorianValueWriter.cs:104-108` — all `catch (Exception) { …; return false; }`. The consumer expects the opposite: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs:744-755`: + ```csharp + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Shutdown cut this write short. ... swallowed as `ok = false` it becomes a consecutive failure, and when a + // sibling group already wrote this pass the fold books it — so a routine node stop can tip a perfectly + // HEALTHY tag over its attempt budget and discard its queued samples on the way out. Rethrow ... + throw; + } + ``` +- **Failure scenario:** At shutdown mid-pass the cancelled groups return `false`; if any sibling already wrote, `_tagWriteFailures[tag]++` for each cancelled tag (recorder `:826-834`) — precisely the outcome the guard was written to prevent. Today the damage is bounded (+1 per process stop, counters are in-memory, `_lifetime` cancels only in `PostStop`, recorder `:340`), so no data is lost, but the contract between writer ("never throws") and recorder ("rethrow cancellation") is contradictory and the next refactor that cancels drains without restarting (a capture-gate close, a backend switch) will discard healthy tags' backlog after `MaxWriteAttempts` such events. +- **Suggested fix:** `catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; }` ahead of the catch-all in all three writers (the alarm writers already special-case it), and state it on `IHistorianValueWriter`. + +### 4.2 Gateway value writer relabels a Local-kind timestamp as UTC instead of converting it +- **Severity:** Medium **Confidence:** Medium (depends on a driver ever producing `DateTimeKind.Local`) +- **Location:** `Gateway/Recorder/GatewayHistorianValueWriter.cs:93-98`: + ```csharp + live.Timestamp = Timestamp.FromDateTime(DateTime.SpecifyKind(timestampUtc, DateTimeKind.Utc)); + ``` + vs the SQL backends' shared normaliser `Sql.Common/LiveWritePreparer.cs:31`: `t = t.Kind == DateTimeKind.Local ? t.ToUniversalTime() : DateTime.SpecifyKind(t, DateTimeKind.Utc);`. +- **Failure scenario:** A driver that stamps `DateTime.Now`-derived source times (or an OpcUaClient upstream server returning Local) historizes correctly on SqlServer/QuestDb but shifted by the node's UTC offset on Gateway — and the HistoryUpdate path on SQL backends keeps the kind (`HistoryUpdatePlanner` ⇒ `LiveWritePreparer.Truncate` only), so the same value can land at two different times on two backends. +- **Suggested fix:** Run gateway writes through `LiveWritePreparer.Prepare` (it already does the non-finite ⇒ Empty and dedup work the gateway path lacks), or at minimum `ToUniversalTime()` for `Local`. + +### 4.3 SqlServer `DeleteRaw` removes an unbounded range in a single statement/transaction with `OUTPUT INTO` +- **Severity:** Low **Confidence:** High +- **Location:** `SqlServer/SqlServerHistorianDataUpdater.cs:99-113`. +- **Failure scenario:** `DELETE FROM Sample OUTPUT … INTO SampleModified WHERE TagId=@id AND SourceTimeUtc >= @s AND < @e` over years of a 1 Hz tag deletes tens of millions of rows and writes as many audit rows in one transaction: lock escalation on `Sample` (blocking the live writer), transaction-log growth, and a 30 s `CommandTimeout` that aborts and rolls back the whole thing — the retention service deliberately batches `DELETE TOP (n)` for this reason. QuestDB's updater already pages (`QuestDbHistorianDataUpdater.cs:135-166`). +- **Suggested fix:** Page the delete (`DELETE TOP (@n) … OUTPUT …` in a loop under one transaction or per-batch commits; the OPC UA result is a single status either way). + +### 4.4 QuestDB HistoryUpdate: non-atomic trail + per-instance locks + duplicating retries (documented residuals) +- **Severity:** Low **Confidence:** High +- **Location:** `QuestDb/QuestDbHistorianDataUpdater.cs:18-21, 28-34, 155-160, 235-240`; `.claude/rules/historian.md` ("a QuestDB trail INSERT retried after a client timeout duplicates (no DEDUP key)"). +- **Failure scenario:** A crash or failed sample INSERT after `WriteTrailAsync` leaves orphan `sample_modified` rows describing changes that never landed; a second node (or the live writer) racing the same key bypasses the 64 striped `SemaphoreSlim`s; a Npgsql timeout retry duplicates trail rows. All documented; listed so the matrix is complete. +- **Suggested fix:** Record a `applied` marker (a second append with the same `modified_id`) after the sample write so `ReadModifiedAsync` can drop orphans; use a DEDUP key `(ts, tag, modified_id)` on the trail. + +### 4.5 Client: failover nulls subscription state outside the lock the rest of the class relies on +- **Severity:** Low **Confidence:** High +- **Location:** `Client.Shared/OpcUaClientService.cs:734-735` (`_dataSubscription = null; _alarmSubscription = null;` in `RunFailoverAsync`, no lock) vs `:260-263` (`SubscribeAsync` reads/creates `_dataSubscription` unguarded) and the class comment `:18-22` that promises all bookkeeping is under `_subscriptionLock`. +- **Failure scenario:** A `SubscribeAsync` racing a keep-alive failover can add a monitored item to the dying session's subscription (silently lost, not replayed) or observe a null and create a second subscription on the new session that `ReplaySubscriptionsAsync` then duplicates. +- **Suggested fix:** Take `_subscriptionLock` around the null-outs and guard `SubscribeAsync` with the same snapshot pattern `DisconnectAsync` uses; also `ConnectAsync` (`:97-128`) should refuse (or disconnect) when already connected instead of overwriting `_session`. + +--- + +## 5. Checked and fine (looked like problems, are not) + +- **SQL injection:** every dynamic identifier is regex-validated before splicing — SqlServer `Schema` `^[A-Za-z_][A-Za-z0-9_]{0,127}$` (`SqlServerHistorianOptions.cs:49-59`, `SqlHistorianConnection.Q` brackets it, `$(Schema)` substituted only after validation), QuestDb `TablePrefix` `^[A-Za-z0-9_]+$` (`QuestDbHistorianOptions.cs:32-38`, re-checked in `QuestDbTables.Name`), `table_columns('{table}')` uses that validated name, QuestDB `AtTimeSql` inlines only generated ints, TTL days is `int`. All values (tag names, times, principals, event fields) are parameters; the SqlServer alarm writer deliberately uses `NVARCHAR(MAX)` params so truncation is a server error, not a silent clip. +- **Continuous-historization buffer bound:** the FasterLog outbox is disk-backed with `Capacity` (500 000 default) drop-oldest and `DroppedCount`/`UnreadableCount` metrics; drop-oldest truncates + commits, corrupt head records cannot pin the head (`FasterLogHistorizationOutbox.cs:172-200, 243-250`). Not unbounded. +- **Flush on shutdown:** `PerEntry` commit mode fsyncs before `AppendBatchAsync` returns; `Dispose` cancels the periodic loop, awaits it, then `Commit(spinWait: true)` (`:365-406`). Appends in flight at process kill are the documented `Periodic` trade-off only. +- **Time-weighted aggregates stream, they do not materialise:** both SQL backends feed `TimeWeightedAggregator` row by row with `TOP/LIMIT (cap + 1)` and `CheckWindowRows` fails the node one row past `MaxTimeWeightedRawRows`; memory is O(buckets) (`SqlServerHistorianDataSource.cs:528-550`, `QuestDbHistorianDataSource.cs:545-579`, `TimeWeightedAggregator.cs:132-154`). Gateway per-sample aggregates likewise accumulate into `RawBucketAccumulator` (O(buckets)). +- **`BucketPlan.MaxBuckets = 100 000`** bounds every processed read's result list and the aggregator arrays before any I/O. +- **Tag catalogs** (`SqlServerTagCatalog._cache`, `QuestDbTagCatalog._known`) are positive-only and never pruned, but are bounded by the number of distinct historian tag names ever written on this node (≤ a few hundred KB); a renamed tag leaves one stale int entry. Not a leak worth fixing. +- **Gateway channels:** four `HistorianGatewayClientAdapter`s (data source, alarm writer, provisioner, value writer) each own a `GrpcChannel`, each disposed by its owner (`DisposeAsync` on all four). Documented as deliberate; a co-located sidecar tolerates four HTTP/2 connections. +- **`GatewayRowCeiling`** is a single `int` + ticks with `Volatile`/`Interlocked`; a race costs one extra refusal, never a wrong answer. `GatewayAtTimeReader` is capped at `64 + 16 × times` reads and `1 024` rows per read, so a pathological tag fails `BadTooManyOperations` rather than looping. +- **Schema bootstrap:** `SqlServerSchemaBootstrapper`/`QuestDbSchemaBootstrapper` use `SemaphoreSlim(1,1)` + a 5 s negative cache (`BootstrapFailureCache`), so an outage does not serialise every call behind one round trip; the SqlServer applock is session-owned and the pool is cleared when release fails (otherwise the lock would ride a pooled connection forever) — same pattern in retention. +- **Timestamps on the SQL backends:** `datetime2(7)` with explicit UTC `SpecifyKind` on read/write; QuestDB rounds to µs and nudges `Min/MaxValue` so Npgsql never binds ±infinity; pre-1970 is refused/dropped with counters rather than failing a batch. `IHistorianDataSource.TimestampResolutionTicks = 10` lets the paging step correctly on QuestDB. +- **`GatewayHistorianDataSource.Dispose()`** is sync-over-async (`DisposeAsync().AsTask().GetAwaiter().GetResult()`) — shutdown-only, channel shutdown is quick; acceptable. +- **Analyzer (`UnwrappedCapabilityCallAnalyzer`)**: registers nothing when the guarded interfaces are absent from the compilation, resolves symbols once per compilation, and walks ancestors only up to the enclosing wrapper invocation; the one acknowledged limitation (any lambda argument to `ExecuteAsync` suppresses) is documented and currently unreachable. No production hazard. +- **Client `DisposeBridge`** bounds every sync-over-async teardown (`CloseAsync`/`DeleteAsync`) at 5 s and cancels the token when the wait loses, so a dead server cannot hang `Dispose`. +- **`HistorizationOutboxEntrySerializer`** is bounds-checked for both layouts (negative length prefixes, short records, invalid DateTime binary, non-0/1 booleans all return `false`), so a corrupt record is counted and skipped, never thrown. diff --git a/docs/superpowers/plans/2026-10-22-review-remediation.md b/docs/superpowers/plans/2026-10-22-review-remediation.md new file mode 100644 index 00000000..e7a5f4aa --- /dev/null +++ b/docs/superpowers/plans/2026-10-22-review-remediation.md @@ -0,0 +1,565 @@ +# System Review Remediation Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development (recommended) or superpowers-extended-cc:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Resolve every finding from the 2026-10-21 8-slice Fable system review of OtOpcUa (master `d51d35bd`): 1 Critical, ~25 High, ~60 Medium, ~85 Low. These are memory/resource leaks, unimplemented features, performance limiters, and concurrency/health-contract hazards. + +**Architecture:** The work is split into **file-disjoint lanes**. Each lane is executed by a subagent in its own git worktree off master, using TDD inside each task. Each lane gets an independent adversarial review by a model at least as strong as its implementer, then merges to local master in a fixed order. Lanes run in 4 waves. A lane only touches the files it owns. A finding whose fix needs a file another lane owns is sequenced into a later wave, not shared. + +**Tech Stack:** .NET 10, Akka.NET, OPC Foundation UA-.NETStandard 1.5.378, EF Core/SQL Server, Roslyn scripting, Blazor Server, gRPC (mxaccessgw / HistorianGateway), xUnit + Shouldly. + +**Spec:** the review reports. These are copied into the repo by Task 0 because the scratchpad is session-scoped: +`docs/reviews/2026-10-21/00-SUMMARY.md` … `08-historians-clients.md`. +Finding refs below use `R:`. For example, `R07:H10` is report 07, finding H10. Each finding in a report carries its location, evidence, failure scenario and a fix recipe, and that recipe is the starting design for its task. + +## Context + +The user asked for a parallel review hunting memory leaks, unimplemented features and performance limiters. Eight Fable reviewers produced verified findings, and the user now wants all of them resolved. The review found 8 cross-cutting patterns (summary §"Cross-cutting themes"). The plan fixes each pattern once, then sweeps the per-driver instances: + +1. The runtime never disposes drivers. This is the root of the Critical Galaxy leak: each leaked gateway session pins an x86 worker for 30 min, so about 64 respawns exhaust `MaxSessions` (mxaccessgw cross-check). +2. Timed-out scripts orphan a thread per evaluation. +3. The health and watchdog contract is wrong in both directions across drivers. +4. A plain `Dictionary` is mutated under live poll loops. +5. One device can stall every device in its driver. +6. Array writes are advertised but unimplemented. +7. Config fields are authored but never consumed. +8. Mailbox-blocking `ReceiveAsync` calls. + +## Global Constraints + +- **Worktree hygiene:** every lane dispatch runs `git reset --hard ` in its worktree and verifies it with `git merge-base --is-ancestor HEAD`, because worktrees start on stale commits. Each lane rebases on master before review. +- **File ownership is binding:** a lane edits only the paths in its "Owns" list. If a lane needs a change elsewhere, it creates an `F` follow-up task (memory: track deviations as follow-ups, don't ask) instead of editing. +- **Fix the root, then the instance:** where a pattern has a shared fix (Lane A disposes drivers, Lane E quarantines scripts), driver lanes must still make their own `ShutdownAsync` a complete teardown. The runtime fix is a safety net, not a substitute. +- **Health contract (from `.claude/rules/drivers.md` + #527/#611):** + - A deterministic device refusal is per-tag Bad, with a once-per-tag Warning and no health write. + - Comms faults write `Degraded` and preserve `LastSuccessfulRead`. + - Any data proving the device answered advances `LastSuccessfulRead`. + - Recovery dedup is on `(value, status)`, never value-only. + + Every driver lane is reviewed against this checklist. +- **Re-init safety:** device tables that are read by retained poll loops are build-and-swapped (`Volatile.Write`) or made `ConcurrentDictionary`, never `.Clear()`ed in place. +- **Array writes (user decision):** encode `count` elements at the resolved element width, refuse a length mismatch with `BadOutOfRange`, and keep the ScadaBridge wire shape. ScadaBridge writes CLR lists such as `int[]` to Int16 nodes, so the element type converts and is not exact-matched. +- **Dead config fields (user decision): wire every one.** Persisted JSON with unknown keys must keep loading. +- **Absence assertions need a positive control** (memory). Any "no longer leaks / no longer publishes" test must also prove the positive path delivers. +- **AdminUI has no bUnit:** Razor changes are live-verified on the docker-dev rig (`http://localhost:9200`, login disabled) with Chrome automation, not unit tests alone. +- **Historian paging changes require a paged-vs-unpaged fuzz test** against a wire-faithful fake (memory, #692), re-run on every revision. +- **No remote commands on wonder boxes** (no psbridge or winrm). Galaxy and AVEVA gates use network service access only, and any remote action is a userGate task for the user to run. +- **`.claude/` is untracked**, so worktree agents cannot edit `.claude/rules/*`. They put suggested rule text in their final report, and the coordinator applies it in the main checkout. +- **Subagents cannot write to the coordinator scratchpad.** They return reports in their final reply. +- **Never commit secrets.** Commits end with the session attribution line. + +**User decisions (already made):** +- "Implement array encode now" for AbCip, AbLegacy and TwinCAT. Real-hardware proof is a userGate. +- "Wire them all" for every authored-but-unconsumed field: MQTT `birthObservationWindowSeconds`, Galaxy `DiscoverPageSize`, Galaxy and OpcUaClient `ProbeTimeoutSeconds`, Modbus `Family`/`MelsecSubFamily`/`DisableFC23`, `TelemetryDial:CallTimeoutSeconds`, `/auth/token` JWT (finish it with `AddJwtBearer`), and the `GetDiagnostics` snapshot (populate it with real data and give it a consumer). +- "Gitea issues + local merge, push on OK": one issue per lane with findings as a checklist, merge to local master, and push/close only after the user OKs. The scadaproj index is updated on its `main`. + +--- + +## Model assignment + +| Tier | Model | Used for | +|---|---|---| +| frontier | **fable** | Actor lifecycle (A), OPC UA node manager/SDK (B), deploy coordinator (C1), script runtime + alarm actor restart (E1/E4), Galaxy (F), HistoryRead cap (H1). Fable reviews these lanes. | +| standard | **opus** | All driver lanes, control-plane remainder, Roslyn incremental analysis, sandbox, historian remainder. Opus adversarial review of every lane, including sonnet ones. | +| mechanical | **sonnet** | AdminUI UI fixes, CLI fixes, client library fixes, transport buffer pooling, docs/index edits, Gitea issue filing | +| — | haiku | not used; every task here needs code judgement | + +## Waves and parallelism + +``` +Wave 0 (serial, coordinator) : T0 setup, T1 baseline suite +Wave 1 (6 lanes parallel) : A Runtime · B AddressSpace · C ControlPlane · E Scripting · F Galaxy · H1 HistoryRead cap +Wave 2 (8 lanes parallel) : AbCip · AbLegacy · TwinCAT · FOCAS · Modbus · S7 · G1 OpcUaClient+MTConnect · G2 MQTT+Sql +Wave 3 (4 lanes parallel) : D AdminUI · H2 Historian remainder (+B2 last) · K Clients/CLI/Transport · E5 Alarm comment table +Wave 4 (serial) : T-INT full suite + fixture gates + docker-dev gates + hw userGates + docs/index + push-on-OK +``` + +The wave order follows file contention: +- Wave 2 driver lanes depend on A's dispose contract only for review context, not for code. +- D waits for the Modbus/FOCAS editor and form edits in Wave 2. +- B2 waits for B (`OtOpcUaNodeManager.cs`). +- H2 waits for H1 (`HistoryPaging.cs`) and owns the recorder. + +Merge order inside a wave is alphabetical by lane, rebasing as needed. Two lanes never share a function. + +--- + +## Wave 0 + +### Task 0: Setup — persist the spec, file the issues, materialize the plan +**Goal:** The reports and plan live in the repo, there is one Gitea issue per lane, and native tasks exist. +**Files:** Create `docs/reviews/2026-10-21/*.md` (copy all 9 from the session scratchpad `…/scratchpad/review/`), `docs/superpowers/plans/2026-10-22-review-remediation.md` (this plan), and `….md.tasks.json`. +**Acceptance Criteria:** +- [ ] `ls docs/reviews/2026-10-21 | wc -l` = 9 +- [ ] One Gitea issue per lane (A, B, B2, C, D, E, E5, F, G1, G2, H1, H2, K, AbCip, AbLegacy, TwinCAT, FOCAS, Modbus, S7). Each issue carries its findings as `- [ ] R0x:ID` lines, and the issue numbers are recorded in tasks.json. +- [ ] Cross-repo issues are filed: + - mxaccessgw: lease never extended without `StreamEvents` + no renew RPC; `mxgateway.sessions.open` not decremented on sweep close. + - HistorianGateway: `max_events` on `ReadEvents`; a batch `SendEvents` RPC; historiangw#27 tracking reference. +- [ ] Commit `docs: add 2026-10-21 system review + remediation plan`. + +**Verify:** `git show --stat HEAD` lists the reports and the plan. +**Model:** sonnet (issue filing), coordinator commits. + +### Task 1: Baseline full suite +**Goal:** Record the pre-change green or flake baseline, so regressions are attributable. +**Acceptance Criteria:** +- [ ] `dotnet build ZB.MOM.WW.OtOpcUa.slnx -c Release` succeeds. +- [ ] `dotnet test ZB.MOM.WW.OtOpcUa.slnx --filter "Category!=E2E&Category!=LiveIntegration"` has its pass/fail/flaky list saved to `docs/reviews/2026-10-21/baseline.txt`. + +**Verify:** `tail -5 docs/reviews/2026-10-21/baseline.txt`. +**Model:** coordinator. + +--- + +## Wave 1 — high-severity core (6 parallel lanes) + +### Lane A — Runtime driver lifecycle (fable; review fable) +**Owns:** +- `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/**` +- `Runtime/Health/**` +- `Runtime/Telemetry/**` +- `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs` + the telemetry dialer +- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriver.cs` +- `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/DiscoveryDriverBrowser.cs` +- the `IFleetDiagnosticsClient` consumer page (one new Razor file) + `AdminUI/Clients/FleetDiagnosticsClient.cs` + +**Tests:** `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests`, `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests` + +- **A1 Dispose drivers on final stop** (`R02:L1`, `R02:P4`, `R06` host context, `R07:L2`). + - In `DriverInstanceActor.PostStop`: `ShutdownAsync(cts.Token).WaitAsync(5 s)`, with a logged timeout. Then `DisposeAsync`/`Dispose`, but only when the actor is stopping for good. Override `PreRestart` to skip disposal, because the Props closure reuses the driver. + - `DiscoveryDriverBrowser` disposes its throwaway driver in `finally`. + - Tests: + - A fake `IAsyncDisposable` driver is disposed exactly once on `Context.Stop`. + - It is NOT disposed on a supervision restart (positive control: it still serves after restart). + - A hung `ShutdownAsync` doesn't pin a thread beyond 5 s. +- **A2 Single-flight reconnect** (`R02:H2`). Add an `_initInFlight` gate cleared by the piped `InitializeSucceeded`/`Failed`, and bound the teardown. Test: with a 15 s init and a 10 s cadence, `ShutdownAsync` and `InitializeAsync` never overlap (record interleavings in the fake). +- **A3 Writes off the mailbox** (`R02:P2`). Run `WriteAttribute` via `PipeTo`, capturing `Sender`, with a per-driver in-flight cap (default 4). Subscribe/unsubscribe stay `ReceiveAsync`. Tests: + - Two concurrent 3 s writes both complete under the host's 8 s Ask. + - Data changes are delivered during a pending write. +- **A4 Housekeeping:** + - `_lastReprovisionUtc.Remove` in `StopChild` (`R02:L2`). + - `Stale()` drop-receives (`R02:H5`). + - Remove the unused `ILoggerFactory` ctor slot (`R02:U5`). + - Move the `OnAttributeValuePublished` seam to a test helper (`R02:U4`). + - `DbHealthProbeActor` off-thread + 2 s command timeout (`R02:P7`). + - Audit all `PollGroupEngine.Unsubscribe` sync callers and switch them to `UnsubscribeAsync` (`R05:8`). + - `TelemetryLocalHub._evicted` timestamped sweep with TTL > max in-flight call (`R02:L4`). +- **A5 Wire `TelemetryDial:CallTimeoutSeconds`** into the dialer's `CallOptions` deadline (`R02:U2`). Test: a stalled stream is cancelled at the configured deadline. +- **A6 Real `GetDiagnostics`** (`R02:U1`). Populate it from child health snapshots (real id, device counts from `GetHostStatuses`, last-change time), and give `IFleetDiagnosticsClient` a consumer: a per-node diagnostics panel on `/fleet`. That Razor file is owned here and live-verified in Wave 4. + +**Lane gate:** Runtime.Tests green. The docker-dev gate (Wave 4) shows a driver config change disposing the old instance: the `ObjectDisposed`/dispose log appears, and process thread/handle counts stay flat across 20 respawns (`dotnet-counters`). + +### Lane B — OPC UA address space (fable; review fable) +**Owns:** +- `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/**`, except `HistoryPaging.cs`/`RawPageLimit.cs` (H1) and the HistoryReadEvents projection (B2) +- `Runtime/OpcUa/OpcUaPublishActor.cs` + +**Tests:** `OpcUaServer.Tests`, `OpcUaServer.IntegrationTests` + +- **B1 No phantom nodes** (`R01:L1`, `L4`, `H2`): + - `WriteValueLocked` is non-creating (unknown key ⇒ skipped, counted, Debug-logged). + - `WriteAlarmCondition` unknown id ⇒ logged no-op. + - Delete the applier's equipment-id `RemovedConditionState` write. + - Add a cross-map `(realm,id)` collision guard in `EnsureFolder`/`EnsureVariable`/`MaterialiseAlarmCondition`. + - Tests: + - A publish racing a surgical remove leaves no node. + - A re-add after that gets the authored DataType, parent and writability (positive control). +- **B2a Stale MonitoredNode2** (`R01:L2`). + - On every removal path (rebuild, `RemoveVariableNode`, `RemoveAlarmConditionNode`, `RemoveEquipmentSubtree`): queue `BadNodeIdUnknown` to the node's items, `MonitoredNodes.Remove(nodeId)`, and drop `_liveItems`/`_eventItems`. + - Revalidation restore re-resolves the node and does not reuse `Handle.Node`. + - Integration test (real SDK): subscribe, rebuild, create a new item on the same NodeId. The new item receives values (positive control) and the old one gets `BadNodeIdUnknown`. +- **B3 Lock-time performance:** + - Rebuild detaches top-level subtrees, not leaves (`R01:P1`). + - Per-equipment descendant index for `RemoveEquipmentSubtree` (`P2`). + - `LowerAllEventsMonitoring` for the surgical removal sites (`P3`). + - Notifier wiring de-quadratic (`P5`). + - Benchmark test: rebuild a 10k-tag folder and remove 200 equipment from a 50k-node space. Assert completion < 1 s on CI hardware and record the before/after in the commit message. +- **B4 Write-path robustness:** + - `OnEquipmentTagWrite` observes `t.Exception` and wraps a synchronous throw (`R01:L3`, `H5`). + - Shelve/condition timestamps use the injected `TimeProvider` (`H4`). + - SDK request quotas + `TransportQuotas` configurable via `OpcUa:Quotas:*` with the current defaults (`P6`). + - HistoryRead arms: lower the default `MaxConcurrentHistoryReadBatches` on small nodes, and use an `IAsyncNodeManager` override if the SDK line exposes `HistoryReadAsync`; otherwise document it (`P4`). +- **B5 Delete dead v3 equipment-tag materialisation** (`R01:U2`): the composer's `EquipmentTags`, applier `MaterialiseEquipmentTags`, the surgical `ChangedEquipmentTags` loop, and the classifier `TagDeltaIsSurgicalEligible`. Keep `IsEmpty` semantics; the full OpcUaServer suite stays green. +- **B6 Off-thread deploy parse/plan** (`R02:P5`). + - `DeploymentArtifact.Open` + `ParseComposition` + `AddressSpacePlanner.Compute` run via `PipeTo`; only sink materialisation runs on the pinned thread. + - Delete the bytes-less `LoadArtifact` SQL branch. + - Test: value batches keep flowing while a 50k-node plan computes. +- **B7 Capability bits refresh** (`R01:U3`). Re-evaluate event-notifier/HistoryWrite bits when `HistorianDataSource`/`Updater` is set after the first apply, so a late assignment no longer needs a full rebuild. +- Documented, no change: `R01:U4` (native Confirm/shelve, by design) and `R01:L5` (bounded). Note both in the issue. + +**Lane gate:** both suites green. Wave 4 docker-dev gate: a ScadaBridge-style subscription survives a surgical add, and a re-created item after a rebuild receives data (Client.CLI `subscribe`). + +### Lane C — Control plane / host / security (fable for C1–C2, opus for C3–C5; review fable) +**Owns:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/**`, `src/Server/ZB.MOM.WW.OtOpcUa.Host/**` (except `Host/Engines/**`, which belongs to E), `src/Server/ZB.MOM.WW.OtOpcUa.Security/**`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/**` (except `Validation/DraftValidator.cs`, which belongs to E). +**Tests:** `ControlPlane.Tests`, `Host.Tests`, `Security.Tests`, `Configuration.Tests` (central SQL) + +- **C1 Deploy wedge + consistent snapshot** (`R03:2.2`, `3.2`, `3.5`): + - In PreStart, a recovered deployment with an empty expected set is re-seeded and re-dispatched (or marked `Failed` + `ResetForNext`). + - `HandleDeadline` takes the race branch only when `_expectedAcks.Count > 0`. + - The `Dispatching` row and the expected set are created together in the coordinator. + - Validate + compose from ONE snapshot (extend `DraftSnapshot` with clusters/nodes/ApiKeys) inside a `SNAPSHOT` read transaction. + - Project `ArtifactBlob` away in full-row reads. + - Tests: + - Inject a `HandleDispatch` throw; the next `StartDeployment` succeeds (positive control: a normal deploy seals). + - An edit between validate and compose cannot ship unvalidated. +- **C2 Admin singleton mailbox** (`R03:3.1`). Move `TestDriverConnect` to a per-request child actor (or `Task.Run` + `PipeTo`) and run deploy compose off-actor; only the in-flight check is serialized. Test: an alarm ack completes in < 1 s while a 60 s Test Connect is pending. +- **C3 OPC UA start failure is visible** (`R03:2.1`, `1.4`): + - Add an `OpcUaEndpointHealthCheck` (Ready + Active tags), Unhealthy while the SDK failed to start. + - A driver-only node rethrows (process exits non-zero). + - The "SDK not started" fact feeds the ServiceLevel calculator, so the peer takes Primary. If the calculator lives in a Lane B-owned file (`OpcUaServer/**`), C only exposes the fact (`IOpcUaEndpointState`) and files an `F` follow-up for Wave 3 to consume it. + - `StopAsync` stops the SDK; `DisposeAsync` is idempotent. + - Tests: port-in-use ⇒ `/health/ready` 503, and the ServiceLevel published drops. +- **C4 Security:** + - `ApiKeyOptionsValidator` + `ValidateOnStart` (non-blank pepper, minimum length, whenever driver or admin) (`R03:2.3`). + - **Finish JWT** (`R03:2.4`, user decision): `AddJwtBearer` using `BuildValidationParameters()`; `JwtOptions` `ValidateOnStart` when `/auth/token` is mapped; the `/api/*` machine endpoints accept a Bearer token as well as API keys. + - Wire `IAuditActorAccessor` through `AuthenticationStateProvider`, so audit rows carry the circuit principal (`R03:2.5`). + - Delete the unregistered `CookieAuthenticationStateProvider`. + - LDAP login exception logged at Warning (`2.7`). + - Admin ops return a fixed message + correlation id (`4.1`). +- **C5 Retention + throughput:** + - `DeploymentRetentionActor` on the admin singleton: keep the last N=50 sealed rows per cluster plus anything referenced in the last 30 days; null `ArtifactBlob` on older rows; configurable `Deployments:Retention:*` (`R03:1.2`). + - `AuditWriterActor` handles duplicate keys per row (2601/2627 ⇒ detach and retry) (`2.6`). + - `MeshGrpcCommandDialer` evicts endpoints absent from `ContactsLoaded` (`1.3`). + - Async PSK refresh with an atomic swap instead of invalidate-on-refresh (`3.4`). + - Pre-auth API-key DoS: an in-memory key-id set from the artifact or `IDataPlaneAuthoritySource`, refreshed every `StoreCacheSeconds`; an unknown id is rejected without SQL; plus a per-endpoint token bucket (`3.3`). + - Document the `4.2` fail-open window in `docs/security.md`. + +**Lane gate:** suites green. Wave 4 docker-dev gate: kill central-1 mid-dispatch, then the next deploy succeeds. Port 4840 taken ⇒ `/health/ready` 503. + +### Lane E — Scripting runtime safety + scripted alarms (fable for E1/E4, opus for E2/E3; review fable) +**Owns:** +- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/**`, `Core.ScriptedAlarms/**`, `Core.AlarmHistorian/**` +- `Runtime/ScriptedAlarms/**`, `Runtime/VirtualTags/**` +- `Host/Engines/**` +- `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/**` +- `Configuration/Validation/DraftValidator.cs` + +**Tests:** `Core.Scripting.Tests`, `Core.ScriptedAlarms.Tests`, `Core.AlarmHistorian.Tests`, `Driver.Calculation.Tests`, `Runtime.Tests` (VirtualTags/ScriptedAlarms folders) + +- **E1 Script quarantine** (`R03:1.1`, `R05:2`, `R07:L1`, `R07:P5`): + - A shared `ScriptRunGuard` in Core.Scripting, keyed per script instance. + - It refuses to start a run while that script's previous orphan is still executing. The result is Bad `"previous evaluation still running"` plus a counter. + - It quarantines after 3 consecutive timeouts: stop evaluating, publish Bad, log Error, metric `otopcua_script_quarantined`, cleared on redeploy or source change. + - Pass a `CancellationToken` into scripts. + - Make the Calculation pump async, so pool starvation is no longer charged to scripts. + - Wired into the scripted-alarm engine, `RoslynVirtualTagEvaluator`/`VirtualTagHostActor`, and `CalculationEvaluator`. + - Tests: a `while(true)` script on a 1 Hz trigger leaves ≤ 1 orphaned thread after 30 triggers (`ThreadPool.ThreadCount` delta). Positive control: a healthy sibling keeps evaluating. +- **E2 Sandbox hardening** (`R05:10`, `R05:11`): + - Deny `System.Threading` with an allow-list (`CancellationToken`, `Interlocked`, `Volatile`). + - Deny `System.Type.GetType`/`GetMethod*`/`InvokeMember` and `System.Runtime.CompilerServices.RuntimeHelpers`. + - Add an allocation budget: sample `GC.GetAllocatedBytesForCurrentThread` around each run and quarantine on breach (`Scripting:MaxAllocBytesPerRun`, default 64 MB). Document that a pre-emptive OOM still needs an out-of-process runner, and file a follow-up issue for that. + - Tests: each forbidden form gets an `OTSCRIPT` diagnostic, and the Monaco editor parity test still matches. +- **E3 Scripted-alarm engine** (`R05:1`, `3`, `6`, `7`, `9`, `12`): + - Clear `_valueCache` in `LoadAsync`; move its write below the `_disposed` guard. + - Bounded `Dispose` drain (5 s, cancelable token). + - `ScriptedAlarmSource` catch logs at Warning. + - `LocalDbStoreAndForwardSink` cached dead-letter counter, resynced with the queue counter. + - Script-log sinks rate-limited per source (20/min + summary). +- **E4 Alarm host restart + virtual-tag log storms** (`R02:H1`, `H3`, `P6`, `L3`; `R07:P4`, `H6`, `H8`, `U5`): + - `ScriptedAlarmHostActor` receives a `Func<(upstream, engine)>` factory, building its engine per incarnation. + - Linked 10 s timeouts on `OnAlarmCommand`. + - `DependencyMuxTagUpstreamSource.RetainOnly(loaded deps)`. + - `VirtualTagActor` per-tag failure-log rate limit. + - Calculation: + - negative-cache compile failures by source hash, with the Warning/script-log only on the Good→Bad transition; + - the mux retains the last `(value,status,ts)` and replays it on `RegisterInterest`, with inputs recorded regardless of `_running`; + - structural array dedup; + - `ClearCompiledScripts` from `ShutdownAsync`; + - a DraftValidator rule `CalculationNoEffectiveTrigger`. + - Tests: + - Throw inside a `DependencyValueBatch` handler, then a later `ApplyScriptedAlarms` loads cleanly. + - A calc Restart republishes the current inputs. + +**Lane gate:** suites green. Wave 4 docker-dev gate: author a `while(true)` virtual tag. The node stays responsive, the tag shows quarantined, and the thread count stays flat over 2 min. + +### Lane F — Galaxy driver (fable; review fable) +**Owns:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy*/**`, the AdminUI `GalaxyDriverForm.razor`. +**Tests:** `Driver.Galaxy.Tests`, `Driver.Galaxy.Browser.Tests` + +- **F1 Real teardown** (`R06:G-1`, `G-2`, `G-9`, `G-3`): + - `ShutdownAsync` stops the pump, alarm feed, deploy watcher, probe watcher and supervisor, disposes the session + clients, and nulls the seams so `InitializeAsync` rebuilds. `DisposeAsync` stays the idempotent final teardown. + - `StartDeployWatcherAsync` stops the previous watcher. + - `ReconnectSupervisor` disposes each cycle's CTS. + - Tests with fake gRPC clients: + - After `ShutdownAsync`, every stream is cancelled and both channels are disposed. + - Five watchdog cycles leave exactly one DeployWatcher (positive control: rediscovery still fires once per deploy). +- **F2 Probe watcher + liveness:** + - Register probe handles in `SubscriptionRegistry` under a reserved id, and `Rebind` them in `ResubscribeAllAsync` (`G-4`). + - `GalaxyDiscoverer` returns its fetched hierarchy, so discovery fetches once (`G-7`). + - A throttled `LastSuccessfulRead` stamp from `OnPumpDataChange` (`G-11`). + - An `EventPump` dispatch-loop exit ⇒ `NotifyStreamFault` (`G-10`). + - Test: a `ScanState` event updates `GetHostStatuses` and survives a reconnect. +- **F3 Wire knobs + writes:** + - `DiscoverPageSize` → `DiscoverHierarchyAsync`. If the mxaccessgw RPC lacks a page parameter, add a cross-repo task in mxaccessgw and page client-side meanwhile. + - `ProbeTimeoutSeconds` → probe deadline (`G-5`, `G-6`). + - First-touch write pipeline: concurrent AddItem/Advise across distinct tags, bounded at 8 (`G-8`; acks stay serial by design). + +**Lane gate:** suites green. Wave 4 userGate: with a reachable mxaccessgw, 20 config deploys keep `mxgateway.sessions.open` flat, read from the gw metrics endpoint. + +### Lane H1 — HistoryRead raw cap (fable; review fable + paging fuzz) +**Owns:** `OpcUaServer/HistoryPaging.cs`, `RawPageLimit.cs`, the `ServerHistorian` options type, and the `ReadRawAsync` methods of the three `*HistorianDataSource.cs` files. +**Tests:** `OpcUaServer.Tests` (paging), `Historian.Conformance`, the backend `*.Tests` + +- `NumValuesPerNode = 0` ⇒ server page size `ServerHistorian:MaxRawValuesPerRead` (default 10 000), with a continuation point via the existing `RawPageLimit` paging. The cap is advertised as `HistoryServerCapabilities/MaxReturnDataValues`. +- Backend `TOP`/`LIMIT`/`max_values` are bounded by the same cap (`R08:1.1`). +- A paged-vs-unpaged fuzz test: random windows/caps against a wire-faithful fake for each backend, where the concatenated pages equal the unpaged truth, with boundary ties. + +**Lane gate:** the fuzz runs 10k iterations green. The Wave 4 SqlServer/QuestDb fixture gate reads a year window at N=0 and gets pages of ≤ 10k with a CP, and heap stays < 500 MB. + +--- + +## Wave 2 — driver lanes (8 parallel; opus implementer, opus review) + +Every driver lane also: +- (a) makes `ShutdownAsync` a complete teardown; +- (b) applies the Global health-contract checklist; +- (c) build-and-swaps device tables; +- (d) adds `IBatchSubscribable` where it runs `PollGroupEngine` (`R02:P1`). + +Each lane owns `src/Drivers/*/**` + its `tests/Drivers/*`, its AdminUI tag-config editor/driver form files, and its `Cli/.Cli`. + +### Lane AbCip +- Probe classification: `NotFound` ⇒ reachable, with a config warning (`R07:H10`). +- Widen `IsReachableStatus`, cache the runtime with its refusal status, map 0x1C/0x20/0xFF-0x2104 explicitly (`H1`, `L4`). +- Native window ≥ 64 (`P8`); cold waiters don't hold slots (`P7` addendum). +- Bit-in-word parent width from `GetSize()` (`H17`). +- Write-side health no longer latches or overwrites (`H19`). +- BOOL-array element decodes bit n%32 of DWORD n/32 (`U11`). +- **Array write encode** (`U10`). +- UDT layout cache (`P11`). +- Device-table swap (`H13`); `volatile _health` (`H21`). +- `DisposeAsync` instead of sync-over-async (`F10`). + +### Lane AbLegacy +- Per-device `ConnectionBackoff` + single-flight connect + bounded parallel devices + adjacent-element coalescing (`R07:P7`). +- `NotSupportedException` ⇒ `BadNotSupported`, no health, reject at `FromTagConfig` (`H16`). +- Table swap (`H13`); prune semaphores on index change (`L7`). +- **Array write encode** (`U10`); `IBatchSubscribable`. +- Document the PLC-5 write capability gap with a hardware follow-up issue (`U6`). + +### Lane TwinCAT +- Per-registration isolation with dead-registration retry (`R07:H11`). +- Stamp `LastSuccessfulRead` from notifications + probe, and degrade on N consecutive router failures (`H12`). +- Warn once per tag (`P10`). +- Parent-width bit RMW (`H18`). +- **Array + string writes via the codecs** (`U9`). +- `BadNodeIdUnknown` for unresolved native refs (`U12`). +- Bounded probe join, drop counter, queue purge on dispose, re-init table swap (`L8`); `IBatchSubscribable`. + +### Lane FOCAS +- `ConcurrentDictionary`/swap `_devices` (`R07:H13`). +- Widen macro/param numbers to int on the wire client (`H14`). +- Fixed-tree loop writes health (`H15`). +- Reject `String` in the factory + editor (`U8`). +- Concurrent per-device reads (`P9`). +- Recycle under `ConnectGate` (`L6`). +- Alarm-layout heuristic uses the record count from the header (`H20`). + +### Lane Modbus +- Deadband baselines on `(value,status)` (`R07:H2`). +- Transport self-heals with a throttled in-gate connect (`P3`). +- Dispose the failed `TcpClient`, one in-gate attempt, DNS under the timeout (`L3`). +- **Wire Family/MelsecSubFamily/DisableFC23** (`U1`): the editor + factory route the address through `ModbusAddressParser` and persist the resolved region/offset; `DisableFC23` gates multi-write. +- Coalescing on by default (gap 8), plus coils/DI/BitInRegister folding (`U2`). +- String[]/BitInRegister[] writes (`U3`). +- Region/type validation at the factory (`P6`). +- Exception codes 01/02/03 are refusals (`H4`). +- Write status mapping (`H7`); PDU length check (`H9`). + +### Lane S7 +- Dead-handle short-circuit inside a pass (`R07:P1`). +- Per-member span fallback + "do not group" memo (`P2`). +- Per-tag skip + `TryParse` in `FromTagConfig`/Inspect, so the deploy gate refuses bad tags (`H3`). +- Non-fatal `ReadData`/`WriteData` are refusals (`H4`). +- `_lifecycleGate` (Modbus pattern) (`H5`). +- Bool-array bit-offset reject at authoring, or bit-packed decode (`U4`). +- Probe gate contention ≠ Stopped; honour `TimeoutMs` (`H7`). + +### Lane G1 — OpcUaClient + MTConnect +**OpcUaClient:** +- Data-subscribe cleanup on throw (`R06:O-1`). +- Ack rethrows the transport status (`O-8`). +- Split the read/write gate from subscription management; no gate across the recursive browse (`O-6`). +- Close the local session on an init throw (`O-2`). +- One teardown budget (`O-9`). +- Wire `ProbeTimeoutSeconds` (`O-4`). +- Browse `AttributesAsync` real + certificate-auth browse (`O-5`). + +**MTConnect:** +- Generation guard on cursor publish (`T-1`). +- Read through the live index when the pump is healthy (`T-4`). +- Lag metric + adaptive `count` (`T-5`). +- Instance-id check in `ReadAsync` (`T-2`). +- Delete dead parsed fields (`T-3`). +- Batched PUT (`T-6`). +- Headers-only probe (`T-7`). +- Redacted URI everywhere (`T-8`). + +### Lane G2 — MQTT + Sql +**MQTT:** +- Failed SUBSCRIBE ⇒ pending set retried on reconnect/reconcile, `SubscribeAsync` throws on a transport failure (`R06:M-1`). +- **Wire `birthObservationWindowSeconds`** into `MqttDriverBrowser.OpenBudget` + discovery UntilStable (`M-4`). +- Array leaves not bindable, plus a Sparkplug-array-read feature issue (`M-5`). +- Prune `LastValueCache` on Register (`M-2`). +- Dispose the failed connection (`M-3`). +- Wildcard topic trie (`M-6`); static UTF-8 encoder (`M-7`). +- Bounded concurrent publishes (`M-8`); guards (`M-10`, `M-11`). + +**Sql:** +- Row cap with a `TOP/LIMIT N+1` dialect + config-error status (`S-1`). +- Rate-limited warnings (`S-6`); plan cache (`S-7`). +- Dispose `_gate` (`S-3`). +- `Query` model refused at the deploy gate via the parser + feature issues for MySql/Odbc/allowWrites (`S-4`). +- `IS_NULLABLE` in browse attributes (`S-5`). +- Withhold JSON parser text (`S-8`); `IBatchSubscribable`. + +**Driver lane gates:** +- Wave 4 fixture runs on `10.100.0.35`: modbus `standard`, s7 `s7_1500`, abcip `controllogix`, opcuaclient opc-plc, mqtt + sparkplug, sql. Each lane adds or extends an IntegrationTests case per High finding. +- Hardware userGates for real controllers: AbCip/AbLegacy/TwinCAT array writes, FOCAS macro > 32767, AbCip probe-typo recovery. + +--- + +## Wave 3 + +### Lane D — AdminUI (opus for D1–D2, sonnet for D3–D4; review opus) +**Owns:** `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/**`, except the per-driver editors/forms owned in Wave 2 and the A6 diagnostics panel. + +- **D1 Monaco sync correctness** (`R04:P1`, `P9`, `S2`, `S3`): + - Fetch the editor text on demand at Save/blur via `MonacoBlazor.getValue` instead of per keystroke. + - `HubOptions.MaximumReceiveMessageSize` = 1 MB. + - Diagnostics generation counter + `AbortController`. + - Drop the inlay-hints provider/endpoint. + - Build a Problems list using `MarkersChanged` + `RevealLineAsync`. +- **D2 Roslyn analysis** (`P2`, `P3`, `S1`): + - One compilation per scoped service with `ReplaceSyntaxTree`. + - `RequestAborted` plumbing + Monaco token honouring. + - Sorted `string[]` + dictionary in the catalog snapshot. + - An `OTSCRIPT_SETVTAG_INERT` warning marker. +- **D3 Rendering** (`P4`–`P8`, `H2`): + - `/hosts` dirty flag on tick. + - `RawTree` cached menus + `Virtualize` + 500-per-level paging. + - CSV import grid `Virtualize` + export via a download endpoint. + - ScriptLog/Alerts 200 ms coalescing + ring buffer. + - `/scripts` projection. + - Cached modal parameter dictionaries. +- **D4 Leaks/hazards/stubs** (`L1`–`L5`, `H1`, `H3`, `S4`–`S7`): + - Snapshot-store eviction against the config load. + - `Threading.Timer` in `DriverTestConnectButton`. + - `AlarmsHistorian` page `Fleet.razor` pattern + "no historian on role" state. + - Close an orphaned browse token after dispose. + - Per-subscriber try/catch in broadcasters. + - Confirm dialogs on equipment-page and ScriptEdit deletes. + - Document the theme (`H3`). + - Delete dead `UnsTreeService` members. + - `GET /api/deployments/{id}`. + - Equipment CSV via `CsvParser`. + - Gate `/dev/*` behind `#if DEBUG`. + +**Lane gate (userGate-free, coordinator-run):** docker-dev Chrome pass. +- Paste a 100 KB script, Save, reload: the source persists. +- Two-tab `/hosts` with 50 drivers: CPU stays flat. +- Expand a 5k-tag device: renders in < 2 s. +- Each delete confirms. +- The A6 diagnostics panel shows real counts. + +### Lane H2 — Historian remainder (opus; review opus) +**Owns:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.*/**` (minus H1's files), `Runtime/Historian/**` (recorder + adapter), `docs/Historian.md`, the AdminUI historian backend picker. + +- Gateway events: drain to completion with a budget, cancellation arms (`R08:1.2`, `2.5`). +- Drop native `TimeAverage`/`Total` behind `ServerHistorian:Gateway:EnableNativeTimeWeighted=false` until historiangw#27 (`2.1`). +- Capability matrix in docs + picker + a startup warning for continuous historization on Gateway (`2.2`). +- `ProbeAsync` health timer (`2.4`). +- Writers: + - OCE rethrow in all three (`4.1`). + - Gateway via `LiveWritePreparer` (`4.2`). + - `IHistorianValueWriter.FlushAsync` pass-end hook, QuestDB WAL wait once per pass with exponential poll (`3.1`). +- Batched alarm-history writers: SqlServer TVP + `OUTPUT`; QuestDb multi-row; Gateway bounded pipelining until a batch RPC (`3.2`). +- Retention pre-filter (`3.3`). +- QuestDB symbol indexes + migration (`3.4`). +- Presence cache exact lookup (`3.5`). +- Paged `DeleteRaw` (`4.3`). +- QuestDB trail `applied` marker + DEDUP key (`4.4`). +- Recorder: `CommitMode=Periodic(100 ms)` default + off-mailbox append with an in-flight cap + mailbox-depth metric (`R02:P3`); timeouts on `GetStatus` (`R02:H3`). +- `HistorianAdapterActor` bounded channel + delete the senderless receive (`R02:H4`, `U3`). +- Outbox scan outside the lock (`R08:1.3`). + +**Lane gate:** `Historian.Conformance` green on all backends. The Wave 4 AVEVA live gate (userGate, existing recipe in `.claude/rules/historian.md`) checks events paging and that `TimeAverage` is refused. + +### Lane K — Clients, CLI, transport (sonnet; review opus) +**Owns:** `src/Client/**`, `src/Drivers/Cli/Driver.Cli.Common` + the AbCip/AbLegacy CLI command files (after the Wave 2 merge), `Driver.Ab.Transport/**`. + +- Client: + - release the raw CP (`R08:1.4`); + - `RemoveSubscription` (`1.5`); + - align `AggregateType` with `AggregateFamilies.Served` (`2.3`); + - lazy HasChildren (`3.6`); + - NodeId dictionary in the UI (`3.6`); + - failover under `_subscriptionLock`, and `ConnectAsync` refusing when already connected (`4.5`). +- CLI fixes (`R07:U7`). +- Pooled buffers in `EncapsulationClient`/`CipWriter`/`LogixTagClient` + Modbus transports via `ArrayPool` (`R07:L5`). Benchmark-gated: no regression in `Ab.Transport.Tests` timings. + +### Lane B2 — HistoryReadEvents full projection — folded into Lane H2 as its LAST task (opus; review fable) +This runs inside H2 because it shares the backends' event read paths with H2's Gateway events-drain fix. It runs after H2's other tasks, and `OtOpcUaNodeManager.cs` is free by then because B merged in Wave 1. +**Adds to H2's ownership:** the `HistoricalEvent` contract in Core.Abstractions and the HistoryReadEvents projection in `OtOpcUaNodeManager.cs`. +- Carry `EventType`/`ConditionName`, map `SourceNode` when resolvable, and honour an `OfType` where-clause (`R01:U1`). +- Test over the wire: a Client.CLI `historyread --events` with a select clause including EventType returns non-null values. + +### Lane E5 — Alarm comment audit table (opus; review fable) +**Owns:** `Core.ScriptedAlarms/Part9StateMachine.cs`, the alarm state store + its LocalDb schema/migration. +- Move the append-only comments to their own table keyed by alarm id, keeping replication to the pair peer. Each condition row stays a fixed size. +- Migrate existing inline comments idempotently (`R05:4`). +- Test: 10k comments, then a transition's `SaveAsync` payload size is constant. A pair-replication test shows the comments on the peer. + +--- + +## Wave 4 — integration (serial, coordinator) + +### Task INT-1: Full suite + flake triage +Run the full suite again. Any new failure versus `baseline.txt` is either fixed or filed. Loop any suspect test under `yes` contention. + +### Task INT-2: Fixture gates (sims on 10.100.0.35) +For each driver lane, sync and bring up its fixture, then run `*.IntegrationTests` with the env endpoints. Record the results in `docs/reviews/2026-10-21/gates.md`. + +### Task INT-3: docker-dev live gates (coordinator, Chrome automation + Client.CLI) +This runs the lane gates for A, B, C, D and E listed above, plus the A6 `/fleet` panel and the H1 N=0 paging read through Client.CLI `historyread`. + +### Task INT-4 (userGate): hardware and AVEVA gates +> **USER-ORDERED GATE — NON-SKIPPABLE.** This task was requested by the user in the current conversation (array encode on real controllers). It MUST NOT be closed by walking around it, by declaring it "verified inline", or by substituting a cheaper check. Close only after every item in `acceptanceCriteria` has been re-validated independently, with output captured. + +Acceptance criteria: +- An AbCip array write round-trips on a real ControlLogix. +- An AbLegacy N-file array write round-trips on a real SLC/MicroLogix. +- A TwinCAT array + STRING write round-trips on real hardware. +- A FOCAS `MACRO:` > 32767 reads the correct variable. +- Galaxy: 20 deploys keep `mxgateway.sessions.open` flat. +- AVEVA: the events paging + `TimeAverage` refusal gate. + +`requiresUserSpecification: true` for the bench endpoints. + +### Task INT-5: Docs, rules, index, push-on-OK +- Apply each lane's suggested `.claude/rules/*.md` text in the main checkout. +- Update `CLAUDE.md` where behaviour summaries changed: the health contract, surgical/rebuild notes, the historian N=0 rule. +- Update `../scadaproj/.claude/skills/sister-project-history/otopcua.md` (+ the index row only if the summary changes) on scadaproj `main`, following the CLAUDE.md recipe, and push scadaproj. +- Draft the per-issue closing comments, then ask the user for OK before pushing OtOpcUa master and closing the issues. +- Save project memory for lessons learned. + +--- + +## Verification (end to end) + +1. `dotnet build ZB.MOM.WW.OtOpcUa.slnx -c Release` + the full unit suite match the baseline or better. +2. Fixture IntegrationTests green per driver (INT-2). +3. docker-dev gates (INT-3): + - Respawn churn leaves threads/handles flat (`dotnet-counters monitor -p System.Runtime`). + - A re-subscribe after a rebuild receives data. + - The deploy wedge recovers. + - A spin script is quarantined. + - A 100 KB script saves. + - An N=0 HistoryRead pages. +4. Hardware/AVEVA userGates (INT-4). +5. Each Gitea issue checklist has every `R0x:ID` ticked with a commit SHA, or explicitly re-filed as a follow-up. + +## Self-review notes +- Coverage: every finding ID in reports 01–08 maps to a lane above. The two documented-no-change items (`R01:U4`, `R01:L5`) are recorded in the B issue, and `R08:2.2` is gateway-side and handled through cross-repo issues + docs. +- Conflict check: the files shared across lanes are `OtOpcUaNodeManager.cs` (B in Wave 1, then H2's last task in Wave 3), `HistoryPaging.cs` (H1 only), the three `*HistorianDataSource.cs` (H1's `ReadRawAsync` in Wave 1, H2 for the rest in Wave 3), the recorder (H2 only), `DraftValidator.cs` (E only), and AdminUI driver forms (driver lanes in Wave 2, then D in Wave 3). No two lanes in the same wave share a file. +- Concurrency budget: up to 8 worktree builds in Wave 2. If the machine thrashes, split Wave 2 into 2×4: AbCip/AbLegacy/TwinCAT/FOCAS, then Modbus/S7/G1/G2.