From 6c63bf8fabe318c93a093bed70151b5b67a5abb4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:51:39 -0400 Subject: [PATCH] fix(mtconnect): back off a /sample re-baseline that resolved nothing; scope Test Connect to the configured device Driver.MTConnect-001 (Medium): StreamVerdict.Reopen reset the reconnect ladder unconditionally. The InvalidDataException catch that routes an Agent OUT_OF_RANGE to a /current re-baseline also catches an unparseable chunk and an over-sized multipart part - neither of which a re-baseline can resolve - so the pump reopened and failed with no delay at any point: two HTTP requests and one Warning per iteration, forever. The Reopen arm now uses the same discriminator the transient arm does (did THIS connection deliver a chunk?), so a buffer roll behind a working stream still reopens instantly while a re-baseline that resolved nothing advances BackoffFor. Driver.MTConnect-002 (Medium): MTConnectDriverProbe requested {AgentUri}/probe while MTConnectAgentClient requests {AgentUri}/{DeviceName}/probe, so a mistyped or since-renamed deviceName showed GREEN on Test Connect and then Faulted the driver at InitializeAsync (an unknown device path answers MTConnectError under HTTP 200). The probe's minimal DTO now reads deviceName - the rule is 'which keys compose the URL', not 'which keys are cheap to read'. Driver.MTConnect-008 (Low): corrected two stale claims that a since-changed RawBrowseCommitMapper invalidated (see Driver.MTConnect-003, which remains open and belongs in AdminUI), and made DiscoverAsync log the options snapshot it deliberately read once instead of the field. Tests: 538 pass (534 base + 4 new, each red before its fix); AdminUI.Tests 1466 pass. --- docs/drivers/MTConnect.md | 18 ++++- .../MTConnectDataTypeInference.cs | 20 ++++-- .../MTConnectDriverOptions.cs | 6 +- .../MTConnectDriver.cs | 54 ++++++++++---- .../MTConnectDriverProbe.cs | 43 ++++++++--- .../MTConnectDriverProbeTests.cs | 59 +++++++++++++++ .../MTConnectSubscribeTests.cs | 72 +++++++++++++++++++ 7 files changed, 239 insertions(+), 33 deletions(-) diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 47e0c7c7..489448ed 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -167,9 +167,11 @@ universal browser commits and the key the driver resolves reads/subscribes against. `MTConnectTagConfigModel.FromJson` (the AdminUI typed editor) tries three spellings in order — `fullName` → `dataItemId` → `address` — and takes the first non-blank one, normalising onto `fullName` on save. `address` is -accepted because that's the field name `RawBrowseCommitMapper` writes for a -driver with no typed editor path, so a browse-committed tag stays readable -even before the editor round-trips it. +accepted because that is the field name `RawBrowseCommitMapper` writes for a +driver with no typed-editor branch of its own — MTConnect has had such a +branch since Gitea #536 and now commits under `fullName`, but blobs committed +before it, and hand-authored/CSV/API blobs, still carry `address` and must +stay readable. ### Coercion-type precedence @@ -192,6 +194,16 @@ into) is resolved in this order, first non-null wins: (category/type/units/representation → `DriverDataType`). 4. `DriverDataType.String` — the coercion that cannot fail. +> ⚠️ **Step 3 does not currently rescue a browse-committed tag.** `RawBrowseCommitMapper`'s +> MTConnect branch emits a whole `MTConnectTagConfigModel`, whose `ToJson()` always writes +> `dataType` from a field defaulting to `String` — a non-blank authored type, so step 1 wins +> and step 3 is never consulted. The raw tag's OPC UA `DataType` meanwhile *is* taken from the +> browsed leaf's inferred type (`RawBrowseCommitMapper.MapLeaf` → `MapDataType`), so a browsed +> `SAMPLE` lands as a `Double` node whose driver coerces the observation to text. Fix belongs in +> the commit mapper (forward the leaf's `DriverDataType` into the model, as the AbCip / TwinCAT / +> FOCAS branches already do). Recorded as `Driver.MTConnect-003` in +> [`code-reviews/Driver.MTConnect/findings.md`](../../code-reviews/Driver.MTConnect/findings.md). + ### Quality mapping `UNAVAILABLE` — MTConnect's one explicit "I have no value" sentinel — maps to diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs index 4bddb20c..d8e16053 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs @@ -29,11 +29,21 @@ public readonly record struct MTConnectInferredType(DriverDataType DataType, boo /// /// /// -/// This lives in .Contracts because three call sites must agree byte-for-byte, or a -/// tag's OPC UA type stops matching its authored config: ITagDiscovery.DiscoverAsync -/// (stamps on every browsed leaf), the -/// universal browser's commit path (writes the browsed leaf into TagConfig), and the -/// AdminUI typed tag editor (shows the inferred type and offers an override). +/// This lives in .Contracts because every call site must agree byte-for-byte, or a +/// tag's OPC UA type stops matching its authored config. At HEAD the callers are both inside +/// the driver: ITagDiscovery.DiscoverAsync (stamps +/// on every browsed leaf) and +/// MTConnectDriver.InferDefinition (the runtime coercion fallback for a raw tag whose +/// blob declares no type). +/// +/// +/// ⚠️ Two call sites this remark used to name do not exist. The universal browser's +/// commit path (RawBrowseCommitMapper) never calls this — it re-maps the browsed leaf's +/// already-inferred DriverDataType onto the raw tag's OPC UA type but writes a +/// TagConfig whose dataType is the model default — and the AdminUI typed editor +/// (MTConnectTagConfigModel) deliberately does not recompute the inference at all. The +/// first of those is a live defect, not merely a stale comment: see +/// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). /// /// /// MTConnect is weakly typed on the wire — every observation is text, and the standard diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs index 6c601e76..ef6a8edb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs @@ -86,7 +86,11 @@ public sealed class MTConnectDriverOptions /// type comes from the blob's driverDataType; failing that, from a /// entry naming the same DataItem id; failing that, from the Agent's /// own /probe declaration (); failing - /// that, — the coercion that cannot fail. + /// that, — the coercion that cannot fail. + /// ⚠️ The /probe step is not reached by a browse-committed tag at HEAD: + /// the AdminUI's commit mapper writes a full typed blob whose dataType defaults + /// to String, which the first step then honours. See + /// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). /// A tag in only is reachable by its DataItem id /// (the driver CLI's authoring surface, and every pre-v3 test) and still contributes /// its coercion type to the index. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index a666a27d..7734a187 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -1224,7 +1224,7 @@ public sealed class MTConnectDriver // downstream). Its data items are unreachable either way. _logger.LogWarning( "MTConnect driver {DriverInstanceId} skipped a device in {AgentUri}'s /probe model that declares neither a name nor an id; it cannot be given a browse path.", - _driverInstanceId, _options.AgentUri); + _driverInstanceId, options.AgentUri); continue; } @@ -1242,7 +1242,7 @@ public sealed class MTConnectDriver { _logger.LogWarning( "MTConnect driver {DriverInstanceId} is scoped to device '{DeviceScope}', but {AgentUri}'s /probe model declares no device with that name or id ({DeclaredCount} declared). The browse is empty because the scope matched nothing, not because the Agent has nothing.", - _driverInstanceId, deviceScope, _options.AgentUri, model.Devices.Count); + _driverInstanceId, deviceScope, options.AgentUri, model.Devices.Count); return; } @@ -1252,7 +1252,7 @@ public sealed class MTConnectDriver _driverInstanceId, devices, variables, - _options.AgentUri, + options.AgentUri, deviceScope is null or "" ? string.Empty : $" (scoped to '{deviceScope}')"); } @@ -1854,10 +1854,13 @@ public sealed class MTConnectDriver /// "just re-initialize" would be a silent permanent deadlock. /// /// - /// Backoff applies to failures only. A re-baseline is a protocol event, not a - /// fault, so it reopens the stream immediately and resets the attempt count; only a - /// genuine failure (dropped connection, blown heartbeat, a re-baseline that itself - /// failed) advances . + /// Backoff applies to failures only — and to a re-baseline that resolved nothing. + /// A re-baseline behind a stream that delivered is a protocol event, not a fault, so it + /// reopens immediately and resets the attempt count. One on a connection that delivered + /// nothing advances like any other failure, because the + /// path that reaches it also carries conditions a + /// /current cannot resolve (an unparseable chunk, an over-sized part) and would + /// otherwise reopen-and-fail as fast as the Agent can answer. /// /// private async Task RunSampleStreamAsync( @@ -1927,8 +1930,21 @@ public sealed class MTConnectDriver return; case StreamVerdict.Reopen: - attempt = 0; - _reconnectAttempts = 0; + // A re-baseline behind a stream that DELIVERED is the protocol event this + // verdict was written for — a sequence gap or an OUT_OF_RANGE rejection, both + // of which the /current genuinely resolves — so it reopens immediately on a + // clean ladder. + // + // One that delivered NOTHING is not necessarily resolved by anything. The + // InvalidDataException catch that routes OUT_OF_RANGE here also catches an + // unparseable chunk and an over-sized part, and a /current cannot fix either: + // resetting the ladder there reopens instantly, fails identically, and spends + // one /sample AND one /current per iteration forever — the unbounded hot loop + // every other arm of this pump (the Unsupported latch, the backoff floor, the + // non-positive cap) exists to prevent. The discriminator is the same one the + // transient arm below uses, for the same reason. + attempt = outcome.DeliveredAny ? 0 : attempt + 1; + _reconnectAttempts = attempt; break; @@ -3208,10 +3224,22 @@ public sealed class MTConnectDriver /// Coercion type precedence (see ): /// the blob's own driverDataType, else a /// entry naming the same DataItem id, else the Agent's /probe declaration for that - /// data item, else . The third step is what makes a - /// browse-committed tag work: RawBrowseCommitMapper writes only the address into - /// the blob, so without it every value from the address picker would publish as text - /// under a numerically-typed OPC UA node. + /// data item, else . + /// + /// + /// ⚠️ The third step does NOT currently rescue a browse-committed tag, and the + /// AdminUI is where that has to be fixed. It was written on the premise that + /// RawBrowseCommitMapper writes only the address into the blob — true when this + /// was authored, false at HEAD: the MTConnect branch of + /// RawBrowseCommitMapper.BuildTagConfig now emits a whole + /// MTConnectTagConfigModel, whose ToJson unconditionally writes + /// "dataType" from a field defaulting to . That + /// is a non-blank authored type, so step 1 wins and this step is never reached — every + /// browse-committed tag coerces to text while its raw-tag node was typed Double / + /// Int64 from the SAME probe inference (RawBrowseCommitMapper.MapLeaf). + /// The step is kept because it is still the correct rule and still serves a blob authored + /// by hand, by CSV or by API with no type; see + /// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). /// /// /// The options document to derive from. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs index 85658b9a..73fe78bb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs @@ -16,19 +16,29 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// Deliberately does NOT delegate to . That /// method builds the full driver options document, including every authored /// MTConnectTagDefinition (BuildTag can throw on a malformed tag entry that has -/// nothing to do with reachability). A probe only needs agentUri, so this type parses -/// its own minimal DTO — a config that is invalid for the driver (bad tag, bad data type) can -/// still be reachability-tested, matching the AdminUI's "does the endpoint answer" intent for -/// Test Connect. It also avoids a compile-time dependency on the factory DTO shape -/// (MTConnectDriverFactoryExtensions), which is authored separately (Task 15). +/// nothing to do with reachability). A probe needs only the fields that compose the request +/// URL, so this type parses its own minimal DTO — a config that is invalid for the driver (bad +/// tag, bad data type) can still be reachability-tested, matching the AdminUI's "does the +/// endpoint answer" intent for Test Connect. It also avoids a compile-time dependency on the +/// factory DTO shape (MTConnectDriverFactoryExtensions), which is authored separately +/// (Task 15). /// /// -/// Accepted gap: reachable ≠ deployable. Because only agentUri is read, a -/// config can show green on Test Connect and still fail to deploy — e.g. a non-positive -/// requestTimeoutMs or a malformed tag entry the factory's full parse would reject. -/// This is the direct consequence of the independent-parsing decision above, judged -/// acceptable because Test Connect's job is "is the endpoint there", not "will this exact -/// config deploy". +/// The minimal DTO is "everything in the URL", not "agentUri". It reads +/// deviceName too, because narrows every read to +/// {AgentUri}/{DeviceName}/probe: probing the agent-wide path instead would answer a +/// different question from the one the operator asked, and a mistyped or since-renamed device +/// would show GREEN here and then Fault the driver at InitializeAsync (the Agent +/// answers an unknown device path with an MTConnectError under HTTP 200). The +/// distinction is not "which keys are cheap to read" but "which keys change the endpoint". +/// +/// +/// Accepted gap: reachable ≠ deployable. Because only the URL-composing fields are +/// read, a config can show green on Test Connect and still fail to deploy — e.g. a +/// non-positive requestTimeoutMs or a malformed tag entry the factory's full parse +/// would reject. This is the direct consequence of the independent-parsing decision above, +/// judged acceptable because Test Connect's job is "is the endpoint there", not "will this +/// exact config deploy". /// /// /// Deliberately DOES reuse and @@ -117,6 +127,12 @@ public sealed class MTConnectDriverProbe : IDriverProbe private sealed class MTConnectProbeConfigDto { public string? AgentUri { get; set; } + + /// + /// The optional device scope. Read because it is part of the URL, not part of the + /// driver's tag/timing configuration — see the "reachable ≠ deployable" remarks on the type. + /// + public string? DeviceName { get; set; } } /// @@ -169,6 +185,11 @@ public sealed class MTConnectDriverProbe : IDriverProbe var options = new MTConnectDriverOptions { AgentUri = agentUri, + + // Part of the request URL, so it must be carried: MTConnectAgentClient narrows every read + // to {AgentUri}/{DeviceName}/probe, and a probe of the agent-wide path answers a different + // question from the one the operator asked. See the type remarks. + DeviceName = string.IsNullOrWhiteSpace(dto.DeviceName) ? null : dto.DeviceName.Trim(), RequestTimeoutMs = requestTimeoutMs, }; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs index 03ce5781..8dd14e9d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs @@ -301,6 +301,65 @@ public sealed class MTConnectDriverProbeTests r.Latency.ShouldBeNull(); } + // ── review follow-up: the probe must request the URL the DRIVER will request ── + + /// + /// Test Connect has to probe the driver's own request path, device scope included. + /// MTConnectAgentClient.BuildRequestUri narrows every read to + /// {AgentUri}/{DeviceName}/probe, so a probe that requested the agent-wide + /// {AgentUri}/probe answers a different question from the one the operator asked. A + /// mistyped or since-renamed deviceName then shows GREEN on Test Connect and Faults the + /// driver at InitializeAsync — the Agent answers that path with an MTConnectError + /// under HTTP 200, which the parser surfaces as . + /// + /// This is a different class from the accepted "reachable ≠ deployable" gap the type + /// documents (a bad tag entry, a non-positive timeout): those do not change the URL, and + /// the whole point of a reachability probe is which endpoint it reaches. + /// + /// + [Fact] + public async Task Configured_deviceName_scopes_the_probe_request_the_way_the_driver_does() + { + Uri? requested = null; + var probe = ProbeWithHandler((request, _) => + { + requested = request.RequestUri; + + return Task.FromResult(XmlResponse(ValidProbeXml)); + }); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent/\",\"deviceName\":\"VMC-1\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeTrue(); + requested.ShouldNotBeNull(); + requested.AbsolutePath.ShouldBe("/VMC-1/probe"); + } + + /// + /// …and with no deviceName the probe stays agent-wide, exactly as the driver does. The + /// falsification control for the test above: unconditionally inserting a segment would pass it + /// while breaking every unscoped config. + /// + [Fact] + public async Task An_absent_deviceName_leaves_the_probe_agent_wide() + { + Uri? requested = null; + var probe = ProbeWithHandler((request, _) => + { + requested = request.RequestUri; + + return Task.FromResult(XmlResponse(ValidProbeXml)); + }); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent/\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeTrue(); + requested.ShouldNotBeNull(); + requested.AbsolutePath.ShouldBe("/probe"); + } + // ── minor: credentialed agentUri + a SUCCESSFUL response must still redact ── [Fact] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs index b12c32ff..84b5d790 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs @@ -990,6 +990,78 @@ public sealed class MTConnectSubscribeTests } } + /// + /// A re-baseline that never resolves anything must not reopen without a backoff. + /// StreamVerdict.Reopen is "a protocol event, not a fault", so it reopens immediately + /// and resets the ladder — correct for the two cases it was written for (a sequence gap and an + /// OUT_OF_RANGE rejection), both of which the /current re-baseline genuinely + /// fixes, so they cannot recur. + /// + /// But the catch that routes OUT_OF_RANGE to the + /// re-baseline also catches an unparseable chunk and an oversized part — conditions a + /// /current cannot fix. Reopening those with the ladder reset is an unbounded hot + /// loop against the Agent (one /sample AND one /current per iteration, as fast + /// as they complete, one Warning each), which is exactly what the reconnect backoff exists + /// to prevent everywhere else in this pump. + /// + /// + /// The discriminator is the one the transient path already uses: did THIS connection deliver + /// a chunk? A re-baseline behind a working stream still reopens instantly; one on a + /// connection that has never delivered advances the ladder. Asserted as monotonic growth, + /// never as a duration. + /// + /// + [Fact] + public async Task A_rebaseline_that_delivers_nothing_still_advances_the_reconnect_backoff() + { + // A 1 ms cap keeps the failing loop from actually sleeping; nothing here asserts a duration. + var options = Opts(new MTConnectReconnectOptions { MinBackoffMs = 0, MaxBackoffMs = 1 }); + var (driver, client) = await InitializedDriverAsync(options: options); + + // Sticky, and NOT a transient stream fault: a chunk this driver can never parse. /current keeps + // answering, so every re-baseline "succeeds" and the pump reopens — forever. + client.SampleFailure = new InvalidDataException("chunk is not a well-formed MTConnectStreams document"); + + var handle = await driver.SubscribeAsync(["dev1_pos"], TimeSpan.FromMilliseconds(50), Ct); + + try + { + await client.WaitForSampleCallsAsync(3).WaitAsync(Watchdog, Ct); + var early = driver.ReconnectAttempts; + + await client.WaitForSampleCallsAsync(6).WaitAsync(Watchdog, Ct); + + early.ShouldBeGreaterThanOrEqualTo(2); + driver.ReconnectAttempts.ShouldBeGreaterThan(early); + } + finally + { + await driver.UnsubscribeAsync(handle, Ct).WaitAsync(Watchdog, Ct); + } + } + + /// + /// …and the falsification control for the test above: a re-baseline BEHIND a stream that was + /// delivering keeps its immediate reopen. Without this, "advance the ladder on every re-baseline" + /// would pass the test above while making a healthy Agent's ordinary buffer roll cost a backoff. + /// + [Fact] + public async Task A_rebaseline_after_a_delivered_chunk_still_reopens_immediately() + { + var (driver, client) = await InitializedDriverAsync(); + await driver.SubscribeAsync(["dev1_pos"], TimeSpan.FromMilliseconds(50), Ct); + + // One good chunk, so this connection has provably delivered… + await client.PumpAsync(Chunk(PrimedNextSequence, 113, ("dev1_pos", "1.5"))).WaitAsync(Watchdog, Ct); + + // …then the Agent's buffer rolls past the cursor. The re-baseline is a protocol event behind a + // working stream: no backoff, no ladder. + client.CurrentAnswers.Enqueue(Chunk(5000, 5005, ("dev1_pos", "2.5"))); + await client.PumpAsync(Chunk(5000, 5005, ("dev1_pos", "2.5"))).WaitAsync(Watchdog, Ct); + + driver.ReconnectAttempts.ShouldBe(0); + } + /// /// I3 — a non-positive cap is an operator-authorable hot loop. Clamping to it (the /// obvious Math.Max(0, …)) makes EVERY delay zero, so the pump reconnect-spins as fast