Joseph Doherty
611 lines
73 KiB
Markdown
611 lines
73 KiB
Markdown
# Design & Implementation Plan — Server & Runtime Subsystem (Report 03)
|
||
|
||
> **Status (2026-07-08):** ✅ **S1 (Critical) typed-option DONE** — branch `fix/archreview-crit1-split-brain-resolver`
|
||
> @ `a25c9ed0` (typed `KeepOldestOption`, unit guards + e2e config-resolution verified). ✅ **U1 doc DONE**
|
||
> (CLAUDE.md KNOWN LIMITATION 2 corrected). ✅ **S1 hard-kill failover integration test DONE** (task 9, same branch
|
||
> `a25c9ed0`) — `HardKillFailoverTests` LIVE-verified in-process + negative-control-proven. ⚠️ **that negative
|
||
> control surfaced task #11: SBR was ALREADY active on master** (Akka.Cluster.Hosting default + pre-existing
|
||
> akka.conf keep-oldest), so Crit-1's "was NoDowning" premise is inaccurate — correct the rationale/docs.
|
||
> ✅ **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`](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`:
|
||
```csharp
|
||
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:
|
||
```csharp
|
||
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 `LogWarning`s (`: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 `Tell`s.
|
||
- `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.
|