docs(archreview): 2026-07-12 re-review at f6eaa267 + 12 round-2 remediation plans
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.
This commit is contained in:
Joseph Doherty
+345
-164
@@ -1,22 +1,69 @@
|
||||
# Architecture Review — Core Composition Pipeline and Core Libraries
|
||||
|
||||
**Date:** 2026-07-08
|
||||
**Commit:** `9cad9ed0` (master)
|
||||
**Date:** 2026-07-12
|
||||
**Commit:** `f6eaa267` (master, clean tree)
|
||||
**Note:** This updates the 2026-07-08 review at `9cad9ed0`. Since that baseline the arch-review
|
||||
remediation branches merged to master; the pieces landing in this domain are: the
|
||||
`IDriverCapabilityInvoker`/`IDriverCapabilityInvokerFactory` seam in Core.Abstractions +
|
||||
`DriverCapabilityInvokerFactory` in Core (`bacea1a4`, task #10), per-instance ResilienceConfig
|
||||
read-path plumbing through the deploy artifact (`75403caa`, task #13), the reflection-exhaustive
|
||||
deferred-sink forwarding guard in Commons.Tests (`a65c2ced`, task #6), the typed SBR option in
|
||||
Cluster (`a81dea10` + premise correction `eaf78aad`), the OTOPCUA0001 analyzer wiring (`f0082af5`),
|
||||
`IScriptCacheOwner` in Commons (`7fd44f0f`), and a fixdocs XML-doc sweep (`3b5ef439`).
|
||||
|
||||
**Reviewer scope:**
|
||||
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Core` (driver hosting, resilience, stability, authorization trie)
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions` (driver capability interfaces, `PollGroupEngine`, historian seams)
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions` (driver capability interfaces, `PollGroupEngine`, historian seams, the new invoker seam)
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Commons` (cluster message contracts, deferred sinks, NodeId scheme, telemetry)
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Configuration` (EF config persistence, generation-sealed local cache, draft validation)
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Cluster` (Akka bootstrap, role info, ServiceLevel)
|
||||
- `src/Core/ZB.MOM.WW.OtOpcUa.Cluster` (Akka bootstrap, role info, ServiceLevel, SBR options)
|
||||
- Plus the address-space composition pipeline itself — `AddressSpaceComposer` / `AddressSpacePlanner` / `AddressSpaceApplier`
|
||||
|
||||
**Scope note (location drift).** The review brief places the composition pipeline in
|
||||
`ZB.MOM.WW.OtOpcUa.Core`, but since the Phase7→AddressSpace rename it lives in
|
||||
`src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/` (`AddressSpaceComposer.cs`, `AddressSpacePlan.cs`,
|
||||
`AddressSpaceApplier.cs`), with its tests under `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/`.
|
||||
The `Core` project's own `OpcUa/` folder now holds only the retired `EquipmentNodeWalker` and an
|
||||
unused `GenericDriverNodeManager` (see U-1). The pipeline was reviewed in full regardless.
|
||||
The `Core` project's own `OpcUa/` folder still holds only the retired `EquipmentNodeWalker` and an
|
||||
unused `GenericDriverNodeManager` (see U-1). The pipeline was reviewed in full regardless. The
|
||||
composition pipeline itself received **zero code changes** in the 9cad9ed0→f6eaa267 window; the
|
||||
domain's delta is concentrated in `Core/Resilience`, `Core.Abstractions`, `Cluster`, and the Runtime
|
||||
dispatch layer that consumes the new seam.
|
||||
|
||||
---
|
||||
|
||||
## Prior-finding status (9cad9ed0 → f6eaa267)
|
||||
|
||||
Every prior finding was re-verified against the current code, not the STATUS.md claims.
|
||||
|
||||
| ID | One-line description | Verified status |
|
||||
|---|---|---|
|
||||
| S-1 | Applier swallows every sink failure; deploy reported applied even when broken | **STILL OPEN** — `Safe*` helpers unchanged (`AddressSpaceApplier.cs:373–383, 612–622`); `AddressSpaceApplyOutcome` (`:691`) still has no failure fields; `rebuilt = true` still set after `SafeRebuild()` (`:148–153`). STATUS.md itself lists 01/S-1 under "Suggested next". |
|
||||
| S-2 | `PollGroupEngine.Unsubscribe` blocks up to 5 s per subscription | **STILL OPEN** — `task.Wait(TimeSpan.FromSeconds(5))` at `PollGroupEngine.cs:109`. |
|
||||
| S-3 | No disposed guard: `Subscribe`/`RegisterAsync` race `DisposeAsync` | **STILL OPEN** — no `_disposed` flag in `PollGroupEngine.cs` or `Core/Hosting/DriverHost.cs`. |
|
||||
| S-4 | `ScheduledRecycleScheduler` catch-up recycle storm after a stall | **STILL OPEN** — `_nextRecycleUtc += _recycleInterval` (`ScheduledRecycleScheduler.cs:82`), no fast-forward past `utcNow`. |
|
||||
| S-5 | `GenericDriverNodeManager` re-walk teardown unsynchronized | **STILL OPEN (moot)** — code unchanged; the #10 triage added an OTOPCUA0001 pragma (`GenericDriverNodeManager.cs:69`) explicitly memorializing "zero production references", strengthening the U-1 delete case. |
|
||||
| P-1 | `Compose` parses each tag's TagConfig JSON four times | **STILL OPEN** — four `JsonDocument.Parse` sites unchanged (`AddressSpaceComposer.cs:541, 606, 638, 670` from `:418–433`). |
|
||||
| P-2 | Per-operation allocations on the authorization hot path | **STILL OPEN** — `ldapGroups.ToHashSet(...)` per call at `PermissionTrie.cs:42`. |
|
||||
| P-3 | `PollGroupEngine`: task-per-subscription, no coalescing | **STILL OPEN** — engine unchanged. |
|
||||
| P-4 | Per-write allocation in the non-idempotent write path | **STILL OPEN** — options record + `Dictionary` per call at `CapabilityInvoker.cs:117–131` (and now production-unreachable — see S-8). |
|
||||
| C-1 | Byte-parity TagConfig parsing replicated in four projects | **STILL OPEN** — all four copies + "MUST parse identically" comments intact; #13 added a fifth small parity-coupled read (`DeploymentArtifact.TryReadSpec` ResilienceConfig) though that one is a trivial string read. |
|
||||
| C-2 | Core depends on Configuration; EF entities are the domain model | **STILL OPEN (accepted-risk)** — `Core.csproj:16` still references Configuration. |
|
||||
| C-3 | `DraftValidator` re-derives NodeId scheme + hard-codes `"GalaxyMxGateway"` | **STILL OPEN** — `DraftValidator.cs:48, 65, 75–97` unchanged. |
|
||||
| C-4 | Composer violates purity contract with `Trace.TraceWarning` | **STILL OPEN** — `AddressSpaceComposer.cs:493`. |
|
||||
| C-5 | Inconsistent duplicate-key defensiveness in `Compose` | **STILL OPEN** — last-wins `deviceHostById` (`:357–363`) vs throw-on-dupe `ToDictionary` (`:401–402, 453`). |
|
||||
| C-6 | Vestigial `Akka` package reference in Commons | **STILL OPEN** — `Commons.csproj:9`. |
|
||||
| C-7 | Stale scaffold-era XML docs on load-bearing classes | **PARTIALLY FIXED** — fixdocs `3b5ef439` converted `CapabilityInvoker`'s duplicated docs to `<inheritdoc/>`, but both flagged narratives survive verbatim: the applier's F14-scaffold doc ("For now we record the work", "the SDK adapter that lands in F10b will decide…" — `AddressSpaceApplier.cs:7–26`) and `DriverHost`'s "Phase 2 via named-pipe RPC" promise (`Core/Hosting/DriverHost.cs:5–10`). |
|
||||
| U-1 | Dead/dormant code retained: `GenericDriverNodeManager`, `EquipmentNodeWalker`, `TryParseRelayBody` | **STILL OPEN** — all three present (`Core/OpcUa/` lists both classes + `IdentificationFolderBuilder`; `EquipmentScriptPaths.cs:164`). Evidence grew: the analyzer triage now documents GDNM as test-only scaffolding in a pragma. |
|
||||
| U-2 | Tier C recycle machinery has no `IDriverSupervisor` implementation | **STILL OPEN** — repo-wide search still finds zero concrete implementations; the new `DriverResilienceOptionsParser` even validates `RecycleIntervalSeconds` as "Tier C only" for a tier that cannot execute (`DriverResilienceOptionsParser.cs:101–110`). |
|
||||
| U-3 | Continuous-historization initial interest set empty at spawn | **PARTIALLY FIXED / RE-ANALYSED** — the recorder is still spawned with `Array.Empty<string>()` (`Runtime/ServiceCollectionExtensions.cs:277`), but re-analysis shows restart convergence is mechanically present: `DriverHostActor.RestoreApplied` (`DriverHostActor.cs:1293–1320`) fires `RebuildAddressSpace` on bootstrap, `OpcUaPublishActor.HandleRebuild` diffs against an **empty** `_lastApplied` baseline → a full-add plan → `Apply` → `FeedHistorizedRefs` posts the complete set. The prior claim "historizes nothing until the next deploy" was too strong. What remains is exactly CLAUDE.md Known Limitation 2 (reworded by the remediation docs branch): no restart-convergence test and no live end-to-end verification. Kept in the body, downgraded. |
|
||||
| U-4 | Additive-only ACL model (no Deny) | **STILL OPEN (documented v2.0 gap)** — `PermissionTrie.cs:13–17` unchanged. |
|
||||
| U-5 | Test coverage strong but unevenly distributed | **PARTIALLY IMPROVED** — new suites landed: `SplitBrainResolverActivationTests` (Cluster.Tests, now 4 files), `DriverCapabilityInvokerFactoryTests` (148 lines), pipeline-builder logging tests, and the Commons `DeferredSinkForwardingReflectionTests` guard. But `ClusterRoleInfo` — the only concurrency-bearing Cluster class — still has zero tests, and the S-1/S-3 surfaces remain untestable because they don't exist. Kept in the body, narrowed. |
|
||||
|
||||
Net: **0 of the 5 stability findings, 0 of the 4 performance findings, and 6 of the 7 convention
|
||||
findings are unchanged**; the remediation wave targeted other domains' Criticals and *landed new
|
||||
code in this domain* rather than fixing this domain's backlog. That new code is reviewed below
|
||||
(S-6/S-7/S-8, U-6, C-8).
|
||||
|
||||
---
|
||||
|
||||
@@ -24,7 +71,7 @@ unused `GenericDriverNodeManager` (see U-1). The pipeline was reviewed in full r
|
||||
|
||||
### Composition pipeline (deploy → address space)
|
||||
|
||||
The pipeline is a clean three-stage compose/diff/apply design:
|
||||
The pipeline is a clean three-stage compose/diff/apply design (unchanged this window):
|
||||
|
||||
1. **`AddressSpaceComposer.Compose`** (`OpcUaServer/AddressSpaceComposer.cs:290`) — a pure static
|
||||
projection from EF config entities (`UnsArea`, `UnsLine`, `Equipment`, `DriverInstance`, `Tag`,
|
||||
@@ -60,35 +107,63 @@ The pipeline is a clean three-stage compose/diff/apply design:
|
||||
The sink boundary (`Commons.OpcUa.IOpcUaAddressSpaceSink`) keeps the whole pipeline SDK-free:
|
||||
production binds `SdkAddressSpaceSink` via the late-swap `DeferredAddressSpaceSink` (actors resolve
|
||||
the wrapper at DI time, the OPC UA hosted service swaps the real sink in after `StandardServer`
|
||||
starts); dev/tests bind the `NullOpcUaAddressSpaceSink` no-op.
|
||||
starts); dev/tests bind the `NullOpcUaAddressSpaceSink` no-op. The forwarding contract of that
|
||||
wrapper is now guarded by a reflection-exhaustive test (`Commons.Tests/OpcUa/
|
||||
DeferredSinkForwardingReflectionTests.cs`) — see C-8 for its one blind spot.
|
||||
|
||||
### The new resilience dispatch seam (landed this window)
|
||||
|
||||
The Phase 6.1 `CapabilityInvoker`, which the 2026-07-08 review cycle found built-but-never-wired
|
||||
(RESILIENCE-DISPATCH-GAP), is now live in production dispatch via a Polly-free seam that mirrors
|
||||
`IDriverFactory`:
|
||||
|
||||
- **`Core.Abstractions/IDriverCapabilityInvoker.cs`** — the invoker abstraction (3 methods matching
|
||||
`CapabilityInvoker`'s shapes exactly) plus `NullDriverCapabilityInvoker`, a genuine pass-through.
|
||||
- **`Core.Abstractions/IDriverCapabilityInvokerFactory.cs`** — per-instance factory abstraction plus
|
||||
the `Null` factory.
|
||||
- **`Core/Resilience/DriverCapabilityInvokerFactory.cs`** — the concrete factory: resolves the
|
||||
driver-type tier, layers per-instance `ResilienceConfig` JSON on the tier defaults via
|
||||
`DriverResilienceOptionsParser.ParseOrDefaults` (logging the parse diagnostic), calls
|
||||
`builder.Invalidate(instanceId)` so a respawn with changed options rebuilds cached pipelines, and
|
||||
snapshots the options once per `Create` so the per-call accessor is allocation-free.
|
||||
- The Host's `DriverFactoryBootstrap` registers the factory (`Host/Drivers/DriverFactoryBootstrap.cs:63–70`),
|
||||
Runtime resolves it like `IDriverFactory` (`Runtime/ServiceCollectionExtensions.cs:365–366`),
|
||||
`DriverHostActor.SpawnChild` creates one invoker per child (`DriverHostActor.cs:1630`), and
|
||||
`DriverInstanceActor` wraps its six dispatch sites. `DriverSpawnPlanner` forces a respawn on any
|
||||
ResilienceConfig change (`DriverSpawnPlan.cs:40–46`); `DeploymentArtifact.TryReadSpec` reads the
|
||||
column null-safely from the artifact (`DeploymentArtifact.cs:168–171`). The OTOPCUA0001 analyzer
|
||||
is wired tree-wide and knows the interface as a wrapper home, so an unwrapped capability call is a
|
||||
build error.
|
||||
|
||||
The layering is genuinely clean — Runtime stays Polly-free, null objects keep every test harness
|
||||
byte-identical, and the invalidate/respawn pairing closes the options-blind-cache trap. The new
|
||||
findings below (S-6, S-7, S-8, U-6) are all second-order issues *inside* this new machinery, not
|
||||
problems with its architecture.
|
||||
|
||||
### Supporting Core libraries
|
||||
|
||||
- **`Core/Hosting`** — `DriverFactoryRegistry` (driver-type → factory), `DriverFactoryRegistryAdapter`
|
||||
(v1 registry → v2 `IDriverFactory`), `DriverHost` (id → `IDriver` lifecycle registry).
|
||||
- **`Core/Hosting`** — `DriverFactoryRegistry` (driver-type → factory + `GetTier`),
|
||||
`DriverFactoryRegistryAdapter`, `DriverHost` (id → `IDriver` lifecycle registry).
|
||||
- **`Core/Resilience`** — Polly pipelines cached per `(DriverInstanceId, HostName, Capability)`
|
||||
(`DriverResiliencePipelineBuilder`), executed via `CapabilityInvoker` (with the
|
||||
non-idempotent-write no-retry override), fanned out per host by `AlarmSurfaceInvoker`, observed
|
||||
by `DriverResilienceStatusTracker` for Admin `/hosts`.
|
||||
- **`Core/Stability`** — `MemoryTracking` (median-baseline soft/hard breach), `MemoryRecycle` and
|
||||
`ScheduledRecycleScheduler` (Tier C recycle via `IDriverSupervisor`), `WedgeDetector`
|
||||
(demand-aware stall detection).
|
||||
(`DriverResiliencePipelineBuilder`, now with retry/breaker event logging as the interim
|
||||
observability surface), executed via `CapabilityInvoker` (with the non-idempotent-write no-retry
|
||||
override), built per instance by `DriverCapabilityInvokerFactory`, fanned out per host by
|
||||
`AlarmSurfaceInvoker`, observed by `DriverResilienceStatusTracker` for Admin `/hosts`.
|
||||
- **`Core/Stability`** — `MemoryTracking`, `MemoryRecycle` and `ScheduledRecycleScheduler`
|
||||
(Tier C recycle via `IDriverSupervisor`), `WedgeDetector`.
|
||||
- **`Core/Authorization`** — generation-sealed `PermissionTrie` per `(ClusterId, GenerationId)`
|
||||
cached in `PermissionTrieCache` (CAS-pruned), walked by `TriePermissionEvaluator` against
|
||||
per-session `UserAuthorizationState` (freshness window + fail-closed staleness ceiling).
|
||||
- **`Commons`** — immutable record message contracts for the Akka DPS topics (deploy dispatch/ack,
|
||||
alerts `AlarmTransitionEvent`, redundancy state, admin ops, script log), strongly-typed ids
|
||||
(`DeploymentId`, `RevisionHash`, `CorrelationId`, `NodeId`), the deferred sink/publisher pair,
|
||||
`EquipmentScriptPaths` (shared script-path/dependency extraction), and `OtOpcUaTelemetry`
|
||||
(central Meter/ActivitySource).
|
||||
- **`Configuration`** — `OtOpcUaConfigDbContext` (26 entities, logical-id unique indexes, JSON
|
||||
check constraints, RowVersion concurrency), the generation-sealed LiteDB fallback cache
|
||||
(`GenerationSealedCache` + atomic `CURRENT` pointer, fail-closed on corruption),
|
||||
`ResilientConfigReader` (timeout→retry→cache-fallback with secret-scrubbed logging),
|
||||
`DraftValidator` (all-errors-in-one-pass pre-publish rules).
|
||||
- **`Cluster`** — `AddOtOpcUaCluster` / `WithOtOpcUaClusterBootstrap` (Akka.Hosting bootstrap with
|
||||
embedded HOCON + Serilog logger wiring), `ClusterRoleInfo` (lock-guarded role topology snapshot
|
||||
fed by a subscriber actor), `ServiceLevelCalculator` (pure 0–255 tiering), `RoleParser`.
|
||||
per-session `UserAuthorizationState`.
|
||||
- **`Commons`** — immutable record message contracts for the Akka DPS topics, strongly-typed ids,
|
||||
the deferred sink/publisher pair, `EquipmentScriptPaths`, `OtOpcUaTelemetry`, and now
|
||||
`Engines/IScriptCacheOwner` (the apply-boundary compile-cache drop capability for
|
||||
`IVirtualTagEvaluator` implementations).
|
||||
- **`Configuration`** — `OtOpcUaConfigDbContext` (26 entities), the generation-sealed LiteDB
|
||||
fallback cache, `ResilientConfigReader`, `DraftValidator`.
|
||||
- **`Cluster`** — `AddOtOpcUaCluster` / `WithOtOpcUaClusterBootstrap` (now with the extracted,
|
||||
testable `BuildClusterOptions` setting `KeepOldestOption { DownIfAlone = true }` explicitly — the
|
||||
#11 premise correction is accurately reflected in the akka.conf comment, the XML doc, and
|
||||
`SplitBrainResolverActivationTests`), `ClusterRoleInfo`, `ServiceLevelCalculator`, `RoleParser`.
|
||||
|
||||
Dependency direction: `Core.Abstractions` (leaf) ← `Configuration` ← `Core`; `Commons` (leaf,
|
||||
Akka+Audit packages) ← `Cluster`. The composer/applier in `OpcUaServer` reference both
|
||||
@@ -100,27 +175,28 @@ Akka+Audit packages) ← `Cluster`. The composer/applier in `OpcUaServer` refere
|
||||
|
||||
### 1. Stability
|
||||
|
||||
#### S-1 — Applier swallows every sink failure; a deploy is reported applied even when the address space is broken — **High**
|
||||
#### S-1 — Applier swallows every sink failure; a deploy is reported applied even when the address space is broken — **High** *(unchanged since 2026-07-08)*
|
||||
|
||||
`AddressSpaceApplier` wraps every sink call in catch-all "Safe" helpers that log and continue:
|
||||
`SafeEnsureFolder`/`SafeEnsureVariable` (`AddressSpaceApplier.cs:612–622`), `SafeRebuild`
|
||||
(`AddressSpaceApplier.cs:373–383`), `SafeWriteAlarmCondition`/`SafeMaterialiseAlarmCondition`
|
||||
(`AddressSpaceApplier.cs:677–687`). `AddressSpaceApplyOutcome` (`AddressSpaceApplier.cs:691`)
|
||||
carries only Added/Removed/Changed counts and `RebuildCalled` — **no failure count**. If
|
||||
`RebuildAddressSpace()` throws, `rebuilt` is still reported `true` (`AddressSpaceApplier.cs:150–153`)
|
||||
`RebuildAddressSpace()` throws, `rebuilt` is still reported `true` (`AddressSpaceApplier.cs:148–153`)
|
||||
and the outcome flows back to the deploy coordinator as a success; the deployment seals while the
|
||||
running server holds a partially-materialised (or entirely stale) address space. Individual
|
||||
`EnsureVariable` failures during a materialise pass likewise vanish into per-node Warnings.
|
||||
|
||||
The never-fail-a-deploy posture is defensible for the *detached* hooks (provisioning, historized-ref
|
||||
feed — both correctly isolated), but structural materialisation failures are the deploy's core
|
||||
contract.
|
||||
contract. This is the domain's highest-priority open item; STATUS.md already queues it as a
|
||||
"failure-visibility High".
|
||||
|
||||
**Recommendation:** add `FailedNodes`/`RebuildFailed` to `AddressSpaceApplyOutcome`, propagate into
|
||||
the `ApplyAck`/`DeploymentFailed` decision and the audit log, and escalate `SafeRebuild` failure to
|
||||
at least a degraded ack. Tests should assert the failure surface (the fixture sink can throw).
|
||||
|
||||
#### S-2 — `PollGroupEngine.Unsubscribe` blocks the calling thread up to 5 s per subscription — **Medium**
|
||||
#### S-2 — `PollGroupEngine.Unsubscribe` blocks the calling thread up to 5 s per subscription — **Medium** *(unchanged)*
|
||||
|
||||
`StopState` does a synchronous `task.Wait(TimeSpan.FromSeconds(5))`
|
||||
(`Core.Abstractions/PollGroupEngine.cs:99–112`). `Unsubscribe` is the teardown path drivers call
|
||||
@@ -134,45 +210,114 @@ not.
|
||||
background drain), keeping the "no callback after teardown" guarantee via the existing
|
||||
LoopTask/CTS-dispose ordering.
|
||||
|
||||
#### S-3 — `PollGroupEngine.Subscribe` has no disposed guard — **Low**
|
||||
#### S-3 — `PollGroupEngine.Subscribe` has no disposed guard — **Low** *(unchanged)*
|
||||
|
||||
`Subscribe` racing `DisposeAsync` can insert a fresh `SubscriptionState` after the dispose loop has
|
||||
snapshotted `_subscriptions.Values` (`PollGroupEngine.cs:73–84` vs `213–236`), leaking a live poll
|
||||
loop with no owner. Same pattern in `DriverHost.RegisterAsync` vs `DisposeAsync`
|
||||
(`Core/Hosting/DriverHost.cs:53–71` vs `92–106`): a registration that wins the race after the
|
||||
snapshot+clear is initialized but never shut down. Both are shutdown-window races, unlikely in
|
||||
(`Core/Hosting/DriverHost.cs:53–71` vs `92–106`). Both are shutdown-window races, unlikely in
|
||||
practice but cheap to close with a `_disposed` flag checked inside the lock / before insert.
|
||||
|
||||
#### S-4 — `ScheduledRecycleScheduler` catch-up recycle storm after a stall — **Low**
|
||||
#### S-4 — `ScheduledRecycleScheduler` catch-up recycle storm after a stall — **Low** *(unchanged)*
|
||||
|
||||
`TickAsync` advances `_nextRecycleUtc` by exactly one interval per fire
|
||||
(`Core/Stability/ScheduledRecycleScheduler.cs:72–84`). If the host was suspended (VM pause, long
|
||||
(`Core/Stability/ScheduledRecycleScheduler.cs:74–82`). If the host was suspended (VM pause, long
|
||||
outage) across K intervals, the next K ticks each trigger a full Tier C process recycle
|
||||
back-to-back. **Recommendation:** fast-forward `_nextRecycleUtc` past `utcNow` after a fire.
|
||||
|
||||
#### S-5 — `GenericDriverNodeManager` re-walk teardown is not synchronized — **Low**
|
||||
#### S-5 — `GenericDriverNodeManager` re-walk teardown is not synchronized — **Low** *(unchanged; dormancy now formally documented)*
|
||||
|
||||
`BuildAddressSpaceAsync` tears down the previous alarm forwarder and clears `_alarmSinks`
|
||||
non-atomically (`Core/OpcUa/GenericDriverNodeManager.cs:58–71`); a concurrent driver alarm event
|
||||
during the window between unsubscribe and re-subscribe is dropped, and two concurrent Build calls
|
||||
would interleave badly. Currently moot because the class has no production caller (see U-1) — fold
|
||||
this into whatever decision is made there.
|
||||
non-atomically (`Core/OpcUa/GenericDriverNodeManager.cs:58–73`). Still moot: the #10 analyzer triage
|
||||
added a pragma at `GenericDriverNodeManager.cs:69` stating outright that this class has "zero
|
||||
production references" and is exercised only by its own tests — fold this into the U-1 decision.
|
||||
|
||||
**Positive observations.** The failure-mode engineering in `Configuration/LocalCache` is exemplary:
|
||||
`GenerationSealedCache` writes temp-file + atomic `File.Replace` pointer swaps, fails closed on any
|
||||
corruption rather than silently serving an older generation (`GenerationSealedCache.cs:111–155`);
|
||||
`LiteDbConfigCache` documents and defuses two real LiteDB concurrency traps (global `BsonMapper`
|
||||
races, find-then-insert upsert races) (`LiteDbConfigCache.cs:16–36`). `ResilientConfigReader`
|
||||
correctly distinguishes SQL-command `TaskCanceledException` from genuine caller cancellation
|
||||
(`ResilientConfigReader.cs:117–138`) and scrubs connection-string secrets from log messages.
|
||||
`PermissionTrieCache.Prune` is a correct reference-equality CAS loop (`PermissionTrieCache.cs:73–102`).
|
||||
The applier's two historian hooks are properly detached (continuation observes the task, never
|
||||
`.Result` on a fault — `AddressSpaceApplier.cs:259–286`). `PollGroupEngine` classifies fatal
|
||||
exceptions and hardens the `onError` callback (`PollGroupEngine.cs:153–169`).
|
||||
#### S-6 — NEW: a valid-JSON `ResilienceConfig` with an out-of-range timeout permanently fails every guarded capability call — violates the parser's own never-brick contract — **High**
|
||||
|
||||
`DriverResilienceOptionsParser.ParseOrDefaults` defends against *malformed JSON* (`catch
|
||||
(JsonException)` at `DriverResilienceOptionsParser.cs:71–76`), unknown capability names, and a
|
||||
misapplied Tier-C recycle interval — but performs **no range validation** on the numeric override
|
||||
values it merges (`DriverResilienceOptionsParser.cs:91–95`): `TimeoutSeconds`, `RetryCount`, and
|
||||
`BreakerFailureThreshold` pass straight through. The pipeline builder then does
|
||||
`builder.AddTimeout(new TimeoutStrategyOptions { Timeout = TimeSpan.FromSeconds(policy.TimeoutSeconds) })`
|
||||
(`DriverResiliencePipelineBuilder.cs:111–114`). Polly v8 range-validates `TimeoutStrategyOptions.Timeout`
|
||||
(and `RetryStrategyOptions.MaxRetryAttempts`) at pipeline build time — so `"timeoutSeconds": 0`, a
|
||||
negative value, or an absurdly large one throws a `ValidationException` inside the
|
||||
`_pipelines.GetOrAdd` factory on the driver's **first capability call**, and on **every subsequent
|
||||
call** (a throwing factory caches nothing). Result: subscribe, discover, write, and alarm-ack all
|
||||
fail persistently for that driver while the driver itself is healthy — precisely the "misconfigured
|
||||
ResilienceConfig should never brick a working driver" outcome the parser's remarks
|
||||
(`DriverResilienceOptionsParser.cs:30–33`) promise to prevent.
|
||||
|
||||
This is operator-reachable: the AdminUI's `ResilienceFormModel` binds `TimeoutSeconds` as a bare
|
||||
`int?` with **no range validation anywhere in the form model**
|
||||
(`AdminUI/Components/Shared/Drivers/ResilienceFormModel.cs:32–37`), so typing `0` into the timeout
|
||||
field, publishing, and deploying produces the failure. And because #13 correctly forces a respawn on
|
||||
any ResilienceConfig change, the bad config takes effect immediately.
|
||||
|
||||
**Recommendation:** clamp/reject out-of-range values in `ParseOrDefaults` (the diagnostic
|
||||
out-parameter already exists — `TimeoutSeconds < 1 → tier default + diagnostic`, cap at Polly's max;
|
||||
`RetryCount < 0 or > 100 → clamp`), add a parser test per field, and mirror the same bounds as
|
||||
form-side validation in `ResilienceFormModel`. Defense in depth belongs in the parser: the JSON
|
||||
column is writable by more paths than the AdminUI form.
|
||||
|
||||
#### S-7 — NEW: respawn `Invalidate` race can permanently re-cache a stale-options pipeline — **Low**
|
||||
|
||||
`DriverCapabilityInvokerFactory.Create` calls `_builder.Invalidate(driverInstanceId)`
|
||||
(`DriverCapabilityInvokerFactory.cs:71`) before returning the new invoker, and `DriverHostActor`
|
||||
processes `plan.ToStop` before `plan.ToSpawn` in the same handler (`DriverHostActor.cs:1278–1280`)
|
||||
— but `StopChild` is an asynchronous actor stop. An in-flight capability call still executing inside
|
||||
the *old* child's handler can reach `GetOrCreate` **after** the new spawn's `Invalidate` has run,
|
||||
re-populating the cache with a pipeline built from the **old** options snapshot. Because the builder
|
||||
deliberately ignores options on a cache hit (`DriverResiliencePipelineBuilder.cs:66–79`), the new
|
||||
invoker then serves that stale pipeline **indefinitely** — the transient race has a permanent
|
||||
consequence, silently undoing the operator's ResilienceConfig change for the affected
|
||||
(host, capability) keys. The window is small (one in-flight handler) but the failure is invisible.
|
||||
|
||||
**Recommendation:** make staleness impossible rather than racing it — include an options generation
|
||||
(or the options reference) in `PipelineKey`, or have `Create` capture a per-invoker generation the
|
||||
old invoker can no longer write into. Alternatively, defer the new spawn until the old child's
|
||||
`Terminated` is observed (the actor-name generation counter shows the respawn already tolerates the
|
||||
old child lingering).
|
||||
|
||||
#### S-8 — NEW: production dispatch hardcodes `isIdempotent: true`; the non-idempotent write safeguard is unreachable — **Medium**
|
||||
|
||||
The only production call to `ExecuteWriteAsync` is `DriverInstanceActor.HandleWriteAsync` passing
|
||||
`isIdempotent: true` unconditionally (`Runtime/Drivers/DriverInstanceActor.cs:597–601`); a
|
||||
repo-wide search finds no caller passing `false`. The `!isIdempotent` no-retry branch in
|
||||
`CapabilityInvoker.ExecuteWriteAsync` (`CapabilityInvoker.cs:117–131`) — the mechanism Phase 6.1
|
||||
built specifically so non-idempotent device writes are never retried — is production-dead code. The
|
||||
inline comment's reasoning ("an OPC UA attribute set is idempotent") holds for plain value writes,
|
||||
but drivers surface write-through targets where a duplicated write is *not* harmless
|
||||
(command/pulse coils, increment registers, Galaxy supervisory writes), and the
|
||||
`WriteIdempotentAttribute` the invoker was designed around is never consulted at the seam. Today
|
||||
this is behavior-neutral (every tier's Write default is `RetryCount: 0` —
|
||||
`DriverResilienceOptions.cs:83, 94, 105`), but the entire point of #13 is that an operator can now
|
||||
switch Write retries on per instance — and the moment one does, **all** of that driver's writes
|
||||
retry, with the no-retry safety valve unreachable.
|
||||
|
||||
**Recommendation:** either thread the tag-level idempotence signal (from `WriteIdempotentAttribute`
|
||||
/ driver metadata) through `WriteAttribute` to the dispatch site, or — if all-writes-idempotent is
|
||||
the deliberate v1 posture — delete the `isIdempotent` parameter from the seam and document the
|
||||
invariant, so the dead branch (and its per-write allocation, P-4) stops implying a guarantee that
|
||||
isn't wired.
|
||||
|
||||
**Positive observations.** The failure-mode engineering in `Configuration/LocalCache` remains
|
||||
exemplary (fail-closed `GenerationSealedCache`, atomic pointer swaps, the documented LiteDB traps);
|
||||
`ResilientConfigReader` and `PermissionTrieCache.Prune` are unchanged and correct. The applier's two
|
||||
historian hooks are properly detached (`AddressSpaceApplier.cs:255–300`). The new remediation code is
|
||||
itself defensively built: `TryReadSpec` reads `ResilienceConfig` null-safely and the spec survives
|
||||
absent/null values (`DeploymentArtifact.cs:168–181`); the factory logs parse diagnostics instead of
|
||||
throwing; `Invalidate` is a correct enumerate-and-`TryRemove` pass over the concurrent dictionary;
|
||||
`DriverSpawnPlanner`'s respawn-on-ResilienceConfig-change with in-place delta for pure DriverConfig
|
||||
changes (`DriverSpawnPlan.cs:40–46`) is exactly the right split; and the `DriverInstanceActor`
|
||||
wrapping preserved the hard-won no-`ConfigureAwait(false)`-on-the-actor-await rule with an explicit
|
||||
comment that the invoker's internal `ConfigureAwait(false)` does not leak to the caller
|
||||
(`DriverInstanceActor.cs:290–300`) — backed by a real-invoker actor-context test.
|
||||
|
||||
### 2. Performance
|
||||
|
||||
#### P-1 — `Compose` parses each tag's `TagConfig` JSON four times — **Medium**
|
||||
#### P-1 — `Compose` parses each tag's TagConfig JSON four times — **Medium** *(unchanged)*
|
||||
|
||||
`ExtractTagFullName`, `ExtractTagAlarm`, `ExtractTagHistorize` and `ExtractTagArray` each do their
|
||||
own `JsonDocument.Parse` of the same blob per tag (`AddressSpaceComposer.cs:418–433` calling
|
||||
@@ -183,7 +328,7 @@ path, so this is a scalability tax rather than a latency bug today.
|
||||
**Recommendation:** parse once per tag into a small `TagConfigIntent` record (which also collapses
|
||||
the byte-parity duplication — see C-1).
|
||||
|
||||
#### P-2 — Per-operation allocations on the authorization hot path — **Medium**
|
||||
#### P-2 — Per-operation allocations on the authorization hot path — **Medium** *(unchanged)*
|
||||
|
||||
`PermissionTrie.CollectMatches` allocates a fresh `HashSet<string>` from the session's LDAP groups
|
||||
plus a `List<MatchedGrant>` on every call (`Core/Authorization/PermissionTrie.cs:42–45`), and
|
||||
@@ -192,77 +337,69 @@ CreateMonitoredItems batch turns this into per-node garbage. `UserAuthorizationS
|
||||
immutable per membership refresh — the case-insensitive set can be computed once per session
|
||||
version and reused.
|
||||
|
||||
#### P-3 — `PollGroupEngine`: one background task + independent timer per subscription, no cross-subscription batching — **Medium**
|
||||
#### P-3 — `PollGroupEngine`: one background task + independent timer per subscription, no cross-subscription batching — **Medium** *(unchanged)*
|
||||
|
||||
Each `Subscribe` spawns a dedicated `Task.Run` loop (`PollGroupEngine.cs:73–84`). Drivers that map
|
||||
each OPC UA monitored-item group (or each poll group) to a subscription get linear task/timer
|
||||
growth, and two subscriptions polling overlapping tag sets at the same interval issue duplicate
|
||||
protocol reads — the engine has no interval-bucketed scheduler or read coalescing. Fine at dozens
|
||||
of subscriptions; a scaling wall at hundreds per driver instance. Worth a note in the driver-facing
|
||||
docs at minimum; an interval-bucketed shared loop is the structural fix if tag counts grow.
|
||||
of subscriptions; a scaling wall at hundreds per driver instance.
|
||||
|
||||
#### P-4 — Per-write allocation in the non-idempotent write path — **Low**
|
||||
#### P-4 — Per-write allocation in the non-idempotent write path — **Low** *(unchanged; now also dead code — see S-8)*
|
||||
|
||||
`CapabilityInvoker.ExecuteWriteAsync` builds a new options record + `Dictionary` per non-idempotent
|
||||
write (`Core/Resilience/CapabilityInvoker.cs:135–152`) even though `GetOrCreate` keys only on
|
||||
`(id, host, capability)` and ignores the options after the first build — the comment even cites the
|
||||
"1% pipeline budget". Cache the no-retry snapshot per options generation, or key the fast path on a
|
||||
precomputed pipeline.
|
||||
write (`Core/Resilience/CapabilityInvoker.cs:117–131`) even though `GetOrCreate` keys only on
|
||||
`(id, host, capability)` and ignores the options after the first build. Since the only production
|
||||
caller now passes `isIdempotent: true` (S-8), this path is unreachable — resolve it together with
|
||||
S-8 (either cache the no-retry snapshot or delete the branch).
|
||||
|
||||
**Positive observations.** Pipeline resolution is lock-free `ConcurrentDictionary` reads
|
||||
(`DriverResiliencePipelineBuilder.cs:58–71`). The planner's diff is O(N) dictionary passes with
|
||||
deterministic sorted outputs. The surgical-apply path exists precisely to avoid the expensive full
|
||||
rebuild + subscription teardown for attribute-only edits, and its whitelist-via-`with`-expression
|
||||
technique (`AddressSpaceApplier.cs:630–663`) is future-field-safe (unknown new fields force the
|
||||
safe rebuild). Telemetry instruments are no-op until a listener attaches (`OtOpcUaTelemetry.cs`).
|
||||
**Positive observations.** Pipeline resolution remains lock-free `ConcurrentDictionary` reads; the
|
||||
new factory snapshots options **once per `Create`** so the per-call accessor is allocation-free —
|
||||
an explicit hot-path improvement over re-parsing per call. The planner's diff is O(N) dictionary
|
||||
passes with deterministic sorted outputs; the surgical-apply path and its
|
||||
whitelist-via-`with`-expression technique (`AddressSpaceApplier.cs:630–663`) are unchanged and
|
||||
future-field-safe. The new resilience logging only attaches callbacks when a tracker or logger is
|
||||
present, so unit-test pipelines stay callback-free.
|
||||
|
||||
### 3. Conventions
|
||||
|
||||
#### C-1 — "Byte-parity by convention": TagConfig parsing replicated in four projects with comment-enforced sync — **High**
|
||||
#### C-1 — "Byte-parity by convention": TagConfig parsing replicated in four projects with comment-enforced sync — **High** *(unchanged)*
|
||||
|
||||
`ExtractTagFullName` (and friends) exist as deliberate copies in:
|
||||
|
||||
- `OpcUaServer/AddressSpaceComposer.cs:536–551` (+ `ExtractTagAlarm`/`ExtractTagHistorize`/`ExtractTagArray`/`TryExtractDeviceHost`)
|
||||
- `Runtime/Drivers/DeploymentArtifact.cs` (the artifact-decode mirror, per its own comments)
|
||||
- `Core/OpcUa/EquipmentNodeWalker.cs:193–208`
|
||||
- `Configuration/Validation/DraftValidator.cs:60–71` (a fourth "small local copy, consistent with
|
||||
this codebase where the composer keeps its own")
|
||||
- `Configuration/Validation/DraftValidator.cs:60–71`
|
||||
|
||||
Each copy carries a "MUST parse identically (byte-parity)" comment. The equality of the deployed
|
||||
address space across the live-compose and artifact-decode seams — and therefore the correctness of
|
||||
the diff/no-op-redeploy behavior — rests on humans keeping four JSON parsers in sync. The codebase
|
||||
already demonstrated the fix once: `EquipmentScriptPaths` (`Commons/Types/EquipmentScriptPaths.cs`)
|
||||
was created exactly to de-duplicate the `ctx.GetTag` extraction "those two seams used to duplicate".
|
||||
was created exactly to de-duplicate the `ctx.GetTag` extraction.
|
||||
|
||||
**Recommendation:** move the TagConfig/DeviceConfig intent parsing into `Commons` (it has no EF or
|
||||
SDK dependency; every current copy's host project already references Commons or could) as a single
|
||||
**Recommendation:** move the TagConfig/DeviceConfig intent parsing into `Commons` as a single
|
||||
`TagConfigIntent.Parse(string)` — also resolving P-1 — and keep one cross-seam parity test instead
|
||||
of N replicated implementations.
|
||||
|
||||
#### C-2 — Core depends on Configuration: EF entities and enums are the domain model — **Medium**
|
||||
#### C-2 — Core depends on Configuration: EF entities and enums are the domain model — **Medium** *(unchanged, accepted-risk)*
|
||||
|
||||
`ZB.MOM.WW.OtOpcUa.Core.csproj` references `Configuration`; `PermissionTrie` consumes
|
||||
`Configuration.Enums.NodePermissions`/`NodeAclScopeKind` (`Core/Authorization/PermissionTrie.cs:1`),
|
||||
`EquipmentNodeWalker` consumes `Configuration.Entities` directly, and the composer's plan records
|
||||
are projections of EF entities. There is no separate domain layer — persistence types *are* the
|
||||
model. This is a deliberate, consistent choice (and the plan records do decouple the applier), but
|
||||
it means: (a) any EF-driven entity change ripples straight into authorization and composition
|
||||
logic; (b) `Configuration` (with EF Core, SqlServer, LiteDB, DataProtection packages) is pulled
|
||||
into anything referencing Core. Flagging as accepted-risk to document rather than refactor —
|
||||
new code should keep preferring the plan-record boundary the composer established.
|
||||
`ZB.MOM.WW.OtOpcUa.Core.csproj:16` references `Configuration`; `PermissionTrie` consumes
|
||||
`Configuration.Enums`, `EquipmentNodeWalker` consumes `Configuration.Entities` directly, and the
|
||||
composer's plan records are projections of EF entities. A deliberate, consistent choice — flagged
|
||||
as accepted-risk to document rather than refactor. Notably, the new remediation code got this
|
||||
boundary **right**: the invoker seam lives in `Core.Abstractions` precisely so Runtime avoids the
|
||||
Core→Polly closure — new code should keep preferring that pattern.
|
||||
|
||||
#### C-3 — `DraftValidator` re-derives the NodeId scheme and hard-codes a driver-type string — **Medium**
|
||||
#### C-3 — `DraftValidator` re-derives the NodeId scheme and hard-codes a driver-type string — **Medium** *(unchanged)*
|
||||
|
||||
`ValidateNoEquipmentSignalNameCollision` re-implements the folder-scoped NodeId key by hand
|
||||
(`Configuration/Validation/DraftValidator.cs:75–97`) because `Configuration` cannot reference
|
||||
`Commons.OpcUa.EquipmentNodeIds` (dependency direction) — a third copy of the
|
||||
`{equipment}/{folder}/{name}` scheme whose drift would silently break the collision check.
|
||||
`ValidateGalaxyTagFullName` hard-codes `dtype != "GalaxyMxGateway"`
|
||||
(`DraftValidator.cs:41–55`) rather than a shared driver-type constant. Both are symptoms of the
|
||||
same missing shared-contract home as C-1; a Commons-level scheme/constants type (or moving
|
||||
`EquipmentNodeIds` down to a project Configuration can see) fixes both.
|
||||
`Commons.OpcUa.EquipmentNodeIds` — a third copy of the `{equipment}/{folder}/{name}` scheme.
|
||||
`ValidateGalaxyTagFullName` hard-codes `dtype != "GalaxyMxGateway"` (`DraftValidator.cs:48`). Both
|
||||
are symptoms of the same missing shared-contract home as C-1.
|
||||
|
||||
#### C-4 — Composer violates its own purity contract with `Trace.TraceWarning` — **Low**
|
||||
#### C-4 — Composer violates its own purity contract with `Trace.TraceWarning` — **Low** *(unchanged)*
|
||||
|
||||
The class doc says "Same inputs → same outputs, no logging" (`AddressSpaceComposer.cs:280–283`),
|
||||
but the dangling-predicate-script skip emits `Trace.TraceWarning` (`AddressSpaceComposer.cs:493–496`)
|
||||
@@ -270,103 +407,147 @@ but the dangling-predicate-script skip emits `Trace.TraceWarning` (`AddressSpace
|
||||
production sinks. Either return skipped-alarm ids in the composition for the caller to log, or
|
||||
accept an injected logger and fix the doc.
|
||||
|
||||
#### C-5 — Inconsistent duplicate-key defensiveness inside `Compose` — **Low**
|
||||
#### C-5 — Inconsistent duplicate-key defensiveness inside `Compose` — **Low** *(unchanged)*
|
||||
|
||||
`deviceHostById` is built with a deliberate last-wins `foreach` and a comment explaining why
|
||||
`ToDictionary` (throw-on-dupe) would diverge from the decode side (`AddressSpaceComposer.cs:352–364`),
|
||||
yet three lines later `driversById`/`namespacesById`/`scriptsById` use plain `ToDictionary`
|
||||
(`AddressSpaceComposer.cs:401–402, 453`) — a duplicate logical id (only possible on DB-constraint
|
||||
bypass) crashes the whole compose. Pick one posture; the defensive one is already argued for.
|
||||
`ToDictionary` (throw-on-dupe) would diverge from the decode side (`AddressSpaceComposer.cs:357–363`),
|
||||
yet `driversById`/`namespacesById`/`scriptsById` use plain `ToDictionary`
|
||||
(`AddressSpaceComposer.cs:401–402, 453`). Pick one posture; the defensive one is already argued for.
|
||||
|
||||
#### C-6 — Vestigial `Akka` package reference in Commons — **Low**
|
||||
#### C-6 — Vestigial `Akka` package reference in Commons — **Low** *(unchanged)*
|
||||
|
||||
`Commons.csproj` references the `Akka` package but no Commons source uses an Akka type (only doc
|
||||
comments mention actors). Harmless today because every consumer is Akka-hosted anyway, but it
|
||||
contradicts Commons' role as the dependency-light contracts layer. Remove it.
|
||||
`Commons.csproj:9` references the `Akka` package but no Commons source uses an Akka type (the new
|
||||
`IScriptCacheOwner` doesn't either — only doc comments mention actors). Remove it.
|
||||
|
||||
#### C-7 — Stale scaffold-era XML docs on load-bearing classes — **Low**
|
||||
#### C-7 — Stale scaffold-era XML docs on load-bearing classes — **Low** *(partially fixed)*
|
||||
|
||||
`AddressSpaceApplier`'s class doc still describes the F14 scaffold ("For now we record the work",
|
||||
"the SDK adapter that lands in F10b will decide…" — `AddressSpaceApplier.cs:7–26`) although both
|
||||
features shipped. `DriverHost`'s doc promises "per-process isolation for Tier C … implemented in
|
||||
Phase 2 via named-pipe RPC" (`Core/Hosting/DriverHost.cs:5–10`), an architecture superseded by the
|
||||
out-of-repo mxaccessgw gateway. In a codebase whose XML documentation is otherwise a genuine asset
|
||||
(and evidently relied on), stale top-of-class narratives are actively misleading.
|
||||
The fixdocs sweep (`3b5ef439`) fixed `CapabilityInvoker`'s doc duplication, but both narratives this
|
||||
finding flagged survive verbatim: `AddressSpaceApplier`'s class doc still describes the F14 scaffold
|
||||
("For now we record the work", "the SDK adapter that lands in F10b will decide…" —
|
||||
`AddressSpaceApplier.cs:7–26`) although both features shipped, and `DriverHost`'s doc still promises
|
||||
"per-process isolation for Tier C … implemented in Phase 2 via named-pipe RPC"
|
||||
(`Core/Hosting/DriverHost.cs:5–10`), an architecture superseded by the out-of-repo gateways. In a
|
||||
codebase whose XML documentation is otherwise a genuine asset, stale top-of-class narratives are
|
||||
actively misleading.
|
||||
|
||||
**Positive observations.** Naming and structure are highly consistent (logical-id + RowVersion
|
||||
entity pattern, `Null*`/`Deferred*` sink pattern, options records with `Validate()`, message
|
||||
records per DPS topic). Zero `TODO`/`HACK`/`FIXME`/`Obsolete` markers across all five projects. The
|
||||
DbContext matches a documented schema with an introspection compliance test
|
||||
(`SchemaComplianceTests`). The `DeferredAddressSpaceSink` explicitly forwards the surgical
|
||||
capability with a comment memorializing the F10b prod-inertness bug (`DeferredAddressSpaceSink.cs:52–70`)
|
||||
— institutional memory encoded where the next person will trip.
|
||||
#### C-8 — NEW: the deferred-sink forwarding guard's "exhaustive" interface detection is namespace-filtered — **Low**
|
||||
|
||||
`DeferredSinkForwardingReflectionTests.DeferredAddressSpaceSink_implements_exactly_the_known_forwarding_interfaces`
|
||||
— the guard-of-the-guard whose whole job is catching a *new* capability interface added to the
|
||||
wrapper — filters `GetInterfaces()` to `i.Namespace == typeof(IOpcUaAddressSpaceSink).Namespace`
|
||||
(`Commons.Tests/OpcUa/DeferredSinkForwardingReflectionTests.cs:44`), i.e. only
|
||||
`ZB.MOM.WW.OtOpcUa.Commons.OpcUa`. Commons already has sibling namespaces the wrapper could
|
||||
plausibly grow an interface from (`Commons.Engines` — home of the brand-new `IScriptCacheOwner` —
|
||||
and `Commons.Types`): a future forwarding interface declared outside `Commons.OpcUa` would be
|
||||
silently excluded from `ForwardingInterfaces` enforcement, re-opening exactly the
|
||||
F10b/PR#423 inert-capability trap class this guard exists to close. The per-method forwarding test
|
||||
and the `DummyArg` fabricator are otherwise well built (the `DispatchProxy` recorder auto-covers
|
||||
future members; bool-returning members return `true` so capability-sniffing short-circuits still
|
||||
record).
|
||||
|
||||
**Recommendation:** drop the namespace filter and instead exclude a small explicit allowlist of
|
||||
known non-forwarding interfaces (e.g. `IDisposable`), so any *unknown* interface trips the guard
|
||||
regardless of where it lives.
|
||||
|
||||
**Positive observations.** Naming and structure remain highly consistent, and the new code follows
|
||||
the house patterns exactly: the seam mirrors `IDriverFactory`, both null objects are private-ctor
|
||||
singletons, options records keep `Validate()`-style diagnostics, and every new public member is
|
||||
documented. Two convention-enforcement mechanisms landed that materially raise the floor: the
|
||||
OTOPCUA0001 analyzer is wired tree-wide (an unwrapped capability call is now a build error, with
|
||||
the one remaining pragma carrying a full rationale — `GenericDriverNodeManager.cs:69`), and the
|
||||
reflection forwarding guard turns the deferred-sink trap from a code-review convention into a
|
||||
failing test. The #11 premise correction is honest engineering — the SBR comments/docs now state
|
||||
that the typed option is reinforcing, not the activator, instead of quietly keeping the wrong
|
||||
war story.
|
||||
|
||||
### 4. Underdeveloped Areas
|
||||
|
||||
#### U-1 — Dead/dormant code retained in Core: `GenericDriverNodeManager`, `EquipmentNodeWalker`, `TryParseRelayBody` — **Medium**
|
||||
#### U-1 — Dead/dormant code retained in Core: `GenericDriverNodeManager`, `EquipmentNodeWalker`, `TryParseRelayBody` — **Medium** *(unchanged; evidence strengthened)*
|
||||
|
||||
- `GenericDriverNodeManager` (`Core/OpcUa/GenericDriverNodeManager.cs`) has **no production
|
||||
references** — only its own test file. The production path is the composer→applier→sink chain.
|
||||
- `GenericDriverNodeManager` (`Core/OpcUa/GenericDriverNodeManager.cs`) has no production references
|
||||
— now stated *in the source itself* by the #10 triage pragma ("zero production references",
|
||||
`GenericDriverNodeManager.cs:69`).
|
||||
- `EquipmentNodeWalker` (`Core/OpcUa/EquipmentNodeWalker.cs:160–165`) self-declares "retained for
|
||||
unit-test support only; real server deployments never invoke Walk" — ~280 lines plus
|
||||
`IdentificationFolderBuilder` and a full test suite exercising a code path that cannot run in
|
||||
production. Its `ExtractFullName` copy is also one of the four C-1 duplicates.
|
||||
unit-test support only" — ~280 lines plus `IdentificationFolderBuilder` and a full test suite for
|
||||
a path that cannot run in production. Its `ExtractFullName` copy is one of the four C-1 duplicates.
|
||||
- `EquipmentScriptPaths.TryParseRelayBody` (`Commons/Types/EquipmentScriptPaths.cs:164–172`)
|
||||
serviced the relay→alias converter that was deleted when Galaxy became a standard driver
|
||||
(2026-06-12); only tests call it now.
|
||||
serviced the deleted relay→alias converter; only tests call it.
|
||||
|
||||
Dormant code with green tests is worse than deleted code: it passes every gate while silently
|
||||
diverging from the live path. **Recommendation:** delete the walker + GDNM + relay parser (git
|
||||
preserves them), or, if the walker must survive as a test fixture, move it into the test project.
|
||||
diverging from the live path — the review cycle's own RESILIENCE-DISPATCH-GAP episode proved the
|
||||
genre. **Recommendation:** delete all three (git preserves them), or move the walker into the test
|
||||
project if it must survive as a fixture.
|
||||
|
||||
#### U-2 — The entire Tier C recycle machinery has no `IDriverSupervisor` implementation — **Medium**
|
||||
#### U-2 — The entire Tier C recycle machinery has no `IDriverSupervisor` implementation — **Medium** *(unchanged; now accreting further)*
|
||||
|
||||
`MemoryRecycle`, `ScheduledRecycleScheduler`, and `DriverResilienceStatusTracker.RecordRecycle`
|
||||
all drive `IDriverSupervisor.RecycleAsync`, but a repo-wide search finds **zero concrete
|
||||
implementations** of `IDriverSupervisor` (`Core.Abstractions/IDriverSupervisor.cs` + the two
|
||||
Stability consumers are the only references). Since the out-of-process driver story moved to the
|
||||
external mxaccessgw/HistorianGateway sidecars, no in-repo driver is Tier C, so the recycle path is
|
||||
compiled, tested, and unreachable. Either document it as speculative infrastructure for a future
|
||||
tier-migration workflow (the `MemoryRecycle` doc hints at one) or prune it alongside U-1.
|
||||
all drive `IDriverSupervisor.RecycleAsync`, but a repo-wide search still finds **zero concrete
|
||||
implementations**. The gap is now *growing* supporting surface: `DriverResilienceOptionsParser`
|
||||
carefully gates `RecycleIntervalSeconds` to "Tier C only" (`DriverResilienceOptionsParser.cs:101–110`)
|
||||
and the AdminUI authors the field — configuration and validation for an execution path that does
|
||||
not exist. Either document it as speculative infrastructure for a future tier-migration workflow or
|
||||
prune it alongside U-1.
|
||||
|
||||
#### U-3 — Continuous-historization interest set: delta feed landed, initial set still empty — **Medium (known-limitation debt)**
|
||||
#### U-3 — Continuous-historization restart convergence: mechanically present, unverified — **Low** *(downgraded from Medium; prior framing partially superseded)*
|
||||
|
||||
The applier now pushes add/remove historized-ref deltas per deploy
|
||||
(`AddressSpaceApplier.cs:310–353` → `IHistorizedTagSubscriptionSink`), which is the convergent
|
||||
design for *subsequent* deploys — but per the documented KNOWN LIMITATION 2 (CLAUDE.md), the
|
||||
recorder is still spawned with an empty ref set, so after a process restart the recorder historizes
|
||||
nothing until the next deploy produces a delta. The seam contract
|
||||
(`Core.Abstractions/Historian/IHistorizedTagSubscriptionSink.cs`) supports the fix; the missing
|
||||
piece is a full-set replay at spawn/rebuild (e.g. have the applier feed the *complete* current set
|
||||
on `RebuildCalled`, not just the diff). This is the highest-leverage half-finished feature in the
|
||||
subsystem.
|
||||
Re-analysis this cycle shows the prior "after a restart the recorder historizes nothing until the
|
||||
next deploy" framing was too strong: `DriverHostActor.RestoreApplied` fires a `RebuildAddressSpace`
|
||||
on bootstrap (`DriverHostActor.cs:1293–1320`), and `OpcUaPublishActor.HandleRebuild` diffs the
|
||||
restored artifact against an empty `_lastApplied` baseline (`OpcUaPublishActor.cs:290–340`) — a
|
||||
full-add plan whose `Apply` feeds the **complete** historized set through `FeedHistorizedRefs`
|
||||
(`AddressSpaceApplier.cs:310–353`). The spawn-time empty set
|
||||
(`Runtime/ServiceCollectionExtensions.cs:269–278`) is therefore covered by the restore path, and the
|
||||
recorder is deliberately spawned before the applier wiring so the feed can reach it. What remains is
|
||||
exactly what CLAUDE.md Known Limitation 2 (as reworded by the remediation) says: **no
|
||||
restart-convergence test** asserting the recorder re-registers the deployed set after a process
|
||||
restart, and no live end-to-end verification. Given the subsystem's history of
|
||||
mechanically-present-but-inert paths, that test is cheap insurance and should be written.
|
||||
|
||||
#### U-4 — Additive-only ACL model (no Deny) is a documented v2.0 gap — **Low**
|
||||
#### U-4 — Additive-only ACL model (no Deny) is a documented v2.0 gap — **Low** *(unchanged)*
|
||||
|
||||
`PermissionTrie` is explicitly pure-union ("no explicit Deny in v2.0",
|
||||
`Core/Authorization/PermissionTrie.cs:13–17`): a broad cluster-root grant cannot be carved back at
|
||||
a sub-scope. Acceptable for the current flat-role deployments (AdminUI roles are global by design),
|
||||
but it bounds how fine-grained OT authorization can get without a trie-walk semantics change —
|
||||
worth keeping visible in the security docs.
|
||||
a sub-scope. Acceptable for the current flat-role deployments, but it bounds how fine-grained OT
|
||||
authorization can get — keep visible in the security docs.
|
||||
|
||||
#### U-5 — Test coverage is strong but unevenly distributed — **Low**
|
||||
#### U-5 — Test coverage: `ClusterRoleInfo` remains the riskiest untested seam — **Low** *(narrowed from prior)*
|
||||
|
||||
Test method counts: Core.Tests 208, Configuration.Tests 94, Core.Abstractions.Tests 65,
|
||||
Commons.Tests 45, Cluster.Tests 21. The composition pipeline itself is thoroughly covered (10+
|
||||
dedicated files in `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpace*`). Gaps:
|
||||
`ClusterRoleInfo` — the only concurrency-bearing class in the Cluster project (lock + subscriber
|
||||
actor + event fan-out) — has no test (Cluster.Tests covers only the three pure classes);
|
||||
`DriverHost` dispose/register races (S-3) and the applier failure surface (S-1) are untested
|
||||
because the surface doesn't exist. An Akka TestKit harness for `ClusterRoleInfo` leader-change
|
||||
sequencing would close the riskiest untested seam.
|
||||
This cycle materially improved the domain's test posture: `SplitBrainResolverActivationTests`
|
||||
(Cluster.Tests), `DriverCapabilityInvokerFactoryTests` (invalidate-per-instance, malformed-config
|
||||
fallback, override-reaches-execution), pipeline-builder logging tests, and the Commons forwarding
|
||||
reflection guard all landed. Still missing: `ClusterRoleInfo` — the only concurrency-bearing class
|
||||
in the Cluster project (lock + subscriber actor + event fan-out) — has **no test** (Cluster.Tests
|
||||
still covers only pure classes plus the SBR option), and the S-1 applier failure surface and S-3
|
||||
dispose races remain untested because the surfaces don't exist. An Akka TestKit harness for
|
||||
`ClusterRoleInfo` leader-change sequencing would close the riskiest untested seam.
|
||||
|
||||
#### U-6 — NEW: bulkhead is advertised, authored, parsed — and never applied — **Medium**
|
||||
|
||||
The resilience surface documents "retry / circuit-breaker / bulkhead / tracker telemetry" in the
|
||||
seam docs (`IDriverCapabilityInvoker.cs:4–6`), the analyzer's diagnostic message
|
||||
(`UnwrappedCapabilityCallAnalyzer.cs:105`), and the tracker's "Polly bulkhead-depth column" doc;
|
||||
`DriverResilienceOptions` carries `BulkheadMaxConcurrent`/`BulkheadMaxQueue` defaults
|
||||
(`DriverResilienceOptions.cs:23–25`); the parser merges operator overrides for both
|
||||
(`DriverResilienceOptionsParser.cs:116–117`); and the AdminUI form authors them
|
||||
(`ResilienceFormModel.cs:18–20`). But `DriverResiliencePipelineBuilder.Build`
|
||||
(`DriverResiliencePipelineBuilder.cs:99–177`) adds only timeout → retry → circuit-breaker — there is
|
||||
no `AddConcurrencyLimiter` (or any rate/concurrency strategy) anywhere in `src/`. The bulkhead is a
|
||||
dead knob end-to-end: an operator can set it, the deploy respawns the driver, and nothing changes —
|
||||
the same "configured but inert" genre as the dispatch gap this cycle just fixed, now one layer
|
||||
deeper.
|
||||
|
||||
**Recommendation:** either add the concurrency limiter to the pipeline (keyed like the breaker,
|
||||
per host) with a factory test proving the queue bound engages, or strip the fields from the
|
||||
options/parser/form and the word "bulkhead" from the docs until the strategy ships. Don't leave the
|
||||
knob authored-but-unwired — this codebase's history says it will be trusted.
|
||||
|
||||
---
|
||||
|
||||
## Maturity Ratings
|
||||
|
||||
| Dimension | Rating (1–5) | Justification |
|
||||
|---|---|---|
|
||||
| Stability | **4** | Deliberate, well-commented failure-mode engineering throughout (fail-closed caches, atomic pointer swaps, detached hooks, fatal-exception classification); docked for the applier's swallow-everything success reporting (S-1) and the blocking unsubscribe (S-2). |
|
||||
| Performance | **3** | Deploy-path costs are deterministic and diffed to near-zero for no-op redeploys, and the surgical-apply path avoids rebuild churn; but hot paths carry avoidable per-operation allocations (P-2), redundant JSON parsing scales 4× with tag count (P-1), and the poll engine is task-per-subscription with no coalescing (P-3). |
|
||||
| Conventions | **4** | Exceptionally consistent naming, patterns, and XML docs with zero TODO debt; docked for the systemic replicate-and-comment "byte-parity" duplication (C-1/C-3) and the Core→Configuration persistence coupling (C-2). |
|
||||
| Underdeveloped areas | **3** | The live pipeline is feature-complete and well-tested, but Core retains three dead/dormant code paths with green tests (U-1), a fully-built-but-unreachable Tier C recycle subsystem (U-2), and the known continuous-historization initial-set gap (U-3). |
|
||||
| Dimension | 2026-07-08 | 2026-07-12 | Justification |
|
||||
|---|---|---|---|
|
||||
| Stability | **4** | **4** | The deliberate failure-mode engineering (fail-closed caches, detached hooks, null-object seams) still defines the domain, and the new dispatch wiring was built carefully (respawn-on-change, invalidate pairing, actor-context preservation). But nothing on the stability backlog moved — S-1's swallow-everything deploy reporting remains the headline — and the new machinery introduced a genuine operator-reachable bricking vector (S-6) that directly contradicts its own never-brick contract. |
|
||||
| Performance | **3** | **3** | All four performance findings are untouched (4× JSON parse, per-node authorization garbage, task-per-subscription engine, per-write allocation). The new code is hot-path conscious (once-per-Create options snapshot, callback-free pipelines without observers) but adds no throughput improvements to the existing paths. |
|
||||
| Conventions | **4** | **4** | Already exceptionally consistent; this window added *mechanical enforcement* (tree-wide OTOPCUA0001 as a build error, the reflection forwarding guard) which is the right direction — but the systemic byte-parity duplication (C-1/C-3) is untouched, the two flagged stale class narratives survived a dedicated fixdocs pass (C-7), and the new guard itself has a namespace blind spot (C-8). |
|
||||
| Underdeveloped areas | **3** | **3** | The cycle's biggest dormancy (CapabilityInvoker never dispatched) is genuinely fixed and analyzer-guarded, and U-3 turned out mostly mechanically closed. But U-1's three dead code paths and U-2's implementation-less Tier C machinery are untouched — the latter now *accreting* config/validation surface — and a new authored-but-inert knob shipped (U-6 bulkhead). The pattern the review flagged (built, tested, unreachable) is still being produced. |
|
||||
|
||||
Reference in New Issue
Block a user