docs(plan): r3 close-out — outcomes, sweep record, register annotations
This commit is contained in:
Joseph Doherty
@@ -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.
|
||||
|
||||
@@ -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 <sha> (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.
|
||||
|
||||
Reference in New Issue
Block a user