docs(archreview): file follow-up records for open decisions #10 (RESILIENCE-DISPATCH-GAP) + #11 (Crit-1 SBR premise)
This commit is contained in:
Joseph Doherty
@@ -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<DriverResilienceOptions>` 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).
|
||||
@@ -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).
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user