dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity
Files
a2039508481ecf82d5c785854167e929e71b8299
lmxopcua/archreview/plans/03-server-runtime-plan.md
T

72 KiB
Raw Blame History

Design & Implementation Plan — Server & Runtime Subsystem (Report 03)

Status (2026-07-08): ✅ S1 (Critical) activation DONE — branch fix/archreview-crit1-split-brain-resolver @ a81dea10 (typed KeepOldestOption, unit guards + e2e config-resolution verified). ✅ U1 doc DONE (CLAUDE.md KNOWN LIMITATION 2 corrected). ⬜ S1's hard-kill failover integration test still pending (task 9, effort-M). ✅ U2 (reflection forwarding test) DONE — branch fix/archreview-u2-deferred-sink-forwarding-guard @ a65c2ced (DispatchProxy-based exhaustive forwarding guard over IOpcUaAddressSpaceSink + ISurgicalAddressSpaceSink + DeferredServiceLevelPublisher; negative-control verified; the gate for P1/P2 is now in place). Remaining: P1, S2/S3/S4. See STATUS.md.

  • Source report: archreview/03-server-runtime.md
  • Review commit: 9cad9ed0
  • Verification pass performed against tree at: current master (same tree; all cited files present)
  • Scope: OpcUaServer, Host, Runtime, ControlPlane, Security; redundancy, LDAP, node manager, address-space applier.

Verification summary

ID Sev Status One-liner
S1 Critical Confirmed SBR configured in HOCON but never activated → NoDowning; hard-crash failover broken
S2 High Confirmed LDAP auth block-bridges session activation; no timeout, no pooling
S3 High Confirmed HistoryRead block-bridges gateway per node, sequentially, on SDK threads
S4 High Confirmed Primary write/ack gate defaults to allow while role unknown → dual-primary window
P1 High Confirmed Any structural add full-rebuilds address space, kills all subscriptions
S5 Medium Confirmed Redundancy NodeId string-matched across 3 sources, silent on mismatch
S6 Medium Confirmed No SupervisorStrategy anywhere in Runtime; restart wipes subscription state, hot-loops
S7 Medium Confirmed DriverInstanceActor.PostStop block-bridges ShutdownAsync(None)
S8 Medium Confirmed Inbound Part 9 / native-ack routes are at-most-once fire-and-forget, no metric
S9 Medium Confirmed Server cert 12-month lifetime, boot-only check, no expiry monitoring
S10 Low Confirmed SDK start failure swallowed; no health surface
S11 Low Confirmed AuditWriterActor unbounded buffer, drops batches (moot — no producers, U3)
P2 Medium Confirmed Per-value global-lock write, one actor msg per value, no batching
P3 Medium Confirmed HistoryRead-Events unbounded (NumValuesPerNode==0 → int.MaxValue), no paging
P4 Low Confirmed Every deploy re-runs 4 Materialise passes over full composition
C1 Medium Confirmed ServerHistorian bound imperatively in 5 places, no IOptions
C2 Medium Confirmed Two-tier options validation; DevStubMode only log-warned in prod
C3 Low Confirmed Library code logs via static Serilog; node manager uses obsolete Utils.LogError
C4 Low Confirmed IHistorianProvisioning resolve has no missing-registration warning
U1 High (doc) Confirmed stale doc CLAUDE.md "Known Limitation 2" stale — recorder ref-feed IS wired
U2 Medium Confirmed Deferred-sink forwarding correct today but only ~5/10 members test-guarded
U3 Medium Confirmed Audit pipeline built/tested, zero producers
U4 Medium Confirmed FleetStatusBroadcaster.DriverHostStatusHeartbeat dead code, host-only key
U5 Medium Confirmed Native Part 9 conditions: Acknowledge-to-driver only; Confirm/Shelve/Enable no-op upstream
U6 Medium Confirmed Test gaps concentrate on DPS delivery / supervision / hard-kill failover
U7 Low Confirmed IHistoryWriter Null-wired; H2 HistoryUpdate unimplemented; enum gap
U8 Low Confirmed BuildSecurityPolicies doc claims a log it doesn't emit
U9 Low Confirmed EnsureVariable silently ignores changed historize-intent (invariant in comments)

Nothing in this report is stale-because-already-fixed. The one "stale" item (U1) is stale documentation — the code it describes as broken is actually wired. Every other finding reproduces against the current tree.

Top 3 by priority: (1) S1 — activate the split-brain resolver (OVERALL action #1); (2) P1 — surgical pure-adds in the address-space applier (OVERALL action #8, highest operational perf win); (3) S4 + S2/S3 — the availability/blocking cluster (primary-gate default-deny, async LDAP, channelized HistoryRead; OVERALL actions #7/#9).

S1 — CRITICAL — Split-brain resolver configured but never activated

Restatement: akka.conf carries a split-brain-resolver { active-strategy = "keep-oldest" … } block but nothing registers a downing provider, so the cluster runs Akka's default NoDowning. Hard-crashed nodes stay unreachable forever → singletons + driver role-leader never fail over; a partition leaves both sides at ServiceLevel 240.

Verification — Confirmed.

  • src/Core/ZB.MOM.WW.OtOpcUa.Cluster/Resources/akka.conf:40-46 — the SBR block is present, inside akka.cluster { … }.
  • src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs:80-84 — WithClustering(new ClusterOptions { SeedNodes = …, Roles = … }) sets only those two properties; SplitBrainResolver is never assigned.
  • Repo-wide grep for downing-provider|SplitBrainResolver|KeepOldest|downing-provider-class returns zero hits (confirmed empty).
  • Packages: Akka.Cluster / Akka.Cluster.Hosting = 1.5.62 (Directory.Packages.props:6-13). This version ships Akka.Cluster.SBR.SplitBrainResolverProvider and the Akka.Cluster.Hosting ClusterOptions.SplitBrainResolver option surface.
  • min-nr-of-members = 1 (akka.conf:38) makes each partition side self-sufficient for leader election → the "both sides at 240" partition case is real.

Root cause: In Akka.NET the split-brain-resolver HOCON block is inert configuration — it only takes effect when akka.cluster.downing-provider-class points at Akka.Cluster.SBR.SplitBrainResolverProvider (or the Hosting ClusterOptions.SplitBrainResolver option, which sets that provider under the hood). The HOCON block was written but the activating registration was never added. Classic "config-in-HOCON is not config-in-effect" (OVERALL cross-cutting theme #1).

Proposed design. Prefer the Akka.Hosting typed option (ClusterOptions.SplitBrainResolver) over hand-writing downing-provider-class, because the rest of the cluster bootstrap already goes through WithClustering(ClusterOptions) and Akka.Hosting owns provider wiring — mixing a raw HOCON downing-provider-class with Hosting risks the same "Hosting doesn't honor HOCON" foot-gun already documented for akka.loggers (akka.conf:9-13). Keep the HOCON block as the tuning source (stable-after, keep-oldest.down-if-alone) — the SBR provider reads those keys once activated — OR move the strategy fully into the typed option and delete the HOCON block to avoid a two-source drift. Recommendation: use the typed option and keep HOCON only for values the typed option can't express, documenting which wins.

Strategy choice: keep-oldest with down-if-alone = on matches the current HOCON intent and is correct for a 2-node warm-redundancy pair (the oldest — typically the long-running primary — survives an even split; down-if-alone downs a singleton that loses quorum). keep-majority is wrong for 2 nodes (no majority in a 1-1 split). static-quorum would need quorum-size = 2 which defeats single-node deploys. Keep-oldest is the right call; document the reasoning.

stable-after = 15s must be ≥ the failure detector's acceptable-heartbeat-pause (10s) plus margin — current 15s is fine but tie it explicitly to down-removal-margin (also 15s) in a comment.

Implementation steps.

  1. src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs — in WithOtOpcUaClusterBootstrap, set the SBR option on the ClusterOptions: 
    builder.WithClustering(new ClusterOptions
{
    SeedNodes = options.SeedNodes,
    Roles = options.Roles,
    SplitBrainResolver = new KeepOldestOption { DownIfAlone = true }, // Akka.Cluster.Hosting
});
    (Confirm the exact type name against Akka.Cluster.Hosting 1.5.62 — it is KeepOldestOption : SplitBrainResolverOption, with DownIfAlone and optional Role. StableAfter is set on the option or left to the HOCON default.)
  2. Add a startup assertion log line proving SBR is active (OVERALL theme #1 recommendation). In the same method after WithClustering, register a small IActorRef-less startup hook (or log in WithOtOpcUaClusterBootstrap) that reads Context.System.Settings.Config.GetString("akka.cluster.downing-provider-class") at first MemberUp and logs Info if it equals the SBR provider, Error if empty. A cheap version: a tiny actor subscribed to ClusterEvent.IMemberEvent that logs the resolved downing provider once. This converts the silent-inert failure mode into an operator-visible signal.
  3. Reconcile the HOCON: either delete the split-brain-resolver block from akka.conf (typed option now owns it) OR keep it and add a comment that WithClustering(SplitBrainResolver=…) is what activates it. Do not leave both un-cross-referenced.
  4. Update docs/Redundancy.md and docs/v2/Architecture.md — document that SBR is now active, the keep-oldest rationale, and the hard-crash failover expectation.

Tests.

  • Unit: a config test asserting akka.cluster.downing-provider-class == "Akka.Cluster.SBR.SplitBrainResolverProvider, Akka.Cluster" after WithOtOpcUaClusterBootstrap runs (proves the typed option activated the provider). This is the "assert SBR is active" guard.
  • Integration (the load-bearing one): extend the 2-node harness (tests/Server/.../TwoNodeClusterHarness) with a hard-kill failover test: bring both nodes up, confirm role-leader on node A, kill -9-equivalent (abrupt ActorSystem terminate / drop the transport without CoordinatedShutdown), then assert within stable-after + margin that (a) node B becomes driver role-leader, (b) the singletons (RedundancyStateActor, ConfigPublishCoordinator, AdminOperationsActor, AuditWriterActor) hand over to B, (c) B's ServiceLevel rises to 240 and the (dead) A demotes. The current harness only exercises graceful StopAsync — this is exactly the U6 blind spot. If in-process abrupt-kill is hard to simulate deterministically, use Cluster.Get(sys).Down(selfAddress) suppression + transport fault injection, or run it as a nightly docker-host multi-process test.
  • Partition test (optional, nightly): simulate a 1-1 partition (block the transport between the two), assert exactly one side downs the other and only one side keeps 240.

Effort: S (the fix) + M (the failover test harness). Risk/blast-radius: Medium. The one-line option change alters cluster downing behavior globally — a mis-tuned stable-after could down a node during a transient GC pause. Mitigate by keeping stable-after ≥ acceptable-heartbeat-pause + margin and validating on the 2-node rig before merge.

P1 — HIGH — Structural deploy full-rebuilds the address space, severing every client subscription

Restatement: AddressSpaceApplier.Apply forces RebuildAddressSpace() for any added/removed/structurally-changed equipment, alarm, tag, or virtual tag. The rebuild removes and recreates every NodeState, so all existing client monitored items go dead server-wide. Adding one tag to one equipment drops every subscription on the server.

Verification — Confirmed.

  • AddressSpaceApplier.cs:134-140 — structuralRebuild is true whenever AddedEquipmentTags.Count > 0 || RemovedEquipmentTags.Count > 0 || AddedEquipment/AddedAlarms/… > 0. Pure adds fall into this branch.
  • AddressSpaceApplier.cs:150-154 — if (structuralRebuild) { SafeRebuild(); rebuilt = true; }.
  • OtOpcUaNodeManager.cs:1690-1736 — RebuildAddressSpace() clears _variables, _alarmConditions, _folders, _notifierFolders, _eventNotifierSources under one Lock, detaching/removing every NodeState.
  • OpcUaPublishActor.cs:338-354 — after Apply, the actor unconditionally re-runs the four Materialise* passes, which recreate all nodes from scratch. Existing subscriptions bind to the old (now-removed) NodeState instances → dead.
  • The surgical path (AddressSpaceApplier.cs:155-199) covers only changed tags (TagDeltaIsSurgicalEligible) and folder renames — not adds or removes.
  • Building blocks already exist and are idempotent: EnsureFolder (:1284), EnsureVariable (:1369, early-returns on existing id — U9 confirms), MaterialiseAlarmCondition (:586, idempotent guard :57), and RaiseNodesAddedModelChange (:1592).

Root cause: The applier's rebuild predicate treats "add" and "remove" identically to "structural change." But a pure add needs no teardown at all — the idempotent EnsureFolder/EnsureVariable/MaterialiseAlarmCondition passes add exactly the new nodes and no-op the existing ones; RaiseNodesAddedModelChange already exists to announce them to subscribed clients. The rebuild is unnecessary for the pure-add case and catastrophic for live subscriptions.

Proposed design — surgical pure-adds (phase 1), scoped removals (phase 2). This is the highest-leverage item in the subsystem (OVERALL action #8) and the report's own top recommendation. Do it in two phases so phase 1 (adds) ships value fast and low-risk.

Phase 1 — pure-add surgical path. Split the rebuild predicate: a rebuild is only required when there is a removal or a node-affecting change (the existing non-surgical-eligible change set). Pure adds route to the idempotent Materialise passes + a model-change announcement, no teardown:

  • New predicate:
    • requiresRebuild = RemovedEquipment/RemovedAlarms/RemovedEquipmentTags/RemovedEquipmentVirtualTags any > 0 OR ChangedEquipment.Count > 0 OR ChangedAlarms.Count > 0 OR ChangedEquipmentTags.Any(!surgicalEligible) OR ChangedEquipmentVirtualTags.Any(!nodeIrrelevant).
    • pureAdds = !requiresRebuild && (AddedEquipment/AddedAlarms/AddedEquipmentTags/AddedEquipmentVirtualTags any > 0).
  • When pureAdds (and not requiresRebuild): do not call RebuildAddressSpace(). The existing unconditional Materialise passes in OpcUaPublishActor already add only the new nodes (idempotent). Add a step that calls RaiseNodesAddedModelChange for each newly-added node/folder so subscribed clients get a GeneralModelChangeEvent and can re-browse.
  • Because OpcUaPublishActor currently re-runs the Materialise passes regardless, the safest minimal change is: in Apply, compute rebuilt=false for the pure-add case and rely on the downstream passes; then emit model-change events for the plan's added ids. Confirm the Materialise passes run after Apply returns (they do — OpcUaPublishActor.cs:335-354), so the added nodes exist before the announce; if announce must follow materialise, move the announce into the applier's post-materialise hook or have OpcUaPublishActor call a new applier.AnnounceAdds(plan) after the passes.

Phase 2 — scoped per-equipment removal. For removals, add surgical RemoveVariable(nodeId) / RemoveFolder(nodeId) / RemoveAlarmCondition(nodeId) methods to the node manager (mirroring the per-node teardown already inside RebuildAddressSpace's loops but scoped to one id), forwarded through ISurgicalAddressSpaceSink + DeferredAddressSpaceSink (respect the U2 forwarding trap — every new capability method MUST be added to the deferred wrapper and test-guarded). A remove of equipment E only tears down E's subtree, preserving every other equipment's subscriptions. Announce via RaiseNodesDeletedModelChange (add if the SDK exposes it; else a GeneralModelChangeEvent). Only fall back to a full rebuild for the genuinely-ambiguous changed-topology cases.

Alternatives considered: (a) Leave rebuild but re-attach subscriptions to new nodes — not feasible; the SDK binds monitored items to NodeState instances, and there's no supported re-home API. (b) Diff-and-patch every deploy generically — larger and riskier than the add/remove split; the plan already carries typed add/remove/change deltas, so use them. The add/remove split reuses the exact pattern F10b established for surgical changes.

Implementation steps.

  1. AddressSpaceApplier.cs — refactor the rebuild predicate into requiresRebuild / pureAdds; route pure adds away from SafeRebuild(). Add AnnounceAdds(AddressSpacePlan) calling _sink.RaiseNodesAddedModelChange(id) for each added folder/variable/alarm node id (compute ids via EquipmentNodeIds as the surgical change path already does at :177).
  2. OtOpcUaNodeManager.cs — Phase 2: add RemoveVariable/RemoveFolder/RemoveAlarmCondition (scoped teardown under Lock, drop the matching _historizedTagnames/_eventNotifierSources/_nativeAlarmNodeIds registrations); add RaiseNodesDeletedModelChange if available.
  3. ISurgicalAddressSpaceSink (+ SdkAddressSpaceSink, NullAddressSpaceSink, DeferredAddressSpaceSink) — Phase 2: add the remove methods with capability-sniffing forwarding, matching the existing UpdateTagAttributes/UpdateFolderDisplayName pattern (DeferredAddressSpaceSink.cs:58-69).
  4. OpcUaPublishActor.cs — ensure the announce runs after the Materialise passes (call _applier.AnnounceAdds(plan) after line 354, or fold the announce into Apply if ordering allows).

Tests.

  • Unit: applier test — a pure-add plan (one added tag) does not call RebuildAddressSpace (assert via a recording sink) and does call RaiseNodesAddedModelChange for the added id. A changed-only plan still routes surgical/rebuild as today. A remove plan (phase 2) calls RemoveVariable for the removed id and does not rebuild if no other change.
  • Live-verify (the decisive one — F10b/OpcUaServer-001 precedent proves unit-green is insufficient): on docker-dev (rebuild both central-1/2), subscribe a Client.CLI monitored item to an existing equipment tag (subscribe -n ns=2;s=…), then POST /api/deployments a config that adds one new tag to a different equipment. Assert the CLI subscription keeps delivering (no Bad/dead notifications) and the new tag becomes browsable. This is the exact regression the whole item exists to kill. Reuse the OpcUaServer-001 live-verify recipe from memory (project_full_codebase_review_2026-06-19.md).
  • Integration: OpcUaServer.IntegrationTests — over-the-wire subscribe-then-add-tag round-trip asserting subscription survival (also addresses U6's "one test only" gap).

Effort: L (phase 1 M, phase 2 M). Risk/blast-radius: High — this touches the address-space mutation core. Mitigate with the recording-sink unit tests + the mandatory subscription-survival live-verify + the U2 forwarding guard (a new remove capability that isn't forwarded through DeferredAddressSpaceSink ships inert, exactly like F10b).

S4 — HIGH — Primary gate defaults to allow while role unknown (dual-primary data-plane window)

Restatement: DriverHostActor's inbound-write / native-ack / native-alarm-emit gates gate on _localRole is Secondary or Detached and default to allow until the first redundancy snapshot arrives. A freshly-booted or snapshot-missed secondary services shared-field-device writes and emits alerts as primary for up to the 10s heartbeat interval — indefinitely if S5's NodeId mismatch bites.

Verification — Confirmed.

  • DriverHostActor.cs:195 — private RedundancyRole? _localRole; (nullable, null at boot).
  • HandleRouteNodeWrite (:1018-1026): if (_localRole is RedundancyRole.Secondary or RedundancyRole.Detached) { reject } — null falls through to allow.
  • HandleRouteNativeAlarmAck (:1074-1083): same pattern, drops on Secondary/Detached, null → allow.
  • Native-alarm emit (:956-984): if (_localRole is Secondary or Detached) continue; — null → emit.
  • OnRedundancyStateChanged (:1109-1116): a snapshot not mentioning this node leaves _localRole unchanged → stays null → default-allow.

Root cause: The boot-window default-allow is correct for single-node deploys (where no snapshot ever demotes) but wrong for multi-node clusters where a booting secondary has a real primary peer. The gate can't distinguish "single node, no peer" from "multi-node, snapshot not yet arrived." It uses the role signal to gate shared-device writes, and the role signal is under-determined at boot.

Proposed design — cluster-membership-aware default. Discover whether this is a multi-driver cluster from Akka cluster state (already available: OpcUaPublishActor.cs:81 does Akka.Cluster.Cluster.Get(Context.System)). Change the gate default per cluster size:

  • Single-driver cluster (≤1 member with the driver role): keep default-allow (preserves single-node deploys + boot window).
  • Multi-driver cluster (≥2 driver members): default-deny the write/ack gates until at least one redundancy snapshot has been received (_localRole is not null). Native-alarm condition writes stay ungated (the secondary keeps its address space warm — that's :953-954, correct); only the alerts publish + device write + device ack gate deny-on-unknown.

Implementation: DriverHostActor subscribes to ClusterEvent.IMemberEvent (or reads _cluster.State.Members on demand) and tracks driverMemberCount. The gate becomes:

bool servicedByThisNode = _localRole switch {
    RedundancyRole.Secondary or RedundancyRole.Detached => false,
    RedundancyRole.Primary => true,
    null => DriverMemberCount() <= 1,   // unknown: allow only on a single-node cluster
};

Add a _hasReceivedSnapshot flag (distinct from role, to handle the "snapshot arrived but didn't mention us" case → that's the S5 mismatch; log-once per S5 and deny on multi-node).

Alternatives considered: (a) Always default-deny — breaks single-node deploys (no snapshot ever arrives to enable writes). Rejected. (b) A fixed boot grace timer that flips to deny after N seconds — fragile, and a slow snapshot on a healthy multi-node cluster would wrongly deny. The membership-count approach is deterministic and uses signal already in hand. (c) Require a lease/fencing token — larger architectural change (the report notes "no lease or fencing token"); out of scope, membership-gate is the pragmatic fix.

Implementation steps.

  1. DriverHostActor.cs — add private readonly Akka.Cluster.Cluster _cluster = Akka.Cluster.Cluster.Get(Context.System); (mirror OpcUaPublishActor); add DriverMemberCount() counting Up members carrying the driver role from _cluster.State.Members.
  2. Introduce a single private bool ShouldServiceAsPrimary() helper encoding the switch above; call it from all three gate sites (:960, :1022, :1078) so the policy lives in one place.
  3. On a snapshot that omits this node while DriverMemberCount() > 1, log-once Warning (this is the S5 hook — the two findings share the diagnostic).
  4. Document the semantics in docs/Redundancy.md.

Tests.

  • Unit (TestKit): with _localRole == null and a stubbed single-driver membership → write is serviced; with _localRole == null and a two-driver membership → write is rejected ("not primary / role unknown on multi-node"); after a Primary snapshot → serviced; after a Secondary snapshot → rejected.
  • Integration: on the 2-node harness, boot the secondary and immediately issue a write before the first snapshot; assert it is rejected (not silently applied to the field device). This pairs with the S1 failover test.

Effort: S/M. Risk/blast-radius: Medium — changes write-admission policy on multi-node clusters; a bug could deny legitimate primary writes during a snapshot gap. Mitigate: the count check is conservative (only denies when a real peer exists) and the single-node path is untouched.

S2 — HIGH — LDAP authentication block-bridges session activation with a non-cancellable, non-configurable timeout

Restatement: The SDK invokes ImpersonateUser synchronously; OpcUaApplicationHost.HandleImpersonation block-bridges the authenticator with CancellationToken.None. The authenticator adds no timeout; the shared library's connect timeout is a default that LdapOptions.ToLibraryOptions() neither projects nor exposes. Each auth opens a fresh TCP connect + service bind + search + user bind. A DC outage stalls SDK threads ~10-20s each.

Verification — Confirmed.

  • OpcUaApplicationHost.cs:271-273 — authenticator.AuthenticateUserNameAsync(…, CancellationToken.None).GetAwaiter().GetResult(); on the SDK impersonation callback thread.
  • LdapOpcUaUserAuthenticator.cs:30-48 — awaits ldap.AuthenticateAsync(username, password, ct) with no timeout wrapper; the ct it receives from HandleImpersonation is None.
  • LdapOptions.cs:94-107 — ToLibraryOptions() projects Server/Port/Transport/… but no timeout field; LdapOptions has no TimeoutMs/ConnectTimeout property at all.
  • No connection pooling: each AuthenticateAsync is a full connect+bind+search+bind cycle (shared-lib behavior, per report).

Root cause: The timeout responsibility was pushed to the authenticator (per HandleImpersonation's doc comment) but no layer actually enforces one; the shared library's default connect timeout isn't surfaced or bounded per-call, and the call is synchronous under the SDK thread.

Proposed design — hard timeout at the OtOpcUa boundary + surfaced option. The clean fix lives in LdapOpcUaUserAuthenticator (OtOpcUa-owned; the shared library's sync-wrapped AuthenticateAsync can't be easily made cooperative). Add a Task.WhenAny(auth, Task.Delay(timeout)) wrapper that fails closed (deny) on timeout, and surface Security:Ldap:TimeoutMs on LdapOptions. Consider a short negative cache (deny cache, e.g. 5-30s TTL keyed by username) to shed load during a sustained outage — during a DC-down storm, repeated activations for the same user return the cached deny instantly instead of each stalling for the full timeout.

Note: Task.WhenAny + Task.Delay doesn't cancel the underlying blocking LDAP call (it observes CancellationToken.None only at entry), so the orphaned auth task runs to completion in the background — but the SDK thread is released at timeout, which is the goal. Cap concurrent in-flight auths (a SemaphoreSlim) so an outage can't accumulate unbounded orphaned tasks.

Alternatives considered: (a) Make the shared library cooperatively cancellable — cross-repo change to ZB.MOM.WW.Auth, larger blast radius, and the report frames the fix as OtOpcUa-side. Track as a shared-lib follow-up but don't block on it. (b) Connection pooling in the library — desirable but out of scope for this repo; note it. The boundary timeout + negative cache is the contained, correct-fail-closed fix.

Implementation steps.

  1. LdapOptions.cs — add public int TimeoutMs { get; set; } = 10000; (and optionally a NegativeCacheSeconds). Do not project into ToLibraryOptions() (it's an OtOpcUa-boundary concern) unless the library later exposes a matching field.
  2. LdapOpcUaUserAuthenticator.cs — inject IOptions<LdapOptions> (or the timeout value); wrap the ldap.AuthenticateAsync call in a Task.WhenAny(authTask, Task.Delay(TimeoutMs, ct)); on timeout return OpcUaUserAuthResult.Deny("Authentication timed out") and log Warning. Bound concurrency with a SemaphoreSlim(maxConcurrent). Add the optional negative cache (a MemoryCache or a small time-bucketed dictionary).
  3. Optionally pass a real (timeout-derived) CancellationToken from HandleImpersonation instead of None so the entry-check short-circuits — but the authoritative bound is the WhenAny.
  4. Add a validator note (C2): TimeoutMs > 0.

Tests.

  • Unit: authenticator with a stub ILdapAuthService that delays beyond TimeoutMs → returns Deny within ~TimeoutMs (not the full delay); a fast success still succeeds; the negative cache returns a prior deny without re-hitting the stub within TTL; the semaphore bounds concurrency. The existing LDAP fail-closed tests (deny-on-error, opaque messages, zero-role fallback) must stay green.
  • Live/manual: point Security:Ldap:Server at an unreachable host and drive a Client.CLI connect with a UserName token; confirm the activation fails fast (~TimeoutMs) rather than hanging ~10s, and that a burst of activations doesn't serialize.

Effort: M. Risk/blast-radius: Low-Medium — fail-closed on timeout preserves the existing deny-on-error posture; the risk is a too-short default timeout denying a legitimately-slow DC (default 10s matches the library default, so behavior is unchanged unless configured down).

S3 — HIGH — HistoryRead block-bridges the gateway per node, sequentially, on SDK request threads

Restatement: All four HistoryRead arms call .GetAwaiter().GetResult() per node handle with CancellationToken.None, bounded only by the gateway client's 30s CallTimeout. A request naming N historized nodes against a slow historian holds one SDK request thread up to N × 30s; with MaxRequestThreadCount=100, a few misbehaving history clients can exhaust the request pool and degrade all OPC UA services.

Verification — Confirmed.

  • OtOpcUaNodeManager.cs:1795-1807 (HistoryReadRawModified), :1823-1847 (HistoryReadProcessed), :1858-1868 (HistoryReadAtTime), :1886-1934 (HistoryReadEvents) — each iterates nodesToProcess in a foreach (sequential).
  • Block-bridge sites: :2059 (ServeNode: read(source, tagname).GetAwaiter().GetResult()), :2170-2171 and :2202-2203 (Raw + tie-cluster overfetch), :1902-1911 (Events) — all CancellationToken.None.
  • OpcUaApplicationHost.cs:329-331 — MinRequestThreadCount = 5, MaxRequestThreadCount = 100, MaxQueuedRequestCount = 200.
  • Comment at :1777-1780 correctly notes the overrides run outside the node-manager Lock, so this is a thread-pool-exhaustion risk, not a lock-freeze risk.

Root cause: The per-node reads are serialized and unbounded per request; there is no per-request deadline and no server-side cap on concurrent HistoryRead work.

Proposed design — bounded per-node parallelism + per-request deadline + concurrency limiter.

  1. Parallelize per-node reads within a batch, bounded. Replace the sequential foreach with a bounded-concurrency fan-out (Parallel.ForEachAsync with MaxDegreeOfParallelism, or Task.WhenAll over a SemaphoreSlim), then block-bridge once on the aggregate at the arm boundary. This turns N × 30s into ~ceil(N/P) × 30s worst case. Keep the per-node try/catch so one node's failure still surfaces Bad for that node only.
  2. Per-request deadline token. Derive a CancellationTokenSource from a configurable server-side HistoryRead deadline (e.g. ServerHistorian:HistoryReadDeadline, default ~60s) and pass it into the gateway calls instead of CancellationToken.None, so a single request can't hold a thread indefinitely regardless of per-node CallTimeout.
  3. Server-side concurrent-HistoryRead limiter. A process-wide SemaphoreSlim (configurable max, e.g. 8-16) gating entry to the arms so a flood of history clients can't consume more than a bounded slice of the request pool; excess requests wait (or fail with BadTooManyOperations if the queue is deep).

Alternatives considered: (a) Fully async HistoryRead — the SDK's HistoryRead override surface is synchronous (void returns filling results/errors), so the block-bridge is structural; parallelizing within the sync boundary is the achievable win. (b) Reject multi-node history requests — breaks legitimate clients. Bounded parallelism + deadline + limiter is the right combination.

Implementation steps.

  1. OtOpcUaNodeManager.cs — refactor each arm's foreach into a bounded parallel fan-out. Because the arms fill results[handle.Index]/errors[handle.Index] by index (thread-safe, disjoint indices), parallel writes are safe. Collect the per-node async tasks, await them under one GetAwaiter().GetResult() at the arm boundary. Add a MaxHistoryReadConcurrencyPerBatch field (set from options, mirror MaxTieClusterOverfetch at OtOpcUaServerHostedService.cs:223).
  2. Thread a per-request CancellationToken (from a deadline CTS) into ServeNode/ServeRawPaged/HistoryReadEvents in place of CancellationToken.None.
  3. Add a process-wide SemaphoreSlim limiter (field on the node manager or a small injected service) around the arm bodies; make the max configurable.
  4. ServerHistorianOptions — add HistoryReadDeadline (TimeSpan) + MaxConcurrentHistoryReads + MaxHistoryReadConcurrencyPerBatch. Wire through OtOpcUaServerHostedService (same spot as MaxTieClusterOverfetch).

Tests.

  • Unit: with a stub IHistorianDataSource that delays each read, a batch of N nodes completes in ~ceil(N/P) × delay (proves parallelism); a per-node throw still yields Bad for that node and Good for others (regression on the existing per-node isolation); a batch exceeding the deadline cancels and returns Bad/GoodNoData rather than hanging.
  • Integration/live: the env-gated live HistoryRead suite (needs the gateway) plus a stress case with a slow stub asserting the request pool isn't exhausted (measure that concurrent non-history services stay responsive while a slow history batch runs).

Effort: M. Risk/blast-radius: Medium — parallelizing gateway calls increases peak load on the historian gateway; the concurrency caps bound it. The index-disjoint result writes are safe, but verify no shared mutable state in ServeRawPaged's paging/continuation-point synthesis is touched concurrently (it operates per-handle, but audit the continuation-point cache).

S5 — MEDIUM — Redundancy NodeId identity string-matched across three sources, silent on mismatch

Restatement: OpcUaPublishActor matches n.NodeId == _localNode.Value; the snapshot derives host:port from the gossiped Member.Address while the local side derives it from configured PublicHostname:Port. A divergence (DNS vs IP, container advertised name) silently computes ServiceLevel 0 / keeps a stale role — the historical "silently inert delivery" bug shape, with no distinguishing log.

Verification — Confirmed.

  • OpcUaPublishActor.cs:512 — var entry = _lastSnapshot.Nodes.FirstOrDefault(n => n.NodeId == _localNode.Value);
  • :516-520 — if (entry is null || entry.Role == Detached) { ServiceLevelChanged(0); return; } — a missing entry is treated identically to legitimate Detached, silently → 0. No log distinguishes the two.
  • RedundancyStateActor.cs:123-134 — snapshot NodeId = ToNodeId(member.Address) from the gossiped Member.Address.
  • :141-146 — ToNodeId builds canonical host:port "the SAME format ClusterRoleInfo.LocalNode uses" — but the local side's host comes from configured PublicHostname, the snapshot side's from the gossip. They agree only if PublicHostname == the gossiped address host.

Root cause: Two independently-derived string identities are compared for equality with no assertion or diagnostic that they actually match. This is the same class as the historical redundancy-state-delivery bug (project_redundancy_state_delivery.md).

Proposed design — log-once on absence + startup self-check. This is diagnostic, not behavioral (the S4 fix consumes the same signal).

  1. In OpcUaPublishActor.RecomputeServiceLevel (and the DriverHostActor.OnRedundancyStateChanged, ScriptedAlarmHostActor equivalents), when the received snapshot has ≥1 node but none equals _localNode, log once at Warning: local node {LocalNodeId} absent from redundancy snapshot [{snapshotNodeIds}] — check PublicHostname vs gossiped Address. Log-once (a bool latch) so it doesn't spam every heartbeat.
  2. Startup self-check: in the cluster bootstrap or OpcUaPublishActor.PreStart, compare the locally-derived ClusterRoleInfo.LocalNode against Cluster.Get(system).SelfMember.Address projected through ToNodeId; if they differ, log Error (misconfiguration) — fail-fast-ish, this is a deploy config error.
  3. Reset the log-once latch when a snapshot does contain the local node (so a transient bootstrap gap that self-heals doesn't leave a stale warning impression).

Implementation steps. Touch OpcUaPublishActor.cs (RecomputeServiceLevel + PreStart self-check), DriverHostActor.OnRedundancyStateChanged (:1109), and factor ToNodeId(SelfMember.Address) comparison into a shared helper in ClusterRoleInfo so the "canonical local node id" derivation is single-sourced (removes the third independent derivation).

Tests. Unit: snapshot with nodes but not the local node → the log-once fires exactly once and ServiceLevel is 0 (distinguishable in logs from Detached); a subsequent snapshot including the local node resets the latch. Startup self-check: mismatched PublicHostname vs self-address → Error logged.

Effort: S. Risk/blast-radius: Low (diagnostic + one shared helper).

S6 — MEDIUM — Crashed-and-restarted DriverInstanceActor loses desired-subscription state; persistent thrower hot-loops

Restatement: No Runtime actor overrides SupervisorStrategy, so a driver-child exception triggers Akka's default one-for-one unlimited, non-backoff restart. Restart re-runs PreStart but the desired-subscription set arrives post-spawn via SetDesiredSubscriptions stored in actor state; restart wipes it and DriverHostActor has no restart-detection to re-send (restarts don't fire Terminated). A persistent thrower hot-loops with no backoff.

Verification — Confirmed.

  • Grep for SupervisorStrategy|BackoffSupervisor in src/Server/…/Runtime/ → zero hits (confirmed no override → default OneForOneStrategy with unlimited restarts, Directive.Restart, no backoff).
  • DriverInstanceActor.cs:84 — SetDesiredSubscriptions is a message record; :181-194 — the desired set is stored in actor fields (_desiredSubscriptions); :299 PreStart re-inits the driver from Props but does not request the subscription set; :861 StoreDesiredSubscriptions stores it. A restart clears the fields and nothing re-sends.
  • No PostRestart override that re-requests subscriptions.

Root cause: Message-delivered actor state (desired subscriptions) is not reconstructible from Props after a restart, and the parent has no restart-detection to re-push it; plus unlimited non-backoff restart on a persistent fault.

Proposed design — BackoffSupervisor + child pulls state in PreStart.

  1. Wrap each DriverInstanceActor in a BackoffSupervisor (Akka.Pattern.BackoffSupervisor / Backoff.OnFailure) with exponential backoff (e.g. min 1s, max 30s, jitter) and a SupervisorStrategy on the child that restarts on transient faults. This kills the hot-loop.
  2. Have the child request its desired-subscription set from the parent in PreStart/PostRestart — add a RequestDesiredSubscriptions(driverInstanceId) message the child sends to DriverHostActor on (re)start; the parent replies with the current desired set (which it already knows — it pushed it originally). This makes the subscription state reconstructible after any restart. Alternatively, have the parent detect the backoff-restart and re-push; the child-pull is cleaner (self-healing, no parent restart-detection needed).

Alternatives considered: (a) Move desired subscriptions into Props — they change per-deploy independently of the actor spec, so this couples deploy churn to actor respawn; rejected. (b) Akka.Persistence for the child — overkill; the parent is the source of truth and can re-supply. (c) Parent watches for restart via a custom decider — restarts don't fire Terminated, so this needs a bespoke signal; child-pull avoids it.

Implementation steps.

  1. DriverHostActor.cs — spawn each driver child under a BackoffSupervisor Props (or apply Backoff.OnFailure) with exp backoff; keep the (driver, FullName) ⇄ NodeId maps keyed the same. Handle a new RequestDesiredSubscriptions message by replying the current desired set for that driver id.
  2. DriverInstanceActor.cs — in PreStart (covers first start + restart, since Akka calls PreStart after PostRestart), send RequestDesiredSubscriptions to the parent and adopt the reply via the existing StoreDesiredSubscriptions path. Re-apply on the Connected entry as it already does (:187).
  3. Ensure the backoff wrapper doesn't break the health-publish + routing (the parent addresses the supervisor ref; verify _children maps hold the right ref for Ask/Tell — with BackoffSupervisor you Tell the supervisor which forwards).

Tests.

  • Unit (TestKit) — the U6 gap: force a driver child to throw; assert (a) it restarts with backoff (not immediately/unbounded), (b) after restart it re-requests and re-adopts the desired subscriptions, (c) a persistent thrower backs off (increasing delay) rather than hot-looping. This is the "no actor supervision/restart test anywhere" gap.
  • Live: on the docker rig, kill a driver's backing connection to induce faults and confirm the driver recovers subscriptions after a transient throw without redeploy.

Effort: M. Risk/blast-radius: Medium — changes the driver-child supervision topology; verify routing/Ask still resolves through the supervisor and that the backoff doesn't delay legitimate fast recovery (tune min backoff low).

S7 — MEDIUM — DriverInstanceActor.PostStop blocks shutdown on driver shutdown

Restatement: _driver.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult() in PostStop is the only synchronous block in the Runtime actors; a hung protocol stack stalls stop/re-deploy of the whole child set.

Verification — Confirmed. DriverInstanceActor.cs:950 — try { _driver.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult(); } catch (…) { log }. CancellationToken.None → unbounded.

Root cause: Unbounded synchronous wait on driver shutdown inside the actor stop path.

Proposed design — bound the wait, log-and-abandon on expiry. Wrap with a timeout: _driver.ShutdownAsync(cts.Token).WaitAsync(TimeSpan.FromSeconds(N)) where the CTS is created with the same timeout, so both the token and the outer WaitAsync cap it. On TimeoutException, log Warning and abandon (the actor stops regardless; the orphaned shutdown task runs to completion in the background). A configurable DriverShutdownTimeout (default ~5s, matching the write ladder's 5s backend bound) is reasonable.

Implementation steps. DriverInstanceActor.cs:950 — replace with a bounded wait; pass a real cancellation token (from a timeout CTS) into ShutdownAsync so cooperative drivers cancel promptly, and WaitAsync(timeout) as the hard bound for uncooperative ones. Optionally surface the timeout in options.

Tests. Unit: a stub driver whose ShutdownAsync never completes → PostStop returns within ~timeout and logs; a fast driver still shuts down cleanly. This complements S6's supervision tests.

Effort: S. Risk/blast-radius: Low.

S8 — MEDIUM — Inbound Part 9 commands / native acks are at-most-once; client sees Good even if lost

Restatement: The alarm-command router (Publish) and native-ack router (Tell) are fire-and-forget, catch-log-drop; the node manager already returned Good and applied local condition state. A mediator hiccup or missing DriverHostActor silently strands upstream state. Deliberate (non-blocking under Lock) but invisible.

Verification — Confirmed.

  • OtOpcUaServerHostedService.cs:139-155 (alarm-command router) and :167-194 (native-ack router) — both try { … Tell/Publish … } catch (Exception ex) { _logger.LogWarning(…) }, no counter/metric. The native-ack else branch (no DriverHostActor) also only LogWarnings (:181).
  • OtOpcUaNodeManager.cs:769-772 (referenced) — the Part 9 condition method returns Good before/independent of routing.

Root cause: Best-effort routing with only a Warning log — no operator-visible signal (metric/counter) and no reconciliation.

Proposed design — add drop counters/metrics + optional reconciliation.

  1. Add OpenTelemetry counters (the codebase uses OtOpcUaTelemetry — e.g. DriverInstanceLifecycle at DriverInstanceActor.cs:952) for alarm_command_route_dropped and native_ack_route_dropped, tagged by operation/reason (mediator_fault, no_driver_host). This makes the silent drop observable in metrics/alerting.
  2. (Optional, larger) an engine-side reconciliation sweep: periodically the scripted-alarm engine / driver re-asserts authoritative condition state so a dropped command self-heals on the next sweep. Defer unless operationally needed; the metric is the priority.

Implementation steps. OtOpcUaServerHostedService.cs — increment the counters in both catch blocks and the native-ack else. Add the counter definitions to OtOpcUaTelemetry. Surface in the existing metrics dashboard/health if present.

Tests. Unit: inject a throwing mediator accessor / a registry that returns no DriverHostActor → the counter increments and no exception escapes into the SDK path. (These routers are wired in StartAsync; testable by extracting the router lambdas or via a small harness.)

Effort: S. Risk/blast-radius: Low (additive observability).

S9 — MEDIUM — Server certificate has no renewal or expiry monitoring

Restatement: The app cert is auto-created self-signed with SDK defaults (2048-bit, 12-month lifetime), checked only at boot. Nothing monitors expiry; ~12 months after first deploy, Sign/SignAndEncrypt endpoints and UserName-token encryption fail with no warning.

Verification — Confirmed. OpcUaApplicationHost.cs:305-317 — EnsureApplicationCertificateAsync calls CheckApplicationInstanceCertificatesAsync(false, null, ct) with minimumKeySize/lifetimeInMonths: 0 → SDK defaults (comment at :308 states 2048-bit, 12-month). Boot-only; no periodic check, no expiry metric.

Root cause: No lifecycle monitoring for the server certificate (the AdminUI cert-actions cover client certs; the server cert is the gap).

Proposed design — startup + periodic expiry check with a metric/health-check + runbook.

  1. At startup, after EnsureApplicationCertificateAsync, read the resolved cert's NotAfter and log Warning if within a threshold (e.g. 30 days) and Info with the expiry date otherwise.
  2. Register a periodic check (a lightweight hosted service or a timer in the existing health pipeline) that emits a health-check/metric (server_cert_days_to_expiry) consumed by MapOtOpcUaHealth (same surface S10 targets). Degrade /health when under threshold.
  3. Document a rotation runbook in docs/security.md (delete-and-reissue, or a longer lifetimeInMonths at issue). Optionally increase the issued lifetime (pass a non-zero lifetimeInMonths to CheckApplicationInstanceCertificatesAsync) so the reissue cadence is multi-year.

Implementation steps. OpcUaApplicationHost.cs — capture the cert NotAfter after the check and expose it (property or event); add the threshold log. Add a health contributor + metric. docs/security.md — runbook section.

Tests. Unit: a near-expiry cert → the health/metric reports degraded; a fresh cert → healthy. (Injectable via a test cert with a short NotAfter.)

Effort: S/M. Risk/blast-radius: Low.

S10 — LOW — SDK start failure swallowed; node runs with Null sinks, no health surface

Restatement: StartAsync catches, logs, and returns; the node then no-ops all OPC UA work with nothing surfacing the condition through /health or ServiceLevel.

Verification — Confirmed. OtOpcUaServerHostedService.cs:110-129 — catch { LogError; return; } (deliberate: AdminUI stays up). The NodeManager is null path (:124-128) also just warns and returns. No health flag set.

Root cause: The availability trade-off (don't crash the whole binary) leaves no degraded-state signal.

Proposed design — set a health flag consumed by MapOtOpcUaHealth. Introduce an IOpcUaServerHealth singleton (a simple thread-safe flag holder) that OtOpcUaServerHostedService sets to Faulted/Degraded on the start-failure and NodeManager is null paths, and Healthy on success. MapOtOpcUaHealth reads it so fleet status shows the degraded node. Pairs naturally with S9's cert health contributor.

Implementation steps. New IOpcUaServerHealth + impl in Host/OpcUaServer; inject into OtOpcUaServerHostedService; set on each exit path in StartAsync and clear on success/StopAsync; wire into the health-check mapping.

Tests. Unit: forcing a start failure sets the health flag to degraded; success sets healthy. Health endpoint reflects it.

Effort: S. Risk/blast-radius: Low.

S11 — LOW — AuditWriterActor unbounded buffer, drops batches on DB outage (moot — no producers)

Restatement: The audit buffer is unbounded between flushes and drops whole batches on a DB outage. Best-effort by contract; currently moot because the pipeline has no producers (U3).

Verification — Confirmed (ControlPlane/Audit/AuditWriterActor.cs per report; and U3 confirms zero producers — AuditOutcomeMapper.cs:12-18 states "no live structured AuditEvent emit sites").

Root cause: Best-effort design; no backpressure; and the whole pipeline is dormant.

Proposed design. Fold into U3's decision. If the audit pipeline is wired to producers (U3), then bound the buffer (drop-oldest with a metered counter, like SqliteStoreAndForwardSink's _evictedCount) and consider a store-and-forward durable queue for compliance-grade audit. If the pipeline is deleted (U3 alternative), this is moot. Do not fix in isolation — resolve U3 first.

Effort: S (bounding) — gated on U3. Risk: Low.

P2 — MEDIUM — Per-value global-lock write, one actor message per value, no batching

Restatement: Each published value takes the global node-manager Lock once and flows through one actor message per value, contending with SDK read/subscription/publish threads. At high tag counts × fast polls this serializes everything through one lock and allocates one record per hop.

Verification — Confirmed.

  • DriverHostActor.cs:561-589 (ForwardToMux) — per value: dictionary lookup + up to two Tells.
  • OtOpcUaNodeManager.cs:266-281 (WriteValue) — takes lock (Lock) per value.
  • DependencyMuxActor.cs:95-108 — lean (early-drop, set fan-out), confirmed not the bottleneck.

Root cause: No batching of the sink write; the lock is acquired per value rather than per driver-publish cycle.

Proposed design — batched WriteValues(IReadOnlyList<…>) sink call. Add a batch method to IOpcUaAddressSpaceSink that takes one Lock hold per driver publish cycle and applies all values in the batch. The driver child already receives values in poll-cycle batches upstream; carry the batch shape through ForwardToMux → OpcUaPublishActor → sink.WriteValues. Respect the U2 forwarding trap: the new WriteValues must be added to DeferredAddressSpaceSink, NullAddressSpaceSink, and SdkAddressSpaceSink, and exhaustively test-guarded — an un-forwarded batch method ships inert (F10b/PR#423 precedent).

Alternatives considered: (a) Finer-grained locking in the node manager — the SDK's CustomNodeManager2.Lock is the contract boundary for address-space consistency; sub-locking risks correctness. Batching under the existing lock is safer. (b) Lock-free value store — too invasive. Batching is the contained seam-level win the report recommends.

Implementation steps. Add WriteValues to the sink interface + all impls + the deferred wrapper; thread batch shape through ForwardToMux/OpcUaPublishActor; keep single WriteValue for the individual paths (alarm/vtag) or route them through the batch of one.

Tests. Unit: batched write takes the lock once for N values (assert via a recording sink counting lock acquisitions or write calls); U2 forwarding test must include WriteValues. Perf: micro-benchmark N values single vs batched. Live: high-tag-count rig to confirm reduced contention (optional).

Effort: M. Risk/blast-radius: Medium (hot path; U2 trap). Do after/with P1 since both touch the sink seam.

P3 — MEDIUM — HistoryRead-Events unbounded, no paging

Restatement: EventMaxEvents maps NumValuesPerNode == 0 to int.MaxValue, and the Events arm never issues continuation points. A wide window over a busy alarm source materializes the entire result in memory on both gateway and server.

Verification — Confirmed. OtOpcUaNodeManager.cs:1944-1954 (EventMaxEvents → int.MaxValue on 0), :1919-1921 ("never issue continuation points — full window in one shot").

Root cause: Spec-conformant "no limit" translated to truly unbounded with no server-side backstop.

Proposed design — server-side max mirroring MaxTieClusterOverfetch. Impose a configurable server-side max event count for the Events arm (e.g. ServerHistorian:MaxHistoryReadEvents, default 65536 to mirror MaxTieClusterOverfetch's philosophy). When the result would exceed it, either fail loudly (BadHistoryOperationInvalid / a status telling the client to narrow the window) or page (synthesize continuation points as the Raw arm already does — larger effort). Start with the bounded-fail backstop (matches the Raw tie-cluster "loud-fail" design at :2202), add paging later if clients need it.

Implementation steps. OtOpcUaNodeManager.cs — cap EventMaxEvents's unbounded case at the configured max; on overflow return a loud status. Add the option to ServerHistorianOptions, wire via OtOpcUaServerHostedService (same spot as MaxTieClusterOverfetch).

Tests. Unit: EventMaxEvents(0) returns the configured cap (not int.MaxValue); a result at the cap returns the loud status. Update the existing EventMaxEvents unit tests.

Effort: S. Risk/blast-radius: Low.

P4 — LOW — Every deploy re-runs the four Materialise passes over the full composition

Verification — Confirmed. OpcUaPublishActor.cs:338-354 runs all four passes each deploy; acceptable because EnsureFolder/EnsureVariable early-return on existing ids, but each pass takes/releases Lock per node.

Proposed design. Fold into P2's batching — a batched ensure (one lock hold per pass) removes the per-node lock churn. No standalone work; note it when implementing P2. The positive designs (Raw tie-cluster overfetch bound, HandleDiscoveredNodes unchanged-plan short-circuit at DriverHostActor.cs:658-673) are worth preserving.

Effort: S (subsumed by P2). Risk: Low.

C1 — MEDIUM — ServerHistorian bound imperatively in five places, no IOptions

Restatement: ServerHistorian is bound in Program.cs (own bind + Validate()), AddServerHistorian, AddAlarmHistorian, AddHistorianProvisioning, and OtOpcUaServerHostedService's ctor — five Get<> sites that can log warnings twice and drift.

Verification — Confirmed. OtOpcUaServerHostedService.cs:88-90 binds directly (configuration.GetSection(ServerHistorianOptions.SectionName).Get<ServerHistorianOptions>()); Runtime/ServiceCollectionExtensions.cs:86, 132, 168 each re-Get<> the section (confirmed the section is read in AddServerHistorian/AddAlarmHistorian/AddHistorianProvisioning); Program.cs binds + Validate()s. Five sites.

Root cause: No single validated-options registration; each consumer binds independently.

Proposed design — one AddValidatedOptions<ServerHistorianOptions> + inject IOptions<>. Mirror the OpcUa/Ldap pattern already in the tree (Program.cs:102, 254-255). Register once with ValidateOnStart (folds into C2's validator promotion), then inject IOptions<ServerHistorianOptions> into OtOpcUaServerHostedService and the three Add* extensions. This single-sources the section and runs Validate() once.

Implementation steps. Add the registration in Program.cs; change the five consumers to take IOptions<ServerHistorianOptions> (the Add* extensions resolve it from the built provider or accept it as a parameter). Remove the per-site Get<> calls.

Tests. Unit: options bound once, validated once (assert single warning emission). Existing consumers still function.

Effort: S/M. Risk/blast-radius: Low-Medium (touches five wiring sites; verify the Add* extensions run after the options registration).

C2 — MEDIUM — Two-tier options validation; DevStubMode only log-warned in production

Restatement: OpcUa/Security:Ldap get fail-fast ValidateOnStart validators; ServerHistorian/ContinuousHistorization/AlarmHistorian get only advisory Validate() warnings; DevStubMode=true (accept-any-credentials Administrator) is merely log-warned in production.

Verification — Confirmed. OtOpcUaLdapAuthService.cs:79-88 — if (_options.DevStubMode) { … LogWarning("DevStubMode bypass …"); accept } with no environment guard (grep for IsDevelopment/EnvironmentName in the file → none). The historian sections use advisory Validate() (C1).

Root cause: Inconsistent validation posture; a dangerous dev bypass isn't environment-gated.

Proposed design.

  1. Promote the historian sections to the validator pattern (AddValidatedOptions + IValidateOptions<> with ValidateOnStart), folding in C1's single registration. Convert the current Validate() warning lists into validator failures (or keep soft warnings for non-fatal knobs but fail on genuinely-invalid combos, e.g. Enabled=true with empty Endpoint).
  2. Environment-gate DevStubMode: inject IHostEnvironment; if DevStubMode == true && !env.IsDevelopment(), fail startup (throw in a validator) — or at minimum refuse the bypass at runtime (deny + Error log) outside Development. Fail-fast is preferable (a prod deploy with DevStubMode is a critical misconfiguration).

Implementation steps. Add ServerHistorianOptionsValidator/ContinuousHistorizationOptionsValidator/AlarmHistorianOptionsValidator (mirror OpcUaApplicationHostOptionsValidator/LdapOptionsValidator). Add an LdapOptionsValidator (or extend the existing one) rule: DevStubMode requires Development. Wire IHostEnvironment into the validator.

Tests. Unit: historian validators fail on invalid combos, ValidateOnStart surfaces at boot; DevStubMode=true outside Development fails startup; inside Development it's allowed (warned). Existing OpcUa/Ldap validator tests stay green.

Effort: M. Risk/blast-radius: Medium — promoting to fail-fast could break a deploy relying on a currently-tolerated invalid config; validate against real appsettings before merge and document the new hard requirements in the migration note.

C3 — LOW — Library code logs via static Serilog; node manager uses obsolete Utils.LogError

Verification — Confirmed. Runtime/ServiceCollectionExtensions.cs:90,100,136 log via static Log; node manager uses Utils.LogError with #pragma warning disable CS0618 (e.g. :1928-1930, and the report cites :449-451 et al.).

Proposed design. For the Runtime static-logger sites, inject ILogger where an instance is available; where it isn't (pure Add* extension methods), accept an ILoggerFactory/ILogger parameter. For the node manager, wire the acknowledged ITelemetryContext/ILogger seam so the six CS0618 Utils.LogError sites route through a real logger instead of the obsolete static trace. This is hygiene; the static coupling works only because Program.cs:305 assigns Log.Logger (ordering-sensitive).

Effort: M (node-manager logger wiring is the bulk). Risk: Low. Lower priority than the Criticals/Highs.

C4 — LOW — IHistorianProvisioning resolve has no missing-registration warning

Verification — Confirmed. Runtime/ServiceCollectionExtensions.cs:298 — var provisioning = resolver.GetService<IHistorianProvisioning>(); with a comment noting the Null default, but no warning parallel to the evaluator/recorder-deps warnings (:211-237). This is the exact seam that shipped dormant in PR #423. The dispatched=N, requested=0 tally (AddressSpaceApplier.cs:274-282) partially compensates.

Proposed design. Add a startup log line at the resolve site: if GetService<IHistorianProvisioning>() returns the NullHistorianProvisioning singleton while ServerHistorian:Enabled == true, log Warning ("provisioning enabled but Null provisioner resolved"). Mirror the sibling missing-registration warnings. This is the "startup log proving the wiring" guard from OVERALL theme #1.

Effort: S. Risk: Low.

U1 — HIGH (doc drift) — CLAUDE.md "Known Limitation 2" is stale; recorder ref-feed is wired

Restatement: CLAUDE.md says continuous historization "records no values" because the recorder is seeded empty. In code, the applier feeds the delta via ActorHistorizedTagSubscriptionSink → UpdateHistorizedRefs.

Verification — Confirmed stale.

  • Runtime/ServiceCollectionExtensions.cs:272 — recorder still spawned with historizedRefs: Array.Empty<string>().
  • :291 — new ActorHistorizedTagSubscriptionSink(continuousRecorder) is wired.
  • AddressSpaceApplier.cs:213 (FeedHistorizedRefs) + :355-371 (HistorizedRef) feed the add/remove delta.
  • Restart convergence works via in-memory _lastApplied (OpcUaPublishActor.cs:326-336): the first post-boot rebuild diffs against empty and emits the full historized set as Added.
  • Confirmed independently by OVERALL cross-cutting theme #5.

Proposed design — update the doc + add the convergence test.

  1. Update CLAUDE.md — rewrite "KNOWN LIMITATION 2" to state the ref-feed is wired; note the load-bearing chain (empty seed + delta feed + in-memory _lastApplied → full-set-as-Added on first post-boot deploy). Keep any genuinely-remaining caveat (e.g. the numeric-analog-only v1 limitation) but drop the "records no values" claim. Propagate to ../scadaproj/CLAUDE.md's OtOpcUa entry per the repo's cross-repo rule.
  2. Add an explicit restart-convergence test: boot → apply a full plan (composition with historized tags) → assert the recorder's dependency-mux interest is registered for exactly the historized set (the chain is only implicitly covered today). This retires the "load-bearing but untested" risk.

Effort: S. Risk: Low (doc + test only). High-value because it removes a false "known broken" claim that could misdirect future work.

U2 — MEDIUM — Deferred-sink forwarding correct today but only half test-guarded

Restatement: DeferredAddressSpaceSink forwards all 10 members correctly (incl. both surgical methods with capability-sniffing), but tests assert forwarding for only ~5/10 members. The F10b incident proves the failure mode is real.

Verification — Confirmed. DeferredAddressSpaceSink.cs:15 implements IOpcUaAddressSpaceSink, ISurgicalAddressSpaceSink; all members forward (:26-49 for the 8 base members, :58-69 for the 2 surgical with _inner is ISurgicalAddressSpaceSink fallback). Correct today, but per the report only ~5 have per-member forwarding assertions.

Proposed design — reflection-driven exhaustive forwarding test. Add a test that, via reflection, enumerates every method of IOpcUaAddressSpaceSink + every optional capability interface the wrapper implements (ISurgicalAddressSpaceSink, plus any new ones from P1/P2), invokes each on a DeferredAddressSpaceSink wrapping a recording inner sink, and asserts the call reached the inner sink. This mechanically guarantees no member ships inert — retiring the whole trap class (OVERALL theme #1/#2). Apply the same treatment to DeferredServiceLevelPublisher. This test becomes the gate for P1's remove methods and P2's WriteValues.

Implementation steps. New test in tests/Server/…/DeferredAddressSpaceSinkTests.cs (extend the existing file): reflect over interface methods, build default/dummy args per parameter type, invoke, assert the recording sink saw each. Handle the capability-sniffing members (assert they forward when the inner implements the capability, and no-op/return-false safely when it doesn't).

Effort: S/M. Risk: Low. Very high leverage — one test permanently closes the "built-but-never-wired forwarding" class.

U3 — MEDIUM — Structured audit pipeline built/tested with zero producers

Verification — Confirmed. ControlPlane/Audit/AuditOutcomeMapper.cs:12-18 and Security/Audit/AuditActor.cs:18-24 state there are no live structured AuditEvent emit sites (all production audit flows through the bespoke stored-procedure path).

Proposed design — decide: wire or delete. Either (a) wire producers (route the real audit-worthy events — the write-gate audit at OtOpcUaNodeManager.cs, alarm ack/shelve, deploy outcomes — through the structured AuditEvent pipeline, replacing/augmenting the stored-proc path), or (b) delete the dormant pipeline (AuditWriterActor, AuditOutcomeMapper, AuditActor, tests) to remove decay risk. Recommendation: delete unless compliance-grade structured audit is a near-term requirement — the bespoke stored-proc path already serves production, and dormant tested code is the exact "built-but-never-wired" debt the review flags. S11 is subsumed by whichever path is chosen. This is a product decision; surface it to the owner rather than deciding unilaterally.

Effort: M (either path). Risk: Low (deletion) / Medium (wiring — new producers touch hot paths).

U4 — MEDIUM — FleetStatusBroadcaster.DriverHostStatusHeartbeat dead code with latent NodeId-mismatch

Verification — Confirmed (per report: FleetStatusBroadcaster.cs:38, 110-121 no producer; :152-159 keys nodes by host only vs the host:port canon everywhere else).

Proposed design — wire the producer or remove; canonicalize the key regardless. If fleet-status freshness is wanted, wire DriverHostActor (which knows its applied revision) to send DriverHostStatusHeartbeat and canonicalize the broadcaster key to host:port (else a wired host:port NodeId creates phantom records — re-enacting the historical NodeId bug, same class as S5). If not wanted, delete the heartbeat path. Either way, fix the key to host:port. Recommendation: remove unless the fleet-status freshness signal is used by the AdminUI; the host-only key + no producer signals it was never completed.

Effort: S (remove) / M (wire + key fix). Risk: Low.

U5 — MEDIUM — Native Part 9 conditions support Acknowledge-to-driver only

Verification — Confirmed (OtOpcUaNodeManager.cs:647-658 — Confirm/AddComment/Shelve on a native condition route to the scripted engine which doesn't own them; :698-703 — Enable/Disable → BadNotSupported). Operators shelving a Galaxy alarm via a Part 9 client get silent upstream no-op.

Proposed design — route native condition commands to the driver's IAlarmSource, or fail loudly. Extend the native-ack seam (already present: NativeAlarmAckRouter → DriverHostActor.RouteNativeAlarmAck → driver AcknowledgeAsync) to the other Part 9 operations the driver can support (Confirm/AddComment/Shelve/Unshelve) by adding driver-side methods on IAlarmSource (mirroring AcknowledgeAsync) and routing them like the ack. For operations no driver backend supports (Enable/Disable upstream), return a loud status (BadNotSupported is already returned for Enable/Disable — keep, but ensure the silently-ineffective Shelve/Confirm cases either route or return a status telling the operator it won't propagate upstream). This is documented H6c scope — scope it as a follow-on feature, not a quick fix.

Effort: M/L (driver-side IAlarmSource surface expansion + Galaxy gateway support). Risk: Medium (touches the driver alarm contract and Galaxy). Lower priority than the availability/perf cluster.

U6 — MEDIUM — Test-coverage gaps concentrate on the fragile seams

Restatement: DPS delivery of redundancy/ServiceLevel state, actor supervision/restart, outbox durability across process restart, hard-kill failover, and the single OpcUaServer.IntegrationTests test are all untested; docker-gated protocol tests self-skip to green.

Verification — Confirmed (per report's tests/Server sweep; corroborated by S1/S6 verification — no supervision test, no hard-kill test; and OVERALL theme #4).

Proposed design — targeted tests, delivered alongside the fixes above (not as a separate epic).

  • DPS delivery of redundancy/ServiceLevel — a real over-the-mediator delivery test (not a stub) on the 2-node harness; assert a RedundancyStateChanged published by RedundancyStateActor is received by OpcUaPublishActor/DriverHostActor and drives ServiceLevel/role. This is the known "unit tests can't catch it" blind spot (project_redundancy_state_delivery.md).
  • Hard-kill failover — delivered by S1's integration test.
  • Actor supervision/restart — delivered by S6's TestKit test.
  • Outbox durability across process restart — a test that appends to the FasterLog outbox, simulates a restart (new outbox instance over the same directory), and asserts un-acked entries drain (continuous-historization path).
  • OpcUaServer.IntegrationTests expansion — subscription-survival-on-add (from P1), a security-mode matrix (None/Sign/SignAndEncrypt over the wire), and a HistoryRead round-trip.
  • Make docker skips visible — align with OVERALL action #6 (fail-on-skip for the integration job); coordinate with report 07.

Effort: M (spread across the fixes). Risk: Low (test-only). Track each sub-test with its parent finding so coverage lands with the fix, not after.

U7 / U8 / U9 — LOW — Batched hygiene items

  • U7 — IHistoryWriter permanently Null-wired (Runtime/ServiceCollectionExtensions.cs:56-58, infra-gated on a nonexistent live-write RPC); H2 HistoryUpdate unimplemented (OtOpcUaNodeManager.cs:1797-1801, IsReadModified rejected); LdapAuthFailure lacks a DirectoryUnavailable value (directory-down overloaded onto ServiceAccountBindFailed). Verification — Confirmed (all backlog/tracked). Design: leave IHistoryWriter/H2 as tracked backlog (infra-gated). Add the DirectoryUnavailable enum value and map directory-down to it in the LDAP path (pairs with S2's outage handling — a distinct failure reason improves the operator signal). Effort: S (enum) + backlog (rest).
  • U8 — BuildSecurityPolicies doc claims the empty-profile fallback is "logged and very visible" but logs nothing (OpcUaApplicationHost.cs:376-418). Defused in prod by the MinCount ≥ 1 validator but silent for direct embedders. Verification — Confirmed (per report). Design: either add the log the comment promises (a Warning on empty-profile fallback-to-None) or fix the comment. Adding the log is cheap and correct. Effort: S.
  • U9 — EnsureVariable silently ignores changed historize-intent on an existing node (OtOpcUaNodeManager.cs:1345-1349); correct today because the planner routes such deltas elsewhere, but the invariant lives in two comments. Verification — Confirmed. Design: turn the invariant into an assert/Debug.Assert (or a Warning log) when EnsureVariable is called with a differing historize-intent for an existing id, so a future planner regression surfaces instead of silently no-op'ing. Effort: S.

Risk (all three): Low.

Suggested execution order (this report's slice of the OVERALL list)

  1. S1 — activate SBR + hard-kill failover test (OVERALL #1). Do first — nothing else matters if failover is broken.
  2. U1 — update CLAUDE.md (stale Known Limitation 2) + convergence test (OVERALL #11). Cheap, removes a misleading claim before others build on it.
  3. U2 — reflection-exhaustive Deferred-sink forwarding test (OVERALL theme #1/#2). Prerequisite guard for P1/P2 — land it first so P1's remove methods and P2's WriteValues can't ship inert.
  4. S4 — primary-gate default-deny on multi-node (OVERALL #7). Small, closes the dual-primary data-plane window.
  5. P1 — surgical pure-adds (phase 1), scoped removals (phase 2) (OVERALL #8). Highest operational perf/stability win; gated on U2.
  6. S2 / S3 — async LDAP timeout + channelized/bounded HistoryRead (OVERALL #9).
  7. S5, S6, S7, S8 — redundancy-mismatch diagnostics, BackoffSupervisor + subscription re-pull, bounded PostStop, drop metrics.
  8. C1 + C2 — single validated ServerHistorian options + historian validators + DevStubMode env-gate.
  9. P2 (+P4), P3, S9, S10 — batched sink writes, Events cap, cert monitoring, start-failure health flag.
  10. U3, U4, U5, C3, C4, U7-U9 — dormant-code decisions, native Part 9 expansion, and hygiene, as capacity allows.
Reference in New Issue View Git Blame Copy Permalink