docs: add 2026-10-21 system review + remediation plan

Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
2026-10-22 06:04:58 -04:00
parent d51d35bd64
commit ad2b5c4044
10 changed files with 2664 additions and 0 deletions
@@ -0,0 +1,565 @@
# System Review Remediation Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development (recommended) or superpowers-extended-cc:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Resolve every finding from the 2026-10-21 8-slice Fable system review of OtOpcUa (master `d51d35bd`): 1 Critical, ~25 High, ~60 Medium, ~85 Low. These are memory/resource leaks, unimplemented features, performance limiters, and concurrency/health-contract hazards.
**Architecture:** The work is split into **file-disjoint lanes**. Each lane is executed by a subagent in its own git worktree off master, using TDD inside each task. Each lane gets an independent adversarial review by a model at least as strong as its implementer, then merges to local master in a fixed order. Lanes run in 4 waves. A lane only touches the files it owns. A finding whose fix needs a file another lane owns is sequenced into a later wave, not shared.
**Tech Stack:** .NET 10, Akka.NET, OPC Foundation UA-.NETStandard 1.5.378, EF Core/SQL Server, Roslyn scripting, Blazor Server, gRPC (mxaccessgw / HistorianGateway), xUnit + Shouldly.
**Spec:** the review reports. These are copied into the repo by Task 0 because the scratchpad is session-scoped:
`docs/reviews/2026-10-21/00-SUMMARY.md` … `08-historians-clients.md`.
Finding refs below use `R<report>:<id>`. For example, `R07:H10` is report 07, finding H10. Each finding in a report carries its location, evidence, failure scenario and a fix recipe, and that recipe is the starting design for its task.
## Context
The user asked for a parallel review hunting memory leaks, unimplemented features and performance limiters. Eight Fable reviewers produced verified findings, and the user now wants all of them resolved. The review found 8 cross-cutting patterns (summary §"Cross-cutting themes"). The plan fixes each pattern once, then sweeps the per-driver instances:
1. The runtime never disposes drivers. This is the root of the Critical Galaxy leak: each leaked gateway session pins an x86 worker for 30 min, so about 64 respawns exhaust `MaxSessions` (mxaccessgw cross-check).
2. Timed-out scripts orphan a thread per evaluation.
3. The health and watchdog contract is wrong in both directions across drivers.
4. A plain `Dictionary` is mutated under live poll loops.
5. One device can stall every device in its driver.
6. Array writes are advertised but unimplemented.
7. Config fields are authored but never consumed.
8. Mailbox-blocking `ReceiveAsync` calls.
## Global Constraints
- **Worktree hygiene:** every lane dispatch runs `git reset --hard <current master SHA>` in its worktree and verifies it with `git merge-base --is-ancestor <SHA> HEAD`, because worktrees start on stale commits. Each lane rebases on master before review.
- **File ownership is binding:** a lane edits only the paths in its "Owns" list. If a lane needs a change elsewhere, it creates an `F<N>` follow-up task (memory: track deviations as follow-ups, don't ask) instead of editing.
- **Fix the root, then the instance:** where a pattern has a shared fix (Lane A disposes drivers, Lane E quarantines scripts), driver lanes must still make their own `ShutdownAsync` a complete teardown. The runtime fix is a safety net, not a substitute.
- **Health contract (from `.claude/rules/drivers.md` + #527/#611):**
- A deterministic device refusal is per-tag Bad, with a once-per-tag Warning and no health write.
- Comms faults write `Degraded` and preserve `LastSuccessfulRead`.
- Any data proving the device answered advances `LastSuccessfulRead`.
- Recovery dedup is on `(value, status)`, never value-only.
Every driver lane is reviewed against this checklist.
- **Re-init safety:** device tables that are read by retained poll loops are build-and-swapped (`Volatile.Write`) or made `ConcurrentDictionary`, never `.Clear()`ed in place.
- **Array writes (user decision):** encode `count` elements at the resolved element width, refuse a length mismatch with `BadOutOfRange`, and keep the ScadaBridge wire shape. ScadaBridge writes CLR lists such as `int[]` to Int16 nodes, so the element type converts and is not exact-matched.
- **Dead config fields (user decision): wire every one.** Persisted JSON with unknown keys must keep loading.
- **Absence assertions need a positive control** (memory). Any "no longer leaks / no longer publishes" test must also prove the positive path delivers.
- **AdminUI has no bUnit:** Razor changes are live-verified on the docker-dev rig (`http://localhost:9200`, login disabled) with Chrome automation, not unit tests alone.
- **Historian paging changes require a paged-vs-unpaged fuzz test** against a wire-faithful fake (memory, #692), re-run on every revision.
- **No remote commands on wonder boxes** (no psbridge or winrm). Galaxy and AVEVA gates use network service access only, and any remote action is a userGate task for the user to run.
- **`.claude/` is untracked**, so worktree agents cannot edit `.claude/rules/*`. They put suggested rule text in their final report, and the coordinator applies it in the main checkout.
- **Subagents cannot write to the coordinator scratchpad.** They return reports in their final reply.
- **Never commit secrets.** Commits end with the session attribution line.
**User decisions (already made):**
- "Implement array encode now" for AbCip, AbLegacy and TwinCAT. Real-hardware proof is a userGate.
- "Wire them all" for every authored-but-unconsumed field: MQTT `birthObservationWindowSeconds`, Galaxy `DiscoverPageSize`, Galaxy and OpcUaClient `ProbeTimeoutSeconds`, Modbus `Family`/`MelsecSubFamily`/`DisableFC23`, `TelemetryDial:CallTimeoutSeconds`, `/auth/token` JWT (finish it with `AddJwtBearer`), and the `GetDiagnostics` snapshot (populate it with real data and give it a consumer).
- "Gitea issues + local merge, push on OK": one issue per lane with findings as a checklist, merge to local master, and push/close only after the user OKs. The scadaproj index is updated on its `main`.
---
## Model assignment
| Tier | Model | Used for |
|---|---|---|
| frontier | **fable** | Actor lifecycle (A), OPC UA node manager/SDK (B), deploy coordinator (C1), script runtime + alarm actor restart (E1/E4), Galaxy (F), HistoryRead cap (H1). Fable reviews these lanes. |
| standard | **opus** | All driver lanes, control-plane remainder, Roslyn incremental analysis, sandbox, historian remainder. Opus adversarial review of every lane, including sonnet ones. |
| mechanical | **sonnet** | AdminUI UI fixes, CLI fixes, client library fixes, transport buffer pooling, docs/index edits, Gitea issue filing |
| — | haiku | not used; every task here needs code judgement |
## Waves and parallelism
```
Wave 0 (serial, coordinator) : T0 setup, T1 baseline suite
Wave 1 (6 lanes parallel) : A Runtime · B AddressSpace · C ControlPlane · E Scripting · F Galaxy · H1 HistoryRead cap
Wave 2 (8 lanes parallel) : AbCip · AbLegacy · TwinCAT · FOCAS · Modbus · S7 · G1 OpcUaClient+MTConnect · G2 MQTT+Sql
Wave 3 (4 lanes parallel) : D AdminUI · H2 Historian remainder (+B2 last) · K Clients/CLI/Transport · E5 Alarm comment table
Wave 4 (serial) : T-INT full suite + fixture gates + docker-dev gates + hw userGates + docs/index + push-on-OK
```
The wave order follows file contention:
- Wave 2 driver lanes depend on A's dispose contract only for review context, not for code.
- D waits for the Modbus/FOCAS editor and form edits in Wave 2.
- B2 waits for B (`OtOpcUaNodeManager.cs`).
- H2 waits for H1 (`HistoryPaging.cs`) and owns the recorder.
Merge order inside a wave is alphabetical by lane, rebasing as needed. Two lanes never share a function.
---
## Wave 0
### Task 0: Setup — persist the spec, file the issues, materialize the plan
**Goal:** The reports and plan live in the repo, there is one Gitea issue per lane, and native tasks exist.
**Files:** Create `docs/reviews/2026-10-21/*.md` (copy all 9 from the session scratchpad `…/scratchpad/review/`), `docs/superpowers/plans/2026-10-22-review-remediation.md` (this plan), and `….md.tasks.json`.
**Acceptance Criteria:**
- [ ] `ls docs/reviews/2026-10-21 | wc -l` = 9
- [ ] One Gitea issue per lane (A, B, B2, C, D, E, E5, F, G1, G2, H1, H2, K, AbCip, AbLegacy, TwinCAT, FOCAS, Modbus, S7). Each issue carries its findings as `- [ ] R0x:ID` lines, and the issue numbers are recorded in tasks.json.
- [ ] Cross-repo issues are filed:
- mxaccessgw: lease never extended without `StreamEvents` + no renew RPC; `mxgateway.sessions.open` not decremented on sweep close.
- HistorianGateway: `max_events` on `ReadEvents`; a batch `SendEvents` RPC; historiangw#27 tracking reference.
- [ ] Commit `docs: add 2026-10-21 system review + remediation plan`.
**Verify:** `git show --stat HEAD` lists the reports and the plan.
**Model:** sonnet (issue filing), coordinator commits.
### Task 1: Baseline full suite
**Goal:** Record the pre-change green or flake baseline, so regressions are attributable.
**Acceptance Criteria:**
- [ ] `dotnet build ZB.MOM.WW.OtOpcUa.slnx -c Release` succeeds.
- [ ] `dotnet test ZB.MOM.WW.OtOpcUa.slnx --filter "Category!=E2E&Category!=LiveIntegration"` has its pass/fail/flaky list saved to `docs/reviews/2026-10-21/baseline.txt`.
**Verify:** `tail -5 docs/reviews/2026-10-21/baseline.txt`.
**Model:** coordinator.
---
## Wave 1 — high-severity core (6 parallel lanes)
### Lane A — Runtime driver lifecycle (fable; review fable)
**Owns:**
- `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/**`
- `Runtime/Health/**`
- `Runtime/Telemetry/**`
- `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs` + the telemetry dialer
- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IDriver.cs`
- `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Browsing/DiscoveryDriverBrowser.cs`
- the `IFleetDiagnosticsClient` consumer page (one new Razor file) + `AdminUI/Clients/FleetDiagnosticsClient.cs`
**Tests:** `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests`, `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests`
- **A1 Dispose drivers on final stop** (`R02:L1`, `R02:P4`, `R06` host context, `R07:L2`).
- In `DriverInstanceActor.PostStop`: `ShutdownAsync(cts.Token).WaitAsync(5 s)`, with a logged timeout. Then `DisposeAsync`/`Dispose`, but only when the actor is stopping for good. Override `PreRestart` to skip disposal, because the Props closure reuses the driver.
- `DiscoveryDriverBrowser` disposes its throwaway driver in `finally`.
- Tests:
- A fake `IAsyncDisposable` driver is disposed exactly once on `Context.Stop`.
- It is NOT disposed on a supervision restart (positive control: it still serves after restart).
- A hung `ShutdownAsync` doesn't pin a thread beyond 5 s.
- **A2 Single-flight reconnect** (`R02:H2`). Add an `_initInFlight` gate cleared by the piped `InitializeSucceeded`/`Failed`, and bound the teardown. Test: with a 15 s init and a 10 s cadence, `ShutdownAsync` and `InitializeAsync` never overlap (record interleavings in the fake).
- **A3 Writes off the mailbox** (`R02:P2`). Run `WriteAttribute` via `PipeTo`, capturing `Sender`, with a per-driver in-flight cap (default 4). Subscribe/unsubscribe stay `ReceiveAsync`. Tests:
- Two concurrent 3 s writes both complete under the host's 8 s Ask.
- Data changes are delivered during a pending write.
- **A4 Housekeeping:**
- `_lastReprovisionUtc.Remove` in `StopChild` (`R02:L2`).
- `Stale()` drop-receives (`R02:H5`).
- Remove the unused `ILoggerFactory` ctor slot (`R02:U5`).
- Move the `OnAttributeValuePublished` seam to a test helper (`R02:U4`).
- `DbHealthProbeActor` off-thread + 2 s command timeout (`R02:P7`).
- Audit all `PollGroupEngine.Unsubscribe` sync callers and switch them to `UnsubscribeAsync` (`R05:8`).
- `TelemetryLocalHub._evicted` timestamped sweep with TTL > max in-flight call (`R02:L4`).
- **A5 Wire `TelemetryDial:CallTimeoutSeconds`** into the dialer's `CallOptions` deadline (`R02:U2`). Test: a stalled stream is cancelled at the configured deadline.
- **A6 Real `GetDiagnostics`** (`R02:U1`). Populate it from child health snapshots (real id, device counts from `GetHostStatuses`, last-change time), and give `IFleetDiagnosticsClient` a consumer: a per-node diagnostics panel on `/fleet`. That Razor file is owned here and live-verified in Wave 4.
**Lane gate:** Runtime.Tests green. The docker-dev gate (Wave 4) shows a driver config change disposing the old instance: the `ObjectDisposed`/dispose log appears, and process thread/handle counts stay flat across 20 respawns (`dotnet-counters`).
### Lane B — OPC UA address space (fable; review fable)
**Owns:**
- `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/**`, except `HistoryPaging.cs`/`RawPageLimit.cs` (H1) and the HistoryReadEvents projection (B2)
- `Runtime/OpcUa/OpcUaPublishActor.cs`
**Tests:** `OpcUaServer.Tests`, `OpcUaServer.IntegrationTests`
- **B1 No phantom nodes** (`R01:L1`, `L4`, `H2`):
- `WriteValueLocked` is non-creating (unknown key ⇒ skipped, counted, Debug-logged).
- `WriteAlarmCondition` unknown id ⇒ logged no-op.
- Delete the applier's equipment-id `RemovedConditionState` write.
- Add a cross-map `(realm,id)` collision guard in `EnsureFolder`/`EnsureVariable`/`MaterialiseAlarmCondition`.
- Tests:
- A publish racing a surgical remove leaves no node.
- A re-add after that gets the authored DataType, parent and writability (positive control).
- **B2a Stale MonitoredNode2** (`R01:L2`).
- On every removal path (rebuild, `RemoveVariableNode`, `RemoveAlarmConditionNode`, `RemoveEquipmentSubtree`): queue `BadNodeIdUnknown` to the node's items, `MonitoredNodes.Remove(nodeId)`, and drop `_liveItems`/`_eventItems`.
- Revalidation restore re-resolves the node and does not reuse `Handle.Node`.
- Integration test (real SDK): subscribe, rebuild, create a new item on the same NodeId. The new item receives values (positive control) and the old one gets `BadNodeIdUnknown`.
- **B3 Lock-time performance:**
- Rebuild detaches top-level subtrees, not leaves (`R01:P1`).
- Per-equipment descendant index for `RemoveEquipmentSubtree` (`P2`).
- `LowerAllEventsMonitoring` for the surgical removal sites (`P3`).
- Notifier wiring de-quadratic (`P5`).
- Benchmark test: rebuild a 10k-tag folder and remove 200 equipment from a 50k-node space. Assert completion < 1 s on CI hardware and record the before/after in the commit message.
- **B4 Write-path robustness:**
- `OnEquipmentTagWrite` observes `t.Exception` and wraps a synchronous throw (`R01:L3`, `H5`).
- Shelve/condition timestamps use the injected `TimeProvider` (`H4`).
- SDK request quotas + `TransportQuotas` configurable via `OpcUa:Quotas:*` with the current defaults (`P6`).
- HistoryRead arms: lower the default `MaxConcurrentHistoryReadBatches` on small nodes, and use an `IAsyncNodeManager` override if the SDK line exposes `HistoryReadAsync`; otherwise document it (`P4`).
- **B5 Delete dead v3 equipment-tag materialisation** (`R01:U2`): the composer's `EquipmentTags`, applier `MaterialiseEquipmentTags`, the surgical `ChangedEquipmentTags` loop, and the classifier `TagDeltaIsSurgicalEligible`. Keep `IsEmpty` semantics; the full OpcUaServer suite stays green.
- **B6 Off-thread deploy parse/plan** (`R02:P5`).
- `DeploymentArtifact.Open` + `ParseComposition` + `AddressSpacePlanner.Compute` run via `PipeTo`; only sink materialisation runs on the pinned thread.
- Delete the bytes-less `LoadArtifact` SQL branch.
- Test: value batches keep flowing while a 50k-node plan computes.
- **B7 Capability bits refresh** (`R01:U3`). Re-evaluate event-notifier/HistoryWrite bits when `HistorianDataSource`/`Updater` is set after the first apply, so a late assignment no longer needs a full rebuild.
- Documented, no change: `R01:U4` (native Confirm/shelve, by design) and `R01:L5` (bounded). Note both in the issue.
**Lane gate:** both suites green. Wave 4 docker-dev gate: a ScadaBridge-style subscription survives a surgical add, and a re-created item after a rebuild receives data (Client.CLI `subscribe`).
### Lane C — Control plane / host / security (fable for C1–C2, opus for C3–C5; review fable)
**Owns:** `src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/**`, `src/Server/ZB.MOM.WW.OtOpcUa.Host/**` (except `Host/Engines/**`, which belongs to E), `src/Server/ZB.MOM.WW.OtOpcUa.Security/**`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/**` (except `Validation/DraftValidator.cs`, which belongs to E).
**Tests:** `ControlPlane.Tests`, `Host.Tests`, `Security.Tests`, `Configuration.Tests` (central SQL)
- **C1 Deploy wedge + consistent snapshot** (`R03:2.2`, `3.2`, `3.5`):
- In PreStart, a recovered deployment with an empty expected set is re-seeded and re-dispatched (or marked `Failed` + `ResetForNext`).
- `HandleDeadline` takes the race branch only when `_expectedAcks.Count > 0`.
- The `Dispatching` row and the expected set are created together in the coordinator.
- Validate + compose from ONE snapshot (extend `DraftSnapshot` with clusters/nodes/ApiKeys) inside a `SNAPSHOT` read transaction.
- Project `ArtifactBlob` away in full-row reads.
- Tests:
- Inject a `HandleDispatch` throw; the next `StartDeployment` succeeds (positive control: a normal deploy seals).
- An edit between validate and compose cannot ship unvalidated.
- **C2 Admin singleton mailbox** (`R03:3.1`). Move `TestDriverConnect` to a per-request child actor (or `Task.Run` + `PipeTo`) and run deploy compose off-actor; only the in-flight check is serialized. Test: an alarm ack completes in < 1 s while a 60 s Test Connect is pending.
- **C3 OPC UA start failure is visible** (`R03:2.1`, `1.4`):
- Add an `OpcUaEndpointHealthCheck` (Ready + Active tags), Unhealthy while the SDK failed to start.
- A driver-only node rethrows (process exits non-zero).
- The "SDK not started" fact feeds the ServiceLevel calculator, so the peer takes Primary. If the calculator lives in a Lane B-owned file (`OpcUaServer/**`), C only exposes the fact (`IOpcUaEndpointState`) and files an `F<N>` follow-up for Wave 3 to consume it.
- `StopAsync` stops the SDK; `DisposeAsync` is idempotent.
- Tests: port-in-use ⇒ `/health/ready` 503, and the ServiceLevel published drops.
- **C4 Security:**
- `ApiKeyOptionsValidator` + `ValidateOnStart` (non-blank pepper, minimum length, whenever driver or admin) (`R03:2.3`).
- **Finish JWT** (`R03:2.4`, user decision): `AddJwtBearer` using `BuildValidationParameters()`; `JwtOptions` `ValidateOnStart` when `/auth/token` is mapped; the `/api/*` machine endpoints accept a Bearer token as well as API keys.
- Wire `IAuditActorAccessor` through `AuthenticationStateProvider`, so audit rows carry the circuit principal (`R03:2.5`).
- Delete the unregistered `CookieAuthenticationStateProvider`.
- LDAP login exception logged at Warning (`2.7`).
- Admin ops return a fixed message + correlation id (`4.1`).
- **C5 Retention + throughput:**
- `DeploymentRetentionActor` on the admin singleton: keep the last N=50 sealed rows per cluster plus anything referenced in the last 30 days; null `ArtifactBlob` on older rows; configurable `Deployments:Retention:*` (`R03:1.2`).
- `AuditWriterActor` handles duplicate keys per row (2601/2627 ⇒ detach and retry) (`2.6`).
- `MeshGrpcCommandDialer` evicts endpoints absent from `ContactsLoaded` (`1.3`).
- Async PSK refresh with an atomic swap instead of invalidate-on-refresh (`3.4`).
- Pre-auth API-key DoS: an in-memory key-id set from the artifact or `IDataPlaneAuthoritySource`, refreshed every `StoreCacheSeconds`; an unknown id is rejected without SQL; plus a per-endpoint token bucket (`3.3`).
- Document the `4.2` fail-open window in `docs/security.md`.
**Lane gate:** suites green. Wave 4 docker-dev gate: kill central-1 mid-dispatch, then the next deploy succeeds. Port 4840 taken ⇒ `/health/ready` 503.
### Lane E — Scripting runtime safety + scripted alarms (fable for E1/E4, opus for E2/E3; review fable)
**Owns:**
- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/**`, `Core.ScriptedAlarms/**`, `Core.AlarmHistorian/**`
- `Runtime/ScriptedAlarms/**`, `Runtime/VirtualTags/**`
- `Host/Engines/**`
- `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/**`
- `Configuration/Validation/DraftValidator.cs`
**Tests:** `Core.Scripting.Tests`, `Core.ScriptedAlarms.Tests`, `Core.AlarmHistorian.Tests`, `Driver.Calculation.Tests`, `Runtime.Tests` (VirtualTags/ScriptedAlarms folders)
- **E1 Script quarantine** (`R03:1.1`, `R05:2`, `R07:L1`, `R07:P5`):
- A shared `ScriptRunGuard` in Core.Scripting, keyed per script instance.
- It refuses to start a run while that script's previous orphan is still executing. The result is Bad `"previous evaluation still running"` plus a counter.
- It quarantines after 3 consecutive timeouts: stop evaluating, publish Bad, log Error, metric `otopcua_script_quarantined`, cleared on redeploy or source change.
- Pass a `CancellationToken` into scripts.
- Make the Calculation pump async, so pool starvation is no longer charged to scripts.
- Wired into the scripted-alarm engine, `RoslynVirtualTagEvaluator`/`VirtualTagHostActor`, and `CalculationEvaluator`.
- Tests: a `while(true)` script on a 1 Hz trigger leaves ≤ 1 orphaned thread after 30 triggers (`ThreadPool.ThreadCount` delta). Positive control: a healthy sibling keeps evaluating.
- **E2 Sandbox hardening** (`R05:10`, `R05:11`):
- Deny `System.Threading` with an allow-list (`CancellationToken`, `Interlocked`, `Volatile`).
- Deny `System.Type.GetType`/`GetMethod*`/`InvokeMember` and `System.Runtime.CompilerServices.RuntimeHelpers`.
- Add an allocation budget: sample `GC.GetAllocatedBytesForCurrentThread` around each run and quarantine on breach (`Scripting:MaxAllocBytesPerRun`, default 64 MB). Document that a pre-emptive OOM still needs an out-of-process runner, and file a follow-up issue for that.
- Tests: each forbidden form gets an `OTSCRIPT` diagnostic, and the Monaco editor parity test still matches.
- **E3 Scripted-alarm engine** (`R05:1`, `3`, `6`, `7`, `9`, `12`):
- Clear `_valueCache` in `LoadAsync`; move its write below the `_disposed` guard.
- Bounded `Dispose` drain (5 s, cancelable token).
- `ScriptedAlarmSource` catch logs at Warning.
- `LocalDbStoreAndForwardSink` cached dead-letter counter, resynced with the queue counter.
- Script-log sinks rate-limited per source (20/min + summary).
- **E4 Alarm host restart + virtual-tag log storms** (`R02:H1`, `H3`, `P6`, `L3`; `R07:P4`, `H6`, `H8`, `U5`):
- `ScriptedAlarmHostActor` receives a `Func<(upstream, engine)>` factory, building its engine per incarnation.
- Linked 10 s timeouts on `OnAlarmCommand`.
- `DependencyMuxTagUpstreamSource.RetainOnly(loaded deps)`.
- `VirtualTagActor` per-tag failure-log rate limit.
- Calculation:
- negative-cache compile failures by source hash, with the Warning/script-log only on the Good→Bad transition;
- the mux retains the last `(value,status,ts)` and replays it on `RegisterInterest`, with inputs recorded regardless of `_running`;
- structural array dedup;
- `ClearCompiledScripts` from `ShutdownAsync`;
- a DraftValidator rule `CalculationNoEffectiveTrigger`.
- Tests:
- Throw inside a `DependencyValueBatch` handler, then a later `ApplyScriptedAlarms` loads cleanly.
- A calc Restart republishes the current inputs.
**Lane gate:** suites green. Wave 4 docker-dev gate: author a `while(true)` virtual tag. The node stays responsive, the tag shows quarantined, and the thread count stays flat over 2 min.
### Lane F — Galaxy driver (fable; review fable)
**Owns:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy*/**`, the AdminUI `GalaxyDriverForm.razor`.
**Tests:** `Driver.Galaxy.Tests`, `Driver.Galaxy.Browser.Tests`
- **F1 Real teardown** (`R06:G-1`, `G-2`, `G-9`, `G-3`):
- `ShutdownAsync` stops the pump, alarm feed, deploy watcher, probe watcher and supervisor, disposes the session + clients, and nulls the seams so `InitializeAsync` rebuilds. `DisposeAsync` stays the idempotent final teardown.
- `StartDeployWatcherAsync` stops the previous watcher.
- `ReconnectSupervisor` disposes each cycle's CTS.
- Tests with fake gRPC clients:
- After `ShutdownAsync`, every stream is cancelled and both channels are disposed.
- Five watchdog cycles leave exactly one DeployWatcher (positive control: rediscovery still fires once per deploy).
- **F2 Probe watcher + liveness:**
- Register probe handles in `SubscriptionRegistry` under a reserved id, and `Rebind` them in `ResubscribeAllAsync` (`G-4`).
- `GalaxyDiscoverer` returns its fetched hierarchy, so discovery fetches once (`G-7`).
- A throttled `LastSuccessfulRead` stamp from `OnPumpDataChange` (`G-11`).
- An `EventPump` dispatch-loop exit ⇒ `NotifyStreamFault` (`G-10`).
- Test: a `ScanState` event updates `GetHostStatuses` and survives a reconnect.
- **F3 Wire knobs + writes:**
- `DiscoverPageSize` → `DiscoverHierarchyAsync`. If the mxaccessgw RPC lacks a page parameter, add a cross-repo task in mxaccessgw and page client-side meanwhile.
- `ProbeTimeoutSeconds` → probe deadline (`G-5`, `G-6`).
- First-touch write pipeline: concurrent AddItem/Advise across distinct tags, bounded at 8 (`G-8`; acks stay serial by design).
**Lane gate:** suites green. Wave 4 userGate: with a reachable mxaccessgw, 20 config deploys keep `mxgateway.sessions.open` flat, read from the gw metrics endpoint.
### Lane H1 — HistoryRead raw cap (fable; review fable + paging fuzz)
**Owns:** `OpcUaServer/HistoryPaging.cs`, `RawPageLimit.cs`, the `ServerHistorian` options type, and the `ReadRawAsync` methods of the three `*HistorianDataSource.cs` files.
**Tests:** `OpcUaServer.Tests` (paging), `Historian.Conformance`, the backend `*.Tests`
- `NumValuesPerNode = 0` ⇒ server page size `ServerHistorian:MaxRawValuesPerRead` (default 10 000), with a continuation point via the existing `RawPageLimit` paging. The cap is advertised as `HistoryServerCapabilities/MaxReturnDataValues`.
- Backend `TOP`/`LIMIT`/`max_values` are bounded by the same cap (`R08:1.1`).
- A paged-vs-unpaged fuzz test: random windows/caps against a wire-faithful fake for each backend, where the concatenated pages equal the unpaged truth, with boundary ties.
**Lane gate:** the fuzz runs 10k iterations green. The Wave 4 SqlServer/QuestDb fixture gate reads a year window at N=0 and gets pages of ≤ 10k with a CP, and heap stays < 500 MB.
---
## Wave 2 — driver lanes (8 parallel; opus implementer, opus review)
Every driver lane also:
- (a) makes `ShutdownAsync` a complete teardown;
- (b) applies the Global health-contract checklist;
- (c) build-and-swaps device tables;
- (d) adds `IBatchSubscribable` where it runs `PollGroupEngine` (`R02:P1`).
Each lane owns `src/Drivers/<Driver>*/**` + its `tests/Drivers/<Driver>*`, its AdminUI tag-config editor/driver form files, and its `Cli/<Driver>.Cli`.
### Lane AbCip
- Probe classification: `NotFound` ⇒ reachable, with a config warning (`R07:H10`).
- Widen `IsReachableStatus`, cache the runtime with its refusal status, map 0x1C/0x20/0xFF-0x2104 explicitly (`H1`, `L4`).
- Native window ≥ 64 (`P8`); cold waiters don't hold slots (`P7` addendum).
- Bit-in-word parent width from `GetSize()` (`H17`).
- Write-side health no longer latches or overwrites (`H19`).
- BOOL-array element decodes bit n%32 of DWORD n/32 (`U11`).
- **Array write encode** (`U10`).
- UDT layout cache (`P11`).
- Device-table swap (`H13`); `volatile _health` (`H21`).
- `DisposeAsync` instead of sync-over-async (`F10`).
### Lane AbLegacy
- Per-device `ConnectionBackoff` + single-flight connect + bounded parallel devices + adjacent-element coalescing (`R07:P7`).
- `NotSupportedException` ⇒ `BadNotSupported`, no health, reject at `FromTagConfig` (`H16`).
- Table swap (`H13`); prune semaphores on index change (`L7`).
- **Array write encode** (`U10`); `IBatchSubscribable`.
- Document the PLC-5 write capability gap with a hardware follow-up issue (`U6`).
### Lane TwinCAT
- Per-registration isolation with dead-registration retry (`R07:H11`).
- Stamp `LastSuccessfulRead` from notifications + probe, and degrade on N consecutive router failures (`H12`).
- Warn once per tag (`P10`).
- Parent-width bit RMW (`H18`).
- **Array + string writes via the codecs** (`U9`).
- `BadNodeIdUnknown` for unresolved native refs (`U12`).
- Bounded probe join, drop counter, queue purge on dispose, re-init table swap (`L8`); `IBatchSubscribable`.
### Lane FOCAS
- `ConcurrentDictionary`/swap `_devices` (`R07:H13`).
- Widen macro/param numbers to int on the wire client (`H14`).
- Fixed-tree loop writes health (`H15`).
- Reject `String` in the factory + editor (`U8`).
- Concurrent per-device reads (`P9`).
- Recycle under `ConnectGate` (`L6`).
- Alarm-layout heuristic uses the record count from the header (`H20`).
### Lane Modbus
- Deadband baselines on `(value,status)` (`R07:H2`).
- Transport self-heals with a throttled in-gate connect (`P3`).
- Dispose the failed `TcpClient`, one in-gate attempt, DNS under the timeout (`L3`).
- **Wire Family/MelsecSubFamily/DisableFC23** (`U1`): the editor + factory route the address through `ModbusAddressParser` and persist the resolved region/offset; `DisableFC23` gates multi-write.
- Coalescing on by default (gap 8), plus coils/DI/BitInRegister folding (`U2`).
- String[]/BitInRegister[] writes (`U3`).
- Region/type validation at the factory (`P6`).
- Exception codes 01/02/03 are refusals (`H4`).
- Write status mapping (`H7`); PDU length check (`H9`).
### Lane S7
- Dead-handle short-circuit inside a pass (`R07:P1`).
- Per-member span fallback + "do not group" memo (`P2`).
- Per-tag skip + `TryParse` in `FromTagConfig`/Inspect, so the deploy gate refuses bad tags (`H3`).
- Non-fatal `ReadData`/`WriteData` are refusals (`H4`).
- `_lifecycleGate` (Modbus pattern) (`H5`).
- Bool-array bit-offset reject at authoring, or bit-packed decode (`U4`).
- Probe gate contention ≠ Stopped; honour `TimeoutMs` (`H7`).
### Lane G1 — OpcUaClient + MTConnect
**OpcUaClient:**
- Data-subscribe cleanup on throw (`R06:O-1`).
- Ack rethrows the transport status (`O-8`).
- Split the read/write gate from subscription management; no gate across the recursive browse (`O-6`).
- Close the local session on an init throw (`O-2`).
- One teardown budget (`O-9`).
- Wire `ProbeTimeoutSeconds` (`O-4`).
- Browse `AttributesAsync` real + certificate-auth browse (`O-5`).
**MTConnect:**
- Generation guard on cursor publish (`T-1`).
- Read through the live index when the pump is healthy (`T-4`).
- Lag metric + adaptive `count` (`T-5`).
- Instance-id check in `ReadAsync` (`T-2`).
- Delete dead parsed fields (`T-3`).
- Batched PUT (`T-6`).
- Headers-only probe (`T-7`).
- Redacted URI everywhere (`T-8`).
### Lane G2 — MQTT + Sql
**MQTT:**
- Failed SUBSCRIBE ⇒ pending set retried on reconnect/reconcile, `SubscribeAsync` throws on a transport failure (`R06:M-1`).
- **Wire `birthObservationWindowSeconds`** into `MqttDriverBrowser.OpenBudget` + discovery UntilStable (`M-4`).
- Array leaves not bindable, plus a Sparkplug-array-read feature issue (`M-5`).
- Prune `LastValueCache` on Register (`M-2`).
- Dispose the failed connection (`M-3`).
- Wildcard topic trie (`M-6`); static UTF-8 encoder (`M-7`).
- Bounded concurrent publishes (`M-8`); guards (`M-10`, `M-11`).
**Sql:**
- Row cap with a `TOP/LIMIT N+1` dialect + config-error status (`S-1`).
- Rate-limited warnings (`S-6`); plan cache (`S-7`).
- Dispose `_gate` (`S-3`).
- `Query` model refused at the deploy gate via the parser + feature issues for MySql/Odbc/allowWrites (`S-4`).
- `IS_NULLABLE` in browse attributes (`S-5`).
- Withhold JSON parser text (`S-8`); `IBatchSubscribable`.
**Driver lane gates:**
- Wave 4 fixture runs on `10.100.0.35`: modbus `standard`, s7 `s7_1500`, abcip `controllogix`, opcuaclient opc-plc, mqtt + sparkplug, sql. Each lane adds or extends an IntegrationTests case per High finding.
- Hardware userGates for real controllers: AbCip/AbLegacy/TwinCAT array writes, FOCAS macro > 32767, AbCip probe-typo recovery.
---
## Wave 3
### Lane D — AdminUI (opus for D1–D2, sonnet for D3–D4; review opus)
**Owns:** `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/**`, except the per-driver editors/forms owned in Wave 2 and the A6 diagnostics panel.
- **D1 Monaco sync correctness** (`R04:P1`, `P9`, `S2`, `S3`):
- Fetch the editor text on demand at Save/blur via `MonacoBlazor.getValue` instead of per keystroke.
- `HubOptions.MaximumReceiveMessageSize` = 1 MB.
- Diagnostics generation counter + `AbortController`.
- Drop the inlay-hints provider/endpoint.
- Build a Problems list using `MarkersChanged` + `RevealLineAsync`.
- **D2 Roslyn analysis** (`P2`, `P3`, `S1`):
- One compilation per scoped service with `ReplaceSyntaxTree`.
- `RequestAborted` plumbing + Monaco token honouring.
- Sorted `string[]` + dictionary in the catalog snapshot.
- An `OTSCRIPT_SETVTAG_INERT` warning marker.
- **D3 Rendering** (`P4`–`P8`, `H2`):
- `/hosts` dirty flag on tick.
- `RawTree` cached menus + `Virtualize` + 500-per-level paging.
- CSV import grid `Virtualize` + export via a download endpoint.
- ScriptLog/Alerts 200 ms coalescing + ring buffer.
- `/scripts` projection.
- Cached modal parameter dictionaries.
- **D4 Leaks/hazards/stubs** (`L1`–`L5`, `H1`, `H3`, `S4`–`S7`):
- Snapshot-store eviction against the config load.
- `Threading.Timer` in `DriverTestConnectButton`.
- `AlarmsHistorian` page `Fleet.razor` pattern + "no historian on role" state.
- Close an orphaned browse token after dispose.
- Per-subscriber try/catch in broadcasters.
- Confirm dialogs on equipment-page and ScriptEdit deletes.
- Document the theme (`H3`).
- Delete dead `UnsTreeService` members.
- `GET /api/deployments/{id}`.
- Equipment CSV via `CsvParser`.
- Gate `/dev/*` behind `#if DEBUG`.
**Lane gate (userGate-free, coordinator-run):** docker-dev Chrome pass.
- Paste a 100 KB script, Save, reload: the source persists.
- Two-tab `/hosts` with 50 drivers: CPU stays flat.
- Expand a 5k-tag device: renders in < 2 s.
- Each delete confirms.
- The A6 diagnostics panel shows real counts.
### Lane H2 — Historian remainder (opus; review opus)
**Owns:** `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.*/**` (minus H1's files), `Runtime/Historian/**` (recorder + adapter), `docs/Historian.md`, the AdminUI historian backend picker.
- Gateway events: drain to completion with a budget, cancellation arms (`R08:1.2`, `2.5`).
- Drop native `TimeAverage`/`Total` behind `ServerHistorian:Gateway:EnableNativeTimeWeighted=false` until historiangw#27 (`2.1`).
- Capability matrix in docs + picker + a startup warning for continuous historization on Gateway (`2.2`).
- `ProbeAsync` health timer (`2.4`).
- Writers:
- OCE rethrow in all three (`4.1`).
- Gateway via `LiveWritePreparer` (`4.2`).
- `IHistorianValueWriter.FlushAsync` pass-end hook, QuestDB WAL wait once per pass with exponential poll (`3.1`).
- Batched alarm-history writers: SqlServer TVP + `OUTPUT`; QuestDb multi-row; Gateway bounded pipelining until a batch RPC (`3.2`).
- Retention pre-filter (`3.3`).
- QuestDB symbol indexes + migration (`3.4`).
- Presence cache exact lookup (`3.5`).
- Paged `DeleteRaw` (`4.3`).
- QuestDB trail `applied` marker + DEDUP key (`4.4`).
- Recorder: `CommitMode=Periodic(100 ms)` default + off-mailbox append with an in-flight cap + mailbox-depth metric (`R02:P3`); timeouts on `GetStatus` (`R02:H3`).
- `HistorianAdapterActor` bounded channel + delete the senderless receive (`R02:H4`, `U3`).
- Outbox scan outside the lock (`R08:1.3`).
**Lane gate:** `Historian.Conformance` green on all backends. The Wave 4 AVEVA live gate (userGate, existing recipe in `.claude/rules/historian.md`) checks events paging and that `TimeAverage` is refused.
### Lane K — Clients, CLI, transport (sonnet; review opus)
**Owns:** `src/Client/**`, `src/Drivers/Cli/Driver.Cli.Common` + the AbCip/AbLegacy CLI command files (after the Wave 2 merge), `Driver.Ab.Transport/**`.
- Client:
- release the raw CP (`R08:1.4`);
- `RemoveSubscription` (`1.5`);
- align `AggregateType` with `AggregateFamilies.Served` (`2.3`);
- lazy HasChildren (`3.6`);
- NodeId dictionary in the UI (`3.6`);
- failover under `_subscriptionLock`, and `ConnectAsync` refusing when already connected (`4.5`).
- CLI fixes (`R07:U7`).
- Pooled buffers in `EncapsulationClient`/`CipWriter`/`LogixTagClient` + Modbus transports via `ArrayPool` (`R07:L5`). Benchmark-gated: no regression in `Ab.Transport.Tests` timings.
### Lane B2 — HistoryReadEvents full projection — folded into Lane H2 as its LAST task (opus; review fable)
This runs inside H2 because it shares the backends' event read paths with H2's Gateway events-drain fix. It runs after H2's other tasks, and `OtOpcUaNodeManager.cs` is free by then because B merged in Wave 1.
**Adds to H2's ownership:** the `HistoricalEvent` contract in Core.Abstractions and the HistoryReadEvents projection in `OtOpcUaNodeManager.cs`.
- Carry `EventType`/`ConditionName`, map `SourceNode` when resolvable, and honour an `OfType` where-clause (`R01:U1`).
- Test over the wire: a Client.CLI `historyread --events` with a select clause including EventType returns non-null values.
### Lane E5 — Alarm comment audit table (opus; review fable)
**Owns:** `Core.ScriptedAlarms/Part9StateMachine.cs`, the alarm state store + its LocalDb schema/migration.
- Move the append-only comments to their own table keyed by alarm id, keeping replication to the pair peer. Each condition row stays a fixed size.
- Migrate existing inline comments idempotently (`R05:4`).
- Test: 10k comments, then a transition's `SaveAsync` payload size is constant. A pair-replication test shows the comments on the peer.
---
## Wave 4 — integration (serial, coordinator)
### Task INT-1: Full suite + flake triage
Run the full suite again. Any new failure versus `baseline.txt` is either fixed or filed. Loop any suspect test under `yes` contention.
### Task INT-2: Fixture gates (sims on 10.100.0.35)
For each driver lane, sync and bring up its fixture, then run `*.IntegrationTests` with the env endpoints. Record the results in `docs/reviews/2026-10-21/gates.md`.
### Task INT-3: docker-dev live gates (coordinator, Chrome automation + Client.CLI)
This runs the lane gates for A, B, C, D and E listed above, plus the A6 `/fleet` panel and the H1 N=0 paging read through Client.CLI `historyread`.
### Task INT-4 (userGate): hardware and AVEVA gates
> **USER-ORDERED GATE — NON-SKIPPABLE.** This task was requested by the user in the current conversation (array encode on real controllers). It MUST NOT be closed by walking around it, by declaring it "verified inline", or by substituting a cheaper check. Close only after every item in `acceptanceCriteria` has been re-validated independently, with output captured.
Acceptance criteria:
- An AbCip array write round-trips on a real ControlLogix.
- An AbLegacy N-file array write round-trips on a real SLC/MicroLogix.
- A TwinCAT array + STRING write round-trips on real hardware.
- A FOCAS `MACRO:` > 32767 reads the correct variable.
- Galaxy: 20 deploys keep `mxgateway.sessions.open` flat.
- AVEVA: the events paging + `TimeAverage` refusal gate.
`requiresUserSpecification: true` for the bench endpoints.
### Task INT-5: Docs, rules, index, push-on-OK
- Apply each lane's suggested `.claude/rules/*.md` text in the main checkout.
- Update `CLAUDE.md` where behaviour summaries changed: the health contract, surgical/rebuild notes, the historian N=0 rule.
- Update `../scadaproj/.claude/skills/sister-project-history/otopcua.md` (+ the index row only if the summary changes) on scadaproj `main`, following the CLAUDE.md recipe, and push scadaproj.
- Draft the per-issue closing comments, then ask the user for OK before pushing OtOpcUa master and closing the issues.
- Save project memory for lessons learned.
---
## Verification (end to end)
1. `dotnet build ZB.MOM.WW.OtOpcUa.slnx -c Release` + the full unit suite match the baseline or better.
2. Fixture IntegrationTests green per driver (INT-2).
3. docker-dev gates (INT-3):
- Respawn churn leaves threads/handles flat (`dotnet-counters monitor -p <pid> System.Runtime`).
- A re-subscribe after a rebuild receives data.
- The deploy wedge recovers.
- A spin script is quarantined.
- A 100 KB script saves.
- An N=0 HistoryRead pages.
4. Hardware/AVEVA userGates (INT-4).
5. Each Gitea issue checklist has every `R0x:ID` ticked with a commit SHA, or explicitly re-filed as a follow-up.
## Self-review notes
- Coverage: every finding ID in reports 01–08 maps to a lane above. The two documented-no-change items (`R01:U4`, `R01:L5`) are recorded in the B issue, and `R08:2.2` is gateway-side and handled through cross-repo issues + docs.
- Conflict check: the files shared across lanes are `OtOpcUaNodeManager.cs` (B in Wave 1, then H2's last task in Wave 3), `HistoryPaging.cs` (H1 only), the three `*HistorianDataSource.cs` (H1's `ReadRawAsync` in Wave 1, H2 for the rest in Wave 3), the recorder (H2 only), `DraftValidator.cs` (E only), and AdminUI driver forms (driver lanes in Wave 2, then D in Wave 3). No two lanes in the same wave share a file.
- Concurrency budget: up to 8 worktree builds in Wave 2. If the machine thrashes, split Wave 2 into 2×4: AbCip/AbLegacy/TwinCAT/FOCAS, then Modbus/S7/G1/G2.