diff --git a/docs/plans/2026-08-19-low-findings-remediation-plan.md b/docs/plans/2026-08-19-low-findings-remediation-plan.md new file mode 100644 index 00000000..578beafd --- /dev/null +++ b/docs/plans/2026-08-19-low-findings-remediation-plan.md @@ -0,0 +1,357 @@ +# Low-Findings Remediation Plan — 2026-08-19 Arch Review + +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.