Joseph Doherty
125 lines
16 KiB
Markdown
125 lines
16 KiB
Markdown
# 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<Lazy<>>` 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.
|