Joseph Doherty
9fadead6a6
Add the 7 per-domain design+implementation plans (archreview/plans/) with an index, produced from the 2026-07-08 architecture review. Fix two confirmed doc drifts the review flagged (theme #5): - CLAUDE.md KNOWN LIMITATION 2: the continuous-historization historized-ref feed IS wired (AddressSpaceApplier.FeedHistorizedRefs -> UpdateHistorizedRefs -> recorder); rewrite to reflect that value-capture is code-complete and only the live end-to-end + restart-convergence verification remains. - CLAUDE.md ScriptAnalysis gating: endpoints use Roles=Administrator,Designer via RequireAuthorization, not the FleetAdmin policy.
122 lines
15 KiB
Markdown
122 lines
15 KiB
Markdown
# OtOpcUa — Deep Architecture Review: Overall Report
|
||
|
||
**Date:** 2026-07-08
|
||
**Commit:** `9cad9ed0` (master, clean tree)
|
||
**Method:** Seven parallel domain reviews, each scoring the same four dimensions (stability, performance, conventions, underdeveloped areas) with severity-rated, file:line-referenced findings. This document is the synthesis; the detail lives in the per-domain reports.
|
||
|
||
## 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)
|
||
|
||
| Subsystem | Stability | Performance | Conventions | Underdeveloped |
|
||
|---|---|---|---|---|
|
||
| 01 Core & composition | 4 | 3 | 4 | 3 |
|
||
| 02 Scripting & alarms | 3.5 | 3 | 4 | **2** |
|
||
| 03 Server & runtime | 3 | 3 | 4 | 3 |
|
||
| 04 AdminUI | 4 | 3 | 3 | 3 |
|
||
| 05 Protocol drivers | 3 | **2** | 3 | 3 |
|
||
| 06 Gateway integrations | 4 | 4 | 4 | 3 |
|
||
| 07 Client & engineering | **2.5** | 3.5 | 3 | 3 |
|
||
|
||
**Read of the matrix:** the system's *design* is strong — conventions score 3–4 everywhere, layering is disciplined (Akka-free OpcUaServer, SDK-free Runtime, clean gateway seams), and there is essentially zero TODO/stub debt. The weaknesses cluster in *operational* stability (failover, reconnect, timeout paths that only fire under fault) and in the *verification system* (CI proves little; distributed and fault-injection behavior is outside the test net). This is the profile of a codebase built by careful review + live-verification rather than by automated gates — it works, but its correctness under failure is largely unproven.
|
||
|
||
---
|
||
|
||
## Critical findings (fix first)
|
||
|
||
These four can each take down or silently corrupt production data flow, and none is caught by any existing test.
|
||
|
||
1. **Split-brain resolver is configured but never activated** — the SBR block exists in `akka.conf:40-46`, but nothing sets `ClusterOptions.SplitBrainResolver` (`src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs:80-84`), so Akka runs with default NoDowning. A hard-crashed node is never downed: **cluster singletons and the driver role-leader never fail over on a hard crash**, and a partition leaves both redundancy sides advertising ServiceLevel 240 indefinitely. Invisible to the 2-node harness because it only exercises graceful stops. *(Report 03, S1)*
|
||
|
||
2. **Production virtual-tag script timeout is ineffective** — `Host/Engines/RoslynVirtualTagEvaluator.cs:102-106` calls `ScriptEvaluator.RunAsync` directly, bypassing `TimedScriptEvaluator` (which exists and is tested). Script execution is synchronous and only checks the token at entry, so one user-authored infinite loop hangs the owning `VirtualTagActor` forever. *(Report 02, U2)*
|
||
|
||
3. **S7 driver has no reconnect path** — the PLC connection is opened only in `InitializeAsync` (`Driver.S7/S7Driver.cs:165-167`); the probe detects the outage but nothing repairs it. A PLC reboot permanently kills the driver instance until redeploy. Zero S7 reconnect tests. *(Report 05)*
|
||
|
||
4. **TwinCAT native ADS subscriptions are orphaned after reconnect** — the default subscription mode is never re-registered on the replacement client after reconnect (`TwinCATDriver.cs:615-619`, `AdsTwinCATClient.cs:422-436`); data silently stops with no Bad status. *(Report 05)*
|
||
|
||
## High-severity findings (system-wide risk ranking)
|
||
|
||
**Data-integrity / failure-visibility group** — the system tends to report success it hasn't earned:
|
||
|
||
- **Deploy applier swallows sink failures**: a deploy whose `RebuildAddressSpace()` threw still reports success to the deploy coordinator; `AddressSpaceApplyOutcome` has no failure field (`AddressSpaceApplier.cs:150-153, 373-383`). *(01, S-1)*
|
||
- **Galaxy write success is optimistic**: empty status arrays translate to Good, and a failed supervisory-advise still lets the write "proceed" though it can never reach the Galaxy — write-outcome self-correction structurally cannot fire for Galaxy (`GatewayGalaxyDataWriter.cs:163-169, 234-243, 281-302`). *(06, S-1)*
|
||
- **Primary gate defaults to allow while role is unknown** (`DriverHostActor.cs:1018-1026`): a booting secondary services field-device writes for up to a 10 s window; compounds with at-most-once DPS delivery. *(03, S4)*
|
||
- **Modbus transport desyncs permanently after a response timeout** — timeout/TxId-mismatch aren't classified socket-fatal, leaving the single-flight stream misaligned (`ModbusTcpTransport.cs:155-165, 233-235`). *(05)*
|
||
- **AbCip races concurrent ops on shared libplctag handles with no lock** (`AbCipDriver.cs:544-575`) — torn values with Good status; AbLegacy guards this exact hazard. FOCAS has the sibling bug on plain-Dictionary caches. *(05)*
|
||
|
||
**Availability / blocking group:**
|
||
|
||
- **LDAP authentication block-bridges OPC UA session activation** with an unconfigurable 10 s connect timeout and no pooling (`OpcUaApplicationHost.cs:271-273`) — an LDAP outage serially stalls SDK threads. *(03, S2)*
|
||
- **HistoryRead block-bridges per node sequentially** on SDK request threads bounded only by the 30 s gateway timeout — large batches × slow historian can exhaust the request pool (`OtOpcUaNodeManager.cs:1902-2203`). *(03, S3)*
|
||
- **Any structural deploy change (one added tag) full-rebuilds the address space and kills every client subscription server-wide** — the surgical path covers only attribute edits/renames. Surgical pure-adds are the highest-leverage performance fix in the repo. *(03, P1)*
|
||
- **Evaluator-cache ALC accretion**: the live path caches script evaluators in a raw `ConcurrentDictionary` never cleared on republish, leaking collectible AssemblyLoadContexts across script edits; the purpose-built `CompiledScriptCache` is unused. *(02, U3)*
|
||
|
||
**Security / authorization group:**
|
||
|
||
- **AdminUI's largest mutating surface is effectively ungated** — GlobalUns, EquipmentPage, ClusterEdit, NodeEdit, all 8 driver pages, AclEdit, and Reservations carry only bare `[Authorize]`: any authenticated user, including read-only, can mutate UNS/driver/ACL config. Authorization is a three-idiom mix (bare / role-string / policy). *(04, #1)*
|
||
|
||
**Verification-system group:**
|
||
|
||
- **CI gates only 5 of 47 test projects** (`.github/workflows/v2-ci.yml:47-52`); Client (388 tests), Tooling, all driver and most Core suites never run in CI. The integration job runs fixture-less, so probes `Assert.Skip` and the badge is green — "skipped" is indistinguishable from "passed". *(07, S-1/S-2)*
|
||
- **The sole custom analyzer (OTOPCUA0001) is referenced by zero consuming projects** — the CapabilityInvoker-wrapping rule it enforces is inert in every build. One `ProjectReference` in `Directory.Build.props` fixes it. *(07, C-1)*
|
||
|
||
---
|
||
|
||
## Cross-cutting themes
|
||
|
||
These emerged independently from multiple reviewers and are the real architectural signal — individual bugs are symptoms of these.
|
||
|
||
### 1. "Built-but-never-wired" is the house failure mode
|
||
At least six independent instances: the dormant `GatewayTagProvisioner` (PR #423, since fixed), the F10b `DeferredAddressSpaceSink` inertness bug (fixed), the analyzer wired to zero projects, `GatewayHistorianDataSource.RefreshConnectionStateAsync` with no production caller (health flags permanently false), the SBR HOCON block with no activating registration, and the entire ~2.4k-line `Core.VirtualTags` engine stack that production bypasses in favor of `VirtualTagHostActor` + `RoslynVirtualTagEvaluator` — which is precisely why the timeout and cache defenses (built into the dormant engine's path) are missing from the live one. **Recommendation:** every hook/capability/config block needs a *production-caller* assertion, not just unit tests of the component — e.g. a reflection-exhaustive forwarding test for `DeferredAddressSpaceSink` (only ~5 of 10 members are asserted today), a startup log line proving SBR is active, and wiring the analyzer repo-wide since it was built to catch exactly this class.
|
||
|
||
### 2. Fixes don't flow back to shared templates
|
||
Every High in the driver fleet is a fix present in one sibling and absent in another: S7 forked the poll loop (gaining backoff the shared `PollGroupEngine` lacks, keeping a CTS race the engine fixed); AbLegacy guards the handle race AbCip has; OpcUaClient's reconnect machine is exemplary while S7 has none; the `ReadEnum` helper is copy-pasted 6×; no driver passes `onError` to the poll engine so its error sink is dead fleet-wide. The same pattern exists in Core as **duplication-by-convention**: TagConfig intent parsing is byte-parity-replicated in four projects, synced only by "MUST parse identically" comments. **Recommendation:** consolidate into `TagConfigIntent.Parse` in Commons (the `EquipmentScriptPaths` precedent) and treat `PollGroupEngine` + a shared reconnect/backoff primitive as the single home for fleet fixes.
|
||
|
||
### 3. Optimistic success — failure signals don't reach operators
|
||
The "Safe*" swallow-and-log wrapper pattern (never fail the caller) recurs across the applier, Galaxy writes, and the primary gate's default-allow. Individually defensible; together they mean a failed deploy reports `rebuilt=true`, a lost Galaxy write reports Good, and a booting secondary accepts writes. **Recommendation:** audit each swallow site for an operator-visible signal (health flag, meter, alert row) — the pattern is fine only when the failure surfaces *somewhere*.
|
||
|
||
### 4. The verification system trusts live-verification that CI can't reproduce
|
||
Repo-wide: no bUnit (razor `@code` logic is only live-verified), CI runs ~10% of suites, integration tier skip-gates to green, DPS delivery/actor supervision/hard-kill failover have zero tests, `OpcUaServer.IntegrationTests` has exactly one test, and driver test debt concentrates on reconnect/concurrency — exactly where the four Criticals live. The project's own history (F10b, redundancy-state delivery, enum serialization) shows live-`/run` catching what 250+ green unit tests missed. **Recommendation:** expand `v2-ci.yml` to all unit suites (cheap, immediate), make skips visible (report skipped counts as a failure condition for the integration job), and add a small fault-injection tier (hard-kill a node, drop a PLC connection) even if only run nightly against the docker host.
|
||
|
||
### 5. Documentation drift on load-bearing claims
|
||
CLAUDE.md's KNOWN LIMITATION 2 (empty historized-ref set) is **stale — closed in code** (applier `FeedHistorizedRefs` → recorder `UpdateHistorizedRefs`), confirmed independently by two reviewers; ScriptAnalysis endpoints are documented as FleetAdmin-gated but use `Roles="Administrator,Designer"`; `docs/Client.CLI.md` documents 8 of 14 commands. **Recommendation:** update CLAUDE.md now (and add the missing restart-convergence test + live value-capture verification that the limitation's closure deserves).
|
||
|
||
### 6. Equipment-tag (TagConfig-JSON) paths are second-class citizens
|
||
Across drivers, the equipment-tag parse/resolve path lags the authored-tag path: silent enum defaulting on typos, forced `Writable:true` (FOCAS is read-only yet defaults writable), `ResolveHost` mis-keying refs to the first device (breaking per-host breaker isolation in 4 drivers), and capability gates bypassed. Since Galaxy-standard-driver made TagConfig the primary authoring surface, this path *is* the product. **Recommendation:** a dedicated hardening pass on the `EquipmentTagRefResolver` seam across all drivers, plus the parse-strictness unification from theme 2.
|
||
|
||
### 7. Repo hygiene debt
|
||
Retired `Historian.Wonderware*` directories still on disk (absent from slnx); 7 committed proprietary AVEVA DLLs in `lib/` referenced by nothing (license/redistribution risk); `pending.md`/`current.md` tracked despite pending.md's own "never stage" rule; five stale planning files at root. (`sql_login.txt` was verified NOT committed — gitignored, local-only.)
|
||
|
||
---
|
||
|
||
## Prioritized action list
|
||
|
||
| # | Action | Source | Effort | Impact |
|
||
|---|---|---|---|---|
|
||
| 1 | Activate the split-brain resolver (`ClusterOptions.SplitBrainResolver`) + a hard-kill failover test | 03 S1 | Small | Critical — failover currently doesn't work |
|
||
| 2 | Route production VT evaluation through `TimedScriptEvaluator` + `CompiledScriptCache` | 02 U2/U3 | Small | Critical — user script can hang an actor; ALC leak |
|
||
| 3 | Add S7 reconnect + TwinCAT ADS re-registration after reconnect (use OpcUaClient's machine as the template) | 05 | Medium | Critical — PLC power-cycle currently defeats both |
|
||
| 4 | Classify Modbus timeout/TxId-mismatch as socket-fatal; lock AbCip handles (copy AbLegacy's guard) | 05 | Small | High — silent data corruption/desync |
|
||
| 5 | Gate AdminUI mutating pages with real policies (one authz idiom, constants not literals) | 04 | Medium | High — any authenticated user can mutate config |
|
||
| 6 | Wire OTOPCUA0001 analyzer repo-wide; expand CI to all unit suites; fail-on-skip for integration | 07 | Small | High — restores meaning to green CI |
|
||
| 7 | Add a failure field to `AddressSpaceApplyOutcome`; surface Galaxy write-loss and gate default-deny on unknown role | 01/06/03 | Medium | High — failure visibility |
|
||
| 8 | Surgical pure-adds in the address-space applier (stop full rebuilds killing all subscriptions per tag add) | 03 P1 | Large | High — biggest operational perf win |
|
||
| 9 | Async LDAP + configurable timeout; channel-ize HistoryRead bridging | 03 S2/S3 | Medium | Medium-High — outage resilience |
|
||
| 10 | Consolidate TagConfig parsing (`TagConfigIntent.Parse` in Commons) + driver `ReadEnum`/parse-strictness unification + `ResolveHost` fix | 01 C-1 / 05 | Medium | Medium — kills the byte-parity duplication class |
|
||
| 11 | Update CLAUDE.md (Known Limitation 2 closed; ScriptAnalysis policy) + Client.CLI docs; delete Wonderware dirs, `lib/` AVEVA DLLs, untrack planning files | 03/04/06/07 | Small | Medium — doc trust + license risk |
|
||
| 12 | Retire or converge the dormant `Core.VirtualTags` engine; delete `MemoryRecycle`/`IDriverSupervisor` dead machinery | 02 U1 / 01 U-2 | Medium | Medium — removes the built-vs-runs divergence |
|
||
|
||
## Overall assessment
|
||
|
||
OtOpcUa is a well-architected system with unusually disciplined conventions, clean layering, strong seams, and near-zero TODO debt — the design work is mature. Its exposure is concentrated in three places: **fault paths** (failover, reconnect, timeout — where all four Criticals live and where test coverage is thinnest), **failure visibility** (a systematic bias toward optimistic success), and **the verification gap** (CI proves ~10% of what the test suite could; the "built-but-never-wired" pattern has now recurred six times because nothing asserts production wiring). The prioritized list above front-loads small, high-leverage fixes: items 1–6 are roughly a week of work and eliminate every Critical plus the two systemic guards (analyzer + CI) that prevent the pattern from recurring.
|