From 9cd1f8c4e7a8705229db2d31054d1139f55e7791 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 8 Jul 2026 20:06:15 -0400 Subject: [PATCH] docs(archreview): file follow-up records for open decisions #10 (RESILIENCE-DISPATCH-GAP) + #11 (Crit-1 SBR premise) --- .../FOLLOWUP-10-resilience-dispatch-gap.md | 77 +++++++++++++++++++ .../plans/FOLLOWUP-11-crit1-sbr-premise.md | 64 +++++++++++++++ archreview/plans/STATUS.md | 4 + 3 files changed, 145 insertions(+) create mode 100644 archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md create mode 100644 archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md diff --git a/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md b/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md new file mode 100644 index 00000000..4c0005c6 --- /dev/null +++ b/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md @@ -0,0 +1,77 @@ +# Follow-up #10 — Wire CapabilityInvoker into the production dispatch layer (RESILIENCE-DISPATCH-GAP) + +> **Status:** OPEN · **Surfaced by:** guard 07/C-1 (wiring the OTOPCUA0001 analyzer) on +> `fix/archreview-c1-wire-analyzer` `f0082af5` · **Task:** TaskCreate #10 · **Severity:** likely High · +> **Effort:** M–L · **Risk:** Medium (behavior-changing hot path). See [`STATUS.md`](STATUS.md). + +## The finding + +Wiring the custom `UnwrappedCapabilityCallAnalyzer` (OTOPCUA0001) tree-wide surfaced that the entire +**Phase 6.1 `CapabilityInvoker` resilience pipeline is constructed only in tests** — it is instantiated +nowhere in production. The production dispatch layer calls driver-capability methods +(`IReadable`/`IWritable`/`ISubscribable`/`ITagDiscovery`/`IAlarmSource`) **directly**, bypassing the +retry / circuit-breaker / bulkhead / tracker-telemetry pipeline entirely. + +`CapabilityInvoker`'s own XML doc falsely claims: *"The server's dispatch layer routes every capability +call … through this invoker."* It does not. This is the exact "built-but-never-wired" class the whole +arch review targets — the analyzer did its job on the first wiring. + +### Evidence +- `grep -rn "new CapabilityInvoker"` → only `tests/Core/…` (7 test files). Zero production constructions. +- `DriverInstanceActor` holds a raw `IDriver _driver` and calls it directly at 6 sites. +- `GenericDriverNodeManager.BuildAddressSpaceAsync` calls `discovery.DiscoverAsync` directly (1 site). + +### The 7 deferred sites (all marked `#pragma warning disable OTOPCUA0001 // RESILIENCE-DISPATCH-GAP`) +Grep the marker to find them: `grep -rn "RESILIENCE-DISPATCH-GAP" src`. + +| File | ~line | Call | +|---|---|---| +| `src/Server/…/Runtime/Drivers/DriverInstanceActor.cs` | 575 | `writable.WriteAsync` | +| `src/Server/…/Runtime/Drivers/DriverInstanceActor.cs` | 623 | `src.AcknowledgeAsync` | +| `src/Server/…/Runtime/Drivers/DriverInstanceActor.cs` | 660 | `subscribable.SubscribeAsync` | +| `src/Server/…/Runtime/Drivers/DriverInstanceActor.cs` | 685 | `subscribable.UnsubscribeAsync` | +| `src/Server/…/Runtime/Drivers/DriverInstanceActor.cs` | 762 | `src.SubscribeAlarmsAsync` | +| `src/Server/…/Runtime/Drivers/DriverInstanceActor.cs` | 821 | `discovery.DiscoverAsync` | +| `src/Core/…/Core/OpcUa/GenericDriverNodeManager.cs` | 71 | `discovery.DiscoverAsync` | + +> These are distinct from the 3 **driver-internal self-call** sites (AbCipAlarmProjection ×2, S7Driver ×1) +> which are `#pragma`'d as *intentional* — a driver must not re-wrap its own internal poll/ack calls; the +> invoker wraps the driver from the dispatch layer OUTWARD. Do NOT touch those. + +## Why it wasn't fixed inside 07/C-1 + +07/C-1's scope was "wire the analyzer + triage." Actually routing all 7 sites through `CapabilityInvoker` +is a substantial, behavior-changing hot-path remediation (activates retry/breaker/bulkhead on every live +driver read/write/subscribe/discover), which must be its own verified change — exactly the discipline the +review espouses. So the sites were suppressed as a *tracked* gap (not "intentional"), keeping the tree +green and the analyzer live everywhere else, and this follow-up was filed. + +## Proposed remediation + +1. **Construct a per-`DriverInstance` `CapabilityInvoker`** and thread it into `DriverInstanceActor` (via + its `Props`/ctor) and `GenericDriverNodeManager`. It needs: + - the process-singleton `DriverResiliencePipelineBuilder`, + - a `Func` options accessor (source the per-instance resilience options — + confirm where they live / how Admin edits them), + - the `driverType` string (already on the actor), + - the `DriverResilienceStatusTracker` (so Admin `/hosts` shows in-flight/bulkhead depth), + - the per-call host resolver (`IPerCallHostResolver`) for multi-host drivers. +2. **Route each of the 7 sites** through `ExecuteAsync` / `ExecuteWriteAsync` (writes/acks use the write + variant). Remove the `RESILIENCE-DISPATCH-GAP` pragma at each site as it is wired. +3. **Fix `CapabilityInvoker`'s XML doc** once the claim becomes true. + +## Verification bar (unit-green ≠ wired) + +- **Unit:** an actor test asserting the capability call goes through a fake/recording invoker (not the raw + driver). The analyzer itself becomes the standing guard — once the pragmas are gone, any regression that + re-introduces a raw call fails the build. +- **Live (decisive):** on the docker-dev / driver rig, prove the pipeline is actually engaged — e.g. use + the modbus `exception_injector` (see the write-outcome memory) or a flaky-read fixture to observe a retry + / breaker-open transition and the `/hosts` in-flight counter moving. A behavior change on the live data + path must not ship on unit tests alone. + +## Decision needed + +- Confirm this is worth doing now (it changes runtime latency/behavior on every driver call) vs. deferring. +- If deferring, keep the `RESILIENCE-DISPATCH-GAP` pragmas + this file as the standing record. +- Own branch off `master` (like the Criticals). diff --git a/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md b/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md new file mode 100644 index 00000000..5a547894 --- /dev/null +++ b/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md @@ -0,0 +1,64 @@ +# Follow-up #11 — Correct Critical 1's premise: SBR was already active on master (not NoDowning) + +> **Status:** OPEN · **Surfaced by:** the #9 hard-kill failover test's negative control on +> `fix/archreview-crit1-split-brain-resolver` `a25c9ed0` · **Task:** TaskCreate #11 · **Severity:** Low +> (docs/rationale accuracy; the code change is harmless) · **Effort:** S. See [`STATUS.md`](STATUS.md). + +## The finding + +Critical 1 (03/S1) claimed: *the akka.conf `split-brain-resolver` block was inert, the cluster ran Akka's +default **NoDowning**, and setting the typed `ClusterOptions.SplitBrainResolver = KeepOldestOption { +DownIfAlone = true }` is what activated the resolver so hard-crashed nodes fail over.* + +**That premise is inaccurate.** The cluster was **never** running NoDowning; hard-crash failover already +worked on master before Critical 1. + +### Evidence (empirical + source) + +1. **The `#9` failover test's negative control.** With the typed `SplitBrainResolver` option **removed**, + the hard-kill failover test **still passes** — node B downs the alone crashed oldest node and takes + over. Only forcing **explicit** NoDowning (`akka.cluster.downing-provider-class = ""`) makes the test + time out (node stuck Up-but-Unreachable). So removing Critical 1's change does *not* disable failover; + only an explicit NoDowning does. + +2. **Akka.Cluster.Hosting default (v1.5.51).** `WithClustering(ClusterOptions)` applies + `SplitBrainResolverOption.Default` when `SplitBrainResolver` is `null` — i.e. it **enables the SBR + downing provider by default**. That provider then reads the `akka.cluster.split-brain-resolver` HOCON. + (Confirmed by IL inspection of `Akka.Cluster.Hosting.dll`: the null branch loads `…Default` and calls + `.Apply(builder, …)`.) + +3. **The akka.conf keep-oldest block pre-existed Critical 1.** `git show master:…/Resources/akka.conf` + already contains `split-brain-resolver { active-strategy = "keep-oldest"; stable-after = 15s; + keep-oldest { down-if-alone = on } }`. Critical 1's diff to akka.conf added **only comments** (plus the + typed option in `ServiceCollectionExtensions.BuildClusterOptions`). + +**Net:** the effective resolver on master was already `keep-oldest` + `down-if-alone` (active via +Akka.Cluster.Hosting's default provider reading the pre-existing akka.conf block). Critical 1's typed +option produces the *same* effective behavior — it is **reinforcing/explicit**, not the activator. + +## What is (and isn't) wrong + +- ❌ **Wrong:** the claim that SBR was inert / the cluster ran NoDowning / hard-crash failover was broken + before Critical 1. This appears in the `BuildClusterOptions` XML comment, the akka.conf comment Critical 1 + added, and the `Redundancy.md` / `Architecture.md` wording Critical 1 touched. +- ✅ **Fine to keep:** the typed `KeepOldestOption { DownIfAlone = true }` itself. Making the strategy + explicit in code (independent of the framework's default) is defensible belt-and-suspenders and matches + the akka.conf block. **Do not revert the code.** + +## Proposed correction + +1. Rewrite the `BuildClusterOptions` XML comment to state accurately: *Akka.Cluster.Hosting enables an SBR + downing provider by default (applies `SplitBrainResolverOption.Default` when the option is null), which + reads the akka.conf `keep-oldest` block; this typed option makes the strategy **explicit in code** rather + than relying on the framework default — it is not the sole activator.* +2. Correct the akka.conf comment (drop "inert" / "NoDowning" framing). +3. Correct `docs/Redundancy.md` + `docs/v2/Architecture.md` wording Critical 1 added — do **not** describe + Critical 1 as "fixing a NoDowning bug." +4. Keep the #9 `HardKillFailoverTests` as the standing guard — it verifies the failover OUTCOME regardless + of activation path, and its comments already reflect the corrected understanding. + +## Decision needed + +Confirm the correction (docs + comments only, no code revert), or flag if there is production context where +the akka.conf block or the Hosting default would *not* apply (none found — production uses the same +`WithClustering` path and ships the same akka.conf). Bundle with the docs branch (low-risk). diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 0c590ced..e0963191 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -55,6 +55,10 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif ## Findings surfaced by the guards (NEW — need user decision) +> Each has a dedicated follow-up file with the full design/action: +> [`FOLLOWUP-10-resilience-dispatch-gap.md`](FOLLOWUP-10-resilience-dispatch-gap.md) · +> [`FOLLOWUP-11-crit1-sbr-premise.md`](FOLLOWUP-11-crit1-sbr-premise.md). + - **#10 — RESILIENCE-DISPATCH-GAP (from wiring the analyzer, 07/C-1):** the Phase 6.1 `CapabilityInvoker` resilience pipeline (retry/breaker/bulkhead/telemetry) is constructed **only in tests** — the production dispatch layer (`DriverInstanceActor` + `GenericDriverNodeManager`) calls driver capability methods directly.