diff --git a/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md b/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md index 5a547894..d5f914bf 100644 --- a/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md +++ b/archreview/plans/FOLLOWUP-11-crit1-sbr-premise.md @@ -1,9 +1,31 @@ # 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 +> **Status:** ✅ IMPLEMENTED (`fix/archreview-crit1-split-brain-resolver` `eaf78aad`) · **Surfaced by:** the #9 +> hard-kill failover test's negative control on `a25c9ed0` · **Task:** TaskCreate #11 · **Severity:** Low > (docs/rationale accuracy; the code change is harmless) · **Effort:** S. See [`STATUS.md`](STATUS.md). +## Outcome (implemented) + +Docs/comment-only correction landed on the crit1 branch (`eaf78aad`) — **no code reverted** (the typed +`KeepOldestOption { DownIfAlone = true }` is correct belt-and-suspenders). Corrected the inaccurate +"HOCON inert / cluster runs NoDowning / never fails over" framing in all four places it appeared: + +1. `ServiceCollectionExtensions.BuildClusterOptions` XML comment → now states Akka.Cluster.Hosting enables + an SBR provider by default (applies `SplitBrainResolverOption.Default` when the option is null, reading + the akka.conf `keep-oldest` block); the typed option makes the strategy **explicit in code**, reinforcing + not activating. +2. `akka.conf` `split-brain-resolver` comment → the block IS active (via the Hosting default); the typed + option makes it explicit; only an explicit `downing-provider-class = ""` would disable failover. +3. `docs/Redundancy.md` Split-brain section → rewrote the "activated in code / without it → NoDowning" + paragraph; dropped the "now triggers failover" wording; cross-referenced `HardKillFailoverTests` + its + negative control. +4. `SplitBrainResolverActivationTests` class summary + the `ShouldNotBeNull` message; renamed + `BuildClusterOptions_activates_a_split_brain_resolver` → `..._sets_an_explicit_split_brain_resolver`. + +**Verification:** Cluster.Tests 29/29 green (the guards still assert the typed option is set — a refactor +can't silently drop the explicitness). Failover *behaviour* remains guarded by `HardKillFailoverTests`, +which passes regardless of activation path. + ## The finding Critical 1 (03/S1) claimed: *the akka.conf `split-brain-resolver` block was inert, the cluster ran Akka's diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 8e0c9afe..6a0200f4 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -9,7 +9,7 @@ truth for remediation progress. Update it as work lands. |---|---|---| | `master` | `9cad9ed0` | untouched baseline | | `docs/archreview-plans-and-drift-fixes` | `9fadead6` | the 7 domain plans + `00-INDEX.md` + this `STATUS.md` + the two CLAUDE.md doc-drift fixes. **(the plans live ONLY here)** | -| `fix/archreview-crit1-split-brain-resolver` | `a25c9ed0` | Critical 1 (03/S1) typed SBR option + guard test + Redundancy.md; **+ #9 hard-kill failover test** (`a25c9ed0`) | +| `fix/archreview-crit1-split-brain-resolver` | `eaf78aad` | Critical 1 (03/S1) typed SBR option + guard test + Redundancy.md; **+ #9 hard-kill failover test** (`a25c9ed0`); **+ #11 premise correction** (`eaf78aad`) | | `fix/archreview-crit2-vt-timeout` | `7fd44f0f` | Critical 2 (02/U2 + 02/U3) + tests | | `fix/archreview-crit3-s7-reconnect` | `25c0c6f6` | Critical 3 (05/STAB-1) IS7Plc seam + lazy reconnect + unit guards + live bounce test | | `fix/archreview-crit4-twincat-replay` | `af318fb4` | Critical 4 (05/STAB-2) native-notification replay + probe-recycle + unit guards | @@ -38,6 +38,7 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif | **Guard — CI to all suites + fail-on-skip + deflake** | 07/S-1,S-2,S-4 | `fix/archreview-s1-s2-ci-coverage` `10b89830` | S-1: replaced the 5-project `unit-tests` matrix with ONE whole-solution leg (self-maintaining; now covers Client's 388 + Analyzers' 31 + all drivers). S-2: `scripts/ci/assert-not-all-skipped.sh` fail-on-skip gate (turns "green==all-skipped" red) — strict on the unit leg, report-only on the fixtureless integration leg. S-4: readiness-signal (`SubscribeInvoked`/`SubscribeAlarmsInvoked`) replaces 11 fixed-sleep startup races in the newly-CI'd Client.CLI.Tests. Skip-gate proven locally (real trx OK / all-skip trx exit 1 / report-only / edge cases); Client.CLI.Tests 104/104. **CI job execution itself is verifiable only on push (nothing pushed).** | | **#10 — wire CapabilityInvoker into dispatch (seam)** | RESILIENCE-DISPATCH-GAP (07/C-1) | `fix/archreview-crit10-wire-capability-invoker` `62556c24` | **Discovered the filed plan was infeasible** — Runtime is deliberately Polly-free (refs `Core.Abstractions`, not `Core`). Fix = a seam mirroring `IDriverFactory`: `IDriverCapabilityInvoker`(+null) + `IDriverCapabilityInvokerFactory`(+null) in Core.Abstractions; `CapabilityInvoker` implements it; `DriverCapabilityInvokerFactory` (Core) builds per-instance over the singleton builder+tracker+`GetTier`; Host `DriverFactoryBootstrap` registers all three; Runtime SCE resolves like `IDriverFactory`; `DriverInstanceActor` wraps its **6** dispatch sites; analyzer taught the interface as a wrapper home; all 6 pragmas removed. Triage corrections: the 7th site (`GenericDriverNodeManager`) is test-only scaffolding (not wired, re-annotated); **tier-defaults only** (ResilienceConfig not in the artifact → sub-gap). Added retry/breaker **logging** to the pipeline builder (the tracker has no reader — Stream E.2/E.3 never shipped — so it ran invisibly). **Verified:** negative-control (unwrap→OTOPCUA0001 build error); recording-invoker routing test; **real-`CapabilityInvoker` actor-context test** (internal `ConfigureAwait(false)` doesn't leak); analyzer + logging tests; clean solution build; Runtime.Tests 358 / Core.Tests 238 / Analyzers 32. **Live behavioral gate pending** (recipe in FOLLOWUP-10). | | **#9 — hard-kill failover integration test** | 03/S1 | `fix/archreview-crit1-split-brain-resolver` `a25c9ed0` | Critical 1's load-bearing integration half. `TwoNodeClusterHarness.HardKillNodeAAsync` (transport-shutdown crash sim → node B sees A Unreachable, no graceful Leave) + `HardKillFailoverTests` asserting B downs the alone oldest node and takes over as `driver` role-leader. **LIVE-verified in-process** (~35s); **negative control** (force `downing-provider-class=""`) makes it correctly time out. ⚠️ The negative control also surfaced a **Crit-1 premise finding (task #11): SBR was already active on master** (Akka.Cluster.Hosting applies `SplitBrainResolverOption.Default` when the typed option is null → reads the pre-existing akka.conf keep-oldest block), so the cluster was NOT NoDowning before Crit-1 — the typed option is reinforcing, not the activator. | +| **#11 — Critical 1 premise correction** | 03/S1 (from #9) | `fix/archreview-crit1-split-brain-resolver` `eaf78aad` | Docs/comment-only fix (no code revert): corrected the "HOCON inert / cluster runs NoDowning / never fails over" framing in `BuildClusterOptions` XML comment, `akka.conf` split-brain-resolver comment, `docs/Redundancy.md`, and `SplitBrainResolverActivationTests` (summary + `ShouldNotBeNull` message + method rename). Akka.Cluster.Hosting enables an SBR provider **by default** (applies `SplitBrainResolverOption.Default` when the typed option is null → reads the pre-existing akka.conf `keep-oldest` block), so the cluster was never NoDowning and the typed `KeepOldestOption` is reinforcing/explicit-in-code, not the activator. Cluster.Tests 29/29; failover behaviour still guarded by `HardKillFailoverTests`. Architecture.md carried no such framing. | ## Task list (TaskCreate IDs) @@ -53,7 +54,9 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif | 8 | Guard — CI to all unit suites + fail-on-skip (07/S-1,S-2, pair S-4) | ✅ completed — `10b89830` | | 9 | Critical 1 follow-up — hard-kill failover integration test (03/S1) | ✅ completed — `a25c9ed0`; surfaced task #11 | | 10 | **Wire CapabilityInvoker into the dispatch layer** (RESILIENCE-DISPATCH-GAP, from 07/C-1) | ✅ code-complete + unit/analyzer/context-verified — `fix/archreview-crit10-wire-capability-invoker` `62556c24` (seam, NOT direct thread — Runtime is Polly-free). **Live behavioral gate pending** (rig recipe in FOLLOWUP-10). Surfaced a sub-gap: ResilienceConfig not in the deploy artifact → tier-defaults only. | -| 11 | **Review Critical 1 premise** — SBR was already active on master, not NoDowning (from #9) | ⬜ pending — correct Crit-1's rationale/docs; the fix isn't harmful but "was NoDowning" is inaccurate. | +| 11 | **Review Critical 1 premise** — SBR was already active on master, not NoDowning (from #9) | ✅ completed — `fix/archreview-crit1-split-brain-resolver` `eaf78aad`; docs/comment-only correction (no code revert), Cluster.Tests 29/29. | +| 12 | **#10 live behavioral gate** — rebuild docker-dev, force a connection-level exception on an invoker-wrapped call, observe retry/breaker logs + recovery (from #10) | ⬜ pending — rig session (recipe in FOLLOWUP-10); the residual live gate for #10. | +| 13 | **Plumb per-instance `ResilienceConfig` through the deploy artifact** (`DriverInstanceSpec` + `ConfigComposer`) so overrides reach `DriverCapabilityInvokerFactory.Create` (from #10) | ⬜ pending — currently tier-defaults only; add pipeline-invalidate on change. | ## Findings surfaced by the guards (NEW — need user decision) @@ -70,12 +73,15 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif artifact → **tier-defaults only** until the composer/artifact is extended; (b) the resilience-status tracker has **no reader** (Admin `/hosts` Stream E.2/E.3 never shipped) — added retry/breaker **logging** as the interim observability surface + the live-verify enabler. -- **#11 — Critical 1 premise inaccurate (from the #9 failover negative control):** SBR was **already active on - master** (Akka.Cluster.Hosting's `WithClustering` applies `SplitBrainResolverOption.Default` when the typed - option is null → enables the downing provider that reads the pre-existing akka.conf `keep-oldest` block). So the - cluster was **not** running NoDowning; hard-crash failover already worked. Crit-1's typed option is reinforcing - (fine to keep), but its "HOCON inert / NoDowning → no failover" justification + the Redundancy.md/Architecture.md - wording are wrong and should be corrected. Do NOT describe Crit-1 as "fixed a NoDowning bug." +- **#11 — Critical 1 premise inaccurate (from the #9 failover negative control): ✅ CORRECTED** on + `fix/archreview-crit1-split-brain-resolver` `eaf78aad`. SBR was **already active on master** + (Akka.Cluster.Hosting's `WithClustering` applies `SplitBrainResolverOption.Default` when the typed option is + null → enables the downing provider that reads the pre-existing akka.conf `keep-oldest` block). So the cluster + was **not** running NoDowning; hard-crash failover already worked. Docs/comment-only fix (no code revert — the + typed `KeepOldestOption` is correct belt-and-suspenders): corrected the "HOCON inert / NoDowning → no failover" + framing in `BuildClusterOptions` XML comment, `akka.conf`, `docs/Redundancy.md`, and the + `SplitBrainResolverActivationTests` summary/message (+ method rename). Cluster.Tests 29/29. (Architecture.md + carried no such framing.) See [`FOLLOWUP-11`](FOLLOWUP-11-crit1-sbr-premise.md). ## Suggested next (from 00-INDEX execution order)