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.
This commit is contained in:
Joseph Doherty
+15
-5
@@ -29,11 +29,21 @@ public readonly record struct MTConnectInferredType(DriverDataType DataType, boo
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// This lives in <c>.Contracts</c> because three call sites must agree byte-for-byte, or a
|
||||
/// tag's OPC UA type stops matching its authored config: <c>ITagDiscovery.DiscoverAsync</c>
|
||||
/// (stamps <see cref="DriverAttributeInfo.DriverDataType" /> on every browsed leaf), the
|
||||
/// universal browser's commit path (writes the browsed leaf into <c>TagConfig</c>), and the
|
||||
/// AdminUI typed tag editor (shows the inferred type and offers an override).
|
||||
/// This lives in <c>.Contracts</c> 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: <c>ITagDiscovery.DiscoverAsync</c> (stamps
|
||||
/// <see cref="DriverAttributeInfo.DriverDataType" /> on every browsed leaf) and
|
||||
/// <c>MTConnectDriver.InferDefinition</c> (the runtime coercion fallback for a raw tag whose
|
||||
/// blob declares no type).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Two call sites this remark used to name do not exist.</b> The universal browser's
|
||||
/// commit path (<c>RawBrowseCommitMapper</c>) never calls this — it re-maps the browsed leaf's
|
||||
/// already-inferred <c>DriverDataType</c> onto the raw tag's OPC UA type but writes a
|
||||
/// <c>TagConfig</c> whose <c>dataType</c> is the model default — and the AdminUI typed editor
|
||||
/// (<c>MTConnectTagConfigModel</c>) deliberately does not recompute the inference at all. The
|
||||
/// first of those is a live defect, not merely a stale comment: see
|
||||
/// <c>code-reviews/Driver.MTConnect/findings.md</c> (Driver.MTConnect-003).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>MTConnect is weakly typed on the wire</b> — every observation is text, and the standard
|
||||
|
||||
@@ -86,7 +86,11 @@ public sealed class MTConnectDriverOptions
|
||||
/// type comes from the blob's <c>driverDataType</c>; failing that, from a
|
||||
/// <see cref="Tags"/> entry naming the same DataItem id; failing that, from the Agent's
|
||||
/// own <c>/probe</c> declaration (<see cref="MTConnectDataTypeInference"/>); failing
|
||||
/// that, <see cref="DriverDataType.String"/> — the coercion that cannot fail.</item>
|
||||
/// that, <see cref="DriverDataType.String"/> — the coercion that cannot fail.
|
||||
/// ⚠️ The <c>/probe</c> step is <b>not</b> reached by a browse-committed tag at HEAD:
|
||||
/// the AdminUI's commit mapper writes a full typed blob whose <c>dataType</c> defaults
|
||||
/// to <c>String</c>, which the first step then honours. See
|
||||
/// <c>code-reviews/Driver.MTConnect/findings.md</c> (Driver.MTConnect-003).</item>
|
||||
/// <item>A tag in <see cref="Tags"/> only is reachable by its DataItem <c>id</c>
|
||||
/// (the driver CLI's authoring surface, and every pre-v3 test) and still contributes
|
||||
/// its coercion type to the index.</item>
|
||||
|
||||
@@ -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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Backoff applies to failures only.</b> 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 <see cref="BackoffFor"/>.
|
||||
/// <b>Backoff applies to failures only — and to a re-baseline that resolved nothing.</b>
|
||||
/// 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 <see cref="BackoffFor"/> like any other failure, because the
|
||||
/// <see cref="InvalidDataException"/> path that reaches it also carries conditions a
|
||||
/// <c>/current</c> cannot resolve (an unparseable chunk, an over-sized part) and would
|
||||
/// otherwise reopen-and-fail as fast as the Agent can answer.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
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
|
||||
/// <b>Coercion type precedence</b> (see <see cref="MTConnectDriverOptions.RawTags"/>):
|
||||
/// the blob's own <c>driverDataType</c>, else a <see cref="MTConnectDriverOptions.Tags"/>
|
||||
/// entry naming the same DataItem id, else the Agent's <c>/probe</c> declaration for that
|
||||
/// data item, else <see cref="DriverDataType.String"/>. The third step is what makes a
|
||||
/// browse-committed tag work: <c>RawBrowseCommitMapper</c> 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 <see cref="DriverDataType.String"/>.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>The third step does NOT currently rescue a browse-committed tag, and the
|
||||
/// AdminUI is where that has to be fixed.</b> It was written on the premise that
|
||||
/// <c>RawBrowseCommitMapper</c> writes only the address into the blob — true when this
|
||||
/// was authored, false at HEAD: the MTConnect branch of
|
||||
/// <c>RawBrowseCommitMapper.BuildTagConfig</c> now emits a whole
|
||||
/// <c>MTConnectTagConfigModel</c>, whose <c>ToJson</c> unconditionally writes
|
||||
/// <c>"dataType"</c> from a field defaulting to <see cref="DriverDataType.String"/>. 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 <c>Double</c> /
|
||||
/// <c>Int64</c> from the SAME probe inference (<c>RawBrowseCommitMapper.MapLeaf</c>).
|
||||
/// 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
|
||||
/// <c>code-reviews/Driver.MTConnect/findings.md</c> (Driver.MTConnect-003).
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="options">The options document to derive from.</param>
|
||||
|
||||
@@ -16,19 +16,29 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect;
|
||||
/// <b>Deliberately does NOT delegate to <see cref="MTConnectDriver.ParseOptions"/>.</b> That
|
||||
/// method builds the full driver options document, including every authored
|
||||
/// <c>MTConnectTagDefinition</c> (<c>BuildTag</c> can throw on a malformed tag entry that has
|
||||
/// nothing to do with reachability). A probe only needs <c>agentUri</c>, 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
|
||||
/// (<c>MTConnectDriverFactoryExtensions</c>), 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 (<c>MTConnectDriverFactoryExtensions</c>), which is authored separately
|
||||
/// (Task 15).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Accepted gap: reachable ≠ deployable.</b> Because only <c>agentUri</c> is read, a
|
||||
/// config can show green on Test Connect and still fail to deploy — e.g. a non-positive
|
||||
/// <c>requestTimeoutMs</c> 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".
|
||||
/// <b>The minimal DTO is "everything in the URL", not "agentUri".</b> It reads
|
||||
/// <c>deviceName</c> too, because <see cref="MTConnectAgentClient"/> narrows every read to
|
||||
/// <c>{AgentUri}/{DeviceName}/probe</c>: 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 <c>InitializeAsync</c> (the Agent
|
||||
/// answers an unknown device path with an <c>MTConnectError</c> under HTTP 200). The
|
||||
/// distinction is not "which keys are cheap to read" but "which keys change the endpoint".
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Accepted gap: reachable ≠ deployable.</b> 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 <c>requestTimeoutMs</c> 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".
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Deliberately DOES reuse <see cref="MTConnectAgentClient"/> and
|
||||
@@ -117,6 +127,12 @@ public sealed class MTConnectDriverProbe : IDriverProbe
|
||||
private sealed class MTConnectProbeConfigDto
|
||||
{
|
||||
public string? AgentUri { get; set; }
|
||||
|
||||
/// <summary>
|
||||
/// The optional device scope. Read because it is part of the <b>URL</b>, not part of the
|
||||
/// driver's tag/timing configuration — see the "reachable ≠ deployable" remarks on the type.
|
||||
/// </summary>
|
||||
public string? DeviceName { get; set; }
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
@@ -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,
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user