fix(mtconnect): volatile _options publication, IConvertible write guard, idempotent client dispose

Driver.MTConnect-005/-006/-007, each TDD'd red-first.

- -005: _options was the one field at the StartCoreAsync commit point published by a
  plain write and read lock-free from ReadAsync/WriteAsync/DiscoverAsync/HostName/health.
  Now Volatile.Write/Volatile.Read like _session/_binding/_announcedInstanceId, pinned by
  a source scan (MTConnectOptionsPublicationTests) that fails on any unfenced touch.
- -006: TryFormatValue guarded null and Array, but Convert.ToString renders ANY
  non-IConvertible value as its type name under a Good status. Guard is now the property
  the remark relies on; the accepted set (string/bool/numeric/DateTime) is unchanged.
- -007: MTConnectAgentClient.Dispose was a check-then-set on a non-volatile bool, so two
  racing disposals both passed and the loser cancelled an already-disposed CTS. Now an
  Interlocked.Exchange claim over an int, with the request seams reading it volatile.
This commit is contained in:
Joseph Doherty
2026-08-10 08:02:15 -04:00
parent 94b8633e68
commit 2d7dffcc42
6 changed files with 290 additions and 41 deletions
@@ -370,6 +370,13 @@ internal sealed class CannedAgentClient : IMTConnectAgentClient, IDisposable
/// <summary>When set, a write throws this instead of answering — the transport-failure leg.</summary>
public Exception? WriteFailure { get; set; }
/// <summary>
/// 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.
/// </summary>
public Action? AfterWrite { get; set; }
/// <inheritdoc/>
public Task<MTConnectWriteOutcome> 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;
}
@@ -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<ObjectDisposedException>(
() => client.WriteObservationAsync("StubDevice", "dev1_msg", "hi", CancellationToken.None));
}
/// <summary>
/// Driver.MTConnect-007 — concurrent disposals must not throw out of <c>Dispose</c>.
/// </summary>
/// <remarks>
/// <para>
/// A plain check-then-set on a non-volatile <c>bool</c> lets two racing disposals both pass
/// the guard; the loser then calls <c>Cancel()</c> on a <see cref="CancellationTokenSource"/>
/// the winner has already disposed, which throws <see cref="ObjectDisposedException"/> out of
/// <c>Dispose</c>. The driver disposes this client under its lifecycle lock and through
/// <c>SafeDispose</c>, 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.
/// </para>
/// <para>
/// <b>The window is a handful of instructions, so one pass proves nothing</b> — 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 <see cref="Interlocked.Exchange(ref int, int)"/> no schedule can fail it, so
/// it cannot flake either way.
/// </para>
/// </remarks>
[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<Exception>();
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)));
}
}
}
@@ -0,0 +1,109 @@
using System.Text.RegularExpressions;
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests;
/// <summary>
/// Driver.MTConnect-005 — <c>MTConnectDriver._options</c> must be published and read the way its
/// three siblings at the same commit point are (<c>_session</c>, <c>_binding</c>,
/// <c>_announcedInstanceId</c>): <c>Volatile.Write</c> once, <c>Volatile.Read</c> everywhere else.
/// </summary>
/// <remarks>
/// <para>
/// <b>Why a source scan and not a behavioural test.</b> The defect is a missing memory fence:
/// a plain field write publishes the new options document with no release, and a reader that
/// reads <c>_options</c> BEFORE it reads <c>_binding</c> — which both <c>ReadAsync</c> and
/// <c>WriteAsync</c> do — has no acquire ordering between the two, so it can in principle
/// judge a batch against the previous <c>Write.Enabled</c> / <c>RequestTimeoutMs</c> 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
/// <c>LibplctagGatewayConsumptionGuardTests</c>: the defect recurs by being reintroduced at a
/// call site no per-site test covers.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
[Trait("Category", "Unit")]
public sealed class MTConnectOptionsPublicationTests
{
/// <summary>Any mention of the field, so an unguarded read is caught as well as an unguarded write.</summary>
private static readonly Regex Mention = new(@"_options\b", RegexOptions.Compiled);
/// <summary>The two guarded forms. Nothing else may touch the field.</summary>
private static readonly Regex Guarded =
new(@"Volatile\.(Read|Write)\(ref _options[,)]", RegexOptions.Compiled);
/// <summary>The declaration itself, which is neither a read nor a write.</summary>
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<string>();
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);
}
/// <summary>Removes a trailing line comment and skips XML-doc lines, which mention the field by name.</summary>
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;
}
}
@@ -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<T> 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<double> { 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");
}
/// <summary>Cancels the batch from inside its own rendering, then renders normally.</summary>
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()
{