From 0b201e410cb9ffd7d647c62efe798aaa8de3eac8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 14 Aug 2026 19:23:56 -0400 Subject: [PATCH] =?UTF-8?q?docs(plans):=20arch-review=20remediation=20plan?= =?UTF-8?q?=20=E2=80=94=20multi-phase,=20subagent-executable=20program=20f?= =?UTF-8?q?or=20all=202026-08-14=20review=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...2026-08-14-arch-review-remediation-plan.md | 533 ++++++++++++++++++ 1 file changed, 533 insertions(+) create mode 100644 docs/plans/2026-08-14-arch-review-remediation-plan.md diff --git a/docs/plans/2026-08-14-arch-review-remediation-plan.md b/docs/plans/2026-08-14-arch-review-remediation-plan.md new file mode 100644 index 00000000..ffc42584 --- /dev/null +++ b/docs/plans/2026-08-14-arch-review-remediation-plan.md @@ -0,0 +1,533 @@ +# Architectural Review Remediation Plan + +**Date:** 2026-08-14 · **Status:** PROPOSED · **Source:** 2026-08-14 performance/architecture review +(eight-pass, all 27 projects + `ZB.MOM.WW.LocalDb`; report artifact +`https://claude.ai/code/artifact/8986cd89-6ab6-48aa-973f-119fddd5cafb`). + +This plan resolves **every** finding from the review — the ten ranked findings plus all +section-level Med/Low items — as an automated, multi-phase program executed by subagents, +parallel where file-ownership makes it safe. It is written so that each work package (WP) is a +self-contained subagent assignment: an executor agent should be able to complete a WP from its +spec here plus the code, without further orchestrator input. + +--- + +## 1. Execution model + +**Orchestrator:** the interactive Claude session (Fable) in this repo. It spawns one subagent per +work package, merges results, runs phase gates, and commits. + +**Isolation:** every code-writing package runs in its **own git worktree** +(`Agent` tool, `isolation: "worktree"`). Packages within a phase touch disjoint file sets (see +the conflict matrix, §7), so merges are clean; the orchestrator resolves any residual conflict +itself. Worktrees also make concurrent `dotnet build`/`test` safe (no shared `obj/`). + +**Per-package protocol.** Each executor agent must: +1. Read this plan's WP section and the referenced source files first; verify the finding still + reproduces in code (line numbers may have drifted — the *description* is authoritative). +2. Implement per the spec. Repo rules apply: edit in place, no backup copies; design doc + code + + tests + docker config travel together. +3. Build the touched projects and run the listed test suites in the worktree + (`dotnet build ZB.MOM.WW.ScadaBridge.slnx`, `dotnet test` filtered to relevant projects). +4. Commit on the worktree branch with the message given in the WP spec. +5. Report back: commit SHA, test summary (counts, any failures), and any spec deviations with + rationale. + +**Phase gates (orchestrator, serial).** After merging a phase's branches into the remediation +branch: full `dotnet build ZB.MOM.WW.ScadaBridge.slnx` + full `dotnet test` must be green; for +phases touching cluster runtime (2 and 3), rebuild the rig (`bash docker/deploy.sh`) and run the +phase's live probes (§6). A red gate stops the program — the orchestrator fixes or reverts the +offending package before proceeding. + +**Branching.** All work lands on `arch-review-remediation` (branched from `main` in Phase 0). +One commit per package (the worktree commit, merged `--no-ff`), plus one docs commit in Phase 4. +Cross-repo work in `~/Desktop/scadaproj` gets its own branch there (WP3.3). Nothing is pushed +without the user. + +### Model matrix + +| Model | Used for | Rationale | +|---|---|---| +| **fable** | Orchestration; design docs for structural seams (WP2.1a, WP3.1a, WP3.2a); final review synthesis | Hardest open design decisions and cross-package judgment | +| **opus** | Concurrency-sensitive implementation (streams, DCL, actors, ingest, UI coalescing, LocalDb library) | Subtle correctness work; failure modes are silent | +| **sonnet** | Mechanical/localized fixes, index migrations, schema tweaks, docs updates | Well-specified, verifiable by tests | +| **opus** (code-reviewer agents) | Phase 4 adversarial diff review | Independent verification pass | + +Approximate agent budget: Phase 1 = 7 executors; Phase 2 = 6 executors + 1 design; Phase 3 = +2 design + 3 executors; Phase 4 = 6 reviewers + 1 docs agent. ~26 agents total. + +--- + +## 2. Phase 0 — Preflight (orchestrator, serial, ~minutes) + +1. `git checkout -b arch-review-remediation` (current `main`; note the pre-existing dirty + `docs/requirements/Component-SiteRuntime.md` — stash or leave untouched, do not absorb it). +2. Baseline: full build + full test run; record pass/fail counts as the reference state. +3. Confirm the rig is deployable (`docker/deploy.sh` last known good) but do **not** redeploy yet. + +Gate: baseline green. If baseline has pre-existing failures, record them — packages are judged +against the baseline, not absolute green. + +--- + +## 3. Phase 1 — Quick wins (7 packages, ALL PARALLEL) + +Highest leverage per line changed. All packages are independent; the two silent-failure defects +(WP1.1, WP1.2) lead. + +### WP1.1 — Stream OK-completion reconnect `[opus]` +**Findings:** #1 (High) — streams silently die at the 4-hour max lifetime; OK completion is +invisible to reconnect logic. +**Files:** `Communication/Grpc/SiteStreamGrpcClient.cs`, `Communication/Actors/SiteAlarmAggregatorActor.cs`, +`Communication/Actors/DebugStreamBridgeActor.cs` (+ tests). +**Changes:** +- Add an `onCompleted` callback to the client subscribe surface (preferred over an error + sentinel — callers can distinguish graceful end from fault). Invoke it when the + `await foreach` ends without exception, including server OK completion at max lifetime and + graceful site shutdown. +- `SiteAlarmAggregatorActor`: on completion, `Tell` self a new `GrpcAlarmStreamCompleted(generation)` + message → set `_streamDown = true`, mark `IsLive = false`, and let the reconcile tick reopen + **without** consuming error-retry/backoff budget (completion is not a fault). Also observe the + `Task.Run` subscription task itself (`ContinueWith` faulted → route into the existing error + path) so no completion or fault is ever unobserved again. +- Same treatment for the debug stream path in `DebugStreamBridgeActor`. +- Generation-fence everything (the fencing pattern already exists in the actor — reuse it). +**Tests:** unit test on the client (fake stream that completes OK → `onCompleted` fires, `onError` +does not); actor test (stream completes OK → `IsLive` false → reopen on next reconcile tick → +backoff counter unchanged). Regression: error path still burns retry budget. +**Commit:** `fix(comms): reconnect on graceful stream completion — kills the 4h silent stream death` + +### WP1.2 — Site audit DB placement + flush `[sonnet]` +**Findings:** #2 (High) — `auditlog.db` on container overlayfs; dead `FlushIntervalMs`; fsync per event. +**Files:** `AuditLog/Site/SqliteAuditWriterOptions.cs`, `AuditLog/Site/SqliteAuditWriter.cs`, +all site-node `appsettings` under `docker/` and `docker-env2/` (+ tests). +**Changes:** +- Make `DatabasePath` **required on Site nodes** (drop the CWD-relative default; `ValidateOnStart`), + mirroring the `LocalDb:Path` precedent — a healthy-looking node silently writing to overlayfs is + the failure mode being closed. Update all rig site-node configs to `/app/data/auditlog.db`. + Update any tests that relied on the implicit default to pass an explicit temp path. +- Implement `FlushIntervalMs` as designed: the writer loop groups commits within the interval + (batch already drains up to 256 — add the time-based commit coalescing). Additionally set + `PRAGMA synchronous=NORMAL` on the audit connection — audit is best-effort by design + (CLAUDE.md: "audit-write failure NEVER aborts the user-facing action"), so NORMAL's + power-loss window is acceptable; say so in a code comment. +- One-time migration note in the commit message: the existing container-local file is abandoned; + already-forwarded rows are safe centrally, pending rows on the old path are lost once — accepted, + this is the bug being fixed. (Cross-reference `docs/known-issues/2026-07-20-cached-telemetry-drain-hot-loop.md`, + which this placement bug caused.) +**Tests:** options validation test (site role boots ⇒ path required); writer test asserting +multi-event single-transaction commit under trickle load. +**Commit:** `fix(auditlog): site audit DB onto the data volume; required path + soft flush` + +### WP1.3 — Conditional CDC registration `[opus]` +**Findings:** #5 (High), partial — non-replicating nodes pay full CDC capture forever. (The +library half — trigger cleanup API, O(1) backlog — is WP3.3.) +**Files:** `Host/SiteLocalDbSetup.cs` (+ tests). Read `~/Desktop/scadaproj/ZB.MOM.WW.LocalDb/` +and the site-a rig configs first to confirm the exact replication-config shape on both the active +and passive peer. +**Changes:** +- Skip all ten `RegisterReplicated` calls when the node has **no replication configured** + (neither `LocalDb:Replication:PeerAddress` nor `ApiKey` — verify the passive-side shape on the + rig before choosing the predicate; the guard must be true on BOTH members of a replicated pair + and false on site-b/site-c). +- Preserve the load-bearing ordering documented in the file: when registration is skipped, the + legacy migrator still runs (its rows simply don't replicate — consistent with an unreplicated node). +- **Known residual, documented in-file:** a database file that was registered under an older build + keeps its stale triggers until WP3.3 ships the library cleanup API; on the docker rig this is + moot (volumes are recreated on redeploy of a schema change) — state this in a comment and in the + commit message. +**Tests:** setup test with and without replication config, asserting trigger presence/absence via +`sqlite_master` (`name LIKE '%localdb%'` — confirm the library's trigger naming first). +**Commit:** `perf(host): install CDC capture only when replication is configured` + +### WP1.4 — SQL quick indexes + sliced purge `[sonnet]` +**Findings:** S&F sweep O(N) scan (High); audit KPI window key-lookup; Notification KPI +delivered-interval scan; unbounded terminal purge (all part of #7/#8 support). +**Files:** `StoreAndForward/StoreAndForwardSchema.cs` (+ its versioned-upgrade path), +`ConfigurationDatabase/Migrations/` (+ manual SQL script for production per repo convention), +`ConfigurationDatabase/Repositories/NotificationOutboxRepository.cs`. +**Changes:** +- `sf_messages`: add `(status, created_at)` index so the ordered due-scan terminates at `LIMIT`; + follow the schema's existing versioned-upgrade mechanism so existing site files get it. +- `IX_AuditLog_OccurredAtUtc` → add `INCLUDE (Status)` (EF migration; remember the repo gotcha — + **build first**, never `--no-build`, delete empty migrations if scaffolded). +- Notifications: filtered index `(DeliveredAt) WHERE Status = 'Delivered'` (or the enum's stored + value — check the column type) covering the delivered-last-interval KPI count. +- `DeleteTerminalOlderThanAsync`: slice into batched deletes — copy the slicing pattern used twice + elsewhere in the same layer (per-channel audit retention delete is one). +**Tests:** migration applies cleanly (dev auto-apply path); a query-shape test is not required — +Phase 4 live probes verify plans via `EXPLAIN`/showplan. +**Commit:** `perf(sql): sweep/KPI covering indexes + sliced notification terminal purge` + +### WP1.5 — Mechanical hot-path fixes `[sonnet]` +**Findings:** InstanceActor nits (Med); per-call `JsonSerializerOptions` ×4 (Med). +**Files:** `SiteRuntime/Actors/InstanceActor.cs`, `ExternalSystemGateway/ExternalSystemClient.cs`, +`SiteRuntime/Repositories/SiteExternalSystemRepository.cs` (options instance only — the query +redesign is WP2.6), `ManagementService/ManagementEndpoints.cs`, `CentralUI/.../ScriptAnalysisService.cs`. +**Changes:** +- `HandleSetStaticAttribute`: use the existing `_resolvedAttributeByName` `TryGetValue` instead of + the LINQ scan (InstanceActor.cs:373). +- `HandleTagValueUpdate`: precompute the parsed `DataType` into the resolved-attribute index entry + at config-apply time; stop re-`Enum.TryParse`-ing per update. Intern/cache the quality strings + (`Quality.ToString()` per update → static readonly map). +- Static-override writes: coalesce per instance on a short timer, reusing the batched-flush pattern + already present for native alarms in the same codebase (copy that shape, don't invent). +- One `static readonly JsonSerializerOptions` per assembly at the four verified sites. +**Tests:** existing InstanceActor suite must stay green; add a test pinning single-flush +coalescing of N rapid static writes. +**Commit:** `perf(runtime): O(1) attribute resolution, precomputed types, coalesced static writes, shared JSON options` + +### WP1.6 — Transport fail-fast `[sonnet]` +**Findings:** known-failure sends burn full Ask timeouts (Med, first half — the timeout stagger +and double-hop are WP2.2). +**Files:** `Communication/Grpc/GrpcSiteTransport.cs` (+ tests). +**Changes:** a send to a site with no configured channel replies `Status.Failure` (with a +descriptive exception) to the asker immediately instead of warn-and-drop. Audit every +warn-and-drop in the file for the same pattern. +**Tests:** Ask against an unconfigured site faults in <1s, not at the 30s timeout. +**Commit:** `fix(comms): fail known-dead sends immediately instead of burning Ask timeouts` + +### WP1.7 — Event-log writer batching + sliced retention `[sonnet]` +**Findings:** per-event transactions in `SiteEventLogger` (part of the site_events High — the +volume *policy* is WP3.2); unbatched `site_events` retention DELETE (Med). +**Files:** `SiteEventLogging/SiteEventLogger.cs`, `SiteEventLogging/EventLogPurgeService.cs` (+ tests). +**Changes:** +- Writer loop: drain the existing batching channel into **one transaction per drain** (up to 256), + exactly the `SqliteAuditWriter` shape — cite it in a comment. +- Retention purge: batch the DELETE at 1000 rows/iteration like the cap purge next to it. +**Tests:** writer test (N queued events → 1 transaction); purge test over >1000 expired rows. +**Commit:** `perf(sitelog): batched event-log commits and sliced retention purge` + +**Phase 1 gate:** merge all seven, full build + test, then rebuild the rig (`docker/deploy.sh` — +WP1.2 changed site configs) and run the Phase 1 live probes (§6). + +--- + +## 4. Phase 2 — Seam rework (6 packages, PARALLEL after WP2.1a) + +### WP2.1 — DCL batch seam `[design: fable → implement: opus]` *(largest package)* +**Findings:** #3 (High, four collapsed findings) + MxGateway 2-RPC subscribe / no-backoff retry +(Med) + quality-counter and alarm-fanout costs (Med). +**Stage a — design memo** (fable, ~1 page, committed as +`docs/plans/2026-08-XX-dcl-batch-seam-design.md`): the batch API shape on `IDataConnection` +(`SubscribeBatchAsync(IReadOnlyList)` returning per-tag results; true bulk +`ReadBatchAsync`/`WriteBatchAsync`), per-subscription monitored-item budget + sharding scheme, +reconnect chunking/stagger parameters (configurable; defaults sized for 37,500 tags), re-seed +deadline semantics, and what the MxGateway adapter can do **without** a cross-repo mxaccessgw +change (fallback: client-side chunked pipelining; note the cross-repo bulk-RPC follow-up if the +proto lacks one — check `~/Desktop/MxAccessGateway` first). +**Stage b — implementation** (opus): +- `RealOpcUaClient`: N×`AddItem` + one `ApplyChangesAsync` per batch; serialize all + `ApplyChangesAsync` on the shared Subscription behind a `SemaphoreSlim` (batching makes + contention rare; the lock makes it safe); shard monitored items across subscriptions above the + per-subscription budget (default ~5,000, configurable). +- `OpcUaDataConnection`: implement the batch interface; `ReadBatchAsync`/`WriteBatchAsync` become + single OPC UA service calls (the MxGateway adapter's `ReadBulk`/`WriteBulk` is the in-repo + reference). +- `DataConnectionActor`: `HandleSubscribe`/`ReSubscribeAll` pass whole tag lists; reconnect + becomes bounded chunked batches honoring the existing failover stagger; `SeedTagsAsync` becomes + chunked batch reads with bounded parallelism and an **overall** deadline (no more 30s-per-tag + serial worst case). Tag-resolution retry gains exponential backoff with a ceiling and batched + probes. Quality counters flush on transitions or a short timer instead of per-change; alarm + subscriber matching gets a source-prefix index and pushes the union filter to the gateway feed. +**Tests:** the existing DCL suite (Become/Stash lifecycle, generation fencing) must stay green; +new tests with a fake client: batch subscribe issues one apply per chunk; concurrent applies +serialize; re-seed respects the deadline; backoff caps. +**Commit:** `perf(dcl): batch subscribe/read/write seam, bounded reconnect, sharded subscriptions` + +### WP2.2 — Central ingest & SQL set-based rework `[opus]` +**Findings:** #6 (High, partition purge), #7 (High, row-at-a-time ingest), #8 (High, KPI scans — +query-shape half), KPI rollup N+1 / read-path hygiene / no pooling (Med). +**Files:** `AuditLog/.../AuditLogIngestActor.cs`, `ConfigurationDatabase/Repositories/AuditLogRepository.cs`, +`SiteCallAudit/.../SiteCallAuditActor.cs` + `SiteCallAuditRepository.cs`, +`ConfigurationDatabase/Repositories/NotificationOutboxRepository.cs`, `KpiHistoryRepository.cs`, +`ConfigurationDatabase/ServiceCollectionExtensions.cs`, gRPC ingest entry in Communication (+ migration + manual SQL script). +**Changes:** +- **Ingest batching:** one TVP (or multi-row `INSERT … SELECT … WHERE NOT EXISTS` batch) per + telemetry packet with per-row fallback on constraint violation; cached-telemetry entry = + one transaction, not ~5 round trips. +- **SiteCalls upsert:** single statement (`UPDATE … ; IF @@ROWCOUNT = 0 INSERT …` guarded for the + newer-status predicate, or `MERGE` with the usual HOLDLOCK hygiene). +- **Reconciliation off-mailbox:** the drain moves out of `ReceiveAsync` to the + `PipeTo` + in-flight-guard shape — `NotificationOutboxActor` next door is the in-repo reference; + ingest/queries/KPI asks must not queue behind post-outage catch-up. +- **Timeout stagger + hop removal:** inner budgets strictly less than outer (site-ask 25s → + ingest 20s → SQL 15s, or similar); the gRPC handler Asks the ingest singleton proxy directly, + removing the redundant intermediate actor hop. +- **Partition purge:** prefer making EventId uniqueness **partition-aligned** + (`UNIQUE (EventId, OccurredAtUtc)`) so `SWITCH` needs no index drop at all; EventId is globally + unique by construction (GUID minted at source), so the idempotency probe still seeks correctly — + document this reasoning in the migration. If alignment proves incompatible with the probe shape, + fall back to `ONLINE = ON` rebuild outside the switch transaction (Standard-edition caveat: check + target SQL edition; if ONLINE is unavailable, alignment is the only acceptable option). +- **KPI query shapes:** split the live-queue seeks from the delivered-interval count (filtered + index from WP1.4); rollup fold preloads existing rows per window into a dictionary (kill the + per-(series,hour) existence SELECT). +- **Hygiene:** `AsNoTracking` + keyset paging on the Notifications read path (mirror the two + sibling repos); targeted `ExecuteUpdate` for delivery-attempt status writes instead of detached + full-row `Update()`; `AddDbContextPool` — **first verify** the context registers no scoped + state/injected services that break pooling; if it does, fix or document why pooling is skipped. +**Tests:** ingest idempotency under batch (duplicate EventIds in one packet + across packets); +upsert newer-status semantics; reconciliation running while an ingest Ask completes promptly. +**Commit:** `perf(central): set-based ingest, aligned partition purge, KPI query shapes, EF hygiene` + +### WP2.3 — Wire efficiency `[opus]` +**Findings:** #10 (Med, alarm seed waste + per-attempt re-fan-out), unbounded pre-snapshot +buffers / lossy shared channel (Med), `PullAuditEvents` at-most-once (Low). +**Files:** `Communication/Actors/SiteAlarmAggregatorActor.cs`, `SiteAlarmLiveCacheService.cs`, +`SiteCommandDtoMapper.cs`, `DebugStreamBridgeActor.cs`, `Communication/Grpc/SiteStreamGrpcServer.cs`, +site audit queue (`ISiteAuditQueue` impl) (+ tests). +**Changes:** +- **Alarms-only snapshot:** the seed travels the `SiteCommandService` query surface + (`QueryReply.DebugViewSnapshot`), so add an additive alarms-only flag on the query DTO — verify + whether the vendored proto is involved at all; if it is, follow the documented manual + toggle-build-copy-untoggle regeneration, additive field numbers only. +- Seed **once per successful (re)connect** plus the existing reconcile as the diff/backstop — + not a full unconditional snapshot every 60s. Jitter the per-site aggregator timers so 10 sites + don't fan out in lockstep; give the seed-retry leg backoff. +- Cap the pre-snapshot buffers (drop-oldest + a dropped-count surfaced on the health report); + hard timer on snapshot arrival that fails the session rather than buffering forever; stream + events must not reset the receive-timeout that ends a stuck session. +- `SubscribeSite` gets its own channel (larger, or wait-mode) instead of sharing the debug view's + 1000-slot DropOldest — an alarm burst during a WAN stall must not silently drop transitions. + Batch multiple events per proto message where the contract allows (additive). +- `PullAuditEvents`: mirror `PullSiteCalls`' composite `(timestamp, id)` keyset cursor; flip rows + to `Reconciled` only when the **next** pull's cursor proves receipt (at-least-once restored). +**Tests:** cursor-proof flip semantics (fault between response and next pull → rows re-served); +seed-once behavior across a simulated reconnect; buffer cap + counter. +**Commit:** `perf(comms): alarms-only seed, capped buffers, at-least-once audit pull` + +### WP2.4 — Central UI performance `[opus]` +**Findings:** #8 (High, per-circuit KPI polling half), AlarmSummary/Health/DebugView per-circuit +costs (High), push-triggered full reloads / serial Asks / no virtualization (Med), sandbox +thread-parking (Low). +**Files:** `CentralUI` — `Health.razor`, `AlarmSummary.razor`, `DebugView.razor`, +`Deployments.razor`, a new process-level KPI snapshot service, sandbox host (+ tests where the +project has them). +**Changes:** +- **Process-level memoized KPI snapshot service** (singleton, 5–10s TTL, single-flight so N + circuits share one SQL round per interval) consumed by Health + Notification Outbox + Site + Calls pages. This plus WP2.2's query shapes closes finding #8 from both ends. +- AlarmSummary derives not-reporting from the alarm aggregator's own reconcile state instead of + re-running the per-instance snapshot fan-out per circuit per 15s. +- DebugView: coalesce renders at ~250ms; rebuild the trees behind a version stamp instead of per + event; marshal off the gRPC thread once per coalesce window. +- Deployments: apply the pushed delta (or debounce reloads ~500ms) instead of reloading all + records + instances per push; server-side paging. +- Health tick: `Task.WhenAll` with short per-tile timeouts so one hung singleton degrades one + tile, not the tick. `Virtualize` on the flat alarm table. +- Sandbox Test Runs: bound with a semaphore instead of parking thread-pool threads on sync accessors. +- Preserve the existing visual design exactly — these are behavioral changes only, no restyling, + no new component frameworks (repo rule). +**Tests:** KPI cache single-flight test; render-coalescing unit test if the code shape allows. +**Commit:** `perf(ui): shared KPI cache, live-cache-backed alarm summary, coalesced debug renders` + +### WP2.5 — Deployment & authoring pipeline `[opus]` +**Findings:** #9 (High, both halves), verdict-cache Clear() leak + unpaged management queries (Med). +**Files:** `TemplateEngine/.../FlatteningPipeline.cs`, `TemplateEngineRepository.cs`, +`TemplateService.cs`, `DeploymentManager/.../DeploymentService.cs`, +`ManagementService/ManagementActor.cs`, `ScriptAnalysis/.../ScriptCompileVerdictCache.cs`, +CLI (+ tests). +**Changes:** +- **Flatten-session cache:** within one deploy/validation session, memoize template chain and + composition loads keyed by template id + version; hoist the three global queries (shared + scripts, schemas, connections) once per session — `ArtifactDeploymentService` is the in-repo + reference shape. +- **`DeploySiteAsync`:** bulk orchestration over a site's instances with bounded parallel fan-out + and per-instance timeouts, reusing the per-instance operation lock; surfaced via ManagementActor + command + CLI (`instance deploy-site ` or consistent naming — follow the CLI's existing + verb conventions); UI wiring optional, defer if it inflates scope. +- **Staleness watermark:** per-template monotonic version bumped on any template-graph mutation; + staleness detection compares watermarks before paying a full flatten. +- **Authoring:** slim projections for acyclicity/collision checks (no script bodies); + `AsNoTracking` on read-only walks; `ReconcileDescendantsAsync` receives the already-loaded + graph instead of re-calling `GetAllTemplatesAsync`. +- **Management queries:** `QueryDeployments` pages DB-side with a summary projection; `ListTemplates` + pages DB-side without full script bodies; batch override apply becomes one read + one commit. +- **Verdict cache:** LRU/segmented eviction instead of wholesale `Clear()` (the Clear re-opens the + non-collectible `InteractiveAssemblyLoader` leak the cache exists to bound); add a retention + policy for terminal deployment records (config, default generous). +**Tests:** flatten count test (N instances, same template ⇒ chain loaded once); DeploySiteAsync +fan-out with an injected slow site (bounded, others complete); verdict-cache eviction keeps hot +entries. +**Commit:** `perf(deploy): flatten-session caching, bulk DeploySiteAsync, paged management queries` + +### WP2.6 — Cross-cutting misc `[sonnet]` +**Findings:** site external-system resolution (Med), Inbound API per-request SQL (Low), unbounded +S&F observer queue (Low), CLI timeout (Low), failback-probe heartbeat pollution (Low), site stream +alarm-vs-attribute shared buffer (Med). +**Files:** `SiteRuntime/Repositories/SiteExternalSystemRepository.cs`, InboundAPI method-lookup +path, `StoreAndForward` observer queue, `SiteRuntime/Streaming/SiteStreamManager.cs`, CLI +`HttpClient` setup, failback probe (+ tests). +**Changes:** +- External-system resolution: name-keyed lookup (the ID reverse-map scan goes); cache the parsed + method list per system, invalidated on redeploy — honoring the documented "indexed query" contract. +- Inbound API: short-TTL ApiMethod cache invalidated by the existing `ScriptArtifactChangeSubscriber` + bus (the invalidation plumbing already exists — lean on it). +- S&F observer queue: bound it DropOldest like its siblings (it is the one unbounded channel in + the system) + a dropped counter. +- `SiteStreamManager`: separate alarm publish source (or priority path) so attribute storms can't + evict alarm transitions; drop counter on the site health report; skip publish at zero subscribers. +- CLI `HttpClient`: explicit timeout (30s, config-overridable). +- Failback probe: mark the probe heartbeat synthetic (flag or reserved SiteId) so it stops + polluting liveness data — check what consumes heartbeats before choosing the mechanism. +**Commit:** `perf(misc): cached hot-path lookups, bounded observer queue, alarm-priority stream path` + +**Phase 2 gate:** merge order WP2.2 → WP2.3 → WP2.1 → WP2.4 → WP2.5 → WP2.6 (Communication-heavy +first — likeliest residual conflicts surface early). Full build + test; rig rebuild; Phase 2 live +probes (§6). + +--- + +## 5. Phase 3 — Structural (design-first; 3 tracks) + +Design stages run in parallel (fable); implementations start as each design lands. Defaults are +pre-decided below so automation never blocks — the design memos may refine but not stall. + +### WP3.1 — Script execution pool split `[design: fable → implement: opus]` +**Findings:** #4 (High) + actor-per-execution overhead / compile-cache Clear (Med). +**Stage a — design doc** (`docs/plans/2026-08-XX-script-pool-split-design.md`), deciding: +- Trigger-expression evaluation leaves the blocking pool — default decision: evals are + non-blocking by construction, so run them as plain async on the default dispatcher with a + concurrency gate, no second dedicated pool. +- Blocking-script pool sizing scales with deployed instance count (default: `max(8, instances/8)` + capped at a config ceiling); stuck-script watchdog **replaces** lost threads. +- Timeout budget includes queue wait (deadline captured at enqueue). +- Per-script in-flight cap — default policy: bounded queue per script (cap 4), overflow **sheds + newest with a site event + counter** (an alarm-triggered run that can't start within its + deadline is stale anyway); design memo must justify or amend. +- `ScriptExecutionActor` elimination: run the guarded task directly under `ScriptActor`, + preserving supervision semantics (Stop-on-failure equivalence), telemetry, and the audit + `ExecutionId`/`ParentExecutionId` threading — this is the riskiest part; the design memo maps + every behavior the actor currently provides to its replacement. +- `SiteScriptCompileCache`: LRU eviction; cache-miss compiles move off-dispatcher (the + DeploymentManager shared-script path is the in-repo reference). +**Stage b — implement + tests:** starvation regression test (8 blocked scripts ⇒ alarm eval still +completes <2s); queue-time-inclusive timeout test; watchdog replacement test; supervision-parity +tests for the actor removal. +**Commit:** `perf(runtime): split trigger evals from the blocking pool; bounded, deadline-aware execution` + +### WP3.2 — site_events volume policy `[design: fable → implement: sonnet]` +**Findings:** #High site_events (policy half; mechanics done in WP1.7). +**Stage a — design memo** deciding: per-run Started/Completed Info events become **sampled/opt-in** +(default: off per-run, aggregate counters per interval instead; per-script opt-in flag for +debugging); whether `site_events` should replicate at all (default recommendation: keep replicated +— central event-log queries hit either node — but the memo must check what actually reads +`site_events` on the peer before confirming; if nothing does, deregister it and use clock-based +local purges that never enter the oplog, coordinating with WP3.3's dereg API). +**Stage b:** implement per memo; update `Component-SiteEventLogging` doc (Phase 4 collects). +**Commit:** `perf(sitelog): sampled per-run events; site_events replication policy` + +### WP3.3 — LocalDb library (CROSS-REPO: `~/Desktop/scadaproj`) `[opus]` +**Findings:** #5 (library half), replication row-count batching (Med), library-level costs (Low). +**Files:** `~/Desktop/scadaproj/ZB.MOM.WW.LocalDb/` on its own branch; then the consuming update here. +**Changes (library):** +- `DeregisterReplicated(table)` / cleanup API: idempotently drops capture triggers (and optionally + prunes that table's oplog rows) — consumed by `SiteLocalDbSetup` when replication is + unconfigured, closing WP1.3's stale-trigger residual. +- Backlog depth becomes O(1) (`MAX(seq) − last_acked_seq` or a maintained counter) — it currently + runs an O(backlog) `COUNT(*)` per minute and per metrics scrape. +- Replication batching by **summed bytes** (budget under the 4 MB gRPC cap, row-count as a + secondary cap) + ack coalescing; the rig's `MaxBatchSize=16` pin can then be retired. +- `LwwApplier` prepare-once-rebind per table; drop the unused `__localdb_oplog_hlc` index (handle + existing files in the schema-version upgrade). +**Changes (this repo, after the library lands):** bump the package/project reference; call the +cleanup API from `SiteLocalDbSetup` when unregistered; relax the rig `MaxBatchSize` pin to the +byte budget; update the CLAUDE.md LocalDb bullet and the **umbrella `../scadaproj/CLAUDE.md` +ScadaBridge entry in the same change** (repo rule). +**Tests:** library suite for dereg idempotency, O(1) depth, byte-budget batching; ScadaBridge +integration: site-b boots with zero triggers and zero oplog growth. +**Commits:** library commit in scadaproj + `chore(deps): LocalDb — dereg API, byte-budget replication` here. + +**Phase 3 gate:** full build + test in BOTH repos; rig rebuild; live probes. + +--- + +## 6. Phase 4 — Verification, review, docs (mixed parallel) + +### Live probes (orchestrator + rig, accumulate per phase gate) +- **Stream lifetime:** set max stream lifetime to ~2 min on the rig; verify the alarm stream + reconnects within one reconcile tick of OK completion and `IsLive` reflects the gap (WP1.1). +- **CDC:** on site-b/site-c nodes, `sqlite_master` shows no capture triggers; oplog table empty + and not growing (WP1.3/WP3.3). Site-a pair still converges (replication regression). +- **Audit path:** site `auditlog.db` present under `/app/data`; survives `docker compose + rm`+recreate of one site container (WP1.2). +- **Query plans:** `EXPLAIN QUERY PLAN` on the S&F due-sweep (index-terminated); SQL Server + showplan on the notification KPI and audit KPI queries (seeks, no full scans) (WP1.4/WP2.2). +- **Deploy timing:** bulk-redeploy a site on the rig before/after; record wall clock (WP2.5). +- **Failover drill:** `docker/failover-drill.sh` still passes (~25s takeover) — protects against + regressions from the actor/timeout changes. +- **Load validation:** the deferred target-scale load test (deferred-work register #25) is the + final proof the ceilings moved — schedule as its own follow-on; this plan's exit criterion is + the probes above, not #25. + +### Adversarial review `[6 parallel code-reviewer agents, opus; fable synthesis]` +One reviewer per area over the full remediation diff (site runtime, DCL, comms, site persistence, +central SQL, UI/deploy), each instructed to try to **refute** the fixes (regressions, changed +semantics, missed call sites, test gaps). Confirmed findings are fixed by targeted follow-up +agents before the docs commit. + +### Documentation propagation `[sonnet]` +- Component docs touched: `Component-DataConnectionLayer`, `Component-SiteRuntime`, + `Component-CentralSiteCommunication`, `Component-AuditLog`, `Component-SiteEventLogging`, + `Component-StoreAndForward`, `Component-NotificationOutbox`, `Component-SiteCallAudit`, + `Component-TemplateEngine`, `Component-DeploymentManager`, `Component-CentralUI`, + `Component-ClusterInfrastructure` (only where behavior/config changed — no cosmetic edits). +- CLAUDE.md Key Design Decisions: stream-completion reconnect, conditional CDC, required site + audit DB path, DCL batch seam, `DeploySiteAsync`, script-pool split, LocalDb byte-budget + replication (retire the MaxBatchSize=16 note). +- Umbrella `../scadaproj/CLAUDE.md`: LocalDb entry (done with WP3.3). +- Close/annotate any related `docs/known-issues/` entries; README component table needs no change + (no components added/removed). +- Final `git diff` review by the orchestrator, then the docs commit. **Push only on user request.** + +--- + +## 7. Parallelism & conflict matrix + +| Phase | Package | Projects touched | Conflicts within phase | +|---|---|---|---| +| 1 | WP1.1 | Communication (client+actors) | WP1.6 same project, different files — merge-safe | +| 1 | WP1.2 | AuditLog(site), docker configs | none | +| 1 | WP1.3 | Host | none | +| 1 | WP1.4 | ConfigurationDatabase, StoreAndForward | none | +| 1 | WP1.5 | SiteRuntime, ESG, ManagementService, CentralUI | single-line edits, none | +| 1 | WP1.6 | Communication (transport) | see WP1.1 | +| 1 | WP1.7 | SiteEventLogging | none | +| 2 | WP2.1 | DataConnectionLayer | none | +| 2 | WP2.2 | AuditLog(central), SiteCallAudit, ConfigurationDatabase, NotificationOutbox | none (WP2.3 owns Communication) | +| 2 | WP2.3 | Communication, site audit queue | none | +| 2 | WP2.4 | CentralUI | none | +| 2 | WP2.5 | TemplateEngine, DeploymentManager, ManagementService, ScriptAnalysis, CLI | none | +| 2 | WP2.6 | SiteRuntime(streaming+repos), InboundAPI, S&F, CLI(http) | CLI touched by WP2.5 (commands) and WP2.6 (HttpClient) — different files; merge WP2.5 first | +| 3 | WP3.1 | SiteRuntime(scripts) | none | +| 3 | WP3.2 | SiteEventLogging, SiteRuntime(one call site) | coordinate with WP3.1 merge order (3.1 first) | +| 3 | WP3.3 | scadaproj (separate repo) + Host/config here | in-repo half lands after 3.1/3.2 | + +## 8. Risks & mitigations + +- **Vendored proto drift** (WP2.3): additive fields only, never reuse numbers; follow the + documented manual regeneration toggle. Mitigation: the executor verifies whether the change is + DTO-only before touching proto at all. +- **Partition-aligned uniqueness** (WP2.2) changes the enforcement scope of EventId uniqueness + from global to per-partition. GUID collision across partitions is not a real risk, but the + ingest probe's semantics must be re-tested for duplicate delivery across a month boundary. +- **Pooled DbContext** (WP2.2): pooling breaks contexts with per-scope injected state — verify + before switching; skipping with a documented reason is an acceptable outcome. +- **ScriptExecutionActor removal** (WP3.1) is the highest-semantic-risk change: supervision, + telemetry, and audit-correlation parity are explicit design-memo obligations, and it ships in + its own commit so it can be reverted independently. +- **Rolling-upgrade caveat** (WP3.3): site pairs already require stop/start together (repo rule); + the LocalDb schema-version bump rides that existing constraint — call it out in the topology guide. +- **EF migration gotcha:** always build before `dotnet ef migrations add`; delete empty scaffolds. +- **Merge conflicts:** worktree isolation + the matrix above; residual conflicts are resolved by + the orchestrator, never by re-running an executor blindly. + +## 9. Exit criteria + +1. Full solution build + tests green in both repos; baseline test count not reduced. +2. All §6 live probes pass on the rig; failover drill unchanged (~25s). +3. Adversarial review pass complete with all CONFIRMED findings resolved. +4. Docs propagated (component docs, both CLAUDE.mds, known-issues) with no stale cross-references. +5. One reviewed commit per package on `arch-review-remediation`; branch ready for the user to + merge/push; target-scale load test (#25) scheduled as the follow-on validation.