From d18f344e66c4e13eaedae82d986ee2d9b95ee6b1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 14:01:23 -0400 Subject: [PATCH] =?UTF-8?q?docs(plans):=20remediation=20plan=20for=20the?= =?UTF-8?q?=20four=202026-08-19=20arch-review=20High=20findings=20(P2-1,?= =?UTF-8?q?=20P2-2,=20P6-1,=20P6-2)=20=E2=80=94=20all=20four=20executed=20?= =?UTF-8?q?in=20this=20session?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...26-08-19-high-findings-remediation-plan.md | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 docs/plans/2026-08-19-high-findings-remediation-plan.md diff --git a/docs/plans/2026-08-19-high-findings-remediation-plan.md b/docs/plans/2026-08-19-high-findings-remediation-plan.md new file mode 100644 index 00000000..1a9449d8 --- /dev/null +++ b/docs/plans/2026-08-19-high-findings-remediation-plan.md @@ -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.