diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs index 7feb6a0f..ceabf3a7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs @@ -197,7 +197,10 @@ internal sealed class SqlBrowseSession : IBrowseSession Name: column.Name, DriverDataType: _dialect.MapColumnType(column.DataType).ToString(), IsArray: false, - // The picker's descriptor line ("{type} · {SecurityClass}") — display only. Nullability is + // ⚠ SecurityClass is (ab)used as the picker's descriptor line ("{type} · {SecurityClass}"), + // exactly as the MQTT browser does. Every consumer today is display-only; a future one that + // parses it as SecurityClassification must not read the Sql value (review m-6 — a dedicated + // descriptor field on AttributeInfo is the Commons follow-up). Display only. Nullability is // what tells an operator a cell can come back NULL, which publishes Uncertain (or Bad under // nullIsBad), before they commit the tag. SecurityClass: column.IsNullable switch diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs index d7212245..ac10102c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -377,13 +377,30 @@ public sealed class SqlDriver // connection string as one unit; adopting options alone would poll a NEW tag set through the OLD // database, which is why this driver was excluded from the first pass. A null rebinder (direct // construction) or an empty/placeholder document keeps the constructor-supplied trio. - if (_rebind is not null && HasConfigBody(driverConfigJson)) + try { - var binding = _rebind(driverConfigJson); - ApplyBinding(binding.Options, binding.Dialect, binding.ConnectionString); - } + if (_rebind is not null && HasConfigBody(driverConfigJson)) + { + var binding = _rebind(driverConfigJson); + ApplyBinding(binding.Options, binding.Dialect, binding.ConnectionString); + } - BuildTagTable(); + BuildTagTable(); + } + catch (Exception ex) + { + // Review m-4: an unresolvable connectionStringRef or a malformed config threw here AFTER Initializing + // was written, and no poll may end Initializing — so the driver latched it with no LastError. Fault + // it like the liveness stage does: exception TYPE only on the operator surface (the factory's own + // messages are already sanitized, but this method cannot rely on that), full exception to the log. + var message = + $"Sql driver '{_driverInstanceId}' could not adopt its configuration: {ex.GetType().Name}. " + + "Check the connectionStringRef and the driver config JSON."; + _logger.LogError(ex, "Sql driver {DriverInstanceId} configuration could not be adopted.", _driverInstanceId); + if (WriteHealthIfCurrent(generation, new DriverHealth(DriverState.Faulted, ReadHealth().LastSuccessfulRead, message))) + TransitionHostTo(HostState.Stopped); + throw; + } try { @@ -539,12 +556,35 @@ public sealed class SqlDriver // The generation this read started under: if a Shutdown/Initialize lands while it is in flight, its // verdict describes a superseded configuration and must not touch the newer health (R06:S-9). var generation = Interlocked.Read(ref _lifecycleGeneration); + return await ReadThroughAsync(_reader, fullReferences, generation, cancellationToken).ConfigureAwait(false); + } + + /// + /// 's body against a specific reader — the one the caller captured. Internal so a + /// test can drive the "reader replaced between capture and use" race deterministically. + /// + internal async Task> ReadThroughAsync( + SqlPollReader reader, IReadOnlyList fullReferences, long generation, CancellationToken cancellationToken) + { try { - var result = await _reader.ReadDetailedAsync(fullReferences, cancellationToken).ConfigureAwait(false); + var result = await reader.ReadDetailedAsync(fullReferences, cancellationToken).ConfigureAwait(false); ObservePollOutcome(result.Snapshots, result.AnsweredRefusalGroups, generation); return result.Snapshots; } + catch (SqlPollReaderDisposedException ex) when (!cancellationToken.IsCancellationRequested) + { + // A rebind swapped the reader under this poll (review m-1: a redeploy re-initialises without + // teardown, so the engine keeps polling). Retry ONCE on the current reader; if there is no newer + // one (the driver itself was disposed), hand the engine an exception it is told not to classify — + // it used to Degrade the driver as "poll failed: OperationCanceledException". + var current = Volatile.Read(ref _reader); + if (!ReferenceEquals(current, reader)) + return await ReadThroughAsync(current, fullReferences, generation, cancellationToken).ConfigureAwait(false); + + Volatile.Write(ref _classifiedByRead, ex); + throw; + } catch (OperationCanceledException) { // Teardown, not a database verdict — leave health and host state exactly as they were. @@ -1226,6 +1266,9 @@ public sealed class SqlDriver _reader.Dispose(); } + /// Re-runs on the current binding — a real reader swap, for the m-1 race test. + internal void RebindForTests() => ApplyBinding(_options, _dialect, _connectionString); + /// The live poll reader. Test observation of the S-3 lifecycle only. internal SqlPollReader ReaderForTests => _reader; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs index 3bb727e5..d4f46fa8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs @@ -199,8 +199,8 @@ public static class SqlDriverFactoryExtensions // connection-string literal is exactly the malformed text that lands here. This message becomes // InitializeFailed -> health LastError -> /hosts. Line/byte position carry no content, so they stay. throw new InvalidOperationException( - $"Sql driver config for '{driverInstanceId}' is not valid JSON (line {ex.LineNumber + 1}, byte " - + $"{ex.BytePositionInLine}). The parser's message is withheld because the malformed text may " + $"Sql driver config for '{driverInstanceId}' is not valid JSON (line " + + $"{(ex.LineNumber is { } line ? line + 1 : (object)"?")}, byte {ex.BytePositionInLine?.ToString(System.Globalization.CultureInfo.InvariantCulture) ?? "?"}). The parser's message is withheld because the malformed text may " + "carry a connection string."); } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs index ffeb3804..0d3a96c5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs @@ -333,6 +333,10 @@ public sealed class SqlPollReader : IDisposable // leaves a sibling group running against a connection nobody is watching. var outcomes = await Task.WhenAll(groups).ConfigureAwait(false); + // Surfaced AFTER WhenAll, as one typed exception: a group throwing it would turn WhenAll into a plain + // TaskCanceledException and the caller could no longer tell "superseded reader" from a caller cancel. + if (outcomes.Any(o => ReferenceEquals(o, GroupResult.ReaderDisposed))) throw new SqlPollReaderDisposedException(); + var answeredRefusals = 0; for (var g = 0; g < plans.Count; g++) { @@ -382,7 +386,7 @@ public sealed class SqlPollReader : IDisposable { // A reader replaced by a reinitialize (and disposed) can still be holding a stale poll: it runs no // new group. Cancellation is the honest answer — the poll that asked has been superseded. - if (!TryAddRef()) throw new OperationCanceledException("The Sql poll reader was disposed."); + if (!TryAddRef()) return GroupResult.ReaderDisposed; try { @@ -477,6 +481,9 @@ public sealed class SqlPollReader : IDisposable { while (true) { + // A disposed reader starts no new group even while older ones are still in flight (review m-2): + // the gate would survive until they finish, but a replaced reader must not take new work. + if (Volatile.Read(ref _disposed) != 0) return false; var current = Volatile.Read(ref _refs); if (current == 0) return false; if (Interlocked.CompareExchange(ref _refs, current + 1, current) == current) return true; @@ -968,6 +975,10 @@ public sealed class SqlPollReader : IDisposable public static GroupResult Failed { get; } = new(SqlStatusCodes.BadCommunicationError, NoRows, NoColumns, []); + /// The reader was disposed before this group could start; surfaced as a typed exception. + public static GroupResult ReaderDisposed { get; } = + new(SqlStatusCodes.BadInternalError, NoRows, NoColumns, []); + /// /// The database answered with more rows than the plan can use — an ambiguous selector (R06:S-1). /// A configuration fact, not a database fault; the driver counts it as answer evidence. @@ -1003,6 +1014,13 @@ public sealed class SqlPollReader : IDisposable } } +/// +/// A poll reached a after it was disposed — a rebind replaced it, or the driver +/// was disposed. Not a database verdict: the driver retries on the current reader, and never degrades for it. +/// +public sealed class SqlPollReaderDisposedException() + : OperationCanceledException("The Sql poll reader was disposed (replaced by a rebind, or the driver was disposed)."); + /// One poll's snapshots plus the answer evidence they cannot carry themselves. /// One snapshot per requested reference, at the same index. /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs index 1eec94e5..ed93c7a5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs @@ -484,6 +484,64 @@ public sealed class SqlDriverTests skipped.Reason.ShouldContain("Query"); } + /// + /// Review m-1. A redeploy re-initialises without teardown, so a poll can capture the reader just + /// before a rebind disposes it. That poll used to throw an OperationCanceledException the engine then + /// classified as a poll failure (Degrade). It now retries once on the current reader — and reads Good, + /// the positive control — with health untouched. + /// + [Fact] + public async Task APollOnAReplacedReader_retriesOnTheCurrentOne() + { + using var fixture = new SqlitePollFixture(); + await using var driver = NewDriver(fixture, KvEntry("Speed", SqlitePollFixture.PresentKey)); + await driver.InitializeAsync(ConfigJson, CancellationToken.None); + var captured = driver.ReaderForTests; + + driver.RebindForTests(); + captured.GateDisposedForTests.ShouldBeTrue(); + + var snapshots = await driver.ReadThroughAsync( + captured, ["Speed"], driver.LifecycleGenerationForTests, CancellationToken.None); + + snapshots[0].Value.ShouldBe(SqlitePollFixture.PresentValue); + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// With no newer reader (the driver's own reader was disposed) the disposed-reader exception still + /// propagates, but the engine's error callback must not degrade for it. Control: any other poll error + /// still degrades. + /// + [Fact] + public async Task ADisposedReaderError_isNotAPollFailure() + { + using var fixture = new SqlitePollFixture(); + await using var driver = NewDriver(fixture, KvEntry("Speed", SqlitePollFixture.PresentKey)); + await driver.InitializeAsync(ConfigJson, CancellationToken.None); + driver.ReaderForTests.Dispose(); + + // Awaited directly (as the poll engine does): an OperationCanceledException-derived throw ends the task + // Canceled, and only a direct await rethrows the original instance — which HandlePollError matches on. + Exception? caught = null; + try + { + await driver.ReadThroughAsync( + driver.ReaderForTests, ["Speed"], driver.LifecycleGenerationForTests, CancellationToken.None); + } + catch (Exception e) + { + caught = e; + } + + caught.ShouldBeOfType(); + driver.HandlePollError(caught!); + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + + driver.HandlePollError(new InvalidOperationException("contract violation")); + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + } + // ---- Driver.Sql-010: the health field is read-modify-written from several poll loops ---- [Fact] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs index f9223f94..12e2fc22 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs @@ -81,6 +81,11 @@ public sealed class SqlReinitConfigAdoptionTests "old-server", customMessage: "a reinitialize that could not resolve its new connection must leave the previous binding whole"); + + // Review m-4: the failure is a fault with a reason, never a latched Initializing with no LastError. + var health = driver.GetHealth(); + health.State.ShouldBe(Core.Abstractions.DriverState.Faulted); + health.LastError.ShouldNotBeNull().ShouldContain("could not adopt its configuration"); } /// An empty/placeholder document keeps the constructor-supplied binding, so the many lifecycle