ScadaBridge/docs/plans/2026-10-21-system-review-remediation.md

# System review remediation (2026-10-21)

Decision and execution record. Source: eight read-only parallel slice reviews of `main @ 513fa1aa` (findings in the review directory `00-SUMMARY.md` + `01..08`), excluding items already known from the 2026-09-09 arch review, the deferred-work register and CLAUDE.md. Remediation base for wave 2 docs: `1627513f`.

## Scope

8 slices (SiteRuntime; site data path DCL/S&F/event log; Communication + cluster; Deploy/Template/ScriptAnalysis/Transport; Management/InboundAPI/Security/ESG/Host; Audit/SiteCalls/Outbox/Notify/KPI/Health; Central UI + CLI; ConfigDB/Commons + gap inventory). ~107 findings: 4 High, ~30 Medium, ~73 Low. Recurring themes: non-collectible Roslyn loads, tracked whole-graph template reads for id/name lookups, uncached LDAP bind + N+1 role mapping, reconciliation paths that replay everything row-at-a-time, options validated but never read, sticky process-wide failure caches.

## Owner decisions

- **CLI auth = bearer per invocation.** One LDAP bind at `POST /auth/token`, then `Authorization: Bearer` on every management/audit/debug-stream call; Basic remains the fallback. Roles in a token are authoritative until expiry (<= `JwtExpiryMinutes`, 15); rotating `JwtSigningKey` invalidates all tokens; `StartupValidator` requires a >= 32-byte key on Central; `/auth/token` answers 404 under `DisableLogin`.
- **`Database:MachineDataDb` removed** (a required boot key nothing read).
- **Config audit stores summaries + optional retention** (script body = length + SHA-256; `AfterStateJson` capped 256 K; `ConfigAudit:RetentionDays` default 0 = keep).
- **Bound Test Run gate:** Test Run against a deployed instance needs Deployer + site scope and writes an audit row (closes the Flow Debugger F2/F3 asymmetry).
- **Overrides seed, not fire:** persisted static overrides loaded at actor start seed script state without firing scripts; alarms still evaluate.

## Lanes (one line each)

- **A SiteRuntime:** actor-name gate (`ActorNameRules`) + guarded `ActorOf` so a bad name can never restart the singleton (High); overrides republished/seeded on start; per-site serialised artifact apply with 10-min watchdog; metadata-only reconcile sweep; batched `Route.To().Get/SetAttributes`; DCL connection actor names ASCII-sanitised with collision rejection; startup warm/parallel compile.
- **B Scripts:** compile-cache eviction counter/warning + configurable bound (`ScriptCompileCacheMaxEntries`), faulted compiles never cached, scheduler slot reuse, shed-log rate limiting, native-alarm deletes batched into the flush.
- **C DCL:** same-adapter reconnect advances the session generation at issue time (High); MxGateway subscription ids `{session}:{handle}`; `IDataConnection.WriteBatchAndWaitAsync` removed; `MxAlarmStreamReopenDebounce` 200 ms; `ClientNamePrefix` now live; OPC UA discovery async and bounded by `OperationTimeoutMs`.
- **D Store-and-forward / event log:** per-lane ranked paging, gauge re-seed; event-log reads off the write lock with `KeywordSearchMaxWindow`; cap-trim count refresh every 10th batch, one vacuum per tick.
- **E Communication:** failback probe is a real readiness RPC (sentinel `$scadabridge-failback-probe`, 5-min probation); site-address refresh single-flight with deadline + sequence; streaming consumers re-resolve addresses on reopen; reconcile cancellation flows from the gRPC call; malformed address keeps the old channel (High); dead `SiteContacts` code deleted.
- **F Script analysis / Template Engine:** compile gate no longer emits/loads assemblies; trust verdicts cached by code hash; `BlockingWaitAnalyzer` warning; `ActorNameRules` at save; lean template projections.
- **G Deployment:** hash-only revision computation; single `FlattenSession` for bulk deploy pre-filter + prepare; reconcile fixed round trips with 3 retries; system artifact deployment records purged under `TerminalDeploymentRecordRetention`.
- **H Site audit:** reconciliation pull serves only un-acked rows; new site-only `PushAbandoned`/`Rejected` states; `IngestAck.rejected_event_ids` (field 2); persisted high-water; set-based paged central ingest (High); paged purge; fail-fast writer; per-site state pruning.
- **I Site calls / outbox / notify / KPI:** node-local `OperationTrackingCentralAck` ledger for un-acked pull; 128-row upsert chunks; conditional-aggregate KPI queries; outbox per-attempt timestamps, Pending/Retrying split claim, `DispatchSweepBudget` 2 min; SMTP `MaxConcurrentConnections` clamps Email parallelism; OAuth2 token cache eviction; KPI fold hour-at-a-time.
- **J Host / Inbound / ESG:** single-flight method compile and warm-up service, relay nonce single-use, in-memory request buffering, `ApiMethodCache` pruning, JSON GET/DELETE query params, central definition cache (`DefinitionCacheTtl` 10 s), secret-store fault vs missing PSK, NodeOptions-bound ports.
- **K Security / CLI:** bearer-per-invocation, `RoleMappingCache` (30 s, invalidated post-commit by the management handlers), 10 s display-state memo, dead refresh members removed from `JwtTokenService`.
- **L Central UI:** blocking-wait warning as Monaco marker, bounded script-analysis payload/concurrency, Topology behind `PollGate`, lean projections, bounded caches/rows, safe-invoke fire-and-forget, negative TTL for a faulted pending-updates flight.
- **M Management:** ListInstances SQL paging/prefix search, 409 `IMPORT_IN_PROGRESS` single-flight for large bodies, `CompileForPublish` once per save, site address + native-alarm-source name validation, terminal `PendingSecuredWrites` purge (`SecuredWriteTerminalRetention` 90 d).
- **N Transport:** import validates site gRPC addresses and actor-safe names as blockers; compressed decrypted content pinned on the session and zeroed on every release; template library read once; `StaleInstanceIds` covers derived/composing templates; `Transport:SchemaVersionMajor` removed.
- **O/P ConfigDB:** deterministic config-audit paging, `InstanceLocator` single projected join, dead repository members deleted, config-audit purge + `ConfigAuditOptions` binding, migration `ReviewRemediationIndexes`.
- NuGet: `System.Security.Cryptography.Xml`/`Pkcs` and SignalR.Common pinned to 10.0.13.

## Adversarial-review catches (fixed before merge)

- **C reconnect race:** the first fix advanced the session generation when the reconnect *result* was applied, so a new session dying before the actor processed that result had its `Disconnected` (stamped with the old generation) dropped as stale — leaving the actor Connected on a dead session with frozen tags. The generation now advances when the reconnect attempt is issued.
- **H central-fault refusals:** central answers DB faults with an empty/partial ack, which the new refusal counting treated as per-row refusals (a SQL maintenance window would mark rows `Rejected`). Central now names only row-intrinsic SQL failures in `IngestAck.rejected_event_ids`; the served high-water mark is persisted so owed rows retire after a site restart; transient faults never count toward reconciliation abandonment.
- **A data-connection actor names:** the actor-name gate covered instances/scripts/alarms but not DCL connection actors; names are sanitised and collisions rejected at deploy.
- **I outbox double-send:** the new Pending/Retrying split claim back-filled Pending with an offset (`Skip(n)`); an operator Retry flipping a Parked row to Pending mid-claim shifted the offset so one notification was claimed twice and delivered twice. The back-fill is now a keyset continuation plus a distinct-id guard.
- Also: failback probe sentinel changed to an illegal actor name; alarm aggregator reconcile no longer reopens during a pending backoff reconnect (double subscribe); `RoleMappingCache` invalidation after commit (lane K review M1).

## Deferred to the register

Rows 128-135 of `docs/plans/2026-07-08-deferred-work-register.md`: TLS production profile; `ExternalDefinitionLookupCache` invalidation; whole-graph load on template member add/update (04/P6 partial); idle `SiteStreamGrpcClientFactory` client eviction; compile-cache eviction Meter; dedicated rejected-rows health field; oversize audit row wedge (accepted); possibly redundant config-audit indexes. Row 36 annotated (same free-text `LIKE` shape on Instances/config audit; ListInstances now prefix-paged). Row 117 closed (already fixed by arch-review MG-6).

## Rollout notes

- **Not deployed:** the docker rig is rebuilt from main; QA (vq03/vq04) and prod (vd03) are not deployed.
- **Migration:** `ReviewRemediationIndexes` adds `AuditLog` `IX_AuditLog_Kind_Occurred` and `IX_AuditLog_Actor_Occurred` (partition-aligned) plus three composite `AuditLogEntries` indexes. Production applies `docs/plans/sql/ReviewRemediationIndexes.sql` by hand (index builds on a large partitioned `AuditLog`: schedule off-hours). Not yet applied to QA/prod.
- **Mixed version:** upgrade central first (new `IngestAck` field 2 and proto additions are additive; an old central ignores `rejected_event_ids` and an old site never rejects). Pair upgrades stay both-nodes-together for LocalDb sites.
- **Config changes:** CLI hosts need a central that serves `/auth/token` (older centrals fall back to Basic automatically); Central must have a >= 32-byte `JwtSigningKey` or fails startup validation; remove `Database:MachineDataDb` and `Transport:SchemaVersionMajor` from appsettings (ignored if left).
- **New keys (all defaulted):** `ScadaBridge:ConfigAudit:{RetentionDays 0, PurgeBatchSize 5000, MaxBatchesPerTick 200, PurgeInterval 1d}`; `Security:RoleMappingCacheSeconds 30`; `SiteEventLog:{KeywordSearchMaxWindow 7d, CapTrimCountRefreshBatches 10}`; `AuditLog` site writer/reconciler `{MaxRejectedPushAttempts 10, PurgeBatchSize 5000, PurgeVacuumPagesPerBatch 1000, MaxPagesPerTick 50}`; `SiteRuntime:ScriptCompileCacheMaxEntries 1024`; `NotificationOutbox:DispatchSweepBudget 2m`; `ManagementService:{SecuredWriteTerminalRetention 90d, SecuredWriteTerminalPurgeInterval 1h}`; ESG `DefinitionCacheTtl 10s` (central); DCL `MxAlarmStreamReopenDebounce 200ms`; `DeploymentManager:TerminalDeploymentRecordRetention` governs system artifact deployment record purge.
- **Behaviour changes operators will notice:** `instance list --search` is prefix-only; Test Run against a deployed instance needs Deployer + site scope; deploys reject non-actor-safe names (rename before redeploy); a redeployed instance's persisted overrides no longer re-fire value-change scripts on start.