From c4d6cba7e1e357c9da94e2e9a30632ca2139c598 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 18 Aug 2026 08:48:40 -0400 Subject: [PATCH] =?UTF-8?q?docs(plan):=20r3=20close-out=20=E2=80=94=20outc?= =?UTF-8?q?omes,=20sweep=20record,=20register=20annotations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/2026-08-17-perf-followups-r2.md | 20 ++ docs/plans/2026-08-17-perf-followups-r3.md | 363 +++++++++++++++++++++ 2 files changed, 383 insertions(+) diff --git a/docs/plans/2026-08-17-perf-followups-r2.md b/docs/plans/2026-08-17-perf-followups-r2.md index 3ad01266..51258611 100644 --- a/docs/plans/2026-08-17-perf-followups-r2.md +++ b/docs/plans/2026-08-17-perf-followups-r2.md @@ -564,22 +564,41 @@ closed on the round-1 register. **None blocks the branch.** one call. **Pre-existing and benign** — it self-heals on the next call and **no verdict is affected** (a deferred stamp cannot serve a stale grant; the generation gate on `TryGet` is what decides that, and it is unaffected). Recorded so the next reader does not re-derive it as a bug. + - **Closed by `441b34a3` (r3).** `InvalidateIfStale` is now a monotonic-max CAS **loop** with + the `<=` guard re-read inside each attempt, so a straggler that loses the CAS retries against + the moved stamp instead of falling out unpublished. **S7** - **Per-item decode health is last-writer-wins.** Pre-existing; now **pinned and documented** rather than fixed, because changing it is a behaviour change to how a mixed-outcome chunk reports driver health, not a test gap. + - **Closed by `4c161181` + `b35e423c` + `1765e300` (r3), for the READ path.** One + worst-state-wins accumulator per `ReadAsync` pass, one end-of-pass `_health` write, with + `LastSuccessfulRead` advanced whenever *any* item succeeded. ⚠️ **Modbus (all paths) and the + S7 WRITE path deliberately keep last-writer-wins** — recorded as a known asymmetry on the r3 + register, not an oversight. - **TM/CT decode at a non-zero device number still needs a real CPU.** The test exists and the env var (`S7_TIMER_NUMBER_ADDRESSING=1`) is ready; python-snap7 cannot serve it (see the BIT-address quirk above). + - **Carried (r3).** Unchanged — still blocked on hardware; explicitly out of scope in the r3 + plan. **docker-dev** - **The first-POST 404's mechanism is unconfirmed** until the new access log catches an occurrence. The gate prevents the symptom; the cause is still open. + - **Closed by `3f04a4d4` (r3).** Mechanism: the window between Traefik binding the `web` + entrypoint and loading the dynamic file-provider config — the listener accepts before the + router table exists (~65–160 ms on the rig; empty `RouterName`/`ServiceName`, `0ms`, Go's + `404 page not found`). The 503 candidate is excluded on evidence. ⚠️ **Not all 404s are + access-logged** — the accesslog handler is itself part of the not-yet-built dynamic chain, so + an empty log does not prove absence. - **The no-body `POST` `Content-Type` binding is unverified against a live rig** — carried as a header NOTE in `deploy.sh` rather than silently assumed correct. + - **Closed by `3f04a4d4` (r3).** Verified live across four request shapes: `DeployRequest?` + **binds null** (no 400, no 415), the request returns **202 Accepted**, and the handler's + fallback puts `CreatedBy = rest-api` — confirmed in the ConfigDb. **SQL test infrastructure** @@ -587,3 +606,4 @@ closed on the round-1 register. **None blocks the branch.** **accepted**: two test processes racing on the same instance can have one reap what the other just planted. The 24 h age floor makes it vanishingly unlikely in practice, and the alternative (a lock table on a shared dev server) costs more than it buys. + - **Carried (r3).** Still accepted, unchanged; explicitly out of scope in the r3 plan. diff --git a/docs/plans/2026-08-17-perf-followups-r3.md b/docs/plans/2026-08-17-perf-followups-r3.md index 37db4a9c..9292f0da 100644 --- a/docs/plans/2026-08-17-perf-followups-r3.md +++ b/docs/plans/2026-08-17-perf-followups-r3.md @@ -508,3 +508,366 @@ ScadaBridge closed the same class with conditional registration + boot-time self 4. Annotate the r2 register in `2026-08-17-perf-followups-r2.md` append-only with "Closed by (r3)" / "Gated live (r3)" sub-bullets. 5. Update the `.tasks.json` ledger; commit docs with explicit pathspec. + +--- + +## Outcomes + +*Written at close-out, 2026-08-18, on branch `perf/followups-r3`. All 10 tasks complete +(8 planned + Tasks 9/10 joined mid-round by amendment `088a1a1e`). This round is a +**follow-up round**: every code item it closes was opened by the round-2 register in +`2026-08-17-perf-followups-r2.md`, and that register is annotated in place with the closing +SHAs. Tasks 9 and 10 are **cross-repo** — rows 38 and 39 of the ScadaBridge deferred-work +register, relayed on Joe's behalf and closed here.* + +### Sweep record + +**The docker-dev rig was torn down BEFORE the sweep** (`docker compose -f +docker-dev/docker-compose.yml down`, 0 `otopcua-dev-*` containers remaining). This is not +housekeeping — a running rig un-skips env-gated tests and would have made the skip count +incomparable to the r2 baseline. It did: `Host.IntegrationTests` reports **229 / 0 / 2** with +the rig up and **226 / 0 / 5** with it down, the same 231 total. + +`dotnet build ZB.MOM.WW.OtOpcUa.slnx --no-incremental` → **0 errors, 904 warnings** — +**identical to r2's 904**, not merely close. The non-incremental flag matters: an incremental +build reported 896 because up-to-date projects re-emit nothing. The two pre-existing families +are unchanged in kind and in count (`OTOPCUA0001` CapabilityInvoker analyzer on test call sites +1012, `xUnit1051` cancellation-token advice 696, plus the small CS tail). **No delta to +attribute** — this round's new test call sites happened to land in files already carrying the +same families, so nothing moved. + +Full unit sweep — every `*.Tests` project, run per-project with `--no-build` after the single +solution build, against the tree at `837d8a17`. `*.IntegrationTests` are build-only by +convention (as in r2); `Host.IntegrationTests` is recorded **separately below** because r2's +baseline did not include it. + +**43 / 43 test projects green.** + +| | Round 2 | **This round** | Δ | +|---|---|---|---| +| Passed | 10 370 | **10 377** | **+7** | +| Failed | 0 | **0** | — | +| Skipped | 19 | **19** | — | +| **Total** | 10 389 | **10 396** | +7 | + +The +7 reconciles **exactly** against the two suites this round added tests to, with nothing +unaccounted for — confirmed at class granularity, not just project granularity: + +| Project | Round 2 | This round | Δ | Source | +|---|---|---|---|---| +| `OpcUaServer.Tests` | 481 | **482** | +1 | `SessionAclDecisionCacheTests` 17 → **18** (T1) | +| `Driver.S7.Tests` | 401 | **407** | +6 | `S7SpanReadParityTests` 13 → **19** (T2) | + +⚠️ **The S7 +6 is the sum of three separate commits, not one addition** — `4c161181` (+3, the +worst-state-wins pins), `b35e423c` (+1, the observe-nothing guard the spec review found), and +`1765e300` (+2, the code-review round's severity-escalation and OCE-flush pins). T3 (docs) and +T4–T7 (live gates) added no tests by design. + +**All 19 skips are the pre-existing env-gated ones, project for project** — `Core.Tests` (1), +`Driver.Galaxy.Tests` (8), `Driver.Historian.Gateway.Tests` (6, the `LiveIntegration` category +needing the `wonder-sql-vd03` VPN), `OpcUaServer.Tests` (1), `Runtime.Tests` (3). **This round +added no skips**, and `Runtime.Tests` is unchanged at 637 / 3 as expected. + +The DB-backed suites ran **against a reachable** `10.100.0.35,14330` — executed, not skipped: +`AdminUI.Tests` **1540 passed / 0 skipped**, `Configuration.Tests` **190 passed / 0 skipped**. + +#### `Host.IntegrationTests` (T9 + T10) — **226 passed / 0 failed / 5 skipped** (231 total) + +Not part of the r2 baseline, so there is no measured predecessor. Derived from the diff against +the merge-base `49d05bc2`, the expected delta is **+9**, all plain `[Fact]` (no theories, so +attribute count equals test count): `LocalDbSetupTests` **7 → 15** (+8) and `LocalDbWiringTests` +**6 → 7** (+1); `RoslynVirtualTagEvaluatorTests` stays **20 → 20** because T9 reworked its +racing test *in place*. ⚠️ **The task brief's "LocalDbSetupTests 13 → 15" was an intermediate +reading** — 13 was the count after `3e62f161` and before the `df533916` review round; against +the merge-base it is 7 → 15. The 5 skips are the rig/fixture-gated +`DriverTestConnectE2eTests` (2, Modbus fixture) and `DriverProbeHandshakeE2eTests` (3) — three +of which run when the rig is up, which is exactly the teardown effect noted above. + +#### ⚠️ One flake, isolated and cleared — `Client.CLI.Tests` + +The first sweep pass reported `Client.CLI.Tests` **1 failed / 103 passed**: +`LoggerLifecycleTests.ConfigureLogging_DisposesPreviousLogger_BeforeReassigning`, asserting +`trackerSink.Disposed` was `False`. **Isolation re-runs cleared it: the single test alone +1/1 green, and the whole project green twice more (104 / 104 both times).** + +The mechanism is a **pre-existing intra-assembly race on the process-global +`Serilog.Log.Logger` static**, not a regression: the test assigns `Log.Logger` at +`LoggerLifecycleTests.cs:26` and then asserts, at `:41`, that the command's `ConfigureLogging` +disposed *that* instance — but the class carries no `[Collection]` and no +`DisableTestParallelization`, so any concurrently-scheduled test whose command path also calls +`ConfigureLogging` (which does `Log.CloseAndFlush()` then reassigns) can swap the global out +from under it in that window. **No commit in this round touches `src/Client` or `tests/Client`** +(`git log 49d05bc2..HEAD -- src/Client tests/Client` is empty). Recorded here rather than in +the register, which the close-out brief scoped explicitly; it is a worthwhile one-line fix +(pin the class to a serial collection) whenever someone is next in that file. + +### Per-task outcomes + +| # | Task | Commit(s) | Outcome + deviation | +|---|---|---|---| +| 1 | ACL `InvalidateIfStale` — monotonic-max CAS loop | `441b34a3`, `f4b91dbb`, `e29fab19` | Done; closes the r2 register's deferred-publication item. ⚠️ **The plan's own 200-round `Barrier` harness was measured GREEN-ON-DEFECT** (~1-in-19k hit rate — `Barrier`'s post-phase release hands one racer a head start), so it was rebuilt as long-lived racers with a third-thread release store: 25–50 % of rounds red, falsification red at rounds 1–13. 10 000 rounds kept over the reviewer's suggested 2 000 after measuring a **1-in-3 vacuous pass at 2 000** under full-suite parallelism (misses burst-correlate). 18/18. | +| 2 | S7 read-pass health — worst-state-wins | `4c161181`, `b35e423c`, `1765e300` | Done; closes the r2 register's last-writer-wins item **for the S7 READ path only** (asymmetry recorded below). The spec review found a second defect in-round — an **empty pass that observed nothing must report nothing**, or it clears a live `Faulted`. The code review then found that the OCE arms skipped the end-of-pass write and froze `LastSuccessfulRead`; the coordinator ruled the OCE path must **flush** the recorded pass evidence via `finally`, which is what protects the watchdog contract. Five falsifications, each red on its own test. 19/19. | +| 3 | Stale `OTOPCUA_CONFIG_MODE` docs | `e9ba1741` | Done as specified; compose facts verified before editing. Historical plan docs deliberately untouched. Trivial ⇒ no review. | +| 4 | Live gate — first-POST 404 + no-body POST | `3f04a4d4` | **PASSED — and the 404 mechanism is now NAMED**, closing an r2 register item the gate was only expected to instrument. Detail below. | +| 5 | Live gate — FetchAndCache legs | *(none — gate only)* | **PASSED, all six legs.** Detail below. | +| 6 | Live gate — alarm-bearing surgical ratchet | *(none — gate only)* | **PASSED, all four routings.** Detail below. | +| 7 | Live gate — ACL-enabled enforcement | *(none — gate only)* | **PASSED**, with a topology adaptation and two non-blocking findings. Detail below. | +| 9 | Roslyn evaluator racing test — remove the timing assumption | `34829ea0`, `837d8a17` | Done. **Cross-repo: ScadaBridge deferred-work register row 38**, relayed 2026-08-18. Detail below. | +| 10 | LocalDb conditional registration + baseline + deregister self-heal | `3e62f161`, `df533916` | Done. **Cross-repo: ScadaBridge deferred-work register row 39.** No pin bump. **The scadaproj umbrella index was updated and pushed to `main` as `1145df6`.** Detail below. | +| 8 | Close-out — sweep + Outcomes + register annotations | *this commit* | Done. Rig torn down first, full build + sweep, this Outcomes section, and the append-only r2 register annotations. | + +### Task 4 — the first-POST 404, mechanism CONFIRMED + +**2026-08-18, local OrbStack docker-dev rig built from the branch tip. Two results, and the +second is the one that matters.** + +- **The prescribed fresh-boot run did NOT reproduce the 404** — the entire Traefik access log + for the boot window was **3 lines, all 2xx**, every one carrying + `RouterName = otopcua-admin@file`. ⚠️ **And `deploy.sh` under `up -d` can never reproduce + it**: Compose blocks on the `depends_on: service_healthy` chain, so `up -d` returned ~30 s + *after* Traefik started and the first poll landed long after the window closed. +- **So the window was exercised directly** — rapid-poll (~10 ms) across + `docker restart otopcua-dev-traefik-1`, twice. Result: `000` (port unbound) → **`404` for + ~65–160 ms** → `200`. Body is exactly `404 page not found` (Go's `http.NotFound`), and the + access-log signature is the discriminator the r2 round asked for: **`RouterName = "-"`, + `ServiceName = "-"`, `0ms`** — no router matched, no backend dialled. +- **NAMED MECHANISM: the window between Traefik binding the `web` entrypoint and loading the + dynamic file-provider config from `/etc/traefik/dynamic.yml`.** The listener accepts + connections before the router table exists. +- **The competing candidate is excluded on evidence, not argument** — a health-checked service + with no healthy server yet would be a *matched* router answering **503**; there are **zero + 503s** anywhere in the session's access log. +- ⚠️ **Caveat now recorded in both `deploy.sh` and the README: not all 404s are + access-logged.** Probe 1's seven 404s produced **zero** log lines; probe 2's produced 4 of 7. + The accesslog handler is itself part of the dynamic chain that has not been built yet, so + **an empty access log does NOT prove the 404 did not happen.** +- **No-body POST binding — VERIFIED**, four request shapes (script stdin, no data flag at all, + explicit empty body, valid JSON), all reaching the handler. The minimal-API `DeployRequest?` + **binds null**; it does not 400 and does not 415. A clean post-seal run returned **HTTP 202 + `Accepted`**, and the DB confirms the null took the fallback: `CreatedBy = rest-api`. + +### Task 5 — FetchAndCache, all six legs + +**PASSED. Two independent full cycles** (deploy `1e990163` rev `0b9289fb`, then the revert +deploy `da56012f` rev `315de861`), both sealed `acks=6`. + +- **Fetch + SHA-verify:** all four site nodes — + `Fetched + verified artifact for 1e99016312004ceca5f73aaba2c899b1 from http://central-1:4055 (1584 bytes).` + The template only logs *after* the SHA-256 == RevisionHash check, so its presence is the + verify proof. The byte count tracked the composition (1584 → 1468 on revert). +- **Serve (central):** four `Served artifact for … (1584 bytes, 1 chunks)` on `central-1`. +- **Applied-from-fetch + cached:** the **`applied fetched deployment`** variant on every site + node — and the two `Direct` central nodes emitted the plain `applied deployment` form on the + *same* deploy, which is the falsification control that the two templates are distinguishable. +- **Boot-from-cache:** after `restart site-a-1`, + `FetchAndCache boot — restored served state for deployment 1e9901… (rev 0b9289fb…) from the LocalDb pointer.` + — the wanted variant, not the empty-cache or failed-pointer ones, matching what the node + cached 52 s earlier. +- **Absence proof** over the node's whole 302-line lifetime: `entering Stale` **0**, any + `stale` **0**, `ConfigDb back` **0**, Direct-form apply **0**, and `[ERR]`/`[WRN]`/`[FTL]` + **0**. ⚠️ The Direct-form check must anchor on `": applied deployment "` — a naive + `grep "applied deployment"` matches the *cache* template and reports a false positive. +- **Bonus:** a post-restart browse showed both namespaces live (`ns=2` raw + `ns=3` UNS) + including the temporary gate area — proof the node materialised the address space from the + artifact it fetched and restored, not from a stale cache. +- ⚠️ **Method note worth carrying:** **same-revision deploys short-circuit the fetch path** + entirely (`DriverHostActor.HandleDispatchFromSteady` immediate-ACK). A bare re-run of this + gate on unchanged config shows **zero fetch lines** and that is not a failure — force a new + revision first. The gate did so with a temporary `UnsArea` row, then reverted; the revert + deploy came back **byte-identical** to the pre-gate revision. + +### Task 6 — the alarm-bearing surgical ratchet, four deploys + +**PASSED. All four applier lines emitted identically on `central-1` and `central-2`**, every +deploy sealed 6/6: + +| Step | Lever | Applier line | Verdict | +|---|---|---|---| +| 1 | `AccessLevel` ReadWrite → Read | `kind=AttributeOnly, changed=1, surgicalRawTags=1, rebuild=False` | surgical ✅ | +| 2 | alarm ADD | `kind=Rebuild, changed=1, surgicalRawTags=0, rebuild=True` | rebuild ✅ (`Alarm != null` current side) | +| 3 | alarm REMOVE | `kind=Rebuild, changed=1, surgicalRawTags=0, rebuild=True` | rebuild ✅ (**previous** side also disqualifies) | +| 4 | `AccessLevel` Read → ReadWrite | `kind=AttributeOnly, changed=1, surgicalRawTags=1, rebuild=False` | surgical ✅ — **the ratchet is per-deploy, not latched** | + +- **Deviation, and why it is sound:** the rig's ConfigDb holds exactly **one** `Tag` row, so + TagS and TagA are the same tag exercised sequentially. Each step is an independent deploy + producing exactly **one** raw-tag delta over orthogonal fields, so every verdict is + unambiguous; a second tag would have added an *added-node* delta for no gain. +- **Proven at the artifact seam, not just the log:** each `Deployment.ArtifactBlob` was pulled, + gunzipped and semantically diffed — **each step's artifact differs by exactly the intended + lever and nothing else.** +- **Only the cluster-MAIN nodes emit an applier line.** Site nodes acked every deploy but + logged nothing: `DeploymentArtifact.ResolveClusterScope` self-scopes the fleet-wide artifact, + the site plan is empty, and `Apply` early-returns on `plan.IsEmpty` before the summary log. + Confirmed they *did* process each artifact via their authority-generation lines. +- ⚠️ **A live-gate fact worth knowing for every future rig gate: the revision hash can never + return to its baseline after any `UPDATE`.** `ConfigComposer` snapshots the SQL Server + `rowversion` column into the artifact, and `rowversion` increments on every `UPDATE` including + one that writes back an identical value. A byte-diff confirmed `RowVersion` was the **only** + difference in the entire 4922-byte artifact. Semantic restore was verified instead (config + hash back to `7F75AB45…`, rowcount unchanged). + +### Task 7 — ACL-enabled enforcement, first live run since the cache re-shape + +**PASSED**, with a forced topology adaptation and two findings. + +- ⚠️ **The planned gated node was not executable.** `site-a` serves **containers only** — the + rig's single tag belongs to cluster **MAIN**, so `site-a-1`'s authority generation always + reports `keys=0, grants=0, resolvedScopes=0` and there is no writable value node to authorize. + The gate moved to **`central-1`** (fused `admin,driver,cluster-MAIN`, serving + `ns=2;s=pymodbus/plc/HR200`) via a **scratchpad-only compose overlay** — the repo compose + file was never edited — with **`central-2` as the ungated A/B control**. Consequence recorded: + central-1 is a ConfigDb node so *key* revocation is immediate rather than next-deployment; + **grants still arrive only via the artifact**, so the code actually under test is exercised + as intended. +- **Authoring fell back to direct ConfigDb INSERT** (the Chrome extension was not connected). + `ApiKeySecret.Generate` was reproduced from source and the pepper read live off the container; + the construction is proven correct because the server accepted the key. +- **The ALLOW leg is genuinely load-bearing, because the ungated baseline was captured first.** + The *identical* key and call on the *ungated* node returned `0x801F0000` — the `Tag`-scoped + grant confers no role, so `ApiKeyInterimRoleMapper` leaves the session with `roles []` and the + server-wide gate denies. With the gate on, the same call returned **`Write successful`**. + That is the live demonstration that **the ACL decision REPLACES the role check** rather than + chaining with it — a deny-only gate would have passed against a build that never consults the + ACL. +- **Denials fail closed:** anonymous write and read both `0x801F0000` with + `AclGate: Read withheld 1 of 1 node(s) for principal anonymous`; out-of-scope browse refused; + the ungated control node still open on the same NodeId. +- **Revocation was split into its two distinct code paths, and only the first tests this + round's fix.** *Grant removal* → generation **1 → 2**, `grants=0, resolvedScopes=0`, a + deliberately **primed** decision cache discarded, and the denial arrives while + `ApiKeyAuth: granted` still logs — that is the per-key-shard/CAS-loop path. *Key disable* → + `0x80210000` `BadIdentityTokenRejected` and + `ApiKeyAuth: denied — no enabled, unexpired key with id …` — a different status, because the + session never activates and never reaches the gate. +- **Restore verified by hash:** the post-task revision returned to the exact pre-task + `a3a9061bbf50…`, with 0 `ApiKey` / 0 `NodeAcl` rows and the ENABLED warning gone. + +### Task 9 — the racing test, and an honest disclosure + +Renamed to `Evaluate_racing_ClearCompiledScripts_never_leaks_a_raw_disposed_failure` and +re-shaped around **three-bucket classification** (leaked ⇒ fail on first; exhausted and starved +each bounded at `max(evals/10, 8)`) with duration-bounded loops (10 s **or** 400 iterations). + +- ⚠️ **The real load failure was not the one the ticket described.** It was the test's own 45 s + `WaitAsync` budget expiring under recompile pressure, not the retry legitimately exhausting. + Old shape: **RED 4/4** at 4-way concurrency. New shape: **GREEN 4/4, then 8/8**. +- The review found a further flake vector — `TimedScriptEvaluator`'s 5 s clock starts at + **enqueue**, so a starved evaluation can time out without any product fault. Closed and + falsified with a 12 ms-budget probe (old classification RED, new GREEN at a 0.11 % starve rate). +- Failure-reason literals were promoted to a shared `ScriptEvalFailureReasons` in Commons, used + by both evaluators and the test. +- ⚠️ **Disclosed honestly: the disposed race does not reproduce on this arm64 macOS box at + all.** The raw-escape assertion is the load-independent guarantee; the exhaustion/starvation + bounds are calibrated loose (see the register). + +### Task 10 — LocalDb registration now follows replication config + +Ported from ScadaBridge's WP1.3/WP3.3 pattern. Predicate = **`PeerAddress` OR `ApiKey`** +(`SyncListenPort` deliberately excluded — the passive endpoint fails closed on the key). +Configured ⇒ `RegisterReplicated(table, baselineExistingRows: true)` per table in the pinned +DDL→register order; not configured ⇒ `DeregisterReplicated` on every table at boot as an +idempotent self-heal. The `CLAUDE.md` bullets that recorded the *unconditional* shape as +deliberate were rewritten in the same commit. + +- **No pin bump.** The 0.2.1 APIs were verified by reflection **and** a live probe (baseline + seeds `hlc=0`; `DeregisterReplicated` returns `bool`) rather than assumed from the changelog. +- **Two vacuous-fixture bugs were surfaced and fixed** in passing (`LocalDbPairHarness`, and an + `AlarmSfLegacyMigrator` two-config split) — fixtures that would have let the new predicate + pass without being exercised. +- **Pair-symmetry is the operational trap**, recorded in `CLAUDE.md` and the runbook: the sync + handshake compares registered-table digests fail-closed, so flipping registration is a + **both-nodes-together** change. +- The scadaproj umbrella index was updated and **pushed to `main` as `1145df6`**, per the + standing cross-repo rule. + +### Review chain + +9 implementers; **12 review verdicts** across the 4 reviewed code tasks (T1 spec + code + +delta re-review; T2 spec + code + delta re-review; T9 spec + code + delta re-review; T10 spec + +code + delta re-review); **4 fix rounds**; **2 CHANGES REQUESTED** (T2, T9). T3 was trivial +(doc-only ⇒ no review) and T4–T7 are live gates with no code to review. **Zero findings papered +over**; one coordinator ruling (T2's OCE flush). + +Three **evidence-hygiene** lessons from this round, all worth carrying forward: + +- ⚠️ **A race harness can be green-on-defect, and you must measure its hit rate to know.** + T1's plan-supplied `Barrier` harness "passed" at ~1-in-19 000 — it looked like a working test. + Only measuring the hit rate exposed that `Barrier`'s post-phase release hands one racer a head + start. **Round count is a function of the measured hit rate under full-suite parallelism**, not + a round number: 2 000 rounds was measured vacuous 1 time in 3 because misses burst-correlate. +- ⚠️ **Tear the rig down before a comparison sweep.** A running rig silently un-skips env-gated + tests; the r2 skip count is only comparable because the rig was down for both. +- ⚠️ **Name the version of a count you are quoting.** The "13 → 15" in this round's own close-out + brief was a mid-round reading; against the merge-base it is 7 → 15. Intermediate counts drift + the moment a review round lands. + +### Follow-up register (opened by this round) + +Only genuinely **new or still-open** items. Everything this round closed is annotated on the +round-2 register. **None of these blocks the branch.** + +**ACL** + +- ⚠️ **Browse transit stops below the realm root — a grant-holding client cannot browse down to + its own granted tag.** With the gate on, the key browses `ns=2;s=pymodbus` and + `ns=2;s=pymodbus/plc` fine but is refused `0x801F0000` on `ns=2;s=OtOpcUa` and on a recursive + browse from the Objects root. `PermissionTrieNode.BrowseTransitPrincipals` marks the ancestors + lying on the tag's **RawPath**; the synthetic realm root sits *above* the RawPath space and is + not in the trie at all. It **fails closed** (not a security hole) and blocked no leg, but it + would make an ACL-enabled deployment look broken to any browse-driven client — including the + Client.CLI's own default recursive browse. Discovered by the T7 live gate. **RECOMMEND filing a + Gitea issue before anyone enables `Security:Acl:Enabled` in production.** Likely fix: mark the + realm-root container browse-transit whenever any grant resolves beneath it. +- **`ns=0` standard nodes are ungated — the boot warning overstates.** With the gate on, both + the key and anonymous read `i=2256` and browse `i=2253` successfully: those are served by the + SDK's `CoreNodeManager` and never reach `DataPlaneAuthorizer`. Defensible (standard server + metadata, no process data), but "A principal with no grant can connect and see **nothing**" + is not literally true. Informational — a doc softening or a deliberate decision to gate ns=0. + +**S7** + +- **`_health` remains a plain field, versus Modbus's `Volatile.Read`/`Volatile.Write` idiom.** + Pre-existing, but **more of an outlier now** that the read pass consolidates to a single + end-of-pass write — the one place an alignment would be cheap and obvious. Follow-up + alignment, not a correctness claim. +- **Worst-state-wins landed for the S7 READ pass only.** Modbus (all paths) and the S7 **WRITE** + path keep last-writer-wins. **Deliberate scope** — both were named out of scope in this plan + because fixing them here doubles the blast radius of a watchdog-adjacent change. Recorded as a + known asymmetry so the next reader does not mistake it for an oversight. + +**LocalDb** + +- **`ZB.MOM.WW.LocalDb 0.3.1` is published to the Gitea feed** (per-table snapshot flags + a sync + fault-teardown fix). **This repo deliberately stays on `0.2.1`** — the 0.2.1 APIs cover + everything Task 10 needed, verified by probe. The per-**database** snapshot caveat is documented; + 0.3.x makes it per-table. ⚠️ A bump is a **both-nodes-together** change (bookkeeping schema + v2 → v3 is one-way), so it wants a planned window, not an incidental pin edit. + +**FetchAndCache** + +- **`central-2`'s serve path was never exercised live.** All four site nodes resolved + `central-1:4055` first and stopped there — the documented walk-in-order, stop-at-first-success + behaviour, so central-2 served nothing. Not a fault; simply an untested half of the pair. + +**Scripting / test calibration** + +- **The disposed race does not reproduce on this arm64 macOS box at all.** The raw-escape + assertion is the load-independent guarantee and holds regardless; the **exhaustion and + starvation bounds are calibrated loose** on a machine that cannot produce the phenomenon they + bound. They would benefit from a Linux-CI observation to tighten. + +**Carried from round 2, still open** + +- **TM/CT decode at a non-zero device number still needs a real S7 CPU.** The test exists and the + env var (`S7_TIMER_NUMBER_ADDRESSING=1`) is ready; python-snap7 resolves TM/CT as a BIT address + and cannot serve it. +- **The cross-*process* plant-vs-reap race on the shared SQL server is accepted.** The 24 h age + floor is the mitigation; a lock table on a shared dev server costs more than it buys. + +### Round-2 register items CLOSED by this round + +- **ACL single-shot CAS deferred publication** → closed by **`441b34a3`** (T1). +- **S7 per-item decode health last-writer-wins** → closed by **`4c161181` + `b35e423c` + + `1765e300`** (T2), for the **read path**; the write-path and Modbus asymmetry is recorded above. +- **docker-dev first-POST 404** → **mechanism NAMED** by **`3f04a4d4`** (T4), and the **no-body + POST binding VERIFIED** by the same commit.