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

fix(sql): stop leaking provider exception text; de-dup health classification; guard Faulted

Browse Source 
C1: SqlDriver's Initialize/Read failure paths no longer interpolate the ADO.NET
provider's ex.Message into LastError, the thrown message, or host status — only
ex.GetType().Name reaches the operator surface; the full exception goes to the log
sink via the structured logger's exception parameter. The driver is dialect-agnostic
over an arbitrary DbProviderFactory, so it cannot rely on any provider's message
discipline (Microsoft.Data.SqlClient's parser echoes an unrecognised keyword
lower-cased, which a value-based redactor misses).

C1a: replace the vacuous credential test (SQLite's "unable to open" message never
contains the connection string, so it passed regardless) with one that fabricates a
DbException whose own .Message carries a credential token and drives it through the
Initialize liveness-failure path via the injectable factory seam — red against the
pre-fix ex.Message interpolation.

I1: HandlePollError now ignores the DbException class ReadAsync already classified,
so a subscribed-read outage is classified (and logged) once, not twice with the
worse message.

I2: the poll's Healthy verdict routes through a shared SetHealthUnlessFaulted guard
(the one Degrade already used), so a late in-flight poll cannot un-fault a driver a
concurrent ReinitializeAsync just faulted.

I3: DiscoverAsync warns when materializing an omitted-type tag as String, the only
operator signal for the declared-vs-published type mismatch on a numeric column.

M1: BuildTagTable wraps the per-entry parse so a stray non-JsonException throw skips
the tag instead of stranding health at Initializing (it runs before Initialize's
try/catch). M2 left as a documented TODO to avoid touching SqlPollReader.

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
Joseph Doherty
2026-07-24 15:28:17 -04:00
parent 443b718158
commit 37f444b99c
2 changed files with 261 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs
+76 -13
View File
@@ -220,10 +220,15 @@ public sealed class SqlDriver
}
catch (Exception ex)
{
// Never interpolate _connectionString — Endpoint is the credential-free rendering.
// Never interpolate _connectionString OR ex.Message onto the operator surface. The driver is
// dialect-agnostic over an arbitrary DbProviderFactory, so it cannot rely on any one provider's
// message discipline — Microsoft.Data.SqlClient's connection-string parser, for one, echoes an
// unrecognised keyword lower-cased, which a value-based password redactor misses entirely. Only
// the exception TYPE reaches LastError/host status; the full exception object goes to the log
// SINK via the structured logger's exception parameter (below), never into a rendered string.
var message =
$"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.Message} " +
$"Check the database is up, the network path is open, and the configured credentials are valid.";
$"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.GetType().Name}. " +
$"Check the database is reachable and the connection string is valid.";
_logger.LogError(ex, "Sql driver {DriverInstanceId} liveness check against {Endpoint} failed.",
_driverInstanceId, Endpoint);
WriteHealth(new DriverHealth(DriverState.Faulted, null, message));
@@ -296,6 +301,20 @@ public sealed class SqlDriver
var folder = builder.Folder(DriverTypeName, DriverTypeName);
foreach (var tag in Tags.Values)
{
if (tag.DeclaredType is null)
{
// Discovery cannot infer a column's real type — that needs live result-set metadata, which
// only a poll returns — so an omitted-type tag materialises as String (the same fallback
// ISqlDialect.MapColumnType uses) while the reader coerces and publishes the column's actual,
// possibly numeric, CLR value against that String node. Nothing else signals the operator to
// this declared-vs-published mismatch, so warn and point them at the fix.
_logger.LogWarning(
"Sql tag '{Tag}' has no authored \"type\" and is discovered as String; a numeric column " +
"will then publish numeric values against a String node. Author an explicit \"type\" on " +
"numeric tags. Driver={DriverInstanceId}",
tag.Name, _driverInstanceId);
}
folder.Variable(tag.Name, tag.Name, new DriverAttributeInfo(
FullName: tag.Name,
DriverDataType: tag.DeclaredType ?? DriverDataType.String,
@@ -344,7 +363,9 @@ public sealed class SqlDriver
// The reader throws for one reason only: opening a connection failed (IReadable's contract).
_logger.LogWarning(ex, "Sql driver {DriverInstanceId} could not reach {Endpoint} during a read.",
_driverInstanceId, Endpoint);
Degrade($"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.Message}");
// Type name only — never ex.Message (see InitializeAsync for why); the full exception is already
// on the log sink via the LogWarning exception parameter above.
Degrade($"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.GetType().Name}.");
TransitionHostTo(HostState.Stopped);
throw;
}
@@ -414,7 +435,7 @@ public sealed class SqlDriver
/// cadence rather than backed off from.</para>
/// </summary>
/// <param name="snapshots">The snapshots the reader returned for one poll.</param>
private void ObservePollOutcome(IReadOnlyList<DataValueSnapshot> snapshots)
internal void ObservePollOutcome(IReadOnlyList<DataValueSnapshot> snapshots)
{
if (snapshots.Count == 0) return;
@@ -447,7 +468,11 @@ public sealed class SqlDriver
if (good == 0) return; // authoring-class Bad only — not a statement about the database.
WriteHealth(new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null));
// Route through the Faulted guard, not a raw WriteHealth: a poll still in flight when a concurrent
// ReinitializeAsync has recorded Faulted (the engine's ~5 s teardown wait is shorter than the 15 s
// default operationTimeout, so a stale poll can still complete afterwards) must not flip Faulted back
// to Healthy with no real recovery. Only a successful (re)initialize clears Faulted.
SetHealthUnlessFaulted(new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null));
TransitionHostTo(HostState.Running);
}
@@ -464,6 +489,15 @@ 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
// classified — good, endpoint-bearing LastError + host Stopped — before rethrowing. The poll engine
// then routes that same exception here. Re-degrading would overwrite the good LastError with a worse,
// guidance-free one (and, for a provider whose text echoes credentials, re-open the C1 leak) and log
// the outage a second time. So skip the connection class ReadAsync owns; the only exceptions that
// reach past this guard are the engine's own contract-violation throws — an InvalidOperationException
// carrying no provider text — which nothing else classifies.
if (ex is DbException) return;
_logger.LogWarning(ex, "Sql poll failed. Driver={DriverInstanceId} Endpoint={Endpoint}",
_driverInstanceId, Endpoint);
Degrade(ex.Message);
@@ -478,8 +512,21 @@ public sealed class SqlDriver
private void Degrade(string reason)
{
var current = ReadHealth();
if (current.State == DriverState.Faulted) return;
WriteHealth(new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, reason));
SetHealthUnlessFaulted(new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, reason));
}
/// <summary>
/// Publishes <paramref name="value"/> unless the driver is already <see cref="DriverState.Faulted"/>
/// — a liveness/config verdict that only a successful (re)initialize may clear. Every non-Initialize
/// health write routes through here: <see cref="Degrade"/> and the poll's Healthy verdict in
/// <see cref="ObservePollOutcome"/>, so a late in-flight poll cannot un-fault a driver a concurrent
/// <see cref="ReinitializeAsync"/> just faulted.
/// </summary>
/// <param name="value">The health snapshot to publish when the driver is not Faulted.</param>
private void SetHealthUnlessFaulted(DriverHealth value)
{
if (ReadHealth().State == DriverState.Faulted) return;
WriteHealth(value);
}
/// <summary>Advances <c>LastSuccessfulRead</c> without changing the current state or error.</summary>
@@ -524,12 +571,26 @@ public sealed class SqlDriver
var table = new Dictionary<string, SqlTagDefinition>(_options.RawTags.Count, StringComparer.Ordinal);
foreach (var entry in _options.RawTags)
{
if (SqlEquipmentTagParser.TryParse(entry.TagConfig, entry.RawPath, out var definition))
table[entry.RawPath] = definition;
else
_logger.LogWarning(
"Sql tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}",
try
{
if (SqlEquipmentTagParser.TryParse(entry.TagConfig, entry.RawPath, out var definition))
table[entry.RawPath] = definition;
else
_logger.LogWarning(
"Sql tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}",
_driverInstanceId, entry.RawPath);
}
catch (Exception ex)
{
// TryParse is contracted never to throw, but BuildTagTable runs BEFORE InitializeAsync's
// try/catch, so a stray throw here (a parser bug on one blob) would escape Initialize and
// strand health at Initializing across every DriverInstanceActor retry. Skip the offending
// tag — the same "one bad tag must not take the whole driver down" rule the malformed-blob
// branch above follows — rather than fault the driver over a single entry.
_logger.LogError(
ex, "Sql tag config threw while parsing; skipping. Driver={DriverInstanceId} RawPath={RawPath}",
_driverInstanceId, entry.RawPath);
}
}
Volatile.Write(ref _tagsByRawPath, table);
@@ -618,6 +679,8 @@ public sealed class SqlDriver
/// <summary>
/// Observes an abandoned liveness attempt's eventual fault so it never surfaces as an unobserved
/// task exception. The task still owns — and disposes — its own connection.
/// <para>TODO(M2): a verbatim duplicate of <c>SqlPollReader.Detach</c>; both live in Driver.Sql and
/// could hoist to one internal helper. Left in place here to keep this change off the reader.</para>
/// </summary>
private static void Detach(Task work)
=> _ = work.ContinueWith(