fix(driver.sql): defer HandlePollError on the exception ReadAsync classified, not on its type (Driver.Sql-004)

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs

@@ -155,6 +155,20 @@ public sealed class SqlDriver
 
     private DriverHealth _health = new(DriverState.Unknown, null, null);
 
+    /// <summary>
+    ///     The exception instance <see cref="ReadAsync"/> has already turned into a health verdict, held only
+    ///     until <see cref="HandlePollError"/> — which the poll engine calls with that same object — has
+    ///     consumed it (Driver.Sql-004).
+    /// </summary>
+    /// <remarks>
+    ///     A field rather than a flag on the exception itself: <see cref="Exception.Data"/> is provider-owned
+    ///     and may be read-only, and wrapping the throw would change the type the engine and
+    ///     <see cref="IReadable"/>'s callers see. Concurrent poll loops can overwrite each other's mark; the
+    ///     loser is then merely re-degraded with a poorer message, which is the pre-fix behaviour rather than
+    ///     a new failure.
+    /// </remarks>
+    private Exception? _classifiedByRead;
+
     /// <summary>Occurs when a subscribed tag's value or quality changes.</summary>
     public event EventHandler<DataChangeEventArgs>? OnDataChange;
 
@@ -528,6 +542,10 @@ public sealed class SqlDriver
             // on the log sink via the LogWarning exception parameter above.
             Degrade($"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.GetType().Name}.");
             TransitionHostTo(HostState.Stopped);
+            // Mark THIS exception object as already classified before it goes back to the poll engine, which
+            // routes it to HandlePollError next. The mark is what makes the deferral there exact — see that
+            // method for why inferring it from the exception's type was not (Driver.Sql-004).
+            Volatile.Write(ref _classifiedByRead, ex);
             throw;
         }
     }
@@ -650,11 +668,20 @@ public sealed class SqlDriver
     /// <param name="ex">The exception the poll engine caught.</param>
     internal void HandlePollError(Exception ex)
     {
-        // A subscribed-read outage surfaces as a DbException the reader threw, which ReadAsync has ALREADY
+        // A subscribed-read outage is classified by ReadAsync — good, endpoint-bearing LastError + host
-        // classified — good, endpoint-bearing LastError + host Stopped — before rethrowing. The poll engine
+        // Stopped — before it rethrows, and the poll engine then routes that SAME exception object here.
-        // then routes that same exception here. Re-degrading would overwrite the good LastError with a worse,
+        // Re-degrading would overwrite the good LastError with a worse, guidance-free one and log the outage
-        // guidance-free one and log the outage a second time, so skip the connection class ReadAsync owns.
+        // a second time, so defer to the classification that already happened.
-        if (ex is DbException) return;
+        //
+        // Matched on the exception INSTANCE ReadAsync marked, not on its type. `ex is DbException` was the
+        // stated intent — "the class ReadAsync owns" — but not what it implemented (Driver.Sql-004): the
+        // reader raises a plain InvalidOperationException when the provider factory returns no connection,
+        // and a provider can raise ArgumentException/FormatException off a malformed connection string on
+        // the same open path. Those are classified by ReadAsync exactly like a DbException, yet a type test
+        // let them through and clobbered its message; conversely a DbException that never went through
+        // ReadAsync — the poll engine's own throw — was swallowed. Consumed once, so a later unclassified
+        // exception is never mistaken for this one.
+        if (ReferenceEquals(Interlocked.CompareExchange(ref _classifiedByRead, null, ex), ex)) return;
 
         // Whatever reaches here — an engine contract-violation, or a non-DbException the provider raised
         // (e.g. an ArgumentException from a malformed connection string, which the parser echoes with

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs

@@ -391,23 +391,91 @@ public sealed class SqlDriverTests
     {
         // On a subscribed-read outage the reader throws a DbException; ReadAsync classifies health with a
         // good, endpoint-bearing message + host Stopped and rethrows the SAME exception, which the poll
-        // engine then routes to HandlePollError. HandlePollError must ignore the DbException class ReadAsync
+        // engine then routes to HandlePollError. HandlePollError must leave the classification ReadAsync
-        // owns — pre-fix it re-degraded with the bare ex.Message (the WORSE of the two LastErrors, and a
+        // already made alone — pre-fix it re-degraded with the bare ex.Message (the WORSE of the two
-        // credential-leak path) and logged the outage a second time.
+        // LastErrors, and a credential-leak path) and logged the outage a second time.
+        // Driven through the REAL round trip rather than by handing HandlePollError a bare exception: the
+        // deferral is owned by ReadAsync marking the throw, not by the exception's type (Driver.Sql-004).
         using var fixture = new SqlitePollFixture();
         var logger = new CapturingLogger();
-        await using var driver = NewDriver(fixture, logger, KvEntry("Speed", SqlitePollFixture.PresentKey));
+        var factory = new BreakableFactory(fixture.Factory);
+        await using var driver = NewDriver(
+            fixture, fixture.ConnectionString, logger, factory, KvEntry("Speed", SqlitePollFixture.PresentKey));
         await driver.InitializeAsync(ConfigJson, CancellationToken.None);
         driver.GetHealth().State.ShouldBe(DriverState.Healthy);
 
+        factory.BreakByThrowingOnOpen("connection failed; Password=" + SecretToken);
+        var thrown = await Should.ThrowAsync<DbException>(
+            async () => await driver.ReadAsync(["Speed"], CancellationToken.None));
+
+        var classified = driver.GetHealth();
+        classified.State.ShouldBe(DriverState.Degraded);
+        classified.LastError.ShouldNotBeNull();
+        classified.LastError.ShouldContain("could not reach");
+        var warningsAfterRead = logger.Entries.Count(e => e.Level == LogLevel.Warning);
+
+        driver.HandlePollError(thrown);
+
+        var health = driver.GetHealth();
+        // Untouched: HandlePollError left the exception to ReadAsync, so it neither re-degraded nor leaked…
+        health.LastError.ShouldBe(classified.LastError);
+        health.LastError.ShouldNotContain(SecretToken);
+        // …and it logged nothing (ReadAsync owns the single outage warning).
+        logger.Entries.Count(e => e.Level == LogLevel.Warning).ShouldBe(warningsAfterRead);
+    }
+
+    [Fact]
+    public async Task HandlePollError_forANonDbExceptionReadAsyncClassified_stillDefers_soTheGoodLastErrorSurvives()
+    {
+        // Driver.Sql-004. Not every unreachable-database throw is a DbException: the reader raises a plain
+        // InvalidOperationException when the provider factory hands back no connection, and a provider can
+        // raise ArgumentException/FormatException off a malformed connection string on the same open path.
+        // ReadAsync classifies those exactly as it classifies a DbException — endpoint-bearing LastError,
+        // host Stopped — and rethrows, so HandlePollError must defer to it here too. RED against the
+        // `ex is DbException` type test, which re-degraded with the guidance-free "poll failed: TypeName"
+        // and logged the outage twice.
+        using var fixture = new SqlitePollFixture();
+        var logger = new CapturingLogger();
+        var factory = new BreakableFactory(fixture.Factory);
+        await using var driver = NewDriver(
+            fixture, fixture.ConnectionString, logger, factory, KvEntry("Speed", SqlitePollFixture.PresentKey));
+        await driver.InitializeAsync(ConfigJson, CancellationToken.None);
+
+        factory.BreakByReturningNoConnection();
+        var thrown = await Should.ThrowAsync<InvalidOperationException>(
+            async () => await driver.ReadAsync(["Speed"], CancellationToken.None));
+
+        var classified = driver.GetHealth();
+        classified.State.ShouldBe(DriverState.Degraded);
+        classified.LastError.ShouldNotBeNull();
+        classified.LastError.ShouldContain("could not reach");
+        driver.GetHostStatuses()[0].State.ShouldBe(HostState.Stopped);
+        var warningsAfterRead = logger.Entries.Count(e => e.Level == LogLevel.Warning);
+
+        driver.HandlePollError(thrown);
+
+        var health = driver.GetHealth();
+        health.LastError.ShouldBe(classified.LastError);            // the richer message survived…
+        health.LastError.ShouldNotContain("poll failed");           // …and was not overwritten by the poor one
+        logger.Entries.Count(e => e.Level == LogLevel.Warning).ShouldBe(warningsAfterRead);
+    }
+
+    [Fact]
+    public async Task HandlePollError_forAnExceptionReadAsyncNeverSaw_degrades_evenWhenItIsADbException()
+    {
+        // The falsifiability control for the two deferrals above: the marker is per-exception, not per-type,
+        // so a DbException that never went through ReadAsync's classification is NOT owned by it and must
+        // still reach the health surface. A type test would silently swallow this one.
+        using var fixture = new SqlitePollFixture();
+        var driver = NewDriver(fixture, KvEntry("Speed", SqlitePollFixture.PresentKey));
+
         driver.HandlePollError(new FakeDbException("connection failed; Password=" + SecretToken));
 
         var health = driver.GetHealth();
-        // Untouched: HandlePollError left the DbException to ReadAsync, so it neither degraded nor leaked…
+        health.State.ShouldBe(DriverState.Degraded);
-        health.State.ShouldBe(DriverState.Healthy);
+        health.LastError.ShouldNotBeNull();
-        health.LastError.ShouldBeNull();
+        health.LastError.ShouldNotContain(SecretToken);             // type only, never the message text
-        // …and it logged nothing (ReadAsync owns the single outage warning).
+        health.LastError.ShouldContain(nameof(FakeDbException));
-        logger.Entries.Count(e => e.Level == LogLevel.Warning).ShouldBe(0);
     }
 
     [Fact]
@@ -518,6 +586,26 @@ public sealed class SqlDriverTests
             operationTimeout: TimeSpan.FromSeconds(15), commandTimeout: TimeSpan.FromSeconds(10),
             rawTags: rawTags);
 
+    /// <summary>The same driver, over a caller-supplied provider factory — the outage-injection seam.</summary>
+    private static SqlDriver NewDriver(
+        SqlitePollFixture fixture,
+        string connectionString,
+        CapturingLogger logger,
+        DbProviderFactory factory,
+        params RawTagEntry[] rawTags)
+        => new(
+            new SqlDriverOptions
+            {
+                RawTags = rawTags,
+                OperationTimeout = TimeSpan.FromSeconds(15),
+                CommandTimeout = TimeSpan.FromSeconds(10),
+            },
+            DriverInstanceId,
+            new SqliteDialect(),
+            connectionString,
+            factory: factory,
+            logger: logger);
+
     /// <summary>
     ///     Builds the driver through its production constructor. That constructor <b>is</b> the injection
     ///     seam — dialect, provider factory and already-resolved connection string are all parameters — so no
@@ -668,6 +756,28 @@ public sealed class SqlDriverTests
         public override DbConnection CreateConnection() => new ThrowingConnection(openFailureMessage);
     }
 
+    /// <summary>
+    ///     A provider factory that behaves exactly like the fixture's until it is broken, then reproduces one
+    ///     of the two open-path outages the driver has to classify: a factory that returns <b>no</b>
+    ///     connection (a plain <see cref="InvalidOperationException"/> from the reader — NOT a
+    ///     <see cref="DbException"/>), or one whose connections throw on open.
+    /// </summary>
+    private sealed class BreakableFactory(DbProviderFactory inner) : DbProviderFactory
+    {
+        private Func<DbConnection?>? _broken;
+
+        /// <summary>Hands back no connection, the shape that makes the reader raise InvalidOperationException.</summary>
+        public void BreakByReturningNoConnection() => Volatile.Write(ref _broken, static () => null);
+
+        /// <summary>Hands back a connection that fails to open with <paramref name="message"/>.</summary>
+        /// <param name="message">The provider message, which may carry credentials.</param>
+        public void BreakByThrowingOnOpen(string message)
+            => Volatile.Write(ref _broken, () => new ThrowingConnection(message));
+
+        public override DbConnection? CreateConnection()
+            => Volatile.Read(ref _broken) is { } broken ? broken() : inner.CreateConnection();
+    }
+
     /// <summary>A connection that throws <see cref="FakeDbException"/> on open; every other member is inert.</summary>
     private sealed class ThrowingConnection(string openFailureMessage) : DbConnection
     {