diff --git a/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md b/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md index 057c4241..53e24541 100644 --- a/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md +++ b/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md @@ -29,9 +29,14 @@ the concrete invoker in would reverse that architectural decision. 1. Only **6** sites are real (all in `DriverInstanceActor`). The 7th — `GenericDriverNodeManager` — is **test-only scaffolding** (zero production references; the Server has its own address-space path). Its call is NOT wired; the pragma is re-annotated accordingly, not left as a "tracked gap". -2. **Tier-DEFAULT policy only.** `DriverInstanceSpec` (the deploy artifact) does not carry the per-instance - `ResilienceConfig` JSON, so overrides aren't applied yet. **Sub-gap → new follow-up:** plumb `ResilienceConfig` - through the composer/artifact so per-instance overrides reach the invoker. +2. ~~**Tier-DEFAULT policy only.** `DriverInstanceSpec` (the deploy artifact) does not carry the per-instance + `ResilienceConfig` JSON, so overrides aren't applied yet.~~ **✅ RESOLVED by task #13** + (`fix/archreview-crit13-resilience-config-artifact` `75403caa`): the artifact already emitted the column (the + composer serializes the whole `DriverInstance` entity; AdminUI already authored+persisted it) — only the read + path dropped it. `DriverInstanceSpec` now carries `ResilienceConfig`, `Create` takes + layers it (invalidating + the instance's cached pipelines so a respawn takes effect), and `DriverSpawnPlanner` respawns on a + ResilienceConfig change. Per-instance overrides now reach the invoker. See + [`FOLLOWUP-13`](FOLLOWUP-13-resilience-config-artifact.md). **Observability finding + fix.** The resilience-status tracker has **no reader** (Admin `/hosts`, Phase 6.1 Stream E.2/E.3, was never built) and the pipeline builder logged nothing — so the pipeline ran **invisibly**. Added diff --git a/archreview/plans/FOLLOWUP-13-resilience-config-artifact.md b/archreview/plans/FOLLOWUP-13-resilience-config-artifact.md new file mode 100644 index 00000000..f335a250 --- /dev/null +++ b/archreview/plans/FOLLOWUP-13-resilience-config-artifact.md @@ -0,0 +1,63 @@ +# Follow-up #13 — Plumb per-instance ResilienceConfig through the deploy artifact + +> **Status:** ✅ IMPLEMENTED (`fix/archreview-crit13-resilience-config-artifact` `75403caa`, off the crit10 branch) +> · **Surfaced by:** task #10's residual sub-finding (the invoker got tier defaults only) · **Task:** TaskCreate +> #13 · **Severity:** Medium (a silently-ignored authored config — the review's "unit-green ≠ wired" class) · +> **Effort:** M. See [`STATUS.md`](STATUS.md) + [`FOLLOWUP-10`](FOLLOWUP-10-resilience-dispatch-gap.md). + +## The finding + +Task #10 wired the resilience pipeline into dispatch but the invoker only ever got the driver type's **tier +defaults** — `DriverCapabilityInvokerFactory.Create` passed `resilienceConfigJson: null`. The per-instance +`DriverInstance.ResilienceConfig` JSON column was therefore **silently ignored at runtime**. + +**Key discovery:** the *write* side was already complete — the AdminUI `DriverResilienceSection` authors it, every +driver page persists it to the entity (create + update), and `ConfigComposer.SnapshotAndFlattenAsync` serializes +the **whole** `DriverInstance` entity, so `ResilienceConfig` was **already in the artifact JSON**. Only the driver +node's *read* path dropped it: `DriverInstanceSpec` didn't carry the column and the factory hard-coded null. So +#13 is pure read-path plumbing — a genuine dead-config-to-live fix, not a new feature. + +## What shipped + +- **`DriverInstanceSpec`** gains `string? ResilienceConfig`; **`DeploymentArtifact.TryReadSpec`** reads the column + (lenient `ReadString`, like the other fields). +- **`IDriverCapabilityInvokerFactory.Create`** takes `string? resilienceConfigJson`; the concrete factory parses + it via `DriverResilienceOptionsParser.ParseOrDefaults` (layering onto the tier), logs any parse diagnostic + (malformed JSON / unknown capability / misapplied Tier-C recycle) as a warning, and **never throws** — a bad + config degrades to tier defaults. `NullDriverCapabilityInvokerFactory` ignores the arg. Host DI passes a logger. +- **`DriverHostActor.SpawnChild`** threads `spec.ResilienceConfig` into `Create`. +- **Invalidate-on-change.** The pipeline cache keys on `(instance, host, capability)` and **ignores options on a + cache hit**, so a fresh invoker with changed options would otherwise keep serving the stale pipeline. `Create` + now calls `DriverResiliencePipelineBuilder.Invalidate(driverInstanceId)` first (no-op on first spawn; drops the + stale pipelines on a respawn). Invalidation is per-instance — a sibling's warm pipeline survives. +- **`DriverSpawnPlanner`** treats a `ResilienceConfig` change (incl. `null → json`) as a **stop + respawn** (the + invoker + its resolved options are bound to the child at spawn — the only way a change takes effect is to rebuild + the child). A **pure `DriverConfig` change stays an in-place delta** (no reconnect) — the resilience pipeline is + untouched. Snapshot (`DriverChildSnapshot`) carries `ResilienceConfig` for the diff. + +## Design note — respawn vs. in-place swap + +A ResilienceConfig-only change **reconnects the driver** (respawn). This is the simplest correct wiring: the +invoker/options are immutable for a child's lifetime (a clean invariant) and it reuses the existing stop+spawn +machinery. The alternative — an in-place invoker swap via a new actor message + mutable field — avoids the +reconnect but is materially more code for a rare operator-tuning op. Documented as a possible future optimization; +respawn is the v1. + +## Verification (deterministic) + +- **Override reaches execution** (`DriverCapabilityInvokerFactoryTests`): a `Read → retryCount:0` override + suppresses the tier-A Read retry (1 attempt); a **control** with no config retries (>1) — proving the single + attempt is the override, not a fluke. +- **Invalidate-on-create**: re-creating an instance's invoker drops its cached pipelines (count → 0); a sibling + instance's pipeline survives (scoped). +- **Malformed config** logs a warning + still yields a working (tier-default) invoker. +- **Planner** (`DriverSpawnPlannerTests`): ResilienceConfig change → respawn (incl. `null → json`); pure + DriverConfig change with unchanged resilience → delta. +- **Artifact** (`DeploymentArtifactTests`): the column is carried onto the spec / omitted → null. +- Core.Tests 243 (+5), Runtime.Tests 363 (+5), Host builds clean (0 warnings — dispatch sites untouched). + +## Residual + +None functional. A live rig check would fold into task #12 (the #10 live gate) — deploy a driver with a non-default +ResilienceConfig and confirm the observed retry/breaker behavior matches the override — but the deterministic tests +already prove the override reaches execution. diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 6a0200f4..d8a477a1 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -17,6 +17,7 @@ truth for remediation progress. Update it as work lands. | `fix/archreview-c1-wire-analyzer` | `f0082af5` | Guard 07/C-1 — wire OTOPCUA0001 analyzer tree-wide + triage (surfaced the RESILIENCE-DISPATCH-GAP, task #10) | | `fix/archreview-s1-s2-ci-coverage` | `10b89830` | Guard 07/S-1,S-2,S-4 — whole-solution CI leg + fail-on-skip gate + CLI sleep deflake | | `fix/archreview-crit10-wire-capability-invoker` | `62556c24` | **#10 — wire CapabilityInvoker into dispatch via a Core.Abstractions seam** (off `fix/archreview-c1-wire-analyzer`, NOT master — the analyzer is the standing guard + the pragmas live there). Code-complete + unit/analyzer/context-verified; live gate pending. | +| `fix/archreview-crit13-resilience-config-artifact` | `75403caa` | **#13 — apply per-instance ResilienceConfig from the artifact** (off the crit10 branch — builds on its invoker factory). Read-path plumbing + invalidate/respawn-on-change; deterministically verified. | **Decision (user):** one branch per Critical, each off `master`. **#9 (failover test) lives on the crit1 branch** (not its own branch off master) because SBR only exists there — it is Critical 1's integration half. The plans are only on the docs branch, @@ -39,6 +40,7 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif | **#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. | +| **#13 — apply per-instance ResilienceConfig from the artifact** | RESILIENCE-DISPATCH-GAP sub-gap (from #10) | `fix/archreview-crit13-resilience-config-artifact` `75403caa` | The `DriverInstance.ResilienceConfig` column was authored+persisted (AdminUI) and already in the artifact (the composer serializes the whole entity), but the driver-node read path dropped it → tier-defaults only. `DriverInstanceSpec` now carries it; `TryReadSpec` reads it; `Create` takes+layers it (+ logs parse diagnostics, never throws); `SpawnChild` threads it. **Invalidate-on-change:** `Create` calls `builder.Invalidate(instance)` (the cache is options-blind on a hit) so a respawn with changed options rebuilds; `DriverSpawnPlanner` respawns on a ResilienceConfig change (incl. null→json), a pure DriverConfig change stays an in-place delta. **Verified:** override reaches execution (retryCount:0 suppresses tier-A retry; a control retries), invalidate is per-instance, malformed logs+falls-back; planner+artifact tests. Core.Tests 243 (+5), Runtime.Tests 363 (+5). See [`FOLLOWUP-13`](FOLLOWUP-13-resilience-config-artifact.md). | ## Task list (TaskCreate IDs) @@ -55,24 +57,26 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif | 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) | ✅ 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. | +| 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 (also covers #13's override-behavior end-to-end). | +| 13 | **Plumb per-instance `ResilienceConfig` through the deploy artifact** (from #10) | ✅ completed — `fix/archreview-crit13-resilience-config-artifact` `75403caa`; read-path plumbing (write side was already complete), invalidate-on-change + respawn-on-change; Core.Tests 243/Runtime.Tests 363. See [`FOLLOWUP-13`](FOLLOWUP-13-resilience-config-artifact.md). | ## 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). +> [`FOLLOWUP-11-crit1-sbr-premise.md`](FOLLOWUP-11-crit1-sbr-premise.md) · +> [`FOLLOWUP-13-resilience-config-artifact.md`](FOLLOWUP-13-resilience-config-artifact.md). - **#10 — RESILIENCE-DISPATCH-GAP (from wiring the analyzer, 07/C-1): ✅ IMPLEMENTED** on `fix/archreview-crit10-wire-capability-invoker` `62556c24` — code-complete + unit/analyzer/context-verified; **live behavioral gate pending** (recipe in [`FOLLOWUP-10`](FOLLOWUP-10-resilience-dispatch-gap.md)). The filed "thread CapabilityInvoker into the actor" was infeasible (Runtime is deliberately Polly-free); shipped a seam mirroring `IDriverFactory` instead. Only **6** of the 7 sites were real (the 7th, `GenericDriverNodeManager`, is - test-only scaffolding). **Two residual sub-findings:** (a) per-instance `ResilienceConfig` isn't in the deploy - 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. + test-only scaffolding). **Two residual sub-findings:** (a) ~~per-instance `ResilienceConfig` isn't in the deploy + artifact → tier-defaults only~~ **✅ RESOLVED by task #13** (`75403caa`) — the column was already in the artifact; + the read path now applies it (see [`FOLLOWUP-13`](FOLLOWUP-13-resilience-config-artifact.md)); (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 (still open as an Admin-UI follow-on). - **#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