lmxopcua/docs/reviews/2026-10-21/05-core.md

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.