dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity
Files
549d2d68b3bbb839e467779634ccd2d0fdcef392
lmxopcua/docs/plans/2026-08-17-perf-followups-r2.md
T

38 KiB
Raw Blame History

Perf Follow-ups Round 2 Implementation Plan

For Claude: REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development to implement this plan task-by-task. Opus implementers; classification-driven review chains.

Goal: Close the follow-up register opened by the 2026-08-17 perf follow-ups round (docs/plans/2026-08-17-perf-followups.md §"Follow-up register") — the ACL generation-rollover window, the S7 coverage gaps (cancellation branches, one-bad-decode, Timer/Counter live pin), the historian test hope-waits, the orphaned-test-DB reaper, the MTConnect ownership comment, and the docker-dev deploy readiness gate.

Architecture: No new subsystems. One production concurrency fix (generation stamp on the ACL cache's Shard), one production doc comment (MTConnect), one rig hardening (docker-dev/deploy.sh + Traefik access log); everything else is test/fixture work. Branch perf/followups-r2 off master (7bb888be).

Tech Stack: .NET 10, xUnit + Shouldly, Akka.NET TestKit, python-snap7 fixture on 10.100.0.35, SQL Server at 10.100.0.35,14330, Traefik v3.1 on the local OrbStack rig.

Out of scope (say so once, here): the cross-repo HistorianGateway batch WriteLiveValues RPC (sister repo); the VPN-gated historian E2E; the FetchAndCache / ACL-enabled / alarm-ratchet / load-scale live exercises (separate live-gate effort, on request).

SHARED-CHECKOUT RULES (every implementer, verbatim from round 1)

All implementers share ONE checkout on branch perf/followups-r2. Therefore:

  • Stage and commit with explicit pathspecs only: git add -- <files> then git commit -m "<msg>" -- <files>. NEVER bare git commit, git add ., git add -A.
  • NEVER run git checkout, git switch, git reset, git stash, git clean, or touch files outside your task's Files: contract.
  • On index.lock contention or transient build races (CS2012, MSB3030): wait 5–15 s and retry.
  • Falsification mutations (temporarily reverting a fix to prove a test bites) are made with Edit and reverted with Edit — never with git.
  • TDD red-first where the plan says so; when a test pins existing behaviour instead, label it honestly ("pin, not red-first") in your report.
  • Build/test only your own projects (dotnet build <csproj> / dotnet test <csproj> [--filter ...]), not the whole solution.

Task 1: Close the ACL generation-rollover window; capacity hint on eviction

Classification: high-risk Estimated implement time: ~5 min Parallelizable with: Task 2, Task 3, Task 4, Task 5, Task 6, Task 7

Files:

  • Modify: src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs
  • Test: tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs

The defect (pre-existing, recorded by round 1). InvalidateIfStale (lines 342–350) publishes the new _generation in the CAS at line 349 before the winner's ClearAll() (365–370) has purged the shards. A racer entering TryGet (167–183) in that window reads seen == generation, skips clearing, and serves an entry written under the previous generation from a shard the winner has not removed yet. Key revocation/expiry are unaffected (re-checked above the cache), so this is a stale grant walk only — but it is real, and the fix shape was already chosen in round 1's register: stamp the generation on Shard itself.

Spec:

  1. Shard (lines 378–391) gains a constructor taking long generation (stored in a public readonly long Generation field) and an optional initial-capacity hint for Entries (see step 6). All four construction sites in Set (lines 208, 221, 232, 236) pass the current query generation.
  2. TryGet serves a hit only when shard.Generation == generation. A generation-mismatched shard is a miss (do not evict from the read path — TryGet must stay allocation-free and non-mutating; the winner's ClearAll or the next Set reclaims it).
  3. Set: after acquiring the shard, if shard.Generation != generation, evict-swap exactly like the existing per-shard bound path (TryUpdate(sessionKey, new Shard(generation), shard)
    • Interlocked.Add(ref _globalCount, -Seal(shard)), then re-acquire) before inserting. _globalCount accounting must stay exact — the seal/booking protocol from d8849345 is load-bearing; do not weaken TryBookInsert/Seal.
  4. Deterministic pin of the window (this is the red-first test): add an internal test-only seam internal void AdvanceGenerationForTest(long generation) that CAS-moves _generation WITHOUT calling ClearAll — exactly the racer's view of the window. Test: Set under gen 1, AdvanceGenerationForTest(2), then TryGet(gen 2, …) for the same key must miss. Run it before the fix: it goes red (the stale entry is served). After the fix: green. Falsification after fixing: temporarily remove the shard.Generation check via Edit, confirm red, restore.
  5. Extend the existing stress test (lines 186–264) or add a sibling: interleave generation bumps with readers/writers and assert no hit ever returns a value stored under an older generation (encode the generation in the stored bool — e.g. store gen % 2 == 0 — and assert every hit equals the current generation's encoding). Also keep asserting TrackedCount/LiveShardCount exactness at quiescence.
  6. Capacity hint (second register item, same ctor): only the eviction-swap site (line 232 path) passes a capacity hint — the evicted shard's sealed count, clamped to a sane bound (e.g. Math.Clamp(captured, 0, MaxEntriesPerShard) or the existing per-key ceiling const) — because a shard evicted at the ceiling is the crawler signal. Plain GetOrAdd shards stay default-capacity. One test: after an eviction swap, inserts proceed correctly (a behaviour pin; capacity itself is not observable — say so in the test comment).
  7. Update the class-level comments where they describe the invalidation story; do not restate history, state the invariant ("a shard can only serve the generation it was built under").

Steps: red test (4) → fix (1–3) → green → falsification → stress (5) → capacity (6) → dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~SessionAclDecisionCache" → commit with pathspecs: git commit -m "fix(acl): stamp the generation on Shard so a rollover racer cannot serve a stale-generation verdict; eviction-swap capacity hint" -- <the two files>.

Task 2: S7 read-path coverage — cancellation branches + one bad decode in a chunk

Classification: standard Estimated implement time: ~5 min Parallelizable with: Task 1, Task 3, Task 4, Task 5, Task 6, Task 7

Files:

  • Test (only): tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs

Context. Three read paths carry the STAB-15a cancellation contract (_plcDead = true + rethrow): per-tag (S7Driver.cs:649-660, covered by S7DriverReconnectTests.Cancelled_read_marks_handle_dead_and_next_read_reopens), span block read (S7Driver.cs:920-926, uncovered), multi-var leftovers (S7Driver.cs:803-809, uncovered). The per-chunk fallback (:810-817) has one whole-request-fault test; the per-ITEM decode guard (:819-836, null value → InvalidDataException → that tag Bad) and the shape-mismatch arm (:796-801) have none. ImageS7PlcFactory (S7SpanReadParityTests.cs:453, ImageS7Plc at :480) exposes only ReadFault and MultiVarFault knobs today.

Spec — all tests are pins of existing behaviour unless marked red-first:

  1. Extend ImageS7PlcFactory/ImageS7Plc with three knobs, mirroring the existing fault knobs' style:
    • HangReadBytes (a TaskCompletionSource gate): ReadBytesAsync awaits Task.Delay(Timeout.Infinite, ct) so cancellation is the only exit — the S7DriverReconnectTests fake at :557-560 is the pattern.
    • HangMultiVar: same for ReadMultipleVarsAsync.
    • NullItemIndex (int?): ReadMultipleVarsAsync returns the normal answered list but with item N's Value left null — the decode guard's trigger at :824-826. Track a CreatedPlcCount on the factory if not already present (reopen observable).
  2. Cancelled span block read: plan spans on, hang ReadBytesAsync, cancel the read's CTS, assert OperationCanceledException propagates out of ReadAsync, and the NEXT read constructs a fresh plc (CreatedPlcCount increments — the _plcDead reopen contract), using the same assertion shape as the reconnect test at S7DriverReconnectTests.cs:338-355.
  3. Cancelled multi-var read: spans on with a leftover batch (reuse the existing multi-var-eligible tag set from :257/:281), hang ReadMultipleVarsAsync, cancel, same two assertions.
  4. One bad decode inside an otherwise-good chunk: NullItemIndex = 1 on a 3-item multi-var chunk → exactly that tag lands Bad (RecordReadFailure status), its chunk-mates Good with correct values, MultiVarCalls == 1, and no per-tag fallback reads occur for the good items (per-item failure is not chunk failure). Assert driver health reflects the partial failure the same way the existing fallback test does.
  5. Shape mismatch: a knob (or reuse NullItemIndex-style) making ReadMultipleVarsAsync return chunk.Count - 1 items → the whole chunk falls back per-tag (:796-801 → InvalidDataException → :810-817), all values still served Good via fallback.
  6. Falsification for (2) and (3): via Edit, temporarily change the production OCE arm to swallow (mimic the pre-#527-style bug) and confirm the reopen assertion goes red; restore. Note in the report which line was mutated.

Steps: knobs → tests (each run individually) → falsification → full file run (dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests --filter "FullyQualifiedName~S7SpanReadParityTests") → commit test(s7): pin the span/multi-var cancellation reopen contract, one-bad-decode-in-chunk, and multi-var shape mismatch -- tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs.

Task 3: Timer/Counter live pin — TM/CT area in the s7_1500 fixture

Classification: standard (live-gated) Estimated implement time: ~5 min authoring + fixture redeploy wall-time Parallelizable with: Task 1, Task 2, Task 4, Task 5, Task 6, Task 7 (disjoint files; the live leg runs against the shared fixture host, not the checkout)

Files:

  • Modify: tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/profiles/s7_1500.json
  • Modify: tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanReadTests.cs
  • Modify (if needed for seeding TM/CT): tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/server.py
  • Modify: tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/README.md

Context. The round-1 gate could not live-pin the Timer/Counter multi-var exclusion because the fixture profile declares only DB1 + MK (s7_1500.json), though server.py:34-41's AREA_MAP already supports TM/CT (snap7.type.SrvArea). The exclusion itself: S7ReadSpanPlanner.cs:94-103 (planner → leftovers) and S7Driver.cs:1129-1132 (IsBufferType → never multi-var). Timer/Counter entries are 2-byte device-number-indexed cells; a "device number N" seed lands at byte offset 2*N big-endian in the area buffer.

Spec:

  1. Add TM index 0 and CT index 0 areas (size 64 bytes each = 32 devices) to s7_1500.json with 2–3 distinct non-zero seeds each (the #536 rule: a zero-valued source proves nothing) at known device numbers, _desc'd. Keep every existing seed byte-identical — the round-1 tests depend on them.
  2. New [SkippableFact]-style test (follow the file's existing gating convention) in S7_1500SpanReadTests.cs: author a tag set mixing multi-var-eligible scalars with one Timer and one Counter tag; spans on. Assert:
    • the Timer/Counter values read back as the seeded non-zero constants (correct decode on a real wire), and
    • via CountingPlc, no multi-var request ever contains a Timer/Counter item and the TM/CT reads arrive as their own block/per-tag requests — the live pin of the exclusion.
  3. Accepted alternate outcome, stated in advance: S7.Net's TM/CT read is a distinct wire shape the python-snap7 server has never served. If it genuinely cannot (live evidence: error/exception captured, not a guess), do NOT force it: env-gate the test with a skip reason naming the fixture limitation, record the evidence in the Outcomes, and the exclusion stays unit-pinned. A red herring here must not burn the task.
  4. Fixture redeploy recipe (round-1 ops notes, both bit last time): rsync -av --rsync-path="sudo rsync" tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/ dohertj2@10.100.0.35:/opt/otopcua-s7/ then ssh dohertj2@10.100.0.35 'cd /opt/otopcua-s7 && docker compose build && docker compose up -d --force-recreate' — --build is mandatory (profile is baked into the image) and confirm seeded DB1 … seeds=<n> (and the new TM/CT lines) in docker logs.
  5. Run: dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests --filter "FullyQualifiedName~S7_1500SpanReadTests" with the fixture live. All pre-existing tests must stay green (the seed-compat rule in step 1).

Commit: test(s7): live-pin the Timer/Counter multi-var exclusion against a TM/CT-seeded s7_1500 fixture with pathspecs.

Task 4: Historian recorder tests — bounded watches instead of hope-waits

Classification: standard Estimated implement time: ~4 min Parallelizable with: Task 1, Task 2, Task 3, Task 5, Task 6, Task 7

Files:

  • Test (only): tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs

Context. Three fixed sleeps synchronize negative assertions ("X did not happen"): line 602 (FromMilliseconds(500) before asserting no truncation after a shutdown-cancelled write), line 452 (300 ms before asserting an abandoned tag stopped being rewritten), line 857 (300 ms before asserting sequential-drain concurrency). A negative can't be polled for directly; each needs a positive completion observable, then the assert. The Task-9 treatment (FasterLogHistorizationOutboxTests.cs:203-231) is the pattern for the poll shape (Stopwatch + 25 ms + cap, return last observation). InMemoryOutbox has AppendCallCount/AppendBatchCallCount/PeekBatchCallCount/DroppedCount but no RemoveCallCount.

Spec:

  1. Add RemoveCallCount to InMemoryOutbox (increment in RemoveAsync), same style as the existing counters.
  2. Line 602 test: replace the sleep with a bounded watch: after writer.Cancelled, poll (25 ms / 2 s cap) for the bug's positive signal — RemoveCallCount > 0 or CountAsync != 2 — failing fast red if either fires, green after the full window if neither does. Comment: "bounded negative watch — red is immediate when the fold truncates, green costs the window once". Keep the existing preconditions (594–595) untouched.
  3. Line 452 test: replace wall-clock with the drain's own clock: snapshot PeekBatchCallCount, poll until it has advanced by ≥ 3 (more passes have demonstrably run), then assert the Good.Tag write count is unchanged. Cap 5 s; on cap-out without 3 passes, fail with a message naming the observed pass count.
  4. Line 857 test: same pass-count clock (≥ 2 further passes) before the concurrency asserts; keep the existing AwaitAssertAsync precondition.
  5. Do NOT touch the deliberate mechanics delays (1476, 1640, 1656, 1673 — documented pool-thread forcing; leave them and their comments alone).
  6. Each reworked test: run 5× locally (--filter) to check stability; note the timings in the report.

Commit: test(historian): replace the recorder tests' fixed sleeps with bounded watches on drain-pass and truncation observables with pathspecs.

Task 5: Orphaned test-database reaper (AdminUI + Configuration fixtures)

Classification: standard Estimated implement time: ~4 min Parallelizable with: Task 1, Task 2, Task 3, Task 4, Task 6, Task 7

Files:

  • Modify: tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs
  • Modify: tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SchemaComplianceFixture.cs
  • Test: a new [SqlServerFact] (AdminUI side) proving the reaper drops an old-named DB

Context. A killed test process leaks its uniquely-named database forever: OtOpcUaAdminUi_Test_<yyyyMMddHHmmss>_<GuidN> (AdminUiSqlServerFixture.cs:77) and OtOpcUaConfig_Test_… (SchemaComplianceFixture.cs:30). The fixture comment at 127–128 even says the timestamp exists "so a leaked one is identifiable" — the reaper was never written. Natural insertion point: fixture ctor, after the probe early-return (:75), before the name assignment (:77). The drop SQL to reuse is :118-121; TryDropDatabase must be extracted to a static taking a name.

Spec — safety first, this runs DROP DATABASE against a shared server:

  1. Extract the drop body into a static TryDropDatabase(string databaseName) (best-effort, DbException-swallowing, exactly the current semantics); the instance method delegates.
  2. Add a static reaper, run once per process behind a Lazy in AdminUiSqlServer (mirroring the probe): SELECT name FROM sys.databases WHERE name LIKE 'OtOpcUaAdminUi\_Test\_%' ESCAPE '\', parse the embedded yyyyMMddHHmmss (UTC), and drop ONLY rows older than 24 hours; a name whose timestamp does not parse is left alone. Never throw — a reaper failure must not fail a green run. Log nothing louder than Console.WriteLine (test infra convention in this file).
  3. Same shape in SchemaComplianceFixture for the OtOpcUaConfig_Test_ prefix, one-shot per process. (It has no probe/skip machinery — keep the reaper catch-all so an unreachable server behaves exactly as today.)
  4. Test (red-first is impossible for a time-gated reaper without waiting a day, so pin it deterministically): a [SqlServerFact] that creates an empty DB named with a >24 h-old timestamp (e.g. OtOpcUaAdminUi_Test_20250101000000_<guid>), invokes the reaper's core (make the core internal-testable, taking the cutoff as a parameter), and asserts the DB is gone — and a second DB named with a fresh timestamp survives the same call. Clean up the survivor in the test.
  5. The mid-run-blip semantics (one-shot probe → mid-run SqlException = failures, not skips) stay as-is — they are already documented at :34-38; verify the comment survives your edit, extend it with one line noting the deliberate non-choice if it doesn't already say so.

Commit: test(sql): reap >24h-old orphaned test databases at fixture startup (AdminUI + Configuration prefixes) with pathspecs.

Task 6: MTConnect AsStream buffer-ownership comment

Classification: trivial Estimated implement time: ~1 min Parallelizable with: all

Files:

  • Modify: src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs (the AsStream doc comment at lines 241–249)

Spec: extend the existing <summary> with the ownership facts (verified this round): the array-backed branch aliases the caller's array without copying; the buffer is not pooled or rented anywhere on this path; parsing is fully synchronous, so the buffer must stay valid and unmutated only for the duration of the call; the returned MTConnectStreamsResult holds no reference into the buffer (XmlReader materialises its own strings). Comment-only — no code change, no test.

Commit: docs(mtconnect): state AsStream's buffer aliasing and lifetime contract with pathspec.

Task 7: docker-dev readiness-gated deploy helper + Traefik access log

Classification: small Estimated implement time: ~4 min Parallelizable with: all

Files:

  • Create: docker-dev/deploy.sh
  • Modify: docker-dev/docker-compose.yml (Traefik command block, lines ~912-916)
  • Modify: docker-dev/README.md (the bare POST snippet at lines ~88-91)

Context (root-cause facts, record them in the README): the transient first-POST 404 can NOT come from the AdminUI (all routes map synchronously before RunAsync (Program.cs:740 vs :803); the endpoint's own failures are 503/401/422, never 404) and can NOT be Traefik's backend-down answer (that is 503 no available server). A 404 from :9200 means no Traefik router matched at that instant — mechanism unconfirmed; the access log below is how the next occurrence gets diagnosed.

Spec:

  1. deploy.sh (bash, set -euo pipefail): polls http://localhost:9200/health/ready until HTTP 200 (2 s interval, 120 s cap — a 200 through Traefik proves both the router table and a ready backend), then POSTs the artifact exactly as the README snippet does (X-Api-Key: docker-dev-deploy-key, JSON body from $1 or stdin), prints the response, and exits non-zero on any non-2xx. No retry on the POST itself — the gate is the fix, and a post-gate 404 should fail loudly (it would be new evidence).
  2. Add --accesslog=true (and --accesslog.fields.defaultmode=keep) to the Traefik command list so a recurrence shows which router (or none) matched.
  3. README: replace the bare POST snippet with ./deploy.sh, and add a short "first-POST 404" note carrying the root-cause facts above.
  4. Verification is docker-dev-rig-dependent; bash -n deploy.sh + a docker compose config parse check are the in-checkout gate. The rig run happens in Task 8's close-out if the rig is brought up; do not block this task on it.

Commit: chore(docker-dev): readiness-gated deploy.sh + Traefik access log for the first-POST 404 with pathspecs.

Task 8: Close-out — sweep + Outcomes + register annotations

Classification: standard Estimated implement time: ~10 min (mostly sweep wall-time) Parallelizable with: none (blockedBy all)

Files:

  • Modify: docs/plans/2026-08-17-perf-followups-r2.md (append ## Outcomes)
  • Modify: docs/plans/2026-08-17-perf-followups.md (append-only "closed by " annotations on the round-1 register items this round closes; the two live-exercise items that stay open stay open)
  • Modify: docs/plans/2026-08-17-perf-followups-r2.md.tasks.json (ledger)

Steps:

  1. dotnet build ZB.MOM.WW.OtOpcUa.slnx → 0 errors (pre-existing warnings tolerated).
  2. Full per-project unit sweep of every *.Tests project (--no-build after the solution build), same protocol as round 1. DB-backed suites must RUN (not skip) against 10.100.0.35,14330. Expected: 0 failed; new-test count above round 1's 10,358 baseline.
  3. S7 integration suite against the live fixture (Task 3's evidence re-confirmed at the final tree).
  4. Append ## Outcomes: per-task table (commit, outcome, deviation), the TM/CT live evidence (or the documented fixture limitation), the reaper's first live reap count, any new follow-up register (only genuinely new items).
  5. Annotate the round-1 register (append-only, never rewrite): ACL rollover + capacity hint, S7 cancellation + one-bad-decode + TM/CT, historian hope-wait, AdminUI reaper (blip semantics = documented wontfix), MTConnect comment, docker-dev readiness gate — each with its closing SHA.
  6. Commit docs with pathspecs; update the ledger.

Execution notes for the controller

  • Wave 1: Tasks 1–7 dispatched concurrently (all Files: contracts disjoint). Task 8 after all verdicts land.
  • Review chains per classification: T1 high-risk (spec serial → code); T2/T3/T4/T5/T8 standard (spec ∥ code); T7 small (code only); T6 trivial (none).
  • Implementers: Opus (user preference), including the trivial task for uniformity.
  • Reviewers judge via git show <sha>; file-disjoint scheduling makes on-disk fallback acceptable as in round 1.

Outcomes

Written at close-out, 2026-08-17, on branch perf/followups-r2. All 8 tasks complete. This round is a follow-up round: every item it closes was opened by the round-1 register in 2026-08-17-perf-followups.md, and that register is annotated in place with the closing SHAs.

Sweep record

dotnet build ZB.MOM.WW.OtOpcUa.slnx → 0 errors, 904 warnings — the same two pre-existing families round 1 recorded (xUnit1051 cancellation-token advice and the repo's own OTOPCUA0001 CapabilityInvoker analyzer firing on test call sites), up 8 from round 1's 896 because this round's new test call sites are more of the same. No new warning kind.

Full unit sweep — every *.Tests project in the solution, run per-project with --no-build against the tree at 580b77ce. *.IntegrationTests are build-only by convention; the S7 suite ran separately as Task 3's live gate (below).

43 / 43 test projects green (exit code 0 on every one).

Round 1 This round Δ
Passed 10 358 10 370 +12
Failed 0 0 —
Skipped 19 19 —
Total 10 377 10 389 +12

No test failed, so no isolation re-run was needed, and nothing flaked. The +12 reconciles exactly against the three suites this round added tests to, with nothing unaccounted for:

Project Round 1 This round Δ Source
OpcUaServer.Tests 476 481 +5 SessionAclDecisionCacheTests 12 → 17 (T1)
Driver.S7.Tests 397 401 +4 S7SpanReadParityTests 9 → 13 (T2)
AdminUI.Tests 1 537 1 540 +3 SqlTestDatabaseReaperTests (T5)

⚠️ Note the S7 delta is +4, not the +5 the plan estimated — the plan's task text counted five contracts pinned, which landed as four test methods (one method carries two of them). T4 added no tests (it reworked 3 of the 5 existing recorder tests in place), and the S7 integration suite's +2 do not appear here because it is build-only in the sweep.

All 19 skips are the pre-existing, env-gated ones round 1 recorded, 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.

The DB-backed suites ran against a reachable 10.100.0.35,14330 — executed, not skipped: AdminUI.Tests 1540 passed / 0 skipped and Configuration.Tests 190 / 0 skipped. That zero is the load-bearing number for T5: a reaper suite that skipped itself would have proved nothing about the DDL it runs as sa.

Two verifications outside the sweep proper, because two comment-only fixes (below) landed after the sweep's build: the solution rebuilt 0 errors, and OpcUaServer.Tests re-ran 481 / 0 / 1, unchanged.

Per-task outcomes

# Task Commit(s) Outcome + deviation
1 ACL generation stamp on Shard + eviction capacity hint 1979cf41, 98d6a190 Done, and the review found a second, pre-existing defect that was closed in-round. The generation is now stamped on Shard (ctor + readonly Generation); TryGet is generation-gated and non-mutating; Set displaces a mismatched shard and drops the verdict if it is still mismatched after the displacement. The eviction capacity hint is gated on the generation matching — a previous generation's fill level is not evidence about this one. 98d6a190 added a monotonic <= guard to InvalidateIfStale after the reviewer spotted that a stale-generation straggler could roll the stamp backward and thrash ClearAll. Falsifications: removing the stamp check went deterministically red on the pin and produced 1 357 cross-generation hits in the stress test; reverting the <= guard to == rolled the stamp 2 → 1. 17/17.
2 S7 cancellation branches + one-bad-decode + shape mismatch 4f860733, 120e885b Done. The span and multi-var cancellation reopen contracts are pinned (hang knobs + entered-TCS + CreatedPlcCount), plus one-bad-decode-in-chunk (only that tag goes Bad, no fallback) and the short-answer shape mismatch (whole-chunk fallback). 120e885b strengthened the pins to assert the stale handle is disposed, not merely replaced. Falsification: each OperationCanceledException arm was weakened in turn and exactly the matching test went red — no arm was covered by a neighbour's test. 13/13.
3 Timer/Counter live pin — TM/CT area in the s7_1500 fixture fd202d81, f797df51 LIVE PIN PASSED vs 10.100.0.35:1102. Evidence + the fixture quirk below.
4 Historian recorder tests — bounded watches 45d67092, 68461fbf Done; three fixed sleeps → bounded watches. Detail below.
5 Orphaned test-database reaper 8946644b, 580b77ce Done — and the code review found a real Critical, fixed pre-merge. Detail below.
6 MTConnect AsStream ownership comment 1edfc263 Done as specified; comment-only, no test. Trivial ⇒ no review.
7 docker-dev readiness-gated deploy.sh + Traefik access log a61b3c98, 9f09a62a Done as a gate plus instrumentation, not a diagnosis — say so plainly. Detail below.
8 Close-out — sweep + Outcomes + register annotations d54b4fab, this commit Done. d54b4fab carries the two comment-only nits the T1 re-review left for close-out; this commit carries the Outcomes, the sweep record, the round-1 register annotations, and the ledger.

Live pin — S7 Timer/Counter multi-var exclusion (Task 3)

2026-08-17, against the s7_1500 fixture at 10.100.0.35:1102. PASSED — 4 passed / 1 env-gated skip.

TM and CT areas were added to the fixture profile: TM0 (T0 = 0x2456 = 456.0 s, T5 = 0x1123 = 12.3 s, T9) and CT0 (C0 = 4711, C7 = 31337, C11).

  • The claim under test, asserted on real wire shapes: a batch mixing Timer/Counter tags with ordinary ones produced 1 block read + 1 multi-var request carrying 3 scalar items + one 2-byte TM read + one 2-byte CT read, and MultiVarItemAreas never contains Timer or Counter. The round-1 exclusion — added because S7.Net boxes TM/CT as int where the single-read path yields ushort, and box parity is load-bearing downstream — now rests on the wire, not on unit tests alone.
  • ⚠️ Fixture quirk, discovered here and documented with a recaptured live probe transcript: python-snap7's server resolves a TM/CT request address as a BIT address (it serves byte addr >> 3) where S7.Net and a real CPU send the device number. Device 0 is the fixed point of both conventions, which is why value decode is live-pinned at T0/C0.
  • The device-number leg (T5/C7) is therefore env-gated on S7_TIMER_NUMBER_ADDRESSING=1 — it is the single skip above, and it is a ready-made real-CPU gate rather than a silent omission. Not closed by this round: TM/CT decode at a non-zero device number still needs a real CPU.

Task 4 — what replaced each sleep, and the one place a pass clock does not exist

Three fixed sleeps became bounded watches on real observables, not shorter sleeps:

  • Shutdown test: a 25 ms/1 s negative watch on the new RemoveCallCount and the queue depth. The 1 s cap is not a guess — it is anchored to the measured ~150 ms at which the falsification tripped, so the cap is roughly 6× the observed signal rather than an arbitrary round number. 68461fbf also reduced it to a single counter read per poll (two reads per poll can straddle an increment and compare inconsistent values).
  • Poison-tag test: a positive watch on PeekBatchCallCount ≥ 3 — a genuine pass clock.
  • ⚠️ Sequential test: a 25 ms/500 ms negative watch, and this is a disclosed deviation. A pass clock is structurally unavailable there: the drain is single-flight on _draining and the timer re-arms only in OnDrainResult, so there is no counter that advances while the condition under test is false. Verified against the production code and accepted rather than worked around with a fake observable.

Falsification: tripped on RemoveCallCount in ~150 ms. 5× stability green.

Task 5 — the reaper's Critical, and why the live evidence is shaped the way it is

The startup reaper drops test databases orphaned > 24 h (AdminUI + Configuration prefixes, per fixture, one-shot Lazy, probe-gated on the AdminUI side).

  • ⚠️ The code review found a real Critical. The name-shape guard validated only the prefix and the timestamp, so any ..._<14 digits>_<anything> database name on the server would flow into string-interpolated ALTER/DROP DDL executed as sa. Fixed defense in depth: a Guid.TryParseExact("N") suffix validation and parameterized QUOTENAME/sp_executesql. ⚠️ The reviewer's proposed EXEC('…' + QUOTENAME(@name)) snippet was itself invalid T-SQL (verified live — Msg 102); the accepted fix is not the one that was suggested.
  • Also landed: a 3 s reaper ConnectTimeout (a reaper must never be the thing that hangs a test run), DatabaseName minted from the prefix constant rather than re-spelled, the catch widened to Exception, and the reaper tests joined to the AdminUI SQL collection — closing a same-process parallel-collection race that earlier 2-test runs had masked.
  • Live evidence: planted orphans were reaped by exactly the right fixture; non-GUID and unparseable names were spared; the shared server was restored afterwards. 3/3 reaper, 1540/1540 AdminUI ×3, 190/190 Configuration.

Task 7 — what the docker-dev change does and does not establish

deploy.sh polls /health/ready (2 s interval, 120 s cap) and then POSTs once, un-retried (both curls guarded under set -e, --max-time 10/60), and Traefik now runs --accesslog.

⚠️ The 404's mechanism is still unconfirmed. What is established is the elimination: the AdminUI cannot produce it (routes map synchronously before RunAsync; the endpoint's own failure modes are 503/401/422), and Traefik's backend-down answer is 503 no available server, not 404. So a 404 means no router matched at that instant — and the access log is how the next occurrence gets named. The gate makes scripted deploys reliable; it does not explain the bug.

Review chain

7 implementers; 11 review verdicts (T1 spec + code + delta re-review, T2 spec + code, T3 spec + code, T4 spec + code, T5 spec + code + re-review, T7 code + re-review); 4 fix rounds (T1, T3, T5, T7, plus minors on T2/T4); 1 Critical found and fixed pre-merge (T5's DDL injection); zero findings papered over.

Two falsification-hygiene lessons from this round, both worth carrying forward:

  • A falsification must be run against a freshly built binary. A when (false) mutation combined with a --no-build run executed the stale binary and reported a vacuous green — the mutation "passed" without ever having been compiled.
  • A green falsification arm proves nothing until you have seen its red. T1's Set-displacement behaviour was unpinned until a test written specifically for it went red; the existing tests all passed with the displacement removed.

Follow-up register (opened by this round)

Only genuinely new or still-open items. Everything else this round touched is annotated as closed on the round-1 register. None blocks the branch.

ACL

  • The single-shot CAS can defer stamp publication by one call. When generation N wins the race for a shared baseline against generation N+1, publication of the newer stamp is deferred 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

  • A cross-process plant-vs-reap race is inherent to a shared, unlocked server and is 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.
Reference in New Issue View Git Blame Copy Permalink