lmxopcua/code-reviews/Driver.MTConnect/findings.md

Code Review — Driver.MTConnect

Field	Value
Module	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect
Reviewer	Claude (per-module review)
Review date	2026-08-10
Commit reviewed	b990adae
Status	Complete
Open findings	5

Checklist coverage

A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank.

#	Category	Result
1	Correctness & logic bugs	3 findings — Driver.MTConnect-002, Driver.MTConnect-003, Driver.MTConnect-006
2	OtOpcUa conventions	No issues found. DriverType and the factory's DriverTypeName are both aliased to DriverTypeNames.MTConnect (the TwinCat/Focas drift fix); every status code is a NAMED const matching an Opc.Ua.StatusCodes member, which the StatusCodeParityTests reflection guard requires; there is deliberately no JsonStringEnumConverter on the config reader, so enum-carrying DTO fields stay string? and go through ParseEnum. Authorization is never decided here — the driver reports SecurityClassification as browse metadata only, and says so explicitly. .NET 10 / AnyCPU, Serilog via ILogger<T> with a NullLogger default, no backend NuGet.
3	Concurrency & thread safety	1 finding — Driver.MTConnect-005. The lock discipline is otherwise sound and documented per field: _lifecycle (non-reentrant, never entered from the pump or the probe loop), _subscriptionLock (never held across an await or a callback; lock order _lifecycle → it; released before StopSampleStreamAsync), _probeLock (field read/write only). _session / _binding / _index are immutable snapshots published by Volatile.Write or compare-and-swap, so a lock-free reader cannot pair one initialize's client with another's model. Subscriber callbacks are raised outside every lock with the invocation list walked by hand, so one throwing consumer cannot starve the others.
4	Error handling & resilience	1 finding — Driver.MTConnect-001. Otherwise strong: failure is never dressed as success (a /probe that succeeds while the priming /current fails is Faulted, not "healthy with no data"); a quiet end of the /sample enumeration is reported rather than read as idle; an unreadable config document faults only a driver that had no working session; Faulted is reserved for a failed initialize because DriverHealthReport maps it to a /readyz 503; /current and /sample own separate degrade flags so neither papers over the other's failure.
5	Security	1 finding — Driver.MTConnect-004 (a capability gate nobody can turn on, not a weakness). No credential is logged: MTConnectDriverProbe builds every message from a scheme/host/port/path redaction that excludes Uri.UserInfo, and its last-resort catch forwards only the exception TYPE. The Agent's refusal text reaches a log line, never DriverHealth.LastError. Write-back is off at two independent gates and CONDITION items are excluded. XDocument.Parse/Load run under the BCL default DtdProcessing.Prohibit, so the XML paths are not XXE-exposed, and the multipart reader caps its buffer at 32 MB against a hostile Agent. Every caller-supplied URL part is percent-encoded at the seam.
6	Performance & resource management	1 finding — Driver.MTConnect-007. Otherwise careful: one /current per read batch whatever the batch size; one shared /sample long poll behind every subscription handle; FrozenDictionary for the RawPath↔dataItemId tables; a separator-insensitive comparer that avoids allocating on the discovery hot path; a multipart reader that never buffers more than the part in flight. Each CancellationTokenSource is disposed only once its loop is provably gone (an abandoned loop still holds the token), and the two HttpClients exist precisely so the unary deadline stays bounded while the long poll is not.
7	Design-document adherence	1 finding — Driver.MTConnect-008 (stale claims, corrected). The implementation otherwise matches docs/drivers/MTConnect.md and docs/plans/2026-08-05-mtconnect-writeback-design.md: IWritable is observation injection through AllowPut, doubly gated and CONDITION-excluded; RediscoverPolicy.Once plus an instanceId watch; the dormant discovered-node injection path is correctly not relied on.
8	Code organization & conventions	No issues found. Capability seams are implemented on the concrete type and the factory deliberately returns it un-narrowed, which is why IWritable was dispatched with no factory edit. The Options pattern is honoured, the config document has exactly one parser (ParseOptions, which the factory delegates to rather than mirroring), and the transport sits behind IMTConnectAgentClient so the whole driver is exercised with no socket.
9	Testing coverage	No issues found. 538 tests across 14 classes cover the lifecycle, read, write, subscribe, discovery, probe, both parsers, the observation index and the multipart framing; each way the stream can be knocked off sequence (gap, OUT_OF_RANGE, restart, drop, not-supported) has a test, several with an explicit falsification control. Nothing waits on wall-clock time — the pump exposes its task as the teardown barrier and the probe cadence is an injected scheduler. The two gaps this review found are Driver.MTConnect-001 / -002; both now have tests, each verified red first.
10	Documentation & comments	1 finding — Driver.MTConnect-008. The XML documentation is unusually thorough and states the reasoning (why a lock is not reentrant, why an exception beats an empty result, why a fallback exists) rather than restating the code. Both stale items found were premises another module had since invalidated, not drifted descriptions of this code.

Findings

Driver.MTConnect-001

Field	Value
Severity	Medium
Category	Error handling & resilience
Location	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:1932
Status	Resolved

Description: The /sample pump's StreamVerdict.Reopen arm reset the reconnect ladder unconditionally (attempt = 0; _reconnectAttempts = 0;) and reopened the stream with no backoff. That is right for the two conditions the verdict was written for — a sequence gap and an Agent OUT_OF_RANGE rejection — because the /current re-baseline genuinely resolves both, so neither recurs immediately.

But the catch (InvalidDataException) in ConsumeStreamAsync that routes OUT_OF_RANGE to a re-baseline also catches two conditions a /current cannot resolve: a chunk MTConnectStreamsParser cannot parse (thrown from inside the await foreach) and a part larger than the multipart reader's 32 MB cap. Against an Agent producing either, the pump ran /sample → parse failure → /current (succeeds) → Reopen with the ladder reset → /sample again, with no delay at any point — an unbounded hot loop issuing two HTTP requests per iteration as fast as the Agent can answer, and one Warning per iteration. That is exactly the failure mode every other arm of this pump is written to prevent (the _streamUnsupported latch, BackoffGrowthFloorMs, the non-positive-cap fallback).

Recommendation: Use the discriminator the transient arm already uses — did this connection deliver a chunk? A re-baseline behind a stream that delivered stays an immediate reopen on a clean ladder; one on a connection that delivered nothing advances BackoffFor like any other failure.

Resolution: Fixed 6c63bf8f, 2026-08-10 — the Reopen arm now sets attempt = outcome.DeliveredAny ? 0 : attempt + 1, so a re-baseline that resolved nothing backs off while a buffer roll behind a working stream still reopens instantly. Pinned by A_rebaseline_that_delivers_nothing_still_advances_the_reconnect_backoff (red before the change: ReconnectAttempts stayed 0 across six stream generations) plus the falsification control A_rebaseline_after_a_delivered_chunk_still_reopens_immediately, which fails any "back off on every re-baseline" over-correction.

Driver.MTConnect-002

Field	Value
Severity	Medium
Category	Correctness & logic bugs
Location	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs:192
Status	Resolved

Description: MTConnectDriverProbe (the AdminUI Test Connect probe) parsed only agentUri from the driver config and requested {AgentUri}/probe, while MTConnectAgentClient narrows every real request to {AgentUri}/{DeviceName}/probe. A driver scoped to a device therefore had its reachability tested against a different URL from the one it will use.

The consequence is the failure-that-looks-like-success shape: a mistyped, since-renamed, or not-yet-deployed deviceName shows GREEN on Test Connect and then Faults the driver at InitializeAsync — a cppagent answers an unknown device path with an MTConnectError document under HTTP 200, which the parser surfaces as InvalidDataException. The type's own "reachable ≠ deployable" remark did not cover this: the examples it names (requestTimeoutMs, a malformed tag entry) do not change the endpoint, and which endpoint is reached is the one thing a reachability probe exists to establish.

Recommendation: Read deviceName into the probe's minimal DTO and pass it into the MTConnectDriverOptions handed to MTConnectAgentClient, so the probe issues the driver's own request path. Keep the deliberate independence from ParseOptions — the distinction is "which keys compose the URL", not "which keys are cheap to read".

Resolution: Fixed 6c63bf8f, 2026-08-10 — the probe DTO gained DeviceName and forwards it, and the type remarks now state the URL-composing rule. Pinned by Configured_deviceName_scopes_the_probe_request_the_way_the_driver_does (red before the change: requested /probe, expected /VMC-1/probe) and the control An_absent_deviceName_leaves_the_probe_agent_wide.

Driver.MTConnect-003

Field	Value
Severity	High
Category	Correctness & logic bugs
Location	src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs:256 (cause) · src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:3286 (BuildBinding precedence)
Status	Open

Description: Every browse-committed MTConnect tag is coerced to String at runtime while its OPC UA node is typed from the probe inference, so a browsed SAMPLE publishes text into a Double node.

RawBrowseCommitMapper.MapLeaf does two things with one browsed leaf:

• it sets the raw tag row's OPC UA DataType from the leaf's inferred DriverDataType (MapDataType: Float64 → Double, Int64 → Int64); and
• it calls BuildTagConfig, whose MTConnect branch is new MTConnectTagConfigModel { FullName = address }.ToJson().

MTConnectTagConfigModel.DataType defaults to DriverDataType.String and ToJson() unconditionally writes dataType, so the committed blob is {"fullName":"dev1_pos","dataType":"String"}. That is a non-blank authored type, so MTConnectTagDefinitionFactory.TryFromTagConfig returns a definition, BuildBinding installs it as "the deploy-time authority", and step 3 of the documented coercion precedence — the Agent's own /probe declaration via MTConnectDataTypeInference — is never reached for a browse-committed tag. The driver's own remark that this step "is what makes a browse-committed tag work" rested on a premise (RawBrowseCommitMapper writes only the address) that was true when written and is false at HEAD.

This is the same defect class as Gitea #536, which is exactly why the AbCip, TwinCAT and FOCAS branches of BuildTagConfig forward the browsed native type via NativeType(addressFields, …). MTConnect was given the typed-model branch without that forwarding.

Severity is High rather than Medium because the address picker is the primary authoring path for this driver, the mistyping is silent (no warning at authoring and none at the deploy gate — the blob is perfectly valid), and the resulting node advertises a numeric type it will never carry.

Recommendation: Cross-module — do not fix in this driver. In RawBrowseCommitMapper, forward the browsed leaf's DriverDataType into the MTConnect branch as the other three typed-model branches already do. MapLeaf already holds driverDataType; either thread it into BuildTagConfig alongside addressFields, or supply it through addressFields under the dataType key NativeType already reads. Add the MTConnect case to whatever test pins the #536 behaviour for AbCip/TwinCAT/FOCAS, and assert the pair — that the tag row's OPC UA type and the blob's coercion type agree — rather than either alone: asserting only the blob would pass against a mapper that had stopped setting the row's type.

Resolution: (open — the fix belongs in ZB.MOM.WW.OtOpcUa.AdminUI, outside this review's modules. The stale premises this defect invalidated inside these two modules are corrected under Driver.MTConnect-008 and Driver.MTConnect.Contracts-001.)

Driver.MTConnect-004

Field	Value
Severity	Medium
Category	Design-document adherence
Location	src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor:1 · src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs:146
Status	Open

Description: IWritable shipped for this driver on 2026-08-05 (b29623cf) behind Write.Enabled, and docs/drivers/MTConnect.md §"Two independent gates" tells the operator to set "Write": { "Enabled": true } in the driver config. There is no way to author that through the product. MTConnectDriverForm is the only editor DriverFormMaps maps for this driver type, it exposes no write section, and its header comment still asserts "MTConnect is read-only — there are no write knobs to author". DriverConfigModal renders the typed form only (no raw-JSON escape hatch), and the AdminUI exposes no driver-instance API — its only Map* endpoints are the deploy and script-analysis ones. So the documented enablement path exists only as a direct edit of DriverInstance.DriverConfig in the config DB.

Mitigating, and worth knowing before fixing: MTConnectDriverForm.BuildConfigJson preserves unknown top-level keys, so a write block introduced by any other means is not destroyed by a later edit through the form. The capability is unreachable rather than fragile.

Recommendation: Cross-module — add a "Write-back" section to MTConnectDriverForm with an Enabled switch and form text stating that it is observation injection, that the Agent's own AllowPut is the other gate, and that it does not by itself make any node writable over OPC UA (Tag.AccessLevel still composes the CurrentWrite bit). Add Write to the form's mirrored MTConnectConfigDto so the value round-trips, and delete the now-false "MTConnect is read-only" comment. Until then the driver-side gate is correct and fails closed, which is why this is Medium rather than High.

Resolution: (open — the fix belongs in ZB.MOM.WW.OtOpcUa.AdminUI, outside this review's modules.)

Driver.MTConnect-005

Field	Value
Severity	Low
Category	Concurrency & thread safety
Location	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:2655 · :689 · :797 · :1425
Status	Open

Description: _options is the one piece of shared driver state published by a plain field write (_options = options; at the StartCoreAsync commit point) and read lock-free from other threads — ReadAsync, WriteAsync, DiscoverAsync, HostName, DegradeAfterReadFailure. Every other value published at that same commit point (_session, _binding, _index, _announcedInstanceId) uses Volatile.Write and is read with Volatile.Read, and the field remarks explain at length why that matters.

The practical exposure is small — a reference-typed field cannot tear, and the Volatile.Write of _binding two statements later acts as a release fence for it — but a reader that reads _options before it reads _binding (which both ReadAsync and WriteAsync do) has no acquire ordering between them, so it can in principle judge a batch against a stale options document paired with the new tag table. The worst observable outcome is one batch judged against the previous Write.Enabled or RequestTimeoutMs.

Recommendation: Publish and read _options with Volatile.Write / Volatile.Read like its three siblings, or fold it into the AgentSession / TagBinding snapshot so the "read once, as one consistent unit" rule this file already argues for covers it too. Record-only for now: the change touches ~10 read sites and the exposure does not justify unreviewed churn.

Resolution: (open)

Driver.MTConnect-006

Field	Value
Severity	Low
Category	Correctness & logic bugs
Location	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:914
Status	Open

Description: TryFormatValue refuses null and Array, then renders everything else with Convert.ToString(value, InvariantCulture). Its own remark gives the right reason for the array guard — Convert.ToString falls back to object.ToString() for a non-IConvertible value, so an array would be injected into the Agent as the literal text System.Double[], which the Agent stores happily and which stays invisible until someone reads it back. That reasoning is not specific to arrays: any other non-IConvertible value reaching this method (an Opc.Ua.ExtensionObject, an XmlElement, a List<T> from a non-SDK caller) is injected as its type name under a Good status.

Not reachable today by the production path — the SDK coerces a written value against the node's DataType before DriverInstanceActor.HandleWriteAsync builds the WriteRequest, and every MTConnect tag is a scalar Boolean/numeric/String — hence Low.

Recommendation: Refuse on the property the remark actually relies on rather than on one type that happens to have it: if (value is null or Array or not IConvertible) return false;. string, bool, every numeric and DateTime are IConvertible, so the accepted set is unchanged.

Resolution: (open)

Driver.MTConnect-007

Field	Value
Severity	Low
Category	Performance & resource management
Location	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs:85
Status	Open

Description: MTConnectAgentClient.Dispose() is a plain check-then-set on a non-volatile _disposed field, so two concurrent disposals can both pass the guard. The second then calls _disposeCts.Cancel() on an already-disposed CancellationTokenSource, which throws ObjectDisposedException. The class documents that disposal is safe against an in-flight enumeration but claims nothing about concurrent disposal, and in practice the driver disposes only under _lifecycle and wraps the call in SafeDispose, so the throw is caught and traced — hence Low rather than Medium.

Recommendation: Guard with Interlocked.Exchange(ref _disposed, 1) != 0 over an int field, or state on Dispose that it is not thread-safe and is called only under the driver's lifecycle lock.

Resolution: (open)

Driver.MTConnect-008

Field	Value
Severity	Low
Category	Documentation & comments
Location	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:3210 · docs/drivers/MTConnect.md:169 · MTConnectDriver.cs:1227/:1245/:1255
Status	Resolved

Description: Three stale or self-contradicting pieces of documentation, each of which would lead a reader to a wrong conclusion:

1. BuildBinding's remarks state that the probe-inference step of the coercion precedence "is what makes a browse-committed tag work: RawBrowseCommitMapper writes only the address into the blob". That premise is false at HEAD and the step is in fact dead for browse-committed tags — see Driver.MTConnect-003.
2. docs/drivers/MTConnect.md §"FullName == DataItem id" says address "is the field name RawBrowseCommitMapper writes" for MTConnect. It writes fullName; address is the legacy spelling that must stay readable.
3. DiscoverAsync reads var options = _options; once, with an explicit comment that a concurrent re-initialize must not scope half the walk to one device and half to another — then logs _options.AgentUri (the field) at all three of its log sites. Harmless in effect, but it contradicts the rule stated two lines above it, which is how such a rule gets broken later.

Recommendation: Correct (1) and (2) to describe HEAD and cross-reference the live defect rather than deleting the rule, which is still correct for hand-authored, CSV and API blobs; make (3) use the snapshot it deliberately took.

Resolution: Fixed 6c63bf8f, 2026-08-10 — BuildBinding and docs/drivers/MTConnect.md now carry an explicit warning naming Driver.MTConnect-003 and the mapper branch responsible, and the three DiscoverAsync log sites use the once-read options snapshot.