diff --git a/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md b/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md index 4c0005c6..057c4241 100644 --- a/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md +++ b/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md @@ -1,8 +1,71 @@ # 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). +> **Status:** IMPLEMENTED (code-complete + unit/analyzer/context-verified) · live behavioral gate PENDING · +> **Branch:** `fix/archreview-crit10-wire-capability-invoker` @ `62556c24` (off `fix/archreview-c1-wire-analyzer`, +> NOT master — the analyzer is the standing guard + the pragmas to remove live there) · **Surfaced by:** guard +> 07/C-1 · **Task:** #10 · **Severity:** High · **Risk:** Medium (behavior-changing hot path). See [`STATUS.md`](STATUS.md). + +## OUTCOME (2026-07-08) — implemented as a Core.Abstractions SEAM, not a direct thread + +**The filed plan below was INFEASIBLE as written.** It assumed `DriverInstanceActor` could reference +`CapabilityInvoker` directly. It cannot: **Runtime is deliberately Polly-free** — it references +`Core.Abstractions`, NOT `Core` (where `CapabilityInvoker` + Polly live), the exact boundary `IDriverFactory` +documents ("so the Runtime project doesn't pull in Core which would drag in Polly + driver hosting"). Threading +the concrete invoker in would reverse that architectural decision. + +**Fix shipped = a seam mirroring `IDriverFactory`:** +- `Core.Abstractions`: `IDriverCapabilityInvoker` (+ `NullDriverCapabilityInvoker` pass-through), + `IDriverCapabilityInvokerFactory` (+ null factory). +- `Core`: `CapabilityInvoker` implements the interface; new `DriverCapabilityInvokerFactory` builds a per-instance + invoker over the singleton pipeline builder + status tracker + `DriverFactoryRegistry.GetTier`. +- `Runtime`: `DriverInstanceActor` takes an `IDriverCapabilityInvoker` (default = pass-through) and wraps its + **6** dispatch sites; `DriverHostActor.SpawnChild` injects the real invoker built by the factory (resolved from + DI in the Runtime SCE like `IDriverFactory`). +- `Host`: `DriverFactoryBootstrap` registers the tracker + builder + concrete factory. +- **Analyzer**: `IDriverCapabilityInvoker.ExecuteAsync/ExecuteWriteAsync` added as wrapper homes (interface-typed + invoker calls must suppress OTOPCUA0001 too). All 6 `RESILIENCE-DISPATCH-GAP` pragmas removed. + +**Two triage corrections vs. the filed table below:** +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. + +**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 +retry / breaker-open / breaker-close **structured logging** to `DriverResiliencePipelineBuilder` (optional ILogger, +wired in the Host DI). This is the operator-facing observability surface AND the prerequisite for any live-verify. + +**Verification done (deterministic + local):** +- Negative control: unwrapping any site fails the Runtime build with OTOPCUA0001. +- Recording-invoker Runtime test: write routes via `ExecuteWriteAsync` with the `IPerCallHostResolver` host; + subscribe via `ExecuteAsync` — proves runtime routing + capability + host key. +- **Real-`CapabilityInvoker` actor-context test**: a genuinely-yielding discovery driven through the real invoker + publishes `DiscoveredNodesReady` — proving the invoker's internal `ConfigureAwait(false)` does NOT leak to the + actor's `await` (the one path pass-through can't cover; needed a test-only Core ref in Runtime.Tests). +- Analyzer test: the interface is a valid wrapper home. Pipeline-builder test: retry events are logged. +- Full solution builds clean (0 errors); Runtime.Tests 358, Core.Tests 238, Analyzers 32 green; pass-through + default keeps existing dispatch tests byte-for-byte unchanged. + +**RESIDUAL — live behavioral gate (pending, recipe below).** The two real risks (routing + actor-context) are now +covered deterministically. What's unrun is an end-to-end rig check. Recipe: +1. `docker compose -f docker-dev/docker-compose.yml build central-1 central-2` (builds the Host FROM SOURCE → + picks up this branch; note: the container restore hits the private Gitea feed — may need feed creds/nuget.config + in the build context). +2. Deploy a Modbus driver (AdminUI `:9200` or `POST /api/deployments`) pointing at the modbus sim + (`10.100.0.35:5020`). +3. Force an **exception** on an invoker-wrapped call: bounce the sim (`ssh dohertj2@10.100.0.35 docker restart `) + while the actor is (re)subscribing/discovering — reads are subscription-based (NOT invoker-wrapped) and device + rejects return bad-status (no exception), so only connection-level exceptions on subscribe/discover/write trip + retry/breaker. +4. Observe the `Driver resilience retry` / `circuit-breaker OPENED` log lines in the central-1 container logs, and + recovery (`circuit-breaker closed`). Also confirms the Host DI resolves + injects the factory end-to-end. + +--- + +## (Original filed analysis — retained for history; superseded by OUTCOME above) ## The finding diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index e0963191..8e0c9afe 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -16,6 +16,7 @@ truth for remediation progress. Update it as work lands. | `fix/archreview-u2-deferred-sink-forwarding-guard` | `a65c2ced` | Guard 03/U2 — reflection-exhaustive deferred-sink forwarding test (+ DeferredServiceLevelPublisher) | | `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. | **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, @@ -35,6 +36,7 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif | **Guard — reflection-exhaustive deferred-sink forwarding** | 03/U2 | `fix/archreview-u2-deferred-sink-forwarding-guard` `a65c2ced` | New `DeferredSinkForwardingReflectionTests` (Commons.Tests) reflects over **every** method of every forwarding interface `DeferredAddressSpaceSink` implements (`IOpcUaAddressSpaceSink` + `ISurgicalAddressSpaceSink`) — plus the sibling `DeferredServiceLevelPublisher` — invokes each on the wrapper over a `DispatchProxy` recording inner, and asserts the call **reached** the inner. A `DispatchProxy` recorder auto-implements future members (zero maintenance); a guard-of-the-guard asserts the wrapper implements exactly the known forwarding-interface set, so a brand-new capability *interface* trips too. **This closes the F10b/PR#423 trap class and is the prerequisite gate for 03/P1 remove methods + 02/P `WriteValues`.** 3 new tests green (Commons.Tests 57/57); **negative control verified** — stubbing `UpdateFolderDisplayName` to a non-forwarding no-op fails the guard with the exact member-named diagnostic. NO production change. | | **Guard — wire OTOPCUA0001 analyzer + triage** | 07/C-1 | `fix/archreview-c1-wire-analyzer` `f0082af5` | Injected the analyzer tree-wide via `Directory.Build.props` (`OutputItemType="Analyzer"`), then triaged ~280 surfaced hits: **(1) RESILIENCE-DISPATCH-GAP** — 7 production sites (`DriverInstanceActor` ×6 + `GenericDriverNodeManager`) scoped-`#pragma`'d as a **real tracked gap** (CapabilityInvoker built but never wired into dispatch; **new task #10**); **(2) driver-internal self-calls** — 3 sites (AbCip/S7 own poll/ack) `#pragma`'d as intentional; **(3) 12 wire-level test/CLI projects** → commented `NoWarn`. Full build green, 0 hits; analyzer 31/31; **negative control** — dropping one dispatch-gap pragma re-fires OTOPCUA0001 and fails the build, proving it's live tree-wide. | | **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. | ## Task list (TaskCreate IDs) @@ -50,7 +52,7 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif | 7 | Guard — wire OTOPCUA0001 analyzer + triage (07/C-1) | ✅ completed — `f0082af5`; surfaced task #10 | | 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) | ⬜ pending — real gap the analyzer caught; behavior-changing hot-path remediation, needs a live rig gate. Own branch off master. | +| 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. | ## Findings surfaced by the guards (NEW — need user decision) @@ -59,12 +61,15 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif > [`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. - The 7 sites are `#pragma`-suppressed with a greppable `RESILIENCE-DISPATCH-GAP` marker (tree stays green + the - analyzer stays live everywhere else). Wiring the invoker in is a behavior-changing hot-path remediation that - needs a live rig gate — a genuine High, its own branch off master. +- **#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. - **#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