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

# High-Findings Remediation Plan — 2026-08-19 Arch Review

Scope: the four **High** findings from the 2026-08-19 updated architectural review
(report artifact `43c7fa20-9626-4d59-a9c9-061524794660`; all four adversarially verified
CONFIRMED). Baseline `main @ e9e44a9e`. Medium/Low findings and register-row reopens
(95, 99) are explicitly out of scope for this plan.

Execution model: four file-disjoint tasks, one dedicated git worktree + subagent each,
run in parallel; integration = cherry-pick onto main, combined build, affected test
suites, `git diff` review, single merge session.

---

## T1 — P2-1 (High): Subscription completions carry no adapter generation

**File:** `src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs`
(`SubscribeCompleted` ~:2806, `AlarmSubscribeCompleted` ~:2814 — actor-internal records,
no wire-evolution constraint).

**Defect:** completions are applied unconditionally. An in-flight subscribe on
generation N (window up to `SeedOverallTimeout` 120 s) racing a `SwapAdapter`
(row-97 in-place reconfigure, failover) to generation N+1 causes:
(a) stale completion stores a dead gen-N handle (~:1368), then the live chunk's
duplicate guard (`_subscriptionIds.ContainsKey`, ~:2109) **releases the live monitored
item** (~:2145) — tag reads resolved-forever, delivers nothing;
(b) ~:1431 re-stamps old-endpoint seed values with current `_adapterGeneration`,
bypassing the stale-data guard;
(c) stale connection-level-failure rows knock a healthy connection back to Reconnecting (~:434);
(d) alarm twin: a real completion releases the NEW live alarm feed as a duplicate (~:2548–2557).

**Fix:**
- Add `AdapterGeneration` (stamped at subscribe launch) to both completion records;
  handlers drop mismatched completions entirely (no handle store, no seed apply, no
  failure-row processing, no duplicate-release).
- On drop, release any successfully-created handles **against the captured adapter
  reference**, not the current one (also audit release sites ~:1295/:1404 — MxGateway
  int handles collide across sessions, so a handle must never be released against a
  different adapter instance than created it).
- Do NOT widen scope to `ConnectResult` (P2-3, Med — separate follow-up).

**Tests (DataConnectionLayer.Tests):** stale `SubscribeCompleted` after a generation
bump (1) does not overwrite/store handles, (2) does not release the live item,
(3) does not seed stale values; alarm variant for `AlarmSubscribeCompleted`; released
handles go to the captured adapter.

## T2 — P2-2 (High): MxGateway native alarm feed permanently dead after plain reconnect

**Files:** `src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/MxGatewayDataConnection.cs`
(`ConnectAsync` :93–157; alarm state `_alarmCts`/`_alarmSubCount`/`_alarmSubSources`/
`_alarmStreamPrefix`), `RealMxGatewayClient.cs` (:415–467 `RunAlarmStreamAsync` retry loop).

**Defect:** `ConnectAsync` resets the event loop/client/`_disconnectFired` but not the
alarm-stream state (reset only in Disconnect/Dispose, which a reconnect never calls).
After a gateway restart, `SubscribeAlarmsAsync` sees `_alarmCts != null && Covers` and
returns success **without opening a stream on the new client**, while the orphaned
`RunAlarmStreamAsync` retries the disposed old client every 5 s forever. The native-alarm
mirror silently stays Uncertain until failover/reconfigure/redeploy.

**Fix:** in `ConnectAsync`, cancel + dispose `_alarmCts` and reset the alarm-subscription
bookkeeping so the actor's post-reconnect re-subscribe opens a fresh stream on the new
client; ensure the old stream task observes cancellation and exits (no zombie retry
loop). Verify end-to-end that the actor's transparent re-subscribe path actually calls
`SubscribeAlarmsAsync` again after reconnect and the feed resumes.

**Tests:** reconnect-then-alarm-resubscribe with a fake client: new stream opened on the
new client, old loop terminated, alarm events flow again.

## T3 — P6-1 (High): Secured-write reject is not CAS-guarded

**Files:** `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs`
(reject ~:1629–1655), `SecuredWriteRepository` (approve CAS `TryMarkApprovedAsync`
:90–114 is the pattern; reject uses a blind full-row `UpdateAsync` :44–52), plus the
repository interface.

**Defect:** handlers run concurrently (fire-and-forget PipeTo per envelope, :145–148),
`PendingSecuredWrite` has no concurrency token. Reject reads Pending → approve's CAS
wins and the multi-second `WriteTagAsync` relay runs → reject's blind update lands
mid-relay, stamping Rejected while the plant write executes; approve's final blind
update then overwrites back to Executed. The two-person-control record can be falsified
in either direction.

**Fix:**
- Add `TryMarkRejectedAsync` (`UPDATE … WHERE Status = 'Pending'`) mirroring the approve
  CAS; reject handler uses it and, on CAS failure, re-reads and returns a precise
  conflict error (same UX shape as approve's existing lost-race path).
- Confirm approve's post-relay status write can no longer collide (once reject can only
  win from Pending, the relay leg owns the row) — if any other blind `UpdateAsync` on
  this row remains reachable concurrently, CAS it too.
- Emit the `SecuredWriteReject` audit row only on CAS success.

**Tests (ManagementService.Tests):** reject-after-approve returns conflict and writes
nothing; approve-after-reject returns conflict; repository-level CAS test (two
transitions from one Pending row — exactly one wins).

## T4 — P6-2 (High): AlarmSummary page bypasses site scoping

**File:** `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/AlarmSummary.razor`
(~:278). Pattern donors: `DebugView.razor` (:363/:427), `EventLogs.razor`, `ParkedMessages.razor`.

**Defect:** the page is `RequireDeployment`-gated (Deployer — exactly the role
`SiteScopeService` scopes) but never injects `SiteScopeService`: `_sites =
GetAllSitesAsync()` unfiltered, and no scope check guards site selection or
`AlarmSummaryService.GetSiteAlarmsAsync`. A scoped Deployer can watch any site's live
alarms including `CurrentValue`/`LimitValue` process data. Server-side gap (Blazor Server).

**Fix:** mirror the sibling pattern exactly — inject `SiteScopeService`, filter the site
list, and enforce scope on the selected site before every alarm fetch (a crafted
interaction naming an out-of-scope site must be denied server-side, not just hidden).

**Tests (CentralUI.Tests):** mirror whatever pins the siblings' scoping (site list
filtered; out-of-scope fetch denied). If no sibling UI-scoping test exists, add a
service/page-logic-level test rather than a new UI harness.

---

## Shared constraints (all tasks)

- Fix + tests only; no drive-by refactors. Match surrounding style/comment density.
- Targeted verification per task: build the touched project(s) + test project(s), run
  the affected suites filtered — not the full solution sweep (that runs once at
  integration).
- Each agent works ONLY inside its own worktree (absolute paths, `git -C`), commits with
  a pathspec limited to its own files, and reports the commit SHA + test results.
- Update the relevant `docs/requirements/Component-*.md` only where it documents the now-
  contradicted behavior; one sentence, no restructuring.

## Integration

1. Cherry-pick the four commits onto `main` (file-disjoint; T1/T2 share a project but
   not files — verify no test-fixture collisions).
2. `dotnet build ZB.MOM.WW.ScadaBridge.slnx`, then run DataConnectionLayer, Management-
   Service, and CentralUI test suites (plus any suites the agents touched).
3. `git diff` review, commit plan doc + register/CLAUDE.md touch-ups if any, remove
   worktrees. No push without the user.