ScadaBridge/docs/plans/2026-08-19-low-findings-remediation-plan.md

Low-Findings Remediation Plan — 2026-08-19 Arch Review

Status 2026-08-20: EXECUTED — all 15 tasks landed (14 via dedicated low/T* worktrees cherry-picked onto low/integration, T5 as LocalDb 0.3.3 in scadaproj 122940c + review follow-up 9663279, published to the Gitea feed and pinned here). Honest outcomes of the two plausible-needs-verification items: P3-4 reproduced (deterministically, by construction; how often the >10s header path occurs in production is NOT demonstrated — noted in the test's doc comment) and P7-5 reproduced (4-instance cascade measured on a real SQLite-backed ScadaBridgeDbContext). Plan decisions 1–3 were all taken as written (P7-6 behaviour fix + --force; P4-6 real fix via SiteLocalDbSetup.EnsureAutoVacuumOnFreshFile — fresh files only, register row 104; P4-7 as 0.3.3, with the FK-parent constraint recorded as register row 105). Integration caught one cross-task regression (T12's Topology fixture vs. T13's new IStaleInstanceProbe ctor dependency) and one bundle-review Minor (T11's copied pattern re-rendered after Dispose) — both fixed in 57c8973b. Register row 103 closed.

Scope: the 21 Low findings left open after the High wave (c4d3c8b8..d18f344e) and the Medium/Med-High wave (d18f344e..9b67c7cb, deployed to the rig), plus register row 103 (the two TTL-bounded lesser instances of P6-6, deferred by the Medium wave to "next CentralUI hardening pass" — that pass is Phase 4 here). Baseline: main @ 9b67c7cb.

Structured as five phases, one per subsystem section. All 15 tasks are file-disjoint across the whole plan, so phases may run concurrently under the usual rolling dispatch — the phase grouping is thematic (one reviewer lens per phase at integration), not a dependency ordering. The one true ordering constraint: T5 (LocalDb 0.3.3) must publish before the pin bump lands on main.

Model assignment rule (same as the Medium wave): Opus for tasks whose defect is a lifecycle/ordering race, a state-machine gap, or a cross-repo library invariant — anywhere the fix can be subtly wrong while tests pass. Sonnet for pattern-mirroring, mechanical guards, index/migration work, docs, and test hygiene where the correct pattern already exists in-repo and the task is to copy it faithfully.

Execution model (unchanged): one dedicated manual git worktree per task (memory rule: isolation:worktree is broken here), model-matched subagent, pathspec commits, cherry-pick onto main, combined build + affected suites, git diff review, single merge session.

Shared constraints (every task)

• Proof of failure: every new test must be shown to fail against the unfixed code (stash/unstash or targeted revert) before the fix lands. Where the defect is timing-dependent and won't reproduce at test sizes, do what T1 did in the Medium wave: inject a modeled delay to prove the mechanism, plus a timing-independent invariant test, and record the honesty note in the test's doc comment.
• No new sleep-then-assert (register row 40): AssertHoldsFor / event-based waits; Watch/Stop/ExpectTerminated on any ticker ≤1s (rows 82/95).
• EF migrations: build first, never --no-build; every migration ships the handwritten-SQL twin under docs/plans/sql/ with parity (Medium-wave T7 precedent).
• Additive-only message/DTO/proto evolution; no proto changes are expected in this plan at all.
• Register and CLAUDE.md edits happen at integration only, never by task agents (file contention). Integration closes row 103 (T12) and applies the P7-6 doc decision.
• Verifier confidence is quoted from the review: findings marked plausible-needs- verification (P3-4, P7-5) require the agent to confirm the defect first and report honestly if it does not reproduce — a guard test still lands, but the register note must say what was and wasn't proven.

Decisions made in this plan (owner may override)

1. P7-6 takes the behavior fix, not the docs fix: bulk DeploySiteAsync gains an up-to-date pre-filter via IStaleInstanceProbe (skipped instances reported in the existing SkippedInstances list with reason "up-to-date"), plus an explicit Force flag (management command + CLI deploy site --force) that restores today's unconditional behavior. Rationale: an unconditional redeploy wipes static overrides and native alarm state on unchanged instances — that destructive side effect, not the wording, is the hazard; and CLAUDE.md already promises "out-of-date" semantics.
2. P4-6 takes the real fix, not the comment deletion: run the auto_vacuum pragma before LocalDbSchema.EnsureCreated via a documented pre-create hook/ordering in SiteLocalDbSetup, with VACUUM fallback explicitly NOT attempted (too costly on a 1 GB live file); if ordering proves impossible without a LocalDb API change, fall back to deleting the misleading comment + register row, and say so.
3. P4-7 ships as LocalDb 0.3.3 (wire-compatible, no schema change), published to the Gitea feed and pinned here in the same integration session — same flow as 0.3.2.

---

Phase 1 — Site Runtime / DCL / Communication (streams)

T1 (Sonnet) — P1-4: projected single-row reads in DeploymentManagerActor

Files: src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs (:1573, :2061, :1489), Services/SiteStorageService.cs (:84), its interface, tests.

HandleEnsureInstanceStarted, HandleDeploymentStateQuery, and HandleEnable each call GetAllDeployedConfigsAsync — materializing ALL configs including ConfigJson blobs — to FirstOrDefault one row. A volume-restore reconcile healing hundreds of instances does hundreds of full-table reads; the sweep tick needs only (name, revision, is_enabled). Fix: add GetDeployedConfigAsync(name) (single row) and a metadata-only projection for the sweep; switch the three call sites; leave semantics identical (T1 of the Medium wave already threads StoreGenerationAtRead here — do not disturb the generation fencing; capture the generation at the same point the full-table read did). Tests: projection returns identical decisions to the old path; generation capture still fences (existing tests keep passing).

T2 (Opus) — P2-5 + P2-6: DataConnectionActor probe seeding + Connecting-state gap

File: src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs (+ Adapters/OpcUaDataConnection.cs read-only for the _disconnectFired trace), tests.

• P2-5: initial subscribe (:984) and reconnect resubscribe (:2329-2344) call SeedTagsAsync; the resolution-probe path (:1975-1988) does not. OPC UA masks it via the initial notification; MxGateway advise can be VT_EMPTY, so a late-resolving static tag stays Uncertain forever. Fix: seed newly probe-resolved tags — same call the other two paths make.
• P2-6: Connecting has no AdapterDisconnected case → Unhandled. A connect-then- immediate-drop can enqueue the disconnect before ConnectResult(true); the adapter's once-only _disconnectFired guard (OpcUaDataConnection.cs:486) means no later Disconnected ever arrives, so a zero-subscription connection sits Connected on a dead adapter. Fix: stash (or handle) AdapterDisconnected in Connecting so it is replayed/processed after the state transition; respect the Medium-wave T3 AdapterGeneration fencing (a stale generation's disconnect must still be dropped). Opus because the fix sits inside the Become/Stash machine next to fresh generation logic. Tests: ordered enqueue disconnect-before-ConnectResult ends Disconnected/ reconnecting, not Connected; stale-generation disconnect still ignored.

T3 (Opus) — P3-4 + P3-5: stream open-attempt deadline + duplicate-subscribe cap order

Files: src/ZB.MOM.WW.ScadaBridge.Communication/Actors/SiteAlarmAggregatorActor.cs (:800, :444), Grpc/SiteStreamGrpcServer.cs (:368-385), tests.

• P3-4 (plausible-needs-verification): if response headers take >10s (ConnectedHeaderTimeout) and an alarm-only site stays quiet, GrpcAlarmStreamConnected never fires; _streamDown stays true but _openInFlight stays pinned, so OnReconcileTick's reopen is skipped until the 4h stream lifetime cycles. Mitigated by the server header flush — verify first whether the starvation is still reachable; regardless, add an open-attempt deadline that clears _openInFlight so reconcile can retry (self-healing beats reachability analysis).
• P3-5: duplicate-subscribe cancels the incumbent stream (:368-379) then checks the cap (:385): at cap, a reconnect kills the healthy old stream and refuses the replacement (ResourceExhausted) → zero streams until retry. Fix: treat the duplicate case as a replacement (exclude it from the cap check) or check the cap before cancelling; must not regress the newest-wins semantics. Tests: at-cap reconnect ends with exactly one live stream; cap still refuses a genuinely new subscriber.

Phase 2 — LocalDb / Site Event Log

T4 (Sonnet) — P4-5 + P4-6: purge startup yield + auto_vacuum reality

Files: src/ZB.MOM.WW.ScadaBridge.SiteEventLogging/EventLogPurgeService.cs (:169, :208-212, :516-519), SiteEventLogSchema.cs (:42-46), src/ZB.MOM.WW.ScadaBridge.Host/SiteLocalDbSetup.cs, tests.

• P4-5: ExecuteAsync runs the initial RunPurge() synchronously before its first await, so StartAsync blocks every later hosted service (including AkkaHostedService) behind the purge — ~17s ceiling-limited, ~285s unpaced worst case; a throwing gate is also treated as active (:208-212). Fix: await Task.Yield() first (and make the gate's throw path fail closed with a logged fallback, not "active"). Test: hosted-service start returns promptly with a slow purge stubbed in.
• P4-6: auto_vacuum=INCREMENTAL is only honored on a table-less database, and LocalDbSchema.EnsureCreated runs first — so the consolidated file is permanently auto_vacuum=NONE and the per-trim PRAGMA incremental_vacuum is a silent no-op while the adjacent comment claims otherwise (the standalone auditlog.db does it right, SqliteAuditWriter.cs:154-157). Fix per plan decision 2: issue the pragma before any table exists (pre-EnsureCreated ordering in SiteLocalDbSetup); if that requires a LocalDb API change, do NOT make one in this task — delete the trap comment, log a one-time Information note that vacuum is unavailable on legacy files, and report for a register row. Note either way: existing rig files already have tables, so the pragma only helps fresh files — document that in the code comment honestly. Test: fresh file reports PRAGMA auto_vacuum = 2.

T5 (Opus) — P4-7: LwwApplier latent hazards → LocalDb 0.3.3 (cross-repo)

Files (scadaproj): ~/Desktop/scadaproj/ZB.MOM.WW.LocalDb/src/.../Internal/LwwApplier.cs (:95-99, :273), SqliteLocalDb.cs (RegisterReplicated), Directory.Build.props (0.3.2 → 0.3.3), tests. Files (here, at integration): Directory.Packages.props pin.

• Hazard 1: INSERT OR REPLACE's implicit delete fires FK ON DELETE CASCADE with capture suppressed (foreign_keys=ON) → silent divergence for any future FK'd replicated table (none of the ten today — verified by the review). Fix: RegisterReplicated gains an FK guard — refuse (clear exception) to register a table that is the parent of an FK with cascading delete from another table, and document the constraint in the LocalDb README.
• Hazard 2: the tombstone apply path runs DeleteAsync + WriteRowVersionAsync as two statements under a poison catch — a failure between them leaves a deleted row with a stale live ledger (SnapshotStreamer warns+skips). Fix: make the pair atomic (single transaction), preserving the poison-catch semantics around the transaction.

Wire-compatible, no schema change, no ScadaBridge code change. Publish 0.3.3 via build/push.sh (env-held GITEA_NUGET_SOURCE/GITEA_NUGET_KEY), bump the three-package pin here, restore/build 0/0. Opus: replication-library invariants where a wrong fix diverges pairs silently. Tests: FK-parent registration throws; injected failure between delete and ledger write leaves both or neither. Integration also updates the umbrella scadaproj/CLAUDE.md index (0.3.3 entry) per the cross-repo propagation rule.

Phase 3 — Central Outbox / Audit / KPI

T6 (Sonnet) — P5-3 + P5-4: Notifications list index + operator-path status guard

Files: src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/NotificationOutboxRepository.cs (:192-203, :280-294), a new EF migration + docs/plans/sql/ twin, src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxActor.cs (:1088-1129, :1172-1213), tests.

• P5-3: the list page sorts CreatedAt DESC, NotificationId DESC but every index leads elsewhere → the default page is a 2× whole-table scan/sort (COUNT + page). Fix: add IX_Notifications_Created_NotificationId (CreatedAt DESC, NotificationId DESC) — exact mirror of the Medium wave's IX_SiteCalls_Created_TrackedOperationId (T7), including the VarChar-typed FromSqlRaw parameter check and the handwritten SQL.
• P5-4: operator Retry/Discard is read-check-write keyed on NotificationId only; detached PipeTo tasks interleaving with the dispatcher can stamp a Delivered row Discarded with stale fields. Fix: add a Status == Parked predicate to the operator-path ExecuteUpdate (conditional update, rows-affected checked — the CAS shape T-P6-1 used); surface "already progressed" to the caller instead of silent overwrite. Tests: migration parity; Retry/Discard against a row that just went Delivered is a no-op with the honest reply.

T7 (Sonnet) — P5-5: cross-validate the rate-fold interval triple

Files: src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/KpiHistoryRepository.cs (:169-172), the options classes' validators (KpiHistoryOptions, SiteCallAuditOptions, the outbox DeliveredKpiWindow owner), tests.

Rate classification silently assumes DeliveredKpiWindow (1 min) == SiteCallAuditOptions.KpiInterval (1 min) == KpiHistoryOptions.SampleInterval (60s); widening DeliveredKpiWindow to 15 min makes rollups over-count ~15×. Fix (pick the robust half): compute the fold against SampleInterval directly where possible, and add eager cross-validation (ValidateOnStart, the §1.5 pattern) that fails boot with a message naming all three keys when they diverge — do not leave a validator-only fix if the computation can simply stop depending on the equality. Tests: divergent config fails validation with the three keys named; fold correct at a non-default SampleInterval.

T8 (Sonnet) — P5-6: deterministic synthetic ReconciliationAbandoned rows

File: src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/SiteAuditReconciliationActor.cs (:312-336, :362-364), tests.

After MaxPermanentInsertAttempts removal, the cursor held at since by a co-batch unresolved row re-pulls the abandoned row; its attempt counter restarts and a new synthetic ReconciliationAbandoned row is minted with a fresh EventId every 5-tick cycle. Fix: make the synthetic EventId deterministic — derived from the lost row's EventId (e.g. namespaced GUID) — so re-abandonment is idempotent under the existing insert-if-not-exists ingest; keep an in-memory abandoned-set only as a cheap short-circuit, never as the correctness mechanism (it dies with the singleton). Tests: same lost EventId abandoned twice yields one row; different lost rows yield distinct synthetic ids.

Phase 4 — Central UI / Management / Deployment (the "CentralUI hardening pass")

T9 (Sonnet) — P6-10: strict enum parsing in ManagementActor (row-88 class)

File: src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs (:2922, :2937, :2854, :2867 — alarm trigger type, ParseDataType, ParseElementDataType), tests.

Raw Enum.Parse accepts ordinals ("2") and undefined values ("99" → deploy-time bomb); a typo surfaces as a masked "internal error". Fix: apply the ParseDeploymentStatus guard pattern (:2745) to all three parse sites — names only, defined values only, honest error naming the field and the valid set. Mind the Medium-wave T8 nuance: Enum.TryParse accepts comma-joined name lists and bare ordinals; the guard must reject both (reuse or mirror ParseEnumListStrict's comma-token rejection from Commons where a single value is expected). Tests: ordinal, undefined, comma-joined, and typo inputs all rejected with the field-naming error; every defined name accepted.

T10 (Sonnet) — P6-11: request-generation guard on Deployments page

File: src/ZB.MOM.WW.ScadaBridge.CentralUI/.../Deployments.razor (:428, :500-552), bUnit tests.

Three fetch initiators (search, paging, timer) interleave at awaits and a stale flight overwrites a newer grid; _currentPage = 1 is also written on the Timer thread (:428). Fix: a monotonic request generation captured at fetch start and checked at apply (last-write-wins by generation, not by completion order); marshal the timer's state writes through InvokeAsync. Tests: delayed stale response arriving after a newer one does not overwrite; timer refresh does not reset the page mid-search.

T11 (Sonnet) — P6-12: dispose sandbox-run CTS in the two script forms

Files: src/ZB.MOM.WW.ScadaBridge.CentralUI/.../ApiMethodForm.razor (:233), SharedScriptForm.razor, bUnit tests.

Neither form implements IDisposable, so an abandoned sandbox run parks a thread on GetAwaiter().GetResult() until the script timeout and its continuation touches disposed state. TemplateScriptDialog.razor:354-358 does it right. Fix: copy that pattern exactly — IDisposable, cancel+dispose _runCts, guard the continuation on disposal. Tests: disposing the form mid-run cancels the sandbox run; continuation after dispose is a no-op.

T12 (Sonnet) — Register row 103: TransportImport slot strand + Topology timer

Files: src/ZB.MOM.WW.ScadaBridge.CentralUI/.../TransportImport.razor.cs (:244), Topology.razor (:201-214), bUnit tests.

The two TTL-bounded lesser instances of the P6-6 class, deferred by the Medium wave: TryLoadAsync abandoned mid-start strands 1 of 8 import session slots for 30 min (partially defeating row 43), and Topology.razor's timer survives disposal in the same window shape. Fix: apply the Medium wave T10 pattern (DebugStreamHub's take-once _sessionLock/TakeSession()) — post-await disposal re-check with exactly-once cleanup ownership; timer created under the same discipline. Closing row 103 happens at integration. Tests: dispose-during-start releases the import slot immediately; disposed Topology page leaves no live timer (Watch/ExpectTerminated-style assertion, not sleep).

T13 (Opus) — P7-5 + P7-6: bulk-deploy DbContext poisoning + out-of-date pre-filter

Files: src/ZB.MOM.WW.ScadaBridge.TemplateEngine/.../DeploymentService.cs (:677-683, :804), StaleInstanceProbe/IStaleInstanceProbe call path, the bulk-deploy management command + SiteDeploymentSummary (additive), CLI deploy site (--force), tests.

• P7-5 (plausible-needs-verification): the three bulk phases share one scoped DbContext; a persistent SaveChanges failure on one tracked entity rethrows on every later save including MarkDeploymentFailedAsync cleanup — later instances fail and records stay InProgress (locks are released; reconcile self-heals next deploy). Verify first with a forced save failure; fix: detach the failed entries on cleanup-save failure so subsequent instances save cleanly.
• P7-6 (plan decision 1): implement the up-to-date pre-filter using IStaleInstanceProbe (its memo timing was fixed by Medium-wave P7-4 — FlattenSession.VisitedTemplateVersions records at the right point, so the probe is trustworthy here); up-to-date instances land in the existing additive SkippedInstances list with reason "up-to-date"; Force = true (management command, additive; CLI --force) restores unconditional behavior. Must compose with the P7-2 Disabled-skip: Disabled wins (never deployed even under --force at bulk scope — single-instance deploy remains the explicit re-enable path). Opus: deployment-semantics change composing three prior fixes. Tests: unchanged instance skipped with reason and its static overrides untouched; --force deploys it; Disabled skipped under both; poisoned entity no longer cascades.

Phase 5 — Tests / Infra / Docs

T14 (Sonnet) — P8-8 + P8-9: residual test-health instances

Files: tests/.../DataConnectionLayer.Tests/Actors/DataConnectionActorBatchTests.cs (:181-184, :206, :214), SiteAlarmLiveCacheServiceTests.cs (:453-454, :471-472), StoreAndForwardServiceTests.cs (:891-892), KpiHistoryRecorderActorTests.cs (:1043-1049), tests/.../Host.Tests/ScadaBridgeTelemetryTests.cs (:30).

• P8-8: the remaining row-40-class wall-clock sleeps outside Communication.Tests — apply the Medium-wave T17 treatment (AssertHoldsFor for absence assertions, event/condition waits for presence; no widened tolerances that mask real regressions).
• P8-9: SetQueueDepthProvider(() => 5) mutates the process-global row-32 slot with no clear and no [Collection] — a cross-assembly landmine. Fix: compare-and-clear in finally (restore prior provider), plus a collection if any second reader appears. Proof-of-failure here = demonstrate the pre-fix flake mechanism (e.g. run the sleep assertion under load) or, where impractical, the honesty-note pattern.

T15 (Sonnet) — P8-7 + P8-10: docs drift + env2 parity tail

Files: docker/README.md (:74, :155), docker-env2/README.md (:56), docker-env2/docker-compose.yml (:34 area), docker-env2/teardown.sh (:13, :163-171), CLAUDE.md (:15, at integration if contended), infra/ compose comment.

• P8-7: both rig READMEs still list the retired scadabridge-ldap container; the infra compose retirement comment is true only of env2; CLAUDE.md names Traefik as an infra/ service (it is per-rig, docker/docker-compose.yml:411) and points at scadaproj/infra/glauth where docker/ actually uses glauth-redundant. Fix: correct all five spots to match the running rigs (LDAP = shared GLAuth 10.100.0.35:3893; name the actual glauth config each rig uses).
• P8-10: env2 maps no host port for the central gRPC control plane (docker/ maps 9013/9014→8083; 9113/9114 are free — use them), and teardown.sh still claims data is "preserved in node directories" — untrue since the named-volume migration — with no volume-rm recipe. Fix: add the port mappings + rewrite teardown's data note with an explicit docker volume rm recipe (destructive step clearly labeled). Verify env2 still boots: bash docker-env2/deploy.sh gate (it runs concurrently with docker/ — do not touch docker/ ports).

---

Integration (single session, after all phases report)

1. Cherry-pick order: T5 (LocalDb 0.3.3 publish + pin) first or last-but-isolated; everything else in phase order. Interim build every ~6 picks.
2. Combined dotnet build ZB.MOM.WW.ScadaBridge.slnx 0/0, then the full sweep (known flakes: SiteConnectionUpGauge_*, DCL027, IO-errno under load — rerun in isolation before calling regression).
3. Register: close row 103 (T12); add rows for anything a task honestly deferred (P4-6 fallback branch, P3-4 if starvation didn't reproduce); new row for the P4-7 FK-guard constraint ("replicated tables must not be cascade-delete FK parents").
4. CLAUDE.md: bulk-deploy bullet gains the out-of-date filter + --force sentence (P7-6); LocalDb bullet gains the 0.3.3 line; fix the :15 Traefik/glauth drift if T15 left it for integration. Umbrella scadaproj/CLAUDE.md: 0.3.3 entry.
5. git diff review, single descriptive merge-session commit set; push and rig redeploy only on explicit request.