From f06cbe16d93d70ca1acef0df3bf8035393205fc8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 03:41:40 -0400 Subject: [PATCH] =?UTF-8?q?docs(plan):=20perf=20follow-ups=20=E2=80=94=201?= =?UTF-8?q?5-task=20plan=20off=20the=20remainder's=20follow-up=20register?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/2026-08-17-perf-followups.md | 615 ++++++++++++++++++ .../2026-08-17-perf-followups.md.tasks.json | 21 + 2 files changed, 636 insertions(+) create mode 100644 docs/plans/2026-08-17-perf-followups.md create mode 100644 docs/plans/2026-08-17-perf-followups.md.tasks.json diff --git a/docs/plans/2026-08-17-perf-followups.md b/docs/plans/2026-08-17-perf-followups.md new file mode 100644 index 00000000..c22fd8d5 --- /dev/null +++ b/docs/plans/2026-08-17-perf-followups.md @@ -0,0 +1,615 @@ +# Perf Follow-ups Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development +> to implement this plan task-by-task (per the standing preference: Opus implementers, +> classification-driven review chains). + +**Goal:** Close the actionable items in the perf-roadmap-remainder follow-up register +(`docs/plans/2026-08-15-perf-roadmap-remainder.md` §"Follow-up register") and run the two +release-condition live gates, on branch `perf/followups` off master merge `e40a0385`. + +**Architecture:** No new subsystems. Each task is a surgical close of a recorded follow-up: +a metrics ride-along on the existing shared `OtOpcUaTelemetry.Meter`, four defect fixes in +already-reviewed code (drain cancellation, ACL eviction race, revalidate memo key, +`TrackBackgroundTask` ordering), two SQL-Server-backed test gaps, one parser streaming +rewrite, an S7 multi-var read extension over the existing `IS7Plc` seam, and two live gates +(s7_1500 fixture + local docker-dev rig). + +**Tech Stack:** .NET 10 / C#, xUnit + Shouldly, System.Diagnostics.Metrics, EF Core +(SqlServer + InMemory), S7netplus 0.20.0, FasterLog, docker-dev rig (OrbStack). + +**Out of scope (recorded, deliberately not here):** +- **HistorianGateway multi-tag batch `WriteLiveValues` RPC** — cross-repo + (`~/Desktop/HistorianGateway` proto + client-package release + pin bump here). Stays in + `docs/Historian.md` as the recorded ceiling on `DrainConcurrency`. +- **S7 `SByte`** — a plan error, already corrected; not a gap. +- `UpsertNodeDeploymentState` / `TryRecoverFromStale` synchronous writes — commented in + `DriverHostActor.cs:2930-2932` / `:2979-2982` as deliberate; not on the hot path. +- Host-side mux interest gating — recorded as not-worth-it after the batch envelope. +- Historian end-to-end live gate against the real AVEVA Historian — needs the + `wonder-sql-vd03` VPN (user-driven); recipe stays + `docs/plans/2026-08-01-continuous-historization-live-gate.md`. +- Pushing master — user decision, unchanged. + +**Execution rules (unchanged from the remainder plan):** shared checkout — explicit-pathspec +`git add`/`git commit -- ` only (never a bare commit); never checkout/switch/reset/ +stash/clean; retry on `index.lock` and CS2012/MSB3030 build races; reviewers judge commits +via `git show`, never the working tree; falsification mutations reverted via Edit; TDD +red-first with honest "pin, not red-first" reporting where the behaviour predates the test. + +--- + +### Task 1: Per-driver poll-overrun meter + PollGroupEngine doc notes + +**Classification:** standard +**Estimated implement time:** ~5 min +**Parallelizable with:** Tasks 2, 3, 4, 5, 6, 7, 8, 10, 11 + +**Files:** +- Modify: `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs` (new counter) +- Modify: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs` (doc notes only) +- Modify (one-line each, at the `new PollGroupEngine(` call sites): `ModbusDriver.cs:157`, + `FocasDriver.cs:117`, `TwinCATDriver.cs:76`, `AbLegacyDriver.cs:74`, `S7Driver.cs:65`, + `SqlDriver.cs:226`, `AbCipDriver.cs:157` +- Test: `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/` (PollGroupEngine tests file) + + one per-driver wiring pin is NOT required (see below) + +**Spec.** The seven poll drivers' pacing change currently ships with no production consumer +of `PollGroupEngine.OverrunCount` (the accepted-not-fixed item). Close it the way the +engine's own doc note (`PollGroupEngine.cs:47-50`) prescribes: the driver wires its own +meter to the existing `onOverrun` callback (ctor param, line 105). + +1. Add to `OtOpcUaTelemetry` (following the existing `CreateCounter` field pattern, e.g. + `otopcua.driver.lifecycle` at line 42): + `public static readonly Counter PollOverruns = Meter.CreateCounter("otopcua.driver.poll.overrun", unit: "{overrun}", description: "Poll cycles that ran longer than the poll interval (slow success and slow failure both count — see PollGroupEngine.NoteOverrun).");` +2. In each of the seven drivers, pass + `onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, new KeyValuePair("driver.type", ""))` + where `` is the driver's `DriverTypeNames` constant (use the constant, not a + literal — the `TwinCat`/`Focas` drift was a real bug once). If the constructor scope has + a cheap device/instance identifier already in hand, add a second `device` tag; do NOT + plumb new state to get one. + Note: Commons is referenced from driver projects already via the messages namespace — if + any of the seven driver csproj lacks the `Commons` reference, add the ProjectReference; + report it rather than inventing a per-driver Meter (Galaxy's own-meter pattern is the + outlier, not the template). +3. Doc notes in `PollGroupEngine.cs` (comment-only): + - Update lines 47-50: the follow-up is now delivered driver-side via + `OtOpcUaTelemetry.PollOverruns`; the engine still deliberately owns no Meter. + - `NoteOverrun` (lines 315-322): add the caveat that it conflates a slow success with a + slow failure — both increment; disambiguation would need the loop's outcome, and no + consumer needs it yet. + - `backoffCap` ctor param (line 104): doc its units (a `TimeSpan` cap on the exponential + failure backoff) and its interaction with `minInterval` (backoff replaces, not adds to, + the interval on the failure path — state whichever the loop at 261-263 actually does; + read it, don't guess). + +**Tests.** One unit test on the engine pinning that an overrun invokes the `onOverrun` +callback exactly as many times as `OverrunCount` increments (may already exist from Task 11 +of the remainder — if so, extend or cite it; do not duplicate). The seven driver wirings are +one-line closures over a static counter — no per-driver test; a wiring test would just +restate the line. + +**Step order:** test (or cite existing) → counter → 7 wirings → doc notes → build the +solution → commit. + +**Commit:** `feat(drivers): per-driver poll-overrun meter on the shared OtOpcUa Meter; NoteOverrun + backoffCap doc notes` + +--- + +### Task 2: S7 span-option validation guard + +**Classification:** small +**Estimated implement time:** ~3 min +**Parallelizable with:** Tasks 1, 3, 4, 5, 6, 7, 8, 10, 11 + +**Files:** +- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs` (`ParseOptions`, line 46 area) +- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs` (doc remark only, if needed) +- Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/` (the factory/options parse test file) + +**Spec.** `SpanReadMaxSpanBytes` (`S7DriverOptions.cs:128`, default 220) has no validation: +a nonsensical value is accepted at config time and only fails (or silently misbehaves) at +read time. There is no `IValidateOptions` machinery for S7 (confirmed) — `ParseOptions` in +`S7DriverFactoryExtensions.cs` is the single choke point, so validate there: + +- `SpanReadMaxSpanBytes < 1` → throw `ArgumentException`/config-error consistent with how + `ParseOptions` reports other bad config (read the method; match its existing error style). +- `SpanReadMaxGapBytes < -1` → same (−1 = disabled is legal, per `S7Driver.cs:571`'s + `>= 0` enable check; 0 is legal "no gap tolerated"). +- Values **above** the negotiated PDU size cannot be checked here — the negotiated size is + opaque inside S7.Net (confirmed: no API exposes it) and S7.Net transparently chunks + oversized reads. Do NOT invent a fake upper bound; instead add one doc line to the + `SpanReadMaxSpanBytes` remark in `S7DriverOptions.cs` stating that over-PDU values are + legal but degrade to multi-request chunking inside S7.Net. + +**Tests.** Red-first: `SpanReadMaxSpanBytes = 0` rejected; `SpanReadMaxGapBytes = -2` +rejected; `-1`/`0`/defaults accepted. + +**Commit:** `fix(s7): validate span-read options at parse time; document the over-PDU degradation` + +--- + +### Task 3: Historian drain — shutdown cancellation is not a per-tag failure + +**Classification:** high-risk (concurrency, #538 quarantine semantics) +**Estimated implement time:** ~5 min +**Parallelizable with:** Tasks 1, 2, 4, 5, 6, 7, 8, 10, 11 + +**Files:** +- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs:703-712` +- Test: `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs` + +**Spec.** In `DrainOnceAsync`, the per-tag parallel lambda wraps +`_writer.WriteLiveValuesAsync(g.Key, g.Values, ct)` in a **bare `catch`** (`:707-712`) that +converts a shutdown `OperationCanceledException` into `ok = false`. When sibling tags in the +same pass already succeeded, the #538 logic (`:775-784`) then counts the cancelled tag as a +consecutive per-tag failure — a shutdown can push a healthy tag toward abandonment. The +outer `catch (OperationCanceledException) when (ct.IsCancellationRequested)` (`:812-816`) +already treats whole-pass cancellation as non-fault but is never reached because the inner +catch eats it first. + +Fix: in the per-tag lambda, rethrow genuine token cancellation ahead of the bare catch: + +```csharp +catch (OperationCanceledException) when (ct.IsCancellationRequested) +{ + throw; +} +catch +{ + ok = false; +} +``` + +The rethrow propagates through the awaited per-tag task to the pass's `await` and lands in +the existing outer catch → `DrainResult(true, 0, 0)`, no ack, no failure counts. Verify the +`SemaphoreSlim window` release still runs on this path (it is in a `finally` — confirm, and +keep it that way). Do NOT touch the #538 discriminator ("did another tag write this pass"), +the all-or-nothing ack, or the falsification control's `succeeded == 0` keying. + +**Tests.** Red-first: a writer fake where tag A completes successfully, then the token is +cancelled and tag B's write throws `OperationCanceledException(ct)`. Assert: the pass does +not ack (outbox not truncated), and — the load-bearing assertion — tag B accumulates **no** +failure count (observe via the same seam the existing #538 tests +`A_permanently_failing_tag_is_abandoned...` (`:408`) and +`A_backend_that_fails_every_tag_abandons_nothing` (`:456`) use: run `MaxWriteAttempts` +shutdown-cancelled passes interleaved with sibling successes, then show the tag is NOT +abandoned). Also keep a control: a non-cancellation throw from B with A succeeded still +counts (pins that the fix is scoped to genuine `ct` cancellation — an OCE thrown while +`ct.IsCancellationRequested == false` must still count as a failure, which the `when` filter +gives for free; test that too). + +**#491 hazard note:** this file is the `ConfigureAwait(false)`-destroys-`Self` site. The fix +touches only the catch clauses inside an existing async lambda — do not add `ConfigureAwait` +changes, do not touch `Self` usage. + +**Commit:** `fix(historian): a shutdown cancellation mid-drain is a pass abort, not a per-tag failure` + +--- + +### Task 4: Direct-apply artifact read — explicit command timeout + +**Classification:** standard +**Estimated implement time:** ~4 min +**Parallelizable with:** Tasks 1, 2, 3, 5, 6, 7, 8, 10, 11 + +**Files:** +- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs:2082-2107` (`LoadArtifactBlob`) +- Test: the DriverHostActor direct-apply test file (locate the Task 26 tests around `BeginDirectApply`/`ArtifactForApply` in `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/`) + +**Spec.** The Task 26 Important: `LoadArtifactBlob` (EF read of `Deployment.ArtifactBlob`, +runs in `Task.Run` off the mailbox) carries no timeout or cancellation. The code relies on +an implicit bound and does not assert it. Confirmed by exploration: nothing sets a command +timeout anywhere on the ConfigDb path (`ServiceCollectionExtensions.cs:30` has no +`CommandTimeout`, no connection string in the repo carries `Command Timeout=`), so the only +bound is SqlClient's default 30 s command timeout. + +Fix — make the bound explicit and local (do NOT change the global pooled-factory options; +that would silently re-time every ConfigDb consumer, including migrations and long admin +queries): + +1. In `LoadArtifactBlob`, after `CreateDbContext()`: + `db.Database.SetCommandTimeout(TimeSpan.FromSeconds(30));` + with a comment: this read runs off the mailbox on a pool thread with no CancellationToken + (the actor cannot be wedged, but a hung SQL read would pin a pool thread); the explicit + timeout replaces reliance on SqlClient's default so a future connection-string or + provider-default change cannot unbound it. A timeout surfaces as the existing + catch-to-null path (`:2093-2106`) → the apply fails, revision does not advance — the + #485-family "no answer" semantics, already correct. +2. Do not thread a `CancellationToken` — `FirstOrDefault` would need to become async and the + only caller-side token is actor lifetime; the timeout gives the same bound without + touching the actor seam. State this in the comment. + +**Tests.** The behaviour delta is not black-box observable without a hung SQL server, so: +pin with a source-level test only if the repo already has the source-scan pin pattern for +this file; otherwise a unit test that `LoadArtifactBlob`'s failure path (throwing +DbContext) still returns null and fails the apply (may already exist — cite it), plus the +comment. Say honestly in the report which it was. + +**Commit:** `fix(runtime): explicit 30s command timeout on the Direct-apply artifact read` + +--- + +### Task 5: SessionAclDecisionCache — eviction race on `_globalCount` + stress test + +**Classification:** high-risk (lock-free concurrency) +**Estimated implement time:** ~5 min +**Parallelizable with:** Tasks 1, 2, 3, 4, 6, 7, 8, 10, 11 + +**Files:** +- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs:178-182` +- Test: `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs` + +**Spec.** The per-shard eviction branch (`Set()`, lines 178-182) runs +`shard.Entries.Clear()` then `Interlocked.Add(ref _globalCount, -Interlocked.Exchange(ref shard.ApproxCount, 0))` +as two non-atomic steps. A concurrent `Set` on the same shard can insert into the +just-cleared dictionary and increment both counts between them; the evictor's `Exchange` +then subtracts a count that includes the racer's fresh insert → `_globalCount` under-counts +(negative-only drift; trims early, never late — correctness of decisions is unaffected, +which is why this was deferred). + +Fix by making the eviction atomic with respect to concurrent inserts: **replace the shard +object** rather than clearing the shared dictionary — the same pattern `ClearAll()` already +uses (it drops shard objects wholesale). Read the shard-storage shape first (how shards are +held per session key) and swap a fresh `Shard` in with a CAS/indexer replace, then subtract +the *displaced* shard's `ApproxCount` from `_globalCount`. A racer that inserted into the +displaced shard loses its entry (acceptable — it re-caches on next miss) but its global +increment is subtracted with the shard it landed in, so the count cannot over-subtract. +Preserve: the outer-bound gate at `:171`, generation-rollover `ClearAll()` at `:238`/`:250`, +and the "increment only on genuine `TryAdd`" invariant at `:186-189`. + +**Tests.** Red-first is impractical for a race; do it as: (1) a deterministic interleaving +test if the seams allow (inject at the eviction boundary), else honestly a stress test — +N writer threads hammering one session's shard past `MaxEntriesPerSession` while others +insert, barrier-started, several rounds; assert at quiescence `_globalCount` equals the +actual live entry total (expose via an internal count-audit helper or reflection over +shards) and is never negative. Run the stress test with the fix reverted (Edit-revert, then +restore) and report whether it goes red — if the race won't reproduce under test load, say +so honestly; the deterministic invariant audit is then the pin. + +**Commit:** `fix(acl): shard eviction swaps the shard object so _globalCount cannot over-subtract; concurrency stress test` + +--- + +### Task 6: Revalidate memo — reject empty node identifiers + +**Classification:** small +**Estimated implement time:** ~3 min +**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 7, 8, 10, 11 + +**Files:** +- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs:3319-3321` (`IsSubscriptionStillPermitted`) +- Test: `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs` + +**Spec.** The pass-local revalidation memo key (`:3327`) tolerates an empty string +identifier: the guard at `:3319-3321` rejects only a *non-string* identifier, so all items +on empty-`s=` nodes for one (session, realm) collapse onto one memoized verdict. +`DataPlaneAuthorizer.Evaluate` already rejects exactly this at +`DataPlaneAuthorizer.cs:253` (`identifier.Length == 0`). Mirror it: extend the guard with +`|| identifier.Length == 0` so empty identifiers fall through to the un-memoized +`IsAllowedCached` call at `:3323-3324` (fail-safe: un-memoized means each item gets its own +authorizer verdict, and the authorizer's own guard then handles the empty id). + +**Tests.** Red-first in `NodeManagerStage6EnforcementTests` (it already drives +`RevalidateMonitoredItems` directly at `:484`/`:515`): two monitored items on nodes with +empty string identifiers, different underlying permit outcomes by whatever seam the tests +use — assert they do NOT share one memoized verdict after the fix. If distinct outcomes for +two empty-id nodes aren't constructible through the fake, pin instead that empty-id items +bypass the memo (e.g. authorizer-call count equals item count for empty ids, vs. 1 for a +memoized duplicate pair) — state which shape was used. + +**Commit:** `fix(acl): revalidation memo rejects empty node identifiers, mirroring the authorizer's guard` + +--- + +### Task 7: SQL-Server-backed tests for the drift probe + rename script scan + +**Classification:** standard +**Estimated implement time:** ~5 min +**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 8, 10, 11 + +**Files:** +- Create: `tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs` +- Create: `tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs` +- Create: `tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceRenameWarningSqlServerTests.cs` +- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs:1451` (one comment line) +- Modify (if needed): `tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj` (`Microsoft.Data.SqlClient` if not already transitive) + +**Spec.** Two register items say the same thing: EF InMemory does not exercise the SQL +Server provider's translation, so (a) the Task 17 `GroupBy(_ => 1)` watermark aggregate +(`DeploymentDriftProbe.ReadWatermarkAsync`, `DeploymentDriftProbe.cs:122-135`, over +**`ConfigEdits`** — not deployments; the folder name misleads) and (b) the Task 21 +expression-built `SourceCode.Contains` OR-chain (`RawTreeService.AnyPathContained`, +`:1488-1513`) are unverified where they matter. + +1. **Fixture**: copy `SchemaComplianceFixture`'s shape + (`tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SchemaComplianceFixture.cs`): env + `OTOPCUA_CONFIG_TEST_SERVER` (default `10.100.0.35,14330`) + + `OTOPCUA_CONFIG_TEST_SA_PASSWORD` (default `OtOpcUaDev_2026!`), unique per-run DB name, + `Database.Migrate()`, drop-on-dispose, collection fixture. **One deliberate deviation**: + probe reachability first (short TCP/`SqlConnection` attempt) and expose a `SkipReason`; + tests call `Assert.Skip` when unreachable (the `Snap7ServerFixture.cs:41-92` pattern) — + AdminUI.Tests must stay green offline, unlike Configuration.Tests which fails hard. + Document the deviation in the fixture doc comment. +2. **Drift probe test**: seed `ConfigEdits` rows with known timestamps, run the real probe + against the SQL-backed context factory, assert `LastEditUtc`/`EditCount` — proving the + `GroupBy(_ => 1)` aggregate translates and returns the right watermark (a translation + failure throws; wrong translation returns wrong values — both caught). +3. **Rename scan test**: seed a `Script` whose `SourceCode` contains a tag path in + **different case** than the rename target. On SQL Server's default case-insensitive + collation the scan must find it (the "collation widening" the InMemory provider cannot + show — InMemory `string.Contains` is case-sensitive). Assert the warning names the + script. Also one same-case control. Exercise through the real + `BuildScriptReferenceWarningAsync` (`RawTreeService.cs:1437-1470`) with > 1 batch if + cheap (201 paths) to also cross the chunk boundary against the real provider. +4. **Comment**: one line at the batching loop (`RawTreeService.cs:1451`): worst case is + ~250 sequential round-trips for a rename touching the 50k-path scale (200-term chunks); + deliberate — bounded memory beats one giant OR-chain. + +**Commit:** `test(adminui): SQL-Server-backed pins for the drift watermark aggregate and the rename scan's collation widening` + +--- + +### Task 8: ScriptedAlarmEngine — TrackBackgroundTask ordering + volatile `_disposed` + +**Classification:** high-risk (concurrency, disposal) +**Estimated implement time:** ~5 min +**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 7, 10, 11 + +**Files:** +- Modify: `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs` (`TrackBackgroundTask` `:996-1006`, `_disposed` `:172`, call sites `:559`, `:989`) +- Test: `tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs` + +**Spec.** Two recorded gaps: + +1. **Registration-ordering**: callers evaluate the async-method argument (which starts the + work) *before* `TrackBackgroundTask` adds it to `_inFlight` + (`TrackBackgroundTask(DrainDirtyAsync(...))` at `:559`). `Dispose` can snapshot + `_inFlight` (`:1090-1094`) in that window and drain past a genuinely in-flight task. + Fix by removing the window, not shrinking it: change the call shape to register-first — + e.g. give `TrackBackgroundTask` a `Func` overload that (under `_inFlightLock`) + installs a placeholder/`TaskCompletionSource` (or checks `_disposed` and declines to + start), then invokes the factory and links completion — so a task that runs is always + visible to `Dispose`'s snapshot, and after `_disposed` no new background work starts. + Preserve exactly: the drain-latch handover semantics + (`Sustained_synchronous_drain_failures_hand_over_without_deepening_the_stack`, `:1627` — + the handover MUST keep spawning via `Task.Run` on a fresh stack, commit `c8a70143`), the + `ExecuteSynchronously` self-removal, and `Dispose`'s synchronous `WhenAll` drain. If the + handover call site (`TrackBackgroundTask(Task.Run(() => DrainDirtyAsync(ct)))`) needs + reshaping to the factory form, keep the `Task.Run` inside. +2. **`_disposed`**: make it `volatile` (`:172`) — it is written in `Dispose` (`:1078`) and + read unsynchronized on other threads (`:235, 544, 603, 662, 682, 987, 1027`). + +**Tests.** Red-first for the ordering fix where constructible: a factory whose task +completes synchronously/immediately while `Dispose` races — deterministically, pin the new +invariant instead: (a) after `Dispose`, `TrackBackgroundTask`'s factory form does not invoke +the factory (no new work post-disposal); (b) a task started via the factory form is awaited +by `Dispose` even when it completes before the continuation attaches. Every existing engine +test (`:817`, `:1458`, `:1521`, `:1563`, `:1627`) must stay green — they are the spec for +what must not change. + +**Commit:** `fix(alarms): background tasks register before they start; volatile _disposed` + +--- + +### Task 9: Test-reliability hardening (durability second-reader, #491 controls) + +**Classification:** small +**Estimated implement time:** ~4 min +**Parallelizable with:** Tasks 4, 5, 6, 7, 8, 10, 11 (NOT Task 3 — same recorder test file) +**Blocked by:** Task 3 + +**Files:** +- Modify: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/FasterLogHistorizationOutboxTests.cs:172-197` (+ helper `ReadCommitted` `:203+`) +- Modify: `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs:359-387`, `:797-831` (doc comments only) + +**Spec.** +1. `AppendBatch_group_commit_is_durable_before_dispose` depends on filesystem flush + visibility to a second `FasterLog` reader, which macOS does not guarantee on the + Windows/Linux timescale. Prefer a robust wait: retry the second-reader scan (re-open or + re-read up to `CommittedUntilAddress`) with a bounded poll (e.g. up to 5 s) before + asserting — the property under test is "durable before dispose", which a bounded wait + still proves (the writer is not disposed during the poll). Only if FasterLog's reader + genuinely cannot observe later flushes on re-scan, fall back to an OS-conditional + `Assert.Skip` on macOS with a comment naming the platform limitation. Report which. +2. The two #491 yielding-fake tests: ensure each carries a comment stating the + falsification control is nondeterministic under load (ThreadStatic actor cell) — a single + green run of the intended-red mutation is not proof the regression is caught. The `:790-793` + comment covers part of this; make both tests carry the load-nondeterminism warning. + +**Commit:** `test(historian): bounded-wait durability read; honest nondeterminism notes on the #491 controls` + +--- + +### Task 10: MTConnect `Build` streams through XmlReader + +**Classification:** standard +**Estimated implement time:** ~5 min +**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 7, 8, 11 + +**Files:** +- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs` (`Build` `:221`, the three `Parse` overloads `:102/:138/:155`) +- Test: the existing MTConnect parser test file(s) under `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/` + +**Spec.** Task 27 removed the per-chunk string copies; `Build(XDocument)` still materialises +the whole document tree per sample chunk. Rewrite the parse path to stream via `XmlReader` +(`XmlReader.Create(stream, settings)` with DTD prohibited, async off — match the sync call +shape) extracting exactly what `Build` extracts today, and route all three `Parse` overloads +through it (the string overload via a `StringReader`). Requirements: + +- **Byte-for-byte behavioural parity** with the current `Build` on every existing test — + the current suite is the spec. Namespace handling: MTConnect documents version their + namespace URI; whatever name/namespace matching `Build` does today (local-name-based or + namespace-qualified) must be preserved exactly — read `Build` first and mirror it. +- Keep the `AsStream` zero-copy path (`:216-219`) feeding the reader. +- If, mid-implementation, `Build`'s extraction turns out to lean on document-order queries + that make a single-pass reader genuinely intractable (e.g. backward references), STOP and + report rather than shipping a half-streamed hybrid — that finding would itself close the + register item as "not worth it". + +**Tests.** The existing suite must pass unchanged (parity). Add one test only if a gap is +found in existing coverage of odd shapes (empty streams element, condition entries, unknown +elements skipped) — cite, don't duplicate. + +**Commit:** `perf(mtconnect): stream sample-chunk parsing through XmlReader; no per-chunk document tree` + +--- + +### Task 11: IS7Plc multi-variable read seam + +**Classification:** small +**Estimated implement time:** ~3 min +**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 7, 8, 10 + +**Files:** +- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs` (interface `:18-66`, `S7PlcAdapter` `:125-149`) + +**Spec.** S7.Net 0.20.0 ships `Plc.ReadMultipleVarsAsync(List, CancellationToken)` +(confirmed in the shipped DLL); `IS7Plc` does not expose it. Add +`Task> ReadMultipleVarsAsync(List items, CancellationToken ct)` +to the interface and implement in `S7PlcAdapter` delegating to the library, wrapped in +`S7OperationDeadline.RunAsync` exactly like the existing members (`:126-149` pattern — +mirror it, including the deadline/exception shape). Return the same `DataItem` list S7.Net +populates. No driver-side use yet (Task 12). Update any test fakes implementing `IS7Plc` +(they will fail to compile otherwise — implement as `throw new NotSupportedException` in +fakes that don't care, or a delegate hook in the shared fake if one exists). + +**Tests.** Compile-level only for the adapter (it is a one-line delegation under the same +deadline wrapper the other members already have pinned); the behaviour tests come with Task 12's fake-driven driver tests. State this honestly. + +**Commit:** `feat(s7): expose ReadMultipleVarsAsync through the IS7Plc seam` + +--- + +### Task 12: S7 multi-var reads for span leftovers + +**Classification:** high-risk (wire behaviour, partial-failure semantics) +**Estimated implement time:** ~6 min (accepted over budget — do not split further; the fallback semantics need one head) +**Blocked by:** Tasks 1, 11 (both touch `S7Driver.cs` / `IS7Plc.cs`) + +**Files:** +- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (the `perTag` leftover loop, `:602-614` area) +- Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/` (extend `S7SpanReadParityTests.cs` or sibling) + +**Spec.** The Task 13 planner reads one PDU per span; leftovers (`plan.LeftoverIndexes`: +Timer/Counter tags, unparseable addresses, `PrefersPerTagRead` singles) still go one +round-trip each. Batch the *batchable* leftovers through the new +`IS7Plc.ReadMultipleVarsAsync`: + +- **Partition** leftovers into DataItem-expressible vs not. Timer/Counter: S7.Net's + `DataItem`/`VarType` does support Timer/Counter — verify against the library types and + include them only if the driver's existing per-tag path for them maps cleanly onto a + `DataItem`; if the mapping is not obviously identical, leave Timer/Counter on the per-tag + path and say so (correctness beats the round-trip). Unparseable addresses stay per-tag by + definition. +- **Chunk** conservatively: a multi-var request's item headers consume PDU space + (~12 bytes/item request-side against the common 240-byte PDU) and S7.Net does NOT chunk + multi-var reads for you. Use a `const int MultiVarReadMaxItems = 12` with a comment + deriving the number; no new config key (YAGNI — the planner options weren't config either + until they were needed). +- **Fallback, per chunk**: if `ReadMultipleVarsAsync` throws for a chunk, fall back to the + existing per-tag `ReadOneAsync` path for that chunk's tags — a multi-var failure must + never make tags Bad that a per-tag read would have served. Only per-tag failure produces + the Bad snapshot (reuse `RecordReadFailure` semantics). Per-item errors inside a + successful multi-var response (S7.Net surfaces item error codes) likewise fall back + per-tag for the failed items only, if the library exposes them; read the library's + `DataItem` result shape and handle what it actually gives you. +- Results must sort back into batch order exactly as today (`:613`). +- Health semantics unchanged: the same success/failure accounting the per-tag loop feeds. + +**Tests.** Fake `IS7Plc` driven, red-first: (1) N batchable leftovers issue +`⌈N/12⌉` multi-var calls, zero per-tag calls; (2) parity — values decoded from multi-var +equal the per-tag path's (mirror `S7SpanReadParityTests` structure); (3) a throwing +multi-var chunk falls back to per-tag and serves Good; (4) per-tag-only tags (Timer/Counter +if excluded, unparseable) untouched; (5) falsification: break the chunk size (mutate to +1000) or the fallback and show the corresponding test goes red, then revert via Edit. + +**Commit:** `perf(s7): span leftovers batch through multi-var read PDUs with per-chunk fallback` + +--- + +### Task 13: S7 live gate — span planner + multi-var against the s7_1500 fixture + +**Classification:** standard (integration test + live run) +**Estimated implement time:** ~6 min including the live run +**Blocked by:** Tasks 2, 12 + +**Files:** +- Create: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanReadTests.cs` + +**Spec.** The register's ⚠️: the 16/220 span defaults are analytically chosen and never +live-gated. Add fixture-backed tests in the established pattern +(`[Collection(Snap7ServerCollection.Name)]`, `[Trait("Category","Integration")]`, +`[Trait("Device","S7_1500")]`, `Assert.Skip(sim.SkipReason)` when unreachable — see +`S7_1500SmokeTests.cs:16-24`, fixture `Snap7ServerFixture.cs:40-92`, endpoint env +`S7_SIM_ENDPOINT` default `10.100.0.35:1102`): + +1. Seed (via whatever helper the existing smoke tests use to seed DB bytes) a tag layout + that genuinely exercises the planner: several tags in one DB within the 16-byte gap + (one span), a tag beyond the gap (second span or leftover), and ≥ 2 leftover-class tags + (to exercise the Task 12 multi-var path against the real wire). +2. Read the batch with span reads enabled (defaults) and again with spans disabled + (`SpanReadMaxGapBytes = -1`); assert value-for-value parity and all-Good quality. +3. **Run it live** against the fixture: probe `10.100.0.35:1102` first; if down, bring the + stack up over SSH (`ssh dohertj2@10.100.0.35 'cd /opt/otopcua-s7 && docker compose --profile s7_1500 up -d'` + — the Mac path per CLAUDE.md; `lmxopcua-fix` does not exist here). Then + `dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests --filter "FullyQualifiedName~S7_1500SpanRead"`. + The gate PASSES only on a real run (skipped-because-unreachable is not a pass — report + honestly). Record the result in the test file header comment and the task report. + +**Commit:** `test(s7): live-gate span reads + multi-var leftovers against the s7_1500 fixture` + +--- + +### Task 14: docker-dev live gate (regression of the merged remainder) + +**Classification:** standard (no code — live verification) +**Estimated implement time:** ~10 min wall (mostly rig startup) +**Blocked by:** Tasks 1-12 (gates the branch's code state) + +**Files:** none (evidence appended to this plan's Outcomes) + +**Spec.** The remainder branch merged with **no live gate** (accepted at the time; recorded +as a release condition). Run the bounded regression gate on the local docker-dev rig +(OrbStack — the rig runs locally on this Mac; only fixtures live on 10.100.0.35): + +1. `docker compose -f docker-dev/docker-compose.yml up -d --build` (builds the current + branch image); wait for `cluster-seed` "done." (`docker-dev/README.md:71-77`). +2. Verify all nodes healthy; AdminUI at `http://localhost:9200` (login disabled — drive it + directly). +3. Deploy: headless `POST http://localhost:9200/api/deployments` with + `X-Api-Key: docker-dev-deploy-key`; confirm the deployment seals green (all nodes acked). +4. Data plane: Client.CLI `browse` + `subscribe` against `opc.tcp://localhost:4840`; + confirm values stream Good. Make a config edit that produces a surgical apply, deploy, + and confirm the open subscription survives (the 2026-08-15 gate's step 3, reduced). +5. `/alerts` and `/hosts` pages load and stream (exercises the Fleet/Hosts poll-guard and + telemetry changes). +6. Tear down: `docker compose -f docker-dev/docker-compose.yml down -v`. + +Out of gate scope (say so in the evidence): historian end-to-end (needs VPN/real gateway), +FetchAndCache flip, load-scale throughput. Any failure: STOP, report, do not "fix the rig" +around a product defect. + +**Commit:** none (evidence lands in Task 15's docs commit). + +--- + +### Task 15: Close-out — full sweep + Outcomes + +**Classification:** small +**Estimated implement time:** ~5 min plus sweep wall time +**Blocked by:** all prior tasks + +**Files:** +- Modify: this plan (append `## Outcomes`: per-task commit SHAs, deviations, the two live-gate evidence blocks) +- Modify: `docs/plans/2026-08-17-perf-followups.md.tasks.json` (final ledger) +- Modify: `docs/plans/2026-08-15-perf-roadmap-remainder.md` (tick the closed register items — a one-line "closed by , 2026-08-17" annotation per item; do NOT rewrite history) +- Modify (if any behaviour docs drifted): `docs/Historian.md`, `docs/ScriptedAlarms.md`, `CLAUDE.md` (only the lines these tasks invalidate — e.g. the "no production consumer of the overrun counter" caveat) + +**Spec.** (1) `dotnet build ZB.MOM.WW.OtOpcUa.slnx` — 0 errors. (2) Full unit sweep across +all test projects (the remainder plan's §Task 28 procedure; DB suites against +`10.100.0.35,14330` if reachable, recorded either way; the two empty Wonderware test dirs +are glob noise, not failures). (3) Outcomes + register annotations + ledger. (4) Memory +update is the orchestrator's job, not this task's. + +**Commit:** `docs(plan): perf follow-ups close-out — outcomes, register annotations, sweep record` diff --git a/docs/plans/2026-08-17-perf-followups.md.tasks.json b/docs/plans/2026-08-17-perf-followups.md.tasks.json new file mode 100644 index 00000000..63c7e2a8 --- /dev/null +++ b/docs/plans/2026-08-17-perf-followups.md.tasks.json @@ -0,0 +1,21 @@ +{ + "planPath": "docs/plans/2026-08-17-perf-followups.md", + "tasks": [ + {"id": 1, "subject": "Task 1: Per-driver poll-overrun meter + PollGroupEngine doc notes", "status": "pending"}, + {"id": 2, "subject": "Task 2: S7 span-option validation guard", "status": "pending"}, + {"id": 3, "subject": "Task 3: Historian drain — shutdown cancellation is not a per-tag failure", "status": "pending"}, + {"id": 4, "subject": "Task 4: Direct-apply artifact read — explicit command timeout", "status": "pending"}, + {"id": 5, "subject": "Task 5: SessionAclDecisionCache eviction race + stress test", "status": "pending"}, + {"id": 6, "subject": "Task 6: Revalidate memo — reject empty node identifiers", "status": "pending"}, + {"id": 7, "subject": "Task 7: SQL-Server-backed tests for drift probe + rename scan", "status": "pending"}, + {"id": 8, "subject": "Task 8: ScriptedAlarmEngine TrackBackgroundTask ordering + volatile _disposed", "status": "pending"}, + {"id": 9, "subject": "Task 9: Test-reliability hardening (durability second-reader, #491 controls)", "status": "pending", "blockedBy": [3]}, + {"id": 10, "subject": "Task 10: MTConnect Build streams through XmlReader", "status": "pending"}, + {"id": 11, "subject": "Task 11: IS7Plc multi-variable read seam", "status": "pending"}, + {"id": 12, "subject": "Task 12: S7 multi-var reads for span leftovers", "status": "pending", "blockedBy": [1, 11]}, + {"id": 13, "subject": "Task 13: S7 live gate — span planner + multi-var vs s7_1500 fixture", "status": "pending", "blockedBy": [2, 12]}, + {"id": 14, "subject": "Task 14: docker-dev live gate (regression of the merged remainder)", "status": "pending", "blockedBy": [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]}, + {"id": 15, "subject": "Task 15: Close-out — full sweep + Outcomes", "status": "pending", "blockedBy": [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14]} + ], + "lastUpdated": "2026-08-17T08:00:00Z" +}