Merge branch 'perf/followups' — follow-up register closed: per-driver overrun meter, drain-cancellation + ACL eviction + alarm-tracking fixes, SQL-Server-backed test pins, MTConnect XmlReader streaming, S7 multi-var leftovers; S7 + docker-dev live gates PASSED
v2-ci / build (push) Successful in 5m10s
v2-ci / unit-tests (push) Failing after 16m0s

This commit is contained in:
Joseph Doherty
2026-08-17 05:01:59 -04:00
45 changed files with 4294 additions and 168 deletions
+4 -3
View File
@@ -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
+53 -16
View File
@@ -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 <sha>, 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.
+780
View File
@@ -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 -- <files>` only (never a bare commit); never checkout/switch/reset/
stash/clean; retry on `index.lock` and CS2012/MSB3030 build races; reviewers judge commits
via `git show`, never the working tree; falsification mutations reverted via Edit; TDD
red-first with honest "pin, not red-first" reporting where the behaviour predates the test.
---
### Task 1: Per-driver poll-overrun meter + PollGroupEngine doc notes
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** Tasks 2, 3, 4, 5, 6, 7, 8, 10, 11
**Files:**
- Modify: `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs` (new counter)
- Modify: `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs` (doc notes only)
- Modify (one-line each, at the `new PollGroupEngine(` call sites): `ModbusDriver.cs:157`,
`FocasDriver.cs:117`, `TwinCATDriver.cs:76`, `AbLegacyDriver.cs:74`, `S7Driver.cs:65`,
`SqlDriver.cs:226`, `AbCipDriver.cs:157`
- Test: `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/` (PollGroupEngine tests file) +
one per-driver wiring pin is NOT required (see below)
**Spec.** The seven poll drivers' pacing change currently ships with no production consumer
of `PollGroupEngine.OverrunCount` (the accepted-not-fixed item). Close it the way the
engine's own doc note (`PollGroupEngine.cs:47-50`) prescribes: the driver wires its own
meter to the existing `onOverrun` callback (ctor param, line 105).
1. Add to `OtOpcUaTelemetry` (following the existing `CreateCounter` field pattern, e.g.
`otopcua.driver.lifecycle` at line 42):
`public static readonly Counter<long> PollOverruns = Meter.CreateCounter<long>("otopcua.driver.poll.overrun", unit: "{overrun}", description: "Poll cycles that ran longer than the poll interval (slow success and slow failure both count — see PollGroupEngine.NoteOverrun).");`
2. In each of the seven drivers, pass
`onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, new KeyValuePair<string, object?>("driver.type", "<type>"))`
where `<type>` is the driver's `DriverTypeNames` constant (use the constant, not a
literal — the `TwinCat`/`Focas` drift was a real bug once). If the constructor scope has
a cheap device/instance identifier already in hand, add a second `device` tag; do NOT
plumb new state to get one.
Note: Commons is referenced from driver projects already via the messages namespace — if
any of the seven driver csproj lacks the `Commons` reference, add the ProjectReference;
report it rather than inventing a per-driver Meter (Galaxy's own-meter pattern is the
outlier, not the template).
3. Doc notes in `PollGroupEngine.cs` (comment-only):
- Update lines 47-50: the follow-up is now delivered driver-side via
`OtOpcUaTelemetry.PollOverruns`; the engine still deliberately owns no Meter.
- `NoteOverrun` (lines 315-322): add the caveat that it conflates a slow success with a
slow failure — both increment; disambiguation would need the loop's outcome, and no
consumer needs it yet.
- `backoffCap` ctor param (line 104): doc its units (a `TimeSpan` cap on the exponential
failure backoff) and its interaction with `minInterval` (backoff replaces, not adds to,
the interval on the failure path — state whichever the loop at 261-263 actually does;
read it, don't guess).
**Tests.** One unit test on the engine pinning that an overrun invokes the `onOverrun`
callback exactly as many times as `OverrunCount` increments (may already exist from Task 11
of the remainder — if so, extend or cite it; do not duplicate). The seven driver wirings are
one-line closures over a static counter — no per-driver test; a wiring test would just
restate the line.
**Step order:** test (or cite existing) → counter → 7 wirings → doc notes → build the
solution → commit.
**Commit:** `feat(drivers): per-driver poll-overrun meter on the shared OtOpcUa Meter; NoteOverrun + backoffCap doc notes`
---
### Task 2: S7 span-option validation guard
**Classification:** small
**Estimated implement time:** ~3 min
**Parallelizable with:** Tasks 1, 3, 4, 5, 6, 7, 8, 10, 11
**Files:**
- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs` (`ParseOptions`, line 46 area)
- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs` (doc remark only, if needed)
- Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/` (the factory/options parse test file)
**Spec.** `SpanReadMaxSpanBytes` (`S7DriverOptions.cs:128`, default 220) has no validation:
a nonsensical value is accepted at config time and only fails (or silently misbehaves) at
read time. There is no `IValidateOptions` machinery for S7 (confirmed) — `ParseOptions` in
`S7DriverFactoryExtensions.cs` is the single choke point, so validate there:
- `SpanReadMaxSpanBytes < 1` → throw `ArgumentException`/config-error consistent with how
`ParseOptions` reports other bad config (read the method; match its existing error style).
- `SpanReadMaxGapBytes < -1` → same (−1 = disabled is legal, per `S7Driver.cs:571`'s
`>= 0` enable check; 0 is legal "no gap tolerated").
- Values **above** the negotiated PDU size cannot be checked here — the negotiated size is
opaque inside S7.Net (confirmed: no API exposes it) and S7.Net transparently chunks
oversized reads. Do NOT invent a fake upper bound; instead add one doc line to the
`SpanReadMaxSpanBytes` remark in `S7DriverOptions.cs` stating that over-PDU values are
legal but degrade to multi-request chunking inside S7.Net.
**Tests.** Red-first: `SpanReadMaxSpanBytes = 0` rejected; `SpanReadMaxGapBytes = -2`
rejected; `-1`/`0`/defaults accepted.
**Commit:** `fix(s7): validate span-read options at parse time; document the over-PDU degradation`
---
### Task 3: Historian drain — shutdown cancellation is not a per-tag failure
**Classification:** high-risk (concurrency, #538 quarantine semantics)
**Estimated implement time:** ~5 min
**Parallelizable with:** Tasks 1, 2, 4, 5, 6, 7, 8, 10, 11
**Files:**
- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/ContinuousHistorizationRecorder.cs:703-712`
- Test: `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs`
**Spec.** In `DrainOnceAsync`, the per-tag parallel lambda wraps
`_writer.WriteLiveValuesAsync(g.Key, g.Values, ct)` in a **bare `catch`** (`:707-712`) that
converts a shutdown `OperationCanceledException` into `ok = false`. When sibling tags in the
same pass already succeeded, the #538 logic (`:775-784`) then counts the cancelled tag as a
consecutive per-tag failure — a shutdown can push a healthy tag toward abandonment. The
outer `catch (OperationCanceledException) when (ct.IsCancellationRequested)` (`:812-816`)
already treats whole-pass cancellation as non-fault but is never reached because the inner
catch eats it first.
Fix: in the per-tag lambda, rethrow genuine token cancellation ahead of the bare catch:
```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<Task>` overload that (under `_inFlightLock`)
installs a placeholder/`TaskCompletionSource` (or checks `_disposed` and declines to
start), then invokes the factory and links completion — so a task that runs is always
visible to `Dispose`'s snapshot, and after `_disposed` no new background work starts.
Preserve exactly: the drain-latch handover semantics
(`Sustained_synchronous_drain_failures_hand_over_without_deepening_the_stack`, `:1627` —
the handover MUST keep spawning via `Task.Run` on a fresh stack, commit `c8a70143`), the
`ExecuteSynchronously` self-removal, and `Dispose`'s synchronous `WhenAll` drain. If the
handover call site (`TrackBackgroundTask(Task.Run(() => DrainDirtyAsync(ct)))`) needs
reshaping to the factory form, keep the `Task.Run` inside.
2. **`_disposed`**: make it `volatile` (`:172`) — it is written in `Dispose` (`:1078`) and
read unsynchronized on other threads (`:235, 544, 603, 662, 682, 987, 1027`).
**Tests.** Red-first for the ordering fix where constructible: a factory whose task
completes synchronously/immediately while `Dispose` races — deterministically, pin the new
invariant instead: (a) after `Dispose`, `TrackBackgroundTask`'s factory form does not invoke
the factory (no new work post-disposal); (b) a task started via the factory form is awaited
by `Dispose` even when it completes before the continuation attaches. Every existing engine
test (`:817`, `:1458`, `:1521`, `:1563`, `:1627`) must stay green — they are the spec for
what must not change.
**Commit:** `fix(alarms): background tasks register before they start; volatile _disposed`
---
### Task 9: Test-reliability hardening (durability second-reader, #491 controls)
**Classification:** small
**Estimated implement time:** ~4 min
**Parallelizable with:** Tasks 4, 5, 6, 7, 8, 10, 11 (NOT Task 3 — same recorder test file)
**Blocked by:** Task 3
**Files:**
- Modify: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Gateway.Tests/Recorder/FasterLogHistorizationOutboxTests.cs:172-197` (+ helper `ReadCommitted` `:203+`)
- Modify: `tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/ContinuousHistorizationRecorderTests.cs:359-387`, `:797-831` (doc comments only)
**Spec.**
1. `AppendBatch_group_commit_is_durable_before_dispose` depends on filesystem flush
visibility to a second `FasterLog` reader, which macOS does not guarantee on the
Windows/Linux timescale. Prefer a robust wait: retry the second-reader scan (re-open or
re-read up to `CommittedUntilAddress`) with a bounded poll (e.g. up to 5 s) before
asserting — the property under test is "durable before dispose", which a bounded wait
still proves (the writer is not disposed during the poll). Only if FasterLog's reader
genuinely cannot observe later flushes on re-scan, fall back to an OS-conditional
`Assert.Skip` on macOS with a comment naming the platform limitation. Report which.
2. The two #491 yielding-fake tests: ensure each carries a comment stating the
falsification control is nondeterministic under load (ThreadStatic actor cell) — a single
green run of the intended-red mutation is not proof the regression is caught. The `:790-793`
comment covers part of this; make both tests carry the load-nondeterminism warning.
**Commit:** `test(historian): bounded-wait durability read; honest nondeterminism notes on the #491 controls`
---
### Task 10: MTConnect `Build` streams through XmlReader
**Classification:** standard
**Estimated implement time:** ~5 min
**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 7, 8, 11
**Files:**
- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectStreamsParser.cs` (`Build` `:221`, the three `Parse` overloads `:102/:138/:155`)
- Test: the existing MTConnect parser test file(s) under `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/`
**Spec.** Task 27 removed the per-chunk string copies; `Build(XDocument)` still materialises
the whole document tree per sample chunk. Rewrite the parse path to stream via `XmlReader`
(`XmlReader.Create(stream, settings)` with DTD prohibited, async off — match the sync call
shape) extracting exactly what `Build` extracts today, and route all three `Parse` overloads
through it (the string overload via a `StringReader`). Requirements:
- **Byte-for-byte behavioural parity** with the current `Build` on every existing test —
the current suite is the spec. Namespace handling: MTConnect documents version their
namespace URI; whatever name/namespace matching `Build` does today (local-name-based or
namespace-qualified) must be preserved exactly — read `Build` first and mirror it.
- Keep the `AsStream` zero-copy path (`:216-219`) feeding the reader.
- If, mid-implementation, `Build`'s extraction turns out to lean on document-order queries
that make a single-pass reader genuinely intractable (e.g. backward references), STOP and
report rather than shipping a half-streamed hybrid — that finding would itself close the
register item as "not worth it".
**Tests.** The existing suite must pass unchanged (parity). Add one test only if a gap is
found in existing coverage of odd shapes (empty streams element, condition entries, unknown
elements skipped) — cite, don't duplicate.
**Commit:** `perf(mtconnect): stream sample-chunk parsing through XmlReader; no per-chunk document tree`
---
### Task 11: IS7Plc multi-variable read seam
**Classification:** small
**Estimated implement time:** ~3 min
**Parallelizable with:** Tasks 1, 2, 3, 4, 5, 6, 7, 8, 10
**Files:**
- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/IS7Plc.cs` (interface `:18-66`, `S7PlcAdapter` `:125-149`)
**Spec.** S7.Net 0.20.0 ships `Plc.ReadMultipleVarsAsync(List<DataItem>, CancellationToken)`
(confirmed in the shipped DLL); `IS7Plc` does not expose it. Add
`Task<IReadOnlyList<DataItem>> ReadMultipleVarsAsync(List<DataItem> items, CancellationToken ct)`
to the interface and implement in `S7PlcAdapter` delegating to the library, wrapped in
`S7OperationDeadline.RunAsync` exactly like the existing members (`:126-149` pattern —
mirror it, including the deadline/exception shape). Return the same `DataItem` list S7.Net
populates. No driver-side use yet (Task 12). Update any test fakes implementing `IS7Plc`
(they will fail to compile otherwise — implement as `throw new NotSupportedException` in
fakes that don't care, or a delegate hook in the shared fake if one exists).
**Tests.** Compile-level only for the adapter (it is a one-line delegation under the same
deadline wrapper the other members already have pinned); the behaviour tests come with Task 12's fake-driven driver tests. State this honestly.
**Commit:** `feat(s7): expose ReadMultipleVarsAsync through the IS7Plc seam`
---
### Task 12: S7 multi-var reads for span leftovers
**Classification:** high-risk (wire behaviour, partial-failure semantics)
**Estimated implement time:** ~6 min (accepted over budget — do not split further; the fallback semantics need one head)
**Blocked by:** Tasks 1, 11 (both touch `S7Driver.cs` / `IS7Plc.cs`)
**Files:**
- Modify: `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs` (the `perTag` leftover loop, `:602-614` area)
- Test: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/` (extend `S7SpanReadParityTests.cs` or sibling)
**Spec.** The Task 13 planner reads one PDU per span; leftovers (`plan.LeftoverIndexes`:
Timer/Counter tags, unparseable addresses, `PrefersPerTagRead` singles) still go one
round-trip each. Batch the *batchable* leftovers through the new
`IS7Plc.ReadMultipleVarsAsync`:
- **Partition** leftovers into DataItem-expressible vs not. Timer/Counter: S7.Net's
`DataItem`/`VarType` does support Timer/Counter — verify against the library types and
include them only if the driver's existing per-tag path for them maps cleanly onto a
`DataItem`; if the mapping is not obviously identical, leave Timer/Counter on the per-tag
path and say so (correctness beats the round-trip). Unparseable addresses stay per-tag by
definition.
- **Chunk** conservatively: a multi-var request's item headers consume PDU space
(~12 bytes/item request-side against the common 240-byte PDU) and S7.Net does NOT chunk
multi-var reads for you. Use a `const int MultiVarReadMaxItems = 12` with a comment
deriving the number; no new config key (YAGNI — the planner options weren't config either
until they were needed).
- **Fallback, per chunk**: if `ReadMultipleVarsAsync` throws for a chunk, fall back to the
existing per-tag `ReadOneAsync` path for that chunk's tags — a multi-var failure must
never make tags Bad that a per-tag read would have served. Only per-tag failure produces
the Bad snapshot (reuse `RecordReadFailure` semantics). Per-item errors inside a
successful multi-var response (S7.Net surfaces item error codes) likewise fall back
per-tag for the failed items only, if the library exposes them; read the library's
`DataItem` result shape and handle what it actually gives you.
- Results must sort back into batch order exactly as today (`:613`).
- Health semantics unchanged: the same success/failure accounting the per-tag loop feeds.
**Tests.** Fake `IS7Plc` driven, red-first: (1) N batchable leftovers issue
`⌈N/12⌉` multi-var calls, zero per-tag calls; (2) parity — values decoded from multi-var
equal the per-tag path's (mirror `S7SpanReadParityTests` structure); (3) a throwing
multi-var chunk falls back to per-tag and serves Good; (4) per-tag-only tags (Timer/Counter
if excluded, unparseable) untouched; (5) falsification: break the chunk size (mutate to
1000) or the fallback and show the corresponding test goes red, then revert via Edit.
**Commit:** `perf(s7): span leftovers batch through multi-var read PDUs with per-chunk fallback`
---
### Task 13: S7 live gate — span planner + multi-var against the s7_1500 fixture
**Classification:** standard (integration test + live run)
**Estimated implement time:** ~6 min including the live run
**Blocked by:** Tasks 2, 12
**Files:**
- Create: `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanReadTests.cs`
**Spec.** The register's ⚠️: the 16/220 span defaults are analytically chosen and never
live-gated. Add fixture-backed tests in the established pattern
(`[Collection(Snap7ServerCollection.Name)]`, `[Trait("Category","Integration")]`,
`[Trait("Device","S7_1500")]`, `Assert.Skip(sim.SkipReason)` when unreachable — see
`S7_1500SmokeTests.cs:16-24`, fixture `Snap7ServerFixture.cs:40-92`, endpoint env
`S7_SIM_ENDPOINT` default `10.100.0.35:1102`):
1. Seed (via whatever helper the existing smoke tests use to seed DB bytes) a tag layout
that genuinely exercises the planner: several tags in one DB within the 16-byte gap
(one span), a tag beyond the gap (second span or leftover), and ≥ 2 leftover-class tags
(to exercise the Task 12 multi-var path against the real wire).
2. Read the batch with span reads enabled (defaults) and again with spans disabled
(`SpanReadMaxGapBytes = -1`); assert value-for-value parity and all-Good quality.
3. **Run it live** against the fixture: probe `10.100.0.35:1102` first; if down, bring the
stack up over SSH (`ssh dohertj2@10.100.0.35 'cd /opt/otopcua-s7 && docker compose --profile s7_1500 up -d'`
— the Mac path per CLAUDE.md; `lmxopcua-fix` does not exist here). Then
`dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests --filter "FullyQualifiedName~S7_1500SpanRead"`.
The gate PASSES only on a real run (skipped-because-unreachable is not a pass — report
honestly). Record the result in the test file header comment and the task report.
**Commit:** `test(s7): live-gate span reads + multi-var leftovers against the s7_1500 fixture`
---
### Task 14: docker-dev live gate (regression of the merged remainder)
**Classification:** standard (no code — live verification)
**Estimated implement time:** ~10 min wall (mostly rig startup)
**Blocked by:** Tasks 1-12 (gates the branch's code state)
**Files:** none (evidence appended to this plan's Outcomes)
**Spec.** The remainder branch merged with **no live gate** (accepted at the time; recorded
as a release condition). Run the bounded regression gate on the local docker-dev rig
(OrbStack — the rig runs locally on this Mac; only fixtures live on 10.100.0.35):
1. `docker compose -f docker-dev/docker-compose.yml up -d --build` (builds the current
branch image); wait for `cluster-seed` "done." (`docker-dev/README.md:71-77`).
2. Verify all nodes healthy; AdminUI at `http://localhost:9200` (login disabled — drive it
directly).
3. Deploy: headless `POST http://localhost:9200/api/deployments` with
`X-Api-Key: docker-dev-deploy-key`; confirm the deployment seals green (all nodes acked).
4. Data plane: Client.CLI `browse` + `subscribe` against `opc.tcp://localhost:4840`;
confirm values stream Good. Make a config edit that produces a surgical apply, deploy,
and confirm the open subscription survives (the 2026-08-15 gate's step 3, reduced).
5. `/alerts` and `/hosts` pages load and stream (exercises the Fleet/Hosts poll-guard and
telemetry changes).
6. Tear down: `docker compose -f docker-dev/docker-compose.yml down -v`.
Out of gate scope (say so in the evidence): historian end-to-end (needs VPN/real gateway),
FetchAndCache flip, load-scale throughput. Any failure: STOP, report, do not "fix the rig"
around a product defect.
**Commit:** none (evidence lands in Task 15's docs commit).
---
### Task 15: Close-out — full sweep + Outcomes
**Classification:** small
**Estimated implement time:** ~5 min plus sweep wall time
**Blocked by:** all prior tasks
**Files:**
- Modify: this plan (append `## Outcomes`: per-task commit SHAs, deviations, the two live-gate evidence blocks)
- Modify: `docs/plans/2026-08-17-perf-followups.md.tasks.json` (final ledger)
- Modify: `docs/plans/2026-08-15-perf-roadmap-remainder.md` (tick the closed register items — a one-line "closed by <sha>, 2026-08-17" annotation per item; do NOT rewrite history)
- Modify (if any behaviour docs drifted): `docs/Historian.md`, `docs/ScriptedAlarms.md`, `CLAUDE.md` (only the lines these tasks invalidate — e.g. the "no production consumer of the overrun counter" caveat)
**Spec.** (1) `dotnet build ZB.MOM.WW.OtOpcUa.slnx` — 0 errors. (2) Full unit sweep across
all test projects (the remainder plan's §Task 28 procedure; DB suites against
`10.100.0.35,14330` if reachable, recorded either way; the two empty Wonderware test dirs
are glob noise, not failures). (3) Outcomes + register annotations + ledger. (4) Memory
update is the orchestrator's job, not this task's.
**Commit:** `docs(plan): perf follow-ups close-out — outcomes, register annotations, sweep record`
---
## Outcomes
*Written at close-out, 2026-08-17, on branch `perf/followups`. All 15 tasks complete; both
release-condition live gates PASSED.*
### Sweep record
`dotnet build ZB.MOM.WW.OtOpcUa.slnx` → **0 errors**, 896 warnings (all pre-existing:
`xUnit1051` cancellation-token advice and the repo's own `OTOPCUA0001` CapabilityInvoker
analyzer firing on test call sites).
Full unit sweep — every `*.Tests` project in the solution, run per-project with `--no-build`
against the tree at the close-out commit. `*.IntegrationTests` are build-only by convention
and were not run in the sweep (the S7 suite ran separately as Task 13's live gate; the two
empty `tests/Drivers/…Driver.Historian.Wonderware*.Tests` directories carry no csproj and are
glob noise).
**43 / 43 test projects green (exit code 0 on every one).**
| | |
|---|---|
| Passed | **10 358** |
| Failed | **0** |
| Skipped | 19 |
| **Total** | **10 377** |
No test failed, so no isolation re-run was needed. All 19 skips are **pre-existing, env-gated**
suites, spread across five projects — `Core.Tests` (1), `Driver.Galaxy.Tests` (8),
`Driver.Historian.Gateway.Tests` (6, the `LiveIntegration` category that needs the
`wonder-sql-vd03` VPN), `OpcUaServer.Tests` (1), `Runtime.Tests` (3). **None of the skips are
the new SQL-Server-backed tests**: `AdminUI.Tests` ran **1537 / 0 skipped** and
`Configuration.Tests` **190 / 0 skipped**, which is the evidence that Task 7's pins actually
executed against a real SQL Server rather than skipping past themselves.
The DB-backed suites (`Configuration.Tests`, `AdminUI.Tests` including Task 7's new
SQL-Server-backed pins) ran **against a reachable** `10.100.0.35,14330` — they were executed,
not skipped. None of the known load-flake candidates flaked
(`DriverInstanceActorCommsLossWatchdogTests`, `RawTreeServiceRenameWarningTests`,
`RouteNativeAlarmAck_on_non_primary_is_dropped`, plus this branch's new
`SessionAclDecisionCache` concurrency stress test and the `ContinuousHistorizationRecorder`
timing tests).
### Per-task outcomes
| # | Task | Commit(s) | Outcome + deviation |
|---|---|---|---|
| 1 | Per-driver poll-overrun meter + `PollGroupEngine` doc notes | `e22ecea0`, `97902d68` | Done. Review deviation: the tag keys were literals; moved to snake_case consts (`driver_type` / `driver_instance`) so they match the rest of the Meter and a dashboard's group-by cannot split across two spellings. |
| 2 | `SpanReadMaxSpanBytes` validation guard | `acb37724` | Done, as planned — validated at parse time, with the over-PDU degradation documented. |
| 3 | Historian drain rethrows a shutdown `OperationCanceledException` | `7870c91f` | Done. Deviation in *severity*, not shape: the red run showed the bug did not merely inflate `_tagWriteFailures` — a shutdown mid-drain **discarded healthy tags' samples**. A pass abort, not a per-tag failure. |
| 4 | Bound the Direct-mode `LoadArtifactBlob` read | `dba5f806` | Done, with one addition the plan did not anticipate: an `IsRelational()` guard, because the extension throws on the InMemory provider the actor tests use. EF 10.0.7 verified **empirically** to reset the timeout on return to the pool, so the local `SetCommandTimeout` cannot pollute the pooled factory. ⚠️ The relational branch is consequently untested in-suite. |
| 5 | ACL `_globalCount` drift + concurrency stress test | `d8849345` | Done — **beyond the planned scope, and provably so.** The planned counter swap alone was insufficient; a seal sentinel *and* a `TryBookInsert` compare-exchange were both required. Falsification: removing either went 6/6 red. |
| 6 | Revalidation memo rejects empty node identifiers | `c4d8e4a2` | Done as a pattern-form guard mirroring the authorizer's; the bypass is pinned by a clock-call count rather than by observing the memo directly. |
| 7 | SQL-Server-backed pins for the drift watermark + rename collation | `c149410b` | Done. Deviation: `AdminUI.Tests` is pinned to **xunit v2** by `Akka.TestKit.Xunit2`, so `Assert.Skip` is unavailable — an overriding `SqlServerFactAttribute` gives a genuine skip instead. Both the `GroupBy` watermark aggregate and the `CHARINDEX` collation widening confirmed **live** against SQL Server. |
| 8 | `TrackBackgroundTask` registration ordering + volatile `_disposed` | `5ae43023` | Done via a TCS-placeholder **register-first** factory (registration precedes the work starting, closing the gap at its root rather than narrowing it); `_disposed` made volatile. |
| 9 | Durability-test robustness + honest #491 nondeterminism notes | `6b46c485` | Done. Deviation: the planned macOS skip proved **unnecessary** — verified empirically that a bounded wait is sufficient, so the test runs on macOS rather than skipping. The #491 falsification control's nondeterminism is inherent and is now documented, not fixed. |
| 10 | MTConnect sample-chunk parsing through `XmlReader` | `cda82c07` | Done. 566/566 parity against the pre-change parser, ~19% allocation win. One **disclosed** behaviour change: on a doubly-broken document the *ordering* of the reported errors differs. |
| 11 | Expose `ReadMultipleVarsAsync` through the `IS7Plc` seam | `6ee553cf` | Done, as planned. |
| 12 | S7 span leftovers batch through multi-var read PDUs | `20022083` | Done, with two forced deviations: S7.Net throws for the **whole** multi-var request rather than per item, so a bad item is handled by a **per-chunk fallback**; and Timer/Counter areas are **excluded** because S7.Net boxes them as `int` where the single-read path yields `ushort`, and box parity is load-bearing downstream. |
| 13 | S7 live gate (span reads + multi-var leftovers) | `57c66416` | **LIVE GATE PASSED** vs `10.100.0.35:1102`. Evidence below. |
| 14 | docker-dev live gate (regression of the merged remainder) | *(no commit — evidence below)* | **LIVE GATE PASSED.** Evidence below. |
| 15 | Close-out — full sweep + Outcomes | `7c5a843a`, *this commit* | Done. `7c5a843a` carries the review round's comment-only fixes; this commit carries the Outcomes, the live-gate evidence, the register annotations, and the ledger. |
### Live gate — S7 span reads (Task 13)
**2026-08-17, against the `s7_1500` fixture at `10.100.0.35:1102`. PASSED.**
- **Results:** 3/3 span-read tests green; 7 passed / 1 skipped across the full
`Device=S7_1500` suite. The single skip is the env-gated SYN-blackhole test, which needs a
recipe the fixture does not provide — not a silent skip of the gated behaviour.
- **Wire shape (the actual claim under test):** with spans **on**, 10 tags produced
**3 requests** — 2 block reads + 1 multi-var read carrying 3 items. With spans **off**, the
same 10 tags produced **10 requests**. Multi-variable and cross-area batching both confirmed
on the wire, and a falsification bit was flipped to prove the counter was measuring the real
traffic.
- **Reconnect suite** ran **live** (not simulated) via `S7_RECONNECT_BOUNCE_CMD` and passed.
- **Not live-pinned:** the Timer/Counter exclusion — the fixture has no TM/CT area, so the
box-parity reasoning behind excluding them rests on unit tests only.
- **Ops notes for whoever re-runs this** (both cost time on this run):
- The fixture **profile is baked into the image**, so `--build` is mandatory after editing a
profile. Confirm the seed actually took by looking for
`seeded DB1 … seeds=<n>` in `docker logs`; a stale image seeds the old profile silently.
- `/opt/otopcua-s7` on the docker host is **root-owned**, so a plain `rsync` fails with
status 23 **while looking successful** in its summary output. Use
`--rsync-path="sudo rsync"`.
### Live gate — docker-dev regression (Task 14)
**2026-08-17, local OrbStack rig, branch at `6b46c485`, ~14.5 min wall. PASSED.**
- **Boot:** clean-slate build + boot; 8 containers healthy, `RestartCount=0` across the board.
- **Deploy:** `c50678b2` sealed in **306 ms** with **6/6 Applied**.
- **Data plane:** browse / read / write end-to-end against the pymodbus fixture; a **180 s**
subscription streamed with **0 Bad** values.
- **Surgical apply (the headline regression target):** a **real** UNS-reference surgical apply
— deploy `3df4b321`, logging
`applied plan kind=PureAdd added=2 changed=1 surgicalRawTags=1 rebuild=False`. The open
subscription streamed **Good across the apply**, and the newly-added UNS mirror served live
immediately. This is the behaviour the 2026-08-15 surgical-deploys work exists to protect,
exercised as a genuine surgical path rather than a rebuild in disguise.
- **Deploy gate:** correctly rejected a malformed first attempt with **422**
(`UnsSegmentInvalid`, `EquipmentIdNotDerived`) — the gate is live, not bypassed.
- **AdminUI:** `/fleet`, `/alerts`, `/hosts`, `/uns`, `/raw`, `/deployments` all 200 with live
data.
- **Logs:** 0 ERR/FTL, excluding the by-design standby 503s.
- **Teardown:** clean.
- **Anomalies (non-blocking, all disclosed):**
- A transient **404 on the first `POST /api/deployments`** ~40 s after boot; the retry
succeeded. Scripted deploys should **poll** rather than fire once (see the follow-up
register below).
- Boot-time telemetry-dial **reconnect warnings**, settling within one cycle.
- `ScriptLogEntry` **dead-letters on site nodes**, one per deploy — **pre-existing**, not
introduced by this branch.
- **NOT exercised (say so plainly, do not read the pass as wider than it is):** historian
end-to-end (needs the VPN), the `FetchAndCache` flip, the ACL-**enabled** enforcement path,
the alarm-bearing surgical **ratchet** to rebuild, load-scale throughput, and real-browser
Blazor circuits.
### Follow-up register (opened by this round)
Open items this branch surfaced but deliberately did not close. **None blocks the branch.**
**ACL**
- ⚠️ **Pre-existing generation-rollover window in `SessionAclDecisionCache.InvalidateIfStale`.**
While the CAS winner is running `ClearAll()`, a racing caller can still serve a
stale-generation verdict out of a shard the winner has not purged yet. The window is
microseconds and **key revocation/expiry are unaffected** (both are re-checked *above* the
cache on every call), so this is a stale *grant walk*, not a stale key. Fix shape if it ever
matters: stamp the generation on `Shard` itself, or make the racer wait on the winner.
- **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.
@@ -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"
}
@@ -42,6 +42,40 @@ public static class OtOpcUaTelemetry
Meter.CreateCounter<long>("otopcua.driver.lifecycle", unit: "{event}",
description: "DriverInstanceActor lifecycle transitions (event=spawn|stop|fault).");
/// <summary>
/// Poll cycles that ran longer than their subscription's publishing interval, as counted by
/// <c>PollGroupEngine.OverrunCount</c>. <c>Core.Abstractions</c> deliberately carries no metrics
/// dependency, so each poll driver wires this counter to the engine's <c>onOverrun</c> callback;
/// this is the production consumer that surface was always missing. Tagged
/// <see cref="PollOverrunDriverTypeTag"/> (<c>driver_type</c>, a <c>DriverTypeNames</c> constant)
/// + <see cref="PollOverrunDriverInstanceTag"/> (<c>driver_instance</c>) — 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.
/// </summary>
public static readonly Counter<long> PollOverruns =
Meter.CreateCounter<long>("otopcua.driver.poll.overrun", unit: "{overrun}",
description: "Poll cycles that ran longer than the poll interval (slow success and slow failure both count — see PollGroupEngine.NoteOverrun).");
/// <summary>
/// Tag key carrying the <c>DriverTypeNames</c> constant on <see cref="PollOverruns"/>.
/// <para><b>The snake_case spelling is load-bearing, not a style choice.</b> It is the same
/// literal key <see cref="DriverInstanceLifecycle"/> emits (<c>DriverInstanceActor</c>), and
/// every other instrument on this <see cref="Meter"/> uses bare/snake_case keys
/// (<c>event</c>, <c>kind</c>, <c>outcome</c>, <c>site</c>, <c>reason</c>). A dotted
/// <c>driver.type</c> 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.</para>
/// </summary>
public const string PollOverrunDriverTypeTag = "driver_type";
/// <summary>
/// Tag key carrying the emitting driver's instance id on <see cref="PollOverruns"/>. Cardinality
/// is bounded by the number of configured driver instances on the node. Snake_case for the same
/// convention reason as <see cref="PollOverrunDriverTypeTag"/>.
/// </summary>
public const string PollOverrunDriverInstanceTag = "driver_instance";
// ---------------- VirtualTag / ScriptedAlarm engines ----------------
public static readonly Counter<long> VirtualTagEval =
@@ -44,10 +44,16 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions;
/// <c>consecutiveFailures &gt; 0</c> with a <c>backoffCap</c> configured, the capped-exponential
/// schedule wins, because a failing device must not be hammered at fixed rate.</para>
///
/// <para><b>No <c>Meter</c> lives here.</b> <c>Core.Abstractions</c> deliberately references
/// nothing, so <see cref="OverrunCount"/> plus the <c>onOverrun</c> 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.)</para>
/// <para><b>No <c>Meter</c> lives here, and that is still deliberate.</b>
/// <c>Core.Abstractions</c> references nothing, so <see cref="OverrunCount"/> plus the
/// <c>onOverrun</c> 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 <b>delivered</b>: every one of the seven poll drivers now passes an
/// <c>onOverrun</c> closure that bumps <c>OtOpcUaTelemetry.PollOverruns</c>
/// (<c>otopcua.driver.poll.overrun</c>) on the shared OtOpcUa <c>Meter</c>, tagged with the
/// driver's <c>DriverTypeNames</c> constant and its driver-instance id. The counter lives in
/// <c>Commons.Observability</c>, on the consumer side of the reference graph — this type still
/// owns no <c>Meter</c> and takes no metrics dependency.</para>
///
/// <para>Programmer errors and obviously-fatal exceptions (<see cref="OutOfMemoryException"/>,
/// <see cref="ThreadAbortException"/>, <see cref="StackOverflowException"/>,
@@ -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. <c>null</c> (default) keeps the
/// byte-identical fixed-interval cadence for consumers that have not opted in.</param>
/// byte-identical fixed-interval cadence for consumers that have not opted in.
/// <para><b>Units:</b> a <see cref="TimeSpan"/> ceiling on the post-poll <i>wait</i> — 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
/// <c>backoffCap</c> between every retry, forever.</para>
/// <para><b>Interaction with <paramref name="minInterval"/> — it REPLACES the paced delay,
/// it does not add to it.</b> 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
/// <c>max(interval − pollDuration, 0)</c>. With a cap configured AND
/// <c>consecutiveFailures &gt; 0</c>, the whole remainder computation is bypassed and the
/// wait becomes <c>ConnectionBackoff.ComputeDelay(interval, failures, cap)</c> — 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.</para>
/// <para>The <c>interval</c> feeding that schedule is the per-subscription interval
/// <i>already clamped</i> to <paramref name="minInterval"/> in <see cref="Subscribe"/>, so
/// the first failure's wait equals the clamped interval (shift 0 → 1×) and the floor is
/// honoured for free. One asymmetry worth knowing: <c>ComputeDelay</c> saturates at the cap
/// unconditionally, so a <paramref name="backoffCap"/> set SMALLER than the clamped
/// interval makes failures retry FASTER than the interval floor — the cap outranks
/// <paramref name="minInterval"/> on the failure branch. Configure it above the floor
/// (every driver here uses 30 s against a 100 ms floor).</para></param>
/// <param name="onBatch">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 <paramref name="onChange"/> callbacks. The
@@ -311,6 +337,16 @@ public sealed class PollGroupEngine : IAsyncDisposable
/// Record a poll that took longer than its interval: bump <see cref="OverrunCount"/> and
/// hand the measured duration to the optional <c>onOverrun</c> sink. Defensive against a
/// sink that throws, for the same reason <see cref="ReportError"/> is.
/// <para><b>Caveat — this CONFLATES a slow success with a slow failure.</b> It is called
/// from the <c>finally</c> 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 <c>onOverrun</c> with the
/// same duration. A consumer reading <c>otopcua.driver.poll.overrun</c> therefore learns
/// "the loop cannot keep its cadence", NOT "the device is healthy but slow" — pair it with
/// the driver's health surface (which <c>onError</c> 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.</para>
/// </summary>
private void NoteOverrun(TimeSpan interval, TimeSpan elapsed)
{
@@ -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));
}
/// <summary>
@@ -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));
}
/// <summary>
/// Register a fire-and-forget task so <see cref="Dispose"/> 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 <see cref="Dispose"/> can await it.
/// </summary>
private void TrackBackgroundTask(Task task)
/// <remarks>
/// <para><b>The factory shape is the point: registration happens BEFORE the work starts.</b>
/// Taking a <see cref="Task"/> meant the caller evaluated the argument — which starts the
/// work — and only then called in to register it. <see cref="Dispose"/> snapshots
/// <c>_inFlight</c> under <c>_inFlightLock</c> 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 <c>SaveAsync</c> 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.</para>
/// <para>The factory is invoked OUTSIDE <c>_inFlightLock</c> — it runs engine code
/// synchronously up to its first real await (and, via the injected clock, can re-enter
/// <see cref="OnUpstreamChange"/> and this method), none of which belongs under that lock.</para>
/// <para><b>Post-disposal the call DECLINES: the factory is never invoked and nothing is
/// registered</b> (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 <c>_drainScheduled</c> stuck true;
/// that is harmless, because a disposed engine drops pushes in
/// <see cref="OnUpstreamChange"/> and so never consults the latch again.</para>
/// </remarks>
/// <param name="factory">Starts the background work. Invoked at most once, outside the lock.</param>
private void TrackBackgroundTask(Func<Task> 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
/// </summary>
internal void RunShelvingCheckForTest() => RunShelvingCheck();
/// <summary>
/// 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.
/// </summary>
/// <param name="factory">Starts the background work.</param>
internal void TrackBackgroundTaskForTest(Func<Task> factory) => TrackBackgroundTask(factory);
/// <summary>Test hook — the number of background tasks currently registered for the Dispose drain.</summary>
internal int InFlightCountForTest
{
get { lock (_inFlightLock) { return _inFlight.Count; } }
}
private async Task ShelvingCheckAsync(IReadOnlyList<string> alarmIds, CancellationToken ct)
{
var pending = new List<ScriptedAlarmEvent>(0);
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.AbCip),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
_alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval, _logger);
}
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.AbLegacy),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
}
/// <summary>Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8).</summary>
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.FOCAS),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
}
/// <summary>True when the supplied DriverConfig JSON carries a real body. The bootstrapper always passes
@@ -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;
/// </item>
/// </list>
/// <para>
/// <b>The read is forward-only.</b> A <c>/sample</c> chunk is parsed once and thrown away,
/// so it is read straight off an <see cref="XmlReader"/> rather than through an
/// <see cref="XDocument"/>: on the pump's hot path the tree was pure garbage — one
/// <c>XElement</c> 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 <see cref="XDocument"/> dropped them (see
/// <see cref="ReaderSettings"/>), and that an observation's slot in the result list is
/// reserved <i>before</i> its subtree is read so observations still arrive in document
/// order. The one place a tree is still built is the cold non-<c>MTConnectStreams</c> root
/// below, where the Agent's own error text has to be lifted out of a document this parser
/// is otherwise finished with.
/// </para>
/// <para>
/// <b>One thing did change, and only for a document that is wrong twice over.</b> A missing
/// <c>&lt;Header&gt;</c> 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 <see cref="InvalidDataException"/>, and nothing that is wrong in
/// only one way reports differently than it did.
/// </para>
/// <para>
/// <b>Failure posture.</b> A malformed document, a non-<c>MTConnectStreams</c> root (an Agent
/// <c>MTConnectError</c> served under HTTP 200 included), an unusable header, or an
/// observation missing its <c>dataItemId</c>/<c>timestamp</c> all throw
@@ -87,10 +110,32 @@ internal static class MTConnectStreamsParser
private const string HeaderOwner = "the <Header>";
/// <summary>
/// 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.
/// </summary>
private static readonly string[] ObservationContainers = ["Samples", "Events", ConditionContainer];
/// <remarks>
/// <para>
/// <b><see cref="XmlReaderSettings.IgnoreWhitespace"/> reproduces
/// <see cref="XDocument"/>'s default.</b> <c>XDocument.Load</c>/<c>Parse</c> without
/// <see cref="LoadOptions.PreserveWhitespace"/> sets exactly this, so a whitespace-only
/// text node never became an <c>XText</c> and never reached <c>XElement.Value</c>.
/// Leaving it at the reader's own default would silently start folding indentation into
/// observation values that contain element children — <c>"1 2"</c> where the tree read
/// <c>"12"</c>.
/// </para>
/// <para>
/// <b>A DTD is prohibited</b> 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
/// <see cref="XDocument"/> defaulted to.
/// </para>
/// </remarks>
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
};
/// <summary>Parses a <c>/current</c> response, or one <c>/sample</c> chunk, held as a string.</summary>
/// <param name="xml">The raw <c>MTConnectStreams</c> XML document.</param>
@@ -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);
}
/// <summary>
@@ -128,7 +166,7 @@ internal static class MTConnectStreamsParser
/// <remarks>
/// The <c>/sample</c> 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; <see cref="XDocument"/>
/// once and thrown away. Parsing from the bytes drops both copies; <see cref="XmlReader"/>
/// does its own UTF-8 decoding off the stream. The framing tolerance the string overload's
/// <c>Trim</c> provided is preserved by <see cref="TrimFraming"/>, 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);
}
/// <summary>
@@ -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)
/// <summary>
/// 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.
/// </summary>
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 <Header>; 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));
}
/// <summary>
/// Collects every observation in the document, in Agent-supplied order. Containers are found
/// by descending the whole <c>&lt;Streams&gt;</c> subtree rather than by walking a fixed
/// DeviceStream → ComponentStream chain, so a vendor's extra nesting level cannot silently
/// hide a device's observations.
/// </summary>
private static IReadOnlyList<MTConnectObservation> ReadObservations(XElement root)
private static MTConnectStreamsResult ReadDocument(XmlReader reader)
{
var observations = new List<MTConnectObservation>();
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 <Error> 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<MTConnectObservation>();
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 <Header> 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 <Header>; 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);
/// <summary>
/// The three sequence numbers the sample pump runs on, read off the <c>&lt;Header&gt;</c>'s
/// start tag. Read eagerly, where the header is met, so an unusable header is still reported
/// ahead of anything the <c>&lt;Streams&gt;</c> that follows it might also be wrong about.
/// </summary>
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),
/// <summary>
/// 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 <i>parent</i> 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.
/// </summary>
/// <remarks>
/// <b>Cursor contract:</b> on return the reader sits on the element's
/// <see cref="XmlNodeType.EndElement"/>, or on the element itself when it was written empty
/// — either way the caller's next <see cref="XmlReader.Read"/> lands on whatever follows.
/// Every recursion below relies on that, which is why the children are walked by hand
/// rather than through <c>Skip</c>/<c>ReadSubtree</c>.
/// </remarks>
/// <param name="reader">A reader positioned on the element's start tag.</param>
/// <param name="parent">What the enclosing element is, which decides whether this one is an observation.</param>
/// <param name="observations">Collects the observations found, in document order.</param>
/// <param name="textSink">
/// The text accumulator of the nearest enclosing observation whose value is its text, or
/// <c>null</c> when there is none. Text found anywhere below an observation belongs to it —
/// that is what <c>XElement.Value</c> meant.
/// </param>
private static void ReadElement(
XmlReader reader,
ContainerKind parent,
List<MTConnectObservation> 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 <Entry key="…"> 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);
}
/// <summary>
/// Walks a non-empty element's children through its <see cref="XmlNodeType.EndElement"/>,
/// reporting whether any of them was an element.
/// </summary>
private static bool ReadChildren(
XmlReader reader,
ContainerKind kind,
List<MTConnectObservation> 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;
}
/// <summary>
@@ -285,16 +472,12 @@ internal static class MTConnectStreamsParser
/// token to recognize; every other state is reported as-named (<c>Normal</c>, <c>Warning</c>,
/// <c>Fault</c>, and any vendor state).
/// </summary>
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));
/// <summary>
/// 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.
/// </summary>
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;
}
/// <summary>What an element's children are: nothing special, valued observations, or conditions.</summary>
private enum ContainerKind
{
/// <summary>Not an observation container; its element children are just more structure.</summary>
None,
/// <summary><c>&lt;Samples&gt;</c> or <c>&lt;Events&gt;</c> — children take their value from their text.</summary>
Values,
/// <summary><c>&lt;Condition&gt;</c> — children take their value from their element NAME.</summary>
Condition
}
/// <summary>The three sequence numbers read off a <c>&lt;Header&gt;</c>.</summary>
private readonly record struct HeaderSequences(long InstanceId, long NextSequence, long FirstSequence);
/// <summary>
/// Accumulates one observation's text across however many nodes the parser hands it, without
/// a <see cref="StringBuilder"/> for the single-text-node shape every ordinary scalar
/// observation has — which is the shape this parser sees thousands of times a second.
/// </summary>
private sealed class ValueSink
{
private string? _single;
private StringBuilder? _builder;
/// <summary>Everything appended so far, untrimmed — <c>XElement.Value</c>'s concatenation.</summary>
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;
}
}
}
@@ -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);
/// <summary>
/// Does the element the reader is positioned on have the given local name, whatever its
/// namespace?
/// </summary>
/// <remarks>
/// <see cref="MTConnectStreamsParser"/> reads its documents through a forward-only
/// <see cref="XmlReader"/> rather than a materialised tree — one sample chunk is read once
/// and thrown away, so building an <see cref="XDocument"/> 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.
/// </remarks>
/// <param name="reader">A reader positioned on an element.</param>
/// <param name="localName">The unqualified name to match.</param>
public static bool IsNamed(XmlReader reader, string localName) =>
string.Equals(reader.LocalName, localName, StringComparison.Ordinal);
/// <summary>Direct element children of <paramref name="parent"/> with the given local name.</summary>
/// <param name="parent">The element whose children to filter.</param>
/// <param name="localName">The unqualified name to match.</param>
@@ -52,19 +70,46 @@ internal static class MTConnectXml
return string.IsNullOrWhiteSpace(value) ? null : value;
}
/// <summary>
/// Reads an unqualified attribute off the element the reader is positioned on, normalizing a
/// present-but-empty value to <c>null</c>. See <see cref="IsNamed(XmlReader, string)"/> for
/// why the reader overloads exist.
/// </summary>
/// <remarks>
/// <see cref="XmlReader.GetAttribute(string)"/> matches on the attribute's <i>qualified</i>
/// name, so a prefixed <c>x:dataItemId</c> does not answer to <c>dataItemId</c> — exactly
/// what rule 2 in the type-level remarks requires, and what <see cref="XElement.Attribute"/>
/// does for a namespace-free <see cref="XName"/>.
/// </remarks>
/// <param name="reader">A reader positioned on an element.</param>
/// <param name="name">The unqualified attribute name.</param>
public static string? OptionalAttribute(XmlReader reader, string name)
{
var value = reader.GetAttribute(name);
return string.IsNullOrWhiteSpace(value) ? null : value;
}
/// <summary>Reads a required unqualified attribute, or throws naming the owner and the attribute.</summary>
/// <param name="element">The element carrying the attribute.</param>
/// <param name="name">The unqualified attribute name.</param>
/// <param name="owner">How to describe the element in the error message.</param>
/// <param name="subject">How to describe the document in the error message (e.g. "/probe response").</param>
/// <exception cref="InvalidDataException">The attribute is absent or blank.</exception>
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.");
}
/// <summary>
/// Reads a required unqualified attribute off the element the reader is positioned on, or
/// throws naming the owner and the attribute.
/// </summary>
/// <param name="reader">A reader positioned on an element.</param>
/// <param name="name">The unqualified attribute name.</param>
/// <param name="owner">How to describe the element in the error message.</param>
/// <param name="subject">How to describe the document in the error message.</param>
/// <exception cref="InvalidDataException">The attribute is absent or blank.</exception>
public static string RequiredAttribute(XmlReader reader, string name, string owner, string subject) =>
OptionalAttribute(reader, name) ?? throw MissingAttribute(name, owner, subject);
/// <summary>Reads an optional unqualified attribute as an <see cref="int"/>.</summary>
/// <param name="element">The element carrying the attribute.</param>
@@ -95,10 +140,28 @@ internal static class MTConnectXml
/// <param name="owner">How to describe the element in the error message.</param>
/// <param name="subject">How to describe the document in the error message.</param>
/// <exception cref="InvalidDataException">The attribute is absent, blank, or not an integer.</exception>
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);
/// <summary>
/// Reads a required unqualified attribute off the element the reader is positioned on, as a
/// <see cref="long"/>.
/// </summary>
/// <param name="reader">A reader positioned on an element.</param>
/// <param name="name">The unqualified attribute name.</param>
/// <param name="owner">How to describe the element in the error message.</param>
/// <param name="subject">How to describe the document in the error message.</param>
/// <exception cref="InvalidDataException">The attribute is absent, blank, or not an integer.</exception>
public static long RequiredLongAttribute(XmlReader reader, string name, string owner, string subject) =>
AsLong(RequiredAttribute(reader, name, owner, subject), name, owner, subject);
/// <summary>The one spelling of "that attribute is missing", shared by both flavours.</summary>
private static InvalidDataException MissingAttribute(string name, string owner, string subject) =>
new($"MTConnect {subject} is malformed: {owner} has no '{name}' attribute.");
/// <summary>The one spelling of "that attribute is not a number", shared by both flavours.</summary>
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(
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.Modbus),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
}
/// <summary>Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8).</summary>
@@ -88,10 +88,11 @@ public sealed class S7DriverOptions
/// </summary>
/// <remarks>
/// <para>
/// <b>Any value <c>&lt;= -1</c> disables span grouping entirely</b> — 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.
/// <b><c>-1</c> disables span grouping entirely</b> — 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.
/// <c>-1</c> is the floor: <c>S7DriverFactoryExtensions.ParseOptions</c> rejects anything
/// below it, so a config cannot express "disabled" by accident with an arbitrary negative.
/// </para>
/// <para>
/// 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.
/// <para>
/// There is deliberately <b>no upper bound</b>: 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. <c>S7DriverFactoryExtensions.ParseOptions</c> therefore validates only the
/// lower bound (<c>&gt;= 1</c>); a non-positive cap can never admit a single block read.
/// </para>
/// </remarks>
[Display(Name = "Span read max span (bytes)",
Description = "Largest single planned block read. Bounds wasted-gap amplification, not correctness.",
@@ -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
/// <returns>The raw byte block.</returns>
Task<byte[]> ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken);
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// ⚠️ <b>The CALLER owns chunking.</b> 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 <see cref="S7DataItem.Value"/> in place and hands the same list
/// back; the returned list is the authoritative result and is ordered as supplied.
/// </remarks>
/// <param name="items">The items to read; each carries its area / DB / offset / var-type and receives its value.</param>
/// <param name="cancellationToken">Cancellation token for the read.</param>
/// <returns>The read items, in the supplied order, with their values populated.</returns>
Task<IReadOnlyList<S7DataItem>> ReadMultipleVarsAsync(List<S7DataItem> items, CancellationToken cancellationToken);
/// <summary>Writes a scalar value by S7 address string; the value must be the unsigned wire type for the address size.</summary>
/// <param name="address">The S7 address string.</param>
/// <param name="value">The wire-typed value to write.</param>
@@ -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);
/// <inheritdoc />
public async Task<IReadOnlyList<S7DataItem>> ReadMultipleVarsAsync(List<S7DataItem> items, CancellationToken cancellationToken)
{
ArgumentNullException.ThrowIfNull(items);
// S7.Net returns the SAME list with each item's Value populated; await + widen because
// Task<List<T>> is not assignable to Task<IReadOnlyList<T>>.
return await S7OperationDeadline.RunAsync(
t => _plc.ReadMultipleVarsAsync(items, t),
_operationTimeout, $"multi-var read of {items.Count} item(s)", cancellationToken).ConfigureAwait(false);
}
/// <inheritdoc />
public Task WriteAsync(string address, object value, CancellationToken cancellationToken) =>
S7OperationDeadline.RunAsync(
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.S7),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
}
// ---- ISubscribable + IHostConnectivityProbe state ----
@@ -508,11 +515,19 @@ public sealed class S7Driver
/// <c>ReadBytesAsync</c>, 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 <see cref="PrefersPerTagRead"/>) then goes through the unchanged
/// worth issuing (see <see cref="PrefersPerTagRead"/>) falls to the leftover list.
/// </para>
/// <para>
/// Those leftovers get a SECOND grouping pass: the ones a
/// <see cref="S7DataItem"/> expresses cleanly — the narrow scalars, which is what a
/// demoted single-member span is — are batched into multi-var requests by
/// <see cref="ReadLeftoversByMultiVarAsync"/>, which crosses <c>(area, DB)</c>
/// 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 <see cref="ReadOneAsync"/> path.
/// </para>
/// <para>
/// Setting <c>SpanReadMaxGapBytes</c> below zero skips the first pass entirely, which
/// Setting <c>SpanReadMaxGapBytes</c> 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 <see cref="RecordReadFailure"/> 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;
}
/// <summary>
/// Items one multi-var request may carry. <b>The caller owns chunking — S7.Net does NOT
/// split a multi-var read</b>, it throws when the request or the projected response exceeds
/// the negotiated PDU (<c>AssertPduSizeForRead</c>).
/// </summary>
/// <remarks>
/// The request costs a 19-byte header plus <b>12 bytes per item</b>; 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.
/// </remarks>
private const int MultiVarReadMaxItems = 12;
/// <summary>
/// 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
/// <see cref="PrefersPerTagRead"/> applies to a lone narrow span.
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
private const int MultiVarReadMinItems = 2;
/// <summary>
/// Serves the span planner's leftovers that a <see cref="S7DataItem"/> can express by
/// batching them into multi-var requests, and returns the indexes still owed an individual
/// <see cref="ReadOneAsync"/> — in ascending reference order.
/// </summary>
/// <remarks>
/// <para>
/// <b>Only a narrow scalar is batched.</b> A <see cref="S7DataItem"/> carrying the same
/// area / DB / offset / bit / <c>VarType</c> that S7.Net's own address parser derives
/// from the tag's address string, with <c>Count == 1</c>, is decoded by the SAME
/// <c>ParseBytes</c> call the per-address read path uses — so its boxed value is
/// byte-for-byte and type-for-type what <see cref="ReadOneAsync"/> would have produced,
/// and <see cref="ReinterpretRawValue"/> applies to it unchanged.
/// </para>
/// <para>
/// <b>Timer and Counter deliberately stay on the per-tag path</b> even though S7.Net's
/// <c>VarType</c> names them. Their per-tag path does not go through <c>ParseBytes</c>
/// at all — it block-reads the 2-byte word and decodes it in
/// <see cref="DecodeScalarBlock"/>, which surfaces a Counter as <c>int</c> where
/// S7.Net's own <c>Counter.FromByteArray</c> returns <c>ushort</c>, 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.
/// </para>
/// <para>
/// <b>A multi-var fault is never a tag's fault.</b> 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
/// <see cref="RecordReadFailure"/> is deliberately NOT called for the request itself.
/// S7.Net offers no per-item error surface to be finer-grained with: a
/// <see cref="S7DataItem"/> carries only its <c>Value</c>, 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.
/// </para>
/// </remarks>
/// <param name="plc">The live connection (caller holds <see cref="_gate"/>).</param>
/// <param name="resolved">The batch's resolved tags, indexed by reference position.</param>
/// <param name="leftovers">Indexes the span pass left for the per-tag path, ascending.</param>
/// <param name="results">The batch's result array, written in place at each tag's original index.</param>
/// <param name="now">The batch timestamp, shared by every tag in the batch.</param>
/// <param name="ct">Cancellation token for the reads.</param>
/// <returns>The indexes still owed an individual read, ascending.</returns>
private async Task<List<int>> ReadLeftoversByMultiVarAsync(
IS7Plc plc,
S7TagDefinition?[] resolved,
List<int> leftovers,
DataValueSnapshot[] results,
DateTime now,
CancellationToken ct)
{
var stillPerTag = new List<int>();
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<S7DataItem> answered;
try
{
var items = new List<S7DataItem>(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;
}
/// <summary>
/// Builds the multi-var request item for one narrow scalar — the exact
/// <c>(DataType, DB, StartByteAdr, BitAdr, VarType, Count)</c> tuple S7.Net's own address
/// parser derives from the equivalent address string, so the item reads and decodes
/// identically to <see cref="ReadOneAsync"/>'s per-address read.
/// </summary>
/// <param name="addr">The tag's parsed address; its <see cref="S7Size"/> picks the var type.</param>
/// <returns>The request item, with no value yet.</returns>
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,
};
/// <summary>
/// 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.
@@ -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
/// <summary>Gets the probe configuration.</summary>
public S7ProbeDto? Probe { get; init; }
/// <summary>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.</summary>
/// block request; <c>-1</c> disables block-read grouping and anything below it is rejected.
/// Null keeps the default.</summary>
public int? SpanReadMaxGapBytes { get; init; }
/// <summary>Gets the largest single planned block read in bytes. Null keeps the default.</summary>
public int? SpanReadMaxSpanBytes { get; init; }
@@ -24,6 +24,11 @@
<ItemGroup>
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.S7.Tests"/>
<!-- The live span-read gate (S7_1500SpanReadTests) wraps the REAL S7PlcFactory in a counting
decorator so it can assert the wire shape the driver actually produces against a live PLC.
Decorating the shipped factory (rather than re-implementing IS7Plc in the test) keeps
S7PlcAdapter + S7OperationDeadline on the wire, so the gate measures production's path. -->
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests"/>
</ItemGroup>
</Project>
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.Sql),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
}
/// <summary>
@@ -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<string, object?>(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.TwinCAT),
new KeyValuePair<string, object?>(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)));
}
/// <summary>Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8).</summary>
@@ -1448,6 +1448,9 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> 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<string, string>(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)];
@@ -3298,9 +3298,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// identifier spaces, so an identifier alone does not name a node.
/// </para>
/// <para>
/// 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
/// <see cref="Security.DataPlaneAuthorizer"/> excludes it from its own decision cache: it
/// addresses no ACL scope, so it is not an identity to key on.
/// </para>
/// <para>
/// 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);
@@ -56,8 +56,12 @@ public sealed class SessionAclDecisionCache
/// Enforced against an <see cref="Interlocked"/> counter rather than
/// <see cref="ConcurrentDictionary{TKey,TValue}.Count"/>, 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 <b>booked</b> entry is unbooked exactly once, when the shard
/// holding it is sealed. (Not every entry is booked: one whose <see cref="TryBookInsert"/> loses
/// the compare-exchange to a seal is never booked at all, because it is already unreachable in a
/// displaced shard.) See <see cref="SealedShard"/> for how that survives an eviction racing an
/// insert.
/// </remarks>
public const int MaxEntries = 100_000;
@@ -77,6 +81,18 @@ public sealed class SessionAclDecisionCache
/// </remarks>
public const int MaxEntriesPerSession = 20_000;
/// <summary>
/// Sentinel written over a shard's <see cref="Shard.ApproxCount"/> 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
/// <see cref="Interlocked.Exchange(ref int, int)"/> that captures the count being subtracted from
/// <see cref="_globalCount"/>, so an insert either books its increment <i>before</i> 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.
/// </summary>
private const int SealedShard = int.MinValue;
private readonly ConcurrentDictionary<string, Shard> _shards = new(StringComparer.Ordinal);
private long _generation = -1;
private int _globalCount;
@@ -99,6 +115,29 @@ public sealed class SessionAclDecisionCache
}
}
/// <summary>
/// The running counter both ceilings are enforced against. Exposed for the concurrency audit only:
/// the property a test wants is almost always <see cref="Count"/>, and the single interesting fact
/// about this one is that the two agree.
/// </summary>
internal int TrackedCount => Volatile.Read(ref _globalCount);
/// <summary>
/// Sum of the live shards' own counters — the middle term of the invariant
/// <c>TrackedCount == LiveShardCount == Count</c>. 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.
/// </summary>
internal int LiveShardCount
{
get
{
var total = 0;
foreach (var shard in _shards.Values) total += Volatile.Read(ref shard.ApproxCount);
return total;
}
}
/// <summary>
/// Looks a decision up without allocating anything — the hot half, and the reason the store half is
/// a separate method rather than a <see cref="Func{TResult}"/> 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);
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// A plain <see cref="Interlocked.Increment(ref int)"/> here would race the seal in the other
/// direction: an increment landing after the displacing thread captured the count would leave
/// <see cref="_globalCount"/> 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
/// <see cref="SealedShard"/> and books nothing. The loop retries only against other <i>inserts</i>
/// on the same shard; the sealed case exits.
/// <para>
/// The shard's counter is moved <b>before</b> the global one, and that order matters: a seal
/// landing between the two subtracts an increment the global counter has not received yet, so
/// <see cref="_globalCount"/> can dip transiently below the live total. That is self-healing —
/// the thread in the gap always completes its increment — and it errs towards <i>not</i>
/// 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.
/// </para>
/// </remarks>
/// <param name="shard">The shard the entry was added to.</param>
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;
}
}
/// <summary>
/// Retires a shard that has just been removed from (or replaced in) <see cref="_shards"/> and
/// returns the number of entries it was still accounted for, for the caller to subtract from
/// <see cref="_globalCount"/>.
/// </summary>
/// <remarks>
/// Only the thread that actually displaced the shard may call this — <see cref="_shards"/>'
/// 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.
/// </remarks>
/// <param name="shard">The displaced shard.</param>
/// <returns>The count to subtract from <see cref="_globalCount"/>.</returns>
private static int Seal(Shard shard)
{
var captured = Interlocked.Exchange(ref shard.ApproxCount, SealedShard);
return captured < 0 ? 0 : captured;
}
/// <summary>
/// Returns the memoized decision for this key, computing and storing it when absent or when the
/// generation has moved on. A convenience over <see cref="TryGet"/> + <see cref="Set"/>.
@@ -239,24 +350,43 @@ public sealed class SessionAclDecisionCache
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// Each shard is removed and then <see cref="Seal"/>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 <i>fresh</i> 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.
/// </remarks>
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));
}
/// <summary>One session's decisions, plus the approximate entry count its own ceiling is enforced on.</summary>
/// <summary>
/// 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
/// <see cref="_shards"/> either still is the live shard or has been sealed — it can never turn
/// into a <i>different</i> generation of the same session's entries under its holder.
/// </summary>
private sealed class Shard
{
public readonly ConcurrentDictionary<ShardKey, bool> Entries = new();
/// <summary>Approximate by design — read and written with <see cref="Interlocked"/>, never derived
/// from <see cref="ConcurrentDictionary{TKey,TValue}.Count"/> on the store path.</summary>
/// <summary>
/// Read and written with <see cref="Interlocked"/>, never derived from
/// <see cref="ConcurrentDictionary{TKey,TValue}.Count"/> on the store path. The name is
/// historical: while the shard is live this tracks <see cref="Entries"/> exactly (entries are
/// only ever added, and every add books here), and once displaced it holds
/// <see cref="SealedShard"/>. It is <i>lagging</i> — an insert is visible in
/// <see cref="Entries"/> a moment before it is booked here — but never wrong.
/// </summary>
public int ApproxCount;
}
@@ -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)
@@ -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;
}
@@ -670,6 +670,50 @@ public sealed class PollGroupEngineTests
overruns.Count.ShouldBeLessThanOrEqualTo(1);
}
/// <summary>
/// The counter and the callback must be the SAME signal, not two approximations of it: every
/// increment of <see cref="PollGroupEngine.OverrunCount"/> is matched by exactly one
/// <c>onOverrun</c> invocation. This is what lets a driver's meter (the production consumer,
/// <c>OtOpcUaTelemetry.PollOverruns</c>) be read as the counter itself — a callback that
/// fired on only some increments, or twice on one, would silently mis-scale the metric.
/// </summary>
/// <remarks>
/// The two existing pacing tests
/// (<see cref="A_slow_reader_overrun_is_counted_and_the_next_poll_starts_promptly"/> /
/// <see cref="A_fast_reader_never_overruns"/>) 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
/// <c>UnsubscribeAsync</c> 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.
/// </remarks>
[Fact]
public async Task Every_overrun_increment_is_matched_by_exactly_one_onOverrun_callback()
{
async Task<IReadOnlyList<DataValueSnapshot>> Reader(IReadOnlyList<string> 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<TimeSpan>();
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);
}
/// <summary>
/// The async teardown must be behaviourally identical to the sync one: the handle is known,
/// the loop stops, and no further callback fires. Mirrors
@@ -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.
// -------------------------------------------------------------------------
/// <summary>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.</summary>
[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");
}
/// <summary>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.</summary>
[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");
}
/// <summary>Verifies Dispose blocks on work started through the factory form, and that the
/// registration is released once that work completes.</summary>
[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");
}
/// <summary>Verifies work whose task is already complete when the factory returns still
/// unregisters cleanly and leaves Dispose with nothing to wait on.</summary>
[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());
}
/// <summary>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.</summary>
[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<InvalidOperationException>(() => 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));
}
/// <summary>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.</summary>
[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);
}
/// <summary>
/// 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 —
@@ -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<Guid[]> 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
@@ -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(
"""
<Events><PathFeedrateOverride dataItemId="ds" timestamp="2026-07-24T12:00:00Z" count="2">
<Entry key="V1">1</Entry>
<Entry key="V2">2</Entry>
</PathFeedrateOverride></Events>
"""));
result.Observations.ShouldHaveSingleItem().Value.ShouldBe("12");
}
[Fact]
public void A_plain_text_observation_containing_spaces_is_NOT_flagged_structured()
{
@@ -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=<n>` at startup — check `<n>` 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:
@@ -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 }
]
}
]
@@ -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;
/// <summary>
/// Live gate for the read-batching layer: the <see cref="S7ReadSpanPlanner"/> 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.
/// </summary>
/// <remarks>
/// <para>
/// <b>Live-gate record.</b> Run 2026-08-17 against <c>10.100.0.35:1102</c>
/// (<c>otopcua-python-snap7-s7_1500</c>, profile <c>Docker/profiles/s7_1500.json</c>):
/// <b>PASSED</b> — 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.
/// </para>
/// <para>
/// <b>What the layout buys.</b> The ten tags are positioned so the planner's rules produce a
/// known, non-trivial partition rather than one that would pass by accident:
/// </para>
/// <list type="bullet">
/// <item>
/// <b>Cluster A</b> (<c>DB1.DBW300 … DB1.DBD316</c>) — 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 (<c>0xDEAD</c>, <c>0xDEADBEEF</c>) so a mis-computed
/// slice offset yields garbage rather than a zero that could coincidentally match.
/// </item>
/// <item>
/// <b>Cluster B</b> (<c>DB1.DBW400</c> + <c>DB1.DBW402</c>) — 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 <c>PrefersPerTagRead</c> demotion, so it is a real block read.
/// </item>
/// <item>
/// <b>Leftovers</b> (<c>DB1.DBW500</c>, <c>DB1.DBW600</c>, <c>MW10</c>) — each isolated
/// enough to form its own single-member span, each therefore demoted by
/// <c>PrefersPerTagRead</c>, and three of them is past <c>MultiVarReadMinItems</c> — so
/// they ride ONE multi-var request. <c>MW10</c> is deliberately in a different area:
/// a block read can never merge across <c>(area, DB)</c>, and the multi-var pass can,
/// so its presence in the single request is what pins that property live.
/// </item>
/// </list>
/// <para>
/// <b>Timer / Counter are NOT covered here.</b> 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 <c>IsBufferType</c>, two
/// independent guards.
/// </para>
/// <para>
/// <b>Why a counting factory rather than logs.</b> 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.
/// <see cref="CountingPlcFactory"/> decorates the REAL <see cref="S7PlcFactory"/>, so the
/// shipped <c>S7PlcAdapter</c> (and its <c>S7OperationDeadline</c> ceiling) stays on the
/// wire — the gate measures production's path, not a re-implementation of it.
/// </para>
/// </remarks>
[Collection(Snap7ServerCollection.Name)]
[Trait("Category", "Integration")]
[Trait("Device", "S7_1500")]
public sealed class S7_1500SpanReadTests(Snap7ServerFixture sim)
{
/// <summary>
/// Verifies the grouped batch returns exactly what reading every tag individually returns —
/// same values, same boxed CLR types, all Good.
/// </summary>
/// <remarks>
/// Boxed equality is the assertion on purpose: the three paths decode through different code
/// (block slicing, <c>S7.Net.ParseBytes</c> 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.
/// </remarks>
[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");
}
}
/// <summary>
/// Verifies the grouped batch decodes the fixture's seeded values — so both paths agreeing
/// cannot mean "both wrong the same way".
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
[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);
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// 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
/// <c>SpanReadMaxGapBytes &lt; 0</c> is documented to restore the pre-grouping wire shape
/// EXACTLY, and "exactly" is a claim about requests, not values.
/// </remarks>
[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);
}
/// <summary>Reads the whole layout once through a freshly-built driver and reports the wire shape.</summary>
/// <param name="maxGapBytes">The span budget: the shipped default, or a negative value to disable grouping.</param>
/// <param name="driverInstanceId">Instance id, distinct per call so log lines stay attributable.</param>
/// <returns>The batch's snapshots plus the wire-call counts observed for the read itself.</returns>
private async Task<(IReadOnlyList<DataValueSnapshot> 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);
}
/// <summary>
/// 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 <c>Docker/profiles/s7_1500.json</c>, so a seed drift there surfaces here as a value
/// mismatch rather than a mystery failure.
/// </summary>
private static class SpanLayout
{
/// <summary>The shipped gap budget — the value this gate exists to validate.</summary>
public const int DefaultGapBytes = S7DriverOptions.DefaultSpanReadMaxGapBytes;
/// <summary>Any negative gap disables BOTH grouping passes; the documented off switch.</summary>
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),
];
/// <summary>The batch's references, in the order the driver is asked for them.</summary>
public static readonly IReadOnlyList<string> TagNames = [.. Definitions.Select(d => d.Name)];
/// <summary>Builds driver options for the layout at the requested span budget.</summary>
/// <param name="host">The fixture hostname or IP address.</param>
/// <param name="port">The fixture port.</param>
/// <param name="maxGapBytes">Span gap budget; negative disables grouping.</param>
/// <returns>The driver options.</returns>
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),
};
}
/// <summary>
/// <see cref="IS7PlcFactory"/> 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.
/// </summary>
/// <param name="inner">The factory producing the real connection.</param>
private sealed class CountingPlcFactory(IS7PlcFactory inner) : IS7PlcFactory
{
/// <summary>Block reads issued — one per planned span.</summary>
public int BlockReads { get; private set; }
/// <summary>Multi-var requests issued — one per leftover chunk.</summary>
public int MultiVarReads { get; private set; }
/// <summary>Items carried across all multi-var requests.</summary>
public int MultiVarItems { get; private set; }
/// <summary>Per-address reads issued — the unchanged per-tag path.</summary>
public int AddressReads { get; private set; }
/// <summary>Total read round-trips, whichever shape they took.</summary>
public int TotalReadRequests => BlockReads + MultiVarReads + AddressReads;
/// <summary>Zeroes the counters, so a batch can be measured without initialization's calls.</summary>
public void Reset()
{
BlockReads = 0;
MultiVarReads = 0;
MultiVarItems = 0;
AddressReads = 0;
}
/// <inheritdoc />
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++;
/// <summary>Counting pass-through over one real connection.</summary>
/// <param name="inner">The real connection.</param>
/// <param name="owner">The factory holding the counters.</param>
private sealed class CountingPlc(IS7Plc inner, CountingPlcFactory owner) : IS7Plc
{
/// <inheritdoc />
public bool IsConnected => inner.IsConnected;
/// <inheritdoc />
public Task OpenAsync(CancellationToken cancellationToken) => inner.OpenAsync(cancellationToken);
/// <inheritdoc />
public void Close() => inner.Close();
/// <inheritdoc />
public Task<object?> ReadAsync(string address, CancellationToken cancellationToken)
{
owner.CountAddressRead();
return inner.ReadAsync(address, cancellationToken);
}
/// <inheritdoc />
public Task<byte[]> ReadBytesAsync(
S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken)
{
owner.CountBlockRead();
return inner.ReadBytesAsync(area, db, startByteAdr, count, cancellationToken);
}
/// <inheritdoc />
public Task<IReadOnlyList<S7DataItem>> ReadMultipleVarsAsync(
List<S7DataItem> 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);
}
/// <inheritdoc />
public Task WriteAsync(string address, object value, CancellationToken cancellationToken)
=> inner.WriteAsync(address, value, cancellationToken);
/// <inheritdoc />
public Task WriteBytesAsync(
S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken)
=> inner.WriteBytesAsync(area, db, startByteAdr, value, cancellationToken);
/// <inheritdoc />
public Task ReadStatusAsync(CancellationToken cancellationToken) => inner.ReadStatusAsync(cancellationToken);
/// <inheritdoc />
public void Dispose() => inner.Dispose();
}
}
}
@@ -569,6 +569,10 @@ public sealed class S7DriverReconnectTests
public Task<byte[]> 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<IReadOnlyList<global::S7.Net.Types.DataItem>> ReadMultipleVarsAsync(
List<global::S7.Net.Types.DataItem> 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)
@@ -47,6 +47,10 @@ public sealed class S7PollEngineMigrationTests
return Task.FromResult(block);
}
public Task<IReadOnlyList<global::S7.Net.Types.DataItem>> ReadMultipleVarsAsync(
List<global::S7.Net.Types.DataItem> 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;
@@ -0,0 +1,96 @@
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests;
/// <summary>
/// Config-time guard on the two span-read knobs. There is no <c>IValidateOptions</c> machinery
/// for S7 — <see cref="S7DriverFactoryExtensions.ParseOptions"/> 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.
/// </summary>
/// <remarks>
/// The legal-value cases matter as much as the rejections: <c>-1</c> on the gap is the documented
/// escape hatch that disables grouping entirely (<c>S7Driver</c> enables spans on <c>&gt;= 0</c>),
/// and <c>0</c> means "tolerate no gap at all", i.e. merge only strictly adjacent tags. A guard
/// written as <c>&lt; 0</c> would break both.
/// </remarks>
public sealed class S7SpanOptionValidationTests
{
/// <summary>A driver-config blob carrying the given span knob, host included (it is required).</summary>
private static string Config(string knob, int value) =>
"{\"host\":\"10.0.0.1\",\"" + knob + "\":" + value.ToString(System.Globalization.CultureInfo.InvariantCulture) + "}";
/// <summary>A span cap below one can never admit a single planned block read, so it is rejected.</summary>
[Theory]
[InlineData(0)]
[InlineData(-1)]
[InlineData(-220)]
public void A_span_cap_below_one_is_rejected(int maxSpanBytes)
{
var ex = Should.Throw<InvalidOperationException>(() => S7DriverFactoryExtensions.ParseOptions(
"s7-cfg", Config("spanReadMaxSpanBytes", maxSpanBytes)));
ex.Message.ShouldContain("SpanReadMaxSpanBytes");
ex.Message.ShouldContain("s7-cfg");
}
/// <summary>A gap budget below the grouping-disabled sentinel is rejected.</summary>
[Theory]
[InlineData(-2)]
[InlineData(-16)]
public void A_gap_budget_below_minus_one_is_rejected(int maxGapBytes)
{
var ex = Should.Throw<InvalidOperationException>(() => S7DriverFactoryExtensions.ParseOptions(
"s7-cfg", Config("spanReadMaxGapBytes", maxGapBytes)));
ex.Message.ShouldContain("SpanReadMaxGapBytes");
ex.Message.ShouldContain("s7-cfg");
}
/// <summary>
/// The two legal edge values survive: <c>-1</c> (grouping disabled) and <c>0</c> (no gap
/// tolerated). Both are documented behaviour, not degenerate input.
/// </summary>
[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);
}
/// <summary>A one-byte span cap is the smallest legal cap and must not trip the guard.</summary>
[Fact]
public void The_smallest_legal_span_cap_is_accepted()
{
var parsed = S7DriverFactoryExtensions.ParseOptions("s7-cfg", Config("spanReadMaxSpanBytes", 1));
parsed.SpanReadMaxSpanBytes.ShouldBe(1);
}
/// <summary>
/// An over-PDU span cap is legal — S7.Net transparently chunks an oversized <c>ReadBytes</c>
/// 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).
/// </summary>
[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);
}
/// <summary>Omitting both knobs keeps the shipped defaults, which must themselves pass the guard.</summary>
[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);
}
}
@@ -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));
/// <summary>
/// <paramref name="count"/> 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.
/// </summary>
/// <param name="count">How many scattered words to author.</param>
/// <returns>The raw-tag entries.</returns>
private static IReadOnlyList<RawTagEntry> ScatteredWords(int count) => S7RawTags.Entries(
[.. Enumerable.Range(0, count).Select(i =>
new S7TagDefinition($"S{i}", $"DB1.DBW{i * 8}", S7DataType.UInt16, Writable: false))]);
/// <summary>References for <see cref="ScatteredWords"/>, in authoring order.</summary>
/// <param name="count">How many scattered words the batch reads.</param>
/// <returns>The reference names.</returns>
private static string[] ScatteredRefs(int count) => [.. Enumerable.Range(0, count).Select(i => $"S{i}")];
/// <summary>
/// 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
/// (<c>bool</c> / <c>byte</c> / <c>ushort</c> / <c>short</c> / <c>uint</c> / <c>int</c> /
/// <c>float</c>) is exercised through it.
/// </summary>
private static IReadOnlyList<RawTagEntry> 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));
/// <summary>References for <see cref="ScatteredWidths"/>, in authoring order.</summary>
private static readonly string[] ScatteredWidthRefs =
["WBit", "WByte", "WU16", "WI16", "WU32", "WI32", "WF32", "WMem"];
// ── Parity ───────────────────────────────────────────────────────────────────────────
/// <summary>
@@ -120,11 +158,11 @@ public sealed class S7SpanReadParityTests
/// read.
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
[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 ──────────────────────────────────────────────────────────────
/// <summary>
/// The batchable leftovers ride ONE multi-var request per chunk of
/// <c>MultiVarReadMaxItems</c> 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.
/// </summary>
[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]);
}
/// <summary>
/// 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.
/// </summary>
[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);
}
/// <summary>
/// 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.
/// </summary>
[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);
}
/// <summary>
/// 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.
/// </summary>
[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<double>();
results[1].Value.ShouldBeOfType<int>();
// 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 ──────────────────────────────────────────────────────────────────
/// <summary>
@@ -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
/// <summary>Number of per-address string reads issued — the un-grouped narrow path.</summary>
public int StringReadCalls;
/// <summary>Number of multi-var requests issued — one per leftover chunk.</summary>
public int MultiVarCalls;
/// <summary>The items of every multi-var request, in issue order, so a test can assert what was batched.</summary>
public List<IReadOnlyList<global::S7.Net.Types.DataItem>> MultiVarItems { get; } = new();
/// <summary>When set, every read throws this instead of serving the image.</summary>
public Exception? ReadFault { get; init; }
/// <summary>When set, only the MULTI-VAR read throws this — the per-tag fallback still serves the image.</summary>
public Exception? MultiVarFault { get; init; }
/// <inheritdoc />
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)]);
}
/// <inheritdoc />
/// <remarks>
/// Decodes each item from the same byte image the other two read shapes serve, boxing
/// exactly as S7.Net's <c>ParseBytes</c> does for <c>Count == 1</c>: <c>Bit</c> a
/// <c>bool</c> at <c>BitAdr</c>, <c>Byte</c> a <c>byte</c>, <c>Word</c> a
/// <c>ushort</c>, <c>DWord</c> a <c>uint</c>. Like the real library it populates each
/// item's <c>Value</c> in place and hands the same list back.
/// </remarks>
public Task<IReadOnlyList<global::S7.Net.Types.DataItem>> ReadMultipleVarsAsync(
List<global::S7.Net.Types.DataItem> 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<IReadOnlyList<global::S7.Net.Types.DataItem>>(items);
}
/// <inheritdoc />
public Task<object?> ReadAsync(string address, CancellationToken ct)
{
@@ -92,6 +92,10 @@ internal sealed class ConnectingFakeS7PlcFactory : IS7PlcFactory
public Task<byte[]> ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken ct)
=> throw new NotSupportedException("ConnectingFakeS7Plc serves no data");
/// <summary>Not exercised by these tests.</summary>
public Task<IReadOnlyList<global::S7.Net.Types.DataItem>> ReadMultipleVarsAsync(
List<global::S7.Net.Types.DataItem> items, CancellationToken ct)
=> throw new NotSupportedException("ConnectingFakeS7Plc serves no data");
/// <summary>Not exercised by these tests.</summary>
public Task WriteAsync(string address, object value, CancellationToken ct)
=> throw new NotSupportedException("ConnectingFakeS7Plc serves no data");
/// <summary>Not exercised by these tests.</summary>
@@ -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;
/// <summary>
/// A real SQL Server database for the handful of AdminUI tests whose subject is the SQL Server
/// provider's LINQ <b>translation</b> 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.
/// </summary>
/// <remarks>
/// <para>
/// Shaped after <c>ZB.MOM.WW.OtOpcUa.Configuration.Tests.SchemaComplianceFixture</c> (same env
/// vars, same unique-name + <c>Migrate()</c> + drop-via-master lifecycle) with <b>one deliberate
/// deviation</b>: 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 <see cref="SkipReason"/> instead of throwing. Tests carry <see cref="SqlServerFactAttribute"/>,
/// which skips them when the probe failed.
/// </para>
/// <para>
/// The skip is an attribute rather than the <c>Assert.Skip(fixture.SkipReason)</c> call the S7
/// integration suite uses, because AdminUI.Tests is one of the three projects pinned to
/// <b>xunit v2</b> by <c>Akka.TestKit.Xunit2</c> (see <c>Directory.Packages.props</c>) and
/// <c>Assert.Skip</c> is xunit v3 only. Overriding <c>FactAttribute.Skip</c> 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 <see cref="AdminUiSqlServer"/>, so a session probes once.
/// </para>
/// <para>
/// The probe being <b>one-shot</b> bounds what "stays green offline" guarantees: it covers SQL
/// being unreachable <i>before the run</i>. A connectivity blip <i>during</i> a run is never
/// re-probed, so the affected tests surface as <c>SqlException</c> <b>failures</b>, not skips.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
public sealed class AdminUiSqlServerFixture : IDisposable
{
/// <summary>Gets the name of the per-run test database, or <see langword="null"/> when SQL is unreachable.</summary>
public string? DatabaseName { get; }
/// <summary>Gets the connection string for the test database, or <see langword="null"/> when SQL is unreachable.</summary>
public string? ConnectionString { get; }
/// <summary>
/// Gets the reason the SQL-backed tests cannot run (server unreachable, migration failed), or
/// <see langword="null"/> when the database is ready.
/// </summary>
public string? SkipReason { get; }
/// <summary>
/// Gets a context factory pointed at the migrated test database — the seam the services under test
/// (<c>DeploymentDriftProbe</c>, <c>RawTreeService</c>) take. Throws when <see cref="SkipReason"/>
/// is set; guard with <see cref="SqlServerFactAttribute"/>.
/// </summary>
public IDbContextFactory<OtOpcUaConfigDbContext> Factory =>
_factory ?? throw new InvalidOperationException(SkipReason ?? "SQL Server fixture is not available.");
private readonly SqlServerContextFactory? _factory;
/// <summary>Initializes a new instance of the <see cref="AdminUiSqlServerFixture"/> class.</summary>
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<OtOpcUaConfigDbContext>()
.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);
}
/// <summary>Disposes the fixture, dropping the test database.</summary>
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<OtOpcUaConfigDbContext>
{
public OtOpcUaConfigDbContext CreateDbContext() =>
new(new DbContextOptionsBuilder<OtOpcUaConfigDbContext>()
.UseSqlServer(connectionString)
.Options);
public Task<OtOpcUaConfigDbContext> CreateDbContextAsync(CancellationToken cancellationToken = default) =>
Task.FromResult(CreateDbContext());
}
}
/// <summary>
/// One-shot reachability probe + connection-string builder shared by
/// <see cref="AdminUiSqlServerFixture"/> and <see cref="SqlServerFactAttribute"/>. 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.
/// </summary>
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<string?> Probe = new(RunProbe, LazyThreadSafetyMode.ExecutionAndPublication);
/// <summary>Gets the reason SQL Server is unusable, or <see langword="null"/> when it answered.</summary>
public static string? SkipReason => Probe.Value;
/// <summary>Builds a connection string for the named database on the configured test server.</summary>
/// <param name="database">The database to connect to.</param>
/// <returns>The connection string.</returns>
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.";
}
}
}
/// <summary>
/// A <see cref="FactAttribute"/> that skips itself when the shared test SQL Server is unreachable.
/// The xunit v2 equivalent of xunit v3's <c>Assert.Skip</c> — see
/// <see cref="AdminUiSqlServerFixture"/>'s remarks for why this project cannot use the latter.
/// </summary>
[AttributeUsage(AttributeTargets.Method)]
public sealed class SqlServerFactAttribute : FactAttribute
{
/// <summary>Initializes a new instance of the <see cref="SqlServerFactAttribute"/> class.</summary>
public SqlServerFactAttribute()
{
if (AdminUiSqlServer.SkipReason is { } reason) Skip = reason;
}
}
/// <summary>Collection definition binding the SQL-backed AdminUI tests to one shared database.</summary>
[CollectionDefinition(nameof(AdminUiSqlServerCollection))]
public sealed class AdminUiSqlServerCollection : ICollectionFixture<AdminUiSqlServerFixture>
{
}
@@ -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;
/// <summary>
/// The SQL-Server-backed half of <see cref="DeploymentDriftProbeTests"/>: pins that
/// <c>DeploymentDriftProbe</c>'s watermark aggregate — <c>ConfigEdits.GroupBy(_ =&gt; 1).Select(g =&gt;
/// new { g.Max(...), g.Count() })</c> — actually <b>translates</b>, and translates to the right
/// values, on the real provider.
/// </summary>
/// <remarks>
/// <para>
/// 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:
/// </para>
/// <list type="bullet">
/// <item>a later-timestamped edit moves <c>Max</c> only (its <c>Count</c> also moves, so this
/// one alone is not decisive) —</item>
/// <item>an <b>earlier</b>-timestamped edit leaves <c>Max</c> where it was and moves
/// <c>Count</c> alone, which is the only thing that can invalidate the cache; a translation
/// that dropped the <c>Count</c> projection would serve the stale verdict and fail here.</item>
/// </list>
/// <para>
/// 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.
/// </para>
/// <para>
/// The table is cleared at the start of every test: the collection shares one database and the
/// watermark is a whole-table aggregate.
/// </para>
/// </remarks>
[Collection(nameof(AdminUiSqlServerCollection))]
public sealed class DeploymentDriftProbeSqlServerTests(AdminUiSqlServerFixture sql)
{
private const string SealedHash = "sealed-hash-aaaa";
/// <summary>A fixed instant so the "earlier edit" case is unambiguously earlier, clock skew aside.</summary>
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();
}
/// <summary>Stand-in for the <c>ConfigComposer</c> seam that counts how often it was invoked.</summary>
private sealed class CountingCompose(string result)
{
public string Result { get; set; } = result;
public int Count { get; private set; }
public Task<string> ComposeAsync(OtOpcUaConfigDbContext db)
{
Count++;
return Task.FromResult(Result);
}
}
}
@@ -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;
/// <summary>
/// The SQL-Server-backed half of <see cref="RawTreeServiceRenameWarningTests"/>: pins that the
/// expression-built <c>SourceCode.Contains(p0) || … || Contains(pN)</c> disjunction
/// (<c>RawTreeService.AnyPathContained</c>) <b>translates</b> on the real provider, and pins the one
/// behaviour EF InMemory structurally cannot show — the <b>collation widening</b> the method's
/// remarks describe.
/// </summary>
/// <remarks>
/// <para>
/// Under EF InMemory the predicate runs as ordinal <see cref="string.Contains(string)"/>, 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.
/// </para>
/// <para>
/// Also crosses the 200-term <c>ScriptScanBatchSize</c> chunk boundary against the real provider,
/// where a batch too large would be a translation/complexity failure rather than a silent
/// in-memory loop.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
[Collection(nameof(AdminUiSqlServerCollection))]
public sealed class RawTreeServiceRenameWarningSqlServerTests(AdminUiSqlServerFixture sql)
{
/// <summary>Mirrors <c>RawTreeService.ScriptScanBatchSize</c> (private); the boundary under test.</summary>
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";
/// <summary>The RawPath a seeded tag resolves to: Folder/Driver/Device/Tag (the cluster is not in it).</summary>
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<string> Tags, IReadOnlyList<ScriptSeed> Scripts);
private sealed record SeedResult(RawTreeService Service, string DeviceId, byte[] DeviceRowVersion);
private async Task<SeedResult> SeedAsync(Func<string, Seed> 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);
}
}
@@ -11,6 +11,11 @@
<PackageReference Include="xunit"/>
<PackageReference Include="Shouldly"/>
<PackageReference Include="Microsoft.EntityFrameworkCore.InMemory"/>
<!-- AdminUiSqlServerFixture drives a real SQL Server (create/migrate/drop) for the two suites
whose subject is the SQL provider's LINQ translation, not the C# around it. SqlConnection
arrives transitively via EFCore.SqlServer; referenced explicitly because this project uses
it directly, matching Configuration.Tests. -->
<PackageReference Include="Microsoft.Data.SqlClient"/>
<PackageReference Include="Microsoft.NET.Test.Sdk"/>
<!-- Akka.TestKit.Xunit2 ties this project to xunit v2 (TestKit isn't ported to v3 yet);
used by the IAuditWriter adapter test to register a TestProbe in an ActorRegistry. -->
@@ -519,6 +519,62 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable
finally { await host.DisposeAsync(); }
}
/// <summary>
/// An EMPTY <c>s=</c> identifier names no node, so it must not be memoized: every item on such a
/// node asks the authorizer for itself.
/// </summary>
/// <remarks>
/// <para>
/// An empty identifier addresses no ACL scope at all —
/// <see cref="Security.DataPlaneAuthorizer"/> 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.
/// </para>
/// <para>
/// 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.
/// <see cref="Revalidation_evaluates_one_verdict_per_session_and_node"/> is the control that
/// the memo still engages for a non-empty identifier, so this cannot pass by the memo having
/// been removed.
/// </para>
/// </remarks>
[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<string>().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 ─────────────────────────────
/// <summary>A cluster-wide Subscribe grant — what <c>CreateMonitoredItems</c> resolves to.</summary>
@@ -531,9 +587,16 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable
}
/// <summary>Registers a stub data-change item against <paramref name="nodeId"/> as if the SDK had created it.</summary>
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);
/// <summary>
/// Registers a stub data-change item against a hand-built <paramref name="handle"/> — the only way
/// to reach a node id the address space cannot mint, such as an empty <c>s=</c> identifier.
/// </summary>
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;
@@ -163,6 +163,106 @@ public sealed class SessionAclDecisionCacheTests
neighbourCalls.ShouldBe(1);
}
/// <summary>
/// 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.
/// </summary>
[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);
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// <para>
/// 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
/// <c>TryAdd</c> for the duration of the <c>Clear</c> 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.
/// </para>
/// <para>
/// ⚠️ <b>The assertion is equality, not a bound.</b> A ceiling check
/// (<c>TrackedCount &lt;= Count</c>, or <c>&gt;= 0</c>) passes against the broken code, because
/// under-counting is exactly what the broken code does. Only <c>TrackedCount == Count</c>
/// falsifies it — verified by reverting the fix, which turns this test red on the first round.
/// </para>
/// </remarks>
[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");
}
}
/// <summary>
/// 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.
@@ -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;
/// <summary>
/// The failure path the <c>Direct</c>-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.
/// </summary>
/// <remarks>
/// <para>
/// <b>A pin, not red-first.</b> The behaviour predates this class — <c>LoadArtifactBlob</c> has
/// always caught its own exceptions and returned <see langword="null"/>, which
/// <c>HandleArtifactForApply</c> turns into a Failed ACK. What was missing was a test saying so, and
/// a timed-out command is exactly this shape (a <c>SqlException</c> 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).
/// </para>
/// <para>
/// Sibling coverage reaches the same verdict through EMPTY bytes rather than a throw
/// (<c>DriverHostActorUnreadableArtifactTests</c>), which is the missing-row shape, not the
/// read-failed shape — the two arrive at <c>ArtifactForApply</c> by different routes.
/// </para>
/// </remarks>
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<string> { "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<NodeDiagnosticsSnapshot>(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<ApplyAck>(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<NodeDiagnosticsSnapshot>(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<ApplyAck>(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Applied);
actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref);
coordinator.ExpectMsg<NodeDiagnosticsSnapshot>(Timeout).CurrentRevision.ShouldBe(RevA);
}
/// <summary>Seeds a sealed, parseable deployment and returns its id.</summary>
private static DeploymentId SeedDeployment(IDbContextFactory<OtOpcUaConfigDbContext> db, RevisionHash rev)
{
var artifact = JsonSerializer.SerializeToUtf8Bytes(new { DriverInstances = Array.Empty<object>() });
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;
}
/// <summary>
/// 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.
/// </summary>
private sealed class FailingReadConfigDbFactory(IDbContextFactory<OtOpcUaConfigDbContext> inner)
: IDbContextFactory<OtOpcUaConfigDbContext>
{
private int _allowance = int.MaxValue;
private int _throws;
/// <summary>How many contexts have been failed.</summary>
public int Throws => Volatile.Read(ref _throws);
/// <summary>Arms a single throw, after letting <paramref name="allowThrough"/> more contexts past.</summary>
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();
}
}
}
@@ -353,6 +353,18 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit
/// The yield is the whole point: every other test here uses an outbox that returns
/// <c>ValueTask.CompletedTask</c>, 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
/// (<c>Task.Delay(1).ConfigureAwait(false)</c>); a bare <c>Task.Yield()</c> posts back to the
/// CAPTURED ActorTaskScheduler context and is just as vacuous as completing synchronously.
/// </para>
/// <para>
/// ⚠️ 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 <c>Self</c> 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.
/// </para>
/// </summary>
[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) -------
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// The per-tag lambda wrapped the writer call in a bare <c>catch</c>, so a shutdown
/// <see cref="OperationCanceledException"/> 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
/// <c>catch (OperationCanceledException) when (ct.IsCancellationRequested)</c> already treats a
/// cancelled pass as a non-fault, but the inner catch ate the exception before it could be reached.
/// <para>
/// 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.
/// </para>
/// <para>
/// The budget is deliberately the tightest there is (<c>maxWriteAttempts: 1</c>) 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.
/// </para>
/// </remarks>
[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));
}
/// <summary>
/// Scope pin: a NON-cancellation throw from one tag, while a sibling wrote, still counts toward
/// the Gitea #538 budget and still abandons.
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
[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.RecorderStatus>(
ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3));
Assert.Equal(0, status.QueuedDepth);
Assert.Equal(1, status.AbandonedEntries);
}, TimeSpan.FromSeconds(10));
}
/// <summary>
/// Scope pin: an <see cref="OperationCanceledException"/> raised while the drain token is NOT
/// cancelled is an ordinary write failure and still counts toward abandonment.
/// </summary>
/// <remarks>
/// This is what the <c>when (ct.IsCancellationRequested)</c> 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.
/// </remarks>
[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.RecorderStatus>(
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 ----------------
/// <summary>
@@ -792,6 +943,15 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit
/// <c>Task.Yield()</c>, which posts back to the CAPTURED ActorTaskScheduler context — leaves the
/// context installed and the defect invisible.
/// </para>
/// <para>
/// ⚠️ 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 <c>Self</c> 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.
/// </para>
/// </remarks>
[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.</summary>
private readonly HashSet<string> _failTags = new(StringComparer.Ordinal);
/// <summary>Historian tags whose writes THROW instead of returning an ack, keyed to the exception
/// factory to throw (given the drain's token). Distinct from <see cref="_failTags"/> because the
/// recorder's per-tag catch treats the two shapes differently: a returned <c>false</c> is always
/// a failure, whereas a thrown exception is a failure only when it is not the drain token's own
/// cancellation.</summary>
private readonly Dictionary<string, Func<CancellationToken, Exception>> _throwTags =
new(StringComparer.Ordinal);
/// <summary>Starts or stops failing one tag. Callable mid-test so a case can prove a tag that
/// recovers within its budget is never abandoned.</summary>
/// <param name="tag">The historian tag.</param>
@@ -1177,6 +1345,18 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit
}
}
/// <summary>Starts or stops THROWING for one tag's writes.</summary>
/// <param name="tag">The historian tag.</param>
/// <param name="factory">Builds the exception to throw from the drain's token; null stops throwing.</param>
public void SetThrowing(string tag, Func<CancellationToken, Exception>? factory)
{
lock (_gate)
{
if (factory is null) _throwTags.Remove(tag);
else _throwTags[tag] = factory;
}
}
/// <summary>Lifetime count of <see cref="WriteLiveValuesAsync"/> invocations (proves a retry ran).</summary>
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<CancellationToken, Exception>? 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
}
}
/// <summary>
/// <see cref="IHistorianValueWriter"/> double for the shutdown-cancellation drain path: one
/// nominated tag returns a plain failed ack for its first <c>failCalls</c> writes and then BLOCKS
/// on the next one until the drain's token is cancelled, at which point it throws the
/// <see cref="OperationCanceledException"/> a real in-flight gateway call raises at node shutdown.
/// Every other tag succeeds immediately.
/// </summary>
/// <remarks>
/// 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. <see cref="FakeValueWriter"/> cannot express
/// this: it completes synchronously, so the drain token is never live-cancelled underneath it.
/// </remarks>
private sealed class ShutdownBlockingValueWriter : IHistorianValueWriter
{
private readonly Lock _gate = new();
private readonly List<string> _calls = new();
private readonly string _tag;
private readonly int _failCalls;
private readonly TaskCompletionSource _entered = new(TaskCreationOptions.RunContinuationsAsynchronously);
private readonly TaskCompletionSource _cancelled = new(TaskCreationOptions.RunContinuationsAsynchronously);
/// <param name="tag">The historian tag that fails and then blocks.</param>
/// <param name="failCalls">How many of its writes return a plain failed ack before the blocking one.</param>
public ShutdownBlockingValueWriter(string tag, int failCalls)
{
_tag = tag;
_failCalls = failCalls;
}
/// <summary>Completes once the blocking write has been ENTERED — i.e. the pass is mid-write and it
/// is safe to stop the recorder.</summary>
public Task Entered => _entered.Task;
/// <summary>Completes once that blocked write has observed the cancellation and thrown.</summary>
public Task Cancelled => _cancelled.Task;
/// <summary>Lifetime count of writes for one tag.</summary>
/// <param name="tag">The historian tag.</param>
/// <returns>How many times it has been written.</returns>
public int CallCountFor(string tag)
{
lock (_gate)
{
return _calls.Count(c => string.Equals(c, tag, StringComparison.Ordinal));
}
}
public async Task<bool> WriteLiveValuesAsync(
string tag, IReadOnlyList<HistorizationValue> 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);
/// <summary>In-memory <see cref="IHistorizationOutbox"/> double honouring the FIFO-truncate