Joseph Doherty
44f6408e1c
Every round-2 finding re-verified against code (73 closed / 31 partial / 97 carried); matrix 19/28 cells up, one down. New Critical 03/S14 (AdminUI FallbackPolicy 401s every mesh gRPC map on a login-enabled central; masked on every rig gate by DisableLogin=true). New Highs 03/S15, 05/STAB-19, 05/STAB-18, 03/S16 — the stale-Good family at four seams.
184 lines
46 KiB
Markdown
184 lines
46 KiB
Markdown
# Architecture Review — Server & Runtime Subsystem
|
||
|
||
- **Date:** 2026-08-19
|
||
- **Commit:** `12b0035a` (master, clean tree)
|
||
- **Updates:** the 2026-07-12 re-review at `f6eaa267` (which updated the 2026-07-08 original at `9cad9ed0`). **This is the second update.** Since f6eaa267, ~431 commits touched `src/Server` alone (+43.9k/−9.7k lines): the per-cluster mesh program Phases 1–7, the ClusterClient→gRPC migration, ConfigSource/ConfigServe fetch-and-cache, the Phase 5 gRPC telemetry stream, Phase 4 DB-less driver nodes, LocalDb registration (incl. the 2026-08-18 conditional register/deregister), the entire API-key + ACL workstream (#520, 8 stages), the comms-loss watchdog (#527), surgical deploys (2026-08-15), the batched value pipeline, and the continuous-historization Primary gate (#537) + poison-tag quarantine (#538).
|
||
- **Scope:** `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer`, `.Host`, `.Runtime`, `.ControlPlane`, `.Security`, plus `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa`, `src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization`, `src/Core/ZB.MOM.WW.OtOpcUa.Cluster`, and `tests/Server` for coverage assessment.
|
||
- **Dimensions:** Stability · Performance · Conventions · Underdeveloped areas
|
||
|
||
---
|
||
|
||
## Prior-finding status (f6eaa267 → 12b0035a)
|
||
|
||
Every finding from the 2026-07-12 report, verified against the actual code at HEAD (never against STATUS.md or remediation claims). Line numbers are at `12b0035a`.
|
||
|
||
| ID | One-liner | Verified status |
|
||
|---|---|---|
|
||
| S2 | LDAP auth blocks SDK session-activation thread | **CLOSED (superseded by #520), one residual.** `LdapOpcUaUserAuthenticator` is deleted (only historical comments remain — `Security/Ldap/LdapOptions.cs:95`, `LdapOptionsValidator.cs:52-55` confirm the four S2 resilience knobs went with it). The data plane registers `ApiKeyOpcUaUserAuthenticator` (`Host/Program.cs:428,441`; deny-all Null at :448) — one indexed lookup + one HMAC, `FixedTimeEquals` (`Core/Authorization/ApiKeySecret.cs:124`). The impersonation callback still block-bridges with `CancellationToken.None` (`OpcUaApplicationHost.cs:317-319, 375-376`), documented as SDK-irreducible. AdminUI LDAP survives, entirely outside the impersonation path. Residual: on admin/fused nodes the key store is a per-activation SQL read — see **S18** below. |
|
||
| S3 | HistoryRead block-bridges gateway per node, sequentially | **CLOSED (R2-08, verified).** All four arms async-ified with bounded per-node fan-out (`RunBounded`, `OtOpcUaNodeManager.cs:3772-3789`, semaphore = `HistoryReadBatchParallelism` default 4 at :393), exactly one sync bridge at the arm boundary over contractually non-throwing work items (:3788), a process-wide batch limiter degrading to `BadTooManyOperations` (:3804-3835, default 16 at :401), and a per-request deadline CTS → `BadTimeout` per node (:3861-3862, :3914-3919, default 60 s). Options flow verified end-to-end (`OtOpcUaServerHostedService.cs:103-105, 260-262`). |
|
||
| S4 | Primary-gate default-allow while role unknown (dual-primary window) | **CLOSED (R2-04, verified + live-gated).** `PrimaryGatePolicy` (`Runtime/Drivers/PrimaryGatePolicy.cs:21-27`): known role wins; unknown role allows only when driver member count ≤ 1. Wired at all three `DriverHostActor` gates (`:1378-1381` alarm emit, `:1470-1477` write, `:1557-1573` native ack) plus the scripted-alarm emit (`ScriptedAlarmHostActor.cs:490-501`); denial meter `PrimaryGateDenied` with site/reason tags (`Commons/Observability/OtOpcUaTelemetry.cs:109`). The #537 capture gate deliberately inverts to unknown-captures with a mesh-partner scope predicate (`PrimaryGatePolicy.cs:79-125`) — consistent and documented. Residual: the gate methods' XML *summaries* still describe the old default-allow — see **C6**. |
|
||
| S5 | Redundancy NodeId string-match, no mismatch diagnostic | **PARTIALLY CLOSED.** A once-per-process Warning naming the snapshot nodes was added when the snapshot omits this node on a multi-driver cluster — but only in `DriverHostActor.OnRedundancyStateChanged` (`DriverHostActor.cs:1648-1657`). `OpcUaPublishActor` still silently demotes to ServiceLevel 0 (`OpcUaPublishActor.cs:934, 938-941`) and `ScriptedAlarmHostActor` silently stays role-unknown (`:540-544`). The three-source string-match seam itself is unchanged (`ClusterRoleInfo.cs:41` vs `RedundancyStateActor.cs:214-215`). |
|
||
| S6 | Driver-child restart loses desired-subscription state; no backoff supervision | **STILL OPEN** (partially mitigated). Zero `SupervisorStrategy` overrides anywhere under `src/Server` (grep-verified); a restarted child's `_desiredRefs` reset to empty (`DriverInstanceActor.cs:241-247`) and the #488 periodic reconcile only re-subscribes when `_desiredRefs.Count > 0` (`:625-630`) — it heals a lost handle within one incarnation, not a restart. The new operator-restart face of this is **S16**. |
|
||
| S7 | `PostStop` blocks on `ShutdownAsync(CancellationToken.None)` | **STILL OPEN.** `DriverInstanceActor.cs:1327` — try/caught but synchronous and uncancellable. |
|
||
| S8 | Part 9 commands / native acks at-most-once, client sees Good | **STILL OPEN.** `HandleAlarmCommand` returns `Good` unconditionally (`OtOpcUaNodeManager.cs:1409-1413`, native acks :1444-1450); host routers log-and-drop (`OtOpcUaServerHostedService.cs:154-170, 182-209`). Still no drop counter/metric anywhere on the path. |
|
||
| S9 | No server-cert renewal/expiry monitoring | **STILL OPEN.** Cert checked once at boot (`OpcUaApplicationHost.cs:411-423`, 12-month SDK default); the health surface (`Host/Health/HealthEndpoints.cs:34-95`) watches configdb/akka/cluster-primary/localdb-replication — nothing watches the certificate. |
|
||
| S10 | SDK start failure swallowed; node runs with Null sinks, invisible to /health | **STILL OPEN** (deliberate, unchanged). `OtOpcUaServerHostedService.cs:129-144`. No health check reflects OPC UA server state. |
|
||
| S11 | `AuditWriterActor` unbounded buffer, drops batches on outage | **PARTIALLY CLOSED.** The buffer now snapshots-and-clears per flush (`ControlPlane/Audit/AuditWriterActor.cs:90-91`) so it is bounded per outage window, but a DB outage still drops the batch with only an Error log (:113-116). No longer moot — U3 gained producers. |
|
||
| S12 | Write dispatch hardcodes `isIdempotent: true`; `WriteIdempotentAttribute` has zero readers | **CLOSED (R2-02, verified).** Dispatch passes `isIdempotent: false` with the rationale comment (`DriverInstanceActor.cs:802-811`); the parser force-zeroes Write/AlarmAcknowledge/Initialize retries (`Core/Resilience/DriverResilienceOptionsParser.cs:149-158, 165-169`). Per-tag opt-in remains an unshipped forward path, now honestly documented. |
|
||
| S13 | Respawn-on-ResilienceConfig-change stale-pipeline re-cache race | **CLOSED (R2-02 + 2026-08-08).** `Interlocked` options-generation in `PipelineKey` (`DriverCapabilityInvokerFactory.cs:75`, `DriverResiliencePipelineBuilder.cs:208`); `Invalidate` is cleanup-only by design (`DriverCapabilityInvokerFactory.cs:26-28`); the tracker is invalidated with the pipelines (`:83`) — the R2-10 live-gate fix. |
|
||
| P1 | Any structural deploy change → full rebuild severs all monitored items | **SUBSTANTIALLY CLOSED** (R2-07 + surgical deploys 2026-08-15). `AddressSpaceChangeClassifier` is first-match-wins, default-closed Rebuild; the `with`-equality whitelist makes any FUTURE record field rebuild by construction (`AddressSpaceChangeClassifier.cs:156-185`); every surgical `false`/throw ratchets to `SafeRebuild()` (`AddressSpaceApplier.cs:312, 464-486`). Everyday shapes (tag add/remove/attribute edit, UNS reference add/remove/re-point, raw-tag rename) are subscription-preserving. Realistic residual rebuild shapes: alarm-bearing tag deltas (justified — notifier rewiring), raw **container** rename/remove (`ApplyPureRemove` hard-returns false on `RemovedRawContainers` — `AddressSpaceApplier.cs:443` — so deleting one empty TagGroup still severs everything), `ChangedEquipment`/`ChangedAlarms`, protocol-driver DataType edits. Plus the new **P5** subsumption gap, which turns equipment-with-references removal back into a rebuild. |
|
||
| P2 | Per-value global-lock write, no batching | **CLOSED.** The whole pipeline is batched: `DriverHostActor.ForwardBatchToMux` is the only value ingress and expands the raw+UNS-mirror fan-out *inside* one batch (`DriverHostActor.cs:1145-1199`); `OpcUaPublishActor.HandleAttributeBatchUpdate` makes one `sink.WriteValues` call (`OpcUaPublishActor.cs:348-369`); `OtOpcUaNodeManager.WriteValues` takes the `Lock` once per 512-item chunk with validation outside it (`OtOpcUaNodeManager.cs:562, 577-641`); `DeferredAddressSpaceSink` forwards it and the reflection guard covers it. |
|
||
| P3 | HistoryRead-Events unpaged, `0` → `int.MaxValue` | **PARTIALLY CLOSED.** The `0 → no limit` translation is now a deliberate, documented Part 4/11 reading and a positive client cap is honoured (`OtOpcUaNodeManager.cs:3665-3675, 3626`); the Raw arm gained real tie-safe paging (`ServeRawPagedAsync` :3965+). Events still issue **no continuation points** (:3635-3636) — a huge window is one response, now damage-limited by the S3 deadline + batch limiter. |
|
||
| P4 | Deploy re-runs four Materialise passes over the full composition | **CLOSED (2026-08-15).** `MaterialiseNodes` branches every pass on `fullComposition: outcome.RebuildCalled` and passes the plan's delta subsets on a non-rebuild apply (`OpcUaPublishActor.cs:508-509, 629-701`); only `MaterialiseHierarchy` runs full (O(areas+lines+equipment), rationale :504-507). Branching on `RebuildCalled` rather than the classifier kind is itself correctness-load-bearing (:609-613). |
|
||
| C1 | `ServerHistorian` bound imperatively in five places | **PARTIALLY CLOSED.** `AddValidatedOptions<ServerHistorianOptions, ServerHistorianOptionsValidator>` is real (`Host/Program.cs:195-196`) so the section fails fast at boot; but four imperative `GetSection(...).Get<>()` sites remain (`Runtime/ServiceCollectionExtensions.cs:237, 273`; `Program.cs:199`; `OtOpcUaServerHostedService.cs:104`). |
|
||
| C2 | Two-tier options validation; `DevStubMode` only log-warned in prod | **PARTIALLY CLOSED.** Fail-fast now covers ServerHistorian (consumer-gated, AlarmHistorian-only corner included — `ServerHistorianOptionsValidator.cs:58-98`), OpcUa host options (`Program.cs:454`), and LDAP incl. plaintext-transport boot fail. `DevStubMode` still exists and is still only log-warned (`OtOpcUaLdapAuthService.cs:79-88`) but its blast radius shrank to AdminUI login. `ContinuousHistorization` remains warn-only, with a silent `CommitMode` fallback (`Program.cs:265-280`). |
|
||
| C3 | Static Serilog logger islands + node-manager CS0618 pragmas | **STILL OPEN — grew slightly.** Runtime SCE now has 4 static-logger sites (`Runtime/ServiceCollectionExtensions.cs:158, 189, 204, 241`) plus `DpsScriptLogPublisher.cs:61`; the node manager carries 14 CS0618 pragma blocks. One case is documented-deliberate: `LocalDbSetup.OnReady` runs inside the `AddZbLocalDb` singleton factory where DI resolution would nest inside singleton construction (`Host/Configuration/LocalDbSetup.cs:93-100`). |
|
||
| C4 | `IHistorianProvisioning` resolve has no missing-registration warning | **CLOSED.** `AddOtOpcUaRuntime` seeds `NullHistorianProvisioning.Instance` so the resolve is never silently null (`Runtime/ServiceCollectionExtensions.cs:64, 501-505`); the analogous missing-deps cases now log explicit Warnings (:382, :489-490). |
|
||
| U1 | CLAUDE.md "KNOWN LIMITATION 2" stale | **CLOSED** (and the underlying #491 gate itself ran and closed 2026-08-04, incl. restart-convergence live). |
|
||
| U2 | Deferred-sink forwarding trap half test-guarded | **CLOSED — and extended.** `DeferredSinkForwardingReflectionTests` now covers `ISurgicalAddressSpaceSink` (all five surgical members incl. `UpdateTagAttributes`) via the `DispatchProxy` recorder, keeps the interface-set guard-of-the-guard, and adds a realm-discriminator surface lock (`tests/Core/.../DeferredSinkForwardingReflectionTests.cs:29-33, 56-93`). |
|
||
| U3 | Structured audit pipeline has zero producers | **PARTIALLY CLOSED.** Producers now exist: `ManualFailoverService` (`ControlPlane/Redundancy/ManualFailoverService.cs:61-75`) and AdminUI cert ops (`AdminUI/Audit/CertAuditEvents.cs:43-51`). But `AuditOutcomeMapper` still has zero production readers and its "dormant" remark is now stale in the other direction (`AuditOutcomeMapper.cs:13-17`); the #520 ACL workstream did **not** become a producer (no `OpcUaAccessDenied` emit site). |
|
||
| U4 | `FleetStatusBroadcaster` heartbeat dead code | **CLOSED — deleted** (Gitea #542). No trace remains except the deletion comment at `ControlPlane/ServiceCollectionExtensions.cs:141-143`. |
|
||
| U5 | Native Part 9 conditions Acknowledge-only to driver | **PARTIALLY CLOSED.** Native **Acknowledge** now routes to the owning driver (`OtOpcUaNodeManager.cs:1179-1182 → 1432-1451` → `NativeAlarmAckRouter`). But Confirm/AddComment/Shelve on a *native* condition are sent to the scripted engine's router ("H6c scope is Acknowledge only", :1177-1199) — SDK-local effect only, no upstream reach; native Enable/Disable still `BadNotSupported` (:1226-1231). |
|
||
| U6 | Test gaps concentrate on the fragile seams | **PARTIALLY CLOSED.** Since f6eaa267: real-DPS delivery is tested (`PrimaryGateFailoverTests`, in-process 2-node cluster with a built-in negative control); `OpcUaServer.IntegrationTests` gained the over-the-wire `AclSubscriptionEnforcementTests`; recorder restart-convergence was live-gated (#491). Still open: no actor supervision/restart test anywhere (S6), and the **F1/S14 meta-gap** — every mesh live gate ran on a rig whose `DisableLogin` auto-auth neutralizes the exact middleware that breaks the mesh in production (see S14 and theme 1). |
|
||
| U7 | `IHistoryWriter` Null-wired; H2 HistoryUpdate unimplemented | **STILL OPEN.** `NullHistoryWriter.Instance` is the only registration (`Runtime/ServiceCollectionExtensions.cs:68, 388`); no HistoryUpdate override; `NodePermissions.cs:32-35` still reserves the bit. |
|
||
| U8 | `BuildSecurityPolicies` doc claims a log that doesn't exist | **PARTIALLY CLOSED (mitigated).** The method still logs nothing and the comment still lies (`OpcUaApplicationHost.cs:479-524`), but `OpcUaApplicationHostOptionsValidator` now fails host start on an empty profile list (`OpcUaApplicationHostOptionsValidator.cs:31`, wired `Program.cs:454`), making the silent fallback unreachable through the Host path. Fix the comment. |
|
||
| U9 | `EnsureVariable` silently ignores changed historize-intent | **CLOSED (by surgical deploys), stale comment remains.** `UpdateTagAttributes` (`OtOpcUaNodeManager.cs:2187-2230`) updates Historizing + historian-tagname binding (add *and* remove), AccessLevel, and DataType/shape in place; the classifier whitelists `IsHistorized`/`HistorianTagname` deltas onto it; UNS mirrors repair via `Backing*` fields. `EnsureVariable`'s doc-comment (:2064-2068) still describes the old silent ignore. |
|
||
| U10 | `DriverResilienceStatusTracker` has no reader | **CLOSED.** R2-10 reader chain live; the deferred T10 breaker-OPEN live gate ran 2026-08-08 (Initialize wrapped as `DriverCapability.Initialize`, breaker threshold cadence-corrected to 2, tracker invalidated with pipelines — `DriverCapabilityInvokerFactory.cs:83`). |
|
||
|
||
---
|
||
|
||
## Architecture overview (what changed since f6eaa267)
|
||
|
||
The subsystem was re-platformed around **split topology**: the fleet is three independent 2-node Akka meshes; everything that used to assume one gossip ring now rides explicit gRPC contracts. Central→node commands (`NodeCommandService`) and node→central acks (`CentralCommandService`) are per-node unary dials behind per-cluster PSK interceptors; driver nodes serve a telemetry stream central dials into (`TelemetryStreamGrpcService` + `TelemetryDialSupervisor`); config reaches driver-only nodes via `DeploymentArtifactGrpcService` fetch-and-cache (SHA-256-verified against `RevisionHash`, gzip-aware, null-fetch = failed apply, never an empty config); Phase 4 removed ConfigDb from driver-only nodes entirely (`Program.cs:158-161`), with DB-less ServiceLevel semantics (`OpcUaPublishActor.cs:953-962`). `SplitTopologyTransportValidator` and `ConfigSourceOptionsValidator` enforce the split-safe combinations at boot via `ValidateOnStart` on *effective* roles. The redundancy singleton is cluster-scoped, elected as oldest-Up driver member (`RedundancyStateActor.cs:124-177`), with scope derived from `AkkaClusterOptions` (pure config — the Phase 6 stack-overflow landmine is structurally avoided, `ControlPlane/ServiceCollectionExtensions.cs:234-248`). The opt-in bootstrap guard closes the simultaneous-cold-start split (`ClusterBootstrapGuard.cs:112-115`, `ClusterBootstrapCoordinator.cs:118-158`).
|
||
|
||
The **data plane** replaced LDAP with API keys (#520): `ApiKeyOpcUaUserAuthenticator` (no directory I/O; peppered HMAC; uniform deny), keys/grants ride the artifact to driver-only nodes (`OpcUaPublishActor.FeedDataPlaneAuthority` installs ahead of the `plan.IsEmpty` early-return, :477-483), and `Security:Acl:Enabled` gates full per-node enforcement: `DataPlaneAuthorizer` (fail-closed on every path, key revocation re-checked outside the cache on every call — `DataPlaneAuthorizer.cs:238-248`), the sharded per-key `SessionAclDecisionCache` with per-shard generation stamps, the monotonic-counter auth generation (`DataPlaneAuthority.cs:304-317`), browse transit incl. the shared-root anchor fix at HEAD (`DataPlaneAuthorizer.cs:278-289, 382-405` — Raw-realm-qualified, Browse-only, fail-closed in the degenerate collision case), and deploy-triggered monitored-item revalidation that detaches rather than disables (`OtOpcUaNodeManager.cs:3235-3286`, denial queued *before* detach).
|
||
|
||
**Deploys** became surgical (classifier + `ISurgicalAddressSpaceSink`, default-closed, ratchet-to-rebuild), the value hot path became batched end-to-end, and the runtime gained the comms-loss watchdog (#527), the alarm-S&F drain gate, and the historization capture gate (#537) / quarantine (#538).
|
||
|
||
---
|
||
|
||
## Findings
|
||
|
||
Severity scale: **Critical** (correctness/availability broken in a realistic scenario) · **High** (significant risk or debt, plan a fix) · **Medium** (real but bounded) · **Low** (hygiene). Closed findings live only in the status table; still-open findings keep their IDs; new findings continue the scheme (S14+, P5+, C5+, U11+).
|
||
|
||
### 1. Stability
|
||
|
||
**S14 — CRITICAL (NEW): The AdminUI cookie `FallbackPolicy` blocks every mesh gRPC ingress on a login-enabled admin node — and every live gate ran where the blind spot is masked.**
|
||
`AddOtOpcUaAuth` sets a global `FallbackPolicy = RequireAuthenticatedUser` under the cookie scheme (`Security/ServiceCollectionExtensions.cs:143-148`), registered on `hasAdmin` nodes (`Host/Program.cs:520-523`) with `UseAuthentication`/`UseAuthorization` in the pipeline (:732-733). ASP.NET Core applies the fallback policy to every endpoint without explicit authorization metadata — gRPC endpoints included — and none of the mesh gRPC maps carry `AllowAnonymous`: `MapZbLocalDbSync` (:749), `DeploymentArtifactGrpcService` + `CentralCommandGrpcService` (:757, :767), `TelemetryStreamGrpcService` + `NodeCommandGrpcService` (:782-783). The PSK bearer is not an ASP.NET authentication scheme and gRPC interceptors run *inside* the endpoint invocation, i.e. after the authorization middleware — so a correctly-keyed node call is challenged by the cookie handler before the interceptor's verdict matters. Consequences on a production (login-enabled) central: every `FetchAndCache` site node's artifact fetch fails on all endpoints → every fleet deploy fails (fail-closed to last-known-good, per #485 — loud, not corrupting, but a fleet-wide deploy-plane outage); every gRPC `ReportApply` ack is dropped → deploys seal `TimedOut`; central↔fused-pair telemetry and LocalDb sync fail. The codebase already knows this exact trap — `app.MapStaticAssets().AllowAnonymous()` exists because the fallback "401s every CSS/JS request" (:724-728), and the deploy API is explicitly exempted (`AdminUI/Api/DeployApiEndpoints.cs:22`) — the gRPC maps just never got the same treatment. **Why nothing caught it:** the docker-dev rig runs `Security__Auth__DisableLogin=true`, which registers an always-succeeding handler *under the cookie scheme name* so the FallbackPolicy passes (`Security/ServiceCollectionExtensions.cs:82-98`) — every mesh live gate therefore ran with the blocking middleware neutralized; and `MeshGrpcBoundaryTests` builds a minimal pipeline with interceptors but no auth stack (`tests/Server/.../MeshGrpcBoundaryTests.cs:334-346`). Driver-only site nodes are unaffected (no auth services → no authorization middleware), which makes central's serve side the fleet-wide choke point.
|
||
*Recommendation:* `.AllowAnonymous()` on each mesh gRPC map (the fail-closed interceptors are the real gate, exactly as the map-site comments at :754-778 already argue), plus one rig/gate run with `DisableLogin=false` to prove the production middleware stack end-to-end.
|
||
|
||
**S15 — HIGH (NEW): Driver comms loss never marks value variables Bad — external OPC UA clients hold stale-Good for the whole outage.**
|
||
`OnDriverConnectivityChanged` (`DriverHostActor.cs:1248-1283`) annotates the driver's Part 9 alarm conditions (`BadNoCommunication`) and fans a synthetic Bad batch to the *dependency mux* so scripts/virtual tags see the outage (`FanCommsLossToDependencyMux`, :1334-1359 — the #481 fix). The plain raw/UNS value variables get nothing, and the code's own rationale names the motivating shape: "a poll driver whose device goes unreachable publishes nothing at all" (:1240-1245). For such a driver, an external client subscribed to a value node holds the last Good value with a stale timestamp for the entire outage, until a *successful* reconnect republishes. Internal consumers were fixed; the external data plane — the product's whole point — was not. Serving stale-Good to SCADA clients is the classic quality-flag failure this repo's own status-code workstreams (#497/#519) exist to prevent.
|
||
*Recommendation:* extend the comms-loss fan-out to write `BadNoCommunication` (carrying last-known value) to the driver's raw NodeIds + UNS mirrors through the same batched sink path, and force-republish on recovery (the value-dedup trap from the Batch 4 live gate is the known hazard to avoid).
|
||
|
||
**S16 — HIGH (NEW): Operator "Restart driver" produces a subscription-less child until the next deploy; `_lastComposition` is write-only.**
|
||
`HandleRestartDriver` (`DriverHostActor.cs:2916-2932`) stops and respawns from the cached spec but never re-sends `SetDesiredSubscriptions` — the only sender is the apply-time loop (:2660-2672). The fresh child starts with empty `_desiredRefs`, the #488 reconcile can't help (`DriverInstanceActor.cs:625` requires a non-empty set), so the driver reads/subscribes nothing and raises no alarms until the next deployment dispatch. Tellingly, `_lastComposition` ("the composition from the most-recent apply") is declared at :349, assigned at :2709, and read **nowhere** — it is exactly the state a restart re-push was meant to consume and never was: the house built-but-never-wired shape, and the live face of S6.
|
||
*Recommendation:* re-run `PushDesiredSubscriptions` for the instance after `SpawnChild` in the restart path (the state is already cached); then S6's remaining exposure is only the exception-restart path.
|
||
|
||
**S17 — MEDIUM (NEW): The comms-loss watchdog's post-reconnect grace collapses to one health tick.**
|
||
`DriverInstanceActor.cs:574`: `var lastEvidence = observed.LastSuccessfulRead ?? _connectedSinceUtc;`. Drivers deliberately *preserve* `LastSuccessfulRead` across Degraded/Faulted writes (e.g. `ModbusDriver.cs:213, 511, 518` — that preservation is the #527 design), so after a watchdog trip and reconnect against a half-broken device (TCP connects, reads still fail), the first Connected tick sees non-Healthy + a read stamp frozen from before the outage — already > 90 s — and re-trips immediately. The comment at :458-459 ("measured from THIS connect, not from a stale earlier one") describes `max(LastSuccessfulRead, _connectedSinceUtc)`; the code implements `??`, which only helps a never-read driver. Result: reconnect churn at health-tick cadence (~30 s) instead of the designed 90 s grace per connection. Otherwise the watchdog checks out exactly as documented (conjunction at :571-576, single `GetHealth` observation feeding snapshot + watchdog :514-521, disabled at ≤ 0, throwing `GetHealth` never advances it).
|
||
*Recommendation:* one-line fix — take the max.
|
||
|
||
**S18 — MEDIUM (NEW): API-key lookup is a per-activation SQL round-trip on admin/fused nodes, inside the SessionManager-wide lock, with no timeout.**
|
||
The authenticator's "no network I/O on this path at all" (`ApiKeyOpcUaUserAuthenticator.cs:22`) is strictly true only on driver-only nodes (`ArtifactApiKeyStore`, in-memory). On admin/fused nodes `ConfigDbApiKeyStore.FindAsync`/`FindGrantsAsync` (`ConfigDbApiKeyStore.cs:90-115`) run an EF query per activation, block-bridged under the SDK's SessionManager event lock with `CancellationToken.None` (`OpcUaApplicationHost.cs:317-319`), and none of the LDAP-era bound/concurrency/outage-circuit machinery survived the #520 deletion (`OpcUaApplicationHost.cs:269-275` records the removal). A hung — not refusing — ConfigDb serializes all session activations up to the SqlClient timeout each. Far smaller exposure than the LDAP bind this replaced, but the structural hazard S2 described is not zero on fused centrals.
|
||
*Recommendation:* a short per-call timeout in `ConfigDbApiKeyStore`, or a small TTL cache (revocation stays immediate for ACL-enforced deployments because `DataPlaneAuthorizer.Evaluate` re-checks the key per operation — `DataPlaneAuthorizer.cs:238-248`).
|
||
|
||
**S19 — MEDIUM (NEW): Key *expiry* never reaches live monitored items between deploys.**
|
||
`RevalidateMonitoredItems` is deploy-triggered only — the code says so explicitly ("this is the only trigger", `OtOpcUaNodeManager.cs:320-328`). Request-shaped operations re-check expiry per call and the decision cache deliberately excludes the key check for exactly this reason (`SessionAclDecisionCache.cs:20-28`), but a subscription authorized under a then-valid key keeps streaming after `ExpiresUtc` passes, until the next deployment happens to advance the generation. On a quiet fleet that is indefinite — and streaming is how machine consumers actually read this server (the workstream's own rationale for stage 6 revalidation). Revocation-by-deploy is covered; revocation-by-clock is not.
|
||
*Recommendation:* a low-frequency timer (minutes) calling `RevalidateMonitoredItems` — the pass is already built, memo is already pass-local (the comment at :322-325 anticipates exactly this).
|
||
|
||
**S5 — MEDIUM (carried, narrowed): redundancy-identity mismatch diagnostic exists in one consumer of three.** See the status table; add the same once-per-process Warning to `OpcUaPublishActor` (ServiceLevel silently 0 is the historically expensive silent state) and `ScriptedAlarmHostActor`.
|
||
|
||
**S6 — MEDIUM (carried):** no supervision strategy / restart re-push anywhere; see S16 for the operator-visible face and the ready-made fix.
|
||
|
||
**S7 — MEDIUM (carried):** `PostStop` block-bridge (`DriverInstanceActor.cs:1327`); slightly more load-bearing than ever since stop+respawn is the path for ResilienceConfig changes and operator restarts.
|
||
|
||
**S8 — MEDIUM (carried):** at-most-once Part 9 command routing with optimistic `Good` and no drop metric (`OtOpcUaNodeManager.cs:1409-1413`; `OtOpcUaServerHostedService.cs:154-209`).
|
||
|
||
**S9 — MEDIUM (carried):** no certificate expiry monitoring; the health-endpoint surface would be the natural home (`Host/Health/HealthEndpoints.cs:34-95`).
|
||
|
||
**S10 — LOW (carried):** SDK start failure swallowed, invisible to `/health` (`OtOpcUaServerHostedService.cs:129-144`).
|
||
|
||
**S11 — LOW (carried, narrowed):** audit flush drops batches on DB outage (`AuditWriterActor.cs:90-116`); now non-moot since producers exist.
|
||
|
||
**S20 — LOW (NEW): `RemoveEquipmentSubtree` never severs cross-realm inverse `Organizes` references.**
|
||
The d787502d fix (`SeverOutgoingReferences`, `OtOpcUaNodeManager.cs:2640, 2663-2676`) covers only the single-node `RemoveVariableNode`; the subtree path (:2725-2738) removes UNS reference variables leaving the surviving raw node with a dangling inverse edge — the exact accumulation hazard the fix's own comment (:2629-2639) describes. Currently masked because P5 forces a rebuild whenever the scenario arises; **fixing P5 without adding severing here exposes S20 immediately — fix them together.**
|
||
|
||
**S21 — LOW (NEW): TOCTOU on `_authorizer` during shutdown.** `AclEnforced` (`OtOpcUaNodeManager.cs:372`) reads the volatile field once; gated bodies re-dereference `_authorizer!` (:1480-1483, :2865-2867, :3284, :3330, :3337, :3414) while `StopAsync` nulls it (`OtOpcUaServerHostedService.cs:297`) — an in-flight Read/Browse during stop can NRE out of an SDK service call. Snapshot to a local per seam.
|
||
|
||
**S22 — LOW (NEW): terminal-Bad write failures are counted into `failedNodes` inconsistently** — counted at `AddressSpaceApplier.cs:102, 107, 300, 374, 430`; discarded at :380, :389, :419, :435. The :300 counting can needlessly hold back `_lastApplied` and trigger the re-diff→ratchet rebuild for a node whose remove succeeded; the discarding sites under-report. Pick one semantics.
|
||
|
||
**S23 — LOW (NEW): #538 quarantine counters never age.** `_tagWriteFailures` clears only on that tag's own success (`ContinuousHistorizationRecorder.cs:752`), so N isolated blips weeks apart eventually count as "consecutive" and abandon samples that were never a poison pattern; abandoned-tag entries are retained forever (bounded only by tag-name cardinality). The core #538 discriminator logic itself is exactly as designed (outage pass touches nothing :767-775; counters advance only when a sibling wrote :789-798; abandonment deferred until every failing tag is over budget :800-806; shutdown-cancellation rethrown :707-716).
|
||
|
||
**S24 — LOW (NEW): the two node→central endpoint walkers fail over only on `RpcException`.** A non-Rpc throw (e.g. `GrpcChannel.ForAddress` on a malformed endpoint — the validator checks only the scheme prefix, `MeshTransportOptionsValidator.cs:107-117`) escapes `GrpcApplyAckSender.PostAsync`'s per-endpoint catch (`Runtime/Communication/GrpcApplyAckSender.cs:212-217`) and skips the remaining endpoints; same shape in `GrpcDeploymentArtifactFetcher.FetchAsync` (`Runtime/Deployment/GrpcDeploymentArtifactFetcher.cs:104-109`, bounded there by the null-fetch failure mapping).
|
||
|
||
**Positive stability notes (worth preserving):** The #520 core is the best-defended code in the subsystem: fail-closed on every authorizer path with an explicit allow-is-possible test posture; key revocation deliberately outside and above the decision cache (`DataPlaneAuthorizer.cs:238-248`); the monotonic generation counter with the hash-derivation trap documented and pinned (`DataPlaneAuthority.cs:304-317`); per-shard generation stamps making the cache's rollover window inert (`SessionAclDecisionCache.cs:35-43, 483-496`); trie-owned-by-authority-state making the never-installed-trie mistake unrepresentable (`Program.cs:380-386`); browse transit as a non-accumulating marker rather than synthesized grants (`PermissionTrie.cs:84-96`), with the HEAD shared-root fix correctly realm-qualified and fail-closed in its degenerate collision case; revalidation queues the denial before detaching and is reversible (`OtOpcUaNodeManager.cs:3260-3277`). The PSK perimeter is uniformly fail-closed (unset/whitespace key rejects all — the #543 fix; `FixedTimeEquals` everywhere; no key material logged; interceptor registration exactly matches service mapping, `Program.cs:586-602` vs :747-784) — S14 is a middleware-ordering defect *around* it, not a hole *in* it. ConfigSource honors "no answer ≠ empty config" end-to-end (`GrpcDeploymentArtifactFetcher.cs:89-97`; `DriverHostActor.cs:1905-1928`; gzip sniff + 512 MiB guard, `DeploymentArtifact.cs:133, 164-196`). `TelemetryDialSupervisor` is generation-stamped, leak-free on node removal, and never gives up (`AdminUI/Telemetry/TelemetryDialSupervisor.cs:204-286, 354-362`). The bootstrap guard, singleton scoping (options-not-services), and Phase 4 DB-less wiring all check out against their documented rationale.
|
||
|
||
### 2. Performance
|
||
|
||
**P5 — MEDIUM (NEW): `ApplyPureRemove` lacks the subsumption filter for UNS reference variables, so removing any equipment that holds references self-defeats into a full rebuild.**
|
||
The equipment-subtree remove runs first (`AddressSpaceApplier.cs:402-405`) and subsumes the equipment's UNS reference-variable nodes; the planner independently emits those nodes in `RemovedUnsReferenceVariables` (`AddressSpacePlan.cs:216-219`), and the loop over them (:417-421) has no `NotSubsumed` filter — unlike tags/vtags/alarms (:367, :386, :395). `SafeRemoveVariable` on the already-removed node returns false ⇒ ratchet ⇒ `SafeRebuild()`. Correct but maximally expensive: every equipment-with-references removal severs every client subscription — exactly what the surgical-deploys work exists to avoid. Fix: apply the subsumption filter (by the reference's equipment) to the :417 loop — and land S20's severing in the same change, since P5's rebuild is what currently masks S20.
|
||
|
||
**P6 — MEDIUM (NEW, filing the known gap): `AnnounceAddedNodes` is Uns-only — surgically-added raw containers/tags are materialised but never announced.**
|
||
`ComputeAddAnnouncements` collects only UNS parents and the announce call hardcodes `AddressSpaceRealm.Uns` (`AddressSpaceApplier.cs:519-531, 576`); the sink member is already realm-aware (`RaiseNodesAddedModelChange(string, AddressSpaceRealm)`, `OtOpcUaNodeManager.cs:2316`), so the fix is confined to the applier. Values publish and writes route fine (the routing maps rebuild from the full composition every apply, `DriverHostActor.cs:2594-2605`), but added raw nodes are undiscoverable to connected model-aware clients until a manual re-browse. The code comment itself says "a real follow-up, not an accepted limitation" (`OpcUaPublishActor.cs:562-563`) — no tracking issue exists; file one.
|
||
|
||
**P3 — MEDIUM (carried, narrowed):** HistoryRead-Events still returns the full window in one response with no continuation points (`OtOpcUaNodeManager.cs:3635-3675`), now bounded by the S3 deadline + limiter.
|
||
|
||
**Positive performance notes:** the batched value pipeline is the right shape end-to-end (single ingress, fan-out inside the batch, chunked lock holds with validation outside — `OtOpcUaNodeManager.cs:555-641`); the apply path parses the artifact once (`OpcUaPublishActor.cs:465-475`), reads Direct-mode artifacts off the host mailbox with an explicit timeout, and dedups stashed dispatches with an `Applying`-wedge watchdog (warn-only, honestly); delta materialisation makes adding one tag O(delta) instead of O(address space); pinned-dispatcher throughput raised 1→100 without ordering impact.
|
||
|
||
### 3. Conventions
|
||
|
||
**C5 — MEDIUM (NEW): host role wiring reads only `OTOPCUA_ROLES` while the mesh reads effective `Cluster:Roles` — divergence boots a role-less host that gossips roles.**
|
||
`Program.cs:61-63` derives `hasAdmin`/`hasDriver` solely from the env var; the effective Akka roles prefer `Cluster:Roles` with env fallback (`Cluster/RoleParser.cs:115-119`). A node configured only via `Cluster:Roles` (the exact knob the wonder-app-vd03 field fix hand-added, per `RoleParser.cs:96-103`) joins the mesh carrying `driver`/`cluster-*` — passing `SplitTopologyTransportValidator` and `ConfigSourceOptionsValidator`, which read effective roles — yet registers no runtime actors, no ConfigDb/LocalDb, and the deny-all authenticator. Central counts it as a driver node (deploys time out against it) and it can even be the oldest role-carrying member for singleton placement without running the singleton manager. This is the exact inverse of the #532 incident, and no validator cross-checks the two sources.
|
||
*Recommendation:* fail (or Error-log) at boot when the `Cluster:Roles`-derived set ≠ the `OTOPCUA_ROLES`-derived set actually wired.
|
||
|
||
**C6 — LOW (NEW): the primary-gate XML summaries still document the pre-R2-04 default-allow.** `HandleRouteNodeWrite`'s summary says "default-allow while the role is unknown, so single-node deploys + the boot window never reject" (`DriverHostActor.cs:1449-1451`; native ack :1542), contradicting the multi-driver default-DENY the bodies implement (:1465-1468, :1553-1556). A future editor reading only the summaries could "restore" the S4 bug believing it documented behavior. Same class: `EnsureVariable`'s stale historize-intent comment (U9), `AuditOutcomeMapper`'s stale dormancy remark (U3), `BuildSecurityPolicies`' phantom log (U8).
|
||
|
||
**C7 — LOW (NEW): endpoint routing is listener-agnostic** — no `RequireHost` anywhere, so every mapped gRPC service is also reachable on the main HTTP surface (and via Traefik on fused nodes), and health/metrics are served unauthenticated on the dedicated h2c ports (`Program.cs:786-787`; `AllowAnonymous` by design in the shared libs). No auth bypass — interceptors are path-scoped, not listener-scoped — but the exposure surface is wider than the port-per-purpose docs imply; one doc line for firewall authors.
|
||
|
||
**C1 — MEDIUM (carried, narrowed):** four imperative `ServerHistorian` binds remain despite the validator. **C2 — MEDIUM (carried, narrowed):** `ContinuousHistorization` still warn-only with a silent `CommitMode` fallback; `DevStubMode` still log-warn-only (AdminUI-scope now). **C3 — LOW (carried):** the static-logger island grew slightly (4 sites in Runtime SCE + `DpsScriptLogPublisher.cs:61`; 14 node-manager pragma blocks), with `LocalDbSetup` the one documented-legitimate case.
|
||
|
||
**Positive conventions notes:** the new perimeter code is uniformly disciplined — every PSK interceptor shares the same fail-closed template with the same header parsing (`GrpcRequestHeaders.cs:44-69`); the options-not-services rule (`AkkaClusterOptions`, never `IClusterRoleInfo`, at composition time) is now stated and honored at three seams (`ConfigDbApiKeyStore.cs:72-78`, `ControlPlane/ServiceCollectionExtensions.cs:234-248`, Program.cs:419-427); validators name the key and allowed values on failure (`MeshTransportOptionsValidator.cs:49-55`); the classifier's `with`-equality whitelist makes future-field safety structural rather than reviewed-in; and `LocalDbSetup`'s registration-follows-config with idempotent deregister self-heal (`LocalDbSetup.cs:171-179, 209-248`) is the pattern at its best.
|
||
|
||
### 4. Underdeveloped areas
|
||
|
||
**U11 — MEDIUM (NEW): the `Backing*`-field parity invariant is convention-only.**
|
||
The UNS-mirror staleness repair depends on every raw-tag attribute the surgical whitelist covers having a `Backing*` counterpart on `UnsReferenceVariable` (so the planner emits the coincident `UnsReferenceDelta`). The mechanism is verified and pinned for the four current fields (`AddressSpacePlannerDualNamespaceTests.cs:233-269`), but nothing mechanical ties the classifier whitelist (`AddressSpaceChangeClassifier.cs:174-185`) to the `Backing*` set — CLAUDE.md's own warning ("omitting one silently reopens the staleness") is enforced by comments alone. Mitigating factor: widening the whitelist also widens the fixed-arity `UpdateTagAttributes` signature, so a human touches the seam — but that is precisely the "plan names N−1 dispatch sites" trap this repo has already paid for.
|
||
*Recommendation:* a reflection guard in the `DeferredSinkForwardingReflectionTests` style — assert every non-identity whitelisted field has a same-named `Backing*` property, or a planner property test perturbing each whitelisted field and asserting a coincident delta.
|
||
|
||
**U12 — MEDIUM (NEW): a surgical historize-flip or tagname change never fires `EnsureTags` provisioning.**
|
||
`ProvisionHistorizedTags` iterates `plan.AddedRawTags` only (`AddressSpaceApplier.cs:597`), while `FeedHistorizedRefs` *does* handle changed tags (:699-706) and starts the recorder capturing the new ref. Flipping `isHistorized` on an existing tag — now a routine first-class surgical edit — captures values against a historian tag that may not exist; `WriteLiveValues` fails until the #538 quarantine abandons the tag's entries. Pre-existing (rebuilds also only provisioned adds; the documented Issue-5 limitation), but surgical deploys made it routine-reachable. Extend provisioning to the changed-ref add side of the delta.
|
||
|
||
**U13 — LOW (NEW): the telemetry dial's "connected" pill requires a first received event** (`TelemetryDialSupervisor.cs:236-243`; `TelemetryLocalHub.cs:16-22` replays only what was emitted), so a healthy stream from an empty node reads as down. Cosmetic.
|
||
|
||
**U14 — LOW (NEW): `RevalidateMonitoredItems` counts restorations but never reports them** (`OtOpcUaNodeManager.cs:3241, 3264` vs :3282-3286 — only stand-downs are logged); grant-restored feed resumptions are invisible to operators.
|
||
|
||
**U3 — MEDIUM (carried, narrowed):** the audit pipeline has producers now but the mapper has none, and the new producers bypass it — decide whether `AuditOutcomeMapper` is the contract or dead weight; the ACL gate remains a natural producer that was never wired.
|
||
|
||
**U5 — MEDIUM (carried, narrowed):** native Confirm/Shelve route to the scripted engine (upstream no-op) and Enable/Disable stay `BadNotSupported` (`OtOpcUaNodeManager.cs:1177-1231`).
|
||
|
||
**U6 — MEDIUM (carried, reshaped):** the two biggest prior gaps (DPS delivery, ACL over-the-wire) are now tested; the remaining structural gaps are (1) still no actor supervision/restart test anywhere (S6/S16 would both have been caught), and (2) the **DisableLogin masking problem** — the production auth middleware stack has never been exercised against the mesh (S14). A single gate run with `DisableLogin=false` retires an entire class.
|
||
|
||
**U7 — LOW (carried):** `IHistoryWriter` Null-wired; H2 HistoryUpdate unimplemented. **U8 — LOW (carried, mitigated):** fix the `BuildSecurityPolicies` comment.
|
||
|
||
---
|
||
|
||
## Maturity ratings
|
||
|
||
| Dimension | 2026-07-12 | 2026-08-19 | Justification |
|
||
|---|---|---|---|
|
||
| Stability | **3** | **3** | The three carried Highs all closed for real (S2 by deletion, S3 by async fan-out, S4 by default-deny — each verified in code and live-gated), and the new watchdog/gates/quarantine machinery is carefully built. But the fresh code contributed one Critical (S14 — fleet-wide deploy-plane outage on a login-enabled central, masked by every rig gate) and two Highs (S15 stale-Good on comms loss; S16 restart yields a dead driver), all three in the house failure-mode families. Better where it was weak; newly exposed where it grew. |
|
||
| Performance | **3** | **4** | P1 substantially closed (surgical deploys, default-closed with a rebuild ratchet), P2 closed (end-to-end batching with bounded lock holds), P4 closed (delta materialisation). Residuals are bounded and enumerable (P5 subsumption gap, P6 announce gap, P3 events paging, container removes). The hot paths are now designed rather than inherited. |
|
||
| Conventions | **4** | **4** | The new perimeter code is the pattern at its best (uniform fail-closed interceptor template, options-not-services stated at three seams, structural future-field safety in the classifier). Held down by the growing doc-drift class (C6 — stale summaries that document *reverted* behavior are a regression vector, not hygiene), the C5 dual-role-source divergence, and the slightly grown static-logger island. |
|
||
| Underdeveloped | **4** | **3.5** | Strong closes: U4 deleted, U9/U10 closed, U2 extended to the surgical members, audit gained producers, DPS delivery + over-the-wire ACL now tested. But the dominant new theme is guards that cannot see their own blind spot: every mesh live gate ran with the auth middleware neutralized (S14), the Backing*-parity invariant is comment-enforced (U11), `_lastComposition` shipped write-only (S16), and provisioning silently skips the newly-routine edit shape (U12). The half-step down reflects that the *class*, retired in round 2 for sinks and DI, re-emerged in the new subsystems. |
|
||
|
||
---
|
||
|
||
## Cross-cutting themes
|
||
|
||
1. **A dev-mode auth bypass in the rig makes every live gate blind to the production middleware.** S14 is the sharpest instance of the house "verification can't see its own blind spot" mode this review has recorded: `DisableLogin=true` registers a pass-everything handler *under the cookie scheme name*, so the FallbackPolicy — the exact thing that breaks the mesh in production — passes on every rig run, and the boundary tests build pipelines without the auth stack at all. The rig's convenience feature is load-bearing for the gap. One gate run with `DisableLogin=false` (and a standing CI variant of `MeshGrpcBoundaryTests` that includes `UseAuthentication`/`UseAuthorization` + the FallbackPolicy) retires the class.
|
||
2. **Deleting a subsystem deletes its protections; audit what still needed them.** #520 removed the LDAP authenticator and, with it, the timeout/concurrency/outage-circuit machinery — correctly, for the in-memory artifact store, but the ConfigDb store re-introduced network I/O into the same synchronous, lock-holding callback with none of the guards (S18). When a replacement changes the I/O profile per-node-role, the old protections need a per-role disposition, not a wholesale delete.
|
||
3. **Doc drift is now a regression vector, not hygiene.** The gates' XML summaries still describe default-allow (C6), `EnsureVariable` still claims the silent ignore (U9), the audit mapper claims dormancy that is no longer true (U3). Each stale comment describes *the exact behavior a past fix reverted*, which is the worst kind: it invites a confident restoration of the bug. A doc-sweep pass over the three named sites is cheap; the habit of updating the summary in the same commit as the body is the real fix.
|
||
4. **The fail-closed ratchet is doing its job — now trim what it over-catches.** P5, S22, and the container-remove hard-false all resolve to "correct but rebuilds": the one-way ratchet made every surgical gap safe, which is the design working. The next increment of value is not new machinery but subsumption/accounting fixes (P5+S20 together, S22) so the ratchet fires only when it must.
|
||
5. **Convention-enforced invariants in the new subsystems need the mechanical guards the old ones got.** Round 2 retired the built-but-never-wired class for sinks (reflection forwarding) and DI (registration guards). The new code re-created the shape in three places — Backing* parity (U11), `_lastComposition` (S16), announce realm coverage (P6) — each of which admits the same style of cheap structural guard that already proved itself.
|