docs(plans): remediation plan for the four 2026-08-19 arch-review High findings (P2-1, P2-2, P6-1, P6-2) — all four executed in this session

2026-08-19 14:01:23 -04:00
parent 042e2efd1d
commit d18f344e66
1 changed files with 134 additions and 0 deletions
@@ -0,0 +1,134 @@
 # 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.