docs(plans): arch-review remediation plan — multi-phase, subagent-executable program for all 2026-08-14 review findings
This commit is contained in:
Joseph Doherty
@@ -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<TagSubscription>)` 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 <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 <version> — 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.
|
||||
Reference in New Issue
Block a user