Joseph Doherty
1891f5d6a7
Re-ran all seven domain reviews at master f6eaa267 (reports rewritten in
place, each with a prior-finding status table): all 4 round-1 Criticals
verified closed; new top findings are the S7 connect-timeout OCE regression
(05/STAB-14), the ResilienceConfig operator-authorable brick (01/S-6), and
a batch of resilience-seam Mediums. 00-OVERALL.md carries the updated
maturity matrix + 12-item action list.
Adds R2-01..R2-12 design/implementation plans (one per action item, house
format + bite-sized TDD task breakdowns + co-located .tasks.json; 193 tasks,
~18-24 dev-days). STATUS.md updated: round-1 topology marked historical
(all merged+pushed), re-review findings table + plan pointers added.
112 lines
15 KiB
Markdown
112 lines
15 KiB
Markdown
# OtOpcUa — Deep Architecture Review: Overall Report
|
||
|
||
**Date:** 2026-07-12
|
||
**Commit:** `f6eaa267` (master, clean tree)
|
||
**Method:** Seven parallel domain re-reviews, same scopes and four dimensions (stability, performance, conventions, underdeveloped areas) as the original. This is the **first update** of the 2026-07-08 review at `9cad9ed0`: every prior finding was re-verified against code (not against remediation claims), and each domain got a fresh-eyes pass with extra scrutiny on the newly merged remediation code itself. Per-domain reports carry a `Prior-finding status (9cad9ed0 → f6eaa267)` table; remediation history is in [`plans/STATUS.md`](plans/STATUS.md).
|
||
|
||
**Delta reviewed:** all 11 arch-review remediation branches merged and pushed to `origin/master` (4 Criticals, 3 systemic guards, #10 CapabilityInvoker dispatch wiring, #13 ResilienceConfig artifact plumbing, #11 Crit-1 premise correction, docs branch, docker-dev rig fix), plus the fixdocs XML-doc sweep (`3b5ef439`) and the HistorianGateway Client/Contracts 0.1.0→0.2.0 bump (`f6eaa267`). 84 files, +9218/−217.
|
||
|
||
## Report index
|
||
|
||
| # | Report | Scope |
|
||
|---|---|---|
|
||
| 01 | [Core & composition pipeline](01-core-composition.md) | Core, Core.Abstractions, Commons, Configuration, Cluster; AddressSpace Composer/Planner/Applier |
|
||
| 02 | [Scripting, virtual tags & alarm engines](02-scripting-alarms.md) | Core.Scripting(+Abstractions), Core.VirtualTags, Core.ScriptedAlarms, Core.AlarmHistorian |
|
||
| 03 | [Server host, runtime & OPC UA](03-server-runtime.md) | OpcUaServer, Host, Runtime, ControlPlane, Security; redundancy, LDAP, node manager |
|
||
| 04 | [AdminUI (Blazor)](04-adminui.md) | AdminUI pages, ScriptAnalysis, tag editors, authorization |
|
||
| 05 | [Protocol drivers](05-protocol-drivers.md) | Modbus, S7, AbCip, AbLegacy, TwinCAT, FOCAS, OpcUaClient (+Contracts, +Cli) |
|
||
| 06 | [Gateway integrations](06-gateway-integrations.md) | Driver.Galaxy(+Browser,+Contracts), Driver.Historian.Gateway |
|
||
| 07 | [Client, tooling & engineering system](07-client-tooling-engineering.md) | Client.CLI/Shared/UI, Analyzers, build/CPM, CI, test architecture, repo hygiene |
|
||
|
||
## Consolidated maturity matrix (1–5, previous score in parentheses)
|
||
|
||
| Subsystem | Stability | Performance | Conventions | Underdeveloped |
|
||
|---|---|---|---|---|
|
||
| 01 Core & composition | 4 (4) | 3 (3) | 4 (4) | 3 (3) |
|
||
| 02 Scripting & alarms | 3.5 (3.5) | 3 (3) | 4 (4) | 3 (**2**) |
|
||
| 03 Server & runtime | 3 (3) | 3 (3) | 4 (4) | 4 (3) |
|
||
| 04 AdminUI | 4 (4) | 3 (3) | 3 (3) | 3 (3) |
|
||
| 05 Protocol drivers | 3 (3) | **2** (2) | 3 (3) | 3 (3) |
|
||
| 06 Gateway integrations | 4 (4) | 4 (4) | 4 (4) | 3 (3) |
|
||
| 07 Client & engineering | 3.5 (**2.5**) | 3.5 (3.5) | 4 (3) | 3 (3) |
|
||
|
||
**Read of the matrix:** the remediation round did exactly what it targeted. The verification system moved most (07 stability 2.5→3.5, conventions 3→4: whole-solution CI, fail-on-skip, tree-wide analyzer — all verified live with negative controls), and the two engine gaps closed (02 underdeveloped 2→3; 03 underdeveloped 3→4 via the failover test + forwarding guard). Nothing regressed at the matrix level. What kept scores from rising further: the untouched High backlog (failure visibility, AdminUI authz, driver perf/concurrency) and **new second-order findings inside the remediation code itself** — the sharpest of which are one High regression (S7) and one operator-authorable driver-brick (ResilienceConfig).
|
||
|
||
---
|
||
|
||
## Status of the original four Criticals
|
||
|
||
| # | Original Critical | Verdict at `f6eaa267` |
|
||
|---|---|---|
|
||
| 1 | Split-brain resolver never activated | **Premise corrected (#11) + closed** — SBR was already active on master (Akka.Cluster.Hosting applies `SplitBrainResolverOption.Default` → pre-existing akka.conf keep-oldest). Now explicit in code (`KeepOldestOption{DownIfAlone=true}`) and guarded by the hard-kill failover integration test (`HardKillFailoverTests`, runs in CI). |
|
||
| 2 | VT script timeout ineffective + ALC cache leak | **Fixed** (`7fd44f0f`) — production path routes through `TimedScriptEvaluator`; `CompiledScriptCache` + apply-boundary clear. Residuals: new 02/S13 (failure/timeout leaves the node stale-Good, below) and 02/S12 (clear-vs-in-flight race). |
|
||
| 3 | S7 no reconnect | **Fixed** (`25c0c6f6`, live-verified vs python-snap7) — but the fix carries a **new High regression, 05/STAB-14** (below). |
|
||
| 4 | TwinCAT orphaned ADS subscriptions | **Fixed** (`af318fb4`, fake-client verified; no live fixture exists) — residual 05/STAB-16 (replay failure is log-only). |
|
||
|
||
## Critical/High findings — updated ranking
|
||
|
||
**New in this round (all inside or activated by the remediation delta):**
|
||
|
||
1. **05/STAB-14 (High, regression)** — S7's ensure-wrapper rethrows connect-*timeout* `OperationCanceledException` unconditionally, and `PollLoopAsync`'s bare `catch (OCE) { return; }` then **permanently, silently kills every S7 subscription poll** after an unreachable-host outage (PLC dead-to-SYN, not refusing). The live bounce test can't see it — `docker restart` yields connection-refused, not timeout. Two-line filter fix + a fake-factory connect-timeout test (`S7Driver.cs:488, 1000, 1383, 1404, 1206-1208`).
|
||
2. **01/S-6 (High)** — a valid-JSON `ResilienceConfig` with `timeoutSeconds: 0` (or out-of-range) passes the parser but makes Polly throw `ValidationException` at pipeline build **on every capability call** — an operator-authorable permanent driver-brick (the AdminUI form has no range validation), violating the parser's explicit never-brick contract (`DriverResilienceOptionsParser.cs:91-95`, `DriverResiliencePipelineBuilder.cs:111-114`, `ResilienceFormModel.cs:32-37`).
|
||
3. **05/STAB-15 (High)** — S7's `IsS7ConnectionFatal` misses framing/desync errors and cancellation-during-I/O (half-read PDU kept → permanent stream desync — the Modbus STAB-3 failure mode reproduced), and the probe catch never marks the handle dead (`S7Driver.cs:1239-1251, 1542`).
|
||
4. **03/S12 = 01/S-8 (Medium, two reviewers independently)** — write dispatch hardcodes `isIdempotent: true`; `WriteIdempotentAttribute` has zero readers, so the moment an operator enables Write retries via #13, **command-shaped writes get retried** (`DriverInstanceActor.cs:597-601`).
|
||
5. **02/S13 (Medium)** — the timeout fix converted a visible hang into an invisible fault: a script that starts failing/timing out never degrades node quality, leaving the last Good value **stale-Good forever** (`VirtualTagActor.cs:123-128`; the dormant engine maps to BadInternalError, the live path doesn't).
|
||
6. **01/U-6 (Medium)** — the resilience **bulkhead is a fully dead knob**: documented, defaulted, parsed, and authorable in AdminUI, but `Build` never adds a concurrency limiter (`DriverResiliencePipelineBuilder.cs:99-177`).
|
||
7. **06/S-11 (Medium)** — an enabled-but-misconfigured `ServerHistorian` still crash-loops the Host with a raw `UriFormatException` (proven live in the #12 rig session; only the config side was fixed) (`HistorianGatewayClientAdapter.cs:45`).
|
||
8. **06/U-7 (Medium)** — the 0.2.0 bump's proto3-optional `HistorianTagDefinition.DataType` change silently alters `GatewayTagProvisioner` semantics (Boolean was wire-0 → provisioned as Float under 0.1.0, honored as Int1 under 0.2.0); pre-bump tags mismatch and the live EnsureTags leg has never run on 0.2.0 (`GatewayTagProvisioner.cs:57`).
|
||
9. **04/C-7 + 04/U-5 + 03/U10 (Medium)** — the resilience authoring/observability surface is half-built now that #10/#13 made it production-live: the AdminUI form round-trip is lossy (malformed JSON silently overwritten, unknown keys stripped), and `DriverResilienceStatusTracker` has **no reader anywhere** (Admin `/hosts` Stream E.2/E.3 never shipped) — operators author live retry/breaker config blind.
|
||
|
||
**Carried open from 2026-07-08 (re-verified, no remediation branch targeted them):**
|
||
|
||
- **Failure-visibility group** — applier swallows sink failures / no failure field in `AddressSpaceApplyOutcome` (01/S-1); Galaxy write success optimistic (06/S-1); primary gate default-allow on unknown role (03/S4). STATUS.md queues these as "next"; still the top carried batch.
|
||
- **04/C-1 (High)** — AdminUI's largest mutating surface still bare `[Authorize]` (GlobalUns, EquipmentPage, cluster/driver editors, Reservations); three-idiom authz mix. Stakes **rose** this round: the ungated surface now includes live resilience config.
|
||
- **Availability** — LDAP block-bridging (03/S2), HistoryRead block-bridging (03/S3), full-rebuild-per-tag-add killing all subscriptions (03/P1 — now unblocked by the U2 forwarding guard, work not started).
|
||
- **Driver fleet** — Modbus timeout/TxId desync (05/STAB-3), AbCip shared-handle race / FOCAS dictionary race, `ResolveHost` mis-keying, dead `onError` sink, `ReadEnum` ×6, silent enum defaulting on TagConfig paths.
|
||
- **Repo hygiene** — 7 AVEVA DLLs in `lib/`, Wonderware husk dirs, tracked planning files, 5 undocumented CLI commands — all untouched (07).
|
||
|
||
---
|
||
|
||
## Cross-cutting themes — updated
|
||
|
||
### 1. "Built-but-never-wired" — the guards work, and the pattern still recurred
|
||
The two systemic guards shipped and are live (analyzer tree-wide with negative control; reflection-exhaustive sink-forwarding guard; whole-solution CI + fail-on-skip). The analyzer immediately earned its keep by surfacing the RESILIENCE-DISPATCH-GAP, which was then genuinely closed (`bacea1a4` — pragmas removed, not suppressed). **But the pattern reproduced inside the remediation itself**: the bulkhead knob parses-but-never-builds (01/U-6), `WriteIdempotentAttribute` has zero readers (01/S-8), and `DriverResilienceStatusTracker` has no reader (03/U10). The guards catch *interface-forwarding* and *unwrapped-dispatch* inertness; they don't catch *option-parsed-but-unapplied* inertness. A cheap next guard: a test asserting every parsed `DriverResilienceOptions` property influences the built pipeline.
|
||
|
||
### 2. Fixes that can't be seen by their own verification
|
||
The round's sharpest lesson: **05/STAB-14 sits exactly in the blind spot of the live test that validated the fix** (bounce = refused; the bug needs timeout). Same genre: the TwinCAT replay is fake-client-only verified (no fixture exists), and 02/S13 turned a loud failure (hang) into a silent one (stale-Good). When a fix targets a fault path, the verification must enumerate the fault *variants* (refused vs timeout vs desync vs cancel), not just one reproduction.
|
||
|
||
### 3. Optimistic success — unchanged, and now with new instances
|
||
The carried trio (applier, Galaxy writes, primary gate) remains open, and the round added stale-Good VT values (02/S13), log-only TwinCAT replay failure (05/STAB-16), and blind-authored resilience config (04/U-5). This is now the largest coherent batch of open work.
|
||
|
||
### 4. The verification system is materially better
|
||
Biggest mover: all 47 test projects gate CI, skips can't masquerade as green (strict on the unit leg), the analyzer is load-bearing, and the failover + forwarding guards exist with negative controls. Remaining: E2E workflow still gates zero tests (07/S-3), integration matrix runs redundant fixtureless legs (07/P-5), driver suites still carry ~100 fixed sleeps now exposed in CI, no bUnit (04/U-1), and behavioral wiring tests cover only 2 of 6 dispatch sites (03/U6).
|
||
|
||
### 5. Documentation drift — mostly closed, one new instance
|
||
CLAUDE.md's Known Limitation 2 and the ScriptAnalysis policy drift are fixed and verified. New drift: `archreview/plans/STATUS.md` merged to master still claiming "all LOCAL — nothing pushed" (corrected alongside this report), and the C6 "Phase 7 plan" residue survived fixdocs.
|
||
|
||
### 6. The resilience seam is the new second-class citizen
|
||
Six of this round's nine notable new findings live on the #10/#13 resilience surface. The seam's architecture is clean (correct Polly-free layering, null objects, actor-context preservation — all verified), but its edges are unfinished: no range validation (S-6), dead bulkhead (U-6), hardcoded idempotency (S-8), lossy form (C-7), no status reader (U-5), stale-pipeline respawn race (S-7/S13-03). One focused hardening pass closes all of them.
|
||
|
||
---
|
||
|
||
## Prioritized action list (updated)
|
||
|
||
| # | Action | Source | Effort | Impact |
|
||
|---|---|---|---|---|
|
||
| 1 | S7: filter connect-timeout OCE in the ensure-wrapper + poll-loop catches; broaden fatal classification (framing/cancel); mark handle dead in probe catch. Add fake-factory connect-timeout test | 05 STAB-14/15 | Small | **Critical-adjacent — regression silently kills S7 subscriptions** |
|
||
| 2 | ResilienceConfig hardening pass: range-validate in parser (clamp + diagnostic, never brick) and AdminUI form; wire or delete the bulkhead knob; read `WriteIdempotentAttribute` (or default writes non-idempotent); fix lossy form round-trip | 01 S-6/U-6/S-8, 04 C-7 | Small-Medium | High — operator-authorable driver-brick; command writes retried |
|
||
| 3 | Publish Bad quality on VT script failure/timeout (match the dormant engine's BadInternalError); optionally gate the apply-boundary cache clear on changed scripts | 02 S13/P7/S12 | Small | High — silent stale-Good data |
|
||
| 4 | Failure-visibility trio: `AddressSpaceApplyOutcome` failure field; Galaxy fail-closed writes; primary-gate default-deny | 01 S-1, 06 S-1, 03 S4 | Medium | High — carried from round 1 |
|
||
| 5 | Gate AdminUI mutating pages with real policies (stakes rose: resilience config is on the ungated surface) | 04 C-1 | Medium | High |
|
||
| 6 | ServerHistorian fail-fast options validation (misconfig → clear startup error, not crash-loop); assess 0.2.0 DataType provisioning delta + run the live EnsureTags leg | 06 S-11/U-7 | Small | Medium-High |
|
||
| 7 | Surgical pure-adds in the applier (prerequisite guard in place since round 1) | 03 P1 | Large | High — biggest operational perf win |
|
||
| 8 | Async LDAP + configurable timeout; channel-ize HistoryRead bridging | 03 S2/S3 | Medium | Medium-High |
|
||
| 9 | Driver fleet carried batch: Modbus fatal-classification, AbCip/FOCAS locking, `ResolveHost`, `onError`, TwinCAT replay retry + Bad-quality | 05 | Medium | Medium-High |
|
||
| 10 | Resilience observability: a `/hosts` reader for `DriverResilienceStatusTracker` (Stream E.2/E.3) | 03 U10, 04 U-5 | Medium | Medium — operators currently blind |
|
||
| 11 | TagConfig parse consolidation (`TagConfigIntent.Parse` in Commons) + `ReadEnum` unification | 01 C-1/P-1, 05 | Medium | Medium |
|
||
| 12 | Hygiene: delete `lib/` AVEVA DLLs + Wonderware husks, untrack planning files, document 5 CLI commands, E2E workflow or delete it, dedupe CI integration matrix | 07 | Small | Medium — license risk + doc trust |
|
||
|
||
## Overall assessment
|
||
|
||
The remediation round was real: all four Criticals are verifiably closed in code (one by premise-correction), both systemic guards are live and already caught one genuine gap, and the verification system went from "proves ~10%" to whole-solution with skip-detection — the single biggest maturity move available, and it happened. The honest counterweight is that this round's scrutiny of the fixes found what fixes always carry: **one High regression (S7 timeout-vs-refused), one High operator-authorable brick (ResilienceConfig validation), and a cluster of half-wired edges on the new resilience seam** — the house "built-but-never-wired" pattern reproducing inside the code meant to eliminate it. Items 1–3 above are small and close everything new-and-sharp from this round; items 4–5 are the carried Highs that have now waited two rounds and should not wait a third.
|