lmxopcua/docs/reviews/2026-10-21/00-SUMMARY.md

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.