ad2b5c4044
Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
309 lines
28 KiB
Markdown
309 lines
28 KiB
Markdown
# 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<string>)`).
|
||
|
||
### 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<AlarmHistorianEvent>` 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<WriteAttribute>`, `<Subscribe>`, `<Unsubscribe>`, `<SubscribeAlarms>`),
|
||
`: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<DependencyValueBatch>(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<AlarmHistorianEvent>` 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.
|