fix(sql): review minors - superseded-reader polls, rebind faults, disposed-reader guard, JSON position (R06 G2 review m-1 m-2 m-4 m-6 m-7)
m-1: a poll that captured a reader a rebind then disposed retries once on the current reader; with no newer reader the typed SqlPollReaderDisposedException is marked classified, so HandlePollError no longer degrades for it. Groups report a disposed reader as a sentinel so WhenAll surfaces the typed exception, not a bare cancel. m-2: TryAddRef refuses new groups once Dispose ran, matching the doc comment. m-4: a rebind / tag-table failure in InitializeAsync now writes Faulted with a sanitized reason (generation-checked) and host Stopped instead of latching Initializing with no LastError. m-6: SecurityClass-as-descriptor is documented at the Sql site (Commons field is a follow-up). m-7: a position-less JsonException renders '?' instead of blanks. Refs #744 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <see cref="ReadAsync"/>'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.
|
||||
/// </summary>
|
||||
internal async Task<IReadOnlyList<DataValueSnapshot>> ReadThroughAsync(
|
||||
SqlPollReader reader, IReadOnlyList<string> 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();
|
||||
}
|
||||
|
||||
/// <summary>Re-runs <see cref="ApplyBinding"/> on the current binding — a real reader swap, for the m-1 race test.</summary>
|
||||
internal void RebindForTests() => ApplyBinding(_options, _dialect, _connectionString);
|
||||
|
||||
/// <summary>The live poll reader. Test observation of the S-3 lifecycle only.</summary>
|
||||
internal SqlPollReader ReaderForTests => _reader;
|
||||
|
||||
|
||||
@@ -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.");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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, []);
|
||||
|
||||
/// <summary>The reader was disposed before this group could start; surfaced as a typed exception.</summary>
|
||||
public static GroupResult ReaderDisposed { get; } =
|
||||
new(SqlStatusCodes.BadInternalError, NoRows, NoColumns, []);
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A poll reached a <see cref="SqlPollReader"/> 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.
|
||||
/// </summary>
|
||||
public sealed class SqlPollReaderDisposedException()
|
||||
: OperationCanceledException("The Sql poll reader was disposed (replaced by a rebind, or the driver was disposed).");
|
||||
|
||||
/// <summary>One poll's snapshots plus the answer evidence they cannot carry themselves.</summary>
|
||||
/// <param name="Snapshots">One snapshot per requested reference, at the same index.</param>
|
||||
/// <param name="AnsweredRefusalGroups">
|
||||
|
||||
Reference in New Issue
Block a user