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

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

Scope: all Medium and Medium-High findings from the 2026-08-19 updated architectural review (report artifact 43c7fa20-9626-4d59-a9c9-061524794660). The four Highs (P2-1, P2-2, P6-1, P6-2) were fixed in the preceding wave (c4d3c8b8..d18f344e, deployed to the rig). Baseline: main @ d18f344e.

Two borderline findings are rounded in because they are cheap and adjacent to in-scope work: P3-3 (Low-Med, Debug View dual-address requirement) and P4-4 (Med-Low, standby event-log retention). All plain-Low findings stay out of scope (P1-4, P2-5, P2-6, P3-4, P3-5, P4-5..P4-7, P5-3..P5-6, P6-10..P6-12, P7-5, P7-6, P8-7..P8-10).

Register consequences (handled at integration, NOT by task agents, to avoid file contention): row 99 reopened+closed by T8 (P6-3 shows the closure was defective); row 95 reopened+closed by T17 (P8-6 shows its premise correction was wrong).

Execution model (same as the High wave): file-disjoint tasks, one dedicated manual git worktree + model-matched subagent each, pathspec commits, cherry-pick onto main, combined build + affected suites, git diff review, single merge session. Two waves.

Decision made in this plan (owner may override later): P7-2 takes the conservative option — bulk DeploySiteAsync skips Disabled instances and surfaces them in the result as skipped, matching the reconcile path's deliberate preservation of Disabled. Rationale: the alternative (silently force-enabling deliberately-stopped equipment on a bulk push) is the hazard the finding names.

---

Wave 1

T1 (Opus) — P1-1 + P1-2: DeploymentManager races (heal resurrection; redeploy override reset)

Files: src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs, InstanceActor.cs, the EnsureInstanceStartedResult message, InstanceActorTests.cs.

• P1-1 (Med): HandleEnsureInstanceStarted (:1571) re-reads the store off-thread but captures no _storeWriteGeneration; the result handler never calls IsStoreSnapshotStale (unlike boot :521 and sweep :1726). A Disable/Delete processed between read and result resurrects the instance permanently (sweep has no runtime→store stop leg; disabled rows continue, deleted rows are absent). Verifier note: trigger is the ONE-SHOT startup reconcile (SiteReconciliationActor.cs:267), so the window is boot-adjacent; the stale comment at :1567-1570 must go. Fix: thread the generation captured at read through EnsureInstanceStartedResult; discard on mismatch (boot/sweep pattern). Test: heal result arriving after a Disable/Delete is dropped.
• P1-2 (Med, arguably Med-High): ApplyDeployment creates the new InstanceActor (:1202) BEFORE the Task.Run that ends in ClearStaticOverridesAsync (:1222-1234); the new actor's PreStart override load typically beats the clear (verifier: likely interleaving) → pre-redeploy overrides applied for the actor's lifetime, violating "reset on redeployment". Leg 2: old actor's PostStop fire-and-forget FlushDirtyStaticOverrides (InstanceActor.cs:363-364) can durably commit AFTER the clear. Fix: on redeploy, clear overrides in the persistence task BEFORE CreateInstanceActor (or gate the override load behind the persistence result), and order the old actor's final flush before the clear (e.g. await/flush before actor creation — Terminated is already awaited :1004-1066, so flushing synchronously in PostStop's stop path or draining before clear are both acceptable; pick the minimal ordering that closes both legs). InstanceActorTests.cs:355-356 pins the WRONG contract — fix the test to pin the right one. Tests: redeploy never applies pre-redeploy overrides; late flush cannot resurrect.

T2 (Sonnet) — P1-3: Alarm on-trigger runs bypass the stuck-script watchdog

File: src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs (RunAlarmScriptAsync :454-537 / LaunchAlarmScript :149-186).

Script path arms watchdog + slot capture (:244-253); alarm path does none on the same bounded pool — a wedged alarm body silently costs a pool thread forever (bounded 4/alarm by the in-flight cap), with no site event, no detach, no healthCollector error increment, no ScriptRunSummaryRecorder. Fix: mirror the script path exactly — slot/stamp/ watchdog + health/summary threading (timeout catch today is LogWarning-only :518-524). Tests: first tests to drive LaunchAlarmScript — wedged alarm body triggers watchdog detach + site event + DetachedScriptThreads; timeout increments health error.

T3 (Opus) — P2-3 + P2-4: DCL ConnectResult generation; OPC UA batch rollback

Files: src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs, Adapters/RealOpcUaClient.cs.

• P2-3 (Med): ConnectResult piped untagged (:777-787). Reconfigure mid-connect: old adapter's ConnectResult(true) drives ReSubscribeAll+BecomeConnected against the NEW never-connected adapter; new adapter's result dropped in Connected; idempotent- reconfigure guard (:644-652) then blocks recovery until restart/redeploy. Fix: tag ConnectResult with the adapter generation (stamped at connect launch, exactly the P2-1 pattern now on SubscribeCompleted — reuse it); drop mismatches. Tests mirror DataConnectionStaleCompletionTests for the connect leg.
• P2-4 (Med): CreateSubscriptionsCoreAsync (RealOpcUaClient.cs:663): mid-batch ApplyChangesAsync throw leaves all created items registered (_callbacks/_itemShard/ ItemCount/SDK items :628-661) with no rollback, and IsConnectionLevelFailure (DataConnectionActor.cs:1464-1471) misses ServiceResultException → chunk treated per-tag unresolved, each backoff probe RE-ADDS items → unbounded shard growth. Fix: unwind created via ReleaseFailedItem on throw; classify SDK bad-session codes connection-level. Tests: fake-SDK mid-batch throw leaves no residue; bad-session code drives the connection-level path.

T4 (Opus) — P3-1 + P3-2 + P3-3: gRPC client lifecycle/disposal races

Files: src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/SiteStreamGrpcClient.cs, Grpc/SitePairChannelProvider.cs, DebugStreamService.cs.

• P3-1 (Med): RegisterSubscription (:147-153) disposes the prior handler's CTS while its ConsumeStreamAsync may still read the token (ODE at :219/:294/:359/:364); Unsubscribe (:455-462) unguarded Cancel; ReleaseResources (:609-619) unguarded iteration whose ODE aborts the factory's DisposeAsync loop leaving later clients undisposed; an ODE from prior.Cancel() escapes the NEW SubscribeAsync with the CURRENT generation → phantom fault. Fix: cancel-only on replacement (dispose only from the owning path), ODE-guard every Cancel, keep factory disposal loop fault-isolated. SiteStreamGrpcClientTests.cs:680 pins current behavior — re-pin.
• P3-2 (Med): UpdateSite/RemoveSite (:123-131/:155-159) Dispose channels under pair.Gate while ExecuteAsync invokes outside the lock (:203-215) → 60s DB refresh with an edited address kills in-flight RPCs (multi-minute DeployArtifactsCommand) with ODE/Cancelled (not Unavailable → no cross-node retry). Fix: deferred disposal — park replaced channels until quiescent or time-delayed; mirror the stream factory's redesign rationale (SiteStreamGrpcClientFactory.cs:12-22).
• P3-3 (Low-Med, rounded in): DebugStreamService (:111-114) throws unless BOTH gRPC node addresses are set, though single-endpoint sites are supported elsewhere. Fix: apply the N9 degeneration (SiteAlarmLiveCacheService.cs:324-330): reuse the sole endpoint.

T5 (Opus) — P4-1 + P4-3: LocalDb sync divergence + liveness (cross-repo)

Files (scadaproj): ~/Desktop/scadaproj/ZB.MOM.WW.LocalDb/src/.../Internal/SyncSession.cs, OplogStore.cs, SyncBackgroundService.cs, tests. Files (here): src/ZB.MOM.WW.ScadaBridge.Host/Program.cs (site Kestrel h2 keepalive), Directory.Packages.props (0.3.1 → 0.3.2 when published).

• P4-1 (Med): PumpLoopAsync never checks needs_snapshot after a cap-prune though OplogStore.ReadBatchAboveAsync documents the obligation (OplogStore.cs:27-29) — maintenance prunes above _sentThruSeq on a slow-but-alive peer → pump silently skips the gap, pair diverges for the session's lifetime (heals only on reconnect). Fix: fault the live session when EnforceCapsAsync prunes into the in-flight range (the 0.3.1 fault-teardown control-lane flush already exists — reuse it). Test: prune during an active session faults it; reconnect serves the snapshot.
• P4-3 (Med): no keepalive anywhere on the sync stream: bare GrpcChannel.ForAddress client-side, no Kestrel Http2.KeepAlivePingDelay on site 8083 → hard crash/partition of the dialing node leaves the passive node's handler hung on MoveNext holding _active=1, rejecting the rebooted initiator's reconnects AlreadyExists, possibly forever; zombie side reports LocalDbReplicationConnected=true. Fix: client SocketsHttpHandler KeepAlivePing (delay/timeout, always-with-active-requests) in SyncBackgroundService; Kestrel h2 keepalive on the site gRPC listener in Host Program.cs; AND newest-wins eviction (or a bounded read deadline) on the server's single-stream guard so a fresh authenticated Sync can displace a dead one.
• Version/publish: bump LocalDb to 0.3.2, commit in scadaproj (do NOT push); build/push.sh needs the user-held GITEA_NUGET_KEY — if absent, leave the ScadaBridge Directory.Packages.props bump as a flagged follow-up and land only the Host keepalive here. The ScadaBridge-side Kestrel change is independent and lands regardless.

T6 (Sonnet) — P4-2 + P4-4: audit-writer durability; standby event-log retention

• P4-2 (Med, arguably Med-High): AuditLog/Site/SqliteAuditWriter.cs — per-row TrySetResult (:487/:499) BEFORE transaction.Commit() (:503); commit failure (SQLITE_FULL/IOERR) no-ops TrySetException → FallbackAuditWriter sees success, no ring fallback, whole batch silently lost in exactly the outage the ring exists for; SiteAuditWriteFailures stays 0. Bonus (verifier): a mid-batch insert failure on row k loses rows 0..k-1 the same way. Fix: complete TCSes only after commit (the pattern SiteEventLogger.FlushBatch already uses :319/:357-361); on commit/insert failure fail EVERY row's TCS so the ring fallback engages. Tests: failed commit → all TCSes faulted + ring receives batch + failure counter increments.
• P4-4 (Med-Low, rounded in): SiteEventLogging/EventLogPurgeService.cs:215-219 early-exits on the standby on the stale premise "standby receives no writes" — DCL actors run on both nodes since rows 89/90/97 and write site events. On an unreplicated site (site-b/c posture) the standby's site_events grows forever. Fix: let the standby run retention/cap against its own file (no-op-safe on a replicated pair — deletes are CDC-captured and LWW-converge; say so in a comment). Test: purge runs when not primary.

T7 (Sonnet) — P5-1 + P5-2: central ingest fallback budget; SiteCalls query shape

• P5-1 (Med): AuditLog/Central/AuditLogIngestActor.cs:372-405 — per-entry fallback loop passes NO token/timeout (contrast OnIngestAsync's 5s IngestFallbackBudget :268); each entry sits inside SqlServerRetryingExecutionStrategy 5×30s → minutes inside ReceiveAsync, singleton mailbox frozen, site re-sends amplify. Fix: mirror OnIngestAsync — short linked CTS + IngestSqlCommandTimeout per entry, pass the token to InsertIfNotExistsAsync/UpsertAsync. Test: fallback loop honors budget.
• P5-2 (Med): ConfigurationDatabase/Repositories/SiteCallAuditRepository.cs:301-331 orders (CreatedAtUtc DESC, TrackedOperationId DESC) with no index leading on CreatedAtUtc (full scan+sort per unfiltered page, row-35 class) and binds nvarchar(4000) params against IsUnicode(false) columns (may block seeks, incl. the keyset predicate :316; write path types VarChar deliberately :230). Fix: add the (CreatedAtUtc DESC, TrackedOperationId DESC) index via EF migration (BUILD FIRST — --no-build scaffolds empty) + typed varchar params on the read path. Update the handwritten production SQL script if one exists for this table (migration-parity rule).

T8 (Sonnet) — P6-3: ParseEnumListStrict comma smuggling (register row 99 defective)

File: src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs:117-119.

Enum.TryParse ORs comma-joined names for any enum; LooksNumeric checks first char only; dense 0-based AuditStatus makes composites land on defined members — "Forwarded,Attempted" = 1|2 = 3 = Delivered (silent wrong data); comma also smuggles ordinals past the row-88/99 guard ("Failed,7" → Skipped). Server path confirmed: AuditEndpoints.cs:656-658 passes the raw query value whole. Fix: reject any value containing ',' before TryParse (per-token error, same UX as existing invalid-name error). Tests: comma-joined names rejected; "Failed,7" rejected; single names/known-good paths unchanged. Do NOT edit the register (integration does).

Wave 2

T9 (Opus) — P6-4 + P6-5: ManagementActor scoping gaps (single-writer lane)

File: src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs (sole owner this wave).

• P6-4 (Med): HandleListDataConnections (:1875-1881) takes no user, null role gate → lists every site's connections (scrubbed but topology-revealing existence oracle) while HandleGetDataConnection fail-closes (:1888-1900) — Get denies what List hands over. Fix: filter by PermittedSiteIds exactly as HandleListSites (:1719-1729) / HandleListSecuredWrites (:1665-1695) do. Check the Design UI page's separate SiteRepository.GetAllDataConnectionsAsync bypass (DataConnections.razor:309) — if it serves scoped users unfiltered, route it through scope too (same one-line filter).
• P6-5 (Med): resolve-before-scope oracles outside row 98's five shapes: approve (:1465-1474) and reject (:1627-1635) check not-found + status (leaking "is Executed, not Pending") BEFORE EnforceSiteScopeForIdentifier; same shape HandleDebugSnapshot (:3604-3608) + HandleSubmitSecuredWrite (:1417-1423). Fix: hoist scope checks above resolution/status checks at all four sites (row-98 DenyUnresolvedTargetIfScoped semantics). Tests: out-of-scope caller gets the scope denial, not the status oracle, at each site.

T10 (Opus) — P6-6: Debug-stream session abandoned mid-start leaks permanently

Files: src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/DebugView.razor(.cs) (:493), DebugStreamHub.cs (:195).

Connect() awaits StartStreamAsync (≤30s) then assigns _session with no _disposed re-check; if Dispose already ran, StopStream never fires and the singleton DebugStreamService keepalive renews the registered session forever (:157/:213-219; the 5-min orphan net only reaps UNregistered sessions) — holds the site gRPC debug sub + bridge actor + dead circuit graph until restart. Identical shape in DebugStreamHub.cs:195. Fix: post-await _disposed re-check that stops the just-created session (both sites); make Dispose vs late-assign race-safe (interlocked/lock — Blazor Sync context helps but Dispose can run on a different flow). Tests: dispose-during-start stops the session and leaves nothing registered. The lesser TTL-bounded instances (TransportImport slot, Topology timer) are OPTIONAL — take them only if trivial with the same pattern.

T11 (Sonnet) — P6-7 + P6-8: audit export injection + CLI deadline

• P6-7 (Med): ManagementService/AuditEndpoints.cs:810-840 CSV quotes only on , " \r \n; leading = + - @ \t pass through → formula/DDE execution on operator open (CWE-1236); exported Target/ErrorMessage/Request/ResponseSummary/Extra carry inbound- API-caller text verbatim. Fix: prefix-escape cells starting with those chars (' prefix, the standard neutralization). Secondary: CSV omits ExecutionId/ ParentExecutionId/SourceNode vs JSONL and the AuditExportRow.cs:11-13 "24-field" claim — add the columns and align the doc comment.
• P6-8 (Med): CLI/Commands/AuditExportHelpers.cs:129 — SendGetStreamAsync(..., CancellationToken.None) after HttpClient.Timeout = Infinite (residue of the e0e4b246 timeout-honesty fix): a hung server hangs the CLI forever, and body reads (:141/:152)/CopyToAsync (:171) are tokenless (mid-stream fault → raw stack
  • truncated file). Fix: headers-received deadline + overall/inactivity deadline + Ctrl-C token wiring through all reads; friendly error on mid-stream fault. Tests: deadline fires; cancellation propagates.

T12 (Sonnet) — P6-9: LoginThrottle never throttles single-IP password spray

File: src/ZB.MOM.WW.ScadaBridge.CentralUI/Security/LoginThrottle.cs:189-190.

Key is {username}|{ip} → independent 5-failure budget per username per IP; a spray (many usernames, one IP) is never throttled though the class doc claims spray protection. Fix: add a secondary per-IP counter with a higher ceiling (e.g. 20-30 failures/window → per-IP lockout), layered on the existing per-pair key; keep the doc honest. Tests: spray locks the IP; normal per-user behavior unchanged; distinct IPs unaffected.

T13 (Sonnet) — P7-1: ExternalSystem TimeoutSeconds silently dropped by bundles

Files: src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs:248, EntitySerializer.cs:127-141, BundleImporter.cs:3087-3128, ArtifactDiff.cs:166-179, RoundTripEquivalenceTests.cs:445-452.

Row-27 SHAPE recurrence: DTO lacks TimeoutSeconds (operative at runtime and deployed as proto field 6); export never writes it, Add resets to 0, Overwrite keeps old, diff never compares → a tuned-timeout-only bundle previews Identical and the target env silently runs the gateway default. The round-trip guard is vacuous because the fixture never sets the field. Fix: add the DTO field + both serializer directions + both import branches + AddIfDifferent in the diff + a NON-DEFAULT fixture value so the reflection round-trip guard actually covers it. Schema stays additive (schemaVersion unchanged — additive field).

T14 (Opus) — P7-2 + P7-3 + P7-4: bulk-deploy semantics, reconcile deadline, probe TOCTOU

Files: src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs, StaleInstanceProbe.cs, FlattenSession.cs.

• P7-2 (Med, decided above): DeploySiteAsync deploys ALL instances (:804) and every success runs forceEnabledState:true (:610-612) — bulk push re-enables deliberately- Disabled equipment while the reconcile path deliberately preserves Disabled (:1638-1649, pinned). Fix: skip Disabled instances in bulk deploy; surface them in the result as a skipped list with reason; pin with tests (zero Disabled cases exist in DeploySiteAsyncTests today). Update Component-DeploymentManager.md (one sentence).
• P7-3 (Med): phase-1 TryReconcileWithSiteAsync per instance is bounded only by the 30s QueryTimeout → unreachable site × N instances ≈ hours serial before a single send; CLI's 5-min BulkDeployTimeout cancels and re-arms the identical cost. Fix: in bulk mode, short linked per-reconcile deadline AND a once-per-batch site reachability probe — first reconcile timeout marks the site unreachable for the rest of Prepare (instances proceed on DB state, which is what happens on timeout today anyway).
• P7-4 (Med): StaleInstanceProbe.cs:106-109 re-reads per-template versions AFTER flatten (comment claims before) → a non-structural template edit during flatten pairs pre-edit hash with post-edit version; memo permanently serves not-stale. FlattenSession captures correctly (FlattenSession.cs:114) but doesn't expose it. Fix: expose the session's captured MemberVersions; probe stores those instead of re-reading. Test: template edit mid-flatten still reports stale.

T15 (Sonnet) — P8-1: three SiteHealthReport fields never cross the wire

Files: src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/CentralControlDtoMapper.cs:452, the vendored centralcontrol proto (regen via bash docker/regen-proto.sh centralcontrol), SiteRuntime SiteHealthCollector.cs:243/:317-327, HealthReportSender.cs:180.

DetachedScriptThreads, ScriptRunShedCount, SiteStreamAlarmDropCount are populated site-side but ToDto omits all three (proto ends at field 29) — a watchdog-detaching/ shedding/alarm-dropping site looks healthy centrally. Aggravator: AddIntervalCounters takes only the 5 original counters and HealthReportSender never passes the shed count → a failed send permanently loses the interval's shed count (drained by Interlocked.Exchange). Fix: proto fields 30-32 (additive, never reuse numbers), mapper both directions, extend AddIntervalCounters so failed sends restore all drained counters. Follow the vendored-proto workflow exactly (regen script restores the csproj; an active <Protobuf> item must never be committed). Tests: mapper round-trips the three fields; failed send restores shed count.

T16 (Sonnet) — P8-2 + P8-3 + P8-4: env2 topology drift + regen-proto trap

• P8-2 (Med): docker-env2/docker-compose.yml — add init: true to all 4 services (docker/ has it ×8 with the issue-#34 rationale; env2's PID-1 boot failure wedges at 100% CPU instead of restart-looping).
• P8-3 (Med): env2 centrals still carry the SEC-36-rotated LDAP bind cred in plaintext (central-node-a/appsettings.Central.json:28,34 + node-b), plus inline SQL passwords + JWT key. Fix: port the docker/ posture — compose env-var overrides for secrets, and the same LDAP fallback/DisableLogin arrangement docker/ uses. Do NOT invent new secret values; reference the same env vars docker/ consumes.
• P8-4 (Med): docker/regen-proto.sh:58 — trap ... ERR doesn't fire on Ctrl-C during the long build → active <Protobuf> left in the csproj (breaks Docker build). Fix: idempotent restore trap on EXIT (covers ERR/INT/TERM/success; guard so a successful run doesn't double-restore).

T17 (Sonnet) — P8-5 + P8-6: test-health (register rows 95/96/40/102 classes)

• P8-5 (Med): Communication.Tests: (a) add xunit.runner.json maxParallelThreads: 4 (row-96 policy; ~14 TestKit classes + real Kestrel, last uncapped resource-heavy suite); (b) SiteAlarmLiveCacheTelemetryTests.cs:55-93 asserts fixed baselines on the process-global aggregators.active gauge with no [Collection] while two sibling suites move the instruments (row-102 shape) — apply the row-102 treatment (serialize via collection or delta-based assertions); (c) rework the 8 sleep-then-assert-absence sites in SiteAlarmAggregatorActorTests (:385-386, :624-625 et al) per row-40 (await the observable effect / use TestKit expectations, never wall-clock sleeps), and fix the discarded 100ms-tick actor in PendingDeploymentPurgeActorTests.cs:90 (row-95 class: Watch + Stop + ExpectTerminated).
• P8-6 (Med): SiteCallAudit.Tests DOES leak 100ms-tick actors (row 95's premise correction was wrong): SiteCallAuditPurgeTests actors at :127/:152/:174 + discarded Sys.ActorOf :209-212, SiteCallAuditReconciliationTests :58/:248/:372/:592 — zero Watch/Stop/ExpectTerminated in either file. Fix: row-82 treatment on every leaked actor. Do NOT edit the register (integration reopens/closes row 95 with a citation).
• Test-only task: verify by running both suites (Communication.Tests 3× for flake confidence).

---

Shared constraints (all tasks)

• Fix + tests only; no drive-by refactors. Match surrounding style/comment density.
• Targeted verification per task: build touched project(s) + test project(s); run affected suites filtered. Full solution sweep once, at integration.
• Each agent works ONLY inside its own worktree (absolute paths, git -C), commits with a pathspec limited to its own files, reports commit SHA + test results. Agents do NOT touch docs/plans/2026-07-08-deferred-work-register.md or CLAUDE.md (integration does).
• Component-doc updates only where the doc states the now-contradicted behavior; one sentence each.
• Wire/proto changes additive only; never reuse field numbers.

Integration (per wave, then final)

1. Cherry-pick wave commits onto main (verify file-disjointness first; T4/T15/T17 share the Communication project — files must stay disjoint).
2. dotnet build ZB.MOM.WW.ScadaBridge.slnx; run the touched suites; full dotnet test sweep after wave 2.
3. Register edits: reopen+close rows 95 and 99 citing P8-6/P6-3; add rows for anything an agent flags as deliberately deferred.
4. git diff review; commit plan doc + register/docs touch-ups; remove worktrees.
5. No push, no rig redeploy without the user.