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

50 KiB
Raw Blame History

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 -- <files> 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<long> PollOverruns = Meter.CreateCounter<long>("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<string, object?>("driver.type", "<type>")) where <type> 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:

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<Task> 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<DataItem>, CancellationToken) (confirmed in the shipped DLL); IS7Plc does not expose it. Add Task<IReadOnlyList<DataItem>> ReadMultipleVarsAsync(List<DataItem> 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

Outcomes

Written at close-out, 2026-08-17, on branch perf/followups. All 15 tasks complete; both release-condition live gates PASSED.

Sweep record

dotnet build ZB.MOM.WW.OtOpcUa.slnx → 0 errors, 896 warnings (all pre-existing: xUnit1051 cancellation-token advice and the repo's own OTOPCUA0001 CapabilityInvoker analyzer firing on test call sites).

Full unit sweep — every *.Tests project in the solution, run per-project with --no-build against the tree at the close-out commit. *.IntegrationTests are build-only by convention and were not run in the sweep (the S7 suite ran separately as Task 13's live gate; the two empty tests/Drivers/…Driver.Historian.Wonderware*.Tests directories carry no csproj and are glob noise).

43 / 43 test projects green (exit code 0 on every one).
Passed 10 358
Failed 0
Skipped 19
Total 10 377

No test failed, so no isolation re-run was needed. All 19 skips are pre-existing, env-gated suites, spread across five projects — Core.Tests (1), Driver.Galaxy.Tests (8), Driver.Historian.Gateway.Tests (6, the LiveIntegration category that needs the wonder-sql-vd03 VPN), OpcUaServer.Tests (1), Runtime.Tests (3). None of the skips are the new SQL-Server-backed tests: AdminUI.Tests ran 1537 / 0 skipped and Configuration.Tests 190 / 0 skipped, which is the evidence that Task 7's pins actually executed against a real SQL Server rather than skipping past themselves.

The DB-backed suites (Configuration.Tests, AdminUI.Tests including Task 7's new SQL-Server-backed pins) ran against a reachable 10.100.0.35,14330 — they were executed, not skipped. None of the known load-flake candidates flaked (DriverInstanceActorCommsLossWatchdogTests, RawTreeServiceRenameWarningTests, RouteNativeAlarmAck_on_non_primary_is_dropped, plus this branch's new SessionAclDecisionCache concurrency stress test and the ContinuousHistorizationRecorder timing tests).

Per-task outcomes

# Task Commit(s) Outcome + deviation
1 Per-driver poll-overrun meter + PollGroupEngine doc notes e22ecea0, 97902d68 Done. Review deviation: the tag keys were literals; moved to snake_case consts (driver_type / driver_instance) so they match the rest of the Meter and a dashboard's group-by cannot split across two spellings.
2 SpanReadMaxSpanBytes validation guard acb37724 Done, as planned — validated at parse time, with the over-PDU degradation documented.
3 Historian drain rethrows a shutdown OperationCanceledException 7870c91f Done. Deviation in severity, not shape: the red run showed the bug did not merely inflate _tagWriteFailures — a shutdown mid-drain discarded healthy tags' samples. A pass abort, not a per-tag failure.
4 Bound the Direct-mode LoadArtifactBlob read dba5f806 Done, with one addition the plan did not anticipate: an IsRelational() guard, because the extension throws on the InMemory provider the actor tests use. EF 10.0.7 verified empirically to reset the timeout on return to the pool, so the local SetCommandTimeout cannot pollute the pooled factory. ⚠️ The relational branch is consequently untested in-suite.
5 ACL _globalCount drift + concurrency stress test d8849345 Done — beyond the planned scope, and provably so. The planned counter swap alone was insufficient; a seal sentinel and a TryBookInsert compare-exchange were both required. Falsification: removing either went 6/6 red.
6 Revalidation memo rejects empty node identifiers c4d8e4a2 Done as a pattern-form guard mirroring the authorizer's; the bypass is pinned by a clock-call count rather than by observing the memo directly.
7 SQL-Server-backed pins for the drift watermark + rename collation c149410b Done. Deviation: AdminUI.Tests is pinned to xunit v2 by Akka.TestKit.Xunit2, so Assert.Skip is unavailable — an overriding SqlServerFactAttribute gives a genuine skip instead. Both the GroupBy watermark aggregate and the CHARINDEX collation widening confirmed live against SQL Server.
8 TrackBackgroundTask registration ordering + volatile _disposed 5ae43023 Done via a TCS-placeholder register-first factory (registration precedes the work starting, closing the gap at its root rather than narrowing it); _disposed made volatile.
9 Durability-test robustness + honest #491 nondeterminism notes 6b46c485 Done. Deviation: the planned macOS skip proved unnecessary — verified empirically that a bounded wait is sufficient, so the test runs on macOS rather than skipping. The #491 falsification control's nondeterminism is inherent and is now documented, not fixed.
10 MTConnect sample-chunk parsing through XmlReader cda82c07 Done. 566/566 parity against the pre-change parser, ~19% allocation win. One disclosed behaviour change: on a doubly-broken document the ordering of the reported errors differs.
11 Expose ReadMultipleVarsAsync through the IS7Plc seam 6ee553cf Done, as planned.
12 S7 span leftovers batch through multi-var read PDUs 20022083 Done, with two forced deviations: S7.Net throws for the whole multi-var request rather than per item, so a bad item is handled by a per-chunk fallback; and Timer/Counter areas are excluded because S7.Net boxes them as int where the single-read path yields ushort, and box parity is load-bearing downstream.
13 S7 live gate (span reads + multi-var leftovers) 57c66416 LIVE GATE PASSED vs 10.100.0.35:1102. Evidence below.
14 docker-dev live gate (regression of the merged remainder) (no commit — evidence below) LIVE GATE PASSED. Evidence below.
15 Close-out — full sweep + Outcomes 7c5a843a, this commit Done. 7c5a843a carries the review round's comment-only fixes; this commit carries the Outcomes, the live-gate evidence, the register annotations, and the ledger.

Live gate — S7 span reads (Task 13)

2026-08-17, against the s7_1500 fixture at 10.100.0.35:1102. PASSED.

  • Results: 3/3 span-read tests green; 7 passed / 1 skipped across the full Device=S7_1500 suite. The single skip is the env-gated SYN-blackhole test, which needs a recipe the fixture does not provide — not a silent skip of the gated behaviour.
  • Wire shape (the actual claim under test): with spans on, 10 tags produced 3 requests — 2 block reads + 1 multi-var read carrying 3 items. With spans off, the same 10 tags produced 10 requests. Multi-variable and cross-area batching both confirmed on the wire, and a falsification bit was flipped to prove the counter was measuring the real traffic.
  • Reconnect suite ran live (not simulated) via S7_RECONNECT_BOUNCE_CMD and passed.
  • Not live-pinned: the Timer/Counter exclusion — the fixture has no TM/CT area, so the box-parity reasoning behind excluding them rests on unit tests only.
  • Ops notes for whoever re-runs this (both cost time on this run):
    • The fixture profile is baked into the image, so --build is mandatory after editing a profile. Confirm the seed actually took by looking for seeded DB1 … seeds=<n> in docker logs; a stale image seeds the old profile silently.
    • /opt/otopcua-s7 on the docker host is root-owned, so a plain rsync fails with status 23 while looking successful in its summary output. Use --rsync-path="sudo rsync".

Live gate — docker-dev regression (Task 14)

2026-08-17, local OrbStack rig, branch at 6b46c485, ~14.5 min wall. PASSED.

  • Boot: clean-slate build + boot; 8 containers healthy, RestartCount=0 across the board.
  • Deploy: c50678b2 sealed in 306 ms with 6/6 Applied.
  • Data plane: browse / read / write end-to-end against the pymodbus fixture; a 180 s subscription streamed with 0 Bad values.
  • Surgical apply (the headline regression target): a real UNS-reference surgical apply — deploy 3df4b321, logging applied plan kind=PureAdd added=2 changed=1 surgicalRawTags=1 rebuild=False. The open subscription streamed Good across the apply, and the newly-added UNS mirror served live immediately. This is the behaviour the 2026-08-15 surgical-deploys work exists to protect, exercised as a genuine surgical path rather than a rebuild in disguise.
  • Deploy gate: correctly rejected a malformed first attempt with 422 (UnsSegmentInvalid, EquipmentIdNotDerived) — the gate is live, not bypassed.
  • AdminUI: /fleet, /alerts, /hosts, /uns, /raw, /deployments all 200 with live data.
  • Logs: 0 ERR/FTL, excluding the by-design standby 503s.
  • Teardown: clean.
  • Anomalies (non-blocking, all disclosed):
    • A transient 404 on the first POST /api/deployments ~40 s after boot; the retry succeeded. Scripted deploys should poll rather than fire once (see the follow-up register below).
    • Boot-time telemetry-dial reconnect warnings, settling within one cycle.
    • ScriptLogEntry dead-letters on site nodes, one per deploy — pre-existing, not introduced by this branch.
  • NOT exercised (say so plainly, do not read the pass as wider than it is): historian end-to-end (needs the VPN), the FetchAndCache flip, the ACL-enabled enforcement path, the alarm-bearing surgical ratchet to rebuild, load-scale throughput, and real-browser Blazor circuits.

Follow-up register (opened by this round)

Open items this branch surfaced but deliberately did not close. None blocks the branch.

ACL

  • ⚠️ Pre-existing generation-rollover window in SessionAclDecisionCache.InvalidateIfStale. While the CAS winner is running ClearAll(), a racing caller can still serve a stale-generation verdict out of a shard the winner has not purged yet. The window is microseconds and key revocation/expiry are unaffected (both are re-checked above the cache on every call), so this is a stale grant walk, not a stale key. Fix shape if it ever matters: stamp the generation on Shard itself, or make the racer wait on the winner.
    • Closed by 1979cf41 + 98d6a190 (r2). Took the first fix shape — the generation is stamped on Shard, TryGet is generation-gated, and Set displaces a mismatched shard; 98d6a190 added the monotonic <= guard that stops a stale-generation straggler rolling the stamp backward. One benign residual remains (see r2's register).
  • A fresh Shard could take an initial-capacity hint for a session known to be crawling, to avoid the dictionary's growth rehashes on exactly the workload the per-key ceiling exists for.
    • Closed by 1979cf41 (r2). The eviction swap seeds the replacement shard with a capacity hint, gated on the generation matching.

S7

  • No cancellation-branch test on any read path. Pre-existing, but this branch added a third path (multi-var), so the gap now spans three.
    • Closed by 4f860733 (r2). The span and multi-var cancellation reopen contracts are pinned (hang knobs + entered-TCS + CreatedPlcCount), and 120e885b added the assertion that the stale handle is disposed, not merely replaced.
  • No integration case for one bad decode inside an otherwise-good chunk — the per-chunk fallback is unit-tested only.
    • Closed by 4f860733 (r2). One-bad-decode-in-chunk pinned (only that tag goes Bad, no fallback), alongside the short-answer shape mismatch (whole-chunk fallback).
  • The Timer/Counter exclusion needs a live pin, which needs a fixture with a TM/CT area.
    • Closed by fd202d81 (r2). TM/CT areas added to the s7_1500 fixture and the exclusion live-pinned against 10.100.0.35:1102 at device 0, the fixed point of snap7's BIT-address resolution and a real CPU's device-number addressing; the device-number leg (T5/C7) is env-gated on S7_TIMER_NUMBER_ADDRESSING=1 as a ready-made real-CPU gate.

Historian

  • Task 3's Task.Delay(500) is still a hope-wait and could become a bounded poll, the same treatment Task 9 gave the durability test.
    • Closed by 45d67092 + 68461fbf (r2). All three fixed sleeps became bounded watches on real observables (RemoveCallCount, queue depth, PeekBatchCallCount), with the negative watch's cap anchored to a measured ~150 ms falsification trip.

AdminUI tests

  • A mid-run SQL connectivity blip surfaces as SqlException failures, not skips — the reachability probe is one-shot, taken before the run.
    • Documented wontfix (r2). The one-shot probe is deliberate — a per-test probe would turn a genuine mid-run outage into silent skips; verified against the comment at the probe site and left as-is.
  • Orphaned test databases have no reaper. A killed test process leaves its uniquely-named database behind on 10.100.0.35,14330.
    • Closed by 8946644b + 580b77ce (r2). A one-shot startup reaper drops >24 h-orphaned databases per fixture prefix; 580b77ce fixed a review-found Critical (unvalidated name into interpolated sa DDL) with a GUID-suffix shape guard plus parameterized QUOTENAME/sp_executesql.

MTConnect

  • AsStream wants a buffer-ownership comment so a future caller does not have to infer the lifetime from the call site.
    • Closed by 1edfc263 (r2). The doc comment now states the array-backed aliasing, the synchronous call-duration lifetime, and that the result holds no reference into the buffer.

docker-dev

  • The first POST /api/deployments after boot can 404. Worth a readiness gate on the deployments endpoint, so scripted deploys do not have to encode a retry.
    • Closed by a61b3c98 + 9f09a62a (r2) — as a gate plus instrumentation, not a diagnosis: deploy.sh polls /health/ready (2 s/120 s) then POSTs once un-retried, and Traefik now runs --accesslog so the next occurrence names the matching router. The 404's mechanism remains unconfirmed until that log catches one.
Reference in New Issue View Git Blame Copy Permalink