dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

docs(archreview): mark guards 07/C-1, 07/S-1,S-2,S-4 + #9 failover done; record findings #10, #11

Browse Source
This commit is contained in:
Joseph Doherty
2026-07-08 18:27:16 -04:00
parent a203950848
commit 14f0cae7d9
3 changed files with 52 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
archreview/plans/03-server-runtime-plan.md
+7 -4
View File
@@ -1,9 +1,12 @@
# Design & Implementation Plan — Server & Runtime Subsystem (Report 03)
> **Status (2026-07-08):** ✅ **S1 (Critical) activation DONE** — branch `fix/archreview-crit1-split-brain-resolver`
> @ `a81dea10` (typed `KeepOldestOption`, unit guards + e2e config-resolution verified). ✅ **U1 doc DONE**
> (CLAUDE.md KNOWN LIMITATION 2 corrected). ⬜ S1's hard-kill failover **integration test** still pending (task 9,
> effort-M). ✅ **U2 (reflection forwarding test) DONE** — branch `fix/archreview-u2-deferred-sink-forwarding-guard`
> **Status (2026-07-08):** ✅ **S1 (Critical) typed-option DONE** — branch `fix/archreview-crit1-split-brain-resolver`
> @ `a25c9ed0` (typed `KeepOldestOption`, unit guards + e2e config-resolution verified). ✅ **U1 doc DONE**
> (CLAUDE.md KNOWN LIMITATION 2 corrected). ✅ **S1 hard-kill failover integration test DONE** (task 9, same branch
> `a25c9ed0`) — `HardKillFailoverTests` LIVE-verified in-process + negative-control-proven. ⚠️ **that negative
> control surfaced task #11: SBR was ALREADY active on master** (Akka.Cluster.Hosting default + pre-existing
> akka.conf keep-oldest), so Crit-1's "was NoDowning" premise is inaccurate — correct the rationale/docs.
> ✅ **U2 (reflection forwarding test) DONE** — branch `fix/archreview-u2-deferred-sink-forwarding-guard`
> @ `a65c2ced` (DispatchProxy-based exhaustive forwarding guard over `IOpcUaAddressSpaceSink` +
> `ISurgicalAddressSpaceSink` + `DeferredServiceLevelPublisher`; negative-control verified; the gate for P1/P2 is
> now in place). Remaining: P1, S2/S3/S4. See [`STATUS.md`](STATUS.md).
archreview/plans/07-client-tooling-engineering-plan.md
+6 -3
View File
@@ -1,8 +1,11 @@
# Plan 07 — Client Tooling, Analyzers & Cross-Cutting Engineering System
> **Status (2026-07-08):** not started (the ScriptAnalysis-policy doc drift it references was fixed in
> CLAUDE.md). Systemic guards: C-1 (wire OTOPCUA0001 analyzer, task 7 — triage before Warning) + S-1/S-2
> (CI to all suites + fail-on-skip, task 8 — pair S-4 flakes). See [`STATUS.md`](STATUS.md).
> **Status (2026-07-08):** ✅ **C-1 DONE** — `fix/archreview-c1-wire-analyzer` `f0082af5` (OTOPCUA0001 wired
> tree-wide + ~280 hits triaged; negative-control-verified live; **surfaced the RESILIENCE-DISPATCH-GAP, task
> #10**). ✅ **S-1/S-2/S-4 DONE** — `fix/archreview-s1-s2-ci-coverage` `10b89830` (whole-solution unit leg +
> `scripts/ci/assert-not-all-skipped.sh` fail-on-skip gate + CLI sleep→readiness deflake; skip-gate proven
> locally, CI-run verification pending a push). Remaining in this domain: S-3 (nightly E2E no-op), S-7
> (global.json), U-3/U-4/U-5/U-6 hygiene, C-2..C-6. See [`STATUS.md`](STATUS.md).
| | |
|---|---|
archreview/plans/STATUS.md
+39 -15
View File
@@ -9,13 +9,16 @@ 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` | `a81dea10` | Critical 1 (03/S1) activation + guard test + Redundancy.md |
| `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-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 |
| `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 |
**Decision (user):** one branch per Critical, each off `master`. The plans are only on the docs branch,
**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,
so from a critical branch read them via `git show docs/archreview-plans-and-drift-fixes:archreview/plans/0X-<name>.md`.
## Completed
@@ -30,6 +33,9 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif
| **Critical 3 — S7 lazy reconnect** | 05/STAB-1 | `fix/archreview-crit3-s7-reconnect` `25c0c6f6` | New `IS7Plc`/`IS7PlcFactory` seam + `EnsureConnectedAsync` (dispose-dead-handle → reopen on next data call); connection-fatal faults (socket/`ErrorCode.ConnectionError`) mark the handle dead, data-address errors do NOT; probe loop routes through it as a backstop. 4 unit guards + full S7 suite 234/234; **LIVE-verified** against real python-snap7 — bounced the container (`docker restart`), driver observed the outage and resumed Good reads with no redeploy (`S7_1500ReconnectTests`, double-gated on sim + `S7_RECONNECT_BOUNCE_CMD`). Also closes the S7 TEST-1 gap. |
| **Critical 4 — TwinCAT native-notification replay** | 05/STAB-2 | `fix/archreview-crit4-twincat-replay` `af318fb4` | Native ADS notifications were opaque handles (no replayable intent) → silently orphaned on a client swap. Now `NativeRegistration` (symbol/type/bit/interval/onChange + swappable handle) hangs off `DeviceState`; `EnsureConnectedAsync` split into a gate wrapper + under-gate core that **replays every intent onto a freshly-built client** (register+replay both under `ConnectGate`); probe-failure `RecycleClientAsync` disposes+nulls the wire-dead-but-port-open client so the next tick rebuilds+replays. 4 unit guards + full TwinCAT suite 174/174. **Unit-only — no TwinCAT docker fixture exists** (integration needs a real TC3 XAR), so the fake-client tests are authoritative (no live gate like S7 had). |
| **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).** |
| **#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)
@@ -41,22 +47,36 @@ so from a critical branch read them via `git show docs/archreview-plans-and-drif
| 4 | Critical 3 — S7 reconnect (05/STAB-1) | ✅ completed |
| 5 | Critical 4 — TwinCAT ADS re-registration (05/STAB-2) | ✅ completed |
| 6 | Guard — DeferredAddressSpaceSink reflection forwarding test (03/U2) | ✅ completed — `a65c2ced`; gate for 03/P1 + 02/P now in place |
| 7 | Guard — wire OTOPCUA0001 analyzer + triage (07/C-1) | ⬜ pending — triage before Warning severity; C-1 triage → C-2 |
| 8 | Guard — CI to all unit suites + fail-on-skip (07/S-1,S-2, pair S-4) | ⬜ pending |
| 9 | Critical 1 follow-up — hard-kill failover integration test (03/S1) | ⬜ pending — the effort-M harness half; activation itself is done+verified |
| 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. |
| 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)
- **#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.
- **#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."
## Suggested next (from 00-INDEX execution order)
**All 4 Criticals are DONE** + the load-bearing forwarding guard (task 6) has landed. Remaining work is the two
remaining systemic guards + the Critical-1 harness half + failure-visibility Highs:
**All 4 Criticals + all 3 systemic guards (tasks 6/7/8) + the #9 failover test are DONE.** Remaining:
1. **Systemic guards** — task 6 (`DeferredAddressSpaceSink` reflection forwarding test) ✅ **DONE** (`a65c2ced`),
so 03/P1 + 02/P surgical/batch adds can no longer ship inert. Next: task 7 (wire OTOPCUA0001 analyzer,
triage before Warning severity) → task 8 (CI to all unit suites + fail-on-skip, pair the S-4 fixed-sleep flakes).
2. **Critical 1 follow-up (task 9)** — the hard-kill failover integration test on the 2-node harness (the effort-M
harness half; SBR activation itself is done+verified).
3. **Failure-visibility Highs** — 01/S-1 (AddressSpaceApplyOutcome failure field), 06/S-1 (Galaxy fail-closed),
1. **Resolve the two surfaced findings** — task 10 (wire CapabilityInvoker into dispatch; behavior-changing,
live-gate) and task 11 (correct Crit-1's rationale/docs).
2. **Failure-visibility Highs** — 01/S-1 (AddressSpaceApplyOutcome failure field), 06/S-1 (Galaxy fail-closed),
03/S4 (primary-gate default-deny).
3. **P-tier** — 03/P1 surgical adds/removals (now gated by the U2 forwarding guard), 02/P batched `WriteValues`.
## Key resume facts
@@ -66,6 +86,10 @@ remaining systemic guards + the Critical-1 harness half + failure-visibility Hig
each add new sink capability methods that ship inert unless forwarded through `DeferredAddressSpaceSink`
(F10b/PR#423 precedent). The reflection guard now fails loudly if any such member isn't forwarded — add the
member to the recorder-covered interface set and it's auto-checked.
- **Nothing is pushed.** When the user asks to integrate: PR each `fix/archreview-crit*` + the `u2` guard branch;
the docs branch can merge first (plans + doc fixes are low-risk).
- **Nothing is pushed.** 8 local branches: the 4 `fix/archreview-crit*` (crit1 now carries #9 too), the `u2`,
`c1-wire-analyzer`, and `s1-s2-ci-coverage` guard branches, + the docs branch. The docs branch can merge first
(plans + doc fixes are low-risk).
- **CI changes can't be run-verified without a push** — the S-1/S-2 workflow + skip-gate were validated locally
(YAML parse + script against real/synthetic trx), but the actual CI-job run is only observable once pushed.
- **Two findings (#10, #11) need user decisions** before those areas are "closed" — see the Findings section above.
- **Plans are on the docs branch only** — see topology note above for cross-branch reads.