diff --git a/code-reviews/Driver.MTConnect/findings.md b/code-reviews/Driver.MTConnect/findings.md index 2877ed1a..f7fa07f0 100644 --- a/code-reviews/Driver.MTConnect/findings.md +++ b/code-reviews/Driver.MTConnect/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Complete | -| Open findings | 4 | +| Open findings | 0 | ## Checklist coverage @@ -168,7 +168,7 @@ defect left inside these two modules are corrected under `Driver.MTConnect-008` | 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 | +| Status | Resolved | **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 @@ -192,8 +192,32 @@ edit through the form. The capability is unreachable rather than fragile. 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.)_ +**Resolution (2026-08-10):** Fixed `298ad04f`. `MTConnectDriverForm` gained a **Write-back** +section — a switch plus form text stating that a write is observation injection, that the Agent's own +`AllowPut` is the independent second gate, and that arming it does not by itself make any node +writable over OPC UA (`Tag.AccessLevel` still composes `CurrentWrite`; CONDITION items never are). +`Write` was added to the form's mirrored `MTConnectConfigDto` and to `MTConnectFormModel`, the false +"MTConnect is read-only" header comment is gone, and `docs/drivers/MTConnect.md` §"Two independent +gates" now names the section instead of implying a config-DB edit. + +⚠️ **The block is written in BOTH states, unlike the timing knobs.** Those are omitted when unusable +so `ParseOptions` substitutes the driver's own default; an omitted `write` block would instead leave a +previously-authored `"enabled": true` standing — the switch would appear to turn writing off while the +deployed driver went on accepting writes. Pinned by +`Turning_write_back_off_clears_a_previously_authored_true`, the falsification control for that +decision. The finding's own mitigation (unknown top-level keys survive an edit) is likewise not +sufficient on its own: `FromJson` had to LEARN the block, or the modal would show the switch off over +a config that has writing on — `Write_back_round_trips_and_a_hand_authored_block_loads_into_the_form`. + +This project has no bUnit, so the seam is unit-tested from both ends rather than through the page: +`MTConnectDriverFormModelTests` asserts the exact bytes `BuildConfigJson` emits, and +`MTConnectFactoryTests.The_document_the_AdminUI_form_emits_arms_write_back` parses that same document +through `MTConnectDriver.ParseOptions` and asserts `MTConnectDriverOptions.Write.Enabled` is true +(then false for the switched-off variant). The literal is duplicated because a driver test project +cannot reference the AdminUI; `The_write_enabled_document_is_the_literal_the_driver_test_parses` pins +the two copies together so a rename fails on the authoring side, naming the literal to update. +**Residual: live page verification is still a rig step** — the razor markup itself (the switch's +binding and `EmitAsync` wiring) is not covered by any test here. ### Driver.MTConnect-005 @@ -202,7 +226,7 @@ modules.)_ | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:2655` · `:689` · `:797` · `:1425` | -| Status | Open | +| Status | Resolved | **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 @@ -223,7 +247,17 @@ three siblings, or fold it into the `AgentSession` / `TagBinding` snapshot so th 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)_ +**Resolution (2026-08-10):** Fixed `2d7dffcc`. Both writes are `Volatile.Write` (the constructor's as +well as the commit point's) and all thirteen reads are `Volatile.Read`; the field gained the remark +its three siblings already carried. Two sites took a local snapshot rather than repeating the read — +the `_streamUnsupported` refusal, which named the Agent twice, and `GetOrFetchProbeModelAsync`. + +**A source scan is the pin, deliberately.** The defect is a missing fence, and no test can force the +interleaving that exposes it on x86/arm64 in bounded time; what actually regresses is a NEW read site +written without the fence. `MTConnectOptionsPublicationTests` therefore fails on any unguarded touch +of `_options` in `MTConnectDriver.cs` — the same shape as `LibplctagGatewayConsumptionGuardTests`, and +red before the change with all 15 sites listed by line. It carries a floor count so the scan cannot go +blind if the field is renamed. ### Driver.MTConnect-006 @@ -232,7 +266,7 @@ touches ~10 read sites and the exposure does not justify unreviewed churn. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:914` | -| Status | Open | +| Status | Resolved | **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 @@ -250,7 +284,20 @@ MTConnect tag is a scalar Boolean/numeric/String — hence Low. 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)_ +**Resolution (2026-08-10):** Fixed `2d7dffcc` exactly as recommended (`Array` kept in the pattern as +documentation of the shape this was first noticed on, though it is subsumed). Red first: +`A_non_convertible_composite_value_is_refused_as_a_type_mismatch` wrote a `List` and got +`Good` back, having injected the list's own type name into the Agent as the observation value. +The falsification control is the load-bearing half — `An_IConvertible_scalar_still_writes` plus +`A_DateTime_still_writes_as_its_invariant_string` fail any over-correction expressed as "is this a +primitive I recognise", which would start refusing real writes. + +⚠️ **Worth knowing: the fix stranded the vehicle of an existing test.** +`Cancellation_mid_batch_leaves_the_slots_already_written_with_their_real_outcomes` injected its +cancellation from a value's own `ToString`, and such a value is not `IConvertible` — so it was refused +before it was ever rendered and the cancellation never happened. The injection point moved to the +Agent call (`CannedAgentClient.AfterWrite`), which is the seam that test is asserting around anyway; +its determinism property is unchanged. ### Driver.MTConnect-007 @@ -259,7 +306,7 @@ that happens to have it: `if (value is null or Array or not IConvertible) return | Severity | Low | | Category | Performance & resource management | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs:85` | -| Status | Open | +| Status | Resolved | **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 @@ -272,7 +319,16 @@ 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)_ +**Resolution (2026-08-10):** Fixed `2d7dffcc` — `_disposed` is an `int` claimed with +`Interlocked.Exchange`, the four request seams read it with `Volatile.Read`, and `Dispose` now states +that it is idempotent under a race. Pinned by `Concurrent_disposals_are_idempotent`. + +**Its budget is part of the fix.** The window is a handful of instructions and the loser only throws +if it reaches `Cancel()` after the winner has finished disposing two `HttpClient`s, so a small +race proves nothing: four racers over 200 rounds passed against the DEFECTIVE code. Eight racers on a +barrier over 3000 rounds failed at round 690, which is the red this resolution rests on. Once the +claim is atomic no schedule can fail it, so the test cannot flake in either direction — but do not +shrink the budget. ### Driver.MTConnect-008 diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index a93825bb..a2d62af9 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -284,7 +284,10 @@ for why Interfaces was rejected as the write-back shape. Design record: `AllowPutFrom = ` so only named hosts may write; that list is the Agent's own access control and this driver neither reads nor influences it. 2. **The driver** — `"Write": { "Enabled": true }` in the driver config (default - `false`; an absent block is disabled). + `false`; an absent block is disabled). Authored in the AdminUI on the `/raw` driver + config modal's **Write-back** section (`MTConnectDriverForm`), which writes the block + explicitly in both states — so turning the switch back off clears a previously + authored `true` rather than leaving it standing. Either gate alone leaves the shipped read-only behaviour unchanged: with `Write.Enabled` off every slot is refused `BadNotWritable` without an Agent being touched, and with diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs index f40bf3e1..9bd36b8c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs @@ -82,7 +82,16 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable private readonly string _sampleQuerySuffix; private readonly TimeSpan _requestTimeout; private readonly TimeSpan _streamIdleTimeout; - private bool _disposed; + + /// + /// 1 once has claimed the teardown. An rather + /// than a bool so the claim is an : + /// a check-then-set let two concurrent disposals both pass, and the loser then cancelled a + /// the winner had already disposed + /// (Driver.MTConnect-007). Read with Volatile.Read at the request seams for the same + /// reason. + /// + private int _disposed; /// Creates a client for the Agent described by . /// The driver options carrying the Agent URI, device scope, and per-call deadline. @@ -157,7 +166,7 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable /// public async Task ProbeAsync(CancellationToken ct) { - ObjectDisposedException.ThrowIf(_disposed, this); + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); // ResponseContentRead (the default) buffers the whole body *under* this awaited, token-bound // call, so the parse below reads from memory — no network read can outlive the deadline. @@ -171,7 +180,7 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable /// public async Task CurrentAsync(CancellationToken ct) { - ObjectDisposedException.ThrowIf(_disposed, this); + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); using var response = await SendAsync(_currentUri, ct).ConfigureAwait(false); @@ -185,7 +194,7 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable long from, [EnumeratorCancellation] CancellationToken ct) { - ObjectDisposedException.ThrowIf(_disposed, this); + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); var uri = BuildSampleUri(from); @@ -257,7 +266,7 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable public async Task WriteObservationAsync( string deviceName, string dataItemId, string value, CancellationToken cancellationToken) { - ObjectDisposedException.ThrowIf(_disposed, this); + ObjectDisposedException.ThrowIf(Volatile.Read(ref _disposed) != 0, this); ArgumentException.ThrowIfNullOrWhiteSpace(deviceName); ArgumentException.ThrowIfNullOrWhiteSpace(dataItemId); ArgumentNullException.ThrowIfNull(value); @@ -371,15 +380,18 @@ public sealed class MTConnectAgentClient : IMTConnectAgentClient, IDisposable } /// + /// + /// Idempotent, including under a race: the teardown is claimed with a single atomic exchange, + /// so a second concurrent caller returns rather than cancelling an already-disposed + /// (Driver.MTConnect-007). + /// public void Dispose() { - if (_disposed) + if (Interlocked.Exchange(ref _disposed, 1) != 0) { return; } - _disposed = true; - // Cancel BEFORE disposing the clients so an in-flight read observes cancellation rather // than a disposed handler. _disposeCts.Cancel(); 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 ced2d4da..95f76c3e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -292,6 +292,21 @@ public sealed class MTConnectDriver /// private long _announcedInstanceId; + /// + /// The options document currently in force. Installed at the StartCoreAsync commit + /// point alongside / / + /// , and read lock-free from ReadAsync, + /// WriteAsync, DiscoverAsync, HostName and the health paths. + /// + /// Published with Volatile.Write and read with Volatile.Read, exactly + /// like its siblings, so a reader that + /// takes this field BEFORE it takes — which both ReadAsync + /// and WriteAsync do — has the ordering that stops it judging a batch against the + /// previous document's Write.Enabled / RequestTimeoutMs paired with the new + /// tag table (Driver.MTConnect-005). Pinned by + /// MTConnectOptionsPublicationTests, which fails on any unfenced touch. + /// + /// private MTConnectDriverOptions _options; /// @@ -418,7 +433,7 @@ public sealed class MTConnectDriver // Sets _options AND the tag tables derived from it. No /probe model is available here (the // constructor dials nothing), so a raw tag whose blob declares no type falls back to String // until InitializeAsync rebuilds the binding against the Agent's own declaration. - _options = options; + Volatile.Write(ref _options, options); _binding = BuildBinding(options, probeModel: null); // Never null, so every consumer (Task 10's ReadAsync included) can ask it for a snapshot @@ -506,7 +521,7 @@ public sealed class MTConnectDriver internal Task? ProbeLoopTask => Volatile.Read(ref _probeTask); /// The options currently in force — the constructor's, or the last document applied. - internal MTConnectDriverOptions EffectiveOptions => _options; + internal MTConnectDriverOptions EffectiveOptions => Volatile.Read(ref _options); /// /// @@ -581,7 +596,7 @@ public sealed class MTConnectDriver await StopSampleStreamAsync(cancellationToken).ConfigureAwait(false); await StopProbeLoopAsync(cancellationToken).ConfigureAwait(false); - if (existing is null || RequiresClientRebuild(_options, incoming)) + if (existing is null || RequiresClientRebuild(Volatile.Read(ref _options), incoming)) { await TeardownCoreAsync().ConfigureAwait(false); await StartCoreAsync(incoming, existingClient: null, cancellationToken).ConfigureAwait(false); @@ -686,7 +701,7 @@ public sealed class MTConnectDriver // One read of the session reference: the client used below is the one that was live at this // instant, never a half-swapped pairing (see the _session remarks). var client = Volatile.Read(ref _session)?.Client; - var options = _options; + var options = Volatile.Read(ref _options); // Read once, for the same reason the session is: every reference in this batch must be // resolved against ONE tag table, not half against the table a concurrent re-initialize @@ -794,7 +809,7 @@ public sealed class MTConnectDriver // Read once each, for the same reason ReadAsync does: every slot in this batch is judged // against ONE options document and ONE tag table, never half against a table a concurrent // re-initialize just replaced. - var options = _options; + var options = Volatile.Read(ref _options); var client = Volatile.Read(ref _session)?.Client; var binding = Volatile.Read(ref _binding); @@ -895,12 +910,18 @@ public sealed class MTConnectDriver /// it is what an MTConnect consumer will see verbatim. /// /// - /// Arrays are refused rather than rendered. + /// Anything that is not is refused rather than rendered. /// falls back to /// for a non- value, so an array /// would be written as the literal text System.Double[] — which the Agent stores /// happily, making the corruption invisible until someone reads the observation back. - /// TIME_SERIES / DATA_SET writes are out of scope for v1. + /// The guard is therefore written against that property itself rather than against + /// , the one type it was first noticed on: an + /// Opc.Ua.ExtensionObject, an XmlElement or a List<T> from a + /// non-SDK caller corrupts an observation in exactly the same way (Driver.MTConnect-006). + /// string, bool, and every numeric type ARE + /// , so the accepted set is unchanged. TIME_SERIES / DATA_SET + /// writes are out of scope for v1. /// /// /// Every formatting throw is caught, not just the documented conversion ones. The @@ -916,8 +937,10 @@ public sealed class MTConnectDriver text = string.Empty; // A PUT query has no null to send, and the empty string is a real observation rather than an - // absence — so "no value" is refused instead of being silently turned into one. - if (value is null or Array) + // absence — so "no value" is refused instead of being silently turned into one. Everything + // that is not IConvertible is refused with it: Convert.ToString renders such a value as its + // TYPE NAME, which the Agent stores under a Good status (see the remarks). + if (value is null or Array or not IConvertible) { return false; } @@ -1202,7 +1225,7 @@ public sealed class MTConnectDriver // to another. // Both read once, together: a concurrent re-initialize must not scope half this walk to one // device and half to another, nor stamp half its leaves writable and half not. - var options = _options; + var options = Volatile.Read(ref _options); var deviceScope = options.DeviceName?.Trim(); var writeEnabled = options.Write.Enabled; @@ -1422,7 +1445,7 @@ public sealed class MTConnectDriver /// host:port convention: several MTConnect drivers in one server disambiguate on the /// Admin dashboard by endpoint rather than by driver-instance id. /// - public string HostName => _options.AgentUri; + public string HostName => Volatile.Read(ref _options).AgentUri; /// /// @@ -1796,12 +1819,14 @@ public sealed class MTConnectDriver // that is not running; without this, the resubscribe returns a handle, the caller is // told SubscriptionEstablished, and the next successful read reports a perfectly green // driver over a subscription that can never deliver a value (#485). + var agentUri = Volatile.Read(ref _options).AgentUri; + _streamDegraded = true; - Degrade($"The MTConnect Agent at '{_options.AgentUri}' does not serve a framed /sample stream; subscriptions cannot deliver until the driver is re-initialized."); + Degrade($"The MTConnect Agent at '{agentUri}' does not serve a framed /sample stream; subscriptions cannot deliver until the driver is re-initialized."); _logger.LogWarning( "MTConnect driver {DriverInstanceId} refused to start a /sample stream for {AgentUri}: the endpoint was already found not to stream. The subscription is registered but will receive NO updates until the driver is re-initialized.", - _driverInstanceId, _options.AgentUri); + _driverInstanceId, agentUri); return; } @@ -1828,7 +1853,7 @@ public sealed class MTConnectDriver // _subscriberFaultLogged). Interlocked.Exchange(ref _subscriberFaultLogged, 0); - var options = _options; + var options = Volatile.Read(ref _options); var cts = new CancellationTokenSource(); _pumpCts = cts; _pumpTask = Task.Run( @@ -2497,7 +2522,8 @@ public sealed class MTConnectDriver MTConnectProbeModel model; try { - model = await BoundedAsync(session.Client.ProbeAsync, "/probe", _options.RequestTimeoutMs, cancellationToken) + model = await BoundedAsync( + session.Client.ProbeAsync, "/probe", Volatile.Read(ref _options).RequestTimeoutMs, cancellationToken) .ConfigureAwait(false); } catch (ObjectDisposedException ex) @@ -2652,7 +2678,7 @@ public sealed class MTConnectDriver .ConfigureAwait(false); // ---- commit point: everything below is non-throwing bookkeeping ---- - _options = options; + Volatile.Write(ref _options, options); // Derived from the options AND the /probe model just fetched: a raw tag whose TagConfig // blob declares no type takes the type the Agent itself declares for that DataItem, so a @@ -2904,7 +2930,7 @@ public sealed class MTConnectDriver { return HasConfigBody(driverConfigJson) ? ParseOptions(_driverInstanceId, driverConfigJson) - : _options; + : Volatile.Read(ref _options); } catch (Exception ex) { @@ -3013,7 +3039,7 @@ public sealed class MTConnectDriver { // A bare "A task was canceled" names neither the Agent nor the request that stalled. throw new TimeoutException( - $"MTConnect {request} request for driver '{_driverInstanceId}' against '{_options.AgentUri}' did not complete within {timeoutMs} ms."); + $"MTConnect {request} request for driver '{_driverInstanceId}' against '{Volatile.Read(ref _options).AgentUri}' did not complete within {timeoutMs} ms."); } } @@ -3050,7 +3076,7 @@ public sealed class MTConnectDriver _logger.LogWarning( ex, "MTConnect driver {DriverInstanceId} could not read /current from {AgentUri}; the batch is reported Bad.", - _driverInstanceId, _options.AgentUri); + _driverInstanceId, Volatile.Read(ref _options).AgentUri); } /// Publishes without downgrading a Faulted driver. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor index 1bbd9674..5da6e6c6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor @@ -3,7 +3,9 @@ paths (/probe, /current, /sample) to it and optionally narrows to one DeviceName, so unlike Modbus/S7 there is no host/port to split onto the device. - MTConnect is read-only — there are no write knobs to author. *@ + Write-back is authored here too (Driver.MTConnect-004): the driver gained IWritable behind + write.enabled, and until this section existed the documented enablement path was a hand edit of + DriverInstance.DriverConfig in the config DB. *@ @using System.Text.Json @using System.Text.Json.Nodes @using System.Text.Json.Serialization @@ -109,6 +111,34 @@ +@* Write-back *@ +
+
Write-back
+
+
+
+
+ + +
+
Default off.
+
+
+
+ An MTConnect write is observation injection: it sets what the Agent + reports for a DataItem, not the machine itself, and the next adapter update overwrites it. +
+ This is only one of two gates — the Agent's own AllowPut (its + agent.cfg) must be on as well, and neither gate can arm the other. + Turning this on does not by itself make any node writable over OPC UA: the tag's + AccessLevel still composes the CurrentWrite bit. + CONDITION items are never writable. +
+
+
+
+
+ @if (_form.Validate() is { } error) {
@error
@@ -165,8 +195,8 @@ /// /// Unknown TOP-LEVEL keys in are preserved (notably the /// driver's hand-authored tags[] surface, which this form does not expose and must not - /// silently delete). The probe and reconnect sub-objects are replaced wholesale - /// — the form owns every field in both. + /// silently delete). The probe, reconnect and write sub-objects are + /// replaced wholesale — the form owns every field in all three. /// /// /// A key whose form value is unusable (blank URI, non-positive timing knob) is REMOVED rather @@ -268,6 +298,12 @@ public int ReconnectMaxBackoffMs { get; set; } = 30_000; /// Multiplier applied to the reconnect backoff each retry. public double ReconnectBackoffMultiplier { get; set; } = 2.0; + /// + /// Whether the driver's half of the write-back gate is armed. Default false — the + /// driver shipped read-only, and the Agent's own AllowPut is an independent second + /// gate that this cannot arm. + /// + public bool WriteEnabled { get; set; } /// Loads a form model from a DriverConfig JSON document, falling back to the driver's /// own defaults for anything absent or unusable. @@ -293,6 +329,7 @@ ReconnectMinBackoffMs = dto.Reconnect?.MinBackoffMs ?? defaults.ReconnectMinBackoffMs, ReconnectMaxBackoffMs = dto.Reconnect?.MaxBackoffMs ?? defaults.ReconnectMaxBackoffMs, ReconnectBackoffMultiplier = dto.Reconnect?.BackoffMultiplier ?? defaults.ReconnectBackoffMultiplier, + WriteEnabled = dto.Write?.Enabled ?? defaults.WriteEnabled, }; } @@ -319,6 +356,11 @@ MaxBackoffMs = Positive(ReconnectMaxBackoffMs), BackoffMultiplier = ReconnectBackoffMultiplier > 0 ? ReconnectBackoffMultiplier : null, }, + + // Written even when false, unlike the timing knobs: those are omitted so the driver + // substitutes its own positive default, whereas an omitted write block would leave a + // previously-authored "enabled": true standing and the switch could never turn it off. + Write = new MTConnectWriteConfigDto { Enabled = WriteEnabled }, }; /// Returns an operator-facing message describing why this config would not start, or @@ -380,6 +422,8 @@ public MTConnectProbeConfigDto? Probe { get; init; } /// Reconnect backoff knobs. public MTConnectReconnectConfigDto? Reconnect { get; init; } + /// Write-back knobs — the driver's half of the two-layer opt-in on observation injection. + public MTConnectWriteConfigDto? Write { get; init; } } /// Wire mirror of the driver's probe config block. @@ -403,4 +447,11 @@ /// Multiplier applied each retry. public double? BackoffMultiplier { get; init; } } + + /// Wire mirror of the driver's write-back config block. + public sealed class MTConnectWriteConfigDto + { + /// Whether the driver accepts write-back at all. + public bool? Enabled { get; init; } + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/CannedAgentClient.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/CannedAgentClient.cs index d6627661..3e752354 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/CannedAgentClient.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/CannedAgentClient.cs @@ -370,6 +370,13 @@ internal sealed class CannedAgentClient : IMTConnectAgentClient, IDisposable /// When set, a write throws this instead of answering — the transport-failure leg. public Exception? WriteFailure { get; set; } + /// + /// Ran after a write has been recorded and before its outcome is returned. The deterministic + /// injection point for something that must happen BETWEEN two slots of one batch — cancelling + /// the batch's token, for instance — with no timing hope involved. + /// + public Action? AfterWrite { get; set; } + /// public Task WriteObservationAsync( string deviceName, string dataItemId, string value, CancellationToken cancellationToken) @@ -377,6 +384,7 @@ internal sealed class CannedAgentClient : IMTConnectAgentClient, IDisposable ObjectDisposedException.ThrowIf(IsDisposed, this); cancellationToken.ThrowIfCancellationRequested(); Writes.Enqueue((deviceName, dataItemId, value)); + AfterWrite?.Invoke(); return WriteFailure is null ? Task.FromResult(WriteOutcome) : throw WriteFailure; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectAgentClientWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectAgentClientWriteTests.cs index ea7d20ec..b431eccb 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectAgentClientWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectAgentClientWriteTests.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Net; using System.Text; using Shouldly; @@ -353,4 +354,54 @@ public sealed class MTConnectAgentClientWriteTests await Should.ThrowAsync( () => client.WriteObservationAsync("StubDevice", "dev1_msg", "hi", CancellationToken.None)); } + + /// + /// Driver.MTConnect-007 — concurrent disposals must not throw out of Dispose. + /// + /// + /// + /// A plain check-then-set on a non-volatile bool lets two racing disposals both pass + /// the guard; the loser then calls Cancel() on a + /// the winner has already disposed, which throws out of + /// Dispose. The driver disposes this client under its lifecycle lock and through + /// SafeDispose, so the throw is caught and traced today — but "the caller happens to + /// catch it" is not idempotence, and nothing on this type says disposal is single-threaded. + /// + /// + /// The window is a handful of instructions, so one pass proves nothing — the racers + /// are released together on a barrier and the whole thing is repeated. Against the + /// unguarded field this fails within a few hundred rounds (observed: round 690); once the + /// guard is an no schedule can fail it, so + /// it cannot flake either way. + /// + /// + [Fact] + public async Task Concurrent_disposals_are_idempotent() + { + const int racers = 8; + + for (var round = 0; round < 3000; round++) + { + var client = new MTConnectAgentClient(Options(), new StubHandler((_, _) => Task.FromResult(Ok()))); + using var start = new Barrier(racers); + var failures = new ConcurrentBag(); + + await Task.WhenAll(Enumerable.Range(0, racers).Select(_ => Task.Run(() => + { + start.SignalAndWait(); + try + { + client.Dispose(); + } + catch (Exception ex) + { + failures.Add(ex); + } + }))); + + failures.ShouldBeEmpty( + $"Dispose must be idempotent under a race (round {round}); " + + string.Join(" | ", failures.Select(e => e.GetType().Name))); + } + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs index 0f7ee5c7..97b5ab5b 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectFactoryTests.cs @@ -382,6 +382,33 @@ public sealed class MTConnectFactoryTests .DriverInstanceId.ShouldBe("mt1"); } + /// + /// Driver.MTConnect-004: the document the AdminUI's MTConnectDriverForm emits arms + /// write-back through this parse. The literal below is the form's own output for a + /// write-enabled instance — key spelling, casing and block nesting included — since the + /// AdminUI cannot be referenced from a driver test project; the form side of the same + /// contract is pinned by MTConnectDriverFormModelTests, which asserts these exact + /// keys come out of BuildConfigJson. + /// + [Fact] + public void The_document_the_AdminUI_form_emits_arms_write_back() + { + const string formEmitted = + """{"agentUri":"http://agent:5000","requestTimeoutMs":5000,"sampleIntervalMs":1000,"sampleCount":1000,"heartbeatMs":10000,"probe":{"enabled":true,"intervalMs":5000,"timeoutMs":2000},"reconnect":{"minBackoffMs":0,"maxBackoffMs":30000,"backoffMultiplier":2},"write":{"enabled":true}}"""; + + var options = MTConnectDriver.ParseOptions("mt1", formEmitted); + + options.Write.Enabled.ShouldBeTrue(); + options.AgentUri.ShouldBe("http://agent:5000"); + + // The same document with the switch off is read-only again — the form writes the block in + // both states, so this is the round trip an operator turning the feature back off performs. + MTConnectDriver.ParseOptions( + "mt1", + formEmitted.Replace("""enabled":true}""", """enabled":false}""", StringComparison.Ordinal)) + .Write.Enabled.ShouldBeFalse(); + } + /// /// A malformed write block fails exactly as its siblings do — the whole /// document is one JsonSerializer.Deserialize, so a non-boolean enabled faults diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectOptionsPublicationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectOptionsPublicationTests.cs new file mode 100644 index 00000000..2596f477 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectOptionsPublicationTests.cs @@ -0,0 +1,109 @@ +using System.Text.RegularExpressions; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests; + +/// +/// Driver.MTConnect-005 — MTConnectDriver._options must be published and read the way its +/// three siblings at the same commit point are (_session, _binding, +/// _announcedInstanceId): Volatile.Write once, Volatile.Read everywhere else. +/// +/// +/// +/// Why a source scan and not a behavioural test. The defect is a missing memory fence: +/// a plain field write publishes the new options document with no release, and a reader that +/// reads _options BEFORE it reads _binding — which both ReadAsync and +/// WriteAsync do — has no acquire ordering between the two, so it can in principle +/// judge a batch against the previous Write.Enabled / RequestTimeoutMs paired +/// with the new tag table. No test can force that interleaving on x86/arm64 in bounded time; +/// what IS checkable, and what actually regresses, is whether a new read site was written +/// with the fence. This is the same shape as +/// LibplctagGatewayConsumptionGuardTests: the defect recurs by being reintroduced at a +/// call site no per-site test covers. +/// +/// +/// The floor below exists so the scan cannot go blind. If the count drops, the field was +/// renamed or the sites were consolidated — re-derive the floor from the code; never lower it +/// to make this pass. +/// +/// +[Trait("Category", "Unit")] +public sealed class MTConnectOptionsPublicationTests +{ + /// Any mention of the field, so an unguarded read is caught as well as an unguarded write. + private static readonly Regex Mention = new(@"_options\b", RegexOptions.Compiled); + + /// The two guarded forms. Nothing else may touch the field. + private static readonly Regex Guarded = + new(@"Volatile\.(Read|Write)\(ref _options[,)]", RegexOptions.Compiled); + + /// The declaration itself, which is neither a read nor a write. + private static readonly Regex Declaration = + new(@"^\s*private\s+MTConnectDriverOptions\s+_options\s*;\s*$", RegexOptions.Compiled); + + [Fact] + public void Every_touch_of_the_shared_options_field_goes_through_Volatile() + { + var source = File.ReadAllLines(Path.Combine( + LocateRepoRoot(), "src", "Drivers", "ZB.MOM.WW.OtOpcUa.Driver.MTConnect", "MTConnectDriver.cs")); + + var offending = new List(); + var guarded = 0; + + for (var i = 0; i < source.Length; i++) + { + var line = source[i]; + var code = StripComment(line); + + if (!Mention.IsMatch(code) || Declaration.IsMatch(code)) + { + continue; + } + + var mentions = Mention.Matches(code).Count; + var fenced = Guarded.Matches(code).Count; + guarded += fenced; + + if (fenced < mentions) + { + offending.Add($"line {i + 1}: {line.Trim()}"); + } + } + + offending.ShouldBeEmpty( + "MTConnectDriver._options is shared state read lock-free from the read, write, discovery " + + "and health paths while a re-initialize replaces it. Publish it with Volatile.Write and " + + "read it with Volatile.Read, exactly as _session / _binding / _announcedInstanceId are " + + $"(Driver.MTConnect-005). Unfenced: {string.Join(" | ", offending)}"); + + guarded.ShouldBeGreaterThanOrEqualTo(12); + } + + /// Removes a trailing line comment and skips XML-doc lines, which mention the field by name. + private static string StripComment(string line) + { + var trimmed = line.TrimStart(); + if (trimmed.StartsWith("//", StringComparison.Ordinal) || trimmed.StartsWith("*", StringComparison.Ordinal)) + { + return string.Empty; + } + + var comment = line.IndexOf("//", StringComparison.Ordinal); + + return comment >= 0 ? line[..comment] : line; + } + + private static string LocateRepoRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + { + dir = dir.Parent; + } + + dir.ShouldNotBeNull("Could not locate the repo root (ZB.MOM.WW.OtOpcUa.slnx) above the test assembly."); + + return dir.FullName; + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs index 936f3b6b..146330e9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs @@ -191,6 +191,57 @@ public sealed class MTConnectWriteTests client.Writes.ShouldBeEmpty(); } + [Fact] + public async Task A_non_convertible_composite_value_is_refused_as_a_type_mismatch() + { + // Driver.MTConnect-006. The array guard's stated REASON is "Convert.ToString falls back to + // object.ToString for a non-IConvertible value", and an array is only one shape carrying that + // property. A List from a non-SDK caller (or an Opc.Ua.ExtensionObject, or an XmlElement) + // renders as its TYPE NAME, which the Agent stores under a Good status — the identical + // invisible corruption the array guard exists to prevent, refused nowhere. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync( + [new WriteRequest(PositionRawPath, new List { 1.0, 2.0 })], + Ct); + + results[0].StatusCode.ShouldBe(BadTypeMismatch); + client.Writes.ShouldBeEmpty(); + } + + [Theory] + [InlineData((byte)7, "7")] + [InlineData('x', "x")] + [InlineData("already text", "already text")] + public async Task An_IConvertible_scalar_still_writes(object value, string expected) + { + // Falsification control for the guard above: the accepted set must be UNCHANGED. Every scalar + // this driver can carry is IConvertible, so a refusal expressed as "not a primitive I + // recognise" — rather than as the IConvertible property the remark relies on — would silently + // start refusing real writes instead of composite ones. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync([new WriteRequest(PositionRawPath, value)], Ct); + + results[0].StatusCode.ShouldBe(Good); + client.Writes.ShouldHaveSingleItem().Value.ShouldBe(expected); + } + + [Fact] + public async Task A_DateTime_still_writes_as_its_invariant_string() + { + // DateTime is IConvertible but is neither a primitive nor a string — the shape most likely to + // be swept up by a hand-rolled "is this a scalar?" guard. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync( + [new WriteRequest(PositionRawPath, new DateTime(2026, 8, 10, 13, 45, 0, DateTimeKind.Utc))], + Ct); + + results[0].StatusCode.ShouldBe(Good); + client.Writes.ShouldHaveSingleItem().Value.ShouldBe("08/10/2026 13:45:00"); + } + [Fact] public async Task A_value_whose_own_formatter_throws_costs_its_slot_and_nothing_more() { @@ -358,16 +409,19 @@ public sealed class MTConnectWriteTests // The fill must start at the cancelled slot, not overwrite the batch. Slot 0 completed against // the Agent before the token was cancelled, and reporting it BadCommunicationError would tell // an operator a write failed that actually landed — the one thing worse than reporting a - // failure. Cancellation is injected from inside slot 1's own formatting, so the ordering is a - // fact of the run rather than a timing hope. + // failure. Cancellation is injected from inside slot 0's own Agent call, so the ordering is a + // fact of the run rather than a timing hope. (It used to be injected from a value's ToString; + // such a value is not IConvertible and is now refused before it is ever rendered — see + // Driver.MTConnect-006 — so the injection point moved to the seam it is asserting around.) var (driver, client) = await WritableDriverAsync(); using var cts = new CancellationTokenSource(); + client.AfterWrite = () => cts.Cancel(); var results = await driver.WriteAsync( [ new WriteRequest(PositionRawPath, 42.5), - new WriteRequest(PositionRawPath, new CancellingFormatter(cts)), new WriteRequest(PositionRawPath, 7.5), + new WriteRequest(PositionRawPath, 9.5), ], cts.Token); @@ -380,17 +434,6 @@ public sealed class MTConnectWriteTests client.Writes.ShouldHaveSingleItem().Value.ShouldBe("42.5"); } - /// Cancels the batch from inside its own rendering, then renders normally. - private sealed class CancellingFormatter(CancellationTokenSource cts) - { - public override string ToString() - { - cts.Cancel(); - - return "99"; - } - } - [Fact] public async Task One_bad_request_does_not_fail_the_others_in_the_batch() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/MTConnectDriverFormModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/MTConnectDriverFormModelTests.cs index b291a90b..0976b3af 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/MTConnectDriverFormModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/MTConnectDriverFormModelTests.cs @@ -128,6 +128,95 @@ public sealed class MTConnectDriverFormModelTests o["reconnect"]!["minBackoffMs"]!.GetValue().ShouldBe(0); } + // ---- write-back (Driver.MTConnect-004) ----------------------------------------------------- + + [Fact] + public void Write_back_is_off_by_default_and_is_written_explicitly() + { + // The capability shipped 2026-08-05 behind write.enabled and docs/drivers/MTConnect.md tells + // the operator to set it — but the form authored no write block at all, so the documented + // enablement path existed only as a hand edit of DriverInstance.DriverConfig in the config DB. + // + // The default is written rather than omitted: the driver reads an absent block as disabled + // either way, and an explicit "enabled": false is what makes a config an operator opened and + // saved say what it does. It is also what lets the form TURN THE FLAG OFF again — an omitted + // key would leave a previously-authored true standing. + var o = Emit(new MTConnectDriverForm.MTConnectFormModel { AgentUri = "http://agent:5000" }); + + o["write"]!["enabled"]!.GetValue().ShouldBeFalse(); + } + + [Fact] + public void Write_back_enabled_reaches_the_key_the_driver_binds() + { + var o = Emit(new MTConnectDriverForm.MTConnectFormModel + { + AgentUri = "http://agent:5000", + WriteEnabled = true, + }); + + // MTConnectDriver.ParseOptions binds write.enabled into MTConnectDriverOptions.Write.Enabled + // (pinned driver-side by MTConnectFactoryTests). This is the AdminUI half of that contract. + o["write"]!["enabled"]!.GetValue().ShouldBeTrue(); + } + + [Fact] + public void Write_back_round_trips_and_a_hand_authored_block_loads_into_the_form() + { + // The mitigating property the finding recorded — BuildConfigJson preserves unknown TOP-LEVEL + // keys, so a hand-authored write block survived an edit through the form — is not enough on + // its own: unless the form READS it, opening the modal shows the switch off over a config + // that has writing on, and the operator's next save is the one that finally clears it. + const string handAuthored = """{"agentUri":"http://agent:5000","write":{"enabled":true}}"""; + + var form = MTConnectDriverForm.MTConnectFormModel.FromJson(handAuthored); + form.WriteEnabled.ShouldBeTrue(); + + var again = MTConnectDriverForm.MTConnectFormModel.FromJson( + MTConnectDriverForm.BuildConfigJson(form, handAuthored)); + again.WriteEnabled.ShouldBeTrue(); + } + + [Fact] + public void Turning_write_back_off_clears_a_previously_authored_true() + { + // The falsification control for the "always write the block" decision above: a form that + // omitted the key when false would leave the stored true in place, so the switch would appear + // to turn writing off while the deployed driver went on accepting writes. + const string existing = """{"agentUri":"http://agent:5000","write":{"enabled":true}}"""; + + var form = MTConnectDriverForm.MTConnectFormModel.FromJson(existing); + form.WriteEnabled = false; + + Emit(form, existing)["write"]!["enabled"]!.GetValue().ShouldBeFalse(); + } + + [Fact] + public void FromJson_reads_a_pascal_case_write_block() + { + var form = MTConnectDriverForm.MTConnectFormModel.FromJson( + """{"AgentUri":"http://agent:5000","Write":{"Enabled":true}}"""); + + form.WriteEnabled.ShouldBeTrue(); + } + + [Fact] + public void The_write_enabled_document_is_the_literal_the_driver_test_parses() + { + // The AdminUI cannot be referenced from a driver test project, so the driver-side leg of this + // contract (MTConnectFactoryTests.The_document_the_AdminUI_form_emits_arms_write_back) parses a + // literal copy of this form's output. Asserting the bytes here is what stops the two drifting: + // a rename or a reordering on this side fails HERE, naming the literal that must be updated. + MTConnectDriverForm.BuildConfigJson( + new MTConnectDriverForm.MTConnectFormModel { AgentUri = "http://agent:5000", WriteEnabled = true }, + null) + .ShouldBe(WriteEnabledDocument); + } + + /// The form's output for a write-enabled instance, mirrored in the driver test suite. + private const string WriteEnabledDocument = + """{"agentUri":"http://agent:5000","requestTimeoutMs":5000,"sampleIntervalMs":1000,"sampleCount":1000,"heartbeatMs":10000,"probe":{"enabled":true,"intervalMs":5000,"timeoutMs":2000},"reconnect":{"minBackoffMs":0,"maxBackoffMs":30000,"backoffMultiplier":2},"write":{"enabled":true}}"""; + // ---- Validate ------------------------------------------------------------------------------ [Fact] @@ -292,6 +381,8 @@ public sealed class MTConnectDriverFormModelTests dto.Reconnect!.MinBackoffMs.ShouldBe(0); dto.Reconnect.MaxBackoffMs.ShouldBe(30000); dto.Reconnect.BackoffMultiplier.ShouldBe(2.0); + dto.Write.ShouldNotBeNull(); + dto.Write!.Enabled.ShouldBe(false); } private sealed class WireDto @@ -304,6 +395,12 @@ public sealed class MTConnectDriverFormModelTests public int? HeartbeatMs { get; init; } public WireProbe? Probe { get; init; } public WireReconnect? Reconnect { get; init; } + public WireWrite? Write { get; init; } + } + + private sealed class WireWrite + { + public bool? Enabled { get; init; } } private sealed class WireProbe