ScadaBridge/archreview/03-site-runtime-dcl.md

# Architecture Review 03 — Site Runtime & Data Connection Layer (Round 2)

**Date:** 2026-07-12
**Scope:** `src/ZB.MOM.WW.ScadaBridge.SiteRuntime` (#3: DeploymentManager singleton, Instance/Script/Alarm/NativeAlarm actors, script execution scheduler, site-wide Akka stream), `src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer` (#4: OPC UA + MxGateway adapters, connection actors, alarm subscription seam, CertStoreActor), `src/ZB.MOM.WW.ScadaBridge.SiteEventLogging` (#12); matched against `docs/requirements/Component-SiteRuntime.md`, `Component-DataConnectionLayer.md`, `Component-SiteEventLogging.md`.
**Method:** Re-verified every round-1 finding against current source (HEAD `8c888f13`) with file:line evidence — plan claims were not trusted; each fix was read in the code. Fresh sweep over `git diff b910f5eb..HEAD` scoped to the three projects (38 files, +2,205/−640) covering the PLAN-03 fix commits, the PLAN-08 options-validator/notification-excision commits, and the 2026-07-10 aggregated-live-alarm-stream site-side piece (`SubscribeSiteAlarms`).
**Round-1 vs round-2 verdict:** Round 1 found a mature subsystem with three concentrated risks (adapter reconnect lifecycle, script containment, deployment honesty); round 2 confirms all three are genuinely fixed in code — 26 of 28 actionable findings verified fixed, 2 deferred with sound rationale — leaving only one Medium residual (a stale MxGateway event-loop fault can still flap a fresh connection through the generic catch) and a handful of Low polish items.

---

## Round-1 Finding Disposition

Every disposition below was verified by direct read of the cited current source, not the plan's claims.

| Finding | R1 severity | Disposition | Evidence (file:line) |
|---|---|---|---|
| S1 — reconnect abandons previous client; stale `ConnectionLost` flaps healthy connection | High | **Fixed (verified)** — with one Medium residual on the MxGateway fault path (new finding N1) | `OpcUaDataConnection.cs:99-118` detaches `ConnectionLost`, fire-and-forget-disposes the previous client with fault logging, and `StopHeartbeatMonitor()` (:107) kills the old `StaleTagMonitor`; `MxGatewayDataConnection.cs:71-91` cancels+disposes the previous event loop/client before the guard reset. Tests exist: `OpcUaDataConnectionTests.cs` (`Reconnect_DisposesAndDetachesPreviousClient`, `StaleClientConnectionLost_…`), `Adapters/MxGatewayDataConnectionReconnectTests.cs` |
| S2 — cooperative-only timeout; no stuck-thread observability | High | **Fixed (verified)** (observability shipped; ADO.NET command-timeout sub-suggestion deferred with rationale — scripts own their commands, 30 s default) | Gauges: `ScriptExecutionScheduler.cs:29,80-113` (`QueueDepth`/`BusyThreadCount`/`OldestBusyAge` via per-worker `_busySinceTicks`, volatile writes in `WorkerLoop` :121-128). Watchdog: `ScriptExecutionActor.cs:131-154` names the script whose thread never came back after timeout+`StuckScriptGraceMs`, logs + site event; `completed` flipped in the finally (:326). Reporter: `ScriptSchedulerStatsReporter.cs:71-74`, registered `ServiceCollectionExtensions.cs:76-79`; health surface `SiteHealthCollector.cs:151-162,260-262`; additive `SiteHealthReport.cs:65-75` |
| S3 — site compile failure doesn't reject deployment (spec drift) | High | **Fixed (verified)** — deliberate deviation: gate is synchronous on the singleton, not off-thread (see N4/N5) | `DeployCompileValidator.cs:37-116` compiles every script, alarm on-trigger script, and Expression trigger (malformed JSON is itself an error, fronting the poison-config case); gate at `DeploymentManagerActor.cs:508-537` replies `DeploymentStatus.Failed` with the collected errors and returns before any actor creation or persistence. Synchronous-on-mailbox rationale documented in code (:510-521): off-thread piping would reorder deploys vs. delete/disable and break the redeploy-supersede FIFO. Test: `DeploymentManagerActorTests.cs` (`Deploy_WithNonCompilingScript…`) |
| S4 — no retry on failed tag subscription; NativeAlarm lost-response gap | Medium | **Fixed (verified)** — stronger than asked: retry arms on *send*, so lost responses are covered too | `InstanceActor.cs:1047-1068` `SendTagSubscribe` arms a per-connection `TagSubscribeRetryIntervalMs` timer on every send with correlation-id supersede; `HandleSubscribeTagsResponse` (:1075-1099) cancels on Success, logs + emits a site event and leaves the timer armed on failure; timers cancelled in PostStop (:296-298). `NativeAlarmActor.cs:114,128,147,319` — response-timeout retry armed in `SendSubscribe`, cancelled on Success |
| S5 — startup SQLite load failure aborts silently, no retry | Medium | **Fixed (verified)** | `DeploymentManagerActor.cs:286-290,345-356,400-415` — failed load schedules `RetryStartupLoad` at a fixed interval (default 5 s, ctor-overridable), unlimited, with a startup-complete flag so a stale retry cannot re-run after success |
| S6 — dead child ref in `_instanceActors` + Success reported for dead instance | Medium | **Fixed (verified)** — redesigned beyond the plan: two-signal deploy join, not swallow-only | Every Instance Actor is watched (`DeploymentManagerActor.cs:1995`); unexpected `Terminated` evicts the dead ref (:618+). Deploy `Success` requires `InFlightDeploy.Persisted && Initialized` (:872-892, class :2093-2104) where `Initialized` is the new `InstanceActorInitialized` readiness message sent at the end of `InstanceActor.PreStart` (`InstanceActor.cs:280-286`, `Messages/InstanceActorInitialized.cs`); init-failure row rollback is deferred until the store commits (`_initFailedPendingRowRemoval`, :135,657,824) so it can never race the optimistic write. Design-doc capture: `Component-SiteRuntime.md:109` |
| S7 — background tasks read mutable `_adapter` field off-thread | Medium | **Fixed (verified)** | Adapter captured into a local on the actor thread before every background task: `DataConnectionActor.cs:704` (subscribe), `:1155` (write), `:1199` (trigger write), `:1638` (re-subscribe, symmetric with the pre-existing `reseedAdapter`) |
| S8 — site SQLite: no WAL/busy-timeout | Low | **Fixed (verified)** | `SiteStorageService.cs:23` (busy-timeout floor pinned in the normalized connection string), `:63-74` (`PRAGMA journal_mode=WAL` set once in `InitializeAsync`, warning logged on fallback) |
| S9 — fire-and-forget adapter dispose in `PostStop` unobserved | Low | **Fixed (verified)** | `DataConnectionActor.cs:232-237` — `ContinueWith(OnlyOnFaulted, log)` on the dispose task |
| S10 — alarm condition filter last-subscriber-wins, silent overwrite | Low | **Fixed (verified)** | `DataConnectionActor.cs:1840-1851` — warning logged when an incoming filter differs from the stored one; filter also parsed once into `_alarmSourceFilterPredicate` (:126) |
| P1 — trigger expressions block dispatcher threads per attribute change | High | **Fixed (verified)** — with one Low residual (new finding N2) | `ScriptActor.cs:281-334` and `AlarmActor.cs:520-577`: evaluation runs on `ScriptExecutionScheduler.Shared` against a point-in-time snapshot, result piped back as `ExpressionEvalResult`, edge state (`_lastExpressionResult`, WhileTrue timers) applied only on the actor thread, bursts coalesced (one in flight + one pending) |
| P2 — attribute-change fan-out broadcast to every child | Medium | **Fixed (verified)** | New `Scripts/TriggerRouting.cs` (three-way monitored/all-changes/none classification, fail-open on unparseable config); `InstanceActor.PublishAndNotifyChildren` (:1270-1289) routes to `_allChangeChildren` + `_childrenByMonitoredAttribute[attr]` only; `RouteChild` files children at creation (:1451-1469, wired at :1516/:1576) |
| P3 — DCL tag-value fan-out scans all instances per update | Medium | **Fixed (verified)** | `DataConnectionActor.cs:1745-1764` `IndexTag`/`UnindexTag` maintain the `_instancesByTag` reverse index at every subscription mutation site (incl. instance release, :1076); `HandleTagValueReceived` (:1778-1790) fans out O(subscribers-of-tag) |
| P4 — per-transition SQLite upserts on native-alarm mirror | Low | **Fixed (verified)** | `NativeAlarmActor.cs:57-70,115,469-476` — latest-transition-per-source coalescing map, batched flush on a 100 ms timer (`FlushPersistence`), best-effort final flush in PostStop (:139-143) |
| P5 — per-attribute script read is an Ask round-trip | Low | **Deferred (accepted)** | `ScriptRuntimeContext` unchanged on this path; plan rationale (spec-consistent, no profiling evidence, batch `GetAttributes` as follow-up) holds |
| P6 — Roslyn compiles inside `InstanceActor.PreStart` during staggered startup | Low | **Deferred (accepted, partially mitigated)** | Compiles still run in `PreStart` → `CreateChildActors` (`InstanceActor.cs:260-280,1472-1499`); the deploy-path gate (S3) validates earlier but PreStart recompiles — see new finding N4. Risk remains failover time-to-recover only |
| C1 — positive: Akka discipline | — | Still true; the new code keeps the discipline (PipeTo everywhere, captured `Self`, actor-confined state) | e.g. `ScriptActor.cs:289` captured `self`; `DataConnectionActor.cs:704` captured adapter |
| C2 — stale "unbounded channel" comment in SiteEventLogger | Low | **Fixed (verified)** | `SiteEventLogger.cs:21-22,69,215-216` — comments now correctly describe bounded/DropOldest semantics |
| C3 — spec self-contradiction on reconnect-interval scope | Low | **Fixed (verified)** | `Component-DataConnectionLayer.md:304` — "shared setting for all data connections (`ReconnectInterval` in the Shared Settings table)" |
| C4 — quality-only change not delivered to children vs. spec wording | Low | **Fixed (verified)** | `Component-DataConnectionLayer.md:303` — spec now states children are "deliberately NOT re-notified on a quality-only change" with the spurious-firing rationale; divergence is now a documented decision |
| C5 — shared scripts run Root scope, undocumented | Low | **Fixed (verified)** | `Component-SiteRuntime.md:344` — "Shared scripts always execute with Root scope (C5)… Callers needing module-scoped access must pass the values in as parameters" |
| C6 — `EvaluateCondition` fallback uses current-culture `ToString` | Low | **Fixed (verified)** | `ScriptActor.cs:513-516` — threshold rendered with `CultureInfo.InvariantCulture`, `StringComparison.Ordinal` |
| UA1 — cert-trust convergence after node downtime | Underdev. | **Fixed (verified)** — additive reconcile-on-join; removal-reconcile + cross-node list aggregation remain deferred to the documented central-persistence follow-up | `CertStoreActor.cs:139+` exports the trusted set (thumbprint+DER); `DeploymentManagerActor.cs:277-281,321-328` singleton subscribes to `ClusterEvent.MemberUp`; `:1296-1365` re-reads the local store and pushes each cert to the joiner's CertStoreActor (idempotent by thumbprint, additive union only, fire-and-forget with heal-on-next-join). Divergence direction is always singleton→joiner since trust commands flow through the singleton, so the additive push closes the real hole |
| UA2 — `ConfigFetchRetryCount` dead option | Underdev. | **Fixed (verified)** | `SiteRuntimeOptions.cs:68` (default 3), consumed at `SiteReplicationActor.cs:86` (floor 1) with a fixed 2 s inter-attempt delay (:270); validated ≥ 0 (`SiteRuntimeOptionsValidator.cs:56-58`) |
| UA3 — no aggregated live alarm stream | Underdev. | **No longer applicable — shipped post-plan (2026-07-10)** | Deferred by PLAN-03 as central-scope; the aggregated-live-alarm-stream plan delivered it. Site-side piece reviewed: `SiteStreamManager.SubscribeSiteAlarms` (`SiteStreamManager.cs:134-159`) — alarm-only filter off the same BroadcastHub, per-subscriber DropHead buffer, KillSwitch teardown shared with `Unsubscribe`/`RemoveSubscriber`; clean |
| UA4 — native-alarm rehydration loses display metadata | Underdev. | **Fixed (verified)** | `SiteStorageService.cs:174-175,975` — `MetadataJson` (type/category/message/values) persisted alongside `condition_json` and restored on rehydration |
| UA5 — no scheduler stuck-thread/queue-depth observability | Underdev. | **Fixed (verified)** (same fix as S2) | See S2 row |
| UA6 — tag-subscription failure path has no retry/site event | Underdev. | **Fixed (verified)** (same fix as S4; site event emitted at `InstanceActor.cs:1093-1096`) | See S4 row |
| UA7 — test gaps (reconnect-stale-handler, compile-fail deploy status, subscribe race) | Underdev. | **Fixed (verified)** | `OpcUaDataConnectionTests.cs` (both S1 tests present), `Adapters/MxGatewayDataConnectionReconnectTests.cs`, `DeploymentManagerActorTests.cs` (compile-fail deploy + `InstanceActorInitialized` handshake tests) |

**Disposition counts:** 26 Fixed (verified) · 2 Deferred (accepted: P5, P6) · 0 Not fixed · 0 Regressed. (UA3 counted Fixed via the post-plan 2026-07-10 delivery; UA1's removal-reconcile sub-scope remains an explicitly documented deferral inside an otherwise-fixed item.)

---

## New Findings (Round 2)

### N1. [Medium] MxGateway: a stale event loop's *non-OCE* fault can still flap the fresh connection — the S1 fix closed the OPC UA path but left this one open
The S1 fix in `MxGatewayDataConnection.ConnectAsync` cancels the previous loop's CTS and disposes the previous client (`MxGatewayDataConnection.cs:77-89`), then resets the disconnect guard on the very next line (`Interlocked.Exchange(ref _disconnectFired, 0)`, `:91`) — *before* the old loop has actually observed the cancellation. The comment claims a teardown fault "is absorbed by the old guard cycle", but `Cancel()` is not synchronous with the loop's exit: the old loop's `await foreach (… _session!.StreamEventsAsync(_lastSeq, ct))` (`RealMxGatewayClient.cs:278-294`) surfaces its failure milliseconds later, while the new `ConnectAsync` is still awaiting. `RunEventLoopAsync`'s catch (`MxGatewayDataConnection.cs:121-129`) only absorbs `OperationCanceledException`; a cancelled/disposed gRPC stream commonly faults with `RpcException(StatusCode.Cancelled)` (Grpc.Net default without `ThrowOperationCanceledOnCancellation`, which the wrapped `MxGatewayClient` package is not shown to set) or `ObjectDisposedException` from the concurrent `DisposeAsync` — both fall into the generic `catch` → `RaiseDisconnected()` against the *reset* guard → `AdapterDisconnected` → reconnect → which cancels/disposes again → potentially self-sustaining churn, the exact S1 scenario-2 mechanism. Unit tests can't catch it (the reconnect test substitutes the client and never faults the loop), and there is no real gateway in the local env.
**Fix shape:** in the generic catch, treat `ct.IsCancellationRequested == true` as normal shutdown (no `RaiseDisconnected`). Secondary hardening in the same method: `Task.Run(() => RunEventLoopAsync(_eventLoopCts.Token))` (`:108`) reads the `_eventLoopCts` *field* lazily — a rapid double-reconnect can hand loop #1 loop #2's token; capture the token into a local before the lambda.

### N2. [Low] Trigger-expression `PipeTo` has a success mapping only — a faulted scheduler task permanently kills the actor's expression trigger
`ScriptActor.StartExpressionEvaluation` (`ScriptActor.cs:311-313`) and `AlarmActor.StartExpressionEvaluation` (`AlarmActor.cs:552-554`) pipe the evaluation back with `PipeTo(self, success: r => new ExpressionEvalResult(r))` and no `failure:` mapping. The inner body catches all exceptions and returns `false`, so the only fault sources are the outer `Task.Factory.StartNew`/`QueueTask` itself (e.g. `ObjectDisposedException` from a disposed `ScriptExecutionScheduler` during shutdown or in test teardown). If that ever fires, the actor receives an unhandled `Status.Failure` and — critically — `_evalInFlight` stays `true` forever (`ScriptActor.cs:284-285`, `AlarmActor.cs:523-524`), so every subsequent attribute change parks in `_evalPending` and the expression trigger is dead for the actor's lifetime with no log. One-line fix: add `failure: ex => new ExpressionEvalResult(false)` (or a dedicated failure message that logs).

### N3. [Low] New PLAN-03 option knobs are missing from the eager options validator this same initiative introduced
`SiteRuntimeOptionsValidator.cs:20-58` validates ten options, but not the two PLAN-03 added: `TagSubscribeRetryIntervalMs` (`SiteRuntimeOptions.cs:76`, drives `ScheduleTellOnceCancelable` in `InstanceActor.SendTagSubscribe` `:1065-1067` — a 0 hot-loops the subscribe retry, a negative value throws inside the actor) and `StuckScriptGraceMs` (`SiteRuntimeOptions.cs:86`, a negative value throws `ArgumentOutOfRangeException` inside the watchdog's `Task.Delay`, `ScriptExecutionActor.cs:144`). Inconsistent with the PLAN-08 §1.5 "fail fast at boot with a key-naming message" convention; two `RequireThat` lines close it.

### N4. [Low] Every deploy now compiles each script twice — synchronously on the singleton mailbox at the gate, then again in `InstanceActor.PreStart` — with no compile cache
The S3 gate runs `_deployCompileValidator.Validate` on the singleton's actor thread (`DeploymentManagerActor.cs:522`; deliberate and well-argued for mailbox-FIFO ordering, :510-521), and the created Instance Actor then recompiles the identical scripts/expressions in `CreateChildActors` (`InstanceActor.cs:1485,1497-1498`). `ScriptCompilationService` has no compile cache, so a site-wide redeploy of N instances serializes roughly 2×N script-set Roslyn compiles through the singleton + instance start path (each 50–300 ms). Single-instance deploys are fine; bulk redeploy latency and central Ask timeouts are the exposure. The master tracker already earmarks adopting PLAN-05's compile-cache abstraction for `DeployCompileValidator` ("later refactor, not a blocker") — this finding is the concrete reason to actually do it, and a keyed cache would also shrink the P6 failover-recompile cost for free.

### N5. [Low] Spec drift (reintroduced): `Component-SiteRuntime.md` says the compile gate runs "off-thread" — the shipped gate is deliberately synchronous
`Component-SiteRuntime.md:489`: "…the Deployment Manager compiles all of the instance's scripts … **off-thread**; any compile failure rejects the deployment…". The implementation intentionally validates *on* the singleton thread (`DeploymentManagerActor.cs:510-522`), a deviation captured in the master tracker and the in-code comment but never folded back into the component doc. Given this repo's docs-travel-with-code rule — and that round 1's S3 was itself a doc-vs-code honesty finding — the sentence should say "synchronously, as a pure prefix step of the deploy handler (preserving mailbox FIFO ordering)".

### N6. [Low] Observation: trigger expressions now share the 8-thread bounded script scheduler with (possibly stuck) script bodies — saturation silently stalls all Expression triggers and Expression alarms site-wide
The P1 fix routes every expression evaluation through `ScriptExecutionScheduler.Shared` (`ScriptActor.cs:312`, `AlarmActor.cs:553`). Before, a pathological expression stalled a dispatcher thread (bad) but expressions always ran; now, eight stuck script bodies (the S2 scenario) also mean **no Expression-triggered script or alarm evaluates anywhere on the site** until a thread frees — alarm activation latency becomes unbounded during scheduler saturation. The coalescing (one in-flight + one pending per actor) correctly bounds queue growth, and the new gauges/watchdog make the state visible, so this is an acceptable and arguably correct trade-off — but it is a behavioral coupling the component doc's Alarm section should state (one sentence: "Expression alarm evaluation shares the bounded script-execution pool; scheduler saturation delays alarm transitions").

---

## What's Genuinely Good

- **The fix quality matches the codebase's standard.** Every PLAN-03 fix reads like the surrounding hardened code: retries use the existing fixed-interval cancelable-timer idiom with supersede correlation ids (`InstanceActor.cs:1047-1068`), all off-thread work returns via `PipeTo` with state applied on the actor thread, and health metrics ride the additive-only `SiteHealthReport` contract.
- **Two fixes shipped *better* than the plan asked.** S4's retry arms on *send* (covering lost responses, not just failure replies — the plan's sub-gap), and S6 was redesigned mid-execution when the implementer discovered the store commits before `PreStart` throws: the `InstanceActorInitialized` two-signal join (`DeploymentManagerActor.cs:2085-2104`) plus commit-ordered rollback is deterministic where the plan's swallow-only guard would have silently kept the bug. The deviation is documented in the master tracker *and* the component doc (`Component-SiteRuntime.md:109`).
- **The S3 synchronous-gate decision is correctly reasoned.** The in-code comment (`DeploymentManagerActor.cs:510-521`) explains precisely why off-thread validation would break the redeploy-supersede/delete ordering that depends on mailbox FIFO — the kind of concurrency reasoning this file consistently gets right. (It just needs the doc sentence fixed — N5.)
- **Instrumentation is lock-free and cheap.** The scheduler gauges use per-worker `Volatile` slots read without locks (`ScriptExecutionScheduler.cs:29,83-113`); the stuck-watchdog deliberately runs its grace delay on the thread pool, not the possibly-saturated script scheduler (`ScriptExecutionActor.cs:138-139`).
- **The reverse indexes are maintained at every mutation site.** Both P2's child-routing maps (filed at child creation, `InstanceActor.cs:1451-1469`) and P3's `_instancesByTag` (add/remove helpers called from subscribe, unsubscribe, and instance-release paths incl. `:1076`) show the discipline that makes index-based fan-out safe.
- **`TriggerRouting` fails open.** Unparseable trigger configs route to the all-changes bucket rather than silencing a child (`TriggerRouting.cs:81-105`) — routing is an optimization, the child's own gate stays the correctness check, exactly the right layering.
- **Docs kept pace.** C3/C4/C5 spec fixes landed, the S6 deviation is in the component doc, and the shared-script Root-scope surprise is now a documented contract (`Component-SiteRuntime.md:344`).

---

## Severity Tally (new findings only)

| Severity | Count | Findings |
|---|---|---|
| Critical | 0 | — |
| High | 0 | — |
| Medium | 1 | N1 |
| Low | 5 | N2, N3, N4, N5, N6 |

Round-1 residue: P5 and P6 remain deferred with accepted rationale (P6's cost is now partially addressable via N4's compile cache); UA1's removal-reconcile/list-aggregation sub-scope stays parked behind the documented central-persistence-of-cert-trust follow-up (`Component-SiteRuntime.md:125`).