fix(mtconnect): make a /sample stream end loud, and correct the gap contract (Task 7 review)

Code review of c46540ae approved the framing algorithm but moved the risk
onto the contract around it.

C1 (critical). SampleAsync returned normally on three different events:
EOF, the closing --boundary--, and a response that was not multipart at
all (where it yielded one parsed snapshot and finished). The seam
documents the stream as yielding until ct is cancelled, so a Task 11 pump
written to that contract would silently drop its subscription or hot-loop
reconnecting; the non-multipart leg reported a configuration error as a
healthy finished stream, which is the #485 quiet-successful-termination
shape aimed at the very next task. Every non-cancellation end now throws:
MTConnectStreamEndedException (ConnectionClosed / ClosingBoundary -
transient) or MTConnectStreamNotSupportedException (configuration -
reconnecting reproduces it forever), sharing one catchable base. The
non-multipart body is still parsed first so an Agent's own MTConnectError
text wins, but the document is NOT yielded.

I1. IsSequenceGap moved to IMTConnectAgentClient (static) and its first
parameter is now expectedFrom. The old name and doc were wrong for every
chunk after the first - the correct comparand is the PREVIOUS chunk's
NextSequence, and comparing against the opening "from" argument reports a
gap on every chunk once the ring buffer rolls, i.e. an endless /current
re-baseline storm. Both doc blocks also now record that cppagent answers
from < firstSequence with an OUT_OF_RANGE MTConnectError under HTTP 200,
which surfaces as InvalidDataException and NOT as a gap - so Task 11 must
treat that parse failure as a re-baseline trigger too.

I2. Every multipart test served the whole body from one buffer, so every
framing test completed in a single ReadAsync - the split-boundary path,
the split-header path and the multi-fill loop were correct by inspection
only. A ChunkedStream double (N bytes per read, N = 1/3/7) now drives the
fixtures through a split transport, plus a >16 KB document that forces a
buffer resize. Falsifiability: removing either scan overlap, or
collapsing the fill loop, fails ONLY the new chunked tests.

I3. Disposal mid-enumeration surfaces as OperationCanceledException via
an internal dispose-linked token, not a raw ObjectDisposedException that
Task 9's re-init would inflict on an enumerating pump.

I5. HeartbeatMs, SampleIntervalMs and SampleCount join RequestTimeoutMs
in construction-time positive-value validation. All three reach the query
string and an Agent answers heartbeat=0 with an HTTP-200 MTConnectError
that names no config key.

I4/M2. The no-Content-length framing fallback logs a one-shot warning
(the client takes an optional ILogger); a multipart response with no
boundary parameter fails fast instead of degrading into a watchdog
timeout.

M5/M6. Shared element/attribute reading rules extracted to MTConnectXml;
the probe parser documents why it alone does not trim BOM/whitespace. The
literal U+FEFF in source became a '' escape.

Also, from Task 8: MTConnectObservation gained IsStructured (default
false), set from element.HasElements. A DATA_SET/TABLE observation's
<Entry> children concatenate through element.Value to nonsense ("12" for
two entries) that the index would publish as Good, and only the parser
can tell that apart from a legitimate space-bearing Message. False for
CONDITION observations - their value comes from the element name, so
child content cannot corrupt it.

275/275 green in this suite's own files.
This commit is contained in:
Joseph Doherty
2026-07-24 15:01:31 -04:00
parent ac0a284055
commit 5ba9f1be6f
9 changed files with 1232 additions and 294 deletions
+51
View File
@@ -385,6 +385,57 @@ dotnet test tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests --filter "Ful
git add -A && git commit -m "feat(mtconnect): current/sample parse + sequence-gap detection (Task 7)"
```
### Task 7 review remediation (post-`c46540ae`) — three contract changes Tasks 9/11/13 must build against
Code review of `c46540ae` approved the framing algorithm but moved the risk onto the **contract**. Three
changes here are breaking relative to the sketch above; the rest are hardening.
1. **`SampleAsync` never ends normally except by cancellation.** The seam documents the stream as
yielding "indefinitely, until `ct` is cancelled", but the implementation returned normally on EOF, on
the closing `--boundary--`, and — worst — on a response that was not multipart at all, where it
yielded one parsed snapshot and finished. A pump written to the documented contract has no reason to
handle normal completion, so it would silently drop its subscription or hot-loop reconnecting; the
non-multipart leg additionally reported a **configuration error as a healthy finished stream**, which
is the #485 quiet-successful-termination shape aimed straight at Task 11. Every non-cancellation end
now throws: `MTConnectStreamEndedException` (with `MTConnectStreamEndReason.ConnectionClosed` /
`ClosingBoundary` — transient, reconnect) or `MTConnectStreamNotSupportedException` (configuration —
reconnecting reproduces it forever), sharing the base `MTConnectStreamException`. The non-multipart
body is still parsed first, so an Agent's own `MTConnectError` text still wins, but the document is
**not** yielded.
2. **`IsSequenceGap` moved to `IMTConnectAgentClient` (static) and its first parameter is now
`expectedFrom`.** The sketch's `requestedFrom` name and doc were wrong for every chunk after the
first: the correct comparand is the **previous chunk's `NextSequence`**, and comparing against the
opening `from` reports a gap on every chunk once the ring buffer rolls — an endless `/current`
re-baseline storm against a healthy stream. Rehomed onto the seam so Task 11 need not reference the
concrete client. **Task 11 must also treat an `OUT_OF_RANGE` `InvalidDataException` as a re-baseline
trigger:** cppagent answers a `from` below `firstSequence` with an `MTConnectError` under HTTP 200,
so ring-buffer overflow arrives as a *parse failure*, never as a gap-bearing chunk. `IsSequenceGap`
alone does not cover it. Documented on the seam.
3. **`MTConnectObservation` gained `IsStructured`** (default `false`), set from `element.HasElements`.
A DATA_SET/TABLE observation's `<Entry key=…>` children concatenate through `element.Value` to
nonsense (`"12"` for two entries) which the index would otherwise publish as Good, and only the
parser can tell that apart from a legitimate space-bearing `Message` — no value-shape heuristic
works. Deliberately **false for CONDITION** observations: their value comes from the element *name*,
so child content cannot corrupt it. The index maps the flag to a status; the parser does not.
Hardening in the same pass: disposal mid-enumeration now surfaces as `OperationCanceledException`
(via an internal dispose-linked token) rather than a raw `ObjectDisposedException`/`IOException` that
Task 9's re-init would otherwise inflict on an enumerating pump; `HeartbeatMs`, `SampleIntervalMs` and
`SampleCount` join `RequestTimeoutMs` in construction-time positive-value validation (all three reach
the query string, and an Agent answers `heartbeat=0` with an HTTP-200 `MTConnectError` that would name
no config key); a `multipart/*` response with no `boundary` parameter fails fast instead of degrading
into a watchdog timeout; the no-`Content-length` framing fallback logs a one-shot warning (the client
now takes an optional `ILogger`); and the shared element/attribute reading rules moved into
`MTConnectXml`, used by both parsers.
**Coverage gap closed, and worth remembering as a pattern.** Every multipart test served the whole body
from one buffer, so **every framing test completed in a single `ReadAsync`** — the split-boundary path,
the split-header path and the multi-fill loop were correct by inspection only, on the task whose
headline risk is framing. A `ChunkedStream` double returning N bytes per read (N = 1, 3, 7) now drives
the fixtures through a split transport. Falsifiability confirms the gap was real: removing the
split-boundary overlap, removing the split-header overlap, and collapsing `EnsureAsync`'s fill loop each
fail **only** the new chunked tests — every pre-existing multipart test stays green under all three.
---
## Task 8: `MTConnectObservationIndex` + `UNAVAILABLE → BadNoCommunication`