docs: whole-system architectural/code review 2026-09-09 (83 findings, 8 High; 9-WP remediation program)
Eight parallel area reviewers + orchestrator verification at main @ d8229916.
All 12 CLAUDE.md design commitments hold. Highs: DC-1 no unsubscribe ever sent,
DC-2 redundant Mx handle release freezes live tag, DC-3 device writes stashed
across reconnect, SR-1 sync Attributes[] blocks script pool, CM-1 no gRPC
message-size limit, MG-1 trust deny-list gap (CompilerServices/Loader), UI-1 UI
config edits bypass IAuditService, UI-2 session ping defeats idle timeout.
Suite: build 0/0, 9231 passed / 1 failed (ORCH-1 OperationTrackingStore
sync-dispose race). No code changed by this review.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,421 @@
|
||||
# ScadaBridge — architectural / code review (whole system)
|
||||
|
||||
**Date:** 2026-09-09 · **Status:** REVIEW REPORT (no code changed by this review)
|
||||
**Scope:** all 26 `src/` projects + rig config (`docker/`, `docker-env2/`) + docs, at `main` @ `d8229916`
|
||||
(Flow Debugger v1.4 merge). The last whole-system review was 2026-08-14
|
||||
(`2026-08-14-arch-review-remediation-plan.md`); 511 commits have landed since — the remediation
|
||||
waves, LocalDb 0.3.x, Lever 1, inbound standby relay, the shared inbound key store, the template /
|
||||
instance pickers, create-instance bindings, and Flow Debugger v1.0–v1.4. Those ranges got the
|
||||
closest read; the standing design commitments were re-verified as well.
|
||||
**Method:** eight parallel read-only reviewers, one per area (Site Runtime + S&F + event log;
|
||||
Communication + Cluster + Host; Management + Deployment + Templates + ScriptAnalysis; Central UI +
|
||||
Security; edge integrations + audit backbone; Commons + ConfigurationDatabase + Transport; DCL + CLI +
|
||||
DelmiaNotifier + docker scripts; cross-cutting), each required to trace every claim through source
|
||||
and tag it CONFIRMED or PLAUSIBLE. The orchestrator independently re-verified every High and the
|
||||
security-boundary Mediums in source (marked *orch-verified*), and ran the full solution build + test
|
||||
suite (§7). Findings are numbered by area prefix; severities follow the 2026-09-06 review's scale.
|
||||
|
||||
Severity: **High** = data loss / security boundary / outage; **Medium** = wrong behaviour an operator
|
||||
will hit, or a documented property that does not hold; **Low** = quality, consistency, narrow edge.
|
||||
|
||||
---
|
||||
|
||||
## 1. Verdict
|
||||
|
||||
The architecture is intact: every design commitment in `CLAUDE.md` that has a code anchor still
|
||||
holds (§6.H — active-node definition, single ActorSystem name, UTC everywhere, no ClusterClient,
|
||||
auto-down emitted, replication exclusions, metadata resolver on all four compile surfaces, no
|
||||
health publisher, no central buffering for unreachable sites, single trust boundary, additive
|
||||
proto evolution). The dependency graph is an acyclic DAG rooted at Commons with zero project
|
||||
references; every mapped gRPC service is behind a fail-closed constant-time interceptor; all
|
||||
contract changes since August are additive; all eight migrations are sound and the model snapshot
|
||||
matches. Test coverage is broad (≈ 7 600 test attributes, 9 231 passing in the run below).
|
||||
|
||||
Underneath that, the review found **83 findings: 8 High, 33 Medium, 42 Low**. The Highs cluster
|
||||
in three places the recent waves did not touch:
|
||||
|
||||
| # | Sev | Finding | Area |
|
||||
|---|---|---|---|
|
||||
| DC-1 | **High** | **Nothing in production ever unsubscribes a tag or alarm source** — the DCL leaks device subscriptions for the life of the node; dead Instance Actor refs get a `TagValueUpdate` per change | DCL / Site Runtime |
|
||||
| DC-2 | **High** | **Releasing a "redundant" MxGateway handle tears down the LIVE subscription** — an ordinary double-subscribe during resolution silently freezes the tag at its last Good value until the next reconnect | DCL |
|
||||
| DC-3 | **High** | **Device writes are stashed across a reconnect and replayed minutes later**, after the script was told the write failed — the doc says there is no buffering of setpoints | DCL |
|
||||
| SR-1 | **High** | **The synchronous `Attributes[key]` accessor blocks a script-pool worker on a continuation queued to the same pool** — eight concurrent sync writes stall every script on the node until the 60 s watchdog detaches them | Site Runtime |
|
||||
| CM-1 | **High** | **No gRPC message-size limit anywhere**; a 256-row site audit batch of 64 KB error rows exceeds the 4 MB default and, because the drain is deterministic oldest-first, the site's audit forward stalls **permanently** and its audit DB grows unbounded | Communication / AuditLog |
|
||||
| MG-1 | **High** | **Script trust deny-list omits `System.Runtime.CompilerServices` (`Unsafe`, `RuntimeHelpers`) and `System.Runtime.Loader`** — type-punning and constructor bypass pass the deploy gate and the site backstop | ScriptAnalysis |
|
||||
| UI-1 | **High** | **Every configuration edit made in the Central UI bypasses the configuration audit log** (17 pages save straight through the repositories); the same edits via CLI are audited | Central UI |
|
||||
| UI-2 | **High** | **The 30-minute idle timeout never fires while a tab is open** — the session-expiry ping slides the cookie every minute | Security |
|
||||
|
||||
The Mediums split into five themes worth reading as groups: **audit completeness** (EI-1/2/3/5,
|
||||
UI-1), **authorization scope** (MG-2, UI-6/7/8, XC-1, XC-8), **stale/duplicate-apply under
|
||||
failover** (MG-3/4, EI-4/9, DL-1/6, ORCH-1), **subscription/stream lifecycle** (DC-4/5, SR-3/5,
|
||||
CM-2), and **config that lies** (XC-2/3/7). §5 groups everything into a remediation program.
|
||||
|
||||
Nothing here requires a redesign. Every High has a small-to-medium fix and a clear test to pin it.
|
||||
|
||||
---
|
||||
|
||||
## 2. Findings by area
|
||||
|
||||
Each finding: where (file:line), what the code does, why it matters, fix, effort.
|
||||
|
||||
### 2.1 Site Runtime / StoreAndForward / SiteEventLogging (SR)
|
||||
|
||||
**SR-1 — Synchronous `Attributes[key]` blocks a script-pool worker on a continuation queued to that same pool `[High][CONFIRMED, orch-verified]`**
|
||||
`Scripts/ScopeAccessors.cs:58-59` (`GetAwaiter().GetResult()`); `Scripts/ScriptRuntimeContext.cs:704, 914` (the Asks inside `GetAttribute`/`SetAttribute` have no `ConfigureAwait(false)`); `Scripts/ScriptRunLauncher.cs:110-123` (the run body is started with `Task.Factory.StartNew(..., executionScheduler)`). Inside the run, `TaskScheduler.Current` is the `ScriptExecutionScheduler`, so the awaiter captures it and the continuation that completes the Ask is `QueueTask`'d onto the same bounded pool whose worker is blocked in `GetResult()`. Pool floor is 8; eight concurrent runs each doing `Instance.Attributes["Setpoint"] = 42` (the documented, compile-gated API — the rig's own Delmia stand-in is a per-attribute write script) block all 8 workers; no script or alarm body on the node progresses until `ScriptExecutionTimeoutSeconds` + `StuckScriptGraceMs` (30 s + 30 s) elapse and the watchdog detaches/replaces each worker, and every affected run reports a spurious timeout. Below saturation it silently halves effective concurrency. The class comment at `ScopeAccessors.cs:12-21` asserts the opposite. **Fix:** `.ConfigureAwait(false)` on the awaits in `GetAttribute`, `SetAttribute`, `WaitInternal`, `WriteBatchAndWait` (AsyncLocal `TraceStepScope` flows with `ExecutionContext`, not the scheduler, so attribution is unaffected); correct the comment; add a test running `poolSize` concurrent bodies each doing one sync `Attributes["x"]` on a scheduler of that size. Small.
|
||||
|
||||
**SR-2 — Alarm on-trigger runs have no integration surface `[Medium][CONFIRMED]`**
|
||||
`ScriptRunLauncher.cs:701-725` (`RunAlarmScriptAsync`) never creates a DI scope; `_externalSystemClient`, `_databaseGateway`, `_storeAndForward`, `_operationTrackingStore`, `_cachedForwarder`, `_auditWriter`, `_siteId`, `_sourceNode` are null, so `ExternalSystem`/`Database`/`Notify`/`Tracking` throw (`ScriptRuntimeContext.cs:1422/1485/2145/2195/2760`). The deploy gate compiles alarm on-trigger scripts against the full `ScriptCompileSurface`, so `Notify.To("Ops").Send(...)` deploys cleanly and throws the first time the alarm fires. `Component-SiteRuntime.md:482` says the full API is available to alarm on-trigger runs. **Fix:** wire the scope + dependencies exactly as `RunScriptAsync` does (small), or narrow the doc and gate it at design time (medium).
|
||||
|
||||
**SR-3 — `NativeAlarmActor` subscribe retry has no in-flight guard, no backoff, no correlation match `[Medium][CONFIRMED]`**
|
||||
`Actors/NativeAlarmActor.cs:147-165, 320-340`: fresh correlation id per send, discarded; fixed-interval re-send; `HandleSubscribeResponse` checks only `Success`. This is the amplifier Lever 1 (2026-08-28) closed on `InstanceActor` and left as a follow-up; a late `Success` from a superseded request also cancels the timer armed for the current one. **Fix:** port `TagSubscribeState` (in-flight id + ceiling, `NextTagSubscribeInterval`, drop non-matching replies). Small.
|
||||
|
||||
**SR-4 — `AuditingDbConnection` forwards a settable `ConnectionString` while audit rows keep the construction-time name `[Medium][CONFIRMED]`**
|
||||
`Scripts/AuditingDbConnection.cs:113-120` vs `:35`; `AuditingDbCommand.cs:426`. A script can `Close()`, assign an arbitrary connection string, `Open()`, and every `DbWrite` row still says `target = "MES-Prod"`; with `Persist Security Info=true` the getter returns the password. The audit log is the compensating control for the raw-ADO escape hatch. **Fix:** override `ConnectionString` (set → throw, get → empty) and `ChangeDatabase` (throw). Small.
|
||||
|
||||
**SR-5 — `InstanceActor.PostStop` blocks the default dispatcher for up to 5 s per instance `[Medium][CONFIRMED]`**
|
||||
`Actors/InstanceActor.cs:129, 443-444, 693-708` (`flush.Wait(FinalStaticOverrideFlushTimeout)`); no `WithDispatcher` anywhere in SiteRuntime. A bulk redeploy landing during an `EventLogPurgeService` tick (1000-row `DELETE` + `incremental_vacuum` under `SiteEventLogger._writeLock`, `EventLogPurgeService.cs:519-552`) stops N actors concurrently, each burning a dispatcher thread on `SQLITE_BUSY` backoff — node-wide, since DCL, Deployment Manager, Script/Alarm actors and the stream relay share it. **Fix:** gate the redeploy clear on an explicit "flush committed" ack piped from the persist task instead of `Terminated` + a blocking wait; or ≤ 1 s timeout + cap concurrent stops. Medium.
|
||||
|
||||
**SR-6 — Trace arm is consumed by any subsequent value change, not the traced write's echo `[Low][CONFIRMED]`** — `InstanceActor.cs:185, 775-780, 1085-1092`; a periodic device publish inside the 30 s window is stamped with the writer's execution id and the real echo goes untraced. Debug fidelity only. Fix: correlate on the `WriteTagResponse` id or written value, or narrow the window. Small.
|
||||
|
||||
**SR-7 — `EmitTrace`/`Collect` log their own failures through the decorated logger `[Low][CONFIRMED]`** — `ScriptRuntimeContext.cs:415-419, 551, 562, 568` use `_logger` (the `TraceCapturingLogger` on a traced run) where `EmitLog` (:590-645) correctly uses `_innerLogger`; each failed trace write becomes a `ScriptLog` row plus a second write attempt (bounded at depth 2). Fix: `_innerLogger`. Small.
|
||||
|
||||
**SR-8 — `ScriptExecutionScheduler.TryExecuteTaskInline` keys on the thread-name prefix, not identity `[Low][CONFIRMED]`** — `:385-393` vs naming at `:331-335`; co-resident schedulers (the #18 seam) cross-inline, defeating the bound and detach bookkeeping. Latent. Fix: `[ThreadStatic] CurrentScheduler` + `ReferenceEquals`. Small.
|
||||
|
||||
**SR-9 — `TriggerEvalGate.Shared` lacks the disposed re-create guard its comment claims to mirror `[Low][CONFIRMED]`** — `TriggerEvalGate.cs:55-64` vs `ScriptExecutionScheduler.cs:125-137`; a disposed shared gate makes every Expression trigger on the node evaluate false forever (caught as "expression false"). Fix: `IsDisposed` guard. Small.
|
||||
|
||||
**SR-10 — Script helper properties allocate a fresh helper + guard closure per access `[Low][CONFIRMED]`** — `ScriptRuntimeContext.cs:1070, 1092, 1130, 1150, 1213`. Fix: lazily cached fields. Small.
|
||||
|
||||
### 2.2 Communication / ClusterInfrastructure / Host (CM)
|
||||
|
||||
**CM-1 — No gRPC message-size limits anywhere; the audit batch arithmetic can exceed the 4 MB defaults and head-of-line-stalls the site audit pipeline permanently `[High][CONFIRMED, orch-verified]`**
|
||||
`Host/Program.cs:291-294` (central `AddGrpc`) and `:825-829` (site) set no `MaxReceiveMessageSize`; neither do `CentralChannelProvider.cs:111-120`, `SitePairChannelProvider.cs:424-437`, `SiteStreamGrpcClient.cs:87-95`; a repo-wide grep is empty. Defaults: receive 4 MB both ends, send unlimited. `SiteAuditTelemetryOptions.BatchSize = 256` × `AuditLogOptions.ErrorCapBytes = 65 536` = 16 MB possible; ~63 large error rows in one window cross 4 MB. `SqliteAuditWriter.cs:659-661` serves a deterministic oldest-first window and `SiteCommunicationAuditClient.cs:78-82` deliberately does not catch, so the same 256 rows fail every tick forever. `PurgeExpiredAsync` (:1170-1176) never purges `Pending`, so the site audit DB on `/app/data` grows unbounded behind the stuck head. Same arithmetic on the reconciliation pull (`BatchSize = 256`, site → central, 4 MB client receive) and on `DeployArtifactsCommand` (unlimited central send, 4 MB site receive). **Fix:** explicit `MaxReceiveMessageSize` (e.g. 32 MB) on both `AddGrpc` calls and every channel, **and** a byte budget on the drain/pull mirroring LocalDb's `MaxBatchBytes`; at minimum treat `StatusCode.ResourceExhausted` in `SiteAuditTelemetryActor` as "halve the batch". Test: serialize a 256-row batch of `ErrorCapBytes` rows and assert it fits the configured limit. Medium.
|
||||
|
||||
**CM-2 — `docker-env2` runs the same simultaneous `--force-recreate` deploy that split site pairs twice, with the bootstrap guard OFF `[Medium][CONFIRMED]`** — `docker-env2/deploy.sh:24` is byte-identical to `docker/deploy.sh:25`; env2's four node appsettings have no `BootstrapGuard` block (`ClusterOptions.cs:144` default false). A deploy there can produce two 1-node clusters that never merge, and env2 is the second real environment gating Transport work. Fix: add the block to all four files. Small.
|
||||
|
||||
**CM-3 — `PullAuditEvents`/`PullSiteCalls` accept an unbounded `batch_size` `[Low][CONFIRMED]`** — `SiteStreamGrpcServer.cs:727-734, 816-822` only check `> 0`; `PullTraceEvents` (:668-676) caps 1..2000; `SiteAuditReconciliationOptionsValidator.cs:23` only `> 0`. Feeds CM-1. Fix: same ceiling + validator upper bound. Small.
|
||||
|
||||
**CM-4 — `ClusterBootstrapCoordinator` can issue `JoinSeedNodes` during host shutdown `[Low][CONFIRMED]`** — `Host/Actors/ClusterBootstrapCoordinator.cs:186-188` swallows the shutdown cancellation as "partner unreachable", so `RunAsync` (:118-126) self-first-joins with no `ct` re-check. Fix: check `ct` before the join / rethrow. Small.
|
||||
|
||||
**CM-5 — Site-hosted `IngestAuditEvents` / `IngestCachedTelemetry` are permanently inert `[Low][CONFIRMED]`** — `SetAuditIngestActor` (`SiteStreamGrpcServer.cs:159-162`) is called only from `AkkaHostedService.cs:569-570` inside `RegisterCentralActors`, but `SiteStreamGrpcServer` is Site-branch-only (`Program.cs:830, 868`); null on central, never set on site. Both RPCs answer an empty ack + Warning on every real node, reachable on the PSK-gated listener; the comment at `AkkaHostedService.cs:565-568` reads as if live. (XC-6 is the same finding from the RPC-inventory side.) Fix: delete both handlers, the setter and the call site (production path is `CentralControlService`), or return `Unimplemented`. Small.
|
||||
|
||||
**CM-6 — Central Kestrel sets no HTTP/2 keepalive, and the optional metrics listener exposes the whole routing table `[Low][CONFIRMED]`** — `Program.cs:231-263` (no `Limits.Http2.KeepAlivePing*`, unlike `SiteKestrelSetup.cs:51-52`); `:254-262` binds `MetricsPort` as `Http1AndHttp2` on the shared pipeline. Defensible today (unary-only `CentralControlService`), undocumented. Fix: apply the site keepalive on central; document the metrics-port surface. Small. (See XC-1 for the larger form of this.)
|
||||
|
||||
**CM-7 — `ChannelDisposalGrace` (5 min) can be outlived by `RouteToWaitForAttribute` `[Low][PLAUSIBLE]`** — `SitePairChannelProvider.cs:56-62` sizes the grace to "the longest command deadline"; `GrpcSiteTransport.cs:224` resolves that request's deadline as caller-supplied `r.Timeout + IntegrationTimeout`, unclamped (`RouteToInstanceRequest.cs:119`, `DeploymentManagerActor.cs:3036`). A > 5-min wait crossing an address refresh gets `ObjectDisposed`/`Cancelled` (not `Unavailable`, so no cross-node retry). Fix: clamp, or grace = `max(5 min, longest resolvable)`. Small.
|
||||
|
||||
**CM-8 — `SiteStreamGrpcClient.ConnectedHeaderTimeout` is a mutable static test seam `[Low][CONFIRMED]`** — `:460`, mutated by `SiteStreamGrpcClientTests.cs:440, 474` while `SiteAlarmAggregatorActorTests` (:865) depends on the default; a parallel-run flake source of the register-row-118 kind. Fix: instance value from `CommunicationOptions`. Small.
|
||||
|
||||
### 2.3 ManagementService / DeploymentManager / TemplateEngine / ScriptAnalysis (MG)
|
||||
|
||||
**MG-1 — Script trust deny-list omits `System.Runtime.CompilerServices` and `System.Runtime.Loader` `[High][CONFIRMED, orch-verified]`**
|
||||
`ScriptAnalysis/ScriptTrustPolicy.cs:36-62` (`ForbiddenScopes`) blocks `System.Runtime.InteropServices` and nothing else under `System.Runtime.*`; Pass 2's `ReflectionGatewayMembers` (`ScriptTrustValidator.cs:162-172, 266-281`) has none of `Unsafe.As`/`Unsafe.Write`/`RuntimeHelpers.GetUninitializedObject`/`RunClassConstructor`/`LoadFromAssemblyPath`; `DefaultAssemblies` (:149-156) includes CoreLib, which hosts all of them on net10.0. `Unsafe.As<object,T>(ref o)` / `Unsafe.Write` need no `unsafe` context, so `AllowUnsafe=false` does not stop them; type-punning reaches the host's private state (the `ScriptRuntimeContext`, the DI graph, secrets). The deploy gate and the site backstop share the policy, so nothing catches it. `Component-ScriptAnalysis.md:38` claims "Reflection — forbidden entirely". **Fix:** add type-level entries `System.Runtime.CompilerServices.Unsafe`, `System.Runtime.CompilerServices.RuntimeHelpers`, `System.Runtime.Loader` (type-level so `AsyncMethodBuilder`/`CallerMemberName` and compiler-generated attributes stay allowed — verify against the clean-script corpus); negative test per entry; doc table. Small. Ranked High because it is the trust boundary itself, even though it needs the Designer role.
|
||||
|
||||
**MG-2 — `/api/audit/tree` and `FlowDebugReplayCommand` apply no site scope `[Medium][CONFIRMED, orch-verified]`**
|
||||
`AuditEndpoints.cs:280-305` (`HandleTree`) authenticates and role-checks but never calls `ApplySiteScope` (which `HandleQuery`/`HandleExport` do at :584); `ManagementActor.cs:423` passes only `sp, cmd` to `FlowDebugRunner.ReplayAsync` (:538), which takes no `AuthenticatedUser`; `AuditLogRepository.cs:946-1077` walks the whole `(ExecutionId, ParentExecutionId)` chain with no `SourceSiteId` predicate. A site-A-scoped operator picks any `ExecutionId` from their (correctly scoped) query and receives site-B's `ScriptRun`/`AttributeWrite`/`DbOutbound` rows (SQL text + bound values) from a multi-site chain — the boundary rows 98/99 and P6-4/P6-5 hardened on every other read path. The three Live run commands DO enforce scope. **Fix:** optional permitted-`SourceSiteId` set on `GetExecutionTreeAsync`/`GetTraceEventsAsync` (NULL = central rows, always visible); thread the user into `ReplayAsync`; stop-gap: refuse both for scoped non-admins. Medium.
|
||||
|
||||
**MG-3 — `StaleInstanceProbe`'s process-static memo can outlive a standby period and report "up to date" for a changed instance `[Medium][CONFIRMED]`**
|
||||
`DeploymentManager/StaleInstanceProbe.cs:45, 120-140` (static `ConcurrentDictionary` keyed on an in-memory, process-local `ITemplateGraphWatermark`); `DeploymentService.cs:1162-1211`. A active → standby (Traefik drops it, its watermark freezes) → B edits templates → A re-promoted: A's memo is still "valid" by its own unchanged watermark and serves the pre-edit hash, so `DeploySiteAsync` skips genuinely changed instances as `up-to-date` with no signal; only `--force` recovers. The class comment (:28-33, "can only ever cause EXTRA work, never stale work") covers only a cold process. **Fix:** `ClearMemos()` (exists as a test hook) on the active-node edge, the same `ActiveNodeEvaluator` edge that arms `IActiveNodeGate`. Small.
|
||||
|
||||
**MG-4 — `MgmtDeploySiteCommand`'s worst-case runtime exceeds its own Ask timeout by construction `[Medium][CONFIRMED]`**
|
||||
`ManagementEndpoints.cs:44-77` (`LongRunningCommandTimeout` 5 min); `DeploymentManagerOptions.cs:34, 42` (parallelism 4 × 120 s per instance → `ceil(N/4) × 120 s`; N = 11 is already 360 s, before serial prepare and finalize). The caller gets `504` while the batch keeps applying; a redeploy is not idempotent at the site (resets overrides, clears native alarm state), so the retry re-applies live instances and its prepare loses the 5 s lock timeout on still-held locks. **Fix:** derive the bulk Ask timeout from the batch, or make bulk deploy submit-and-poll; at minimum validate at startup and document. Medium.
|
||||
|
||||
**MG-5 — `FlowDebugRunner.MaxWaitSeconds` equals the Ask timeout it must stay under, unvalidated `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:56-63` (300 s) vs `ManagementServiceOptionsValidator.cs:26-28` (only `> 0`); a `--wait-seconds 300` run 504s the caller while the runner keeps a scoped provider and polls the site. Fix: clamp to `LongRunningCommandTimeout − grace` + validator rule. Small.
|
||||
|
||||
**MG-6 — `WriteAndPollAsync` re-fetches a fixed oldest page every second; register row 117 confirmed, plus a silent 64-id truncation `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:501` (`since` never advances), `:510` (`ids.Take(64)` drops descendant ids beyond `MaxExecutionIdsPerRead` with no diagnostic); the site serves `ORDER BY OccurredAtUtc ASC LIMIT` (`SqliteAuditWriter.cs:899-908`), so > 2000 rows in the window makes the awaited run unreachable and up to 300 polls re-transfer the same page. Fix: advance `since` (dedup by known ids) or exclude `ScriptLog` from the awaited poll; warn when `ids.Count > 64`. Small.
|
||||
|
||||
**MG-7 — The value-change / write-kind Flow Debugger result bypasses the `FlowTraceCollector` caps `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:525-526` returns all of `known` with `Truncated: false` hardcoded, vs the capped paths at `:190-197`/`:416-418`; 200 log lines × a cascade is an unbounded reply through `SerializeResult` and the Blazor circuit. Fix: feed `known` through a collector (null redactor — rows were redacted at the site). Small. (Same class as DL-3.)
|
||||
|
||||
**MG-8 — Shared scripts never reach the design-time trust/compile gate on create/update `[Low][CONFIRMED]`** — `TemplateEngine/SharedScriptService.cs:60, 109, 178-206` run only `CSharpDelimiterScanner`; `ValidationService.cs:276-293` compiles `configuration.Scripts` only; `ArtifactDeploymentService.cs:172-179` ships them fleet-wide; only `BundleImporter` (:912, 4658) gates them. A forbidden-API shared script is rejected at the site's `ScriptCompilationService.Compile`, at run time. Fix: `ScriptCompiler.TryCompile` in create/update (memoised by `ScriptCompileVerdictCache`). Small.
|
||||
|
||||
**MG-9 — `ManagementActor.Page` turns an out-of-range `Take` into unlimited `[Low][CONFIRMED]`** — `:685-691` (`take is > 0 and <= 1000 ? take : int.MaxValue`; comment says clamped). Fix: `Math.Min(take, 1000)`. Small.
|
||||
|
||||
**MG-10 — `Component-ManagementService.md` documents none of the four `FlowDebug*` commands `[Low][CONFIRMED]`** — zero occurrences, though `RequiredRoleMatrixTests.cs:16-18` names the catalog as its cross-check. Fix: add the subsection. Small.
|
||||
|
||||
### 2.4 Central UI / Security (UI)
|
||||
|
||||
**UI-1 — Central UI configuration edits bypass `IAuditService`: the Configuration Audit Log misses every change made from the UI `[High][CONFIRMED, orch-verified]`**
|
||||
Seventeen page files mutate entities through the repositories and call `SaveChangesAsync()` with no `LogAsync`: `DataConnectionForm.razor:337, 347, 349`; `LdapMappingForm.razor:170, 175, 178, 200, 223`; `SiteForm.razor:169, 186, 189`; `Sites.razor:224`; `SmtpConfiguration.razor:359, 372`; `SmsConfiguration.razor:264, 287`; `ExternalSystemForm.razor:149, 160`; `DbConnectionForm.razor:103, 112`; `ApiMethodForm.razor:311, 323`; `NotificationListForm.razor:191, 199, 241, 267`; deletes in `DataConnections.razor:420`, `ExternalSystems.razor:192, 265, 340`, `LdapMappings.razor:150-153`; `TemplateEdit.razor:1088-1119`; `InstanceConfigure.razor:1326, 1764-1788`. Only `TransportImport.razor.cs:86` injects `IAuditService`. The `ManagementActor` path for the same entities audits every create/update/delete (`AuditAsync` at :1849 … :3615). An Administrator who grants themselves Administrator through an LDAP mapping in the UI leaves no `AuditLogEntry`; the same act via CLI does. The UI path also skips `ConfigSecretScrubber.MergeSentinels` and the server-side delete/move guards. `Component-CentralUI.md` §Configuration Audit Log Viewer promises before/after per change (and `AuditLogEntry` only has `AfterStateJson` anyway). **Fix:** route these pages through `ManagementActorHolder` envelopes as `SecuredWriteService`/`DataConnectionMoveService`/`FlowDebugService` already do (large); stop-gap: inject `IAuditService` and `LogAsync` before `SaveChangesAsync` in the same transaction (medium); add a sweep test that no `Components/Pages/**` file calls `Repository.(Add|Update|Delete)*Async` without an audit.
|
||||
|
||||
**UI-2 — The documented 30-minute idle timeout never fires while a Central UI tab is open `[High][CONFIRMED, orch-verified]`**
|
||||
`Components/Shared/SessionExpiry.razor:37, 49, 64, 80-100` (mounted unconditionally at `MainLayout.razor:40`) fetches `/auth/ping` every minute with no user-interaction signal; `/auth/ping` (`Auth/AuthEndpoints.cs:186-206`) is a normal request, so the cookie middleware's `SlidingExpiration` renews the window and `CookieSessionValidator.TryRefreshAsync` (:169-206) advances `zb:lastactivity`. The component's own comment (:24-26) concedes "the ping itself slides the cookie". `Component-Security.md:54` says an idle session is rejected; on an unattended HMI workstation with the browser open it never is. **Fix:** `OnCheckSlidingExpiration` → `ShouldRenew = false` for `/auth/ping`, and skip the `LastActivity` advance on that path; optionally a browser idle detector that sends `?active=1` only after real input. Test: advance the clock past `IdleTimeoutMinutes` while pings continue. Small.
|
||||
|
||||
**UI-3 — Flow Debugger's pending-refresh loop mutates component state off the renderer dispatcher `[Medium][CONFIRMED]`** — `FlowDebugger.razor.cs:611-632` calls `MergeRefreshedGraph` (:657-680, writes `_raw`/`_graph`/`_selected` non-atomically) on a thread-pool continuation before `InvokeAsync(StateHasChanged)`; every other poller mutates inside `InvokeAsync` (`DataConnections.razor:263-276`, `AlarmSummary.razor:459-471`). Torn state / blank detail pane during the 2-min poll; concurrent use of the scoped service. Fix: `InvokeAsync(() => { Merge; StateHasChanged(); })`. Small.
|
||||
|
||||
**UI-4 — Health / Alarm Summary poll timers can strand `PollGate`, silently killing auto-refresh `[Medium][CONFIRMED]`** — `Health.razor:719-734, 999-1002`; `AlarmSummary.razor:394-411`: `_pollGate.Exit()` lives only inside the inner `InvokeAsync` lambda, so a throwing `InvokeAsync` (`ObjectDisposedException` is expected elsewhere, `DebugView.razor:806-816`) leaves `_inFlight = 1` forever; `Health.Dispose` has no `_disposed` flag. Fix: the `DataConnections` shape (volatile `_disposed`, early return, outer try/catch/finally). Small.
|
||||
|
||||
**UI-5 — A non-cooperative Test Run script permanently burns a static `SandboxRunGate` slot; four kill Test Run process-wide `[Medium][CONFIRMED]`** — `ScriptAnalysis/ScriptAnalysisService.cs:216-222` (static, 4 slots), `:470-479` (`await Task.Run(() => script.RunAsync(host, linkedCts.Token))`), `:524-527` (release in `finally` only after completion). `while (true) { }` never returns; four such runs disable Test Run for every user until restart, plus four leaked threads. Fix: `WhenAny(run, Delay(timeout))`, release on timeout, observe the abandoned task's fault, log/metric. Small.
|
||||
|
||||
**UI-6 — Manual cluster failover has no server-side role check `[Medium][CONFIRMED]`** — `Components/Health/CentralFailoverControl.razor:5, 11, 20, 99, 114` is gated only by `<AuthorizeView Policy=RequireAdmin>` on a bare-`[Authorize]` page (`Health.razor:292`); `Host/Health/AkkaManualFailoverService.cs` has no role check, unlike `BrowseService.cs:49, 104`, `EndpointVerificationService.cs:51`, `BindingTester.cs:43`, `CertManagementService.cs:51, 87, 119`. Not exploitable today (Blazor validates handler ids) but the most destructive UI action with zero defence-in-depth. Fix: `HasClaim(Administrator)` inside the service. Small.
|
||||
|
||||
**UI-7 — No fallback authorization policy and no route→policy sweep test `[Medium][CONFIRMED]`** — `AuthorizationPolicies.AddScadaBridgeAuthorization` sets no `FallbackPolicy`; `Routes.razor` `AuthorizeRouteView` enforces only what a component declares. All 43 pages carry an attribute today (table in §3), but nothing enforces it, and `MapCentralUI` also routes `ZB.MOM.WW.Secrets.Ui`'s pages (`EndpointExtensions.cs:30`). The CLI has a frozen authz matrix test; the UI has none. Fix: `FallbackPolicy = RequireAuthenticatedUser` (Login already has `[AllowAnonymous]`) and/or a reflection sweep with a frozen table. Small.
|
||||
|
||||
**UI-8 — Flow Debugger's parked-node site-scope check fails open on an unresolvable site `[Low][CONFIRMED]`** — `FlowDebugger.razor.cs:811-834` (`if (site is null) return true;` twice); `FlowDebugService.RelayAsync` (:284-345) has no scope check of its own. Fix: deny for scoped principals; move the check into `RelayAsync` (shared with the Site Calls page). Small.
|
||||
|
||||
**UI-9 — `TemplateFolderTree` rebuilds the whole forest on every parameter set `[Low][CONFIRMED]`** — `:99-103` vs the `InstancePicker.razor.cs:157-166` reference-equality guard. Fix: same guard. Small.
|
||||
|
||||
**UI-10 — `/auth/login` and `/auth/token` disable antiforgery (login CSRF) `[Low][CONFIRMED]`** — `AuthEndpoints.cs:110, 171`; an attacker with valid directory credentials can sign a victim's browser into the attacker's account so later actions audit under the attacker. Fix: `<AntiforgeryToken/>` in `Login.razor` + drop the override on `/auth/login`; document or content-type-gate `/auth/token`. Small.
|
||||
|
||||
**UI-11 — Topology loads every instance and template into the circuit, unpaged `[Low][CONFIRMED]`** — `Topology.razor:346-349`; the report pages page at 50. Fix: lazy per expanded site/area. Medium.
|
||||
|
||||
**UI-12 — Empty "Notifications" nav heading for a Viewer; Flow Debugger unreachable from the nav for the roles its policy admits `[Low][CONFIRMED]`** — `NavMenu.razor:55-58` (false premise: a Viewer holds none of Admin/Design/Deployment) and `:48` (link inside the Deployer section though `RequireFlowDebugger` admits Viewer — the F3 auditor intent is half-served). Fix: any-of gate; own `AuthorizeView`. Small.
|
||||
|
||||
### 2.5 InboundAPI / ESG / Notifications / SiteCallAudit / AuditLog / KpiHistory / HealthMonitoring (EI)
|
||||
|
||||
**EI-1 — A relayed inbound request that no peer serves is audited nowhere `[Medium][CONFIRMED]`** — `InboundApiEndpointFilter.cs:95-100` sets `Items[RelayedToActiveItemKey] = true` before `ForwardAsync` and never clears it; `AuditWriteMiddleware.cs:236, 251` skips the row when set; `HttpStandbyRequestForwarder.cs:160-163` answers `503 ACTIVE_NODE_UNREACHABLE`. During exactly the failover/partition window operators need to reconstruct, inbound calls leave zero `ApiInbound` rows. `Component-InboundAPI.md:366` says "audited once, where it ran". Fix: set the flag only on a real peer response (or clear it on `Unreachable()`). Small.
|
||||
|
||||
**EI-2 — `SkipBodyCapture` does not suppress `errorDetail`, which routinely quotes the body `[Medium][CONFIRMED]`** — `AuditWriteMiddleware.cs:195, 243-249` vs `:335-341`; `InboundScriptExecutor.cs:601-604` (`ErrorDetail: ex.ToString()`); `EndpointExtensions.cs:288-291`. F11 closed this for the `RouteCall` row (`RouteHelper.cs:422`); the failure path re-opens it for every failing call, traced or not (an `ArgumentException` quoting a lot number, a `JsonException` quoting the payload, a `SqlException` echoing a key). Fix: gate the stashed detail on the same `skipBody` verdict (keep `thrown?.Message`), or run per-target redactors over it. Small.
|
||||
|
||||
**EI-3 — Per-target SQL-parameter redaction is not applied to `DbOutbound` error rows `[Medium][CONFIRMED]`** — `ScadaBridgeAuditRedactor.cs:217-231` redacts only the request summary; `InboundDatabaseHelper.cs:251` and `AuditingDbCommand.cs:516` stamp `errorDetail: thrown?.ToString()`, and SQL Server error text carries parameter values (`The duplicate key value is (…)`). Fix: apply the configured regex over `errorDetail`/`errorMessage` in the `DbOutbound` branch. Small.
|
||||
|
||||
**EI-4 — `StandbyForwardTimeout` is never checked against per-method timeouts; expiry duplicates side effects `[Medium][CONFIRMED]`** — `InboundApiOptionsValidator.cs:35-37` (only `> 0`); `HttpStandbyRequestForwarder.cs:112-113, 147-152` (`CancelAfter(60 s)`); `ApiMethod.TimeoutSeconds` unbounded (`EndpointExtensions.cs:260-262`). A 120 s method relayed from the standby → `503` at 60 s while the active node runs it to completion → the caller's documented retry executes it twice. Fix: startup warning; attempt deadline `max(StandbyForwardTimeout, methodTimeout + grace)`. Small.
|
||||
|
||||
**EI-5 — Relayed calls lose the caller's IP in the audit row `[Medium][CONFIRMED]`** — `AuditWriteMiddleware.cs:344` records `ctx.Connection.RemoteIpAddress` (= the standby node); `HttpStandbyRequestForwarder.cs:166-192` adds no `X-Forwarded-For`. Fix: append `X-Forwarded-For`/`-Host`; prefer it in `EmitInboundAudit` only when `X-ScadaBridge-Forwarded` is present. Small.
|
||||
|
||||
**EI-6 — Per-channel retention purge is an unbounded loop on the maintenance path `[Low][CONFIRMED]`** — `AuditLogRepository.cs:699-735` (`while (true) { DELETE TOP … }`) driven serially per channel from `AuditLogPurgeActor.cs:262-325`; a `ScriptTrace`@30 d backlog after `TraceAllCalls` holds the actor and starves later channels. Fix: per-tick row/time budget like `MaxPurgedRowsPerTick`. Small.
|
||||
|
||||
**EI-7 — A hung KPI source silently stops all KPI history for the process lifetime `[Low][CONFIRMED]`** — `KpiHistoryRecorderActor.cs:315-342` awaits `CollectAsync` with only the shutdown token; the in-flight guard stays raised forever. Fix: per-source linked CTS (half the sample interval) + log. Small.
|
||||
|
||||
**EI-8 — The standby relay replays only status + `Content-Type`, while `Accept-Encoding` passes through `[Low][CONFIRMED]`** — `HttpStandbyRequestForwarder.cs:131-141, 206-236` vs `:59-63`; the day response compression is enabled, callers get gzip labelled `application/json`. Fix: strip `Accept-Encoding` or copy response headers minus hop-by-hop. Small.
|
||||
|
||||
**EI-9 — Outbox dispatch has no row-level claim; dual-active duplicates every notification `[Low][CONFIRMED]`** — `NotificationOutboxRepository.cs:140-158` (plain read), status written only after delivery (`NotificationOutboxActor.cs:601-637`); under the accepted auto-down dual-active state both singletons deliver everything. `Component-NotificationOutbox.md:112` describes "one notification, rare failover". Fix: widen the doc; optionally an `UPDATE`-claim with a lease. Small (doc) / medium.
|
||||
|
||||
### 2.6 Commons / ConfigurationDatabase / Transport (DL)
|
||||
|
||||
**DL-1 — `Instance.UniqueName` is assumed globally unique but the DB enforces only `(SiteId, UniqueName)` `[Medium][CONFIRMED, orch-verified]`** — `InstanceConfiguration.cs:61`; `InstanceLocator.cs:25-26` (what `Route.To` uses to pick the **site**) is `FirstOrDefault(i => i.UniqueName == name)` with no site qualifier or ordering; also `DeploymentManagerRepository.cs:516`, `TemplateEngineRepository.cs:468`. Global uniqueness is an app-side check-then-act (`InstanceService.cs:67-70`) on a per-node `ManagementActor`, so two operators on the two centrals can both commit; `BundleImporter` Rename (:4205-4215) does no check. A duplicate makes `Route.To` route setpoints to the wrong plant with nothing logged. Fix: migration adding a unique index on `UniqueName` alone. Small.
|
||||
|
||||
**DL-2 — `DataConnection.Primary/BackupConfiguration` hold protocol credentials in plaintext at rest, unlike every sibling secret column `[Medium][CONFIRMED, orch-verified]`** — `ScadaBridgeDbContext.cs:334-353` encrypts exactly four columns (SMTP credentials, SMS auth token, ESG auth config, DB connection string); `ManagementActor.cs:1936` itself says the DataConnection JSON "can embed OPC UA user and certificate passwords"; `Component-Transport.md:98` puts them in the encrypted `SecretsBlock`; `GuardSecretWritesHaveAKeyRing` (:258-268) omits the entity. Caveat: the key ring is `PersistKeysToDbContext` in the same DB without `ProtectKeysWith*` (`ServiceCollectionExtensions.cs:109-110`), so the existing encryption is defence-in-depth, not a hard boundary — but this is the one column class an operator would assume covered. Fix: add the converter to both properties + the guard switch + a one-off re-encrypt migration; or record the deferral in the register and the doc. Medium.
|
||||
|
||||
**DL-3 — Flow Debugger Replay reads an unbounded audit-row set into memory and over the envelope `[Medium][CONFIRMED]`** — `AuditLogRepository.cs:1182-1230` (`GetTraceEventsAsync`, no `Take`); `FlowDebugRunner.cs:544-553` returns every row. `PullTraceEventsMax = 2000` governs only the site RPC; the Live path is capped 500 events / 1 MB. v1.4's 200 `ScriptLog` rows per execution × a tree, `ApiInbound` rows up to 1 MiB each, and the page polls Replay every 5 s for 2 min. Fix: `.Take(PullTraceEventsMax + 1)` → `FlowGraph.Truncated` (the page already renders the badge). Small.
|
||||
|
||||
**DL-4 — `FlowGraphBuilder` emits two edges between a write and the wait it satisfied `[Low][CONFIRMED]`** — `:150-152` + `:219-224`; `FlowGraphView` draws both, obscuring the dashed echo edge; the test at `FlowGraphBuilderTests.cs:90` asserts `Single(External)`, not `(From, To)` uniqueness. Fix: skip the External edge when the write is the chain predecessor. Small.
|
||||
|
||||
**DL-5 — Refresh merge violates the builder's input-order contract for late-arriving executions `[Low][CONFIRMED mechanism / PLAUSIBLE impact]`** — `FlowDebugger.razor.cs:707-714` (known-first, then fresh) vs the F5 rule (`FlowGraphBuilder.cs:84-117`); a trigger-cascaded child whose rows arrive by telemetry lands at the tail regardless of when it ran. Fix: base on the (complete) Replay order, append live-only rows. Small.
|
||||
|
||||
**DL-6 — A bundle `Overwrite` silently reassigns a deployed instance to a different site `[Medium][CONFIRMED path / PLAUSIBLE operationally]`** — `BundleImporter.cs:4236-4247` rewrites `existing.SiteId` (+ `State = NotDeployed`, and `ReplaceInstanceChildrenAsync` :4419-4438 hard-deletes overrides/bindings). Transport never touches runtime nodes, so the instance keeps running at the old site with no central record binding it there — invisible on the Deployments page. `Component-Transport.md:306-315` never mentions the `SiteId` rewrite. Fix: block the site change on Overwrite when `State != NotDeployed`, or an explicit preview blocker; document. Small.
|
||||
|
||||
**DL-7 — Every audit row is JSON-deserialized 2–3× per graph build `[Low][CONFIRMED]`** — `FlowStepProjector.cs:20, 54, 63, 82` from `FlowGraphBuilder.cs:72-73, 386, 419`; ~4–6 k parses per 5 s poll on a 2 000-row tree. Fix: deserialize once into a tuple list. Small.
|
||||
|
||||
**DL-8 — Replay's root execution id is picked by timestamp order, which F5 declared untrustworthy `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:552` (`events[0]`) though `ResolveChainAnchorAsync` (`AuditLogRepository.cs:945-1004`) already computes the true root. Fix: return `ChainAnchor.RootExecutionId`. Small.
|
||||
|
||||
**DL-9 — `Types/Debugging/` is absent from the Commons spec; `FlowTraceCollector` sits outside REQ-COM-6's carve-out `[Low][CONFIRMED]`** — `Component-Commons.md:171-208, 309-327`. Fix: doc update. Small.
|
||||
|
||||
**DL-10 — No enum lock-in tests for `FlowLane`/`FlowStepKind`/`FlowStepOutcome` `[Low][CONFIRMED]`** — `AuditEnumTests` covers only the four audit enums; `KindRank`'s display-order contract is unguarded. Fix: member-set assertions. Small.
|
||||
|
||||
### 2.7 DataConnectionLayer / CLI / DelmiaNotifier / docker scripts (DC)
|
||||
|
||||
**DC-1 — Nothing in production ever sends `UnsubscribeTagsRequest` / `UnsubscribeAlarmsRequest`; the DCL leaks subscriptions for the life of the node `[High][CONFIRMED, orch-verified]`**
|
||||
Handlers `Actors/DataConnectionActor.cs:1799, 3138`; routing `DataConnectionManagerActor.cs:153, 178`; a repo-wide grep finds senders only in tests — `InstanceActor.cs` and `NativeAlarmActor.cs` contain zero references, and `DataConnectionActor` has no `Context.Watch` at all. The connection actor is never stopped on redeploy (`HandleReconfigureConnection` :118 forwards; `HandleCreateConnection` :75 short-circuits on an existing name). Disable, delete, or redeploy an instance with a narrower attribute set and: the removed tags stay advised on the device (OPC UA monitored items / MxAccess advises never released), `ReSubscribeAll` re-issues them on every reconnect forever, `TotalSubscribedTagCount`/`ResolvedTagCount` never drop, `_subscribers[instanceName]` keeps a dead `IActorRef` that receives a `TagValueUpdate` per value change (dead-letter flood at tag rate), and `_alarmSourceSubscribers` never empties. On a churny site this is unbounded device-side subscription growth. `Component-DataConnectionLayer.md:447, 455` describe the unsubscribe as a live interaction. **Fix:** send both unsubscribes from `InstanceActor.PostStop` / `NativeAlarmActor.PostStop` and on undeploy/disable; belt-and-braces `Context.Watch` each subscriber in `HandleSubscribe`/`HandleSubscribeAlarms` and treat `Terminated` as an unsubscribe. Test: stop an `InstanceActor` and expect an `UnsubscribeTagsRequest` at the DCL probe. Medium.
|
||||
|
||||
**DC-2 — Releasing a "redundant" MxGateway handle tears down the LIVE subscription for that tag `[High][CONFIRMED, orch-verified code path]`**
|
||||
`DataConnectionActor.cs:1642-1653` + `:1703` (`_ = UnsubscribeIdsAsync(msg.Adapter, redundantIds)`), same in `:2500, 2509`. On MxGateway the "subscription id" **is** the MXAccess item handle (`RealMxGatewayClient.cs:193`, `reply.ItemHandle.ToString(...)`), per tag, so the redundant id is normally the live handle — and both removals are identity-blind anyway (`MxGatewayDataConnection.cs:294-304` `_tagToSub.TryRemove(s.TagPath)`; `RealMxGatewayClient.cs:219` `_tagToHandle.TryRemove(tag)`), deleting the mapping the event loop routes on. The trigger is ordinary: `HandleSubscribe`'s dedup partition (:1151) checks `_subscriptionIds` and `_subscribesInFlight` but not `_resolutionInFlight`, so a second instance subscribing to an unresolved tag while its resolution probe (:2277) is in flight double-subscribes. Result: the tag silently stops updating — the attribute freezes at its last Good value, no quality change, nothing in the health report — until the next reconnect. Same class as the QA Z28061 frozen-value incident. **Fix:** (a) include `_resolutionInFlight` in the partition; (b) identity-checked removals (`if (cur == id)`); (c) skip the release when `result.SubscriptionId == _subscriptionIds[tag]`. Test: subscribe one tag twice against the Mx adapter, release the "redundant" id, assert the tag still routes. Small.
|
||||
|
||||
**DC-3 — Device writes are stashed across a reconnect and applied minutes later, contradicting the "no buffered setpoints" invariant `[High][CONFIRMED, orch-verified]`**
|
||||
`DataConnectionActor.cs:417-422` (Connecting) and `:793-796` (Reconnecting) stash `WriteTagRequest`/`WriteTagBatchRequest`; `:494` `UnstashAll` on `BecomeConnected`. `Component-DataConnectionLayer.md:421`: "There is **no store-and-forward for device writes** … buffering stale setpoints would be dangerous." The script's 30 s Ask (`InstanceActor.cs:787`) times out and the script is told the write failed; the reply dead-letters; the value is still written to the device when the link returns. A 10-minute outage ends with a burst of stale setpoints hitting equipment whose scripts may already have compensated. Secondarily the stash is unbounded (no `stash-capacity` configured) and Lever 1's `SubscribeTagsRequest` re-sends (every ≤ 60 s during the outage) are all stashed and replayed, each a full `SeedTagsAsync`. **Fix:** answer writes in Connecting/Reconnecting with an immediate failed `WriteTagResponse` (what the doc describes and the script already handles), or drop stashed writes older than `WriteTimeout` at unstash; dedupe stashed subscribes per instance / set `stash-capacity`. Small.
|
||||
|
||||
**DC-4 — OPC UA silently downgrades to an unsecured endpoint when the requested `SecurityMode` is unavailable or discovery fails `[Medium][CONFIRMED]`** — `RealOpcUaClient.cs:151-158` (`?? endpoints.FirstOrDefault()`), `:386-392` (catch → `new EndpointDescription(url)`, mode None); no post-connect assertion. A `SignAndEncrypt` connection connects unencrypted and reports green; Verify has the same fallback. Fix: fail the connect when `preferred != None` and no match / discovery threw, naming requested vs available. Small.
|
||||
|
||||
**DC-5 — A new MxGateway alarm source covered by the live stream prefix never receives its snapshot `[Medium][CONFIRMED]`** — `MxGatewayDataConnection.cs:319-322` short-circuits when `AlarmFilterPrefix.Covers(prefix, source)`; `Covers` is true for an empty prefix (`AlarmFilterPrefix.cs:57-58`) and `LongestCommonPrefix` collapses to `""` on the first differing character, so every later source takes the no-restart path — the only path without a snapshot. `NativeAlarmActor` populates `_alarms` only on `SnapshotComplete` (:232-237), so standing alarms on a newly deployed instance are invisible until the next reconnect (OPC UA is unaffected: `TriggerConditionRefreshAsync` per handle). Fix: drop the `Covers` short-circuit (the prefix is documented as a bandwidth optimisation only). Small.
|
||||
|
||||
**DC-6 — `deploy site` (and every bulk command) exits 0 when every instance fails `[Medium][CONFIRMED]`** — `CLI/Commands/CommandHelpers.cs:169-193` returns 0 for any 2xx; `DeployCommands.cs:68-84`. A CI `scadabridge deploy site … || rollback` never fires; same for `bundle import` and `flow-debug run-*`. Fix: an `onSuccess` handler inspecting `failureCount` → exit 1; document. Small.
|
||||
|
||||
**DC-7 — DelmiaNotifier treats a standby central's 503 as final and never fails over `[Medium][CONFIRMED]`** — `DelmiaNotifier/Notifier.cs:31-52` rolls over only on `ConnectFailed`. Its HA therefore depends entirely on `StandbyForwardPeerEndpoints` being configured on both centrals — which QA (`vq03`/`vq04`) and prod (`vd03`) still lack (CLAUDE.md). Fix: fail over on 503 (it is "this node cannot serve you", not a business rejection), or add the prerequisite to the README + rollout record. Small.
|
||||
|
||||
**DC-8 — `secured-write` is a shipping CLI group absent from the README and from `CommandTreeTests` `[Low][CONFIRMED]`** — `CLI/Program.cs:42`; `CommandTreeTests.cs:26-45, 63-71` asserts 18 groups vs 19 in `Program.cs` and builds `data-connection` without `ConnectionBrowseCommands.AddTo`, so browse/search/verify-endpoint/certs are uncovered too. Fix: add to the test (19), `AddTo` in the test build, README section. Small.
|
||||
|
||||
**DC-9 — Stale statements in `Component-DataConnectionLayer.md` `[Low][CONFIRMED]`** — `:412` (`volatile bool` → `int` + `Interlocked.Exchange`), `:318` (`UnsubscribeAlarmsRequest` is handled, not stashed, in Reconnecting at `:802`), `:382/384` vs `:421` contradiction on write stashing (DC-3), `:323` prefix restart "costs bandwidth, not correctness" (DC-5). Small.
|
||||
|
||||
**DC-10 — Per-item shard planning allocates a list per monitored item on the subscribe hot path `[Low][CONFIRMED]`** — `RealOpcUaClient.cs:765-772` from the loop at `:686`; ~75 k short-lived objects on a 37.5 k-tag reconnect. Fix: hoist / plan per chunk. Small.
|
||||
|
||||
**DC-11 — DelmiaNotifier aborts the whole failover loop on a malformed `BaseUrls` entry `[Low][CONFIRMED]`** — `HttpRecipeSender.cs:20-45` catches only `HttpRequestException`/`OperationCanceledException`; a `UriFormatException` on the first URL escapes to `Program`'s outer catch. Fix: map any exception to `ConnectFailed`, or `Uri.TryCreate` in `ConfigLoader.SplitBaseUrls`. Small.
|
||||
|
||||
### 2.8 Cross-cutting (XC) and orchestrator (ORCH)
|
||||
|
||||
**XC-1 — Central's entire HTTP endpoint graph is served on the site-facing h2c gRPC listener, bypassing Traefik `[Medium][CONFIRMED, orch-verified]`** — `Host/Program.cs:228-262` binds the HTTP ports (`Http1AndHttp2`) and `centralGrpcPort` (`Http2`) on one `WebApplication`; endpoint routing is not listener-scoped and there is no `RequireHost`/`MapWhen` anywhere in `src/`. Every endpoint mapped at `:711-748` — Central UI, `/management`, `/api/*`, `/api/audit`, `/metrics`, `/health/*`, the SignalR hub — is also served on `:8083`, the port every site dials and the rig publishes (`docker-compose.yml:131, 198`). `curl --http2-prior-knowledge http://central-a:8083/metrics` reaches metrics and health **anonymously**, and the authenticated surfaces with credentials, on a port not behind Traefik — so the `/health/active` active-node routing is bypassed. Register row 56 deliberately made central `/metrics` opt-in on a separate port for this reason. Fix: `.RequireHost($"*:{httpPort}")` on the HTTP-only groups, or an endpoint filter that 404s non-control-plane paths on `centralGrpcPort`; pin with a Host.Tests case. Small.
|
||||
|
||||
**XC-2 — Four SMTP keys in all four rig central appsettings bind to nothing `[Medium][CONFIRMED, orch-verified]`** — `docker/central-node-*/appsettings.Central.json:84-89` and env2 set `Notification:{SmtpServer, SmtpPort, AuthMode, FromAddress}`; `NotificationOptions` (`NotificationService/NotificationOptions.cs:13-25`) has only `ConnectionTimeoutSeconds` and `MaxConcurrentConnections`; zero code hits. Real SMTP config is the DB `SmtpConfiguration` row. An operator editing the host/port here changes nothing, silently. Fix: delete the keys, leave a `_comment` pointing at `notification smtp` / `/notifications/smtp`. Small.
|
||||
|
||||
**XC-3 — `SiteEventLog:PurgeScheduleCron` is set on all 8 site nodes and read by nothing `[Medium][CONFIRMED, orch-verified]`** — `docker/site-*/appsettings.Site.json:75` (`"0 2 * * *"`); no `Cron` in `src/`. The purge is a `PurgeInterval` delay loop anchored to the previous purge's end, so it drifts to whatever time the node last restarted, and a 2.7 M-row retention burst can land mid-shift while the file promises 02:00. Fix: remove the key (small) or implement it (medium).
|
||||
|
||||
**XC-4 — The vendored gRPC C# has no guard tying it to the `.proto` `[Medium][CONFIRMED]`** — `Communication.csproj:64-70` (commented-out `<Protobuf>`, correctly); no test reads a `.proto`. Editing a proto and forgetting `regen-proto.sh` builds green and ships the stale wire shape — invisible because both ends compile from the vendored file, until a third party generates from the real proto. Checked by hand: 565 fields, 0 mismatches today. Fix: a ~40-line test that parses `Protos/*.proto` for `name = n;` and asserts each `PascalCase(name) + "FieldNumber" == n` on the generated types. Small.
|
||||
|
||||
**XC-5 — README, HighLevelReqs, Component-ManagementService and the components skill still describe ClusterClient as the live transport `[Medium][CONFIRMED]`** — `README.md:83, 96`; `HighLevelReqs.md:49-51`; `Component-ManagementService.md:5, 16, 21, 48, 50, 301, 307` (a whole "ClusterClientReceptionist Registration" section, contradicted by line 27 of the same file); `Component-NotificationOutbox.md:31`; `Component-Commons.md:292`; `.claude/skills/scadabridge-components/SKILL.md` entries 5 and 18. These are the entry-point documents an agent loads first. Fix: rewrite to gRPC both directions + `SiteStreamService` + token-gated HTTP config fetch; delete the receptionist section. Small.
|
||||
|
||||
**XC-6 — `SiteStreamService.IngestAuditEvents` / `IngestCachedTelemetry` are dead RPCs kept on the site's authenticated surface `[Medium][CONFIRMED]`** — the RPC-inventory view of CM-5 (`sitestream.proto:14-15`; `SiteStreamGrpcServer.cs:539, 595`); no client outside `Host.Tests`. Fix as CM-5. Small.
|
||||
|
||||
**XC-7 — Migration-only legacy keys still shipped in every rig site appsettings `[Low][CONFIRMED]`** — `ScadaBridge:Database:SiteDbPath` and `ScadaBridge:StoreAndForward:SqliteDbPath` in all 8 site files (both rigs), which CLAUDE.md says to delete once migrated; the other two named keys are already gone. Fix: delete. Small.
|
||||
|
||||
**XC-8 — Both gRPC auth interceptors are allow-by-default for unlisted service prefixes, with no coverage test `[Low][CONFIRMED]`** — `ControlPlaneAuthInterceptor.cs:164-168, 197-207`; `CentralControlAuthInterceptor.cs:50-51, 197-201` (`if (!IsGated(method)) return;` over a hard-coded allowlist). The existing tests assert the prefixes match descriptor names, not that every `MapGrpcService` is covered; a fourth service ships unauthenticated with no failing test. Fix: a test reflecting over the mapped services against the union of gated sets. Small.
|
||||
|
||||
**XC-9 — Package version skew: `Microsoft.Data.SqlClient` pinned below EF's floor, `SignalR.Client` one major behind, one prerelease `[Low][CONFIRMED]`** — `Directory.Packages.props:41` (6.0.2) with `VersionOverride="6.1.1"` in three test csprojs ("EF SqlServer 10.0.7 needs ≥ 6.1.1 … override is test-only"): ESG compiles against 6.0.2 while the Host resolves 6.1.1 at runtime; `SignalR.Client 9.0.3` in a 10.0.7 family; `bunit 2.0.33-preview`. Fix: raise the central pin to 6.1.1 and delete the overrides; evaluate SignalR 10. Small.
|
||||
|
||||
**XC-10 — `OpcUaGlobalOptions` / `MxGatewayGlobalOptions` are the only bound options with neither `ValidateOnStart` nor a validator `[Low][CONFIRMED]`** — `DataConnectionLayer/ServiceCollectionExtensions.cs:32-36`; every sibling chains both (arch-review 08 §1.5). A bad timeout surfaces as a hang at first connect, not at boot; masked today because neither section is set on either rig. Fix: validators + `ValidateOnStart`. Small.
|
||||
|
||||
**XC-11 — Umbrella index is two features behind, and `main` is 46 commits unpushed `[Low][CONFIRMED]`** — `scadaproj/CLAUDE.md:41` pins `main @ 789377ef` (2026-09-06); local `main` = `d8229916`; the instance picker and v1.4 step logs are unrecorded; `origin/main..HEAD` = 46. Fix: push; update the row (short, per the index's own rule). Small.
|
||||
|
||||
**XC-12 — `RequiredSingletonsHealthCheck`'s doc miscounts the central singletons `[Low][CONFIRMED]`** — `Host/Health/RequiredSingletonsHealthCheck.cs:32-38` says "all five … unconditional"; there are seven (`AkkaHostedService.cs:518, 551, 619, 664, 687, 718, 744`), two excluded as best-effort — a reason the comment's two-branch rule would get wrong. Fix: amend the remark. Small.
|
||||
|
||||
**ORCH-1 — `OperationTrackingStore.Dispose()` disposes the write connection under a live pump transaction `[Medium][CONFIRMED, observed in the run]`** — `SiteRuntime/Tracking/OperationTrackingStore.cs:616-634` completes the channel then `_writeConnection.Dispose()` immediately, while the `Task.Run(PumpAsync)` pump may be inside `using var tx = _writeConnection.BeginTransaction()` (:388, :407). `SqliteConnection.Dispose` rolls back the live transaction on the disposing thread while the pump concurrently completes it → `NullReferenceException` inside `SqliteTransaction.Complete()`. Reproduced in this review's suite run: `OperationTrackingStoreTests.SR024_SyncDispose_DoesNotDeadlock_WhenInvokedFromFreshThread` failed with exactly that stack (§7). Production disposes the host singleton through `DisposeAsync` (which drains), so this is a latent hazard on the sync path plus a flaky test, not an outage. Fix: on the sync path, complete the writer and wait for the pump with a short bounded timeout (not the gate) before disposing the connection, or take the write gate synchronously with a timeout; pin with the existing test. Small.
|
||||
|
||||
---
|
||||
|
||||
## 3. Verified correct
|
||||
|
||||
The reviewers recorded what they checked and found holding; the significant ones, by area:
|
||||
|
||||
- **Site Runtime:** a stop does not cancel an in-flight run; deadlines armed at enqueue and shed at dequeue on every path; watchdog detach/replace bookkeeping (`ScriptExecutionScheduler.cs:275-379`); `MaxConcurrentRunsPerScript` counter cannot leak; **no awaited audit write on any script path** (eight SiteRuntime sites + the forwarder, all enqueue-and-observe on `TaskScheduler.Default`); `TraceCapturingLogger` double-wrap guard and the shared-script logger sharing; `TraceStepScope` restore; log caps + `FlushDropped` once + collector reserve arithmetic; Lever 1 in full; `OperationTrackingStore` read-your-writes barrier; `Notify.Send` `maxRetries: 0` + the `MaxRetries > 0` park test; purge pacing pure and faulted ticks don't arm catch-up; trigger eval off-pool with coalescing.
|
||||
- **Communication / Host:** every mapped gRPC service is behind a fail-closed `FixedTimeEquals` interceptor (`CentralControlAuthInterceptor`, `SecretsHubAuthInterceptor` via the shared `UsesGrpcHub` predicate, `ControlPlaneAuthInterceptor` on stream + command, `LocalDbSyncAuthInterceptor`); proto diff vs `ee193cd2` is additive-only with vendored C# in sync and `regen-proto.sh` restoring the csproj on every exit path; batching negotiation via the proto3 default; `ActiveNodeEvaluator` is the sole active definition (`State.Leader` appears only in two "never use" comments); `BuildHocon` emits `AutoDowning`, `min-nr-of-members = 1` pinned, empty seed list under the guard; seed self-first enforced and true in all 14 appsettings; channel park-then-sweep, `(site, endpoint)` factory key, `TryGet` on teardown; reconnect on OK completion wired through both consumers; single-endpoint sites collapse correctly; bounded DropOldest channels; options validated on start everywhere but XC-10; correlation-id path-element guard; Traefik probes `/health/active`.
|
||||
- **Management / Deployment / Templates:** deployment-config token boundary (existence → TTL → token, 256-bit CSPRNG, `FixedTimeEquals`); secured writes CAS on `Pending` for approve/reject with self-approval and scope checks before the CAS (no oracle); bulk deploy failure isolation end to end (P7-3/5/6), `Disabled` wins under `--force`, conservative `IsUpToDateAsync`; `OperationLockManager` ref-counted with CAS-idempotent release on exactly the four mutating commands; `PendingDeployment` per-instance under a 1-permit gate; revision-hash determinism guarded by test; acyclicity via both cycle detectors; all four Roslyn surfaces attach `CachingScriptMetadataResolver.Instance`; compile-surface parity pinned; no `Task.Run`/`.Result`/`.Wait()` in the four projects; `RequiredRoleMatrixTests` (145 entries) + `DispatchCoverageTests` cover the four `FlowDebug*` commands.
|
||||
- **Central UI / Security:** no third-party component framework; no sync-over-async on any render/event/lifecycle path (the 4769e192 fix intact, `MaxPredictionPasses = 8`); `DisableLoginGuard` refuses outside Development, runs before `AddSecurity`; cookie hardening and the two validator inequalities; LDAP-failure semantics (`Keep` on refresh fault, DB-only refresh); JWT key ≥ 32 bytes, `ClockSkew = Zero`; `LoginThrottle` per user|ip and per-ip, bounded; forwarded headers opt-in, `ForwardLimit = 1`, mounted first; sandbox trust gate before execution and for nested `CallShared`; `InstancePicker`'s reference contract honoured by both callers; report pages server-paged; bundle upload size-capped. All 43 routable pages carry an authorization attribute (login `AllowAnonymous`; dashboard + health bare `Authorize`; admin/*, certificates, transport import, SMTP/SMS `RequireAdmin`; design/* + notification lists `RequireDesign`; deployment/*, monitoring/*, reports `RequireDeployment`; flow debugger `RequireFlowDebugger`; audit/* `OperationalAudit`; secured writes `RequireSecuredWriteAccess`; `/admin/secrets` external).
|
||||
- **Edge / audit backbone:** enqueue-and-observe on all eleven script/request-thread audit writes (awaited only on actor paths); redactor on every persistence path including the inline Live copy, failing closed on a bad pattern; site retention SQL purges only `Forwarded`/`Reconciled`, `MarkForwarded` after the central ack; central append-only guard + per-channel purge validated to `[30, RetentionDays]`; SiteCallAudit insert-first then rank-guarded update with `COALESCE(SourceNode)`; inbound auth constant-time with single 401/403 bodies and the scope check before the DB; ESG single deadline, ordered catch filters, bounded read, verb allowlist, escaping, auth never logged, poison parks; EWS https at the sender, `CredentialRedactor.Scrub` on every adapter error, adapters resolved per delivery scope; exactly one `Meter` in `src/` and it plus `LocalDbMetrics` are allowlisted on both roots; health sequence numbers seeded from unix ms; the middleware rewinds the body in `finally` so the relay sees it; KPI per-source fault isolation.
|
||||
- **Data layer:** Commons has zero project references and the graph is an acyclic DAG (§6.A); every changed record since August adds trailing `default(T)`-safe parameters only; `AuditKind.ScriptLog` and `AuditChannel.ScriptTrace` are last and stored as strings; all 8 migrations non-empty with faithful `Down` and no destructive statements; model snapshot == last Designer target model; hot-path index/query matches on SiteCalls, Notifications, KpiSample, DeploymentRecords; UTC converters on the audit read path; no FK on any audit-bearing table, `Restrict` on the instance/site/connection edges; idempotent ingest on all three central stores; bundle import inside the execution strategy with tracker clears and explicit rollback; the import trust gate is Pass 0, fail-closed, over all six script kinds; secrets presence-only in diffs; `LineDiffer` cap before Myers; `ScriptArtifactsChanged` covers `Add`; `DeploymentRecord` rowversion real including the delete stub path.
|
||||
- **DCL / CLI / scripts:** adapter-generation fencing on every completion message and the P2-6 demotion; no overlapping reconnects; OPC UA item accounting leak-free under `AsyncSerialGate`; adapter dispose/reset order; health counts derived; the verify probe never trusts; `TrustServerCertCommand` fans to a per-node `CertStoreActor`, Admin-gated; `AckTime` semantics per spec; CLI timeout honesty; no secrets echoed anywhere; README ↔ code parity on 134 leaf commands (DC-8 the only gap); all ten docker scripts `set -euo pipefail`, rig test creds only, worktree guard, `DELMIA_API_KEY` required.
|
||||
|
||||
---
|
||||
|
||||
## 4. Doc drift (consolidated)
|
||||
|
||||
| Doc | Says | Actually | Finding |
|
||||
|---|---|---|---|
|
||||
| `README.md:83, 96`; `HighLevelReqs.md:49-51`; `Component-ManagementService.md:5,16,21,48-50,301,307`; `Component-NotificationOutbox.md:31`; `Component-Commons.md:292`; components skill entries 5, 18 | ClusterClient / ClusterClientReceptionist is the command/control transport | gRPC both directions since Phase 4 (2026-07-23); `Component-ManagementService.md` contradicts itself at line 27 | XC-5 |
|
||||
| `Component-SiteRuntime.md:482` | full Script Runtime API available to alarm on-trigger runs | no integrations wired | SR-2 |
|
||||
| `Component-StoreAndForward.md:79` | retry-exhausted notifications park | `maxRetries: 0`, park test `MaxRetries > 0`; contradicts line 52 | — |
|
||||
| `Component-StoreAndForward.md` sweep section | ordered `created_at ASC` drain | per-`(Category, Target)` lanes at `SweepTargetParallelism = 4`, lane abandoned on first transient failure — undocumented | — |
|
||||
| `Component-DataConnectionLayer.md:421` vs `:382/384` | no buffering of device writes / writes are stashed | stashed and replayed with no TTL | DC-3 |
|
||||
| `Component-DataConnectionLayer.md:447, 455, 358` | Site Runtime sends unsubscribes | nothing does | DC-1 |
|
||||
| `Component-DataConnectionLayer.md:412, 318, 323` | `volatile bool`; alarm requests stashed while Reconnecting; prefix restart costs bandwidth only | `int`+`Interlocked`; unsubscribe handled; also costs the snapshot | DC-9, DC-5 |
|
||||
| `Component-ScriptAnalysis.md:38` | reflection forbidden entirely | `RuntimeHelpers`/`Unsafe`/`AssemblyLoadContext` allowed | MG-1 |
|
||||
| `Component-ManagementService.md` | command catalog | missing all four `FlowDebug*` | MG-10 |
|
||||
| `CLAUDE.md` | "frozen 138-command authorization matrix" | 145 entries | — |
|
||||
| `Component-InboundAPI.md:366`; `CLAUDE.md` | relayed call audited once; peer response replayed unchanged | zero rows when no peer served; headers dropped | EI-1, EI-8 |
|
||||
| `Component-NotificationOutbox.md:112` | one duplicate on a rare failover | continuous duplication under dual-active | EI-9 |
|
||||
| `Component-CentralUI.md:89, 285, 311, 317` | data-connection pages Admin; audit pages Admin/Audit; LDAP mapping three roles | `RequireDesign`; `OperationalAudit` = {Admin, Viewer}; six roles | — |
|
||||
| `Component-CentralUI.md:311-315` | configuration audit shows before/after per change | UI writes no rows; only `AfterStateJson` exists | UI-1 |
|
||||
| `Component-Security.md:54`; `SessionExpiry.razor:24-26` | idle session rejected; ping never keeps a session alive | ping slides the cookie every minute | UI-2 |
|
||||
| `Component-CentralUI.md` | — | Test Run / sandbox surface undocumented | — |
|
||||
| `Component-Commons.md:171-208, 309-327` | `Types/` subfolder list; pure helpers only | `Types/Debugging/` missing; `FlowTraceCollector` stateful | DL-9 |
|
||||
| `Component-ConfigurationDatabase.md` | — | encrypted vs plaintext secret columns never enumerated | DL-2 |
|
||||
| `Component-Transport.md:298, 306-315` | `UniqueName` identity key; FK rewiring list | not DB-enforced globally; `SiteId` rewrite omitted | DL-1, DL-6 |
|
||||
| `Component-Communication.md:114` | `IngestCachedTelemetry` live | dead | CM-5 |
|
||||
| `docker/*/appsettings*` | SMTP keys; 02:00 cron purge; legacy DB paths | none bound | XC-2, XC-3, XC-7 |
|
||||
| `RequiredSingletonsHealthCheck.cs:32-38`; `ScopeAccessors.cs:12-21`; `TriggerEvalGate.cs:25-26`; `StaleInstanceProbe.cs:28-33`; `FlowDebugRunner.cs:56-63`; `ManagementActor.cs:685-686`; `SitePairChannelProvider.cs:57-62`; `AkkaHostedService.cs:565-568`; `CommandTreeTests.cs:22-24` | code comments | each contradicted by the code beside it | XC-12, SR-1, SR-9, MG-3, MG-5, MG-9, CM-7, CM-5, DC-8 |
|
||||
| `scadaproj/CLAUDE.md:41` | `main @ 789377ef` | `d8229916`, two features later, 46 commits unpushed | XC-11 |
|
||||
|
||||
---
|
||||
|
||||
## 5. Recommended remediation program
|
||||
|
||||
Grouped so each work package is file-disjoint enough to run as a parallel subagent, in the shape
|
||||
of `2026-08-14-arch-review-remediation-plan.md`. Suggested order is by risk, not by count.
|
||||
|
||||
| WP | Findings | Theme | Effort |
|
||||
|---|---|---|---|
|
||||
| **WP1 — DCL subscription lifecycle** | DC-1, DC-2, DC-3, DC-5, SR-3, DC-4 | unsubscribe on stop/undeploy + `Context.Watch`; identity-checked handle release + `_resolutionInFlight` in the partition; fail writes while disconnected; snapshot on every new alarm source; port Lever 1 to `NativeAlarmActor`; no silent security downgrade | medium — needs a rig rebuild + the Lever-1-style live gate (subscribe count on a redeploy, frozen-tag probe) |
|
||||
| **WP2 — Script pool + trust** | SR-1, MG-1, SR-4, MG-8, SR-7/8/9/10 | `ConfigureAwait(false)` on the accessor paths; three deny-list entries; seal `AuditingDbConnection`; gate shared scripts at save | small — pure unit-testable |
|
||||
| **WP3 — gRPC sizing** | CM-1, CM-3, XC-4, XC-8 | explicit receive limits both roots + channels; byte budget on the audit drain/pull; batch-size ceilings; proto↔C# guard test; interceptor coverage test | medium |
|
||||
| **WP4 — Audit completeness + scope** | UI-1, EI-1, EI-2, EI-3, EI-5, MG-2, UI-8 | UI mutations through the management envelope (or the `IAuditService` stop-gap); relay audit-once fix; `SkipBodyCapture` + per-target redaction over `errorDetail`; caller IP; site scope on tree/Replay/relay | large (UI-1) + small ×6 |
|
||||
| **WP5 — Session + UI hygiene** | UI-2, UI-3, UI-4, UI-5, UI-6, UI-7, UI-10, UI-12, UI-9, UI-11 | non-sliding ping; dispatcher-marshalled merge; `PollGate` shape; sandbox gate release on timeout; failover role check; fallback policy + sweep test; login antiforgery | small ×8, medium ×2 |
|
||||
| **WP6 — Failover-safe apply** | MG-3, MG-4, EI-4, DL-1, DL-6, ORCH-1, EI-9 | clear the stale-probe memo on the active edge; batch-derived bulk-deploy deadline or submit-and-poll; relay timeout vs method timeout; global `UniqueName` index; block cross-site Overwrite; sync-dispose ordering; outbox doc/claim | small ×5, medium ×2, one migration |
|
||||
| **WP7 — Flow Debugger follow-ups** | DL-3, MG-7, MG-6, MG-5, DL-4, DL-5, DL-7, DL-8, DL-10, SR-6 | cap Replay + write-kind results; page the site pull (closes register row 117); root id from the anchor; edge dedupe; order-aware merge; parse once; enum tests | small ×10 |
|
||||
| **WP8 — Central listener + config hygiene** | XC-1, XC-2, XC-3, XC-7, CM-2, CM-6, XC-10, XC-9, CM-5/XC-6, CM-4, CM-7, CM-8, EI-6, EI-7, EI-8, DC-6, DC-7, DC-11 | `RequireHost` on the HTTP groups; delete inert keys; env2 bootstrap guard; delete dead RPCs; validators; SqlClient pin; CLI exit codes; notifier 503 failover | small ×18 — mostly one-liners plus a rig config sweep |
|
||||
| **WP9 — Docs sweep** | XC-5, XC-11, XC-12, MG-10, DC-8, DC-9, DL-9, SR-2 doc half, every row of §4 | ClusterClient purge from the entry-point docs; register rows for anything deferred (DL-2 if deferred, EI-9); umbrella index; push | small |
|
||||
|
||||
Phase gate after WP1–WP3 and WP6: full solution build + test, `bash docker/deploy.sh`, then the
|
||||
existing live probes (`failover-drill.sh`, `verify-delmia-notifier.sh` A–J) plus two new ones — a
|
||||
redeploy that narrows an instance's attribute set must drop the device subscription count, and a
|
||||
write issued during a forced reconnect must be refused, not replayed.
|
||||
|
||||
---
|
||||
|
||||
## 6. Inventories (from the cross-cutting pass)
|
||||
|
||||
### 6.A Project dependency graph (`ZB.MOM.WW.ScadaBridge.` stripped)
|
||||
|
||||
```
|
||||
Commons -> (none) [PackageRef: ZB.MOM.WW.Audit only]
|
||||
DelmiaNotifier -> (none) [standalone AOT exe]
|
||||
CLI, ClusterInfrastructure, ConfigurationDatabase, KpiHistory, ScriptAnalysis,
|
||||
Security, SiteEventLogging -> Commons
|
||||
StoreAndForward -> Commons, SiteEventLogging
|
||||
HealthMonitoring -> Commons, StoreAndForward
|
||||
Communication -> Commons, HealthMonitoring
|
||||
TemplateEngine -> Commons, ScriptAnalysis
|
||||
NotificationService -> Commons, StoreAndForward
|
||||
ExternalSystemGateway -> Commons, StoreAndForward
|
||||
DataConnectionLayer -> Commons, HealthMonitoring, SiteEventLogging
|
||||
AuditLog -> Commons, Communication, ConfigurationDatabase
|
||||
DeploymentManager -> Commons, Communication, TemplateEngine
|
||||
NotificationOutbox -> Commons, KpiHistory, NotificationService
|
||||
InboundAPI -> AuditLog, Commons, Communication, ScriptAnalysis
|
||||
Transport -> Commons, ConfigurationDatabase, ScriptAnalysis, TemplateEngine
|
||||
SiteCallAudit -> AuditLog, Commons, Communication, ConfigurationDatabase, KpiHistory
|
||||
SiteRuntime -> Commons, Communication, DataConnectionLayer, HealthMonitoring,
|
||||
ScriptAnalysis, SiteEventLogging, StoreAndForward
|
||||
ManagementService -> Commons, Communication, DeploymentManager, HealthMonitoring,
|
||||
InboundAPI, Security, TemplateEngine
|
||||
CentralUI -> Commons, Communication, DeploymentManager, HealthMonitoring, KpiHistory,
|
||||
ManagementService, NotificationOutbox, ScriptAnalysis, Security,
|
||||
TemplateEngine, Transport
|
||||
Host -> all 22 component projects
|
||||
```
|
||||
No cycles. Nothing but tests references Host. Two cross-role assembly loads are noted, not defects:
|
||||
`AuditLog → ConfigurationDatabase` (central-only EF, loaded on sites via `AddAuditLog`) and
|
||||
`Communication → HealthMonitoring → StoreAndForward` (site-only, loaded on central).
|
||||
|
||||
Shared-lib versions in use: Health 0.3.0 · Telemetry 0.1.0 · MxGateway.Client 0.2.1 ·
|
||||
Configuration 0.1.0 · Auth.* 0.3.0 · Audit 0.1.0 · Theme 0.4.1 · Secrets.* 0.6.2 · LocalDb.* 0.3.4 —
|
||||
all matching the umbrella index. Akka 1.5.62, Grpc 2.76.0 / Protobuf 3.34.1, Microsoft.* 10.0.7
|
||||
uniform; skew only as XC-9.
|
||||
|
||||
### 6.B Composition roots
|
||||
|
||||
| | Central | Site |
|
||||
|---|---|---|
|
||||
| Cluster singletons | `notification-outbox`, `audit-log-ingest`, `site-call-audit`, `audit-log-purge`, `site-audit-reconciliation` (required-for-readiness), `kpi-history-recorder`, `pending-deployment-purge` (best-effort, excluded) | `deployment-manager`, `event-log-handler` (both `role: site-{SiteId}`) |
|
||||
| Per-node actors | `dead-letter-monitor`, `central-communication`, `management` | `dead-letter-monitor`, `site-communication`, `dcl-manager`, `cert-store`, `parked-message-handler`, `SiteReconciliationActor`, `site-audit-telemetry` |
|
||||
| Hosted services | Akka, bootstrap coordinator, inbound key import (SqlServer), script-artifact subscriber, audit partition maintenance, bundle session eviction, health aggregator, health report loop | Akka, bootstrap coordinator, health report sender, storage initializer, event-log purge, audit backlog/retention reporters, scheduler stats, stream drop reporter, run-summary flush, event-log failure counter, LocalDb replication status |
|
||||
| gRPC | `CentralControlGrpcService` + secrets hub | `SiteStreamGrpcServer`, `SiteCommandGrpcService`, LocalDb sync |
|
||||
| Health checks | database, akka-cluster, required-singletons (Ready); active-node + database (Active) | akka-cluster, localdb (Ready); `SitePairActiveNodeHealthCheck` (Active) |
|
||||
|
||||
No duplicate hosted services; `TryAddEnumerable` used correctly; only XC-10's two options classes lack validation.
|
||||
|
||||
### 6.C Cross-cluster RPC inventory
|
||||
|
||||
| Service (host) | RPCs | Interceptor | Caller |
|
||||
|---|---|---|---|
|
||||
| `SiteStreamService` (site) | `SubscribeInstance`, `SubscribeSite`, `PullAuditEvents`, `PullTraceEvents`, `PullSiteCalls` — and dead `IngestAuditEvents`, `IngestCachedTelemetry` | `ControlPlaneAuthInterceptor` | central stream client / audit pull clients |
|
||||
| `SiteCommandService` (site) | `ExecuteLifecycle`/`Opcua`/`Query`/`Parked`/`Route`, `TriggerFailover` | `ControlPlaneAuthInterceptor` | `GrpcSiteTransport` |
|
||||
| `CentralControlService` (central) | `SubmitNotification`, `QueryNotificationStatus`, `IngestAuditEvents`, `IngestCachedTelemetry`, `ReconcileSite`, `ReportSiteHealth`, `Heartbeat` | `CentralControlAuthInterceptor` | `GrpcCentralTransport` |
|
||||
| `LocalDbSync` (site) | library | `LocalDbSyncAuthInterceptor` | peer site node |
|
||||
| Secrets hub (central) | library | `SecretsHubAuthInterceptor` | site secrets sweep |
|
||||
|
||||
Zero removed field lines in `git log -p -- Protos/` since 2026-06-01; 565 fields, 0 proto↔C# mismatches.
|
||||
|
||||
### 6.D Config contract deltas
|
||||
|
||||
Read by code, set on no rig (silently defaulted): `ScadaBridge:{OpcUa, MxGateway, DeploymentManager, SiteCallAudit, ExternalSystemGateway, Sms, SiteRuntime, OperationTracking}`, `Telemetry:{Exporter, OtlpEndpoint}`, `AuditLog:{SiteTelemetry, Purge, Reconciliation, PartitionMaintenance, SiteRetention}`, `SiteEventLog:{PurgeInterval, MaxPurgedRowsPerTick, PurgeCatchUpInterval, QueryPageSize, WriteQueueCapacity}`.
|
||||
Set on the rig, read by nothing: XC-2 (4 SMTP keys), XC-3 (`PurgeScheduleCron`), XC-7 (2 legacy paths).
|
||||
Required keys not checked by `StartupValidator`: `LocalDb:Path` (delegated to `AddZbLocalDb`), `Node:CentralGrpcPort` (defaulted). Every credential literal on both rigs is the documented dev set with its `${secret:}` form named inline.
|
||||
|
||||
### 6.E Deferred-work register (rows 90–118)
|
||||
|
||||
Open: 92, 94, 107, 109, 110, 112, 113, 115, 116, 117, 118 (114 never issued). None should be closed; every open premise re-verified. Row 112 should be re-worded (central now has `Truncated` + `LogsTruncated`; the site half is still unflagged). Row 117 confirmed and extended by MG-6. No dangling row citations in code. New rows to mint from this review if deferred: DL-2, EI-9, XC-3 (if not implemented).
|
||||
|
||||
### 6.F Test-suite shape
|
||||
|
||||
≈ 7 579 `[Fact]`/`[Theory]`/skippable attributes across 32 test projects, all in the slnx; zero static `Skip=`; runtime `Skip.If` concentrated in the MSSQL-fixture and Playwright suites (environment-conditional, not visible in a count); no retry/flaky markers; 56 assertion-less candidates all sampled to throwing helpers — no genuine no-op test found.
|
||||
|
||||
### 6.H Design commitments
|
||||
|
||||
All twelve checked HOLD with a code anchor: active-node definition (`ActiveNodeEvaluator.cs:43`, no `State.Leader` use); single `"scadabridge"` ActorSystem (`AkkaHostedService.cs:190`); UTC (`DateTime.Now`/`DateTimeOffset.Now`/`Today` → 0 hits in `src/`); no ClusterClient; `notification_lists`/`smtp_configurations` unregistered (`SiteLocalDbSetup.cs:60-72`); auto-down emitted (`AkkaHostedService.cs:283-285`); resolver on all four compile surfaces; no `IHealthCheckPublisher`; central never buffers; single trust boundary at five call sites; additive protos; no active `<Protobuf>` item.
|
||||
|
||||
---
|
||||
|
||||
## 7. Test evidence
|
||||
|
||||
`dotnet build ZB.MOM.WW.ScadaBridge.slnx` — **0 warnings, 0 errors**.
|
||||
`dotnet test … --filter "FullyQualifiedName!~PlaywrightTests"` — 29 assemblies, **9 231 passed, 1 failed**:
|
||||
|
||||
`SiteRuntime.Tests.Tracking.OperationTrackingStoreTests.SR024_SyncDispose_DoesNotDeadlock_WhenInvokedFromFreshThread`
|
||||
— `NullReferenceException` at `SqliteTransaction.Complete()` ← `SqliteConnection.Dispose` ←
|
||||
`OperationTrackingStore.Dispose()` (`OperationTrackingStore.cs:635`). This is ORCH-1, a real
|
||||
dispose-vs-pump race, not an environmental flake. The register-row-118 bUnit flake did not fire in
|
||||
this run. Playwright suites need the live rig and were excluded.
|
||||
Reference in New Issue
Block a user