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

24 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.
Reference in New Issue View Git Blame Copy Permalink