Joseph Doherty
298 lines
39 KiB
Markdown
298 lines
39 KiB
Markdown
# 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<object?[]>();
|
||
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<AttributeInfo>()`), `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<SqlTagDefinition, List<int>>` 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`.
|