dohertj2/lmxopcua
1
0
Fork 0
Code Issues 17 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(mtconnect): review remediation for the driver lifecycle (Task 9)

Browse Source 
Important 1 - the probe-model triple was read outside _lifecycle while every
write happened under it. Packed (Client, ProbeModel, ProbeModelDataItemCount)
into one immutable AgentSession swapped by a single Volatile.Write, matching
what _index already does, rather than locking the readers: both await the
network, so taking _lifecycle would stall a shutdown for a whole
RequestTimeoutMs and would widen the non-reentrancy hazard. GetOrFetch and
Flush re-publish via CompareExchange so a late re-cache cannot undo a
concurrent teardown/re-init, and a client disposed mid-fetch now surfaces as
the same "not connected" InvalidOperationException browse already handles
instead of a raw ObjectDisposedException.

Important 2 - HasConfigBody decided emptiness by matching the literals "{}" /
"[]", so "{ }", "{\n}" and every pretty-printed empty document fell through to
ParseOptions, failed the required-AgentUri check, and turned a semantically
empty config into a cold-start fault. Now parsed: an object/array with no
elements (or a bare null) is empty; malformed text is deliberately NOT empty so
ParseOptions produces the real quoted error rather than silently starting on
stale options.

Important 3 - documented the _lifecycle non-reentrancy hazard in the code, on
both the semaphore and StopSampleStreamAsync, naming Task 11's re-baseline as
the specific path that would deadlock and giving the CurrentAsync-directly
pattern that avoids it.

Important 4 - removed the Initialize/Reinitialize asymmetry rather than
documenting it. Both now share one rule via ResolveIncomingOptions: an
unreadable config document never destroys working state, and faults only a
driver that had none. InitializeAsync previously tore down before parsing, so a
bad document on a live instance destroyed a healthy client - the exact outcome
ReinitializeAsync was written to avoid.

Minors: SafeDispose traces the swallowed disposal fault at Debug (a client that
cannot be released is how a handle leak starts); the RequirePositive test is a
Theory over all four timing knobs, not just RequestTimeoutMs (arch-review
01/S-6); the footprint test no longer overclaims "is zero before initialize".

CannedAgentClient gains a one-shot ProbeGate so a lifecycle change landing
mid-request is deterministic - no timers. 346/346 (329 + 17). Six mutations
verified: fetch-CAS, disposed-translation, flush retired-session guard,
literal HasConfigBody, teardown-before-parse, and two dropped RequirePositive
calls each fail only their own tests.
This commit is contained in:
Joseph Doherty
2026-07-24 15:52:29 -04:00
parent 1bfc1722b3
commit a127954cab
3 changed files with 527 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/CannedAgentClient.cs
+31 -2
View File
@@ -81,6 +81,18 @@ internal sealed class CannedAgentClient : IMTConnectAgentClient, IDisposable
/// <summary>When set, <c>/current</c> throws this instead of answering.</summary>
public Exception? CurrentFailure { get; set; }
/// <summary>
/// When set, the <b>next</b> <c>/probe</c> parks here until the test completes it, then the
/// gate clears itself so later calls answer immediately.
/// </summary>
/// <remarks>
/// This is how a test holds a request "in flight" across a concurrent lifecycle change —
/// a shutdown or a re-initialize landing mid-fetch — with no timers and no races of its own.
/// The answer is captured when the request lands, not when it completes, so a test can
/// change <see cref="Probe"/> meanwhile and still tell the two documents apart.
/// </remarks>
public TaskCompletionSource? ProbeGate { get; set; }
/// <summary>
/// When set, <c>/current</c> does not answer until this source completes — an Agent that
/// accepted the request and then went quiet. The wait is cancellation-observing, so the
@@ -108,13 +120,30 @@ internal sealed class CannedAgentClient : IMTConnectAgentClient, IDisposable
public long? LastSampleFrom { get; private set; }
/// <inheritdoc/>
public Task<MTConnectProbeModel> ProbeAsync(CancellationToken ct)
public async Task<MTConnectProbeModel> ProbeAsync(CancellationToken ct)
{
ObjectDisposedException.ThrowIf(IsDisposed, this);
ct.ThrowIfCancellationRequested();
Interlocked.Increment(ref _probeCallCount);
return ProbeFailure is null ? Task.FromResult(Probe) : Task.FromException<MTConnectProbeModel>(ProbeFailure);
if (ProbeFailure is not null)
{
throw ProbeFailure;
}
// Captured now: the Agent answers with the document it held when the request landed.
var answer = Probe;
if (ProbeGate is { } gate)
{
ProbeGate = null;
await gate.Task.WaitAsync(ct).ConfigureAwait(false);
// A real client whose handler was disposed mid-request fails the request; so does this.
ObjectDisposedException.ThrowIf(IsDisposed, this);
}
return answer;
}
/// <inheritdoc/>
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverLifecycleTests.cs
+198 -12
View File
@@ -26,12 +26,18 @@ public sealed class MTConnectDriverLifecycleTests
private static MTConnectDriverOptions Opts(
string agentUri = AgentUri,
string? deviceName = null,
int requestTimeoutMs = 5000) =>
int requestTimeoutMs = 5000,
int heartbeatMs = 10000,
int sampleIntervalMs = 1000,
int sampleCount = 1000) =>
new()
{
AgentUri = agentUri,
DeviceName = deviceName,
RequestTimeoutMs = requestTimeoutMs,
HeartbeatMs = heartbeatMs,
SampleIntervalMs = sampleIntervalMs,
SampleCount = sampleCount,
Tags =
[
new MTConnectTagDefinition("dev1_pos", DriverDataType.Float64),
@@ -176,23 +182,45 @@ public sealed class MTConnectDriverLifecycleTests
client.IsDisposed.ShouldBeTrue();
}
[Fact]
public async Task Initialize_rejects_a_non_positive_request_timeout_before_dialling()
/// <summary>
/// arch-review 01/S-6: an operator-authorable <c>0</c> must never come to mean "wait
/// forever" (or "ask the Agent for nothing"). All four knobs share one
/// <c>RequirePositive</c> path, so all four are asserted — a guard that covered only the
/// one knob with a test is exactly how three of them would quietly lose the check.
/// </summary>
/// <remarks>
/// Asserted through a FAKE client factory, so this proves the DRIVER validates rather than
/// leaning on the production client's own constructor guard.
/// </remarks>
[Theory]
[InlineData("RequestTimeoutMs")]
[InlineData("HeartbeatMs")]
[InlineData("SampleIntervalMs")]
[InlineData("SampleCount")]
public async Task Initialize_rejects_a_non_positive_timing_knob_before_dialling(string knob)
{
// arch-review 01/S-6: an operator-authorable 0 must never come to mean "wait forever".
// Asserted through a FAKE client factory, so this proves the DRIVER validates rather than
// relying on the production client's own constructor guard.
var options = knob switch
{
"RequestTimeoutMs" => Opts(requestTimeoutMs: 0),
"HeartbeatMs" => Opts(heartbeatMs: 0),
"SampleIntervalMs" => Opts(sampleIntervalMs: 0),
"SampleCount" => Opts(sampleCount: 0),
_ => throw new ArgumentOutOfRangeException(nameof(knob), knob, "unhandled knob"),
};
var factoryCalls = 0;
var driver = new MTConnectDriver(Opts(requestTimeoutMs: 0), "mt1", _ =>
var driver = new MTConnectDriver(options, "mt1", _ =>
{
factoryCalls++;
return CannedAgentClient.FromFixtures();
});
await Should.ThrowAsync<ArgumentOutOfRangeException>(
var thrown = await Should.ThrowAsync<ArgumentOutOfRangeException>(
() => driver.InitializeAsync("{}", CancellationToken.None));
// The message must name the offending key, or an operator cannot act on it.
thrown.Message.ShouldContain(knob);
factoryCalls.ShouldBe(0);
driver.GetHealth().State.ShouldBe(DriverState.Faulted);
}
@@ -222,8 +250,24 @@ public sealed class MTConnectDriverLifecycleTests
seen.DeviceName.ShouldBe("VMC-3Axis");
}
[Fact]
public async Task Initialize_with_an_empty_config_body_keeps_the_constructor_options()
/// <summary>
/// "Semantically empty" is decided by parsing, not by matching the literal two-character
/// spellings. A pretty-printer, a formatter, or a hand edit turns <c>{}</c> into <c>{ }</c>
/// or <c>{\n}</c> without changing its meaning — and under literal matching each of those
/// reached ParseOptions, failed the required-AgentUri check, and turned an empty document
/// into a cold-start fault or a rejected deployment.
/// </summary>
[Theory]
[InlineData("{}")]
[InlineData("{ }")]
[InlineData("{\n}")]
[InlineData(" {\r\n\t} ")]
[InlineData("[]")]
[InlineData("[ ]")]
[InlineData("null")]
[InlineData("")]
[InlineData(" ")]
public async Task Initialize_with_a_semantically_empty_config_body_keeps_the_constructor_options(string json)
{
MTConnectDriverOptions? seen = null;
var driver = new MTConnectDriver(Opts(), "mt1", o =>
@@ -233,11 +277,26 @@ public sealed class MTConnectDriverLifecycleTests
return CannedAgentClient.FromFixtures();
});
await driver.InitializeAsync("{}", CancellationToken.None);
await driver.InitializeAsync(json, CancellationToken.None);
seen.ShouldNotBeNull();
seen!.AgentUri.ShouldBe(AgentUri);
seen.Tags.Count.ShouldBe(2);
driver.GetHealth().State.ShouldBe(DriverState.Healthy);
}
[Fact]
public async Task Initialize_treats_a_malformed_config_body_as_a_parse_error_not_as_empty()
{
// The other half of the emptiness rule: unreadable text must NOT fall through to "no config
// supplied" and silently start the driver on stale options.
var (driver, _) = NewDriver();
var thrown = await Should.ThrowAsync<InvalidOperationException>(
() => driver.InitializeAsync("""{"agentUri": """, CancellationToken.None));
thrown.Message.ShouldContain("not valid JSON");
driver.GetHealth().State.ShouldBe(DriverState.Faulted);
}
[Fact]
@@ -251,6 +310,25 @@ public sealed class MTConnectDriverLifecycleTests
driver.GetHealth().State.ShouldBe(DriverState.Faulted);
}
[Fact]
public async Task Initialize_on_a_live_instance_with_an_unreadable_config_keeps_it_serving()
{
// One rule for both entry points: an unreadable document never destroys working state; it
// faults only a driver that had none. InitializeAsync IS reachable on a live instance
// (DriverInstanceActor re-initialises during Connecting/Reconnecting), so parsing must
// happen BEFORE the teardown — otherwise a bad edit kills a healthy client and the driver
// ends up in exactly the state ReinitializeAsync is written to avoid.
var (driver, client) = await InitializedDriverAsync();
await Should.ThrowAsync<InvalidOperationException>(
() => driver.InitializeAsync("""{"deviceName":"no-agent-uri"}""", CancellationToken.None));
driver.GetHealth().State.ShouldBe(DriverState.Healthy);
driver.EffectiveOptions.AgentUri.ShouldBe(AgentUri);
client.IsDisposed.ShouldBeFalse();
driver.ObservationIndex.Count.ShouldBe(FixtureDataItemCount);
}
[Fact]
public async Task Initialize_parses_authored_tags_out_of_the_config_json()
{
@@ -480,7 +558,7 @@ public sealed class MTConnectDriverLifecycleTests
// ---- footprint + flush ----
[Fact]
public async Task Memory_footprint_is_zero_before_initialize_and_positive_after()
public async Task Memory_footprint_grows_when_initialize_populates_the_caches()
{
var (driver, _) = NewDriver();
@@ -545,4 +623,112 @@ public sealed class MTConnectDriverLifecycleTests
await Should.ThrowAsync<InvalidOperationException>(
() => driver.GetOrFetchProbeModelAsync(CancellationToken.None));
}
[Fact]
public async Task Fetching_the_probe_model_after_shutdown_is_rejected()
{
var (driver, _) = await InitializedDriverAsync();
await driver.ShutdownAsync(CancellationToken.None);
await Should.ThrowAsync<InvalidOperationException>(
() => driver.GetOrFetchProbeModelAsync(CancellationToken.None));
}
// ---- lock-free session snapshot: the probe fetch races the lifecycle ----
[Fact]
public async Task A_shutdown_landing_mid_fetch_surfaces_as_not_connected_not_as_object_disposed()
{
// Browse is deliberately lock-free (it awaits the network; holding the lifecycle lock would
// let it stall a shutdown for a whole RequestTimeoutMs), so a fetch CAN outlive the client
// it captured. What must not happen is a raw ObjectDisposedException escaping into browse —
// the caller already handles "not connected" and should have exactly one failure mode.
var (driver, client) = await InitializedDriverAsync();
await driver.FlushOptionalCachesAsync(CancellationToken.None);
// Held locally: the gate is one-shot, so the client has already cleared its own reference by
// the time the request is parked on it.
var gate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
client.ProbeGate = gate;
var inFlight = driver.GetOrFetchProbeModelAsync(CancellationToken.None);
await driver.ShutdownAsync(CancellationToken.None);
client.IsDisposed.ShouldBeTrue();
gate.TrySetResult();
var thrown = await Should.ThrowAsync<InvalidOperationException>(() => inFlight);
thrown.InnerException.ShouldBeOfType<ObjectDisposedException>();
}
[Fact]
public async Task A_fetch_that_completes_after_a_reinitialize_does_not_overwrite_the_newer_cache()
{
// The probe cache is re-published only onto the session it was fetched against. Without
// that check, a slow browse would install a device model belonging to a superseded
// configuration over the one the re-initialize just established — stale browse output that
// nothing would ever correct.
var built = new List<CannedAgentClient>();
var driver = new MTConnectDriver(Opts(), "mt1", _ =>
{
var c = CannedAgentClient.FromFixtures();
built.Add(c);
return c;
});
await driver.InitializeAsync("{}", CancellationToken.None);
await driver.FlushOptionalCachesAsync(CancellationToken.None);
var client = built[0];
var staleModel = client.Probe;
// Park a fetch on the OLD (about to be superseded) session; it captures staleModel now.
// Held locally — the gate is one-shot and clears the client's own reference when it parks.
var gate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
client.ProbeGate = gate;
var inFlight = driver.GetOrFetchProbeModelAsync(CancellationToken.None);
// A config-only re-initialize: same client (so nothing is disposed), brand-new session.
var freshModel = new MTConnectProbeModel(staleModel.Devices);
client.Probe = freshModel;
await driver.ReinitializeAsync(
$$"""{"agentUri":"{{AgentUri}}","tags":[{"fullName":"dev1_pos","driverDataType":"Float64"}]}""",
CancellationToken.None);
built.Count.ShouldBe(1);
driver.CachedProbeModel.ShouldBeSameAs(freshModel);
// Now let the stale fetch finish. It still returns what the Agent gave it...
gate.TrySetResult();
(await inFlight).ShouldBeSameAs(staleModel);
// ...but it must not have published that over the newer session's cache.
driver.CachedProbeModel.ShouldBeSameAs(freshModel);
}
[Fact]
public async Task Flush_after_shutdown_is_a_no_op_and_does_not_resurrect_the_session()
{
// Core polls the footprint and asks for a flush on its own schedule, so a flush arriving
// after the driver was torn down is ordinary, not exotic. It must observe the retired
// session and do nothing — publishing a "flushed" copy of it would reinstate a session
// holding an already-disposed client for every later caller.
//
// NOTE: this pins the retired-session guard, NOT the CompareExchange beneath it. Flush has
// no await point, so nothing can be interleaved between its read and its publish from a
// single-threaded test; the CAS is defence-in-depth against a genuinely concurrent
// lifecycle call and is deliberately claimed as such rather than as tested behaviour. The
// fetch-side CAS, which does have an await point, is covered by the test above.
var (driver, client) = await InitializedDriverAsync();
await driver.ShutdownAsync(CancellationToken.None);
await driver.FlushOptionalCachesAsync(CancellationToken.None);
driver.CachedProbeModel.ShouldBeNull();
driver.GetHealth().State.ShouldBe(DriverState.Unknown);
client.DisposeCount.ShouldBe(1);
await Should.ThrowAsync<InvalidOperationException>(
() => driver.GetOrFetchProbeModelAsync(CancellationToken.None));
}
}