37 KiB
Architecture Review — Server & Runtime Subsystem
- Date: 2026-07-12
- Commit:
f6eaa267(master, clean tree) - Updates: the 2026-07-08 review at
9cad9ed0. Since then all arch-review remediation branches merged to master (~40 commits), including the S1 SBR work + #11 premise correction, the #9 hard-kill failover test, the U2 deferred-sink reflection guard, and the #10/#13 CapabilityInvoker dispatch wiring + per-instance ResilienceConfig. - Scope:
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer,.Host,.Runtime,.ControlPlane,.Security, plus the sink/publisher wrappers insrc/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa, cluster/SBR behavior insrc/Core/ZB.MOM.WW.OtOpcUa.Cluster, andtests/Serverfor coverage assessment. - Dimensions: Stability · Performance · Conventions · Underdeveloped areas
Prior-finding status (9cad9ed0 → f6eaa267)
Every finding from the 2026-07-08 review, verified against the actual code (not just STATUS.md). Production code in OpcUaServer, ControlPlane, Security, and Commons/OpcUa is byte-identical since 9cad9ed0 (only Host/Drivers/DriverFactoryBootstrap.cs, Host/Engines/*, Runtime/Drivers/*, Runtime/ServiceCollectionExtensions.cs, and Cluster/* changed), so most findings carry over verbatim with shifted line numbers noted.
| ID | One-liner | Verified status |
|---|---|---|
| S1 | SBR "configured but never activated"; cluster runs NoDowning | INVALID PREMISE, GAP CLOSED ANYWAY (#11 + #9). The original Critical was factually wrong: Akka.Cluster.Hosting's WithClustering applies SplitBrainResolverOption.Default when the typed option is null, which registers the SBR downing provider that reads the pre-existing akka.conf keep-oldest block — so SBR was already active on master and hard-crash failover already worked; the cluster was never NoDowning. Proven by the #9 negative control (only an explicit downing-provider-class = "" breaks failover). What was real: the strategy was implicit-in-framework-default and hard-kill failover was untested. Both closed: BuildClusterOptions now sets KeepOldestOption { DownIfAlone = true } explicitly (Cluster/ServiceCollectionExtensions.cs:108-116, a81dea10) with corrected-premise docs in the XML comment + akka.conf:40-47 (eaf78aad), and HardKillFailoverTests + TwoNodeClusterHarness.HardKillNodeAAsync (transport shutdown → B downs the alone oldest and takes over as driver role-leader, ~35 s, stable-streak polled) guard the failover outcome regardless of activation path (a25c9ed0). The Failover trait is not excluded by the CI filter (v2-ci.yml:61), so it runs in the whole-solution leg. |
| S2 | LDAP auth blocks SDK session-activation thread, no timeout | STILL OPEN. OpcUaApplicationHost.cs:271-274 still block-bridges with CancellationToken.None; LdapOpcUaUserAuthenticator.cs:30-48 adds no timeout; LdapOptions.ToLibraryOptions() (Security/Ldap/LdapOptions.cs:94-116) still projects no timeout option. No remediation branch existed. |
| S3 | HistoryRead block-bridges gateway per node, sequentially | STILL OPEN. OtOpcUaNodeManager.cs:1911, 2059, 2171, 2203 unchanged — per-node .GetAwaiter().GetResult() with CancellationToken.None, bounded only by the 30 s gateway CallTimeout against MaxRequestThreadCount = 100. |
| S4 | Primary-gate default-allow while role unknown (dual-primary window) | STILL OPEN. DriverHostActor.cs:977 (alarm emit), :1039 (write gate), :1095 (ack gate) still gate on _localRole is Secondary or Detached with null → allow; OnRedundancyStateChanged :1126. Listed in STATUS.md's "suggested next", not started. |
| S5 | Redundancy NodeId string-match, no mismatch diagnostic | STILL OPEN. OpcUaPublishActor.cs:512 unchanged; no log when the local node is absent from a snapshot. |
| S6 | Driver-child restart loses desired-subscription state; no backoff supervision | STILL OPEN. Repo-wide grep: still zero SupervisorStrategy overrides in Runtime/ControlPlane; desired-subs still arrive post-spawn into actor state (DriverInstanceActor.cs:84, 190-203, 912). Still no supervision/restart test. |
| S7 | PostStop blocks on ShutdownAsync(CancellationToken.None) |
STILL OPEN. Now at DriverInstanceActor.cs:1009. |
| S8 | Part 9 commands / native acks at-most-once, client sees Good | STILL OPEN. OtOpcUaServerHostedService.cs:139-194 routers unchanged; no drop metric. |
| S9 | No server-cert renewal/expiry monitoring | STILL OPEN. OpcUaApplicationHost.cs:305-317 unchanged. |
| S10 | SDK start failure swallowed; node runs with Null sinks, invisible to /health | STILL OPEN. OtOpcUaServerHostedService.cs:110-129 unchanged. |
| S11 | AuditWriterActor unbounded buffer, drops batches on outage |
STILL OPEN (and still moot — see U3). |
| P1 | Any structural deploy change → full rebuild severs all monitored items | STILL OPEN. AddressSpaceApplier.cs:134-154 / OtOpcUaNodeManager.cs:1690-1736 unchanged. The U2 reflection guard (now merged) was the declared prerequisite gate for the surgical pure-adds work; the work itself has not started. |
| P2 | Per-value global-lock write, no batching | STILL OPEN. OtOpcUaNodeManager.cs:266-281 unchanged. Same guard-unblocked, not-started status as P1. |
| P3 | HistoryRead-Events unpaged, 0 → int.MaxValue |
STILL OPEN. OtOpcUaNodeManager.cs:1914-1954 unchanged. |
| P4 | Deploy re-runs four Materialise passes (low) | STILL OPEN. OpcUaPublishActor.cs:338-354 unchanged. |
| C1 | ServerHistorian bound imperatively in five places |
STILL OPEN. Program.cs:121, Runtime/ServiceCollectionExtensions.cs:132, 168 (+ AddAlarmHistorian), OtOpcUaServerHostedService.cs:89 — all unchanged. |
| C2 | Two-tier options validation; DevStubMode only log-warned in prod |
STILL OPEN. OtOpcUaLdapAuthService.cs:79-88 unchanged. |
| C3 | Static Serilog logger islands + node-manager CS0618 pragmas | STILL OPEN. Runtime/ServiceCollectionExtensions.cs:90, 100, 136 unchanged. (The new resilience code in DriverFactoryBootstrap correctly uses ILoggerFactory — the convention is honored by new code, not retrofitted.) |
| C4 | IHistorianProvisioning resolve has no missing-registration warning |
STILL OPEN. Now Runtime/ServiceCollectionExtensions.cs:303 (shifted +5). The new invoker-factory resolve (:219-220) is likewise silent, but deliberately so (admin nodes legitimately lack it) and is backstopped by the new DI guard test — see positive notes. |
| U1 | CLAUDE.md "KNOWN LIMITATION 2" stale (ref-feed gap closed in code) | FIXED. Rewritten on the docs branch (9fadead6, merged b67bd9e8): now reads "value-capture path is code-complete… treat continuous value-capture as unproven-in-production rather than absent". The residual recommendation (explicit restart-convergence test) remains open — folded into U6. |
| U2 | Deferred-sink forwarding trap half test-guarded | FIXED. DeferredSinkForwardingReflectionTests (tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/, a65c2ced, merged 1d7e7caa): reflects over every method of every forwarding interface of DeferredAddressSpaceSink + DeferredServiceLevelPublisher, invokes each against a DispatchProxy recording inner (auto-covers future members), asserts arrival, plus a guard-of-the-guard pinning the wrapper's interface set. Negative control verified per STATUS. This retires the F10b/PR#423 trap class and unblocks P1/P2. |
| U3 | Structured audit pipeline has zero producers | STILL OPEN. AuditOutcomeMapper.cs:12-18 / Security/Audit/AuditActor.cs:18-24 unchanged. |
| U4 | FleetStatusBroadcaster heartbeat dead code + host-only NodeId key |
STILL OPEN. FleetStatusBroadcaster.cs:38, 110-121, 152-159 unchanged. |
| U5 | Native Part 9 conditions Acknowledge-only to driver | STILL OPEN. OtOpcUaNodeManager.cs:647-658, 698-703 unchanged. |
| U6 | Test gaps concentrate on the fragile seams | PARTIALLY FIXED. Closed: hard-kill failover (#9, the biggest single blind spot); whole-solution CI leg + fail-on-skip gate + CLI deflake (10b89830, v2-ci.yml:58-61 + scripts/ci/assert-not-all-skipped.sh) — green-because-all-skipped is now detected. Still open: DPS delivery of redundancy/ServiceLevel state untested over a real mediator; no actor supervision/restart test (S6); outbox restart-durability untested; recorder restart-convergence untested (from U1); OpcUaServer.IntegrationTests still contains exactly one test (DualEndpointTests). |
| U7 | IHistoryWriter Null-wired; H2 HistoryUpdate unimplemented; LdapAuthFailure overload |
STILL OPEN (tracked backlog). |
| U8 | BuildSecurityPolicies doc claims a log that doesn't exist |
STILL OPEN. OpcUaApplicationHost.cs:376-418 unchanged. |
| U9 | EnsureVariable historize-intent invariant lives in comments |
STILL OPEN. OtOpcUaNodeManager.cs:1345-1349 unchanged. |
Architecture overview
Host composition
Program.cs (src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs) is a single fused binary whose wiring is driven by the OTOPCUA_ROLES env var (admin, driver, or both). Notable mechanics: per-role appsettings overlays with env-vars/args re-appended so deployment overrides keep precedence (Program.cs:61-73); CWD anchored to AppContext.BaseDirectory for Windows-service path sanity (:83); Serilog via the shared AddZbSerilog with the static Log.Logger assigned post-build so Akka's SerilogLogger routes actor logs (:305).
On driver nodes the host registers the historian stack (three config-gated layers plus the ContinuousHistorization outbox/writer pair), the driver-factory registry (8 drivers, DriverFactoryBootstrap.cs), the Phase 6.1 resilience infrastructure (new since 9cad9ed0: process-singleton DriverResilienceStatusTracker + DriverResiliencePipelineBuilder + the IDriverCapabilityInvokerFactory closing over DriverFactoryRegistry.GetTier, DriverFactoryBootstrap.cs:53-71), the LDAP data-plane authenticator, and the late-binding seams: DeferredAddressSpaceSink / DeferredServiceLevelPublisher are DI singletons that actors capture at construction; OtOpcUaServerHostedService swaps real SDK-backed implementations in after StandardServer start and Nulls back on stop (OtOpcUaServerHostedService.cs:131, 226-231, 241-253).
OPC UA server & node manager
Unchanged since 9cad9ed0. OtOpcUaSdkServer (a StandardServer) creates the single OtOpcUaNodeManager (CustomNodeManager2, 2 424 lines) owning the entire writable address space: UNS folders, typed equipment-tag variables, Part 9 AlarmConditionState nodes, historized-node registration, four HistoryRead override arms. Reverse paths are Akka-free delegates set by the Host at start (AlarmCommandRouter, NativeAlarmAckRouter, NodeWriteGateway, HistorianDataSource). Inbound writes are role-gated (WriteOperate), dispatched fire-and-forget under the SDK Lock, optimistically applied, self-corrected on failed device outcome.
Runtime actor topology (per driver node)
WithOtOpcUaRuntimeActors (Runtime/ServiceCollectionExtensions.cs:196-357) spawns: DbHealthProbeActor → DependencyMuxActor → (gated) ContinuousHistorizationRecorder → OpcUaPublishActor (pinned dispatcher, owns AddressSpaceApplier) → PeerProbeSupervisor → DriverHostActor → HistorianAdapterActor. DriverHostActor spawns one DriverInstanceActor per deployed driver plus the VirtualTag/ScriptedAlarm sub-hosts.
New since 9cad9ed0 — the resilience dispatch seam (#10/#13): DriverInstanceActor now routes all six of its driver-capability call sites through an injected IDriverCapabilityInvoker (Core.Abstractions; Runtime stays deliberately Polly-free — verified: the csproj still references only Core.Abstractions, never Core): Write via ExecuteWriteAsync (DriverInstanceActor.cs:597), alarm-ack AlarmAcknowledge (:650), subscribe/unsubscribe Subscribe (:693, :722), alarm-subscribe AlarmSubscribe (:803), discover Discover (:868). Single-reference calls key the per-host breaker via IPerCallHostResolver (:957; implemented by Modbus/AbCip/AbLegacy/FOCAS/TwinCAT with a safe fall-back-to-instance-id); bulk calls key on the instance id. Per-instance ResilienceConfig from the deploy artifact (DeploymentArtifact.cs:171-185) is layered onto tier defaults by DriverCapabilityInvokerFactory.Create, which also Invalidates the instance's cached pipelines so a respawn with changed options rebuilds them; DriverSpawnPlanner forces stop+respawn on a ResilienceConfig change while a pure DriverConfig change stays an in-place delta (DriverSpawnPlan.cs:45-56). Generation-suffixed actor names (DriverHostActor.cs:1666-1670, pre-existing) make the stop→respawn name-collision-safe, and the apply path re-pushes desired subscriptions after reconcile (:1215-1225), so a respawned child converges. The OTOPCUA0001 analyzer (wired tree-wide via Directory.Build.props:48) is the compile-time guard that every guarded call stays wrapped; ResilienceInvokerFactoryRegistrationTests guards that production DI binds the real factory, not the pass-through.
No actor in Runtime overrides SupervisorStrategy — everything still runs under Akka's default one-for-one unlimited restart decider (S6).
Redundancy model
Non-transparent warm redundancy, unchanged in code; the understanding of it is corrected (S1/#11). RedundancyStateActor derives Primary/Secondary from Akka's driver role-leader, debounces 250 ms, publishes on DPS redundancy-state, re-publishes on a 10 s heartbeat. The split-brain resolver is active and now explicit: BuildClusterOptions sets KeepOldestOption { DownIfAlone = true } (correct for a 2-node pair; keep-majority/static-quorum would be wrong), consistent with the akka.conf block (stable-after = 15s, which only HOCON can express). Hard-crash failover is proven by HardKillFailoverTests (~35 s in-process). There is still no lease or fencing token — correctness rests on Akka's single-role-leader guarantee within a converged cluster, and the boot-window default-allow (S4) remains the data-plane soft spot.
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). Fixed/invalidated findings live only in the status table above; still-open findings keep their IDs; new findings continue the scheme.
1. Stability
S2 — HIGH: LDAP authentication blocks the OPC UA session-activation thread with an effectively non-cancellable, non-configurable timeout.
The SDK invokes ImpersonateUser synchronously; OpcUaApplicationHost.HandleImpersonation block-bridges the authenticator (OpcUaApplicationHost.cs:271-274) with CancellationToken.None. The doc comment shifts timeout responsibility to the authenticator, but LdapOpcUaUserAuthenticator adds none (Host/OpcUa/LdapOpcUaUserAuthenticator.cs:30-48), and the shared library's AuthenticateAsync is synchronous-in-Task.FromResult; its 10 s connect timeout is a library default that LdapOptions.ToLibraryOptions() does not project or expose (Security/Ldap/LdapOptions.cs:94-116). Every authentication opens a fresh TCP connection + service-account bind + search + user bind. Under a directory outage, session activations stall serially ~10-20 s each on SDK threads.
Recommendation: enforce a hard timeout in LdapOpcUaUserAuthenticator (Task.WhenAny + deny on timeout), surface Security:Ldap:TimeoutMs, consider a short negative-cache. Fail-closed semantics are otherwise correct end-to-end.
S3 — HIGH: HistoryRead block-bridges the gateway per node, sequentially, on SDK request threads.
All four HistoryRead arms call .GetAwaiter().GetResult() per node handle with CancellationToken.None (OtOpcUaNodeManager.cs:1911, 2059, 2171, 2203). Safe w.r.t. the node-manager Lock (the overrides run outside it — correctly documented), but bounded only by the gateway client's 30 s CallTimeout. A single request naming N historized nodes against a slow historian holds one SDK request thread up to N × 30 s against MaxRequestThreadCount = 100 / MaxQueuedRequestCount = 200 (OpcUaApplicationHost.cs:329-331) — a handful of misbehaving history clients can exhaust the pool and degrade all OPC UA services.
Recommendation: parallelize per-node reads within a batch (bounded), pass a per-request deadline token, add a server-side concurrent-HistoryRead limiter.
S4 — HIGH: Primary-gate "default-allow while role unknown" opens a dual-primary window on the data plane.
DriverHostActor gates native-alarm emission, HandleRouteNodeWrite, and HandleRouteNativeAlarmAck on _localRole is Secondary or Detached and deliberately defaults to allow until the first redundancy snapshot arrives (DriverHostActor.cs:977, 1039, 1095; OnRedundancyStateChanged :1126). Combined with at-most-once DPS delivery heartbeat-repaired every 10 s, a freshly-booted or snapshot-missed secondary services device writes and emits alerts as if primary for up to the heartbeat interval — and indefinitely if the NodeId identity mismatch (S5) bites. The boot-window rationale (single-node deploys) is sound, but the same signal gates shared-field-device writes. The now-proven SBR failover (S1) does not narrow this window — it is a delivery/boot-order gap, not a downing gap.
Recommendation: default-deny for the write/ack gates once the cluster has >1 driver member, or require one received snapshot before servicing writes on multi-node clusters. (Named in STATUS.md's "suggested next" — not started.)
S5 — MEDIUM: Redundancy NodeId identity is string-matched across three independently-derived sources with no diagnostic on mismatch.
OpcUaPublishActor matches n.NodeId == _localNode.Value (OpcUaPublishActor.cs:512); the snapshot side derives host:port from the gossiped Member.Address (RedundancyStateActor.cs:147-148), the local side from configured PublicHostname:Port. Divergence (DNS vs IP, container advertised name) silently computes ServiceLevel 0 / stale role — the shape of the historical "silently inert delivery" bug, indistinguishable from legitimate Detached.
Recommendation: log-once (Warning) when the local node is absent from a received snapshot; startup identity self-check.
S6 — MEDIUM: A crashed-and-restarted DriverInstanceActor loses its message-delivered state (desired subscriptions) until the next deploy.
Still zero SupervisorStrategy overrides in Runtime (grep re-confirmed at f6eaa267): a throwing driver child restarts one-for-one, unlimited, no backoff. Restart re-runs PreStart but the desired-subscription set arrives post-spawn via SetDesiredSubscriptions stored in actor state (DriverInstanceActor.cs:84, 190-203, 912); a restart wipes it and the parent has no restart-detection to re-send. A persistently-throwing driver hot-loops. Note the new invoker does not change this: retries happen inside a single message handler; a handler that ultimately throws still escalates to the default decider.
Recommendation: BackoffSupervisor for driver children + child-requests-state-on-restart (or parent push on PostRestart). Still no supervision test (U6).
S7 — MEDIUM: DriverInstanceActor.PostStop blocks the actor shutdown path on driver shutdown.
_driver.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult() (DriverInstanceActor.cs:1009) remains the only synchronous block in the Runtime actors; a hung protocol stack stalls stop/re-deploy of the whole child set. Slightly more load-bearing now: #13 makes stop+respawn the path for every ResilienceConfig edit.
Recommendation: bound it (WaitAsync(timeout)), log-and-abandon on expiry.
S8 — MEDIUM: Inbound Part 9 commands and native acks are at-most-once; the client sees Good even if the command is lost.
Unchanged (OtOpcUaServerHostedService.cs:139-194; OtOpcUaNodeManager.cs:769-772). Deliberate (non-blocking under Lock), but invisible in operation.
Recommendation: counter/metric for dropped routes; engine-side reconciliation sweep.
S9 — MEDIUM: Certificate lifecycle has no renewal or expiry monitoring.
Unchanged (OpcUaApplicationHost.cs:305-317): self-signed, 12-month lifetime, checked only at boot. ~12 months after first deploy, Sign/SignAndEncrypt endpoints and UserName-token encryption fail with no warning.
Recommendation: startup + periodic expiry check surfaced via health/metric; rotation runbook.
S10 — LOW: SDK start failure is swallowed and the node keeps running with Null sinks (OtOpcUaServerHostedService.cs:110-129); nothing surfaces it through /health.
S11 — LOW: AuditWriterActor buffer is unbounded between flushes and drops whole batches on DB outage (ControlPlane/Audit/AuditWriterActor.cs:35, 83, 113-116). Explicitly best-effort; currently moot (U3: no producers).
S12 — MEDIUM (NEW): The write dispatch site hardcodes isIdempotent: true, bypassing the per-tag WriteIdempotentAttribute contract.
DriverInstanceActor.HandleWriteTagAttribute routes through _invoker.ExecuteWriteAsync(ResolveHost(msg.TagId), isIdempotent: true, …) (DriverInstanceActor.cs:597-601). The WriteIdempotentAttribute (Core.Abstractions/WriteIdempotentAttribute.cs) — whose whole purpose is per-tag opt-in to write retries, and which DriverCapability.Write's doc and ExecuteWriteAsync's non-idempotent arm are built around — has zero readers anywhere in the tree. Today this is behavior-neutral (the Write tier default is RetryCount = 0, and the site comment says so), but the moment an operator sets a Write retry in a per-instance ResilienceConfig (#13 made exactly this reachable), retries apply to every write on that instance — including command-shaped tags (write-1-to-pulse, PLC auto-resets) where a duplicate write double-triggers. The hardcode silently converts a per-tag safety contract into an instance-wide one.
Recommendation: either resolve idempotence per tag (plumb the attribute/tag-config through WriteTagAttribute) before honoring a configured Write retry, or document loudly on the ResilienceConfig authoring surface that Write.RetryCount > 0 retries all writes on the instance. Until then the safest change is isIdempotent: false with per-tag opt-in later — that keeps the non-idempotent no-retry arm authoritative.
S13 — LOW (NEW): Respawn-on-ResilienceConfig-change has a narrow stale-pipeline re-cache race.
DriverCapabilityInvokerFactory.Create invalidates the instance's cached pipelines (DriverCapabilityInvokerFactory.cs:71) before the old child has terminated (StopChild is an async Context.Stop, DriverHostActor.cs:1278-1280, 1658-1664). An old-child handler already dequeued and mid-await can reach its invoker call after the invalidate; GetOrCreate then re-caches a pipeline built from the old child's captured options snapshot under the same (instance, host, capability) key — and the cache ignores options on a hit (DriverResiliencePipelineBuilder.cs:75-79), so the new child silently serves the stale pipeline until the next respawn. Window is milliseconds (the mailbox suspends on Stop; a pipeline is resolved once per ExecuteAsync at call start), but the failure is the exact "options-blind cache hit" the invalidate was added to prevent.
Recommendation: include an options fingerprint in PipelineKey (removes the invalidate-ordering dependency entirely), or invalidate from the new invoker lazily on first use after the old child's Terminated.
Positive stability notes (worth preserving): the node-manager locking discipline remains exemplary (every mutation under Lock, every ReportEvent outside it, write-dispatch continuation forced off the SDK thread, revert guarded by ShouldRevert). The new resilience wiring is carefully done: exception transparency holds (pipeline exceptions — TimeoutRejectedException, BrokenCircuitException — surface through the same catch (Exception) arms the raw calls used, so failure paths like WriteAttributeResult(false, …) and Reconnecting transitions are unchanged); retry ShouldHandle excludes OperationCanceledException, so the actor's outer cts deadlines (5 s write/ack/unsub, 10 s subscribe, 30 s discover) still hard-bound mailbox suspension even with retries enabled; the Discover site's ConfigureAwait analysis (invoker-internal ConfigureAwait(false) does not leak to the caller's actor-context resume) is correct and covered by a real-CapabilityInvoker actor-context test; pipeline order Timeout→Retry→Breaker means the tier timeout bounds the total including retries. The TwoNodeClusterHarness.HardKillNodeAAsync design (transport shutdown as crash sim, stable-streak takeover polling, diagnostic timeout message naming the inactive-resolver signature) is a model integration test.
2. Performance
P1 — HIGH: Any structural deploy change triggers a full address-space teardown/rebuild that severs every client's monitored items server-wide.
Unchanged (AddressSpaceApplier.cs:134-154; OtOpcUaNodeManager.cs:1690-1736): any added/removed equipment, alarm, tag, or virtual tag forces RebuildAddressSpace(); recreated nodes are new NodeState instances, so existing subscriptions go dead. The F10b surgical path covers only attribute edits and renames. Adding one tag still drops every subscription on a production SCADA server. The declared prerequisite (U2 reflection forwarding guard) is now merged — the surgical pure-adds work is unblocked but not started.
Recommendation: extend the surgical path to pure-add deltas first (EnsureFolder/EnsureVariable are already idempotent; RaiseNodesAddedModelChange exists), then scope removals per-equipment. Still the highest-leverage item in the subsystem.
P2 — MEDIUM: The value hot path is one global lock acquisition + one actor message per value, with no batching.
Unchanged: driver child Tell → ForwardToMux → OpcUaPublishActor → sink.WriteValue taking the global node-manager Lock per value (OtOpcUaNodeManager.cs:266-281). Same unblocked-by-U2, not-started status as P1.
Recommendation: batched WriteValues(IReadOnlyList<…>) sink call — the reflection guard now auto-covers the new member's forwarding.
P3 — MEDIUM: HistoryRead-Events has no paging and translates NumValuesPerNode == 0 to int.MaxValue (OtOpcUaNodeManager.cs:1944-1954; no continuation points :1914-1921). Unchanged.
P4 — LOW: Every deploy re-runs the four Materialise* passes over the full composition (OpcUaPublishActor.cs:338-354) — acceptable (early-return on existing ids); fold into P2 if it profiles hot. Positive: Raw-paging tie-cluster over-fetch stays bounded; HandleDiscoveredNodes unchanged-plan short-circuit preserved. New-code note: the resilience pipeline adds one GetOrCreate dictionary hit + options-snapshot closure per capability call on dispatch paths only (write/ack/subscribe/discover) — the per-value publish hot path is untouched by #10, which is the right call.
3. Conventions
C1 — MEDIUM: The ServerHistorian section is bound imperatively in five places with no IOptions registration.
Unchanged: Program.cs:121, AddServerHistorian/AddAlarmHistorian/AddHistorianProvisioning (Runtime/ServiceCollectionExtensions.cs:132, 168 et al.), OtOpcUaServerHostedService.cs:89.
Recommendation: one AddValidatedOptions<ServerHistorianOptions> + IOptions<> everywhere, matching the existing OpcUa/Ldap pattern.
C2 — MEDIUM: Options validation is two-tier and the tiers disagree.
Unchanged: OpcUa/Security:Ldap get fail-fast ValidateOnStart validators; ServerHistorian/ContinuousHistorization/AlarmHistorian get advisory warning lists; DevStubMode=true is merely log-warned in production (OtOpcUaLdapAuthService.cs:79-88).
Recommendation: promote the historian sections to the validator pattern; environment-gate DevStubMode.
C3 — LOW: Library code logs via the static Serilog logger (Runtime/ServiceCollectionExtensions.cs:90, 100, 136) and the node manager keeps six #pragma warning disable CS0618 Utils.LogError sites. Unchanged — though notably the new resilience registrations do it right (ILoggerFactory-created categorized loggers, DriverFactoryBootstrap.cs:58-70), so the island isn't growing.
C4 — LOW: WithOtOpcUaRuntimeActors service-locator fallbacks are inconsistently warned. resolver.GetService<IHistorianProvisioning>() (Runtime/ServiceCollectionExtensions.cs:303) still has no missing-registration warning despite being the seam that shipped dormant once. The new IDriverCapabilityInvokerFactory resolve (:219-220) is likewise silent-fallback, but is the better-defended instance of the pattern: the fallback is deliberate for admin-only nodes, and ResilienceInvokerFactoryRegistrationTests pins the driver-node bind to the real factory — the guard the provisioning seam still lacks.
Positive conventions notes: the layering discipline held under pressure — #10's filed plan (thread CapabilityInvoker directly into Runtime) would have broken the Polly-free boundary, and the shipped seam (IDriverCapabilityInvoker/IDriverCapabilityInvokerFactory in Core.Abstractions, mirroring IDriverFactory, Null-object defaults, DI-resolved with pass-through fallback) is exactly this codebase's established pattern; the Runtime csproj still references only Core.Abstractions. The corrected S1 docs are a conventions win too: BuildClusterOptions's XML comment and akka.conf now state precisely why the typed option exists, what activates SBR, the keep-oldest-for-2-nodes rationale, and the HOCON/typed-option consistency constraint. Layer inventory unchanged otherwise: Akka-free OpcUaServer, SDK-free Runtime, pure calculators in Core, uniform Props factories + registry keys, scoped-mapper handling in the singleton authenticator.
4. Underdeveloped areas
U3 — MEDIUM: The structured audit pipeline is fully built, tested, and has zero producers. Unchanged (ControlPlane/Audit/AuditOutcomeMapper.cs:12-18; Security/Audit/AuditActor.cs:18-24). Wire producers or delete.
U4 — MEDIUM: FleetStatusBroadcaster.DriverHostStatusHeartbeat is dead code with a latent NodeId-mismatch repeat. Unchanged (FleetStatusBroadcaster.cs:38, 110-121; host-only key :152-159 vs host:port everywhere else).
U5 — MEDIUM: Native Part 9 conditions support Acknowledge-to-driver only. Unchanged (OtOpcUaNodeManager.cs:647-658; Enable/Disable BadNotSupported :698-703). Shelving a Galaxy alarm from a Part 9 client remains silently ineffective upstream.
U6 — MEDIUM: Test-coverage gaps still concentrate on the fragile seams — though the two worst are now closed.
Closed since 9cad9ed0: hard-kill failover (HardKillFailoverTests, in-process 2-node, CI-runnable — the single biggest prior blind spot) and green-because-skipped (whole-solution CI leg + assert-not-all-skipped.sh fail-on-skip gate). Still open, in priority order:
- DPS delivery of redundancy/ServiceLevel state is untested —
RedundancyStateActorTestsstub the broadcast;ServiceLevelEndToEndTestsinject state directly; the harness (even hard-kill) asserts cluster membership, not thatRedundancyStateChangedarrives and flips ServiceLevel/gates on the survivor. The known "unit tests can't catch it" blind spot survives the failover test. - No actor supervision/restart test anywhere (S6).
- Outbox durability across process restart and recorder restart-convergence (the U1 residual) untested.
OpcUaServer.IntegrationTestsstill contains one test (DualEndpointTests) — no over-the-wire security-mode matrix, subscription, or HistoryRead round-trip.- Resilience wiring behavioral coverage is 2 of 6 sites (
DriverInstanceActorResilienceWiringTests: Subscribe + Write-with-resolved-host). The analyzer proves the other four are wrapped but not that they pass the rightDriverCapability/host key — a transposed enum member at the ack or alarm-subscribe site would compile, pass the analyzer, and ship.
U7 — LOW: IHistoryWriter permanently Null-wired; H2 HistoryUpdate unimplemented (OtOpcUaNodeManager.cs:1797-1801); LdapAuthFailure lacks DirectoryUnavailable. Tracked backlog, unchanged.
U8 — LOW: BuildSecurityPolicies doc-comment claims the empty-profile fallback is "logged and very visible" but the method logs nothing (OpcUaApplicationHost.cs:376-418). Unchanged.
U9 — LOW: EnsureVariable silently ignores changed historize-intent on an existing node (OtOpcUaNodeManager.cs:1345-1349) — invariant still lives in two files' comments rather than an assert.
U10 — LOW (NEW): The DriverResilienceStatusTracker has no reader — pipeline observability is log-only. — ✅ REMEDIATED (R2-10, branch r2/10-resilience-observability). The tracker now has a production reader: a periodic (5 s) full-snapshot publisher (DriverResilienceStatusPublisherService, driver nodes, registered next to the tracker with a wiring guard ResilienceStatusReaderRegistrationTests) → driver-resilience-status DPS topic → per-admin-node DriverResilienceStatusBridge → IDriverResilienceStatusStore → a resilience section on DriverStatusPanel.razor (breaker-OPEN / consecutive-failures / in-flight / staleness chips, read in-process per the self-HubConnection ban). Two tracker truthfulness gaps fixed along the way: RecordBreakerClose + IsBreakerOpen (breaker-open state is now derivable) and RecordSuccess wired onto the invoker success paths (ConsecutiveFailures is now a true consecutive counter). Mirrors the driver-health flow one-for-one. (Original finding:) The tracker was registered, fed by every retry/breaker event, and consumed by nothing (Admin /hosts Stream E.2/E.3 never shipped); the interim mitigation was retry/breaker logging in DriverResiliencePipelineBuilder.cs.
Live gate (T10) pending: docker-dev breaker-open observation on both centrals + staleness dim.
Maturity ratings
| Dimension | 2026-07-08 | 2026-07-12 | Justification |
|---|---|---|---|
| Stability | 3 | 3 | The prior Critical dissolved (S1's premise was wrong — SBR was always active — and hard-kill failover is now integration-proven), which removes the cluster-wide failure mode from the ledger. But the three untouched Highs (S2 LDAP thread-stall, S3 HistoryRead pool exhaustion, S4 dual-primary boot window) are exactly the realistic production exposures the band describes, and the new S12 idempotence bypass adds a loaded footgun. Better-evidenced, not yet safer where it counts. |
| Performance | 3 | 3 | Nothing on the hot paths changed. P1 (rebuild severs all subscriptions) and P2 (per-value global lock) remain; their prerequisite guard merged, so the score is now held down purely by unstarted work. The resilience pipeline correctly stayed off the per-value publish path. |
| Conventions | 4 | 4 | The new code is the pattern at its best (Polly-free seam mirroring IDriverFactory, Null-object + DI-guard, corrected load-bearing docs), and the static-logger island stopped growing — but C1/C2/C3/C4 themselves are all still open, so the score holds rather than rises. |
| Underdeveloped | 3 | 4 | The two systemic traps are retired with durable guards (U2 reflection forwarding test closes the F10b/PR#423 class; fail-on-skip CI closes green-because-skipped), U1's doc drift is fixed, the once-dormant CapabilityInvoker is wired end-to-end with analyzer + DI + behavioral guards, and the worst coverage blind spot (hard-kill failover) is tested. Remaining: dormant audit/heartbeat code (U3/U4), the DPS-delivery and supervision test gaps, and the reader-less tracker (U10). |
Cross-cutting themes
- The review's own premises need negative controls too. S1 was filed as Critical on a correct-looking static read (no
downing-provider-classanywhere) that missed a framework default; only the #9 negative control (forcedowning-provider-class = "") exposed it. The corrected framing is now pinned in code comments,akka.conf, and a test that guards the outcome regardless of activation path — the right durable form. Treat "config X is inert" findings as unproven until a negative control flips the observed behavior. - The Deferred/Null seam pattern's trap class is now guarded, not just known. The reflection-exhaustive forwarding test (U2) auto-covers future sink/publisher members via
DispatchProxyplus a guard-of-the-guard on the interface set — new capabilities (P1 surgical adds, P2WriteValues) can no longer ship inert through the wrappers. The same "built-but-never-wired" failure mode got a second guard at the DI layer (ResilienceInvokerFactoryRegistrationTests). The remaining unguarded instance of the pattern is the silentIHistorianProvisioningfallback (C4). - "Default-allow until told otherwise" gates remain the compounding risk. S4 (primary write gate) + at-most-once DPS delivery + the untested delivery path (U6) still stack; the SBR work hardened membership convergence but none of the delivery-dependent gates. S12 joins this family:
isIdempotent: trueis a default-permissive stance on a safety contract, dormant only because a different default (RetryCount=0) currently masks it. - Unit-test greenness overstating distributed correctness is being paid down in the right order — hard-kill failover (done), fail-on-skip (done), DPS delivery / supervision restarts / outbox durability (still open, now the top of the list). The pattern to keep: every guard shipped with a verified negative control.