diff --git a/CLAUDE.md b/CLAUDE.md index ea643afd..e02d5fb1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -763,9 +763,10 @@ Two things to know before relying on any of this: `TriePermissionEvaluator`'s, so **any divergence between the two clocks denies every request** with a verdict indistinguishable from "no grant matched". Found by a test clock two hours ahead. - **Stage 6's hot seams (read/browse/subscribe) go through a per-`(session, node, operation, realm)` - decision cache**, cleared wholesale when the generation advances. It is **sharded per session**, so a - single client crawling a large tree past the per-session ceiling evicts only its own shard and not - every other session's decisions. ⚠️ It caches the **grant walk only** — key revocation and expiry are + decision cache**, cleared wholesale when the generation advances. It is **sharded per API key** + (`ApiKeyId ?? Principal` — despite the type name, sessions sharing one key share one shard and one + budget), so a client crawling a large tree past the per-key ceiling evicts only that key's shard and + not every other key's decisions. ⚠️ It caches the **grant walk only** — key revocation and expiry are re-checked **before** the cache lookup on every call, because expiry moves with the clock rather than with a deployment. - **Browse needs ancestor visibility, and it must NOT be a synthesized grant on the ancestors.** A diff --git a/docs/plans/2026-08-15-perf-roadmap-remainder.md b/docs/plans/2026-08-15-perf-roadmap-remainder.md index 3ab138df..fefb89a4 100644 --- a/docs/plans/2026-08-15-perf-roadmap-remainder.md +++ b/docs/plans/2026-08-15-perf-roadmap-remainder.md @@ -1340,6 +1340,12 @@ whose message mentions it. Open items the work surfaced but deliberately did not close. None blocks the branch. +> **Annotated 2026-08-17 by the `perf/followups` branch** (plan: +> [`2026-08-17-perf-followups.md`](2026-08-17-perf-followups.md)). Items it closed carry a +> ` — closed by , 2026-08-17` suffix; items with no suffix remain open. Annotations are +> **append-only** — the original text of each item is untouched, including where the follow-up +> work reached a different conclusion than the item predicted. + **Cross-repo** - **HistorianGateway needs a multi-tag batch `WriteLiveValues` RPC.** Client `0.3.0` exposes @@ -1351,22 +1357,31 @@ Open items the work surfaced but deliberately did not close. None blocks the bra - **S7: multi-variable PDUs for span leftovers.** The Task 13 planner groups contiguous reads into one PDU per span; tags that do not fall in a span are still one PDU each. A - multi-var PDU would absorb them. + multi-var PDU would absorb them. — closed by `20022083`, 2026-08-17 (per-chunk fallback: + S7.Net throws for the whole multi-var request, so a bad item fails its chunk, not the pass; + Timer/Counter areas are excluded on an int-vs-ushort box-parity difference). - ⚠️ **S7 span planner has no live gate.** The `16` (max gap) / `220` (max span bytes) defaults were chosen analytically and pinned only by unit tests. Run the planner against the `s7_1500` fixture (`lmxopcua-fix up s7 s7_1500`, `S7_SIM_ENDPOINT=10.100.0.35:1102`) - before trusting them on real hardware. + before trusting them on real hardware. — closed by `57c66416`, 2026-08-17 (**LIVE GATE + PASSED** vs `10.100.0.35:1102`; 3 wire requests for 10 tags with spans on vs 10 with spans + off). - **`SpanReadMaxSpanBytes` has no validation guard.** A value above the negotiated PDU size - is accepted at config time and fails at read time. + is accepted at config time and fails at read time. — closed by `acb37724`, 2026-08-17. - **MTConnect `Build` could stream through `XmlReader`.** Task 27 removed the per-chunk - string copies; the document build still materialises. + string copies; the document build still materialises. — closed by `cda82c07`, 2026-08-17 + (566/566 parity, ~19% allocation win; one disclosed error-ordering change on a + double-broken document). - **No driver-level meter for the PollGroupEngine overrun counter.** The counter exists and - is observable in-process but is not exported per driver. + is observable in-process but is not exported per driver. — closed by `e22ecea0`, 2026-08-17 + (tag keys moved to snake_case consts in `97902d68` after review). - **`NoteOverrun` conflates a slow success with a slow failure** — a poll that overran because the device was slow and one that overran because it timed out increment the same - counter. Worth a doc caveat rather than a code change. + counter. Worth a doc caveat rather than a code change. — closed by `e22ecea0`, 2026-08-17 + (doc caveat, as proposed). - **`backoffCap` wants a constructor doc note** so a future caller does not have to read the - loop to learn its units and its interaction with the poll interval. + loop to learn its units and its interaction with the poll interval. — closed by `e22ecea0`, + 2026-08-17. **Runtime / deploy path** @@ -1377,11 +1392,14 @@ Open items the work surfaced but deliberately did not close. None blocks the bra review, Important). The read now runs off the mailbox so it cannot wedge the actor, but a hung SQL read still pins a thread-pool thread indefinitely unless the `ConfigDb` connection string's command timeout bounds it. **Confirm that it does** — the code relies - on it and does not assert it. + on it and does not assert it. — closed by `dba5f806`, 2026-08-17 (explicit 30 s + `SetCommandTimeout` behind an `IsRelational()` guard; EF 10.0.7 verified empirically to + reset it on return to the pool, so there is no pool pollution). - **The historian drain's `catch` swallows a shutdown `OperationCanceledException` as a per-tag failure** when other tags in the same pass succeeded, spuriously incrementing `_tagWriteFailures` (Task 3 review; pre-existing, inherited). Rethrowing a genuine - `ct` cancellation is the fix. + `ct` cancellation is the fix. — closed by `7870c91f`, 2026-08-17 (the red run proved the + bug also discarded healthy tags' samples at shutdown, not just the counter). - **Host-side mux interest gating was skipped** — the batch envelope (Task 2) made the saving it would have bought immaterial. Recorded so it is not re-proposed as an oversight. @@ -1389,30 +1407,42 @@ Open items the work surfaced but deliberately did not close. None blocks the bra - **Task 6: `_globalCount` can drift under concurrent shard eviction**, and there is no concurrency stress test. Drift is bounded and only affects when trimming triggers, not - correctness of any decision. + correctness of any decision. — closed by `d8849345`, 2026-08-17 (the planned counter swap + alone was insufficient: a seal sentinel plus a `TryBookInsert` compare-exchange were both + needed, each proven necessary by a 6/6-red falsification run). - **Task 9: the empty-identifier pattern should be tightened** — the dedup key tolerates an - empty node identifier rather than rejecting it. + empty node identifier rather than rejecting it. — closed by `c4d8e4a2`, 2026-08-17 + (pattern-form guard; the bypass is pinned by a clock-call count). - **Task 17: the `GroupBy(_ => 1)` aggregate is unverified against real SQL Server.** It is covered only by EF InMemory, which does not exercise the provider's translation, so the - single-round-trip claim is untested where it matters. + single-round-trip claim is untested where it matters. — closed by `c149410b`, 2026-08-17 + (aggregate confirmed live against SQL Server; the suite skips cleanly offline via an + xunit-v2 `SqlServerFactAttribute`). - **Task 21: the `CHARINDEX`/collation widening needs a SQL-Server-backed test**, and the worst-case **250 sequential round-trips** (a rename touching the chunking limit) should - carry a one-line comment at the call site. + carry a one-line comment at the call site. — closed by `c149410b`, 2026-08-17 (collation + widening confirmed live; round-trips comment added). **Alarms** - **Task 15: `TrackBackgroundTask` has a registration-ordering gap** (pre-existing) — a task - can complete before it is registered. Also `_disposed` is non-volatile. + can complete before it is registered. Also `_disposed` is non-volatile. — closed by + `5ae43023`, 2026-08-17 (a TCS-placeholder register-first factory closes the ordering gap; + `_disposed` made volatile). **Test-suite reliability** (affects how reliably a regression is *caught*, not shipped behaviour) - **Task 1's second-reader durability test is fragile on macOS** — it depends on filesystem flush visibility to a second reader, which the platform does not guarantee on the same - timescale as Windows/Linux. + timescale as Windows/Linux. — closed by `6b46c485`, 2026-08-17 (a robust bounded wait + replaced the fixed sleep; the anticipated macOS skip turned out to be unnecessary — + verified empirically, the reader does become visible). - **The #491 falsification control is nondeterministic under load.** It relies on a `ThreadStatic` actor cell, so under a loaded sweep the intended-red mutation can occasionally pass. The test is still worth keeping; do not read a single green run of it - as proof the regression is caught. + as proof the regression is caught. — closed by `6b46c485`, 2026-08-17 (documented in the + test file as an honest nondeterminism note; the nondeterminism itself is inherent and + remains). **Accepted-not-fixed at the final integration review (2026-08-15)** — carried knowingly, none blocking: the poll **pacing** behaviour change ships with no production consumer of the overrun @@ -1423,3 +1453,10 @@ branch (docker-dev recipe: `ScriptTagCatalogSnapshotCache` expires on **wall-clock TTL**; the S7 span reader **stamps a partial span** when the PDU comes back short; MTConnect's `IsFramingOnly` fast path is **ASCII-only**; and a minor Razor exposure where `StateHasChanged` runs inside a `finally`. + +**Annotation, 2026-08-17 (append-only).** Two of the accepted-not-fixed items above have since been +retired by `perf/followups`: the overrun counter **now has a production consumer** (the per-driver +meter, `e22ecea0` + `97902d68`), and this branch's code state **has been live-gated** — S7 span reads +against the `s7_1500` fixture (`57c66416`) and a full docker-dev regression on the merged remainder +(2026-08-17, no commit). The `_globalCount` drift item is likewise closed (`d8849345`). The rest of +the paragraph stands as written. 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..317119b3 --- /dev/null +++ b/docs/plans/2026-08-17-perf-followups.md @@ -0,0 +1,780 @@ +# 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` + +--- + +## 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=` 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. +- **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. + +**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. +- **No integration case for one bad decode inside an otherwise-good chunk** — the per-chunk + fallback is unit-tested only. +- **The Timer/Counter exclusion needs a live pin**, which needs a fixture with a TM/CT area. + +**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. + +**AdminUI tests** + +- **A mid-run SQL connectivity blip surfaces as `SqlException` failures, not skips** — the + reachability probe is one-shot, taken before the run. +- **Orphaned test databases have no reaper.** A killed test process leaves its uniquely-named + database behind on `10.100.0.35,14330`. + +**MTConnect** + +- **`AsStream` wants a buffer-ownership comment** so a future caller does not have to infer the + lifetime from the call site. + +**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. 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..2fb65be7 --- /dev/null +++ b/docs/plans/2026-08-17-perf-followups.md.tasks.json @@ -0,0 +1,138 @@ +{ + "planPath": "docs/plans/2026-08-17-perf-followups.md", + "tasks": [ + { + "id": 1, + "subject": "Task 1: Per-driver poll-overrun meter + PollGroupEngine doc notes", + "status": "completed", + "note": "e22ecea0 + fix 97902d68 (snake_case tag consts); spec ✅ + code APPROVED; close-out nit: stale doc comment on PollOverruns at OtOpcUaTelemetry.cs:50 still says driver.type" + }, + { + "id": 2, + "subject": "Task 2: S7 span-option validation guard", + "status": "completed", + "note": "acb37724; code review APPROVED; deviations: new test file, 2 doc corrections (<= -1 wording)" + }, + { + "id": 3, + "subject": "Task 3: Historian drain — shutdown cancellation is not a per-tag failure", + "status": "completed", + "note": "7870c91f; spec ✅ + code APPROVED (Minor: Task.Delay(500) hope-wait at test :590); red run proved healthy-tag sample discard at shutdown; falsification via when-guard weakening" + }, + { + "id": 4, + "subject": "Task 4: Direct-apply artifact read — explicit command timeout", + "status": "completed", + "note": "dba5f806; spec ✅ + code APPROVED; IsRelational guard (InMemory tests); EF 10.0.7 pool-reset verified empirically; minor for close-out: comment could state the relational branch is untested in-suite" + }, + { + "id": 5, + "subject": "Task 5: SessionAclDecisionCache eviction race + stress test", + "status": "completed", + "note": "d8849345; spec ✅ (interleavings traced) + code APPROVED; falsification 6/6 red reverted. FOLLOW-UP for register (pre-existing, Important, non-blocking): InvalidateIfStale generation-rollover window — a racer can serve a stale-generation verdict from a not-yet-purged shard during the winner's ClearAll (microseconds; revocation/expiry unaffected). Fix shape: generation stamp on Shard or wait-on-winner. Minors: fresh-Shard capacity hint, un-elected global ClearAll comment, CAS backoff, stress-test CI wall-time, 'every booked entry' wording" + }, + { + "id": 6, + "subject": "Task 6: Revalidate memo — reject empty node identifiers", + "status": "completed", + "note": "c4d8e4a2; code review APPROVED (pattern-form deviation positive); bypass-count test shape (clock.Calls==2), clock-read-above-guard verified" + }, + { + "id": 7, + "subject": "Task 7: SQL-Server-backed tests for drift probe + rename scan", + "status": "completed", + "note": "c149410b; spec ✅ + code APPROVED (5 Minors: mid-run blip not skipped, kill-orphaned DBs, duplicated 200 const in test, ~3s discovery probe offline, TryDropDatabase catches DbException only); xunit-v2 SqlServerFactAttribute skip; 10/10 ran live vs 10.100.0.35" + }, + { + "id": 8, + "subject": "Task 8: ScriptedAlarmEngine TrackBackgroundTask ordering + volatile _disposed", + "status": "completed", + "note": "5ae43023; spec ✅ + code APPROVED (2 Minors: null-coalesce comment, remove-before-complete ordering comment); TCS placeholder register-first, volatile _disposed, handover Task.Run preserved" + }, + { + "id": 9, + "subject": "Task 9: Test-reliability hardening (durability second-reader, #491 controls)", + "status": "completed", + "blockedBy": [ + 3 + ], + "note": "6b46c485; code review APPROVED; robust-wait path (empirically verified fresh-reader re-open observes later commits; xunit v3 skip was available but unneeded); both #491 tests carry load-nondeterminism warnings" + }, + { + "id": 10, + "subject": "Task 10: MTConnect Build streams through XmlReader", + "status": "completed", + "note": "cda82c07; spec ✅ + code APPROVED (2 Minors: pre-existing error-phrase asymmetry, AsStream ownership comment); disclosed diff: double-broken doc reports observation error first; ~19% alloc win, honest" + }, + { + "id": 11, + "subject": "Task 11: IS7Plc multi-variable read seam", + "status": "completed", + "note": "6ee553cf; code review APPROVED; no shared delegate-hook fake exists — Task 12 gives ImageS7Plc a real multi-var impl" + }, + { + "id": 12, + "subject": "Task 12: S7 multi-var reads for span leftovers", + "status": "completed", + "blockedBy": [ + 1, + 11 + ], + "note": "20022083; spec ✅ + code APPROVED; Timer/Counter excluded (int vs ushort box parity); S7.Net throws wholesale → per-chunk fallback; 2 Minor coverage gaps for register: no cancellation-branch test on any read path (pre-existing), no one-bad-decode-in-chunk integration case" + }, + { + "id": 13, + "subject": "Task 13: S7 live gate — span planner + multi-var vs s7_1500 fixture", + "status": "completed", + "blockedBy": [ + 2, + 12 + ], + "note": "57c66416; GATE PASSED live vs 10.100.0.35:1102 (3 requests vs 10; multi-var leg + cross-area MW10 confirmed; falsification bit); code review APPROVED; ops findings: fixture profile baked into image (--build mandatory), /opt/otopcua-s7 root-owned (rsync needs sudo rsync-path); Timer/Counter not live-pinned (no TM/CT fixture area)" + }, + { + "id": 14, + "subject": "Task 14: docker-dev live gate (regression of the merged remainder)", + "status": "completed", + "blockedBy": [ + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12 + ], + "note": "GATE PASSED (no commit): fresh rig @6b46c485, deploy sealed 306ms 6/6 Applied, 180s subscription 0 Bad across a REAL surgical UNS-ref apply (rebuild=False, surgicalRawTags=1), UNS mirror live, panels 200, 0 ERR/FTL, teardown clean. Anomalies non-blocking: first-POST 404 ~40s after boot (retry), boot telemetry reconnect warnings, ScriptLogEntry dead-letters on sites. Not exercised: historian E2E (VPN), FetchAndCache, ACL-enabled path, alarm ratchet" + }, + { + "id": 15, + "subject": "Task 15: Close-out — full sweep + Outcomes", + "status": "completed", + "note": "07fd17a3 (outcomes/evidence/annotations docs commit; this ledger line recorded in the immediately following commit, since a commit cannot carry its own SHA) preceded by 7c5a843a (review-round comment fixes). Build 0 errors / 896 pre-existing warnings. Full unit sweep 43/43 projects green: 10358 passed, 0 failed, 19 skipped (all pre-existing env-gated; AdminUI.Tests 1537/0-skipped and Configuration.Tests 190/0-skipped confirm Task 7's SQL-backed pins RAN against 10.100.0.35,14330). No failures, so no isolation re-run. CLAUDE.md correction: the ACL decision cache is sharded per API key (ApiKeyId ?? Principal), not per session.", + "blockedBy": [ + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14 + ] + } + ], + "closeOut": "All 15 tasks complete on branch perf/followups. Both release-condition LIVE GATES PASSED: S7 span reads vs the s7_1500 fixture at 10.100.0.35:1102 (57c66416 — 3 wire requests for 10 tags with spans on vs 10 with spans off; multi-var + cross-area confirmed; Timer/Counter exclusion NOT live-pinned, no TM/CT fixture area), and the docker-dev regression on the merged remainder (no commit — deploy sealed 306ms 6/6 Applied, a REAL surgical UNS-reference apply with rebuild=False streaming Good across an open 180s subscription, 0 ERR/FTL, clean teardown). Close-out sweep: build 0 errors, 43/43 test projects green, 10358 passed / 0 failed / 19 skipped (all pre-existing env-gated). 12 register items in docs/plans/2026-08-15-perf-roadmap-remainder.md §Follow-up register are annotated closed (append-only, with SHAs); HistorianGateway's cross-repo batch WriteLiveValues RPC and the synchronous NodeDeploymentState writes stay open. A NEW follow-up register for this round (ACL generation-rollover window, S7 cancellation/one-bad-decode coverage, historian hope-wait, AdminUI mid-run-blip + orphaned test DBs, MTConnect AsStream ownership, docker-dev first-POST 404) lives in the plan's ## Outcomes section — none blocking. Not exercised by either gate: historian E2E (needs VPN), FetchAndCache flip, ACL-enabled enforcement path, alarm-bearing surgical ratchet, load-scale throughput.", + "lastUpdated": "2026-08-17T15:05:00Z" +} diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs index 7b9d7933..09950d24 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs @@ -42,6 +42,40 @@ public static class OtOpcUaTelemetry Meter.CreateCounter("otopcua.driver.lifecycle", unit: "{event}", description: "DriverInstanceActor lifecycle transitions (event=spawn|stop|fault)."); + /// + /// Poll cycles that ran longer than their subscription's publishing interval, as counted by + /// PollGroupEngine.OverrunCount. Core.Abstractions deliberately carries no metrics + /// dependency, so each poll driver wires this counter to the engine's onOverrun callback; + /// this is the production consumer that surface was always missing. Tagged + /// (driver_type, a DriverTypeNames constant) + /// + (driver_instance) — snake_case, per the + /// convention those two consts document. + /// A non-zero and climbing rate means the device cannot sustain its configured poll rate — the + /// loop is already running flat out. + /// + 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)."); + + /// + /// Tag key carrying the DriverTypeNames constant on . + /// The snake_case spelling is load-bearing, not a style choice. It is the same + /// literal key emits (DriverInstanceActor), and + /// every other instrument on this uses bare/snake_case keys + /// (event, kind, outcome, site, reason). A dotted + /// driver.type here would silently split a dashboard's "group by driver type" across + /// two key names that no single query can join. The seven poll drivers reference this const + /// rather than a literal so that can never drift back. + /// + public const string PollOverrunDriverTypeTag = "driver_type"; + + /// + /// Tag key carrying the emitting driver's instance id on . Cardinality + /// is bounded by the number of configured driver instances on the node. Snake_case for the same + /// convention reason as . + /// + public const string PollOverrunDriverInstanceTag = "driver_instance"; + // ---------------- VirtualTag / ScriptedAlarm engines ---------------- public static readonly Counter VirtualTagEval = diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs index fd0e17bc..cdb64c92 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs @@ -44,10 +44,16 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; /// consecutiveFailures > 0 with a backoffCap configured, the capped-exponential /// schedule wins, because a failing device must not be hammered at fixed rate. /// -/// No Meter lives here. Core.Abstractions deliberately references -/// nothing, so plus the onOverrun callback ARE the -/// instrumentation surface — a driver that wants a metric wires its own meter to the callback. -/// (Driver-level meters are a recorded follow-up, not part of this type.) +/// No Meter lives here, and that is still deliberate. +/// Core.Abstractions references nothing, so plus the +/// onOverrun callback ARE the instrumentation surface — a driver that wants a metric +/// wires its own meter to the callback. The driver-level meter this note used to record as a +/// follow-up is delivered: every one of the seven poll drivers now passes an +/// onOverrun closure that bumps OtOpcUaTelemetry.PollOverruns +/// (otopcua.driver.poll.overrun) on the shared OtOpcUa Meter, tagged with the +/// driver's DriverTypeNames constant and its driver-instance id. The counter lives in +/// Commons.Observability, on the consumer side of the reference graph — this type still +/// owns no Meter and takes no metrics dependency. /// /// Programmer errors and obviously-fatal exceptions (, /// , , @@ -85,7 +91,27 @@ public sealed class PollGroupEngine : IAsyncDisposable /// (05/STAB-8). When supplied, sustained reader failures stretch the retry cadence /// (interval, 2×, 4×, … saturating at this cap) instead of hammering a dead device every /// tick; a successful poll resets to the base interval. null (default) keeps the - /// byte-identical fixed-interval cadence for consumers that have not opted in. + /// byte-identical fixed-interval cadence for consumers that have not opted in. + /// Units: a ceiling on the post-poll wait — the + /// longest gap the loop will ever leave between a failed poll and the next attempt. It is + /// not a count, a multiplier, or a total-outage budget: a device that stays down waits + /// backoffCap between every retry, forever. + /// Interaction with — it REPLACES the paced delay, + /// it does not add to it. The loop picks exactly one of two branches per cycle. With no + /// failures outstanding (or no cap configured) the wait is the fixed-RATE remainder + /// max(interval − pollDuration, 0). With a cap configured AND + /// consecutiveFailures > 0, the whole remainder computation is bypassed and the + /// wait becomes ConnectionBackoff.ComputeDelay(interval, failures, cap) — the poll's + /// own elapsed time is NOT subtracted, so the failure branch is a fixed DELAY (cycle = + /// poll + backoff), deliberately unlike the fixed-rate success branch. + /// The interval feeding that schedule is the per-subscription interval + /// already clamped to in , so + /// the first failure's wait equals the clamped interval (shift 0 → 1×) and the floor is + /// honoured for free. One asymmetry worth knowing: ComputeDelay saturates at the cap + /// unconditionally, so a set SMALLER than the clamped + /// interval makes failures retry FASTER than the interval floor — the cap outranks + /// on the failure branch. Configure it above the floor + /// (every driver here uses 30 s against a 100 ms floor). /// Optional per-cycle batch callback — invoked once per poll cycle that /// produced at least one change, carrying exactly that cycle's changed items in reader /// order, after all of the cycle's per-tag callbacks. The @@ -311,6 +337,16 @@ public sealed class PollGroupEngine : IAsyncDisposable /// Record a poll that took longer than its interval: bump and /// hand the measured duration to the optional onOverrun sink. Defensive against a /// sink that throws, for the same reason is. + /// Caveat — this CONFLATES a slow success with a slow failure. It is called + /// from the finally of both poll sites, so the only input is the measured elapsed + /// time; a device that answers in 300 ms on a 200 ms interval and one that throws (or times + /// out) after 300 ms both increment the counter and both raise onOverrun with the + /// same duration. A consumer reading otopcua.driver.poll.overrun therefore learns + /// "the loop cannot keep its cadence", NOT "the device is healthy but slow" — pair it with + /// the driver's health surface (which onError already feeds) to tell the two apart. + /// Disambiguating here would mean threading the cycle's outcome into this method and + /// splitting the counter/callback by it; no consumer needs that yet, so it is deliberately + /// not done. /// private void NoteOverrun(TimeSpan interval, TimeSpan elapsed) { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs index 89b5448d..2acbb9cd 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -169,7 +169,13 @@ public sealed class ScriptedAlarmEngine : IDisposable private readonly SemaphoreSlim _evalGate = new(1, 1); private Timer? _shelvingTimer; private bool _loaded; - private bool _disposed; + + // volatile: written by Dispose and read unsynchronized from every background + // path (LoadAsync's guard, OnUpstreamChange, the drain loop, ReevaluateAsync / + // ShelvingCheckAsync's post-gate re-checks, TrackBackgroundTask's registration + // check). Without it those reads may be hoisted or cached by the JIT, so a + // pushing thread can keep queueing work against a disposed engine. + private volatile bool _disposed; // Tracks fire-and-forget background work launched by OnUpstreamChange // (DrainDirtyAsync) and RunShelvingCheck (ShelvingCheckAsync). Dispose drains @@ -556,7 +562,7 @@ public sealed class ScriptedAlarmEngine : IDisposable // Started outside the lock: DrainDirtyAsync runs synchronously up to its // first real await, and nothing it does should happen under _dirtyLock. - if (startDrain) TrackBackgroundTask(DrainDirtyAsync(CancellationToken.None)); + if (startDrain) TrackBackgroundTask(() => DrainDirtyAsync(CancellationToken.None)); } /// @@ -663,7 +669,9 @@ public sealed class ScriptedAlarmEngine : IDisposable if (!restart) _drainScheduled = false; } - if (restart) TrackBackgroundTask(Task.Run(() => DrainDirtyAsync(ct))); + // Task.Run stays INSIDE the factory: the queued-not-called handover is what + // keeps the replacement off this unwinding stack (see the Stack bound above). + if (restart) TrackBackgroundTask(() => Task.Run(() => DrainDirtyAsync(ct))); } } } @@ -986,22 +994,86 @@ public sealed class ScriptedAlarmEngine : IDisposable { if (_disposed) return; var ids = _alarms.Keys.ToArray(); - TrackBackgroundTask(ShelvingCheckAsync(ids, CancellationToken.None)); + TrackBackgroundTask(() => ShelvingCheckAsync(ids, CancellationToken.None)); } /// - /// Register a fire-and-forget task so can await it. - /// The task removes itself from the set on completion via a continuation. + /// Start a fire-and-forget task and register it so can await it. /// - private void TrackBackgroundTask(Task task) + /// + /// The factory shape is the point: registration happens BEFORE the work starts. + /// Taking a meant the caller evaluated the argument — which starts the + /// work — and only then called in to register it. snapshots + /// _inFlight under _inFlightLock and synchronously awaits the snapshot, so a + /// Dispose landing in that start→Add window drained an empty (or short) set and returned + /// while a genuinely running SaveAsync was still in flight — exactly the outliving + /// store the drain exists to prevent. The window is removed, not shrunk: a placeholder is + /// registered under the lock first, and only then is the factory invoked, so any task that + /// runs at all is already visible to every subsequent snapshot. + /// The factory is invoked OUTSIDE _inFlightLock — it runs engine code + /// synchronously up to its first real await (and, via the injected clock, can re-enter + /// and this method), none of which belongs under that lock. + /// Post-disposal the call DECLINES: the factory is never invoked and nothing is + /// registered (the method returns silently — no throw, since every caller is a + /// fire-and-forget path that already treats a disposing engine as "stop"). The drain-latch + /// handover is the only caller that would otherwise leave _drainScheduled stuck true; + /// that is harmless, because a disposed engine drops pushes in + /// and so never consults the latch again. + /// + /// Starts the background work. Invoked at most once, outside the lock. + private void TrackBackgroundTask(Func factory) { - lock (_inFlightLock) { _inFlight.Add(task); } - // Use ContinueWith with ExecuteSynchronously so the removal runs on the - // completing thread — avoids scheduler delay between completion and - // unregistration that would otherwise let Dispose see a stale set. - task.ContinueWith(t => + // RunContinuationsAsynchronously: the placeholder is completed from the real + // task's ExecuteSynchronously continuation, i.e. on the completing thread — + // this keeps an awaiter of the placeholder from being resumed inline there. + var placeholder = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + lock (_inFlightLock) { - lock (_inFlightLock) { _inFlight.Remove(t); } + if (_disposed) return; + _inFlight.Add(placeholder.Task); + } + + Task work; + try + { + // The null coalesce is defensive only: no current caller can return a null Task + // (every factory is an async lambda or returns a started Task). It costs nothing and + // keeps a future non-async factory from NREing the continuation wiring below. + work = factory() ?? Task.CompletedTask; + } + catch (Exception ex) + { + // A factory that throws synchronously started nothing, so the placeholder must + // not be left pending — a Dispose that already snapshotted it would block on it + // forever. Fault it (and observe the fault, so an un-snapshotted placeholder is + // not reported as an unobserved task exception), then let the caller see the throw + // it would have seen when the argument was evaluated at the call site. + lock (_inFlightLock) { _inFlight.Remove(placeholder.Task); } + placeholder.TrySetException(ex); + _ = placeholder.Task.Exception; + throw; + } + + // ExecuteSynchronously so the removal runs on the completing thread — avoids + // scheduler delay between completion and unregistration that would otherwise + // let Dispose see a stale set. The placeholder mirrors the real task's outcome + // so Dispose's WhenAll still observes (and logs) a background failure. + work.ContinueWith(t => + { + // Removing BEFORE completing the placeholder is safe (not a window where Dispose could + // miss still-running work): this continuation only runs once the REAL task has already + // finished, so anything a concurrent Dispose could still be waiting for is done. + lock (_inFlightLock) { _inFlight.Remove(placeholder.Task); } + if (t.IsFaulted) + { + placeholder.TrySetException(t.Exception!.InnerExceptions); + // Observe the mirror. A Dispose that snapshotted it still sees the fault + // through WhenAll (observing does not suppress propagation); one that did + // not would otherwise leave a second unobserved task exception behind. + _ = placeholder.Task.Exception; + } + else if (t.IsCanceled) placeholder.TrySetCanceled(); + else placeholder.TrySetResult(); }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); } @@ -1012,6 +1084,20 @@ public sealed class ScriptedAlarmEngine : IDisposable /// internal void RunShelvingCheckForTest() => RunShelvingCheck(); + /// + /// Test hook — launches background work through the same registration path the + /// engine's own fire-and-forget callers use, so tests can pin the + /// register-before-start ordering directly. + /// + /// Starts the background work. + internal void TrackBackgroundTaskForTest(Func factory) => TrackBackgroundTask(factory); + + /// Test hook — the number of background tasks currently registered for the Dispose drain. + internal int InFlightCountForTest + { + get { lock (_inFlightLock) { return _inFlight.Count; } } + } + private async Task ShelvingCheckAsync(IReadOnlyList alarmIds, CancellationToken ct) { var pending = new List(0); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 510c245e..931a7069 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.AbCip.PlcFamilies; @@ -161,7 +162,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, onError: HandlePollError, backoffCap: PollBackoffCap, onBatch: (handle, items) => - OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items))); + OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items)), + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.AbCip), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); _alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval, _logger); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 5767d489..1befff77 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; @@ -76,7 +77,12 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover onChange: (handle, tagRef, snapshot) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), onError: HandlePollError, - backoffCap: PollBackoffCap); + backoffCap: PollBackoffCap, + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.AbLegacy), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); } /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 8dc5b847..ea5acf3b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -1,6 +1,7 @@ using System.Collections.Concurrent; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS; @@ -121,7 +122,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, onError: HandlePollError, backoffCap: PollBackoffCap, onBatch: (handle, items) => - OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items))); + OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items)), + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.FOCAS), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); } /// True when the supplied DriverConfig JSON carries a real body. The bootstrapper always passes diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs index 0cae6ccb..a5409aba 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs @@ -1,5 +1,6 @@ using System.Globalization; using System.Runtime.InteropServices; +using System.Text; using System.Xml; using System.Xml.Linq; @@ -60,6 +61,28 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// /// /// +/// The read is forward-only. A /sample chunk is parsed once and thrown away, +/// so it is read straight off an rather than through an +/// : on the pump's hot path the tree was pure garbage — one +/// XElement plus its attributes and text nodes per observation, per chunk, per +/// second — and nothing here needs a second look at a node it has already passed. The +/// extraction is unchanged; the two rules that keep it honest are that whitespace-only text +/// nodes are dropped exactly as dropped them (see +/// ), and that an observation's slot in the result list is +/// reserved before its subtree is read so observations still arrive in document +/// order. The one place a tree is still built is the cold non-MTConnectStreams root +/// below, where the Agent's own error text has to be lifted out of a document this parser +/// is otherwise finished with. +/// +/// +/// One thing did change, and only for a document that is wrong twice over. A missing +/// <Header> used to be reported ahead of a malformed observation, because the +/// tree could be interrogated in whatever order suited; forward-only, the header is not +/// known to be missing until the root's children have run out, so the observation is +/// reported first. Both are , and nothing that is wrong in +/// only one way reports differently than it did. +/// +/// /// Failure posture. A malformed document, a non-MTConnectStreams root (an Agent /// MTConnectError served under HTTP 200 included), an unusable header, or an /// observation missing its dataItemId/timestamp all throw @@ -87,10 +110,32 @@ internal static class MTConnectStreamsParser private const string HeaderOwner = "the
"; /// - /// The three elements whose element children are observations. A ComponentStream may carry - /// any subset of them, including none. + /// How the whole document is read. Two settings are load-bearing rather than incidental. /// - private static readonly string[] ObservationContainers = ["Samples", "Events", ConditionContainer]; + /// + /// + /// reproduces + /// 's default. XDocument.Load/Parse without + /// sets exactly this, so a whitespace-only + /// text node never became an XText and never reached XElement.Value. + /// Leaving it at the reader's own default would silently start folding indentation into + /// observation values that contain element children — "1 2" where the tree read + /// "12". + /// + /// + /// A DTD is prohibited because a streams document is untrusted network input and + /// entity expansion is not a feature this parser has any use for. This too matches what + /// defaulted to. + /// + /// + private static readonly XmlReaderSettings ReaderSettings = new() + { + IgnoreWhitespace = true, + DtdProcessing = DtdProcessing.Prohibit, + + // The caller owns the stream it handed in; disposing the reader must not close it. + CloseInput = false + }; /// Parses a /current response, or one /sample chunk, held as a string. /// The raw MTConnectStreams XML document. @@ -107,19 +152,12 @@ internal static class MTConnectStreamsParser "MTConnect streams response was empty; expected an MTConnectStreams XML document."); } - XDocument document; - try - { - // A chunk lifted out of a multipart frame can carry a byte-order mark or the framing's - // trailing line break; XmlReader rejects either ahead of the XML declaration. - document = XDocument.Parse(xml.Trim('\uFEFF', ' ', '\t', '\r', '\n')); - } - catch (XmlException ex) - { - throw new InvalidDataException($"MTConnect streams response is not well-formed XML: {ex.Message}", ex); - } + // A chunk lifted out of a multipart frame can carry a byte-order mark or the framing's + // trailing line break; XmlReader rejects either ahead of the XML declaration. + using var text = new StringReader(xml.Trim('\uFEFF', ' ', '\t', '\r', '\n')); + using var reader = XmlReader.Create(text, ReaderSettings); - return Build(document); + return Read(reader); } /// @@ -128,7 +166,7 @@ internal static class MTConnectStreamsParser /// /// The /sample hot path used to materialise every chunk twice — once as a decoded /// string, once again as the trimmed copy handed to the parser — for a document that is read - /// once and thrown away. Parsing from the bytes drops both copies; + /// once and thrown away. Parsing from the bytes drops both copies; /// does its own UTF-8 decoding off the stream. The framing tolerance the string overload's /// Trim provided is preserved by , which slices rather than /// copies. @@ -156,17 +194,9 @@ internal static class MTConnectStreamsParser { ArgumentNullException.ThrowIfNull(stream); - XDocument document; - try - { - document = XDocument.Load(stream); - } - catch (XmlException ex) - { - throw new InvalidDataException($"MTConnect streams response is not well-formed XML: {ex.Message}", ex); - } + using var reader = XmlReader.Create(stream, ReaderSettings); - return Build(document); + return Read(reader); } /// @@ -218,57 +248,172 @@ internal static class MTConnectStreamsParser ? new MemoryStream(segment.Array, segment.Offset, segment.Count, writable: false) : new MemoryStream(xml.ToArray(), writable: false); - private static MTConnectStreamsResult Build(XDocument document) + /// + /// Reads the whole document, translating the reader's own well-formedness complaints into + /// this parser's failure family. A forward-only read raises them wherever it happens to + /// reach the damage rather than all at once up front, so the wrapper spans the entire walk. + /// + private static MTConnectStreamsResult Read(XmlReader reader) { - var root = document.Root - ?? throw new InvalidDataException("MTConnect streams response has no root element."); - - if (!MTConnectXml.IsNamed(root, "MTConnectStreams")) + try { - throw new InvalidDataException(MTConnectXml.DescribeUnexpectedRoot(root, "MTConnectStreams", Subject)); + return ReadDocument(reader); + } + catch (XmlException ex) + { + throw new InvalidDataException($"MTConnect streams response is not well-formed XML: {ex.Message}", ex); } - - var header = MTConnectXml.ChildrenNamed(root, "Header").FirstOrDefault() - ?? throw new InvalidDataException( - "MTConnect streams response has no
; without it the sequence bookkeeping the sample pump runs on is unknowable."); - - return new MTConnectStreamsResult( - MTConnectXml.RequiredLongAttribute(header, "instanceId", HeaderOwner, Subject), - MTConnectXml.RequiredLongAttribute(header, "nextSequence", HeaderOwner, Subject), - MTConnectXml.RequiredLongAttribute(header, "firstSequence", HeaderOwner, Subject), - ReadObservations(root)); } - /// - /// Collects every observation in the document, in Agent-supplied order. Containers are found - /// by descending the whole <Streams> subtree rather than by walking a fixed - /// DeviceStream → ComponentStream chain, so a vendor's extra nesting level cannot silently - /// hide a device's observations. - /// - private static IReadOnlyList ReadObservations(XElement root) + private static MTConnectStreamsResult ReadDocument(XmlReader reader) { - var observations = new List(); - - foreach (var container in root.Descendants().Where(IsObservationContainer)) + while (reader.Read() && reader.NodeType != XmlNodeType.Element) { - var isCondition = MTConnectXml.IsNamed(container, ConditionContainer); - foreach (var element in container.Elements()) + // Skip the XML declaration, comments and processing instructions ahead of the root. + } + + if (reader.NodeType != XmlNodeType.Element) + { + throw new InvalidDataException("MTConnect streams response has no root element."); + } + + if (!MTConnectXml.IsNamed(reader, "MTConnectStreams")) + { + // The cold path, and the one place a tree is still built: an MTConnectError arrives in + // exactly this shape (Agents serve one under HTTP 200), and its elements — the + // Agent's own account of what it objected to — are the most useful thing in the whole + // response. Materialising the root is how the shared extraction can reach them. + throw new InvalidDataException( + MTConnectXml.DescribeUnexpectedRoot((XElement)XNode.ReadFrom(reader), "MTConnectStreams", Subject)); + } + + var observations = new List(); + HeaderSequences? header = null; + + if (!reader.IsEmptyElement) + { + while (reader.Read() && reader.NodeType != XmlNodeType.EndElement) { - observations.Add(ReadObservation(element, isCondition)); + if (reader.NodeType != XmlNodeType.Element) + { + continue; + } + + // The
is a direct child of the root, and the first one wins — but it is + // still walked like any other element rather than skipped, so a container nested + // inside it yields its observations exactly as the whole-subtree descent did. + if (header is null && MTConnectXml.IsNamed(reader, "Header")) + { + header = ReadHeader(reader); + } + + ReadElement(reader, ContainerKind.None, observations, textSink: null); } } - return observations; + var sequences = header ?? throw new InvalidDataException( + "MTConnect streams response has no
; without it the sequence bookkeeping the sample pump runs on is unknowable."); + + return new MTConnectStreamsResult( + sequences.InstanceId, + sequences.NextSequence, + sequences.FirstSequence, + observations); } - private static MTConnectObservation ReadObservation(XElement element, bool isCondition) - { - var dataItemId = MTConnectXml.RequiredAttribute(element, "dataItemId", $"Observation <{element.Name.LocalName}>", Subject); + /// + /// The three sequence numbers the sample pump runs on, read off the <Header>'s + /// start tag. Read eagerly, where the header is met, so an unusable header is still reported + /// ahead of anything the <Streams> that follows it might also be wrong about. + /// + private static HeaderSequences ReadHeader(XmlReader reader) => + new( + MTConnectXml.RequiredLongAttribute(reader, "instanceId", HeaderOwner, Subject), + MTConnectXml.RequiredLongAttribute(reader, "nextSequence", HeaderOwner, Subject), + MTConnectXml.RequiredLongAttribute(reader, "firstSequence", HeaderOwner, Subject)); - return new MTConnectObservation( - dataItemId, - isCondition ? ConditionState(element) : element.Value.Trim(), - ReadTimestampUtc(element, dataItemId), + /// + /// Consumes the element the reader is positioned on, collecting any observation it is and + /// any the rest of its subtree holds. Every element is descended into — observations are + /// found by their parent being one of the three containers, not by walking a fixed + /// DeviceStream → ComponentStream chain, so a vendor's extra nesting level cannot silently + /// hide a device's observations. + /// + /// + /// Cursor contract: on return the reader sits on the element's + /// , or on the element itself when it was written empty + /// — either way the caller's next lands on whatever follows. + /// Every recursion below relies on that, which is why the children are walked by hand + /// rather than through Skip/ReadSubtree. + /// + /// A reader positioned on the element's start tag. + /// What the enclosing element is, which decides whether this one is an observation. + /// Collects the observations found, in document order. + /// + /// The text accumulator of the nearest enclosing observation whose value is its text, or + /// null when there is none. Text found anywhere below an observation belongs to it — + /// that is what XElement.Value meant. + /// + private static void ReadElement( + XmlReader reader, + ContainerKind parent, + List observations, + ValueSink? textSink) + { + var isObservation = parent != ContainerKind.None; + var isCondition = parent == ContainerKind.Condition; + var kind = ContainerKindOf(reader); + + var slot = -1; + string? dataItemId = null; + var timestampUtc = default(DateTime); + string? conditionState = null; + + if (isObservation) + { + var name = reader.LocalName; + dataItemId = MTConnectXml.RequiredAttribute(reader, "dataItemId", $"Observation <{name}>", Subject); + timestampUtc = ReadTimestampUtc(reader, dataItemId); + conditionState = isCondition ? ConditionState(name) : null; + + // The slot is reserved BEFORE the subtree is read, and filled after: an observation is + // only complete once its text and its child-element flag are known, but appending it + // then would report a nested observation ahead of the one enclosing it. The tree walk + // this replaced was document-order, and callers key snapshots off that order. + slot = observations.Count; + observations.Add(null!); + } + + // A CONDITION's value is its element NAME, so nothing below it is collected for it — but an + // enclosing observation's text still is, hence the ancestor sink rather than none at all. + var sink = isObservation && !isCondition ? new ValueSink() : textSink; + + var hasElements = !reader.IsEmptyElement && ReadChildren(reader, kind, observations, sink); + + if (!isObservation) + { + return; + } + + string value; + if (isCondition) + { + value = conditionState!; + } + else + { + var raw = sink!.Value; + + // An observation nested inside another one contributes its text to the outer one too, + // which is what concatenating every descendant text node amounted to. + textSink?.Append(raw); + value = raw.Trim(); + } + + observations[slot] = new MTConnectObservation( + dataItemId!, + value, + timestampUtc, // A DATA_SET/TABLE observation keeps its content in children, so the // text read above concatenates them into nonsense ("12" for two entries). Flagged here @@ -276,7 +421,49 @@ internal static class MTConnectStreamsParser // MTConnectObservation.IsStructured. Never true for a CONDITION: its value comes from // the element NAME, so child content cannot corrupt it, and flagging one would throw // away a perfectly well-determined state. - IsStructured: !isCondition && element.HasElements); + IsStructured: !isCondition && hasElements); + } + + /// + /// Walks a non-empty element's children through its , + /// reporting whether any of them was an element. + /// + private static bool ReadChildren( + XmlReader reader, + ContainerKind kind, + List observations, + ValueSink? sink) + { + var hasElements = false; + + while (reader.Read()) + { + switch (reader.NodeType) + { + case XmlNodeType.Element: + hasElements = true; + ReadElement(reader, kind, observations, sink); + + break; + + // Deliberately not XmlNodeType.Whitespace: ReaderSettings suppresses it, and were it + // ever surfaced it would still not belong in the value — the tree this replaced + // dropped whitespace-only nodes on exactly the same setting. + case XmlNodeType.Text: + case XmlNodeType.CDATA: + case XmlNodeType.SignificantWhitespace: + sink?.Append(reader.Value); + + break; + + case XmlNodeType.EndElement: + return hasElements; + } + } + + // Unreachable on any input the reader accepts: running out of document inside an open + // element is a well-formedness error, which the reader raises from Read() itself. + return hasElements; } /// @@ -285,16 +472,12 @@ internal static class MTConnectStreamsParser /// token to recognize; every other state is reported as-named (Normal, Warning, /// Fault, and any vendor state). /// - private static string ConditionState(XElement element) - { - var state = element.Name.LocalName; + private static string ConditionState(string localName) => + string.Equals(localName, "Unavailable", StringComparison.OrdinalIgnoreCase) ? UnavailableSentinel : localName; - return string.Equals(state, "Unavailable", StringComparison.OrdinalIgnoreCase) ? UnavailableSentinel : state; - } - - private static DateTime ReadTimestampUtc(XElement element, string dataItemId) + private static DateTime ReadTimestampUtc(XmlReader reader, string dataItemId) { - var raw = MTConnectXml.RequiredAttribute(element, "timestamp", $"Observation '{dataItemId}'", Subject); + var raw = MTConnectXml.RequiredAttribute(reader, "timestamp", $"Observation '{dataItemId}'", Subject); if (!DateTime.TryParse( raw, @@ -311,7 +494,69 @@ internal static class MTConnectStreamsParser return DateTime.SpecifyKind(parsed, DateTimeKind.Utc); } - private static bool IsObservationContainer(XElement element) => - ObservationContainers.Any(name => MTConnectXml.IsNamed(element, name)); + /// + /// Which of the three observation containers — if any — the element the reader is positioned + /// on is. A ComponentStream may carry any subset of them, including none. + /// + private static ContainerKind ContainerKindOf(XmlReader reader) + { + if (MTConnectXml.IsNamed(reader, ConditionContainer)) + { + return ContainerKind.Condition; + } + return MTConnectXml.IsNamed(reader, "Samples") || MTConnectXml.IsNamed(reader, "Events") + ? ContainerKind.Values + : ContainerKind.None; + } + + /// What an element's children are: nothing special, valued observations, or conditions. + private enum ContainerKind + { + /// Not an observation container; its element children are just more structure. + None, + + /// <Samples> or <Events> — children take their value from their text. + Values, + + /// <Condition> — children take their value from their element NAME. + Condition + } + + /// The three sequence numbers read off a <Header>. + private readonly record struct HeaderSequences(long InstanceId, long NextSequence, long FirstSequence); + + /// + /// Accumulates one observation's text across however many nodes the parser hands it, without + /// a for the single-text-node shape every ordinary scalar + /// observation has — which is the shape this parser sees thousands of times a second. + /// + private sealed class ValueSink + { + private string? _single; + private StringBuilder? _builder; + + /// Everything appended so far, untrimmed — XElement.Value's concatenation. + public string Value => _builder?.ToString() ?? _single ?? string.Empty; + + public void Append(string text) + { + if (_builder is not null) + { + _builder.Append(text); + + return; + } + + if (_single is null) + { + _single = text; + + return; + } + + _builder = new StringBuilder(_single).Append(text); + _single = null; + } + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs index d43c4f65..a8467fd2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs @@ -1,4 +1,5 @@ using System.Globalization; +using System.Xml; using System.Xml.Linq; namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; @@ -33,6 +34,23 @@ internal static class MTConnectXml public static bool IsNamed(XElement element, string localName) => string.Equals(element.Name.LocalName, localName, StringComparison.Ordinal); + /// + /// Does the element the reader is positioned on have the given local name, whatever its + /// namespace? + /// + /// + /// reads its documents through a forward-only + /// rather than a materialised tree — one sample chunk is read once + /// and thrown away, so building an for it is pure waste. The reader + /// overloads here exist so that going forward-only did not fork the two silent rules in the + /// type-level remarks: both flavours match on the local name, read attributes unqualified, + /// and produce the same error text. + /// + /// A reader positioned on an element. + /// The unqualified name to match. + public static bool IsNamed(XmlReader reader, string localName) => + string.Equals(reader.LocalName, localName, StringComparison.Ordinal); + /// Direct element children of with the given local name. /// The element whose children to filter. /// The unqualified name to match. @@ -52,19 +70,46 @@ internal static class MTConnectXml return string.IsNullOrWhiteSpace(value) ? null : value; } + /// + /// Reads an unqualified attribute off the element the reader is positioned on, normalizing a + /// present-but-empty value to null. See for + /// why the reader overloads exist. + /// + /// + /// matches on the attribute's qualified + /// name, so a prefixed x:dataItemId does not answer to dataItemId — exactly + /// what rule 2 in the type-level remarks requires, and what + /// does for a namespace-free . + /// + /// A reader positioned on an element. + /// The unqualified attribute name. + public static string? OptionalAttribute(XmlReader reader, string name) + { + var value = reader.GetAttribute(name); + + return string.IsNullOrWhiteSpace(value) ? null : value; + } + /// Reads a required unqualified attribute, or throws naming the owner and the attribute. /// The element carrying the attribute. /// The unqualified attribute name. /// How to describe the element in the error message. /// How to describe the document in the error message (e.g. "/probe response"). /// The attribute is absent or blank. - public static string RequiredAttribute(XElement element, string name, string owner, string subject) - { - var value = OptionalAttribute(element, name); + public static string RequiredAttribute(XElement element, string name, string owner, string subject) => + OptionalAttribute(element, name) ?? throw MissingAttribute(name, owner, subject); - return value ?? throw new InvalidDataException( - $"MTConnect {subject} is malformed: {owner} has no '{name}' attribute."); - } + /// + /// Reads a required unqualified attribute off the element the reader is positioned on, or + /// throws naming the owner and the attribute. + /// + /// A reader positioned on an element. + /// The unqualified attribute name. + /// How to describe the element in the error message. + /// How to describe the document in the error message. + /// The attribute is absent or blank. + public static string RequiredAttribute(XmlReader reader, string name, string owner, string subject) => + OptionalAttribute(reader, name) ?? throw MissingAttribute(name, owner, subject); /// Reads an optional unqualified attribute as an . /// The element carrying the attribute. @@ -95,10 +140,28 @@ internal static class MTConnectXml /// How to describe the element in the error message. /// How to describe the document in the error message. /// The attribute is absent, blank, or not an integer. - public static long RequiredLongAttribute(XElement element, string name, string owner, string subject) - { - var raw = RequiredAttribute(element, name, owner, subject); + public static long RequiredLongAttribute(XElement element, string name, string owner, string subject) => + AsLong(RequiredAttribute(element, name, owner, subject), name, owner, subject); + /// + /// Reads a required unqualified attribute off the element the reader is positioned on, as a + /// . + /// + /// A reader positioned on an element. + /// The unqualified attribute name. + /// How to describe the element in the error message. + /// How to describe the document in the error message. + /// The attribute is absent, blank, or not an integer. + public static long RequiredLongAttribute(XmlReader reader, string name, string owner, string subject) => + AsLong(RequiredAttribute(reader, name, owner, subject), name, owner, subject); + + /// The one spelling of "that attribute is missing", shared by both flavours. + private static InvalidDataException MissingAttribute(string name, string owner, string subject) => + new($"MTConnect {subject} is malformed: {owner} has no '{name}' attribute."); + + /// The one spelling of "that attribute is not a number", shared by both flavours. + private static long AsLong(string raw, string name, string owner, string subject) + { if (!long.TryParse(raw, NumberStyles.Integer, CultureInfo.InvariantCulture, out var value)) { throw new InvalidDataException( diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 0d34449c..e50d433a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -3,6 +3,7 @@ using System.Collections.Concurrent; using System.Text.Json; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -187,7 +188,12 @@ public sealed class ModbusDriver { if (_cycleBatchItems.TryRemove(handle, out var published) && published.Count > 0) OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, published)); - }); + }, + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.Modbus), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); } /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs index 4b6912e8..a5771e2e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs @@ -88,10 +88,11 @@ public sealed class S7DriverOptions ///
/// /// - /// Any value <= -1 disables span grouping entirely — every tag is read - /// on its own, exactly as the driver behaved before grouping existed. That is the - /// escape hatch for a CPU that dislikes wide block reads, and the control arm the - /// parity tests read through. + /// -1 disables span grouping entirely — every tag is read on its own, + /// exactly as the driver behaved before grouping existed. That is the escape hatch for a + /// CPU that dislikes wide block reads, and the control arm the parity tests read through. + /// -1 is the floor: S7DriverFactoryExtensions.ParseOptions rejects anything + /// below it, so a config cannot express "disabled" by accident with an arbitrary negative. /// /// /// Grouping cannot make a read fail where the per-tag reads would have succeeded: a @@ -121,6 +122,13 @@ public sealed class S7DriverOptions /// one enormous request that is mostly padding. The 220-byte default sits just under the /// usable payload of the common 240-byte S7-300/400 PDU, so a typical span is one request /// on the wire. + /// + /// There is deliberately no upper bound: the negotiated PDU size is opaque inside + /// S7.Net, so a value above it cannot be detected — it stays legal and simply degrades to + /// multi-request chunking inside S7.Net, costing extra wire round-trips per span rather + /// than failing. S7DriverFactoryExtensions.ParseOptions therefore validates only the + /// lower bound (>= 1); a non-positive cap can never admit a single block read. + /// /// [Display(Name = "Span read max span (bytes)", Description = "Largest single planned block read. Bounds wasted-gap amplification, not correctness.", diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs index ca367d95..9e01f6f0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs @@ -1,4 +1,5 @@ using S7.Net; +using S7DataItem = global::S7.Net.Types.DataItem; using S7NetDataType = global::S7.Net.DataType; namespace ZB.MOM.WW.OtOpcUa.Driver.S7; @@ -43,6 +44,22 @@ public interface IS7Plc : IDisposable /// The raw byte block. Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken); + /// + /// Reads several individually-addressed items in ONE PDU round-trip — the multi-var request + /// the span planner's leftovers batch onto instead of issuing an address read apiece. + /// + /// + /// ⚠️ The CALLER owns chunking. S7.Net does NOT split a multi-var request: both the item + /// count and the total payload are bounded by the negotiated PDU size, and an oversized request + /// fails on the wire rather than being paged. Split the item list to PDU capacity before calling. + /// The library populates each in place and hands the same list + /// back; the returned list is the authoritative result and is ordered as supplied. + /// + /// The items to read; each carries its area / DB / offset / var-type and receives its value. + /// Cancellation token for the read. + /// The read items, in the supplied order, with their values populated. + Task> ReadMultipleVarsAsync(List items, CancellationToken cancellationToken); + /// Writes a scalar value by S7 address string; the value must be the unsigned wire type for the address size. /// The S7 address string. /// The wire-typed value to write. @@ -132,6 +149,17 @@ internal sealed class S7PlcAdapter : IS7Plc t => _plc.ReadBytesAsync(area, db, startByteAdr, count, t), _operationTimeout, $"block read of {area} db{db} @{startByteAdr}+{count}", cancellationToken); + /// + public async Task> ReadMultipleVarsAsync(List items, CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(items); + // S7.Net returns the SAME list with each item's Value populated; await + widen because + // Task> is not assignable to Task>. + return await S7OperationDeadline.RunAsync( + t => _plc.ReadMultipleVarsAsync(items, t), + _operationTimeout, $"multi-var read of {items.Count} item(s)", cancellationToken).ConfigureAwait(false); + } + /// public Task WriteAsync(string address, object value, CancellationToken cancellationToken) => S7OperationDeadline.RunAsync( diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index c93606ed..6dd2a123 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -3,6 +3,8 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using S7.Net; using S7NetDataType = global::S7.Net.DataType; +using S7DataItem = global::S7.Net.Types.DataItem; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.S7; @@ -69,7 +71,12 @@ public sealed class S7Driver minInterval: TimeSpan.FromMilliseconds(100), onError: HandlePollError, backoffCap: TimeSpan.FromSeconds(30), - onBatch: (handle, items) => OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items))); + onBatch: (handle, items) => OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items)), + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.S7), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); } // ---- ISubscribable + IHostConnectivityProbe state ---- @@ -508,11 +515,19 @@ public sealed class S7Driver /// ReadBytesAsync, its members decoded by slicing the returned block — this is /// what turns one wire request per tag into one per contiguous block. Whatever the /// planner declines (Timer/Counter, whose offsets are device numbers) plus any span not - /// worth issuing (see ) then goes through the unchanged + /// worth issuing (see ) falls to the leftover list. + /// + /// + /// Those leftovers get a SECOND grouping pass: the ones a + /// expresses cleanly — the narrow scalars, which is what a + /// demoted single-member span is — are batched into multi-var requests by + /// , which crosses (area, DB) + /// boundaries a block read cannot. Whatever remains (Timer/Counter, unparseable + /// addresses, and any chunk whose multi-var request faulted) goes through the unchanged /// per-tag path. /// /// - /// Setting SpanReadMaxGapBytes below zero skips the first pass entirely, which + /// Setting SpanReadMaxGapBytes below zero skips BOTH passes, which /// restores the pre-grouping behaviour exactly. Either way the result array is written /// by original index, so the caller sees its own ordering regardless of how the batch /// was partitioned, and both passes share so a fault @@ -611,6 +626,18 @@ public sealed class S7Driver // Sorted so a batch is served in reference order regardless of how it was partitioned — // the per-tag tail is otherwise ordered by the planner's grouping, not by the caller's. perTag.Sort(); + + // Second grouping pass: the leftovers a DataItem can express ride ONE multi-var request + // per chunk instead of a wire request each. Deliberately gated on the SAME switch as the + // span pass — SpanReadMaxGapBytes < 0 is documented to restore the pre-grouping wire + // shape exactly, and batching the (then complete) per-tag list would break that. What + // comes back is the tags still owed an individual read, still in reference order. + if (_options.SpanReadMaxGapBytes >= 0) + { + perTag = await ReadLeftoversByMultiVarAsync( + plc, resolved, perTag, results, now, cancellationToken).ConfigureAwait(false); + } + foreach (var i in perTag) { try @@ -641,6 +668,205 @@ public sealed class S7Driver return results; } + /// + /// Items one multi-var request may carry. The caller owns chunking — S7.Net does NOT + /// split a multi-var read, it throws when the request or the projected response exceeds + /// the negotiated PDU (AssertPduSizeForRead). + /// + /// + /// The request costs a 19-byte header plus 12 bytes per item; the response costs a + /// 14-byte header plus 4 bytes of item header plus the item's own data. Against the common + /// 240-byte PDU that permits 18 items request-side and — for the 4-byte-or-narrower scalars + /// this pass batches — 28 response-side. 12 sits well inside both, leaving headroom for a + /// CPU that negotiates a smaller PDU than 240, which is the failure this constant exists to + /// avoid: an oversized request is a hard throw, not a paged read. It is deliberately NOT a + /// config key — the safe value is a property of the protocol, not of a deployment. + /// + private const int MultiVarReadMaxItems = 12; + + /// + /// Fewest batchable leftovers worth issuing a multi-var request for. ONE leftover is one + /// wire request either way, so batching it would change the observable wire shape of every + /// single-tag read for zero benefit — the same reasoning + /// applies to a lone narrow span. + /// + /// + /// The gate is on the PASS, not on each chunk: a batch of 13 still splits into 12 + 1, and + /// that trailing single is left in the multi-var path. It costs one wire request either way, + /// so re-routing it would buy nothing and would make the chunk loop's index bookkeeping + /// answer a second question. + /// + private const int MultiVarReadMinItems = 2; + + /// + /// Serves the span planner's leftovers that a can express by + /// batching them into multi-var requests, and returns the indexes still owed an individual + /// — in ascending reference order. + /// + /// + /// + /// Only a narrow scalar is batched. A carrying the same + /// area / DB / offset / bit / VarType that S7.Net's own address parser derives + /// from the tag's address string, with Count == 1, is decoded by the SAME + /// ParseBytes call the per-address read path uses — so its boxed value is + /// byte-for-byte and type-for-type what would have produced, + /// and applies to it unchanged. + /// + /// + /// Timer and Counter deliberately stay on the per-tag path even though S7.Net's + /// VarType names them. Their per-tag path does not go through ParseBytes + /// at all — it block-reads the 2-byte word and decodes it in + /// , which surfaces a Counter as int where + /// S7.Net's own Counter.FromByteArray returns ushort, and which carries a + /// documented BCD follow-up for legacy C-area counters. Batching them would fork that + /// decode into a second site that must be kept in step by hand. Correctness beats the + /// round-trip. Arrays, wide/buffer scalars and unparseable addresses are likewise left + /// alone — the first two already read a block apiece, and the last has no address to + /// build an item from. + /// + /// + /// A multi-var fault is never a tag's fault. The whole chunk falls back to the + /// per-tag path, which is what decides the tag's status — batching must not be able to + /// stamp a tag Bad that reading it one at a time would have served, so + /// is deliberately NOT called for the request itself. + /// S7.Net offers no per-item error surface to be finer-grained with: a + /// carries only its Value, and the response parser + /// throws on the FIRST item the CPU refused (before populating any later item), so + /// per-chunk fallback is the finest granularity the library actually admits. Per-ITEM + /// decoding, by contrast, is guarded individually — exactly as the span path guards its + /// members — because a mis-authored tag must not spoil the neighbours it shared a + /// request with, and it fails identically on the per-tag path anyway. + /// + /// + /// The live connection (caller holds ). + /// The batch's resolved tags, indexed by reference position. + /// Indexes the span pass left for the per-tag path, ascending. + /// The batch's result array, written in place at each tag's original index. + /// The batch timestamp, shared by every tag in the batch. + /// Cancellation token for the reads. + /// The indexes still owed an individual read, ascending. + private async Task> ReadLeftoversByMultiVarAsync( + IS7Plc plc, + S7TagDefinition?[] resolved, + List leftovers, + DataValueSnapshot[] results, + DateTime now, + CancellationToken ct) + { + var stillPerTag = new List(); + List<(int Index, S7TagDefinition Tag, S7ParsedAddress Addr)>? batchable = null; + + foreach (var i in leftovers) + { + var tag = resolved[i]!; + if (tag.ArrayCount is >= 1 + || !TryGetParsedAddress(tag, out var addr) + || IsBufferType(tag, addr)) + { + stillPerTag.Add(i); + continue; + } + (batchable ??= []).Add((i, tag, addr)); + } + + // Nothing to gain: leave the (at most one) batchable tag exactly where it was. + if (batchable is null || batchable.Count < MultiVarReadMinItems) + { + if (batchable is not null) + foreach (var (index, _, _) in batchable) stillPerTag.Add(index); + stillPerTag.Sort(); + return stillPerTag; + } + + for (var start = 0; start < batchable.Count; start += MultiVarReadMaxItems) + { + var chunk = batchable.GetRange(start, Math.Min(MultiVarReadMaxItems, batchable.Count - start)); + + IReadOnlyList answered; + try + { + var items = new List(chunk.Count); + foreach (var (_, _, addr) in chunk) items.Add(ToDataItem(addr)); + + answered = await plc.ReadMultipleVarsAsync(items, ct).ConfigureAwait(false); + + // The contract is "the same items back, in the supplied order". Anything else is + // unmappable to the batch's indexes, so treat it as a failed chunk rather than + // guessing which tag got which value. + if (answered is null || answered.Count != chunk.Count) + { + throw new System.IO.InvalidDataException( + $"S7.Net returned {answered?.Count.ToString() ?? "no"} multi-var item(s) " + + $"for a {chunk.Count}-item request"); + } + } + catch (OperationCanceledException) + { + // Same reasoning as the per-tag and span paths: a cancelled read abandons a half-read + // response on the single gated stream, so the handle must be reopened (STAB-15a). + _plcDead = true; + throw; + } + catch (Exception ex) + { + _logger.LogDebug(ex, + "S7 driver {DriverInstanceId}: multi-var read of {ItemCount} leftover tag(s) failed; " + + "falling back to per-tag reads for that chunk", _driverInstanceId, chunk.Count); + foreach (var (index, _, _) in chunk) stillPerTag.Add(index); + continue; + } + + for (var k = 0; k < chunk.Count; k++) + { + var (index, tag, addr) = chunk[k]; + try + { + var raw = answered[k].Value + ?? throw new System.IO.InvalidDataException( + $"S7.Net returned null multi-var value for '{tag.Address}'"); + results[index] = new DataValueSnapshot(ReinterpretRawValue(tag, addr, raw), 0u, now, now); + _health = new DriverHealth(DriverState.Healthy, now, null); + } + catch (Exception ex) + { + // Decoding is pure and synchronous — no cancellation can surface here, so there + // is deliberately no OperationCanceledException arm to shadow the request one. + results[index] = new DataValueSnapshot(null, RecordReadFailure(ex), null, now); + } + } + } + + // The fallback chunks were appended out of order behind the non-batchable tags. + stillPerTag.Sort(); + return stillPerTag; + } + + /// + /// Builds the multi-var request item for one narrow scalar — the exact + /// (DataType, DB, StartByteAdr, BitAdr, VarType, Count) tuple S7.Net's own address + /// parser derives from the equivalent address string, so the item reads and decodes + /// identically to 's per-address read. + /// + /// The tag's parsed address; its picks the var type. + /// The request item, with no value yet. + private static S7DataItem ToDataItem(S7ParsedAddress addr) => new() + { + DataType = ToS7NetArea(addr.Area), + DB = addr.DbNumber, + StartByteAdr = addr.ByteOffset, + // Non-Bit sizes parse to a bit offset of 0, which S7.Net ignores for those var types. + BitAdr = (byte)addr.BitOffset, + VarType = addr.Size switch + { + S7Size.Bit => VarType.Bit, + S7Size.Byte => VarType.Byte, + S7Size.Word => VarType.Word, + S7Size.DWord => VarType.DWord, + _ => throw new NotSupportedException($"S7 size {addr.Size} has no multi-var VarType mapping"), + }, + Count = 1, + }; + /// /// True when a planned span is better served by the per-tag path: it holds exactly ONE /// narrow scalar, so issuing it as a block read saves no round-trip and buys nothing. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs index 9548caf9..36d754c0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs @@ -56,6 +56,26 @@ public static class S7DriverFactoryExtensions throw new InvalidOperationException( $"S7 driver config for '{driverInstanceId}' missing required Host"); + // Block-read grouping knobs. There is no IValidateOptions machinery for S7, so this parse is + // the single choke point every deployed config passes through — validate here or a nonsensical + // value is accepted at deploy time and only misbehaves later, inside the read loop. + var spanReadMaxGapBytes = dto.SpanReadMaxGapBytes ?? S7DriverOptions.DefaultSpanReadMaxGapBytes; + var spanReadMaxSpanBytes = dto.SpanReadMaxSpanBytes ?? S7DriverOptions.DefaultSpanReadMaxSpanBytes; + + if (spanReadMaxSpanBytes < 1) + throw new InvalidOperationException( + $"S7 driver config for '{driverInstanceId}' has SpanReadMaxSpanBytes {spanReadMaxSpanBytes}; " + + "must be >= 1 — it caps a single planned block read, so a non-positive cap admits none. " + + "There is no upper bound: a value above the negotiated PDU is legal and degrades to " + + "multi-request chunking inside S7.Net."); + + // -1 is the documented escape hatch (span grouping disabled — S7Driver enables spans on >= 0) + // and 0 means "tolerate no gap", i.e. merge only strictly adjacent tags. Both stay legal. + if (spanReadMaxGapBytes < -1) + throw new InvalidOperationException( + $"S7 driver config for '{driverInstanceId}' has SpanReadMaxGapBytes {spanReadMaxGapBytes}; " + + "must be >= -1 — -1 disables span grouping and 0 merges only strictly adjacent tags."); + return new S7DriverOptions { Host = dto.Host!, @@ -69,11 +89,11 @@ public static class S7DriverFactoryExtensions // tags. The driver maps each RawTagEntry's TagConfig blob through S7TagDefinitionFactory // at Initialize; the legacy pre-declared DTO tag path (Name/Address → BuildTag) is retired. RawTags = dto.RawTags is { Count: > 0 } ? [.. dto.RawTags] : [], - // Block-read grouping knobs. Mapped explicitly like every other key on this DTO — the - // options object is never deserialised directly — so omitting them here would leave the - // documented "-1 disables grouping" escape hatch unreachable from a deployed config. - SpanReadMaxGapBytes = dto.SpanReadMaxGapBytes ?? S7DriverOptions.DefaultSpanReadMaxGapBytes, - SpanReadMaxSpanBytes = dto.SpanReadMaxSpanBytes ?? S7DriverOptions.DefaultSpanReadMaxSpanBytes, + // Mapped explicitly like every other key on this DTO — the options object is never + // deserialised directly — so omitting them here would leave the documented + // "-1 disables grouping" escape hatch unreachable from a deployed config. + SpanReadMaxGapBytes = spanReadMaxGapBytes, + SpanReadMaxSpanBytes = spanReadMaxSpanBytes, Probe = new S7ProbeOptions { Enabled = dto.Probe?.Enabled ?? true, @@ -128,7 +148,8 @@ public static class S7DriverFactoryExtensions /// Gets the probe configuration. public S7ProbeDto? Probe { get; init; } /// Gets the largest hole the read planner reads through when merging tags into one - /// block request; any value below zero disables block-read grouping. Null keeps the default. + /// block request; -1 disables block-read grouping and anything below it is rejected. + /// Null keeps the default. public int? SpanReadMaxGapBytes { get; init; } /// Gets the largest single planned block read in bytes. Null keeps the default. public int? SpanReadMaxSpanBytes { get; init; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/ZB.MOM.WW.OtOpcUa.Driver.S7.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/ZB.MOM.WW.OtOpcUa.Driver.S7.csproj index 239b1450..67a0e51c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/ZB.MOM.WW.OtOpcUa.Driver.S7.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/ZB.MOM.WW.OtOpcUa.Driver.S7.csproj @@ -24,6 +24,11 @@ + + diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs index 0c7b7631..402ba588 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -2,6 +2,7 @@ using System.Data.Common; using System.Diagnostics.CodeAnalysis; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; @@ -228,7 +229,12 @@ public sealed class SqlDriver onChange: (handle, tagRef, snapshot) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), onError: HandlePollError, - backoffCap: PollBackoffCap); + backoffCap: PollBackoffCap, + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.Sql), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs index 5e1d75c7..d79a5522 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -1,6 +1,7 @@ using System.Collections.Concurrent; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; @@ -78,7 +79,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery onChange: (handle, tagRef, snapshot) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), onError: HandlePollError, - backoffCap: PollBackoffCap); + backoffCap: PollBackoffCap, + // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this + // closure IS the production consumer of its overrun signal. + onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.TwinCAT), + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); } /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs index 2c0f4c29..72e711f7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs @@ -1448,6 +1448,9 @@ public sealed class RawTreeService(IDbContextFactory dbF // ScriptId, not Name: Name carries no unique index, and collapsing two same-named scripts into one // would change the count this warning reports. var matched = new Dictionary(StringComparer.Ordinal); + // Sequential batches: a rename at the 50k-distinct-path scale costs ~250 round-trips at 200 terms + // each. Deliberate — a rename is an interactive one-off, and bounded memory + a translatable + // predicate beat one giant OR-chain the provider would refuse (or plan pathologically). for (var offset = 0; offset < distinctPaths.Length; offset += ScriptScanBatchSize) { var batch = distinctPaths[offset..Math.Min(offset + ScriptScanBatchSize, distinctPaths.Length)]; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 14c61341..35db4d74 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -3298,9 +3298,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// identifier spaces, so an identifier alone does not name a node. /// /// - /// Anything the key cannot be built from — no role-carrying identity, no principal, a non-string - /// node identifier — falls through to the un-memoized call and keeps its existing verdict. Those - /// are the cases the authorizer denies anyway; memoizing them would only make a deny cheaper. + /// Anything the key cannot be built from — no role-carrying identity, no principal, a node + /// identifier that is not a non-empty string — falls through to the un-memoized call and keeps + /// its existing verdict. Those are the cases the authorizer denies anyway; memoizing them would + /// only make a deny cheaper. The empty identifier is excluded for the same reason + /// excludes it from its own decision cache: it + /// addresses no ACL scope, so it is not an identity to key on. /// /// /// This memo does cover the key expiry check that the authorizer's own cache excludes, but only @@ -3318,7 +3321,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 if (identity is null || (identity.ApiKeyId ?? identity.Principal) is not { Length: > 0 } sessionKey - || nodeId.Identifier is not string identifier) + || nodeId.Identifier is not string { Length: > 0 } identifier) { return _authorizer!.IsAllowedCached( identity, OpcUaOperation.CreateMonitoredItems, nodeId, realm); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs index f9e1fbc4..fa190a03 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -56,8 +56,12 @@ public sealed class SessionAclDecisionCache /// Enforced against an counter rather than /// , which takes every lock in the table — a /// price the store path would pay on every miss to answer a question that only matters at the - /// ceiling. The counter is approximate under concurrency by design, and is reset by every clear so - /// drift cannot ratchet permanently. + /// ceiling. The counter lags — an insert is in its shard's dictionary a moment before it is booked + /// — but it does not drift: every booked entry is unbooked exactly once, when the shard + /// holding it is sealed. (Not every entry is booked: one whose loses + /// the compare-exchange to a seal is never booked at all, because it is already unreachable in a + /// displaced shard.) See for how that survives an eviction racing an + /// insert. /// public const int MaxEntries = 100_000; @@ -77,6 +81,18 @@ public sealed class SessionAclDecisionCache /// public const int MaxEntriesPerSession = 20_000; + /// + /// Sentinel written over a shard's the instant it stops being the + /// live shard for its key. It is the whole of the eviction race fix: it is set by the same + /// that captures the count being subtracted from + /// , so an insert either books its increment before that capture + /// (and is therefore included in the subtraction) or sees the sentinel and books nothing at all. + /// There is no third outcome, which is what makes the counter exact rather than approximate. + /// Negative so it can never be mistaken for a real count, and far enough from zero that the + /// increment path cannot walk it back to a positive value. + /// + private const int SealedShard = int.MinValue; + private readonly ConcurrentDictionary _shards = new(StringComparer.Ordinal); private long _generation = -1; private int _globalCount; @@ -99,6 +115,29 @@ public sealed class SessionAclDecisionCache } } + /// + /// The running counter both ceilings are enforced against. Exposed for the concurrency audit only: + /// the property a test wants is almost always , and the single interesting fact + /// about this one is that the two agree. + /// + internal int TrackedCount => Volatile.Read(ref _globalCount); + + /// + /// Sum of the live shards' own counters — the middle term of the invariant + /// TrackedCount == LiveShardCount == Count. Reading the three separately is what tells a + /// failing audit whether a shard's counter drifted from its dictionary or the global counter + /// drifted from the shards. + /// + internal int LiveShardCount + { + get + { + var total = 0; + foreach (var shard in _shards.Values) total += Volatile.Read(ref shard.ApproxCount); + return total; + } + } + /// /// Looks a decision up without allocating anything — the hot half, and the reason the store half is /// a separate method rather than a parameter. @@ -172,24 +211,96 @@ public sealed class SessionAclDecisionCache { // The outer bound: no single session is over its own ceiling, so there is nothing narrower to // drop. Re-acquire the shard — ClearAll drops the shard objects themselves. + // + // Deliberately NOT elected: concurrent callers crossing the ceiling together all run ClearAll, + // and that is safe rather than merely tolerated. ClearAll displaces each shard with a per-shard + // TryRemove, so exactly one thread wins each shard and seals it exactly once; the losers just + // re-enumerate an emptying dictionary. The redundancy costs an enumeration, not correctness, + // and a CAS-elected clear would add hot-path state for a branch that is a rare fallback. ClearAll(); shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); } else if (Volatile.Read(ref shard.ApproxCount) >= MaxEntriesPerSession) { - shard.Entries.Clear(); - Interlocked.Add(ref _globalCount, -Interlocked.Exchange(ref shard.ApproxCount, 0)); + // The inner bound. This REPLACES the shard object rather than emptying the dictionary in + // place — the same move ClearAll makes, and for the same reason. Emptying in place left the + // clear and the counter reset as two steps over storage a racing Set was still inserting + // into: the racer could land in the just-cleared dictionary and increment both counters + // between them, and the Exchange then subtracted an amount that included the racer's fresh + // entry, under-counting _globalCount for good. Swapping the object makes the displaced + // shard unreachable in one atomic step, after which Seal settles its count exactly. + if (_shards.TryUpdate(sessionKey, new Shard(), shard)) + Interlocked.Add(ref _globalCount, -Seal(shard)); + + // Whoever won the swap, the live shard is now a different object than the one held above. + shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); } // TryAdd, not an indexer assignment: only a genuine insert may move the counters, or a racing pair // of walks over the same key would inflate them and pull the ceilings in for good. if (shard.Entries.TryAdd(new ShardKey(operation, realm, nodeIdentifier), decision)) + TryBookInsert(shard); + } + + /// + /// Books a just-completed insert against the shard's own counter and the global one, unless the + /// shard was displaced first — in which case the entry lives in storage nothing can reach any more + /// and must not be counted. + /// + /// + /// A plain here would race the seal in the other + /// direction: an increment landing after the displacing thread captured the count would leave + /// carrying an entry no live shard holds. The compare-exchange is what + /// linearises the two — either it wins while the shard is still live (and the seal that follows + /// captures and subtracts this increment along with the rest), or it observes + /// and books nothing. The loop retries only against other inserts + /// on the same shard; the sealed case exits. + /// + /// The shard's counter is moved before the global one, and that order matters: a seal + /// landing between the two subtracts an increment the global counter has not received yet, so + /// can dip transiently below the live total. That is self-healing — + /// the thread in the gap always completes its increment — and it errs towards not + /// clearing, which is the harmless direction. The reverse order cannot self-heal: it would book + /// globally before the seal could see the entry, leaving a permanent over-count. + /// + /// + /// The shard the entry was added to. + private void TryBookInsert(Shard shard) + { + while (true) { - Interlocked.Increment(ref shard.ApproxCount); + var seen = Volatile.Read(ref shard.ApproxCount); + + // Displaced between the TryAdd and here: the entry is lost with the shard, and counting it + // would be exactly the over-count this method exists to prevent. + if (seen < 0) return; + + if (Interlocked.CompareExchange(ref shard.ApproxCount, seen + 1, seen) != seen) continue; + Interlocked.Increment(ref _globalCount); + return; } } + /// + /// Retires a shard that has just been removed from (or replaced in) and + /// returns the number of entries it was still accounted for, for the caller to subtract from + /// . + /// + /// + /// Only the thread that actually displaced the shard may call this — ' + /// compare-and-swap removal/replacement elects exactly one, so no shard is ever sealed twice. The + /// already-sealed branch is defence in depth: it returns zero rather than subtracting the + /// sentinel, which would drive the global counter deeply negative and disable both ceilings. + /// + /// The displaced shard. + /// The count to subtract from . + private static int Seal(Shard shard) + { + var captured = Interlocked.Exchange(ref shard.ApproxCount, SealedShard); + return captured < 0 ? 0 : captured; + } + /// /// Returns the memoized decision for this key, computing and storing it when absent or when the /// generation has moved on. A convenience over + . @@ -239,24 +350,43 @@ public sealed class SessionAclDecisionCache } /// - /// Drops every shard and resets the global counter. Dropping the shard objects rather than emptying + /// Drops every shard and settles the global counter. Dropping the shard objects rather than emptying /// them also sheds the shards of sessions that have since closed. A walk racing this holds a shard /// that is no longer in the table; its entry is simply lost, which is the same outcome the clear /// itself produces. /// + /// + /// Each shard is removed and then ed, and only its sealed count is subtracted — + /// rather than the whole counter being slammed to zero. Zeroing is wrong in the other direction + /// from the eviction race: a walk that mints a fresh shard while this is running is a live + /// entry, and resetting the counter after it books its increment would lose it. Subtracting exactly + /// what is being dropped leaves the counter equal to what remains, whatever arrives mid-clear. + /// private void ClearAll() { - _shards.Clear(); - Interlocked.Exchange(ref _globalCount, 0); + foreach (var key in _shards.Keys) + if (_shards.TryRemove(key, out var shard)) + Interlocked.Add(ref _globalCount, -Seal(shard)); } - /// One session's decisions, plus the approximate entry count its own ceiling is enforced on. + /// + /// One session's decisions, plus the entry count its own ceiling is enforced on. A shard is + /// immutable in identity: it is never emptied, only replaced, so a reference obtained from + /// either still is the live shard or has been sealed — it can never turn + /// into a different generation of the same session's entries under its holder. + /// private sealed class Shard { public readonly ConcurrentDictionary Entries = new(); - /// Approximate by design — read and written with , never derived - /// from on the store path. + /// + /// Read and written with , never derived from + /// on the store path. The name is + /// historical: while the shard is live this tracks exactly (entries are + /// only ever added, and every add books here), and once displaced it holds + /// . It is lagging — an insert is visible in + /// a moment before it is booked here — but never wrong. + /// public int ApproxCount; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index 9c5417c5..e95ca092 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -2093,6 +2093,38 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS try { using var db = _dbFactory.CreateDbContext(); + + // Bound this read EXPLICITLY. It runs off the mailbox on a pool thread with no + // CancellationToken (BeginDirectApply's Task.Run), so the actor itself cannot be wedged by a + // hung central-SQL read — but the read would pin a pool thread for as long as SQL takes to + // answer. Until now the only bound was SqlClient's IMPLICIT 30 s command-timeout default: + // nothing on the ConfigDb path sets one (AddPooledDbContextFactory uses a plain + // UseSqlServer(connectionString), and no connection string carries "Command Timeout="), so a + // future connection-string edit or provider-default change could silently unbound it. Stating + // it here pins the bound to this call site. + // + // Deliberately LOCAL to this lease, not on the pooled factory's options: re-timing the global + // factory would re-time every ConfigDb consumer — migrations and long admin queries included. + // Verified against EF Core 10.0.7 (pool size 1, same context instance re-leased): + // Database.SetCommandTimeout does NOT survive the return to the pool — RelationalConnection + // resets it as an IResettableService — so the next lease sees the default again and there is + // no pool pollution to undo. + // + // Deliberately NOT a CancellationToken: threading one would mean making the FirstOrDefault + // below async, and the only caller-side token is actor lifetime — the timeout gives the same + // bound without touching the actor seam. A timeout surfaces as an exception through this + // method's existing catch-to-null, so the apply FAILS and the revision does not advance, + // keeping last-known-good served (the #485-family "no answer, never tear down" semantics — + // already correct, unchanged here). + // + // Guarded on IsRelational: the timeout is a relational-only concept and the extension throws + // on a non-relational provider (the in-memory provider the actor tests run against). + // ⚠️ Consequently the relational branch itself is UNTESTED in-suite — every actor test runs + // InMemory, so the suite only ever exercises the guard's false side. The 30 s bound is pinned + // by the EF 10.0.7 verification recorded above, not by a test. + if (db.Database.IsRelational()) + db.Database.SetCommandTimeout(TimeSpan.FromSeconds(30)); + return db.Deployments.AsNoTracking() .Where(d => d.DeploymentId == deploymentId.Value) .Select(d => d.ArtifactBlob) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs index e66d7a8d..ecd77f80 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs @@ -704,10 +704,24 @@ public sealed class ContinuousHistorizationRecorder : ReceiveActor, IWithTimers { ok = await _writer.WriteLiveValuesAsync(g.Key, g.Values, ct).ConfigureAwait(false); } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Shutdown cut this write short. That is a property of the PASS, not of the tag: + // swallowed as `ok = false` it becomes a Gitea #538 consecutive failure, and when a + // sibling group already wrote this pass the fold books it — so a routine node stop + // can tip a perfectly HEALTHY tag over its attempt budget and discard its queued + // samples on the way out. Rethrow so it propagates through Task.WhenAll to the + // method's outer cancellation catch, which abandons the pass with no ack and no + // counters touched; the entries stay durable and drain next startup. + throw; + } catch { // The writer is non-throwing by contract; guard defensively so one tag's fault // cannot fail the whole drain task (and, now, cannot fault the WhenAll either). + // A cancellation-SHAPED exception raised while our token is still live (a gateway's + // own internal deadline, say) lands here deliberately: it is an ordinary write + // failure, and exempting it would create a tag #538 can never quarantine. ok = false; } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs index 547d1b29..fdd7ed59 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs @@ -670,6 +670,50 @@ public sealed class PollGroupEngineTests overruns.Count.ShouldBeLessThanOrEqualTo(1); } + /// + /// The counter and the callback must be the SAME signal, not two approximations of it: every + /// increment of is matched by exactly one + /// onOverrun invocation. This is what lets a driver's meter (the production consumer, + /// OtOpcUaTelemetry.PollOverruns) be read as the counter itself — a callback that + /// fired on only some increments, or twice on one, would silently mis-scale the metric. + /// + /// + /// The two existing pacing tests + /// ( / + /// ) assert each side against its own loose + /// threshold, so they would both still pass if the callback fired on, say, every other + /// increment. This one compares the two directly, and does so only AFTER + /// UnsubscribeAsync has drained the loop — while a poll is in flight the increment + /// and the callback are two separate statements, so a live read could legitimately catch + /// them one apart and the equality would be flaky rather than falsifiable. + /// + [Fact] + public async Task Every_overrun_increment_is_matched_by_exactly_one_onOverrun_callback() + { + async Task> Reader(IReadOnlyList refs, CancellationToken ct) + { + await Task.Delay(120, ct); + var now = DateTime.UtcNow; + // A fresh value every poll, so the change path runs on every cycle too. + return refs.Select(_ => new DataValueSnapshot(now.Ticks, 0u, now, now)).ToList(); + } + + var overruns = new ConcurrentQueue(); + await using var engine = new PollGroupEngine( + Reader, + (_, _, _) => { }, + minInterval: TimeSpan.FromMilliseconds(20), + onOverrun: e => overruns.Enqueue(e)); + + var handle = engine.Subscribe(["X"], TimeSpan.FromMilliseconds(40)); + await Task.Delay(1000, TestContext.Current.CancellationToken); + await engine.UnsubscribeAsync(handle); + + // Sanity: the scenario actually produced overruns, so the equality below is not vacuous. + engine.OverrunCount.ShouldBeGreaterThanOrEqualTo(3); + overruns.Count.ShouldBe((int)engine.OverrunCount); + } + /// /// The async teardown must be behaviourally identical to the sync one: the handle is known, /// the loop stops, and no further callback fires. Mirrors diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs index e7a7d571..a8ca67b2 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs @@ -1659,6 +1659,134 @@ public sealed class ScriptedAlarmEngineTests await WaitForAsync(() => eng.GetState("A")!.Active == AlarmActiveState.Active, timeoutMs: 5000); } + // ------------------------------------------------------------------------- + // TrackBackgroundTask: fire-and-forget work must be REGISTERED BEFORE IT STARTS. + // + // The old signature took a Task, so the caller evaluated the argument — starting + // the work — and only then called in to register it. Dispose snapshots the + // in-flight set under its lock and synchronously awaits the snapshot, so a Dispose + // landing in that start→Add window drained a set that did not yet contain a + // genuinely running task, and returned while its SaveAsync was still in flight. + // The factory form removes the window rather than shrinking it. + // ------------------------------------------------------------------------- + + /// Verifies the background task is registered for the Dispose drain BEFORE the factory + /// that starts it is invoked, so no snapshot can ever miss a task that runs. + [Fact] + public void Background_work_is_registered_before_the_factory_starts_it() + { + var up = new FakeUpstream(); + using var eng = Build(up, out _); + + var countSeenInsideFactory = -1; + var gate = new TaskCompletionSource(); + eng.TrackBackgroundTaskForTest(() => + { + // Sampled from inside the factory's own synchronous body — i.e. before the + // work's Task even exists, which is precisely the window the old shape left open. + countSeenInsideFactory = eng.InFlightCountForTest; + return gate.Task; + }); + + // Release BEFORE asserting: on the falsification run (registration moved after the + // factory) the assertion throws, and `using` would then Dispose into a drain that + // waits on a gate this method never reached — a hang instead of a failure. + gate.SetResult(); + + countSeenInsideFactory.ShouldBe(1, + "registration must happen before the factory starts the work, or Dispose can drain " + + "past a task that is genuinely in flight"); + } + + /// Verifies that after Dispose the factory form declines: no factory is invoked and + /// nothing is registered, so a disposed engine can never acquire new background work. + [Fact] + public void Disposed_engine_declines_to_start_new_background_work() + { + var up = new FakeUpstream(); + var eng = Build(up, out _); + eng.Dispose(); + + var invoked = false; + eng.TrackBackgroundTaskForTest(() => { invoked = true; return Task.CompletedTask; }); + + invoked.ShouldBeFalse("a disposed engine must not start new background work"); + eng.InFlightCountForTest.ShouldBe(0, "and must not register anything for a drain that already ran"); + } + + /// Verifies Dispose blocks on work started through the factory form, and that the + /// registration is released once that work completes. + [Fact] + public async Task Dispose_awaits_background_work_started_through_the_factory() + { + var up = new FakeUpstream(); + var eng = Build(up, out _); + + var gate = new TaskCompletionSource(); + eng.TrackBackgroundTaskForTest(() => gate.Task); + eng.InFlightCountForTest.ShouldBe(1); + + var disposeTask = Task.Run(() => eng.Dispose()); + var raced = await Task.WhenAny(disposeTask, Task.Delay(300)); + raced.ShouldNotBe(disposeTask, "Dispose must not return while registered background work is still running"); + + gate.SetResult(); + await disposeTask.WaitAsync(TimeSpan.FromSeconds(5)); + eng.InFlightCountForTest.ShouldBe(0, "completed work must unregister itself"); + } + + /// Verifies work whose task is already complete when the factory returns still + /// unregisters cleanly and leaves Dispose with nothing to wait on. + [Fact] + public void Already_completed_background_work_unregisters_and_leaves_Dispose_free() + { + var up = new FakeUpstream(); + var eng = Build(up, out _); + + eng.TrackBackgroundTaskForTest(() => Task.CompletedTask); + + eng.InFlightCountForTest.ShouldBe(0, + "the self-removal continuation runs inline on an already-completed task"); + Should.NotThrow(() => eng.Dispose()); + } + + /// Verifies a factory that throws synchronously surfaces the throw to its caller (the + /// behaviour the old evaluate-at-the-call-site shape had) without wedging Dispose on a + /// placeholder that nothing will ever complete. + [Fact] + public async Task A_factory_that_throws_synchronously_rethrows_and_does_not_wedge_Dispose() + { + var up = new FakeUpstream(); + var eng = Build(up, out _); + + Should.Throw(() => eng.TrackBackgroundTaskForTest( + () => throw new InvalidOperationException("factory failed (test)"))); + + eng.InFlightCountForTest.ShouldBe(0, "work that never started must not be left registered"); + await Task.Run(() => eng.Dispose()).WaitAsync(TimeSpan.FromSeconds(5)); + } + + /// Verifies a faulted background task is still drained by Dispose and its failure is + /// swallowed there (logged at debug), matching the pre-existing shutdown contract. + [Fact] + public async Task A_faulted_background_task_is_drained_by_Dispose_without_throwing() + { + var up = new FakeUpstream(); + var eng = Build(up, out _); + + var gate = new TaskCompletionSource(); + eng.TrackBackgroundTaskForTest(() => gate.Task); + + // Fault it while Dispose is actually blocked on the drain, so the WhenAll observes + // the failure rather than finding an already-unregistered task. + var disposeTask = Task.Run(() => Should.NotThrow(() => eng.Dispose())); + await Task.Delay(200); + gate.SetException(new InvalidOperationException("background failed (test)")); + + await disposeTask.WaitAsync(TimeSpan.FromSeconds(5)); + eng.InFlightCountForTest.ShouldBe(0); + } + /// /// A clock that, once armed, re-dirties an alarm by pushing to the upstream and then throws — /// driving a deterministic chain of drain-latch handovers whose every step is synchronous — diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/FasterLogHistorizationOutboxTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/FasterLogHistorizationOutboxTests.cs index ec37e82e..95e39ba6 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/FasterLogHistorizationOutboxTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/FasterLogHistorizationOutboxTests.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Linq; using FASTER.core; using Xunit; @@ -188,7 +189,10 @@ public sealed class FasterLogHistorizationOutboxTests : IDisposable { await writer.AppendBatchAsync([a, b, c], TestContext.Current.CancellationToken); - Assert.Equal(new[] { a.Id, b.Id, c.Id }, ReadCommitted(dir)); + Guid[] committed = await ReadCommittedWhenVisibleAsync( + dir, expected: 3, TestContext.Current.CancellationToken); + + Assert.Equal(new[] { a.Id, b.Id, c.Id }, committed); } finally { @@ -196,6 +200,36 @@ public sealed class FasterLogHistorizationOutboxTests : IDisposable } } + // Bounded poll over ReadCommitted for the group-commit pin above. AppendBatchAsync's commit has + // already returned by the time we look, but an INDEPENDENT FasterLog learns about it only by + // re-reading the on-disk commit metadata — and how promptly that write becomes visible to a + // second reader is a filesystem property, not a FasterLog guarantee (recorded as fragile on + // macOS, where it is not on the Windows/Linux timescale). A single-shot read is therefore + // timing-dependent and green partly by luck. Each attempt calls ReadCommitted, which builds a + // FRESH device + FasterLog and so re-reads the metadata rather than reusing a stale view — + // verified: a reader opened after a later commit does observe that commit. + // + // The bounded wait does NOT weaken the claim: the writer is never disposed during the poll (the + // only other thing that could commit), and PerEntry mode runs no periodic-commit loop — so + // anything that becomes visible inside the window was still made durable by AppendBatchAsync's + // own group commit and nothing else. A missing commit stays missing for the whole 5 s and the + // test still goes red. + // + // On timeout the last read is returned rather than thrown on, so the caller's Assert.Equal + // reports the actual shortfall (which ids were durable) instead of a bare timeout message. + private static async Task ReadCommittedWhenVisibleAsync(string dir, int expected, CancellationToken ct) + { + var elapsed = Stopwatch.StartNew(); + Guid[] ids = ReadCommitted(dir); + while (ids.Length < expected && elapsed.Elapsed < TimeSpan.FromSeconds(5)) + { + await Task.Delay(25, ct); + ids = ReadCommitted(dir); + } + + return ids; + } + // Opens an independent read-only FasterLog over the outbox's directory and returns the ids of // every record the writer has COMMITTED (BeginAddress .. CommittedUntilAddress). Uncommitted // enqueues sit past CommittedUntilAddress and are therefore invisible here — which is exactly diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectStreamsParseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectStreamsParseTests.cs index 1d5a14bd..6c039508 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectStreamsParseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectStreamsParseTests.cs @@ -1061,6 +1061,30 @@ public sealed class MTConnectStreamsParseTests observation.DataItemId.ShouldBe("ds"); } + [Fact] + public void The_indentation_between_child_entries_is_not_part_of_the_value() + { + // The parser reads forward-only off an XmlReader, and the value it reports is the + // concatenation of the text it meets on the way. That concatenation only matches what the + // XDocument it replaced reported because whitespace-only text nodes are dropped + // (XmlReaderSettings.IgnoreWhitespace, which is what XDocument.Load defaulted to) — so + // entries separated by a line break must still read "12", never "1\n 2". Nothing else in + // this file can tell the two settings apart: the DATA_SET case above writes its entries + // adjacent, and every other observation is a single text node that Trim() cleans up either + // way. The observation index codes a structured observation BadNotSupported, so the value + // itself is not what matters here — the setting is, and this is the only test that moves + // when it changes. + var result = MTConnectStreamsParser.Parse(StreamsDocument( + """ + + 1 + 2 + + """)); + + result.Observations.ShouldHaveSingleItem().Value.ShouldBe("12"); + } + [Fact] public void A_plain_text_observation_containing_spaces_is_NOT_flagged_structured() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/README.md b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/README.md index 50cf4ee5..15cb25e0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/README.md +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/README.md @@ -27,6 +27,31 @@ docker compose -f tests\...\Docker\docker-compose.yml --profile s7_1500 up -d docker compose -f tests\...\Docker\docker-compose.yml --profile s7_1500 down ``` +### ⚠️ Editing a profile requires `--build` + +`Dockerfile` **COPYs `profiles/` into the image**, so the seeds are baked in at +build time — they are not bind-mounted. A plain `up -d`, and even +`up -d --force-recreate`, reuses the existing `otopcua-python-snap7:1.0` image +and silently keeps serving the OLD seeds. After editing a profile JSON: + +```bash +docker compose --profile s7_1500 up -d --build --force-recreate +``` + +Confirm it took: `docker logs otopcua-python-snap7-s7_1500` prints +`seeded DB1 size=1024 seeds=` at startup — check `` matches the seed count +you just wrote. This is the fastest way to tell a stale image from a bad address. + +On the shared Docker host the deployment mirror at `/opt/otopcua-s7/` is +**root-owned**, so a plain `rsync` fails with `status 23` and leaves the old +files in place while still printing the file list. Push with: + +```bash +rsync -av --rsync-path="sudo rsync" \ + tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/ \ + dohertj2@10.100.0.35:/opt/otopcua-s7/ +``` + ## Endpoint - Default: `localhost:1102` (non-privileged; sidesteps Windows Firewall @@ -67,6 +92,30 @@ DB1 (1024 bytes) + MB (256 bytes) with typed seeds at known offsets: Seed types supported: `u8`, `i8`, `u16`, `i16`, `u32`, `i32`, `f32`, `bool` (with `"bit": 0..7`), `ascii` (S7 STRING). +### Span-planner gate seeds (offsets 300+, `S7_1500SpanReadTests`) + +These exist to give `S7ReadSpanPlanner` a layout with a **known** partition under +the shipped 16-byte-gap / 220-byte-span defaults — the offsets are the test, so +moving one changes what the gate proves. + +| Address | Type | Seed | Role in the partition | +|---|---|---|---| +| `DB1.DBW300` | u16 | `51001` | cluster A — merges with the four below into ONE 300..320 span | +| `DB1.DBW302` | i16 | `-21001` | cluster A (gap 0) | +| `DB1.DBD304` | i32 | `305419896` | cluster A (gap 0) | +| `DB1.DBX310.5` | bool | `true` | cluster A (gap 2) | +| `DB1.DBD316` | f32 | `2.7182817` | cluster A (gap 5); seals the span at 20 bytes | +| `DB1.DBW400` | u16 | `40201` | cluster B — 80 bytes on, so the gap budget forces a SECOND span | +| `DB1.DBW402` | u16 | `40202` | cluster B's second member (keeps it off the `PrefersPerTagRead` demotion) | +| `DB1.DBW500` | u16 | `60001` | isolated ⇒ demoted single-member span ⇒ multi-var leftover | +| `DB1.DBW600` | i16 | `-32001` | isolated ⇒ multi-var leftover | +| `MW10` | u16 | `4097` | multi-var leftover in a **different area** — a block read can't cross `(area, DB)`, multi-var can | + +Offsets `308`, `311` and `312` carry **poison** (`0xDEAD`, `0x5A`, `0xDEADBEEF`) +inside the holes cluster A's span reads through. No tag maps to them; they are +there so a mis-computed slice offset returns garbage instead of a zero that could +match by luck. Every seed above is distinct and non-zero for the same reason. + ## Known limitations From the `snap7.server.Server` docstring upstream: diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/profiles/s7_1500.json b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/profiles/s7_1500.json index 20d8955a..bca0c603 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/profiles/s7_1500.json +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/Docker/profiles/s7_1500.json @@ -19,7 +19,34 @@ { "_desc": "DB1.DBW100 — scratch for write-then-read round-trip tests; seeded 0", "offset": 100, "type": "u16", "value": 0 }, { "_desc": "DB1.STRING[200] — S7 string 'Hello' (max 32, cur 5)", - "offset": 200, "type": "ascii", "value": "Hello", "max_len": 32 } + "offset": 200, "type": "ascii", "value": "Hello", "max_len": 32 }, + + { "_desc": "DB1.DBW300 — span-planner gate (S7_1500SpanReadTests) starts here. Cluster A spans 300..320 and merges into ONE block read under the 16-byte gap budget. Every seed from 300 on is distinct and non-zero so a mis-sliced block can never accidentally match (the #536 zero-source trap).", + "offset": 300, "type": "u16", "value": 51001 }, + { "_desc": "DB1.DBW302 — cluster A, adjacent to DBW300 (gap 0)", + "offset": 302, "type": "i16", "value": -21001 }, + { "_desc": "DB1.DBD304 — cluster A, adjacent to DBW302 (gap 0)", + "offset": 304, "type": "i32", "value": 305419896 }, + { "_desc": "DB1 @308 — POISON in the 308..310 hole the span reads through; no tag maps here, so a mis-slice yields 0xDEAD rather than a zero that could pass by luck", + "offset": 308, "type": "u16", "value": 57005 }, + { "_desc": "DB1.DBX310.5 — cluster A bool (gap 2 from the DBD304 end)", + "offset": 310, "type": "bool", "value": true, "bit": 5 }, + { "_desc": "DB1 @311 — POISON in the 311..316 hole", + "offset": 311, "type": "u8", "value": 90 }, + { "_desc": "DB1 @312 — POISON in the 311..316 hole (0xDEADBEEF)", + "offset": 312, "type": "u32", "value": 3735928559 }, + { "_desc": "DB1.DBD316 — cluster A f32, gap 5 from the bool byte's end; seals the span at 300..320 = 20 bytes", + "offset": 316, "type": "f32", "value": 2.7182817 }, + + { "_desc": "DB1.DBW400 — cluster B; 80 bytes past cluster A's end, so the 16-byte gap budget forces a SECOND span rather than one merged read", + "offset": 400, "type": "u16", "value": 40201 }, + { "_desc": "DB1.DBW402 — cluster B's second member; two members keep the span off the PrefersPerTagRead demotion", + "offset": 402, "type": "u16", "value": 40202 }, + + { "_desc": "DB1.DBW500 — isolated narrow scalar: its own single-member span, demoted by PrefersPerTagRead into a leftover that rides the multi-var pass", + "offset": 500, "type": "u16", "value": 60001 }, + { "_desc": "DB1.DBW600 — second isolated narrow scalar, likewise a multi-var leftover", + "offset": 600, "type": "i16", "value": -32001 } ] }, { @@ -28,7 +55,9 @@ "size": 256, "seeds": [ { "_desc": "MW0 — probe target for S7ProbeOptions.ProbeAddress default", - "offset": 0, "type": "u16", "value": 1 } + "offset": 0, "type": "u16", "value": 1 }, + { "_desc": "MW10 — span-planner gate: a narrow scalar in a DIFFERENT area from the DB1 leftovers. A block read can never merge across (area, DB); the multi-var pass can, so this pins that the leftover batching crosses the boundary.", + "offset": 10, "type": "u16", "value": 4097 } ] } ] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanReadTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanReadTests.cs new file mode 100644 index 00000000..d3084ca3 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanReadTests.cs @@ -0,0 +1,364 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using S7DataItem = global::S7.Net.Types.DataItem; +using S7NetDataType = global::S7.Net.DataType; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for the read-batching layer: the block-read pass +/// (16-byte gap / 220-byte span defaults) and the multi-var leftover pass, both exercised against +/// the real python-snap7 S7-1500 fixture rather than a fake. The span defaults were chosen +/// analytically and the multi-var batching shipped with unit coverage only; this suite is what +/// proves they behave on real S7comm wire. +/// +/// +/// +/// Live-gate record. Run 2026-08-17 against 10.100.0.35:1102 +/// (otopcua-python-snap7-s7_1500, profile Docker/profiles/s7_1500.json): +/// PASSED — 3 tests, 0 failed, 0 skipped. Grouped batch collapsed 10 tags to 3 wire +/// requests (2 block reads + 1 three-item multi-var) and matched the ungrouped 10-request +/// batch value-for-value and type-for-type. No behavioural difference between the planner's +/// analytic model and real wire was found. +/// +/// +/// What the layout buys. The ten tags are positioned so the planner's rules produce a +/// known, non-trivial partition rather than one that would pass by accident: +/// +/// +/// +/// Cluster A (DB1.DBW300 … DB1.DBD316) — five tags whose largest hole is 5 +/// bytes, so all five merge into ONE 20-byte span under the 16-byte gap budget. The +/// holes are seeded with poison (0xDEAD, 0xDEADBEEF) so a mis-computed +/// slice offset yields garbage rather than a zero that could coincidentally match. +/// +/// +/// Cluster B (DB1.DBW400 + DB1.DBW402) — 80 bytes past cluster A, so +/// the gap budget is what forces a SECOND span instead of one merged read. Two members +/// keep it clear of the PrefersPerTagRead demotion, so it is a real block read. +/// +/// +/// Leftovers (DB1.DBW500, DB1.DBW600, MW10) — each isolated +/// enough to form its own single-member span, each therefore demoted by +/// PrefersPerTagRead, and three of them is past MultiVarReadMinItems — so +/// they ride ONE multi-var request. MW10 is deliberately in a different area: +/// a block read can never merge across (area, DB), and the multi-var pass can, +/// so its presence in the single request is what pins that property live. +/// +/// +/// +/// Timer / Counter are NOT covered here. The fixture profile declares only DB1 and +/// MK; python-snap7's server can register TM/CT areas but S7.Net's timer/counter read is a +/// distinct wire shape that the fixture has never served, so pinning the planner's per-tag +/// exclusion live would mean standing up and validating a new fixture area. That exclusion +/// stays unit-covered. Nothing else in this suite depends on it — Timer/Counter tags are +/// rejected by the planner on AREA and by the multi-var pass on IsBufferType, two +/// independent guards. +/// +/// +/// Why a counting factory rather than logs. The driver emits no log on a successful +/// grouped read, so the only honest way to assert the wire shape is to observe the calls. +/// decorates the REAL , so the +/// shipped S7PlcAdapter (and its S7OperationDeadline ceiling) stays on the +/// wire — the gate measures production's path, not a re-implementation of it. +/// +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500SpanReadTests(Snap7ServerFixture sim) +{ + /// + /// Verifies the grouped batch returns exactly what reading every tag individually returns — + /// same values, same boxed CLR types, all Good. + /// + /// + /// Boxed equality is the assertion on purpose: the three paths decode through different code + /// (block slicing, S7.Net.ParseBytes via multi-var, and the per-address read), and a + /// path that returned the right number as the wrong CLR type would be a real defect that a + /// numeric-tolerance comparison would hide. + /// + [Fact] + public async Task Grouped_batch_matches_ungrouped_reads_value_for_value() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + + var grouped = await ReadBatchAsync(SpanLayout.DefaultGapBytes, "s7-span-on"); + var ungrouped = await ReadBatchAsync(SpanLayout.GroupingDisabled, "s7-span-off"); + + grouped.Snapshots.Count.ShouldBe(SpanLayout.TagNames.Count); + ungrouped.Snapshots.Count.ShouldBe(SpanLayout.TagNames.Count); + + for (var i = 0; i < SpanLayout.TagNames.Count; i++) + { + var name = SpanLayout.TagNames[i]; + grouped.Snapshots[i].StatusCode.ShouldBe(0u, $"grouped read of '{name}' must be Good"); + ungrouped.Snapshots[i].StatusCode.ShouldBe(0u, $"ungrouped read of '{name}' must be Good"); + grouped.Snapshots[i].Value.ShouldBe( + ungrouped.Snapshots[i].Value, + $"'{name}' must decode identically whichever path served it"); + } + } + + /// + /// Verifies the grouped batch decodes the fixture's seeded values — so both paths agreeing + /// cannot mean "both wrong the same way". + /// + /// + /// Every seed is distinct and non-zero (the #536 trap: a zero-valued source proves nothing, + /// because a read that returned nothing at all would also read as zero). Distinctness matters + /// independently: two tags sharing a value would let a swapped slice offset pass. + /// + [Fact] + public async Task Grouped_batch_decodes_the_seeded_values() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + + var grouped = await ReadBatchAsync(SpanLayout.DefaultGapBytes, "s7-span-seeds"); + var byName = SpanLayout.TagNames + .Select((n, i) => (n, grouped.Snapshots[i])) + .ToDictionary(x => x.n, x => x.Item2); + + foreach (var (name, snapshot) in byName) + snapshot.StatusCode.ShouldBe(0u, $"'{name}' must be Good"); + + // Cluster A — the five members of the single merged span. + byName["SpanA_U16"].Value.ShouldBe((ushort)51001); + byName["SpanA_I16"].Value.ShouldBe((short)-21001); + byName["SpanA_I32"].Value.ShouldBe(305419896); + byName["SpanA_Bool"].Value.ShouldBe(true); + Convert.ToSingle(byName["SpanA_F32"].Value).ShouldBe(2.7182817f, tolerance: 0.000001f); + + // Cluster B — the second span. + byName["SpanB_U16a"].Value.ShouldBe((ushort)40201); + byName["SpanB_U16b"].Value.ShouldBe((ushort)40202); + + // The multi-var leftovers, including the one in a different area. + byName["Lone_U16"].Value.ShouldBe((ushort)60001); + byName["Lone_I16"].Value.ShouldBe((short)-32001); + byName["Lone_MkU16"].Value.ShouldBe((ushort)4097); + } + + /// + /// Verifies the wire shape the batching actually produces against a live PLC: ten tags served + /// by three requests with grouping on, ten with it off. + /// + /// + /// This is the half of the gate that a value-parity test cannot cover — a driver that quietly + /// fell back to per-tag reads for every tag would pass parity perfectly while delivering none + /// of the round-trip saving the feature exists for. The off-case is asserted too, because + /// SpanReadMaxGapBytes < 0 is documented to restore the pre-grouping wire shape + /// EXACTLY, and "exactly" is a claim about requests, not values. + /// + [Fact] + public async Task Grouping_collapses_ten_tags_to_three_wire_requests() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + + var grouped = await ReadBatchAsync(SpanLayout.DefaultGapBytes, "s7-span-shape-on"); + + grouped.Counters.BlockReads.ShouldBe(2, + "cluster A merges into one span and cluster B into a second; the gap budget separates them"); + grouped.Counters.MultiVarReads.ShouldBe(1, + "the three demoted single-member spans are past MultiVarReadMinItems, so they share one request"); + grouped.Counters.MultiVarItems.ShouldBe(3, + "all three leftovers ride that request — including MW10, which no block read could have joined"); + grouped.Counters.AddressReads.ShouldBe(0, + "nothing in this batch should reach the per-tag path once both grouping passes have run"); + grouped.Counters.TotalReadRequests.ShouldBe(3); + + var ungrouped = await ReadBatchAsync(SpanLayout.GroupingDisabled, "s7-span-shape-off"); + + ungrouped.Counters.AddressReads.ShouldBe(SpanLayout.TagNames.Count, + "grouping off must restore one per-address read per tag"); + ungrouped.Counters.BlockReads.ShouldBe(0); + ungrouped.Counters.MultiVarReads.ShouldBe(0, + "the multi-var pass rides the SAME switch as the span pass — disabling grouping must disable both"); + ungrouped.Counters.TotalReadRequests.ShouldBe(SpanLayout.TagNames.Count); + } + + /// Reads the whole layout once through a freshly-built driver and reports the wire shape. + /// The span budget: the shipped default, or a negative value to disable grouping. + /// Instance id, distinct per call so log lines stay attributable. + /// The batch's snapshots plus the wire-call counts observed for the read itself. + private async Task<(IReadOnlyList Snapshots, CountingPlcFactory Counters)> + ReadBatchAsync(int maxGapBytes, string driverInstanceId) + { + var counters = new CountingPlcFactory(new S7PlcFactory()); + await using var drv = new S7Driver( + SpanLayout.BuildOptions(sim.Host, sim.Port, maxGapBytes), driverInstanceId, counters); + + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + // Initialization may open the connection and probe; only the batch's own wire calls are the + // subject here, so the counters start from zero at the read. + counters.Reset(); + + var snapshots = await drv.ReadAsync(SpanLayout.TagNames, TestContext.Current.CancellationToken); + return (snapshots, counters); + } + + /// + /// The tag layout under test, positioned against the shipped 16-byte gap / 220-byte span + /// defaults so the planner produces a known partition. Addresses map 1:1 to the seed offsets + /// in Docker/profiles/s7_1500.json, so a seed drift there surfaces here as a value + /// mismatch rather than a mystery failure. + /// + private static class SpanLayout + { + /// The shipped gap budget — the value this gate exists to validate. + public const int DefaultGapBytes = S7DriverOptions.DefaultSpanReadMaxGapBytes; + + /// Any negative gap disables BOTH grouping passes; the documented off switch. + public const int GroupingDisabled = -1; + + private static readonly S7TagDefinition[] Definitions = + [ + // Cluster A: holes of 0, 0, 2 and 5 bytes — all inside the 16-byte budget, so these five + // merge into one 300..320 span (20 bytes, far under the 220-byte ceiling). + new("SpanA_U16", "DB1.DBW300", S7DataType.UInt16), + new("SpanA_I16", "DB1.DBW302", S7DataType.Int16), + new("SpanA_I32", "DB1.DBD304", S7DataType.Int32), + new("SpanA_Bool", "DB1.DBX310.5", S7DataType.Bool), + new("SpanA_F32", "DB1.DBD316", S7DataType.Float32), + + // Cluster B: 80 bytes past cluster A's end, so the budget forces a second span. Two + // members, so PrefersPerTagRead cannot demote it to the per-tag path. + new("SpanB_U16a", "DB1.DBW400", S7DataType.UInt16), + new("SpanB_U16b", "DB1.DBW402", S7DataType.UInt16), + + // Leftovers: each far enough from every neighbour to form a single-member span, hence + // demoted, hence batched by the multi-var pass. MW10 is in another area entirely. + new("Lone_U16", "DB1.DBW500", S7DataType.UInt16), + new("Lone_I16", "DB1.DBW600", S7DataType.Int16), + new("Lone_MkU16", "MW10", S7DataType.UInt16), + ]; + + /// The batch's references, in the order the driver is asked for them. + public static readonly IReadOnlyList TagNames = [.. Definitions.Select(d => d.Name)]; + + /// Builds driver options for the layout at the requested span budget. + /// The fixture hostname or IP address. + /// The fixture port. + /// Span gap budget; negative disables grouping. + /// The driver options. + public static S7DriverOptions BuildOptions(string host, int port, int maxGapBytes) => new() + { + Host = host, + Port = port, + CpuType = S7CpuType.S71500, + Rack = 0, + Slot = 0, + Timeout = TimeSpan.FromSeconds(5), + // Same reasoning as the smoke profile: a background probe would race these reads for the + // connection gate and would also pollute the wire-shape counts this suite asserts on. + Probe = new S7ProbeOptions { Enabled = false }, + SpanReadMaxGapBytes = maxGapBytes, + SpanReadMaxSpanBytes = S7DriverOptions.DefaultSpanReadMaxSpanBytes, + RawTags = S7RawTags.Entries(Definitions), + }; + } + + /// + /// decorator that counts the read calls the driver makes, so a + /// test can assert the wire shape rather than infer it. Delegates to the real factory, so + /// every counted call is the one production would have issued. + /// + /// The factory producing the real connection. + private sealed class CountingPlcFactory(IS7PlcFactory inner) : IS7PlcFactory + { + /// Block reads issued — one per planned span. + public int BlockReads { get; private set; } + + /// Multi-var requests issued — one per leftover chunk. + public int MultiVarReads { get; private set; } + + /// Items carried across all multi-var requests. + public int MultiVarItems { get; private set; } + + /// Per-address reads issued — the unchanged per-tag path. + public int AddressReads { get; private set; } + + /// Total read round-trips, whichever shape they took. + public int TotalReadRequests => BlockReads + MultiVarReads + AddressReads; + + /// Zeroes the counters, so a batch can be measured without initialization's calls. + public void Reset() + { + BlockReads = 0; + MultiVarReads = 0; + MultiVarItems = 0; + AddressReads = 0; + } + + /// + public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) + => new CountingPlc(inner.Create(cpuType, host, port, rack, slot, timeout), this); + + private void CountBlockRead() => BlockReads++; + + private void CountMultiVarRead(int items) + { + MultiVarReads++; + MultiVarItems += items; + } + + private void CountAddressRead() => AddressReads++; + + /// Counting pass-through over one real connection. + /// The real connection. + /// The factory holding the counters. + private sealed class CountingPlc(IS7Plc inner, CountingPlcFactory owner) : IS7Plc + { + /// + public bool IsConnected => inner.IsConnected; + + /// + public Task OpenAsync(CancellationToken cancellationToken) => inner.OpenAsync(cancellationToken); + + /// + public void Close() => inner.Close(); + + /// + public Task ReadAsync(string address, CancellationToken cancellationToken) + { + owner.CountAddressRead(); + return inner.ReadAsync(address, cancellationToken); + } + + /// + public Task ReadBytesAsync( + S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) + { + owner.CountBlockRead(); + return inner.ReadBytesAsync(area, db, startByteAdr, count, cancellationToken); + } + + /// + public Task> ReadMultipleVarsAsync( + List items, CancellationToken cancellationToken) + { + // Counted before the call: the item count is the request's shape, and S7.Net hands + // the same list back, so reading it afterwards would measure the same thing later. + owner.CountMultiVarRead(items?.Count ?? 0); + return inner.ReadMultipleVarsAsync(items!, cancellationToken); + } + + /// + public Task WriteAsync(string address, object value, CancellationToken cancellationToken) + => inner.WriteAsync(address, value, cancellationToken); + + /// + public Task WriteBytesAsync( + S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) + => inner.WriteBytesAsync(area, db, startByteAdr, value, cancellationToken); + + /// + public Task ReadStatusAsync(CancellationToken cancellationToken) => inner.ReadStatusAsync(cancellationToken); + + /// + public void Dispose() => inner.Dispose(); + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index 6fdabdfa..0faa1125 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -569,6 +569,10 @@ public sealed class S7DriverReconnectTests public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) => throw new NotSupportedException("FakeS7Plc: block reads not needed for the reconnect tests"); + public Task> ReadMultipleVarsAsync( + List items, CancellationToken cancellationToken) => + throw new NotSupportedException("FakeS7Plc: multi-var reads not needed for the reconnect tests"); + public async Task WriteAsync(string address, object value, CancellationToken cancellationToken) { if (WriteHangsUntilCancelled) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7PollEngineMigrationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7PollEngineMigrationTests.cs index 9da4db9d..368229d5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7PollEngineMigrationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7PollEngineMigrationTests.cs @@ -47,6 +47,10 @@ public sealed class S7PollEngineMigrationTests return Task.FromResult(block); } + public Task> ReadMultipleVarsAsync( + List items, CancellationToken ct) + => throw new NotSupportedException("ConstantArrayPlc: multi-var reads not needed for the poll-engine tests"); + public Task WriteAsync(string address, object value, CancellationToken ct) => Task.CompletedTask; public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken ct) => Task.CompletedTask; public Task ReadStatusAsync(CancellationToken ct) => Task.CompletedTask; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs new file mode 100644 index 00000000..c565cda6 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanOptionValidationTests.cs @@ -0,0 +1,96 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Config-time guard on the two span-read knobs. There is no IValidateOptions machinery +/// for S7 — is the single choke point every +/// deployed config passes through — so a nonsensical value used to be accepted at deploy time and +/// only misbehave later, in the read loop, on a driver that had already reported healthy. +/// +/// +/// The legal-value cases matter as much as the rejections: -1 on the gap is the documented +/// escape hatch that disables grouping entirely (S7Driver enables spans on >= 0), +/// and 0 means "tolerate no gap at all", i.e. merge only strictly adjacent tags. A guard +/// written as < 0 would break both. +/// +public sealed class S7SpanOptionValidationTests +{ + /// A driver-config blob carrying the given span knob, host included (it is required). + private static string Config(string knob, int value) => + "{\"host\":\"10.0.0.1\",\"" + knob + "\":" + value.ToString(System.Globalization.CultureInfo.InvariantCulture) + "}"; + + /// A span cap below one can never admit a single planned block read, so it is rejected. + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(-220)] + public void A_span_cap_below_one_is_rejected(int maxSpanBytes) + { + var ex = Should.Throw(() => S7DriverFactoryExtensions.ParseOptions( + "s7-cfg", Config("spanReadMaxSpanBytes", maxSpanBytes))); + + ex.Message.ShouldContain("SpanReadMaxSpanBytes"); + ex.Message.ShouldContain("s7-cfg"); + } + + /// A gap budget below the grouping-disabled sentinel is rejected. + [Theory] + [InlineData(-2)] + [InlineData(-16)] + public void A_gap_budget_below_minus_one_is_rejected(int maxGapBytes) + { + var ex = Should.Throw(() => S7DriverFactoryExtensions.ParseOptions( + "s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes))); + + ex.Message.ShouldContain("SpanReadMaxGapBytes"); + ex.Message.ShouldContain("s7-cfg"); + } + + /// + /// The two legal edge values survive: -1 (grouping disabled) and 0 (no gap + /// tolerated). Both are documented behaviour, not degenerate input. + /// + [Theory] + [InlineData(-1)] + [InlineData(0)] + public void The_documented_gap_edge_values_are_accepted(int maxGapBytes) + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes)); + + parsed.SpanReadMaxGapBytes.ShouldBe(maxGapBytes); + } + + /// A one-byte span cap is the smallest legal cap and must not trip the guard. + [Fact] + public void The_smallest_legal_span_cap_is_accepted() + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 1)); + + parsed.SpanReadMaxSpanBytes.ShouldBe(1); + } + + /// + /// An over-PDU span cap is legal — S7.Net transparently chunks an oversized ReadBytes + /// into several wire requests — so the guard must not invent an upper bound it cannot know + /// (the negotiated PDU size is opaque inside S7.Net). + /// + [Fact] + public void An_over_pdu_span_cap_is_accepted_because_S7Net_chunks_it() + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 4096)); + + parsed.SpanReadMaxSpanBytes.ShouldBe(4096); + } + + /// Omitting both knobs keeps the shipped defaults, which must themselves pass the guard. + [Fact] + public void The_shipped_defaults_pass_the_guard() + { + var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", "{\"host\":\"10.0.0.1\"}"); + + parsed.SpanReadMaxGapBytes.ShouldBe(S7DriverOptions.DefaultSpanReadMaxGapBytes); + parsed.SpanReadMaxSpanBytes.ShouldBe(S7DriverOptions.DefaultSpanReadMaxSpanBytes); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs index 4f8361f6..00160f68 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs @@ -74,6 +74,44 @@ public sealed class S7SpanReadParityTests new S7TagDefinition("B", "DB1.DBW2", S7DataType.UInt16, Writable: false), new S7TagDefinition("C", "DB1.DBW4", S7DataType.UInt16, Writable: false)); + /// + /// narrow words spaced 8 bytes apart. With a gap budget below 6 + /// none of them can merge, so every one becomes a single-member span the driver demotes to + /// the leftover path — which is exactly the population the multi-var batch serves. + /// + /// How many scattered words to author. + /// The raw-tag entries. + private static IReadOnlyList ScatteredWords(int count) => S7RawTags.Entries( + [.. Enumerable.Range(0, count).Select(i => + new S7TagDefinition($"S{i}", $"DB1.DBW{i * 8}", S7DataType.UInt16, Writable: false))]); + + /// References for , in authoring order. + /// How many scattered words the batch reads. + /// The reference names. + private static string[] ScatteredRefs(int count) => [.. Enumerable.Range(0, count).Select(i => $"S{i}")]; + + /// + /// One of every narrow access width, spaced far enough apart that each is its own + /// single-member span — so the whole set reaches the multi-var pass and every box type + /// (bool / byte / ushort / short / uint / int / + /// float) is exercised through it. + /// + private static IReadOnlyList ScatteredWidths() => S7RawTags.Entries( + new S7TagDefinition("WBit", "DB1.DBX0.3", S7DataType.Bool, Writable: false), + new S7TagDefinition("WByte", "DB1.DBB8", S7DataType.Byte, Writable: false), + new S7TagDefinition("WU16", "DB1.DBW16", S7DataType.UInt16, Writable: false), + new S7TagDefinition("WI16", "DB1.DBW24", S7DataType.Int16, Writable: false), + new S7TagDefinition("WU32", "DB1.DBD32", S7DataType.UInt32, Writable: false), + new S7TagDefinition("WI32", "DB1.DBD40", S7DataType.Int32, Writable: false), + new S7TagDefinition("WF32", "DB1.DBD48", S7DataType.Float32, Writable: false), + // A second AREA in the same batch — a multi-var request carries each item's own area/DB, + // which is precisely what a block read cannot do. + new S7TagDefinition("WMem", "MW0", S7DataType.UInt16, Writable: false)); + + /// References for , in authoring order. + private static readonly string[] ScatteredWidthRefs = + ["WBit", "WByte", "WU16", "WI16", "WU32", "WI32", "WF32", "WMem"]; + // ── Parity ─────────────────────────────────────────────────────────────────────────── /// @@ -120,11 +158,11 @@ public sealed class S7SpanReadParityTests /// read. /// /// - /// 14 tags collapse to 4 wire requests: ONE block read covering the eleven DB1 tags from - /// byte 0 to byte 68, the Counter's own unavoidable block read, and two per-address string - /// reads — the far DB1 word (past the gap budget) and the lone Memory word, each of which - /// the driver deliberately leaves on the per-tag path because a single-member narrow span - /// saves no round-trip. + /// 14 tags collapse to 3 wire requests: ONE block read covering the eleven DB1 tags from + /// byte 0 to byte 68, the Counter's own unavoidable block read, and ONE multi-var request + /// carrying the two tags a span can't help — the far DB1 word (past the gap budget) and the + /// lone Memory word, which is in a different AREA entirely. Neither is worth a block read of + /// its own, and batching them together is something only a multi-var request can do. /// [Fact] public async Task Grouping_issues_far_fewer_wire_requests_than_there_are_tags() @@ -136,12 +174,15 @@ public sealed class S7SpanReadParityTests _ = await driver.ReadAsync(MixedRefs, ct); - (factory.ReadBytesCalls + factory.StringReadCalls).ShouldBeLessThan(MixedRefs.Length); + (factory.ReadBytesCalls + factory.StringReadCalls + factory.MultiVarCalls) + .ShouldBeLessThan(MixedRefs.Length); factory.ReadBytesCalls.ShouldBeLessThan(MixedRefs.Length); factory.ReadBytesCalls.ShouldBe(2); - // The nine narrow tags inside the big span issue NO string reads — only the two tags that - // ended up alone in a span of their own do. - factory.StringReadCalls.ShouldBe(2); + // NO per-address reads at all: the nine narrow tags inside the big span are served by its + // block read, and the two that ended up alone in a span of their own are batched together + // into a single multi-var request. + factory.StringReadCalls.ShouldBe(0); + factory.MultiVarCalls.ShouldBe(1); } // ── Failure parity ─────────────────────────────────────────────────────────────────── @@ -205,6 +246,133 @@ public sealed class S7SpanReadParityTests results[2].StatusCode.ShouldBe(0u); } + // ── Multi-var leftovers ────────────────────────────────────────────────────────────── + + /// + /// The batchable leftovers ride ONE multi-var request per chunk of + /// MultiVarReadMaxItems and issue no per-address reads at all. 13 scattered words + /// that the span planner cannot merge used to cost 13 wire requests; they now cost 2. + /// + [Fact] + public async Task Batchable_leftovers_ride_multi_var_requests_instead_of_one_read_each() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ImageS7PlcFactory(Images()); + using var driver = new S7Driver(Options(ScatteredWords(13), maxGap: 2), "s7-multivar", factory); + await driver.InitializeAsync("{}", ct); + + var results = await driver.ReadAsync(ScatteredRefs(13), ct); + + results.ShouldAllBe(s => s.StatusCode == 0u); + results.ShouldAllBe(s => s.Value != null); + // ⌈13 / 12⌉ = 2 requests, and NOT ONE per-address read — the whole point of the batch. + factory.MultiVarCalls.ShouldBe(2); + factory.StringReadCalls.ShouldBe(0); + factory.ReadBytesCalls.ShouldBe(0); + // The chunking is a real split, not one oversized request plus an empty one. + factory.MultiVarItems.Select(c => c.Count).ShouldBe([12, 1]); + } + + /// + /// THE multi-var gate: a tag served by a multi-var request decodes to the same value, the + /// same CLR box type and the same status as the per-tag path reading the same image. + /// + [Fact] + public async Task Multi_var_leftovers_match_the_per_tag_path_snapshot_for_snapshot() + { + var images = Images(); + var ct = TestContext.Current.CancellationToken; + + var batching = new ImageS7PlcFactory(images); + var perTag = new ImageS7PlcFactory(images); + + using var batchingDriver = new S7Driver(Options(ScatteredWidths(), maxGap: 2), "s7-mv-parity", batching); + using var perTagDriver = new S7Driver(Options(ScatteredWidths(), maxGap: -1), "s7-mv-control", perTag); + await batchingDriver.InitializeAsync("{}", ct); + await perTagDriver.InitializeAsync("{}", ct); + + var batched = await batchingDriver.ReadAsync(ScatteredWidthRefs, ct); + var individual = await perTagDriver.ReadAsync(ScatteredWidthRefs, ct); + + for (var i = 0; i < ScatteredWidthRefs.Length; i++) + { + var name = ScatteredWidthRefs[i]; + batched[i].StatusCode.ShouldBe(0u, $"'{name}' should have read Good"); + batched[i].StatusCode.ShouldBe(individual[i].StatusCode, $"status differs for '{name}'"); + batched[i].Value.ShouldNotBeNull($"'{name}' returned no value"); + batched[i].Value!.GetType().ShouldBe(individual[i].Value!.GetType(), $"box type differs for '{name}'"); + batched[i].Value.ShouldBe(individual[i].Value, $"value differs for '{name}'"); + } + + // Proves the batching side really took the multi-var path (otherwise this file's parity + // claim would be vacuous), and the control side really did not. + batching.MultiVarCalls.ShouldBe(1); + batching.StringReadCalls.ShouldBe(0); + perTag.MultiVarCalls.ShouldBe(0); + perTag.StringReadCalls.ShouldBe(ScatteredWidthRefs.Length); + } + + /// + /// A multi-var request that faults falls back to the per-tag path for that chunk's tags and + /// still serves them Good. Batching is an optimisation: it must never be able to turn a tag + /// Bad that reading it one at a time would have served. + /// + [Fact] + public async Task A_failing_multi_var_chunk_falls_back_to_per_tag_reads_and_still_serves_good() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ImageS7PlcFactory(Images()) + { + // A device-layer fault — the shape that WOULD stamp BadDeviceFailure had it come from + // the per-tag path. Through the multi-var pass it must cost nothing but a retry. + MultiVarFault = new PlcException(ErrorCode.ReadData, "S7 CPU rejected the multi-var request"), + }; + using var driver = new S7Driver(Options(ScatteredWords(5), maxGap: 2), "s7-mv-fallback", factory); + await driver.InitializeAsync("{}", ct); + + var results = await driver.ReadAsync(ScatteredRefs(5), ct); + + results.ShouldAllBe(s => s.StatusCode == 0u); + results.ShouldAllBe(s => s.Value != null); + factory.MultiVarCalls.ShouldBe(1); + // Every tag of the failed chunk was re-read individually. + factory.StringReadCalls.ShouldBe(5); + // And the failed batch attempt did NOT degrade the driver — nothing actually failed to read. + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// Tags the per-tag path owns — Timer and Counter, whose offsets are device numbers rather + /// than byte addresses — are never handed to a multi-var request, even though they share the + /// leftover list with the narrow scalars that are. + /// + [Fact] + public async Task Timer_and_counter_leftovers_never_enter_a_multi_var_request() + { + var ct = TestContext.Current.CancellationToken; + var tags = S7RawTags.Entries( + new S7TagDefinition("Tmr", "T2", S7DataType.Float64, Writable: false), + new S7TagDefinition("Cnt", "C1", S7DataType.Int32, Writable: false), + new S7TagDefinition("N0", "DB1.DBW0", S7DataType.UInt16, Writable: false), + new S7TagDefinition("N1", "DB1.DBW100", S7DataType.UInt16, Writable: false)); + var factory = new ImageS7PlcFactory(Images()); + using var driver = new S7Driver(Options(tags, maxGap: 2), "s7-mv-partition", factory); + await driver.InitializeAsync("{}", ct); + + var results = await driver.ReadAsync(["Tmr", "Cnt", "N0", "N1"], ct); + + results.ShouldAllBe(s => s.StatusCode == 0u); + results[0].Value.ShouldBeOfType(); + results[1].Value.ShouldBeOfType(); + + // One multi-var request carrying ONLY the two narrow words; the Timer and the Counter kept + // their own block reads. + factory.MultiVarCalls.ShouldBe(1); + factory.MultiVarItems[0].Count.ShouldBe(2); + factory.MultiVarItems[0].ShouldAllBe(d => d.DataType == S7NetDataType.DataBlock); + factory.ReadBytesCalls.ShouldBe(2); + } + // ── Configurability ────────────────────────────────────────────────────────────────── /// @@ -265,11 +433,15 @@ public sealed class S7SpanReadParityTests var counters = new byte[64]; for (var i = 0; i < counters.Length; i++) counters[i] = (byte)((i * 29 + 3) & 0xFF); + var timers = new byte[64]; + for (var i = 0; i < timers.Length; i++) timers[i] = (byte)((i * 17 + 5) & 0xFF); + return new Dictionary<(S7NetDataType, int), byte[]> { [(S7NetDataType.DataBlock, 1)] = db1, [(S7NetDataType.Memory, 0)] = mem, [(S7NetDataType.Counter, 0)] = counters, + [(S7NetDataType.Timer, 0)] = timers, }; } @@ -289,9 +461,18 @@ public sealed class S7SpanReadParityTests /// Number of per-address string reads issued — the un-grouped narrow path. public int StringReadCalls; + /// Number of multi-var requests issued — one per leftover chunk. + public int MultiVarCalls; + + /// The items of every multi-var request, in issue order, so a test can assert what was batched. + public List> MultiVarItems { get; } = new(); + /// When set, every read throws this instead of serving the image. public Exception? ReadFault { get; init; } + /// When set, only the MULTI-VAR read throws this — the per-tag fallback still serves the image. + public Exception? MultiVarFault { get; init; } + /// public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) => new ImageS7Plc(this); @@ -323,6 +504,42 @@ public sealed class S7SpanReadParityTests return Task.FromResult(image[startByteAdr..(startByteAdr + count)]); } + /// + /// + /// Decodes each item from the same byte image the other two read shapes serve, boxing + /// exactly as S7.Net's ParseBytes does for Count == 1: Bit a + /// bool at BitAdr, Byte a byte, Word a + /// ushort, DWord a uint. Like the real library it populates each + /// item's Value in place and hands the same list back. + /// + public Task> ReadMultipleVarsAsync( + List items, CancellationToken ct) + { + // Counted (and captured) BEFORE the injected fault so a fallback test can assert the + // batch was ATTEMPTED — counting only successes would read zero. + Interlocked.Increment(ref owner.MultiVarCalls); + lock (owner.MultiVarItems) owner.MultiVarItems.Add([.. items]); + if (owner.MultiVarFault is { } mvFault) throw mvFault; + if (owner.ReadFault is { } fault) throw fault; + + foreach (var item in items) + { + var image = owner.Images[(item.DataType, item.DB)]; + var at = item.StartByteAdr; + item.Value = item.VarType switch + { + VarType.Bit => (object)((image[at] & (1 << item.BitAdr)) != 0), + VarType.Byte => image[at], + VarType.Word => (ushort)((image[at] << 8) | image[at + 1]), + VarType.DWord => ((uint)image[at] << 24) | ((uint)image[at + 1] << 16) + | ((uint)image[at + 2] << 8) | image[at + 3], + _ => throw new NotSupportedException($"fake cannot serve VarType {item.VarType}"), + }; + } + + return Task.FromResult>(items); + } + /// public Task ReadAsync(string address, CancellationToken ct) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TestBuilders.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TestBuilders.cs index cb857905..6483fa5c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TestBuilders.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TestBuilders.cs @@ -92,6 +92,10 @@ internal sealed class ConnectingFakeS7PlcFactory : IS7PlcFactory public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken ct) => throw new NotSupportedException("ConnectingFakeS7Plc serves no data"); /// Not exercised by these tests. + public Task> ReadMultipleVarsAsync( + List items, CancellationToken ct) + => throw new NotSupportedException("ConnectingFakeS7Plc serves no data"); + /// Not exercised by these tests. public Task WriteAsync(string address, object value, CancellationToken ct) => throw new NotSupportedException("ConnectingFakeS7Plc serves no data"); /// Not exercised by these tests. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs new file mode 100644 index 00000000..26a8b7bb --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AdminUiSqlServerFixture.cs @@ -0,0 +1,216 @@ +using System.Data.Common; +using Microsoft.Data.SqlClient; +using Microsoft.EntityFrameworkCore; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests; + +/// +/// A real SQL Server database for the handful of AdminUI tests whose subject is the SQL Server +/// provider's LINQ translation rather than the C# around it — EF InMemory evaluates those +/// queries in-process, so it can neither fail on an untranslatable shape nor reproduce SQL Server's +/// collation semantics. Creates a uniquely-named database, applies the EF migrations to it, and drops +/// it at collection teardown. +/// +/// +/// +/// Shaped after ZB.MOM.WW.OtOpcUa.Configuration.Tests.SchemaComplianceFixture (same env +/// vars, same unique-name + Migrate() + drop-via-master lifecycle) with one deliberate +/// deviation: that fixture lets the connection failure surface as a constructor throw, so +/// Configuration.Tests fails hard when SQL Server is unreachable. AdminUI.Tests is an ordinary +/// unit suite that must stay green offline, so this fixture probes reachability FIRST and records +/// a instead of throwing. Tests carry , +/// which skips them when the probe failed. +/// +/// +/// The skip is an attribute rather than the Assert.Skip(fixture.SkipReason) call the S7 +/// integration suite uses, because AdminUI.Tests is one of the three projects pinned to +/// xunit v2 by Akka.TestKit.Xunit2 (see Directory.Packages.props) and +/// Assert.Skip is xunit v3 only. Overriding FactAttribute.Skip is the v2 equivalent +/// and reports a genuine "skipped", not a vacuous pass. Both the attribute and this fixture read +/// the same one-shot probe in , so a session probes once. +/// +/// +/// The probe being one-shot bounds what "stays green offline" guarantees: it covers SQL +/// being unreachable before the run. A connectivity blip during a run is never +/// re-probed, so the affected tests surface as SqlException failures, not skips. +/// +/// +/// Every test in the collection shares ONE database (migrating is the expensive part), so tests +/// must isolate themselves by seeding uniquely-named rows, or by clearing the table they measure. +/// xunit serialises a collection's tests, so a clear-then-seed is safe. +/// +/// +public sealed class AdminUiSqlServerFixture : IDisposable +{ + /// Gets the name of the per-run test database, or when SQL is unreachable. + public string? DatabaseName { get; } + + /// Gets the connection string for the test database, or when SQL is unreachable. + public string? ConnectionString { get; } + + /// + /// Gets the reason the SQL-backed tests cannot run (server unreachable, migration failed), or + /// when the database is ready. + /// + public string? SkipReason { get; } + + /// + /// Gets a context factory pointed at the migrated test database — the seam the services under test + /// (DeploymentDriftProbe, RawTreeService) take. Throws when + /// is set; guard with . + /// + public IDbContextFactory Factory => + _factory ?? throw new InvalidOperationException(SkipReason ?? "SQL Server fixture is not available."); + + private readonly SqlServerContextFactory? _factory; + + /// Initializes a new instance of the class. + public AdminUiSqlServerFixture() + { + // Probe FIRST — see the remarks. A dead server must leave this constructor without throwing, + // because xunit builds collection fixtures before it discovers that every test is skipped. + SkipReason = AdminUiSqlServer.SkipReason; + if (SkipReason is not null) return; + + DatabaseName = $"OtOpcUaAdminUi_Test_{DateTime.UtcNow:yyyyMMddHHmmss}_{Guid.NewGuid():N}"; + ConnectionString = AdminUiSqlServer.ConnectionStringFor(DatabaseName); + + try + { + var options = new DbContextOptionsBuilder() + .UseSqlServer(ConnectionString) + .Options; + + using var ctx = new OtOpcUaConfigDbContext(options); + ctx.Database.Migrate(); + } + catch (Exception ex) + { + // A half-created database would leak, so tear it down before standing the suite down. + TryDropDatabase(); + SkipReason = $"Could not migrate the AdminUI SQL test database: {ex.Message}"; + DatabaseName = null; + ConnectionString = null; + return; + } + + _factory = new SqlServerContextFactory(ConnectionString); + } + + /// Disposes the fixture, dropping the test database. + public void Dispose() => TryDropDatabase(); + + private void TryDropDatabase() + { + if (ConnectionString is null || DatabaseName is null) return; + + try + { + var master = new SqlConnectionStringBuilder(ConnectionString) { InitialCatalog = "master" }.ConnectionString; + + using var conn = new SqlConnection(master); + conn.Open(); + + using var cmd = conn.CreateCommand(); + cmd.CommandText = $@" +IF DB_ID(N'{DatabaseName}') IS NOT NULL +BEGIN + ALTER DATABASE [{DatabaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + DROP DATABASE [{DatabaseName}]; +END"; + cmd.ExecuteNonQuery(); + } + catch (DbException) + { + // Teardown is best-effort: a dropped connection here must not fail an otherwise-green run. + // The database name carries a timestamp, so a leaked one is identifiable. + } + } + + private sealed class SqlServerContextFactory(string connectionString) + : IDbContextFactory + { + public OtOpcUaConfigDbContext CreateDbContext() => + new(new DbContextOptionsBuilder() + .UseSqlServer(connectionString) + .Options); + + public Task CreateDbContextAsync(CancellationToken cancellationToken = default) => + Task.FromResult(CreateDbContext()); + } +} + +/// +/// One-shot reachability probe + connection-string builder shared by +/// and . The attribute is +/// constructed at discovery time and the fixture at collection start, so the probe must be cached or +/// a run pays the connect timeout twice. +/// +internal static class AdminUiSqlServer +{ + private const string DefaultServer = "10.100.0.35,14330"; + private const string DefaultSaPassword = "OtOpcUaDev_2026!"; + private const int ProbeTimeoutSeconds = 3; + + private static readonly Lazy Probe = new(RunProbe, LazyThreadSafetyMode.ExecutionAndPublication); + + /// Gets the reason SQL Server is unusable, or when it answered. + public static string? SkipReason => Probe.Value; + + /// Builds a connection string for the named database on the configured test server. + /// The database to connect to. + /// The connection string. + public static string ConnectionStringFor(string database) + { + var server = Environment.GetEnvironmentVariable("OTOPCUA_CONFIG_TEST_SERVER") ?? DefaultServer; + var password = Environment.GetEnvironmentVariable("OTOPCUA_CONFIG_TEST_SA_PASSWORD") ?? DefaultSaPassword; + + return $"Server={server};Database={database};User Id=sa;Password={password};" + + "TrustServerCertificate=True;Encrypt=False;"; + } + + private static string? RunProbe() + { + var server = Environment.GetEnvironmentVariable("OTOPCUA_CONFIG_TEST_SERVER") ?? DefaultServer; + + try + { + var probe = new SqlConnectionStringBuilder(ConnectionStringFor("master")) + { + ConnectTimeout = ProbeTimeoutSeconds, + }.ConnectionString; + + using var conn = new SqlConnection(probe); + conn.Open(); + return null; + } + catch (Exception ex) + { + return $"SQL Server at '{server}' is unreachable ({ex.GetType().Name}: {ex.Message}); " + + "set OTOPCUA_CONFIG_TEST_SERVER / OTOPCUA_CONFIG_TEST_SA_PASSWORD to point at a reachable instance."; + } + } +} + +/// +/// A that skips itself when the shared test SQL Server is unreachable. +/// The xunit v2 equivalent of xunit v3's Assert.Skip — see +/// 's remarks for why this project cannot use the latter. +/// +[AttributeUsage(AttributeTargets.Method)] +public sealed class SqlServerFactAttribute : FactAttribute +{ + /// Initializes a new instance of the class. + public SqlServerFactAttribute() + { + if (AdminUiSqlServer.SkipReason is { } reason) Skip = reason; + } +} + +/// Collection definition binding the SQL-backed AdminUI tests to one shared database. +[CollectionDefinition(nameof(AdminUiSqlServerCollection))] +public sealed class AdminUiSqlServerCollection : ICollectionFixture +{ +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs new file mode 100644 index 00000000..50646175 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs @@ -0,0 +1,193 @@ +using Microsoft.EntityFrameworkCore; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Deployments; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Deployments; + +/// +/// The SQL-Server-backed half of : pins that +/// DeploymentDriftProbe's watermark aggregate — ConfigEdits.GroupBy(_ => 1).Select(g => +/// new { g.Max(...), g.Count() }) — actually translates, and translates to the right +/// values, on the real provider. +/// +/// +/// +/// EF InMemory evaluates that query with LINQ-to-Objects, so the InMemory suite proves the C# +/// and nothing about the SQL. Here an untranslatable shape throws and fails the test outright, +/// and a mistranslation is caught by moving exactly one half of the aggregate at a time: +/// +/// +/// a later-timestamped edit moves Max only (its Count also moves, so this +/// one alone is not decisive) — +/// an earlier-timestamped edit leaves Max where it was and moves +/// Count alone, which is the only thing that can invalidate the cache; a translation +/// that dropped the Count projection would serve the stale verdict and fail here. +/// +/// +/// The watermark itself is private, so the assertions read it through its one observable +/// consequence: whether the probe recomposes. That is deliberate — asserting a copy of the query +/// would pin a copy, not the production one. +/// +/// +/// The table is cleared at the start of every test: the collection shares one database and the +/// watermark is a whole-table aggregate. +/// +/// +[Collection(nameof(AdminUiSqlServerCollection))] +public sealed class DeploymentDriftProbeSqlServerTests(AdminUiSqlServerFixture sql) +{ + private const string SealedHash = "sealed-hash-aaaa"; + + /// A fixed instant so the "earlier edit" case is unambiguously earlier, clock skew aside. + private static readonly DateTime Anchor = new(2026, 8, 17, 12, 0, 0, DateTimeKind.Utc); + + [SqlServerFact] + public async Task An_empty_ConfigEdits_table_yields_a_verdict_and_caches_it() + { + // The no-rows shape is the one GroupBy(_ => 1) makes subtle: SQL Server produces NO group at all, + // so FirstOrDefaultAsync must land on null rather than on a row of (NULL, 0). + await ClearConfigEditsAsync(); + var compose = new CountingCompose(SealedHash); + var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); + + var first = await probe.GetDriftAsync(SealedHash, CancellationToken.None); + var second = await probe.GetDriftAsync(SealedHash, CancellationToken.None); + + first.ShouldBe(false); + second.ShouldBe(false); + compose.Count.ShouldBe(1); + } + + [SqlServerFact] + public async Task An_unchanged_ConfigEdits_table_reuses_the_cached_verdict() + { + await ClearConfigEditsAsync(); + await AddEditsAsync(Anchor, Anchor.AddSeconds(1)); + var compose = new CountingCompose(SealedHash); + var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); + + await probe.GetDriftAsync(SealedHash, CancellationToken.None); + var second = await probe.GetDriftAsync(SealedHash, CancellationToken.None); + + // Two reads of the same rows must produce an EQUAL watermark. Not a tautology against SQL: + // EditedAtUtc round-trips through datetime2 and an unstable projection would recompose forever. + second.ShouldBe(false); + compose.Count.ShouldBe(1); + } + + [SqlServerFact] + public async Task A_later_edit_moves_the_watermark_and_recomposes() + { + await ClearConfigEditsAsync(); + await AddEditsAsync(Anchor); + var compose = new CountingCompose(SealedHash); + var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); + + await AddEditsAsync(Anchor.AddHours(1)); + compose.Result = "a-different-hash"; + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); + compose.Count.ShouldBe(2); + } + + [SqlServerFact] + public async Task An_earlier_edit_moves_only_the_count_and_still_recomposes() + { + // The decisive case for the Count half: MAX(EditedAtUtc) is unchanged, so a translation that lost + // the COUNT(*) projection would report the same watermark and serve the stale (false) verdict. + await ClearConfigEditsAsync(); + await AddEditsAsync(Anchor); + var compose = new CountingCompose(SealedHash); + var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); + + await AddEditsAsync(Anchor.AddHours(-1)); + compose.Result = "a-different-hash"; + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); + compose.Count.ShouldBe(2); + } + + [SqlServerFact] + public async Task A_same_timestamp_edit_moves_only_the_count_and_still_recomposes() + { + // Two mutations inside one tick — the case the CLAUDE.md/probe remarks call out explicitly. + await ClearConfigEditsAsync(); + await AddEditsAsync(Anchor); + var compose = new CountingCompose(SealedHash); + var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); + + await AddEditsAsync(Anchor); + compose.Result = "a-different-hash"; + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); + compose.Count.ShouldBe(2); + } + + [SqlServerFact] + public async Task Emptying_the_table_after_a_verdict_recomposes() + { + // Exercises the populated-then-empty transition, i.e. the aggregate crossing back to "no group". + await ClearConfigEditsAsync(); + await AddEditsAsync(Anchor, Anchor.AddSeconds(1)); + var compose = new CountingCompose(SealedHash); + var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); + + await ClearConfigEditsAsync(); + compose.Result = "a-different-hash"; + + (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); + compose.Count.ShouldBe(2); + } + + // -------------------------------------------------------------------------------------- Helpers + + private async Task ClearConfigEditsAsync() + { + await using var db = await sql.Factory.CreateDbContextAsync(); + await db.ConfigEdits.ExecuteDeleteAsync(); + } + + private async Task AddEditsAsync(params DateTime[] editedAtUtc) + { + await using var db = await sql.Factory.CreateDbContextAsync(); + foreach (var at in editedAtUtc) + { + db.ConfigEdits.Add(new ConfigEdit + { + EntityType = "Equipment", + EntityId = Guid.NewGuid(), + FieldsJson = "{}", + EditedBy = "test", + SourceNode = "node-1", + EditedAtUtc = at, + }); + } + + await db.SaveChangesAsync(); + } + + /// Stand-in for the ConfigComposer seam that counts how often it was invoked. + private sealed class CountingCompose(string result) + { + public string Result { get; set; } = result; + + public int Count { get; private set; } + + public Task ComposeAsync(OtOpcUaConfigDbContext db) + { + Count++; + return Task.FromResult(Result); + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceRenameWarningSqlServerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceRenameWarningSqlServerTests.cs new file mode 100644 index 00000000..4fb1090f --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceRenameWarningSqlServerTests.cs @@ -0,0 +1,222 @@ +using Microsoft.EntityFrameworkCore; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// The SQL-Server-backed half of : pins that the +/// expression-built SourceCode.Contains(p0) || … || Contains(pN) disjunction +/// (RawTreeService.AnyPathContained) translates on the real provider, and pins the one +/// behaviour EF InMemory structurally cannot show — the collation widening the method's +/// remarks describe. +/// +/// +/// +/// Under EF InMemory the predicate runs as ordinal , so a +/// script naming a RawPath in different casing does NOT match there. On SQL Server the comparison +/// runs under the column collation — case-insensitive on a default install — and it DOES. That is +/// a deliberate, documented widening (a false positive costs an operator one glance; a false +/// negative ships a broken rename), and it is only assertable against a real server. +/// +/// +/// Also crosses the 200-term ScriptScanBatchSize chunk boundary against the real provider, +/// where a batch too large would be a translation/complexity failure rather than a silent +/// in-memory loop. +/// +/// +/// Every test seeds its own cluster/folder/driver subtree under a unique token, so the seeds do +/// not collide and — since the script scan is a whole-table substring search — no test's paths +/// can be matched by another's scripts. +/// +/// +[Collection(nameof(AdminUiSqlServerCollection))] +public sealed class RawTreeServiceRenameWarningSqlServerTests(AdminUiSqlServerFixture sql) +{ + /// Mirrors RawTreeService.ScriptScanBatchSize (private); the boundary under test. + private const int ScriptScanBatchSize = 200; + + [SqlServerFact] + public async Task A_script_naming_the_path_in_different_case_is_matched_under_the_server_collation() + { + var seed = await SeedAsync(token => new Seed( + Tags: ["flow"], + Scripts: + [ + // Same path, opposite casing throughout. Ordinal Contains (EF InMemory) misses this; SQL + // Server's default CI collation finds it. + new ScriptSeed("CaseScript", $"var v = ctx.GetTag(\"{RawPath(token, "flow").ToUpperInvariant()}\");"), + ])); + + var result = await seed.Service.RenameDeviceAsync(seed.DeviceId, "Dev-renamed", seed.DeviceRowVersion); + + result.Ok.ShouldBeTrue(result.Error); + result.Warnings.ShouldContain(w => w.Contains("script") && w.Contains("CaseScript")); + } + + [SqlServerFact] + public async Task A_script_naming_the_path_in_the_same_case_is_matched() + { + // Control for the case-insensitivity test above: if this one failed too, the previous test would + // be proving the scan runs at all rather than proving the widening. + var seed = await SeedAsync(token => new Seed( + Tags: ["flow"], + Scripts: [new ScriptSeed("ExactScript", $"var v = ctx.GetTag(\"{RawPath(token, "flow")}\");")])); + + var result = await seed.Service.RenameDeviceAsync(seed.DeviceId, "Dev-renamed", seed.DeviceRowVersion); + + result.Ok.ShouldBeTrue(result.Error); + result.Warnings.ShouldContain(w => w.Contains("script") && w.Contains("ExactScript")); + } + + [SqlServerFact] + public async Task A_script_naming_no_affected_path_is_not_matched() + { + // Falsification control: the disjunction must be a real filter server-side, not a predicate that + // degenerates to "true" (which every match assertion above would still pass against). + var seed = await SeedAsync(token => new Seed( + Tags: ["flow"], + Scripts: [new ScriptSeed("UnrelatedScript", $"var v = ctx.GetTag(\"{RawPath(token, "not-a-tag-here")}\");")])); + + var result = await seed.Service.RenameDeviceAsync(seed.DeviceId, "Dev-renamed", seed.DeviceRowVersion); + + result.Ok.ShouldBeTrue(result.Error); + result.Warnings.ShouldNotContain(w => w.Contains("script")); + } + + [SqlServerFact] + public async Task Matches_are_found_on_both_sides_of_the_scan_batch_boundary() + { + // 201 distinct paths ⇒ two batches at the 200-term cap. One script names a path in the first + // batch, one names the single path that lands in the second: a loop that dropped either batch, or + // a 200-term OR-chain the provider refused to translate, fails here. + const int tagCount = ScriptScanBatchSize + 1; + var names = Enumerable.Range(0, tagCount).Select(i => $"bulk-{i:D3}").ToArray(); + + var seed = await SeedAsync(token => new Seed( + Tags: names, + Scripts: + [ + new ScriptSeed("BatchFirst", $"ctx.GetTag(\"{RawPath(token, names[0])}\");"), + new ScriptSeed("BatchLast", $"ctx.GetTag(\"{RawPath(token, names[^1])}\");"), + ])); + + var result = await seed.Service.RenameDeviceAsync(seed.DeviceId, "Dev-renamed", seed.DeviceRowVersion); + + result.Ok.ShouldBeTrue(result.Error); + result.Warnings.ShouldContain( + "2 scripts ('BatchFirst', 'BatchLast') reference tags beneath this node by their raw paths; " + + "the references will break unless the scripts are updated."); + } + + // ----------------------------------------------------------------------------------------- Seed + + private const string FolderName = "Plant"; + private const string DriverName = "modbus-1"; + private const string DeviceName = "Dev-scan"; + + /// The RawPath a seeded tag resolves to: Folder/Driver/Device/Tag (the cluster is not in it). + private static string RawPath(string token, string tagName) => + $"{FolderName}{token}/{DriverName}/{DeviceName}/{tagName}"; + + private sealed record ScriptSeed(string Name, string SourceCode); + + private sealed record Seed(IReadOnlyList Tags, IReadOnlyList Scripts); + + private sealed record SeedResult(RawTreeService Service, string DeviceId, byte[] DeviceRowVersion); + + private async Task SeedAsync(Func build) + { + // 8 hex chars: unique per test, and short enough for the 64-char id columns. + var token = Guid.NewGuid().ToString("N")[..8]; + var seed = build(token); + + var clusterId = $"C-{token}"; + var folderId = $"RF-{token}"; + var driverId = $"DRV-{token}"; + var deviceId = $"DEV-{token}"; + + await using (var db = await sql.Factory.CreateDbContextAsync()) + { + db.ServerClusters.Add(new ServerCluster + { + ClusterId = clusterId, + Name = $"Cluster {token}", + Enterprise = "zb", + Site = "warsaw-west", + // NodeCount is NOT optional here as it is under EF InMemory: CK_ServerCluster_ + // RedundancyMode_NodeCount pins None ⇒ 1 (and Warm/Hot ⇒ 2). + NodeCount = 1, + RedundancyMode = RedundancyMode.None, + CreatedBy = "test", + }); + db.RawFolders.Add(new RawFolder + { + RawFolderId = folderId, + ClusterId = clusterId, + ParentRawFolderId = null, + Name = $"{FolderName}{token}", + }); + db.DriverInstances.Add(new DriverInstance + { + DriverInstanceId = driverId, + ClusterId = clusterId, + RawFolderId = folderId, + Name = DriverName, + DriverType = "Modbus", + DriverConfig = "{}", + }); + db.Devices.Add(new Device + { + DeviceId = deviceId, + DriverInstanceId = driverId, + Name = DeviceName, + DeviceConfig = "{}", + }); + + for (var i = 0; i < seed.Tags.Count; i++) + { + db.Tags.Add(new Tag + { + TagId = $"TAG-{token}-{i:D4}", + DeviceId = deviceId, + TagGroupId = null, + Name = seed.Tags[i], + DataType = "Float", + AccessLevel = TagAccessLevel.Read, + TagConfig = "{}", + }); + } + + for (var i = 0; i < seed.Scripts.Count; i++) + { + db.Scripts.Add(new Script + { + ScriptId = $"SC-{token}-{i:D2}", + Name = seed.Scripts[i].Name, + SourceCode = seed.Scripts[i].SourceCode, + SourceHash = $"hash-{token}-{i:D2}", + }); + } + + await db.SaveChangesAsync(); + } + + // SQL Server DOES enforce the rowversion token (EF InMemory does not), so the rename must carry + // the value the server actually stamped. + byte[] rowVersion; + await using (var db = await sql.Factory.CreateDbContextAsync()) + { + rowVersion = await db.Devices.AsNoTracking() + .Where(d => d.DeviceId == deviceId) + .Select(d => d.RowVersion) + .SingleAsync(); + } + + return new SeedResult(new RawTreeService(sql.Factory), deviceId, rowVersion); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj index 88277f85..ba5aba32 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ZB.MOM.WW.OtOpcUa.AdminUI.Tests.csproj @@ -11,6 +11,11 @@ + + diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs index e712777e..a0b3c57d 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -519,6 +519,62 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable finally { await host.DisposeAsync(); } } + /// + /// An EMPTY s= identifier names no node, so it must not be memoized: every item on such a + /// node asks the authorizer for itself. + /// + /// + /// + /// An empty identifier addresses no ACL scope at all — + /// rejects it before its own decision cache for + /// exactly that reason — so collapsing every item on one onto a single pass-local verdict is + /// keying the memo on something that is not an identity. The guard mirrors the authorizer's: + /// an empty identifier falls through to the un-memoized call, which is fail-safe (each item + /// gets its own verdict) rather than merely cheaper. + /// + /// + /// Two items on DISTINCT empty-identifier nodes cannot be constructed — an empty identifier in + /// one realm is one node id — so what is pinned here is the bypass itself: the authorizer is + /// asked once per item, where the memoized path asks once per pass. + /// is the control that + /// the memo still engages for a non-empty identifier, so this cannot pass by the memo having + /// been removed. + /// + /// + [Fact] + public async Task Revalidation_does_not_memoize_an_empty_node_identifier() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new CountingTimeProvider(new DateTime(2026, 8, 15, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = SubscribeAuthorizer(clock); + + // The manager's raw namespace index, borrowed from a real node, with an empty identifier. + var rawNamespace = Variable(nm, "Plant/Modbus/dev1/Speed").NamespaceIndex; + var emptyId = new NodeId(string.Empty, rawNamespace); + + // Non-vacuity: this really is a STRING identifier of length zero, the one case the guard used + // to let through — not a null identifier the guard already rejected. + emptyId.Identifier.ShouldBeOfType().ShouldBeEmpty(); + + var identity = Identity(); + var items = Enumerable.Range(0, 2) + .Select(_ => TrackItem(nm, new NodeHandle { NodeId = emptyId }, identity)) + .ToList(); + + clock.Reset(); + nm.RevalidateMonitoredItems(); + + clock.Calls.ShouldBe(2, "an empty identifier is not a memo key, so each item is asked for itself"); + + // And the un-memoized verdict is the authorizer's own deny, reaching every item. + items.ShouldAllBe(i => i.Denials == 1); + } + finally { await host.DisposeAsync(); } + } + // ───────────────────────────── helpers ───────────────────────────── /// A cluster-wide Subscribe grant — what CreateMonitoredItems resolves to. @@ -531,9 +587,16 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable } /// Registers a stub data-change item against as if the SDK had created it. - private static StubSampledItem TrackItem(OtOpcUaNodeManager nm, NodeId nodeId, IUserIdentity identity) + private static StubSampledItem TrackItem(OtOpcUaNodeManager nm, NodeId nodeId, IUserIdentity identity) => + TrackItem(nm, (NodeHandle)nm.GetManagerHandle(nodeId), identity); + + /// + /// Registers a stub data-change item against a hand-built — the only way + /// to reach a node id the address space cannot mint, such as an empty s= identifier. + /// + private static StubSampledItem TrackItem( + OtOpcUaNodeManager nm, NodeHandle handle, IUserIdentity identity) { - var handle = (NodeHandle)nm.GetManagerHandle(nodeId); var item = new StubSampledItem(handle, identity); nm.TrackLiveMonitoredItem(handle, item); return item; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs index 7e595806..6062c6b7 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs @@ -163,6 +163,106 @@ public sealed class SessionAclDecisionCacheTests neighbourCalls.ShouldBe(1); } + /// + /// After a shard is evicted the global counter equals what actually survived — the arithmetic of + /// the swap, pinned single-threaded where it is exact and every off-by-a-shard is visible. + /// + [Fact] + public void An_eviction_subtracts_exactly_what_it_displaced() + { + var cache = new SessionAclDecisionCache(); + const int overshoot = 25; + + for (var i = 0; i < SessionAclDecisionCache.MaxEntriesPerSession + overshoot; i++) + Ask(cache, generation: 1, session: "crawler-key", node: $"Plant/dev1/Tag{i}", compute: () => true); + + // The cap is reached on the entry that would have been the 20,001st; everything after it is the + // refilled shard. Subtracting anything but the displaced shard's own count would show up here. + cache.Count.ShouldBe(overshoot); + cache.TrackedCount.ShouldBe(cache.Count); + cache.LiveShardCount.ShouldBe(cache.Count); + } + + /// + /// The eviction race. Many writers hammer ONE shard past its ceiling while others insert into it, + /// so evictions and inserts interleave repeatedly; at quiescence the global counter must equal the + /// entries that actually survived. + /// + /// + /// + /// The defect this pins is not reachable single-threaded. Eviction used to empty the shard's + /// dictionary and then reset the counters as two steps: a writer blocked inside + /// TryAdd for the duration of the Clear resumes the moment the table's locks are + /// released — squarely in the window before the counter reset — books an increment, and has it + /// subtracted a second time by the reset that follows. The drift is negative-only, so it never + /// changed a verdict; it made the cache trim progressively earlier than it should. + /// + /// + /// ⚠️ The assertion is equality, not a bound. A ceiling check + /// (TrackedCount <= Count, or >= 0) passes against the broken code, because + /// under-counting is exactly what the broken code does. Only TrackedCount == Count + /// falsifies it — verified by reverting the fix, which turns this test red on the first round. + /// + /// + [Fact] + public void Concurrent_eviction_and_insertion_leave_the_counter_exact() + { + var cache = new SessionAclDecisionCache(); + const string crawler = "crawler-key"; + const int crawlers = 8; + const int neighbours = 2; + const int perCrawler = 40_000; + const int perNeighbour = 500; + + // Sized for exposure, not for coverage: one round drives the shard past its ceiling ~16 times, and + // the interleaving that used to corrupt the counter needs a writer to land inside the few + // nanoseconds between the clear and the counter reset. Fewer rounds let the reverted code pass. + const int rounds = 8; + + for (var round = 0; round < rounds; round++) + { + var currentRound = round; + var start = new Barrier(crawlers + neighbours); + var workers = new Thread[crawlers + neighbours]; + + for (var w = 0; w < workers.Length; w++) + { + var worker = w; + + // The first `crawlers` threads share ONE shard and drive it past its ceiling many times + // over; the rest hold their own shards, so the audit also covers the shards an eviction + // must not touch. + var session = worker < crawlers ? crawler : $"neighbour-{worker}"; + var writes = worker < crawlers ? perCrawler : perNeighbour; + + workers[w] = new Thread(() => + { + start.SignalAndWait(); + + // Node identifiers are unique per (round, worker, index), so every store is a genuine + // insert and nothing is lost to a TryAdd collision instead of to the race. + for (var i = 0; i < writes; i++) + cache.GetOrAdd( + generation: 1, session, OpcUaOperation.Read, AddressSpaceRealm.Raw, + $"Plant/dev1/R{currentRound}-W{worker}-T{i}", static () => true); + }) { IsBackground = true }; + } + + foreach (var worker in workers) worker.Start(); + foreach (var worker in workers) worker.Join(); + start.Dispose(); + + var survived = cache.Count; + + cache.TrackedCount.ShouldBeGreaterThanOrEqualTo( + 0, $"round {currentRound}: the counter must never be driven negative"); + cache.LiveShardCount.ShouldBe( + survived, $"round {currentRound}: a shard's own counter drifted from its entries"); + cache.TrackedCount.ShouldBe( + survived, $"round {currentRound}: the global counter drifted from the live shards"); + } + } + /// /// A decision taken while a deployment lands mid-walk is returned but NOT stored, so the new /// generation never inherits a verdict its own trie did not make. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs new file mode 100644 index 00000000..a0ab5f99 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs @@ -0,0 +1,133 @@ +using System.Text.Json; +using Akka.Actor; +using Microsoft.EntityFrameworkCore; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Interfaces; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Deploy; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Fleet; +using ZB.MOM.WW.OtOpcUa.Commons.Types; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// The failure path the Direct-apply artifact read's explicit 30 s command timeout relies on: a +/// read that THROWS is "no answer", so the apply fails and the revision does not advance. +/// +/// +/// +/// A pin, not red-first. The behaviour predates this class — LoadArtifactBlob has +/// always caught its own exceptions and returned , which +/// HandleArtifactForApply turns into a Failed ACK. What was missing was a test saying so, and +/// a timed-out command is exactly this shape (a SqlException out of the EF read), so the +/// timeout's whole safety argument rests on it: the node keeps serving last-known-good and the same +/// revision can be dispatched again (the #485/#486 family). +/// +/// +/// Sibling coverage reaches the same verdict through EMPTY bytes rather than a throw +/// (DriverHostActorUnreadableArtifactTests), which is the missing-row shape, not the +/// read-failed shape — the two arrive at ArtifactForApply by different routes. +/// +/// +public sealed class DriverHostActorDirectApplyReadFailureTests : RuntimeActorTestBase +{ + private static readonly NodeId TestNode = NodeId.Parse("driver-test"); + private static readonly RevisionHash RevA = RevisionHash.Parse(new string('a', 64)); + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); + + [Fact] + public void A_direct_artifact_read_that_throws_fails_the_apply_and_leaves_the_revision_alone() + { + var inner = NewInMemoryDbFactory(); + var db = new FailingReadConfigDbFactory(inner); + var deploymentId = SeedDeployment(inner, RevA); + + var coordinator = CreateTestProbe(); + var actor = Sys.ActorOf(DriverHostActor.Props( + db, TestNode, coordinator.Ref, + localRoles: new HashSet { "driver" })); + + // Barrier: an answered GetDiagnostics proves PreStart (and its bootstrap ConfigDb read) is done, + // so the allowance below counts only the apply's own contexts. + actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref); + coordinator.ExpectMsg(Timeout); + + // Let ONE more context through — the apply's Applying-row upsert — and fail the NEXT one, which is + // the artifact read. Both misconfigurations fail LOUDLY rather than passing vacuously: too small + // fails the upsert instead (which swallows its own exceptions), so the read succeeds and the ACK is + // Applied; too large never fails the read at all, same Applied ACK. + db.FailAfter(allowThrough: 1); + + actor.Tell(new DispatchDeployment(deploymentId, RevA, CorrelationId.NewId())); + + var ack = coordinator.ExpectMsg(Timeout); + ack.Outcome.ShouldBe(ApplyAckOutcome.Failed); + ack.FailureReason.ShouldNotBeNullOrWhiteSpace(); + db.Throws.ShouldBe(1); // the failure came from the read, not from somewhere incidental + + // Never applied ⇒ never claimed: the node stays on what it was already serving. + actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref); + coordinator.ExpectMsg(Timeout).CurrentRevision.ShouldNotBe(RevA); + + // …and because the revision did not advance, re-dispatching THE SAME revision against a healthy DB + // is not waved through by the "already at this rev" short-circuit — it actually applies. + actor.Tell(new DispatchDeployment(deploymentId, RevA, CorrelationId.NewId())); + coordinator.ExpectMsg(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Applied); + + actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref); + coordinator.ExpectMsg(Timeout).CurrentRevision.ShouldBe(RevA); + } + + /// Seeds a sealed, parseable deployment and returns its id. + private static DeploymentId SeedDeployment(IDbContextFactory db, RevisionHash rev) + { + var artifact = JsonSerializer.SerializeToUtf8Bytes(new { DriverInstances = Array.Empty() }); + var id = DeploymentId.NewId(); + using var ctx = db.CreateDbContext(); + ctx.Deployments.Add(new Deployment + { + DeploymentId = id.Value, + RevisionHash = rev.Value, + Status = DeploymentStatus.Sealed, + CreatedBy = "test", + SealedAtUtc = DateTime.UtcNow, + ArtifactBlob = artifact, + }); + ctx.SaveChanges(); + return id; + } + + /// + /// A ConfigDb factory that throws for exactly ONE armed context — the fake shape of a central-SQL + /// read that times out. It throws once and then behaves normally, so the recovery leg of the test + /// runs against a healthy DB without re-wiring the actor. + /// + private sealed class FailingReadConfigDbFactory(IDbContextFactory inner) + : IDbContextFactory + { + private int _allowance = int.MaxValue; + private int _throws; + + /// How many contexts have been failed. + public int Throws => Volatile.Read(ref _throws); + + /// Arms a single throw, after letting more contexts past. + public void FailAfter(int allowThrough) => Volatile.Write(ref _allowance, allowThrough); + + public OtOpcUaConfigDbContext CreateDbContext() + { + if (Interlocked.Decrement(ref _allowance) == -1) + { + Interlocked.Increment(ref _throws); + throw new InvalidOperationException("central SQL command timed out (test stub)"); + } + + return inner.CreateDbContext(); + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs index b4f44cca..1ca1c182 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs @@ -353,6 +353,18 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit /// The yield is the whole point: every other test here uses an outbox that returns /// ValueTask.CompletedTask, so the continuation runs inline with the context still /// installed and the defect is invisible. The production FasterLog outbox fsyncs per entry. + /// The outbox double must therefore complete on a POOL thread + /// (Task.Delay(1).ConfigureAwait(false)); a bare Task.Yield() posts back to the + /// CAPTURED ActorTaskScheduler context and is just as vacuous as completing synchronously. + /// + /// + /// ⚠️ The falsification control is NONDETERMINISTIC UNDER LOAD. Re-introducing the defect is + /// only observable when the thread that resumes the continuation happens to carry no Akka + /// actor cell in its ThreadStatic slot; on a loaded parallel test sweep the resuming pool + /// thread can already be servicing this ActorSystem, the Self read succeeds, and the + /// mutation passes. So a SINGLE green run of the intended-red mutation is not proof this + /// regression is caught — re-run the mutation several times, ideally on an otherwise idle + /// machine, before concluding the test is falsifiable. /// /// [Fact] @@ -524,6 +536,145 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit }, TimeSpan.FromSeconds(10)); } + // ------- Shutdown cancellation is a pass ABORT, not a per-tag failure (guards #538) ------- + + /// + /// A write that is cut short by the drain token's cancellation (node shutdown) must abort the + /// whole pass, NOT be booked against that tag as a Gitea #538 consecutive failure. + /// + /// + /// The per-tag lambda wrapped the writer call in a bare catch, so a shutdown + /// was converted to "this tag failed to write". When a + /// sibling tag had already written in the same pass, the #538 quarantine counted it — so a + /// routine shutdown could push a perfectly HEALTHY tag over its attempt budget and DISCARD its + /// queued samples on the way out. The outer + /// catch (OperationCanceledException) when (ct.IsCancellationRequested) already treats a + /// cancelled pass as a non-fault, but the inner catch ate the exception before it could be reached. + /// + /// The load-bearing assertion is that BOTH entries are still queued afterwards: the pass + /// neither acked (the healthy tag's entry survives) nor abandoned (the cancelled tag's entry + /// survives). Against the unfixed recorder the failing tag goes one over budget on the + /// cancelled pass and the whole peeked prefix is truncated away, so the outbox reads 0. + /// + /// + /// The budget is deliberately the tightest there is (maxWriteAttempts: 1) with exactly + /// one genuine failure banked first, because a cancelled pass can only ever be observed to + /// abandon when it is the attempt that tips the tag OVER its budget — the counter is clamped + /// to at least 1, so a first-ever cancelled pass is retained either way and would prove nothing. + /// + /// + [Fact] + public async Task A_shutdown_cancelled_write_aborts_the_pass_without_counting_a_tag_failure() + { + var mux = CreateTestProbe(); + // "Bad.Tag" fails once for real, then BLOCKS — so the shutdown lands mid-write on the pass that + // would otherwise tip it over the budget. + var writer = new ShutdownBlockingValueWriter("Bad.Tag", failCalls: 1); + var outbox = new InMemoryOutbox(); + + IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props( + mux.Ref, writer, outbox, historizedRefs: new[] { "Good.Tag", "Bad.Tag" }, + minBackoff: TimeSpan.FromMilliseconds(20), + maxBackoff: TimeSpan.FromMilliseconds(50), + drainInterval: TimeSpan.FromMilliseconds(20), + maxWriteAttempts: 1, + // Sequential, so the batch's FIRST group (the healthy tag) has demonstrably written before + // the second one blocks — "a sibling succeeded this pass" is the #538 discriminator, and + // this test is only meaningful when it is true. + drainConcurrency: 1)); + + rec.Tell(new VirtualTagActor.DependencyValueChanged("Good.Tag", 1.0, DateTime.UtcNow)); + rec.Tell(new VirtualTagActor.DependencyValueChanged("Bad.Tag", 2.0, DateTime.UtcNow)); + + await writer.Entered.WaitAsync(TimeSpan.FromSeconds(10)); + + // Preconditions, so a green result can never be vacuous: both entries are still queued (they were + // peeked as ONE batch — a lone healthy entry would have been acked and gone), and the healthy tag + // has written on this very pass. + Assert.Equal(2, await outbox.CountAsync(default)); + Assert.Equal(2, writer.CallCountFor("Good.Tag")); + + Sys.Stop(rec); // PostStop cancels the drain token underneath the in-flight write + + await writer.Cancelled.WaitAsync(TimeSpan.FromSeconds(10)); + + // Give the (unfixed) fold every chance to book the failure and truncate before we assert it did not. + await Task.Delay(TimeSpan.FromMilliseconds(500)); + Assert.Equal(2, await outbox.CountAsync(default)); + } + + /// + /// Scope pin: a NON-cancellation throw from one tag, while a sibling wrote, still counts toward + /// the Gitea #538 budget and still abandons. + /// + /// + /// The fix narrows the per-tag catch by one exception shape only. A writer that faults for any + /// other reason is exactly the poison tag #538 exists for, and must keep pinning nothing. + /// + [Fact] + public async Task A_non_cancellation_throw_still_counts_toward_abandonment() + { + var mux = CreateTestProbe(); + var writer = new FakeValueWriter { Succeed = true }; + writer.SetThrowing("Bad.Tag", static _ => new InvalidOperationException("gateway write faulted")); + var outbox = new InMemoryOutbox(); + + IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props( + mux.Ref, writer, outbox, historizedRefs: new[] { "Good.Tag", "Bad.Tag" }, + minBackoff: TimeSpan.FromMilliseconds(20), + maxBackoff: TimeSpan.FromMilliseconds(50), + drainInterval: TimeSpan.FromMilliseconds(20), + maxWriteAttempts: 2)); + + rec.Tell(new VirtualTagActor.DependencyValueChanged("Good.Tag", 1.0, DateTime.UtcNow)); + rec.Tell(new VirtualTagActor.DependencyValueChanged("Bad.Tag", 2.0, DateTime.UtcNow)); + + await AwaitAssertAsync(async () => + { + var status = await rec.Ask( + ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3)); + Assert.Equal(0, status.QueuedDepth); + Assert.Equal(1, status.AbandonedEntries); + }, TimeSpan.FromSeconds(10)); + } + + /// + /// Scope pin: an raised while the drain token is NOT + /// cancelled is an ordinary write failure and still counts toward abandonment. + /// + /// + /// This is what the when (ct.IsCancellationRequested) filter buys: only a genuine shutdown + /// aborts the pass. A gateway that surfaces a cancellation-shaped exception from its own internal + /// deadline is a failing tag like any other — without the filter it would become an unquarantinable + /// tag that pins the FIFO head forever, re-opening #538 through the back door. + /// + [Fact] + public async Task An_OperationCanceledException_with_a_live_token_still_counts_toward_abandonment() + { + var mux = CreateTestProbe(); + var writer = new FakeValueWriter { Succeed = true }; + writer.SetThrowing("Bad.Tag", static _ => new OperationCanceledException("not our shutdown")); + var outbox = new InMemoryOutbox(); + + IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props( + mux.Ref, writer, outbox, historizedRefs: new[] { "Good.Tag", "Bad.Tag" }, + minBackoff: TimeSpan.FromMilliseconds(20), + maxBackoff: TimeSpan.FromMilliseconds(50), + drainInterval: TimeSpan.FromMilliseconds(20), + maxWriteAttempts: 2)); + + rec.Tell(new VirtualTagActor.DependencyValueChanged("Good.Tag", 1.0, DateTime.UtcNow)); + rec.Tell(new VirtualTagActor.DependencyValueChanged("Bad.Tag", 2.0, DateTime.UtcNow)); + + await AwaitAssertAsync(async () => + { + var status = await rec.Ask( + ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3)); + Assert.Equal(0, status.QueuedDepth); + Assert.Equal(1, status.AbandonedEntries); + }, TimeSpan.FromSeconds(10)); + } + // ---------------- Gitea #537: only the recording Primary captures ---------------- /// @@ -792,6 +943,15 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit /// Task.Yield(), which posts back to the CAPTURED ActorTaskScheduler context — leaves the /// context installed and the defect invisible. /// + /// + /// ⚠️ The falsification control is NONDETERMINISTIC UNDER LOAD. Re-introducing the defect is + /// only observable when the thread that resumes the continuation happens to carry no Akka + /// actor cell in its ThreadStatic slot; on a loaded parallel test sweep the resuming pool + /// thread can already be servicing this ActorSystem, the Self read succeeds, and the + /// mutation passes. So a SINGLE green run of the intended-red mutation is not proof this + /// regression is caught — re-run the mutation several times, ideally on an otherwise idle + /// machine, before concluding the test is falsifiable. + /// /// [Fact] public async Task A_yielding_batch_append_neither_faults_the_actor_nor_loses_values() @@ -1164,6 +1324,14 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit /// pass still succeed, and that contrast is the evidence the recorder keys on. private readonly HashSet _failTags = new(StringComparer.Ordinal); + /// Historian tags whose writes THROW instead of returning an ack, keyed to the exception + /// factory to throw (given the drain's token). Distinct from because the + /// recorder's per-tag catch treats the two shapes differently: a returned false is always + /// a failure, whereas a thrown exception is a failure only when it is not the drain token's own + /// cancellation. + private readonly Dictionary> _throwTags = + new(StringComparer.Ordinal); + /// Starts or stops failing one tag. Callable mid-test so a case can prove a tag that /// recovers within its budget is never abandoned. /// The historian tag. @@ -1177,6 +1345,18 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit } } + /// Starts or stops THROWING for one tag's writes. + /// The historian tag. + /// Builds the exception to throw from the drain's token; null stops throwing. + public void SetThrowing(string tag, Func? factory) + { + lock (_gate) + { + if (factory is null) _throwTags.Remove(tag); + else _throwTags[tag] = factory; + } + } + /// Lifetime count of invocations (proves a retry ran). public int CallCount { @@ -1208,6 +1388,13 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit _written.Add(new WrittenValue(tag, v.Value, v.Quality, v.TimestampUtc)); } + // A throwing tag overrides every ack rule. Thrown synchronously from inside the awaited + // call, which is where a real gateway client surfaces most faults. + if (_throwTags.TryGetValue(tag, out Func? factory)) + { + throw factory(ct); + } + // Fail the first FailFirstN calls; thereafter honour Succeed — unless this specific tag // is currently marked failing, which overrides both. bool result = _calls > FailFirstN && Succeed && !_failTags.Contains(tag); @@ -1216,6 +1403,90 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit } } + /// + /// double for the shutdown-cancellation drain path: one + /// nominated tag returns a plain failed ack for its first failCalls writes and then BLOCKS + /// on the next one until the drain's token is cancelled, at which point it throws the + /// a real in-flight gateway call raises at node shutdown. + /// Every other tag succeeds immediately. + /// + /// + /// The block is what makes the cancellation land INSIDE a write rather than between passes — the + /// only place the per-tag catch can misclassify it. cannot express + /// this: it completes synchronously, so the drain token is never live-cancelled underneath it. + /// + private sealed class ShutdownBlockingValueWriter : IHistorianValueWriter + { + private readonly Lock _gate = new(); + private readonly List _calls = new(); + private readonly string _tag; + private readonly int _failCalls; + private readonly TaskCompletionSource _entered = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _cancelled = new(TaskCreationOptions.RunContinuationsAsynchronously); + + /// The historian tag that fails and then blocks. + /// How many of its writes return a plain failed ack before the blocking one. + public ShutdownBlockingValueWriter(string tag, int failCalls) + { + _tag = tag; + _failCalls = failCalls; + } + + /// Completes once the blocking write has been ENTERED — i.e. the pass is mid-write and it + /// is safe to stop the recorder. + public Task Entered => _entered.Task; + + /// Completes once that blocked write has observed the cancellation and thrown. + public Task Cancelled => _cancelled.Task; + + /// Lifetime count of writes for one tag. + /// The historian tag. + /// How many times it has been written. + public int CallCountFor(string tag) + { + lock (_gate) + { + return _calls.Count(c => string.Equals(c, tag, StringComparison.Ordinal)); + } + } + + public async Task WriteLiveValuesAsync( + string tag, IReadOnlyList values, CancellationToken ct) + { + int seen; + lock (_gate) + { + _calls.Add(tag); + seen = _calls.Count(c => string.Equals(c, tag, StringComparison.Ordinal)); + } + + if (!string.Equals(tag, _tag, StringComparison.Ordinal)) + { + return true; + } + + if (seen <= _failCalls) + { + return false; + } + + _entered.TrySetResult(); + try + { + await Task.Delay(Timeout.Infinite, ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // Signalled before the rethrow so the test can await the exact moment the drain begins + // unwinding, rather than sleeping and hoping. + _cancelled.TrySetResult(); + throw; + } + + return false; // unreachable: the infinite delay only ever completes by cancelling + } + } + private sealed record WrittenValue(string Tag, double Value, ushort Quality, DateTime? TimestampUtc); /// In-memory double honouring the FIFO-truncate