Files
lmxopcua/archreview/plans/FOLLOWUP-10-resilience-dispatch-gap.md
T
Joseph Doherty 5da7ba6517
v2-ci / build (pull_request) Successful in 4m29s
v2-ci / unit-tests (pull_request) Failing after 10m15s
docs(#456): live grind finding — write-trigger structurally impossible
Ran the full item-1 grind (authored a live Modbus equipment tag, connected
to the sim, LDAP-authed writes, docker pause the peer). 4 failing wrapped
writes produced 0 retry/breaker lines on either central.

Root cause (corrects the issue's hypothesis): ModbusDriver.WriteAsync swallows
all exceptions and returns WriteResult(StatusBadInternalError) — never throws;
CapabilityInvoker feeds only exceptions to Polly; breaker ShouldHandle is
Handle<Exception>; Write retry pinned to 0 (R2-02/S-8). So a failing wrapped
write emits no line by construction, for any polling driver. The line is
reachable ONLY for a session driver (OpcUaClient) faulting mid-Discover/Subscribe
(30s Polly timeout throws) — a production timing race, not deterministically
forcible on the rig. Behaviour stays unit-proven by the pipeline-builder test.

- New: archreview/plans/artifacts/456-retry-breaker-live-finding-2026-07-15.md
- FOLLOWUP-10 updated with the structural finding + recommendation to close item 1.
2026-07-15 14:48:03 -04:00

201 lines
16 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Follow-up #10 — Wire CapabilityInvoker into the production dispatch layer (RESILIENCE-DISPATCH-GAP)
> **Status:** IMPLEMENTED (code-complete + unit/analyzer/context-verified) · live gate RAN 2026-07-08 — DI wiring
> confirmed live, behavioral log-line not observed (disciplined stop; see LIVE GATE below) ·
> **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.~~ **✅ 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 had **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.
> **UPDATE (R2-10, branch `r2/10-resilience-observability`):** the tracker now HAS a proper reader. A periodic
> 5 s DPS publisher (`DriverResilienceStatusPublisherService`, driver nodes) mirrors each snapshot →
> `driver-resilience-status` topic → per-admin `DriverResilienceStatusBridge` → `IDriverResilienceStatusStore` →
> breaker/retry/in-flight/staleness chips on `DriverStatusPanel.razor` (mirrors the driver-health flow). Tracker
> truthfulness fixed too: `IsBreakerOpen`/`RecordBreakerClose` + `RecordSuccess` on the invoker success paths. The
> log-line remains the drill-down; the panel is the primary surface. **The live behavioral gate below (observing the
> `circuit-breaker OPENED` line on a dead endpoint) is now folded into R2-10's T10 live-`/run` — still pending.**
**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.
**LIVE GATE — ran 2026-07-08 (task #12). PARTIAL: DI wiring confirmed live; behavioral log NOT observed (disciplined
stop). Outcome:**
-**Built the Host image FROM SOURCE** on the crit13 tip (`docker compose … build central-1`) — #10 + #13 compile
and publish into a real container. The Gitea feed was **not** a blocker: it answers **200 anonymously**, so the
in-container `dotnet restore` pulled the `ZB.MOM.WW.*` packages fine (the recipe's feed-creds caveat is moot).
-**Stood up the live rig** (`sql` + `migrator` + `central-1`/`central-2` + `traefik`); central-1 booted
**healthy** — cluster up, OPC UA on `:4840`, AdminUI on `:9000` — running the new invoker wiring.
-**Prod DI binds the REAL factory — confirmed two ways:** (1) the new deterministic guard
`ResilienceInvokerFactoryRegistrationTests` (Host.IntegrationTests, `8a8b9ec5`) asserts
`AddOtOpcUaDriverFactories` resolves `IDriverCapabilityInvokerFactory` → the concrete
`DriverCapabilityInvokerFactory` (not the null pass-through) and that it mints a real `CapabilityInvoker`; (2) the
rig booted cleanly with that exact registration active. This closes the **primary** risk (was it wired in prod?).
- 🟡 **Behavioral retry/breaker LOG line — NOT observed live**, by a deliberate stop (not a failure). Reading the
Modbus driver showed **none of its cheap paths throw a wrapped exception**: `DiscoverAsync` is offline/config-driven
(enumerates `_options.Tags`, no connect), `SubscribeAsync` is lazy (`_poll.Subscribe`, no connect), and reads are
the **non-wrapped** poll path returning bad-status. The **only** invoker-wrapped Modbus site that throws a
connection exception is an **idempotent `WriteAsync`** to a dead endpoint — which needs full config authoring
(driver + UNS + equipment + idempotent-writable tag) **plus** a data-plane-LDAP-authorized Client.CLI write. That
multi-dependency chase isn't worth it: the retry/breaker **behavior** is already proven deterministically
(`CapabilityInvokerTests` retry, `DriverCapabilityInvokerFactoryTests` override, the real-invoker actor-context
test, the pipeline-builder logging test). To actually see the log line, S7 (whose reconnect path *does* throw on a
dead socket) or a Modbus idempotent-write-to-dead-endpoint would be the trigger.
- 🐞 **RIG BUG found + ✅ FIXED (unrelated to #10/#13):** the committed `docker-dev/docker-compose.yml` still
carried the **retired Wonderware `ServerHistorian` keys** (`Host`/`Port`/`SharedSecret`) with `Enabled: "true"`,
but the historian-gateway cutover made the code read `ServerHistorian__Endpoint` → **empty-URI crash at every dev
bring-up** (`HistorianGatewayClientAdapter.Create`, unhandled `UriFormatException`) — docker-dev was unbootable
against current code. **Fixed on `fix/docker-dev-serverhistorian-stale-keys` `7233e2ba` (off master, independent of
the archreview work):** central-1 + central-2 now use the gateway-shape keys (`Endpoint`/`ApiKey`/`UseTls`/
`AllowUntrustedServerCertificate`), **disabled by default** (`Enabled=${OTOPCUA_HISTORIAN_ENABLED:-false}`; the rig
has no historian gateway) with env opt-in. Verified live — central-1 boots clean with the committed compose (no
override, no `UriFormatException`).
Original recipe (retained): build central-1/central-2 from source → deploy a Modbus driver at the sim
(`10.100.0.35:5020`) → bounce the sim (`ssh dohertj2@10.100.0.35 docker restart <sim>`) during a wrapped call →
grep central-1 logs for `Driver resilience retry` / `circuit-breaker OPENED` / `closed`. NB the Modbus-path caveat
above — an idempotent write (or an S7 driver) is the reliable trigger, not a plain subscribe/read.
## UPDATE (2026-07-15, Gitea #456) — the write-trigger is STRUCTURALLY IMPOSSIBLE; line is OpcUaClient-only
Ran the full grind (authored a live Modbus equipment tag, connected to the sim, drove LDAP-authed writes, then
`docker pause`d the peer). **4 failing wrapped writes → 0 retry/breaker lines** on either central.
[Finding artifact](artifacts/456-retry-breaker-live-finding-2026-07-15.md). Root cause corrects the recipe's
"idempotent write is the reliable trigger" note:
- **`ModbusDriver.WriteAsync` swallows every exception → returns `WriteResult(StatusBadInternalError)` — it
never throws** (`ModbusDriver.cs:964-967`); the `CapabilityInvoker` only feeds *exceptions* to Polly
(`CapabilityInvoker.cs:84/112/147`, no result-status inspection); breaker `ShouldHandle` is `Handle<Exception>`
and `Write` retry is pinned to 0 (R2-02/S-8). So a failing wrapped **write** emits **no line by construction**,
for **any** polling driver (they all map write faults to a status). Even an idempotent write to a dead endpoint
returns a bad status, not a throw.
- Polling drivers' other wrapped sites don't throw through the seam either (`SubscribeAsync` lazy poll-reg;
`DiscoverAsync` offline/config-driven).
- **The line is reachable ONLY for a session driver (OpcUaClient)** whose session faults *mid-`Discover`/
`Subscribe`* (the 30 s Polly timeout on the post-connect discovery loop throws `TimeoutRejectedException`
`Discover` retry ×2). That is a genuine production event but an inherent **timing race** (connect must succeed,
the fault must land during the wrapped call before keepalive drops the driver to `Reconnecting`) — not
deterministically forcible on the rig. The behaviour stays **unit-proven** by the pipeline-builder logging test.
**Recommend closing item 1 on this finding.**
---
## (Original filed analysis — retained for history; superseded by OUTCOME above)
## 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).