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 71d11215..8dcc9f68 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -98,6 +98,17 @@ public sealed class MTConnectDriver : IDriver, IReadable AllowTrailingCommas = true, }; + /// + /// Reader options for the emptiness probe in . Kept in step with + /// so a document the real parse would accept is never judged + /// malformed here (a stray trailing comma must not change which branch a config takes). + /// + private static readonly JsonDocumentOptions DocumentOptions = new() + { + CommentHandling = JsonCommentHandling.Skip, + AllowTrailingCommas = true, + }; + private readonly string _driverInstanceId; private readonly ILogger _logger; private readonly Func _agentClientFactory; @@ -107,16 +118,76 @@ public sealed class MTConnectDriver : IDriver, IReadable /// / Shutdown all swap the client and the served caches; overlapping them would let a /// shutdown dispose a client an in-flight initialize is about to publish. /// + /// + /// + /// ⚠️ NON-REENTRANT — never call a public lifecycle method from code that already + /// holds this. has no owner tracking, so a re-entrant + /// call does not fail: it deadlocks the driver permanently, with no exception and + /// no log line to diagnose it from. + /// + /// + /// This is aimed squarely at the /sample pump (Task 11). Its ring-buffer + /// re-baseline ("on a sequence gap, re-/current and resume") is naturally written + /// as "just call the existing re-init logic" — and + /// / both take this + /// semaphore, so from inside the pump loop that is a hang, not a re-prime. Re-baseline + /// must instead call directly on the + /// client the pump already holds (through , to keep the + /// deadline) and Apply the result to . Any future + /// shared step belongs in a private, non-locking helper that both the pump and the + /// lock-holding lifecycle methods can call — never in the public entry points. + /// + /// + /// The inverse direction is already safe: the lifecycle methods call + /// while holding this, so that method — and whatever + /// Task 11 fills it with — must never re-enter the lifecycle either. + /// + /// private readonly SemaphoreSlim _lifecycle = new(1, 1); private MTConnectDriverOptions _options; - private IMTConnectAgentClient? _client; private MTConnectObservationIndex _index; - private MTConnectProbeModel? _probeModel; - private int _probeModelDataItemCount; private long? _agentInstanceId; private DriverHealth _health = new(DriverState.Unknown, null, null); + /// + /// The live agent client plus the /probe cache derived from it, held as one + /// immutable snapshot behind a single reference and only ever replaced by a + /// of a whole new instance — the same discipline + /// uses, and for the same reason. + /// + /// + /// + /// Why one field instead of three. These three values are read outside + /// — by , by + /// (Task 12's browse), and by + /// — while every write happens under it. As three + /// separate fields they could be observed mid-update: a reader could see a dropped + /// ProbeModel beside a stale non-zero data-item count and report a footprint for + /// a cache that no longer exists, or pair one initialize's client with the previous + /// one's model. Packing them makes every observation internally consistent by + /// construction, with no lock on the read path. + /// + /// + /// Why not simply take the lock in those readers. Both readers await the + /// network, so they would hold the lifecycle lock across an HTTP round trip — a browse + /// could stall a for a full RequestTimeoutMs — and + /// every added lock site widens the non-reentrancy hazard documented on + /// . It would also make the read path inconsistent with + /// , which is deliberately lock-free. + /// + /// + /// What this does NOT fix. A reader can still capture a session moments before a + /// concurrent teardown disposes its client, and then call it — no lock-free scheme can + /// prevent that, and holding the lock across the call is the trade rejected above. + /// What it guarantees is that the failure is named: the resulting + /// is caught at the seam and reported as the same + /// "driver is not connected" error a caller already has to handle, rather than escaping + /// raw out of a browse. + /// + /// + private AgentSession? _session; + /// Creates a driver instance. Opens no connection and builds no agent client. /// /// The typed configuration this instance starts from. A config document supplied to @@ -173,7 +244,7 @@ public sealed class MTConnectDriver : IDriver, IReadable /// dropped by . Prefer /// , which cannot observe the flushed hole. /// - internal MTConnectProbeModel? CachedProbeModel => _probeModel; + internal MTConnectProbeModel? CachedProbeModel => Volatile.Read(ref _session)?.ProbeModel; /// /// The Agent's instanceId as of the last successful /current. Task 13 watches @@ -198,28 +269,15 @@ public sealed class MTConnectDriver : IDriver, IReadable await _lifecycle.WaitAsync(cancellationToken).ConfigureAwait(false); try { + // Parse BEFORE tearing down — see ResolveIncomingOptions. Initialize is reachable on a + // live instance (DriverInstanceActor re-initialises in Connecting/Reconnecting), and + // destroying a working client before discovering the new document is unreadable would + // be the exact outcome ReinitializeAsync is written to avoid. + var options = ResolveIncomingOptions(driverConfigJson); + // A re-Initialize on a live instance must not orphan the previous client. await TeardownCoreAsync().ConfigureAwait(false); - MTConnectDriverOptions options; - try - { - options = ResolveOptions(driverConfigJson); - } - catch (Exception ex) - { - // A config document that will not parse is just as fatal to a cold start as an - // unreachable Agent, and it must reach GetHealth() the same way — otherwise the - // dashboard keeps showing whatever state the driver was in before. - WriteHealth(new DriverHealth(DriverState.Faulted, ReadHealth().LastSuccessfulRead, ex.Message)); - _logger.LogError( - ex, - "MTConnect driver {DriverInstanceId} could not read its driver config document; the driver is Faulted.", - _driverInstanceId); - - throw; - } - await StartCoreAsync(options, existingClient: null, cancellationToken).ConfigureAwait(false); } finally @@ -257,27 +315,9 @@ public sealed class MTConnectDriver : IDriver, IReadable await _lifecycle.WaitAsync(cancellationToken).ConfigureAwait(false); try { - MTConnectDriverOptions incoming; - try - { - incoming = ResolveOptions(driverConfigJson); - } - catch (Exception ex) - { - // Deliberately NOT Faulted, and deliberately before any teardown: the running - // driver is still serving its previous, valid configuration. Rejecting the edit - // fails the deployment (DriverInstanceActor turns this into ApplyResult(false)); - // downing a working driver over an unreadable document would be a far larger blast - // radius than the change that was refused. - _logger.LogError( - ex, - "MTConnect driver {DriverInstanceId} rejected a new driver config document and kept running its previous configuration.", - _driverInstanceId); + var incoming = ResolveIncomingOptions(driverConfigJson); - throw; - } - - var existing = _client; + var existing = Volatile.Read(ref _session)?.Client; await StopSampleStreamAsync().ConfigureAwait(false); @@ -333,7 +373,7 @@ public sealed class MTConnectDriver : IDriver, IReadable /// constants. /// public long GetMemoryFootprint() => - ((long)_probeModelDataItemCount * ProbeModelBytesPerDataItem) + ((long)(Volatile.Read(ref _session)?.ProbeModelDataItemCount ?? 0) * ProbeModelBytesPerDataItem) + ((long)ObservationIndex.Count * IndexBytesPerEntry) + ((long)_options.Tags.Count * TagBytesPerEntry); @@ -344,21 +384,36 @@ public sealed class MTConnectDriver : IDriver, IReadable /// (it is the read surface, and re-deriving it would mean a full re-prime the caller did not /// ask for). Flushing cannot wedge the driver: every consumer of the model goes through /// , which re-fetches and re-caches on a miss. + /// + /// The drop is a single of a whole new + /// , so the model and its data-item count can never be seen + /// out of step (which would skew ), and a flush that + /// races a concurrent teardown or re-initialize is discarded rather than resurrecting + /// the session it was reading. + /// /// public Task FlushOptionalCachesAsync(CancellationToken cancellationToken) { - var dropped = _probeModelDataItemCount; - - _probeModel = null; - _probeModelDataItemCount = 0; - - if (dropped > 0) + var session = Volatile.Read(ref _session); + if (session?.ProbeModel is null) { - _logger.LogInformation( - "MTConnect driver {DriverInstanceId} flushed its cached /probe device model ({DataItemCount} data items); it is re-fetched on the next browse.", - _driverInstanceId, dropped); + return Task.CompletedTask; } + var flushed = session with { ProbeModel = null, ProbeModelDataItemCount = 0 }; + if (!ReferenceEquals(Interlocked.CompareExchange(ref _session, flushed, session), session)) + { + // Someone swapped the session while we were building the flushed copy — their state is + // newer than ours, and publishing would undo it. Whatever they installed is either a + // teardown (no cache to flush) or a fresh initialize (a cache the operator's budget + // breach never saw), so dropping this flush is correct, not merely safe. + return Task.CompletedTask; + } + + _logger.LogInformation( + "MTConnect driver {DriverInstanceId} flushed its cached /probe device model ({DataItemCount} data items); it is re-fetched on the next browse.", + _driverInstanceId, session.ProbeModelDataItemCount); + return Task.CompletedTask; } @@ -419,7 +474,9 @@ public sealed class MTConnectDriver : IDriver, IReadable return []; } - var client = _client; + // 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; if (client is null) @@ -472,29 +529,54 @@ public sealed class MTConnectDriver : IDriver, IReadable /// model consumer (Task 12's DiscoverAsync) must use, so that /// can never leave browse permanently disabled. /// + /// + /// Lock-free by design — see the remarks for why this does not take + /// . It reads the session once, fetches against that + /// session's client, and re-caches only if that same session is still installed, so a + /// concurrent / can neither be + /// undone by a late re-cache nor leak a model belonging to a client that is gone. A teardown + /// that lands mid-fetch surfaces as the below rather + /// than as a raw from the disposed client. + /// /// Cancellation token. /// The Agent's device model. - /// The driver is not initialized. + /// + /// The driver is not initialized, or it was torn down while this fetch was in flight. + /// internal async Task GetOrFetchProbeModelAsync(CancellationToken cancellationToken) { - var cached = _probeModel; - if (cached is not null) + var session = Volatile.Read(ref _session) ?? throw NotConnected(); + + if (session.ProbeModel is not null) { - return cached; + return session.ProbeModel; } - var client = _client - ?? throw new InvalidOperationException( - $"MTConnect driver '{_driverInstanceId}' has no agent client; the /probe device model cannot be fetched before InitializeAsync succeeds."); + MTConnectProbeModel model; + try + { + model = await BoundedAsync(session.Client.ProbeAsync, "/probe", _options.RequestTimeoutMs, cancellationToken) + .ConfigureAwait(false); + } + catch (ObjectDisposedException ex) + { + // The lock-free window the _session remarks name: a teardown disposed this client after + // we captured it. Reported as the not-connected error the caller already handles, so + // browse has exactly one failure mode to reason about. + throw NotConnected(ex); + } - var model = await BoundedAsync(client.ProbeAsync, "/probe", _options.RequestTimeoutMs, cancellationToken) - .ConfigureAwait(false); - - CacheProbeModel(model); + // Publish only onto the session we actually fetched against. + var recached = session with { ProbeModel = model, ProbeModelDataItemCount = CountDataItems(model) }; + _ = Interlocked.CompareExchange(ref _session, recached, session); return model; } + private InvalidOperationException NotConnected(Exception? inner = null) => + new($"MTConnect driver '{_driverInstanceId}' has no live agent client; the /probe device model cannot be fetched unless InitializeAsync has succeeded and the driver has not since been shut down or re-initialized.", + inner); + /// /// Builds driver options from a DriverConfig JSON document. /// @@ -603,10 +685,11 @@ public sealed class MTConnectDriver : IDriver, IReadable // ---- commit point: everything below is non-throwing bookkeeping ---- _options = options; - _client = client; built = null; - CacheProbeModel(probe); + // One publish, so a concurrent lock-free reader sees the new client and the new probe + // cache together or neither of them. + Volatile.Write(ref _session, new AgentSession(client, probe, CountDataItems(probe))); _agentInstanceId = current.InstanceId; var index = new MTConnectObservationIndex(options.Tags); @@ -649,12 +732,13 @@ public sealed class MTConnectDriver : IDriver, IReadable /// private Task TeardownCoreAsync() { - var client = _client; - _client = null; - SafeDispose(client); + // Retire the whole session in one write, BEFORE disposing: a lock-free reader that still + // holds the old reference gets a disposed client (caught + named at each seam), never a + // session that is half-torn-down. + var session = Volatile.Read(ref _session); + Volatile.Write(ref _session, null); + SafeDispose(session?.Client); - _probeModel = null; - _probeModelDataItemCount = 0; _agentInstanceId = null; // A fresh index rather than Clear(): the tag table it coerces against belongs to the options @@ -670,17 +754,95 @@ public sealed class MTConnectDriver : IDriver, IReadable /// must stop the stream before the client they are about /// to dispose goes away, and that ordering is easy to lose when the pump is bolted on later. /// + /// + /// ⚠️ This runs while is held. Whatever Task 11 fills it with + /// — cancelling the pump's token, awaiting its task — must not call back into + /// , or + /// , directly or by awaiting a pump that is itself blocked on one + /// of them: the semaphore is non-reentrant and the result is a permanent hang with no + /// exception and no log. See the remarks for the re-baseline + /// pattern that avoids this. + /// private static Task StopSampleStreamAsync() => Task.CompletedTask; - /// The document's options when it carries a body; otherwise the ones already in force. - private MTConnectDriverOptions ResolveOptions(string driverConfigJson) => - HasConfigBody(driverConfigJson) ? ParseOptions(_driverInstanceId, driverConfigJson) : _options; + /// + /// Resolves the options a lifecycle call should apply, reporting an unreadable document + /// consistently for both entry points before any state is destroyed. + /// + /// + /// + /// One rule, deliberately shared by and + /// : a config document that cannot be read never + /// destroys working state; it faults only a driver that had none. + /// + /// + /// With a live session the driver keeps serving its previous, valid configuration and + /// health is left alone — rejecting the edit already fails the deployment + /// (DriverInstanceActor turns the throw into ApplyResult(false)), and + /// downing a working driver over an unreadable document is a far larger blast radius + /// than the change that was refused. Publishing while + /// still serving values would also be a lie in the other direction. + /// + /// + /// With no live session there is nothing to protect and every tag is already dark, so + /// the failure must reach as — + /// otherwise the dashboard keeps showing whatever state the driver was in before, which + /// is the failure-that-looks-like-success shape this codebase exists to avoid. + /// + /// + private MTConnectDriverOptions ResolveIncomingOptions(string driverConfigJson) + { + try + { + return HasConfigBody(driverConfigJson) + ? ParseOptions(_driverInstanceId, driverConfigJson) + : _options; + } + catch (Exception ex) + { + if (Volatile.Read(ref _session) is not null) + { + _logger.LogError( + ex, + "MTConnect driver {DriverInstanceId} rejected a new driver config document and kept running its previous configuration.", + _driverInstanceId); + } + else + { + WriteHealth(new DriverHealth(DriverState.Faulted, ReadHealth().LastSuccessfulRead, ex.Message)); + _logger.LogError( + ex, + "MTConnect driver {DriverInstanceId} could not read its driver config document and has no running configuration to fall back on; the driver is Faulted.", + _driverInstanceId); + } + + throw; + } + } /// - /// True when carries a real config body. The - /// bootstrapper always passes a populated document; tests and probe-only callers pass - /// "{}", "[]" or blank, which keep the constructor-supplied options. + /// True when carries a real config body — i.e. a JSON + /// object or array with at least one element. The bootstrapper always passes a + /// populated document; tests and probe-only callers pass a semantically empty one, which + /// keeps the constructor-supplied options. /// + /// + /// + /// Emptiness is decided by parsing, not by string comparison. Matching the + /// literals "{}" / "[]" misses every other spelling of the same document — + /// "{ }", a pretty-printed "{\n}", "{ /* nothing yet */ }" — and + /// each of those would then reach , fail the required-AgentUri + /// check, and turn a semantically empty document into a cold-start fault or a rejected + /// deployment. That is precisely the asymmetry the keep-the-constructor-options rule + /// exists to prevent. + /// + /// + /// A malformed document is emphatically NOT empty and returns true, so + /// runs and produces the real, quoted parse error. Treating + /// unreadable text as "no config supplied" would silently swallow a corrupt document and + /// start the driver on stale options — a failure wearing success's clothes. + /// + /// private static bool HasConfigBody(string? driverConfigJson) { if (string.IsNullOrWhiteSpace(driverConfigJson)) @@ -688,9 +850,26 @@ public sealed class MTConnectDriver : IDriver, IReadable return false; } - var trimmed = driverConfigJson.Trim(); + try + { + using var document = JsonDocument.Parse(driverConfigJson, DocumentOptions); - return trimmed is not "{}" and not "[]"; + return document.RootElement.ValueKind switch + { + JsonValueKind.Object => document.RootElement.EnumerateObject().Any(), + JsonValueKind.Array => document.RootElement.EnumerateArray().Any(), + + // A bare `null` document is the JSON spelling of "nothing supplied". + JsonValueKind.Null => false, + + // A scalar root is not a config object at all; let ParseOptions say so. + _ => true, + }; + } + catch (JsonException) + { + return true; + } } /// @@ -706,13 +885,12 @@ public sealed class MTConnectDriver : IDriver, IReadable || current.SampleIntervalMs != incoming.SampleIntervalMs || current.SampleCount != incoming.SampleCount; - private void CacheProbeModel(MTConnectProbeModel model) - { - _probeModel = model; - - // Counted once here rather than re-walked on every 30 s footprint poll. - _probeModelDataItemCount = model.Devices.Sum(d => d.AllDataItems().Count()); - } + /// + /// Counts every data item the device model declares. Done once, when the model is cached, + /// rather than re-walking the tree on every 30 s poll. + /// + private static int CountDataItems(MTConnectProbeModel model) => + model.Devices.Sum(d => d.AllDataItems().Count()); /// /// Runs one unary agent call under a deadline linked to the caller's token. Belt-and-braces @@ -777,7 +955,15 @@ public sealed class MTConnectDriver : IDriver, IReadable _driverInstanceId, _options.AgentUri); } - private static void SafeDispose(IMTConnectAgentClient? client) + /// + /// Disposes an agent client without letting a disposal fault escape. Teardown runs on the + /// failure path of , where a second exception would replace the + /// real one — but the swallowed exception is still traced rather than vanishing: a + /// client that cannot be released is how a socket-handle leak starts, and an empty catch is + /// the only place in this driver where a failure would leave no evidence at all. Debug + /// level, because it is diagnostic detail about an already-reported failure. + /// + private void SafeDispose(IMTConnectAgentClient? client) { if (client is null) { @@ -788,9 +974,12 @@ public sealed class MTConnectDriver : IDriver, IReadable { client.Dispose(); } - catch (Exception) + catch (Exception ex) { - // Teardown runs on failure paths; a disposal fault must not replace the original error. + _logger.LogDebug( + ex, + "MTConnect driver {DriverInstanceId} swallowed a fault while disposing its agent client during teardown; the client may not have released its connections.", + _driverInstanceId); } } @@ -847,6 +1036,25 @@ public sealed class MTConnectDriver : IDriver, IReadable CultureInfo.InvariantCulture, $"MTConnect driver config for '{driverInstanceId}' tag '{tagName}' has an unrecognised {field} '{raw}'. Expected one of: {string.Join(", ", Enum.GetNames())}.")); + /// + /// The live agent client and the /probe cache derived from it, as one immutable + /// unit. Immutable so that publishing a change is a single reference swap — see the + /// remarks for why the three values must move together. + /// + /// The agent client this session owns and will dispose on teardown. + /// + /// The cached device model, or null once has + /// dropped it. Never null at the moment a session is first published. + /// + /// + /// Data items declared by , counted once at cache time and + /// always 0 when it is null. + /// + private sealed record AgentSession( + IMTConnectAgentClient Client, + MTConnectProbeModel? ProbeModel, + int ProbeModelDataItemCount); + // ---- config DTOs ---- /// 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 9953754b..71fe5e52 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 @@ -81,6 +81,18 @@ internal sealed class CannedAgentClient : IMTConnectAgentClient, IDisposable /// When set, /current throws this instead of answering. public Exception? CurrentFailure { get; set; } + /// + /// When set, the next /probe parks here until the test completes it, then the + /// gate clears itself so later calls answer immediately. + /// + /// + /// 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 meanwhile and still tell the two documents apart. + /// + public TaskCompletionSource? ProbeGate { get; set; } + /// /// When set, /current 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; } /// - public Task ProbeAsync(CancellationToken ct) + public async Task ProbeAsync(CancellationToken ct) { ObjectDisposedException.ThrowIf(IsDisposed, this); ct.ThrowIfCancellationRequested(); Interlocked.Increment(ref _probeCallCount); - return ProbeFailure is null ? Task.FromResult(Probe) : Task.FromException(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; } /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverLifecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverLifecycleTests.cs index d08036b3..197f429e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverLifecycleTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverLifecycleTests.cs @@ -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() + /// + /// arch-review 01/S-6: an operator-authorable 0 must never come to mean "wait + /// forever" (or "ask the Agent for nothing"). All four knobs share one + /// RequirePositive 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. + /// + /// + /// Asserted through a FAKE client factory, so this proves the DRIVER validates rather than + /// leaning on the production client's own constructor guard. + /// + [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( + var thrown = await Should.ThrowAsync( () => 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() + /// + /// "Semantically empty" is decided by parsing, not by matching the literal two-character + /// spellings. A pretty-printer, a formatter, or a hand edit turns {} into { } + /// or {\n} 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. + /// + [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( + () => 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( + () => 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( () => 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( + () => 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(() => inFlight); + thrown.InnerException.ShouldBeOfType(); + } + + [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(); + 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( + () => driver.GetOrFetchProbeModelAsync(CancellationToken.None)); + } }