docs(archreview): record #11 CORRECTED (Crit-1 premise) + track residual #12/#13
- FOLLOWUP-11 → IMPLEMENTED with outcome (docs/comment-only, no code revert; the four corrected sites; Cluster.Tests 29/29). - STATUS.md: task #11 → completed; crit1 branch head → eaf78aad; Completed-table row; Findings #11 bullet flipped to CORRECTED; added task rows #12 (live gate) + #13 (ResilienceConfig-in-artifact).
This commit is contained in:
Joseph Doherty
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user