fix(drivers): complete the Sql + FOCAS in-place config re-parse (#516)

The first #516 pass deliberately left these two out, because each derives more
than options from config: Sql its ISqlDialect and resolved connection string,
FOCAS the IFocasClientFactory its Backend key selects — all injected at
construction. Adopting new options alone would poll a NEW tag set through the
OLD database/backend, and a half-applied config change is worse than a
discarded one because it looks like it worked.

Rather than accept that exclusion, this fixes the blocker. Each factory now
exposes a ParseBinding returning EVERY config-derived dependency as one value,
and the driver adopts them atomically:

- Sql: ApplyBinding takes (options, dialect, connectionString) and rebuilds
  everything downstream — provider factory, Endpoint, and the SqlPollReader
  that captures all three. It runs BEFORE BuildTagTable, so the tag table and
  the connection it is polled over always come from the same revision. A
  test-injected DbProviderFactory survives a rebind (_explicitFactory), so a
  re-derived dialect cannot displace what a test passed in.
- FOCAS: options and backend move as a pair in InitializeAsync.

Re-resolving Sql's connection string on reinit is a side benefit: a rotated
credential is picked up without a process restart.

Drivers constructed directly get no rebinder and keep their constructor
binding, so every existing "{}"-passing lifecycle test is unaffected by
construction rather than by luck.

The tests pin ATOMICITY, not merely that a re-parse happened — a test checking
only options would have passed against the broken version. Confirmed by
simulating the half-fix (adopt options, skip the backend): 2 of 3 FOCAS tests
go red. Reverting Sql's rebind turns its connection-string test red. Sql also
asserts that a reinit which cannot resolve its new connection leaves the
previous binding whole rather than half-adopting.

All 12 drivers now honour a changed config in place. Sql.Tests 226, FOCAS.Tests
275 pass.
This commit is contained in:
Joseph Doherty
2026-07-28 00:30:52 -04:00
parent 88ce8df099
commit 5184a2e107
7 changed files with 467 additions and 62 deletions
+33
View File
@@ -497,6 +497,39 @@ describes for Tier C no longer exists (Galaxy went to the mxaccessgw sidecar in
in-process when its managed wire client landed). No runtime change — turning recycle on for two live in-process when its managed wire client landed). No runtime change — turning recycle on for two live
drivers deserves its own live gate, not a docs-pass side effect. Tracked as Gitea **#522**. drivers deserves its own live gate, not a docs-pass side effect. Tracked as Gitea **#522**.
### 2026-07-28 — the two things §8.3 deliberately did NOT build (1 of 2)
**Sql and FOCAS now re-parse in place too.** The first pass excluded them for a real reason — each derives
more than options from config, so adopting new options alone would run a **new tag set against an old
connection/backend**, and a half-applied change is worse than a discarded one because it looks like it
worked. Rather than accept that, the blocker itself is fixed: each factory now exposes a **`ParseBinding`**
that returns *every* config-derived dependency as one value, and the driver adopts them **atomically**.
| Driver | What one parse now yields | Adopted by |
|---|---|---|
| Sql | options + `ISqlDialect` + resolved connection string | `ApplyBinding`, which also rebuilds the provider factory, `Endpoint` and the `SqlPollReader` that captures all three |
| FOCAS | options + the `IFocasClientFactory` the `Backend` key selects | one assignment pair in `InitializeAsync` |
Details worth keeping:
- **Sql adopts BEFORE `BuildTagTable`**, so the tag table and the connection it will be polled over always
come from the same revision.
- **A test-injected `DbProviderFactory` survives a rebind** (`_explicitFactory`), so a re-derived dialect
cannot silently displace what a test passed in.
- **Re-resolving the connection string on reinit is a side benefit**: a rotated credential is picked up
without a process restart.
- A driver constructed **directly** gets no rebinder and keeps its constructor-supplied binding — every
existing `"{}"`-passing lifecycle test is unaffected by construction, not by luck.
**The tests pin ATOMICITY, not merely "a re-parse happened"** — a test that only checked options would have
passed against the broken version. Verified by simulating the *half-fix* (adopt options, skip the backend):
2 of the 3 FOCAS tests go red. Reverting Sql's rebind turns its connection-string test red. Sql also asserts
that a reinit which **cannot resolve** its new connection leaves the previous binding whole, rather than
half-adopting.
All 12 drivers now honour a changed config in place; `DriverSpawnPlanner`'s stop + respawn remains the outer
guarantee. Sql.Tests 226 / FOCAS.Tests 275 pass.
### 2026-07-27 — §8.4 dispatch-map parity (G-1 … G-6) ### 2026-07-27 — §8.4 dispatch-map parity (G-1 … G-6)
**The maps had to become data before they could be guarded.** A Razor `@switch` compiles into **The maps had to become data before they could be guarded.** A Razor `@switch` compiles into
@@ -21,9 +21,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, ISubscribable, public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, ISubscribable,
IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, IDisposable, IAsyncDisposable IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, IDisposable, IAsyncDisposable
{ {
private readonly FocasDriverOptions _options; /// <summary>Mutable so <see cref="InitializeAsync"/> can adopt a re-parsed config (#516). Written only
/// on the init path, together with <see cref="_clientFactory"/>, before either is used.</summary>
private FocasDriverOptions _options;
private readonly string _driverInstanceId; private readonly string _driverInstanceId;
private readonly IFocasClientFactory _clientFactory; /// <summary>Mutable for the same reason as <see cref="_options"/> — the <c>Backend</c> config key
/// selects it, so the two must move together or a new tag set polls through an old backend.</summary>
private IFocasClientFactory _clientFactory;
/// <summary>Re-parses a <c>DriverConfig</c> into every config-derived dependency. Null when the driver
/// was constructed directly (tests, or a caller supplying its own backend), which keeps the
/// constructor-supplied options authoritative — exactly the prior behaviour.</summary>
private readonly Func<string, FocasDriverBinding>? _rebind;
private readonly PollGroupEngine _poll; private readonly PollGroupEngine _poll;
private readonly ILogger<FocasDriver> _logger; private readonly ILogger<FocasDriver> _logger;
private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase);
@@ -58,14 +67,20 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
/// <param name="driverInstanceId">The unique identifier for this driver instance.</param> /// <param name="driverInstanceId">The unique identifier for this driver instance.</param>
/// <param name="clientFactory">Optional factory for creating FOCAS client instances.</param> /// <param name="clientFactory">Optional factory for creating FOCAS client instances.</param>
/// <param name="logger">Optional logger instance.</param> /// <param name="logger">Optional logger instance.</param>
/// <param name="rebind">Optional re-parse of a <c>DriverConfig</c> into every config-derived dependency,
/// supplied by the factory so <see cref="InitializeAsync"/> can adopt a changed config. Null (direct
/// construction, e.g. a test or a caller supplying its own backend) keeps the constructor-supplied
/// options + backend.</param>
public FocasDriver(FocasDriverOptions options, string driverInstanceId, public FocasDriver(FocasDriverOptions options, string driverInstanceId,
IFocasClientFactory? clientFactory = null, IFocasClientFactory? clientFactory = null,
ILogger<FocasDriver>? logger = null) ILogger<FocasDriver>? logger = null,
Func<string, FocasDriverBinding>? rebind = null)
{ {
ArgumentNullException.ThrowIfNull(options); ArgumentNullException.ThrowIfNull(options);
_options = options; _options = options;
_driverInstanceId = driverInstanceId; _driverInstanceId = driverInstanceId;
_clientFactory = clientFactory ?? new Wire.WireFocasClientFactory(); _clientFactory = clientFactory ?? new Wire.WireFocasClientFactory();
_rebind = rebind;
_logger = logger ?? NullLogger<FocasDriver>.Instance; _logger = logger ?? NullLogger<FocasDriver>.Instance;
_resolver = new EquipmentTagRefResolver<FocasTagDefinition>( _resolver = new EquipmentTagRefResolver<FocasTagDefinition>(
r => _tagsByRawPath.TryGetValue(r, out var t) ? t : null); r => _tagsByRawPath.TryGetValue(r, out var t) ? t : null);
@@ -77,6 +92,16 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
backoffCap: PollBackoffCap); backoffCap: PollBackoffCap);
} }
/// <summary>True when the supplied DriverConfig JSON carries a real body. The bootstrapper always passes
/// a populated document; some unit tests pass <c>"{}"</c> or an empty string to exercise lifecycle shape
/// without a config — those keep the constructor-supplied options + backend.</summary>
private static bool HasConfigBody(string? driverConfigJson)
{
if (string.IsNullOrWhiteSpace(driverConfigJson)) return false;
var trimmed = driverConfigJson.Trim();
return trimmed is not "{}" and not "[]";
}
/// <summary>Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8).</summary> /// <summary>Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8).</summary>
private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30); private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30);
@@ -102,13 +127,15 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
/// <summary> /// <summary>
/// Opens the configured CNC handles and builds the authored tag table. /// Opens the configured CNC handles and builds the authored tag table.
/// <para><paramref name="driverConfigJson"/> is deliberately <b>not</b> re-parsed here, unlike /// <para><paramref name="driverConfigJson"/> <b>is</b> re-parsed here when the driver was built by
/// Modbus / AbLegacy / OpcUaClient (Gitea #516). This driver builds more than options from config — /// the factory (Gitea #516), and the re-parse is <b>all-or-nothing</b>: the typed options and the
/// its <c>IFocasClientFactory</c> is selected from the <c>Backend</c> key and injected at /// <c>IFocasClientFactory</c> the <c>Backend</c> key selects come from one <c>ParseBinding</c> call
/// construction — so adopting a new <c>FocasDriverOptions</c> alone would run a NEW device/tag set /// and are adopted together. That pairing is the point — adopting options alone would poll a NEW
/// against the OLD backend. Half a re-parse is worse than none.</para> /// device/tag set through the OLD backend, which is why this driver was excluded from the first
/// <para>A config change is covered by the stop + respawn in <c>DriverSpawnPlanner</c>, which /// #516 pass.</para>
/// rebuilds options and backend together through the factory.</para> /// <para>A driver constructed directly (every unit test, or a caller supplying its own backend) gets
/// no rebinder and keeps its constructor-supplied pair. <c>DriverSpawnPlanner</c>'s stop + respawn
/// remains the outer guarantee.</para>
/// </summary> /// </summary>
/// <param name="driverConfigJson">The driver configuration JSON (see remarks — not re-parsed).</param> /// <param name="driverConfigJson">The driver configuration JSON (see remarks — not re-parsed).</param>
/// <param name="cancellationToken">Cancellation token for the operation.</param> /// <param name="cancellationToken">Cancellation token for the operation.</param>
@@ -118,6 +145,17 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null)); Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null));
try try
{ {
// #516: adopt a changed config. Options and the backend client factory are re-derived by ONE
// parse and assigned together — a partial adoption would poll a new device/tag set through the
// old backend. A null rebinder (direct construction) or an empty/placeholder document keeps the
// constructor-supplied pair, which is what every lifecycle test passing "{}" relies on.
if (_rebind is not null && HasConfigBody(driverConfigJson))
{
var binding = _rebind(driverConfigJson);
_options = binding.Options;
_clientFactory = binding.ClientFactory;
}
// Fail fast if the factory is a stub/unimplemented backend — the operator must // Fail fast if the factory is a stub/unimplemented backend — the operator must
// see an actionable error at init rather than a phantom-Healthy driver that fails // see an actionable error at init rather than a phantom-Healthy driver that fails
// every read/write/subscribe silently. // every read/write/subscribe silently.
@@ -47,6 +47,29 @@ public static class FocasDriverFactoryExtensions
/// <param name="driverConfigJson">The driver configuration JSON string.</param> /// <param name="driverConfigJson">The driver configuration JSON string.</param>
/// <returns>A configured <see cref="FocasDriver"/> instance.</returns> /// <returns>A configured <see cref="FocasDriver"/> instance.</returns>
internal static FocasDriver CreateInstance(string driverInstanceId, string driverConfigJson) internal static FocasDriver CreateInstance(string driverInstanceId, string driverConfigJson)
{
var binding = ParseBinding(driverInstanceId, driverConfigJson);
return new FocasDriver(
binding.Options,
driverInstanceId,
binding.ClientFactory,
// Hand the driver the SAME parse it was built with, so InitializeAsync can re-derive EVERY
// config-derived dependency on a reinitialize — options AND the backend client factory
// together (#516). Re-parsing options alone would run a new device/tag set against the old
// backend, which is worse than not re-parsing at all.
rebind: json => ParseBinding(driverInstanceId, json));
}
/// <summary>
/// Parses a FOCAS <c>DriverConfig</c> JSON document into <b>every</b> dependency the driver derives
/// from config: the typed options and the <see cref="IFocasClientFactory"/> the <c>Backend</c> key
/// selects. Returned together because they must be adopted together — see the <c>rebind</c> note in
/// <see cref="CreateInstance"/>.
/// </summary>
/// <param name="driverInstanceId">The unique driver instance identifier.</param>
/// <param name="driverConfigJson">The driver configuration JSON string.</param>
/// <returns>The parsed options + backend factory.</returns>
internal static FocasDriverBinding ParseBinding(string driverInstanceId, string driverConfigJson)
{ {
ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId);
ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson);
@@ -86,8 +109,7 @@ public static class FocasDriverFactoryExtensions
HandleRecycle = BuildHandleRecycle(dto.HandleRecycle), HandleRecycle = BuildHandleRecycle(dto.HandleRecycle),
}; };
var clientFactory = BuildClientFactory(dto, driverInstanceId); return new FocasDriverBinding(options, BuildClientFactory(dto, driverInstanceId));
return new FocasDriver(options, driverInstanceId, clientFactory);
} }
/// <summary> /// <summary>
@@ -327,3 +349,14 @@ public static class FocasDriverFactoryExtensions
public TimeSpan? Interval { get; init; } public TimeSpan? Interval { get; init; }
} }
} }
/// <summary>
/// Everything <c>FocasDriver</c> derives from its <c>DriverConfig</c> JSON, returned as one value so a
/// reinitialize adopts all of it atomically.
/// <para>This exists because a partial adoption is a real hazard, not a theoretical one: the driver's
/// <see cref="IFocasClientFactory"/> is chosen by the config's <c>Backend</c> key, so re-parsing
/// <see cref="Options"/> alone would poll a NEW device/tag set through the OLD backend (Gitea #516).</para>
/// </summary>
/// <param name="Options">The typed driver options.</param>
/// <param name="ClientFactory">The backend client factory the <c>Backend</c> key selects.</param>
public sealed record FocasDriverBinding(FocasDriverOptions Options, IFocasClientFactory ClientFactory);
@@ -1,4 +1,5 @@
using System.Data.Common; using System.Data.Common;
using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
@@ -51,16 +52,19 @@ public sealed class SqlDriver
// ---- instance fields (grouped at top for auditability) ---- // ---- instance fields (grouped at top for auditability) ----
private readonly SqlDriverOptions _options; /// <summary>Config-derived, so mutable — see <see cref="ApplyBinding"/>.</summary>
private SqlDriverOptions _options;
private readonly string _driverInstanceId; private readonly string _driverInstanceId;
private readonly ISqlDialect _dialect; /// <summary>Config-derived, so mutable: <see cref="ApplyBinding"/> swaps it with the connection string
private readonly DbProviderFactory _factory; /// and options together on a reinitialize (#516).</summary>
private ISqlDialect _dialect;
private DbProviderFactory _factory;
/// <summary> /// <summary>
/// The resolved connection string — a <b>secret</b>. It is passed to the provider and to nothing /// The resolved connection string — a <b>secret</b>. It is passed to the provider and to nothing
/// else: every log line, health message and host status carries <see cref="Endpoint"/> instead. /// else: every log line, health message and host status carries <see cref="Endpoint"/> instead.
/// </summary> /// </summary>
private readonly string _connectionString; private string _connectionString;
private readonly ILogger<SqlDriver> _logger; private readonly ILogger<SqlDriver> _logger;
@@ -93,11 +97,19 @@ public sealed class SqlDriver
/// <summary>Resolves a read/subscribe RawPath to its <b>catalog-validated</b> definition, or a miss.</summary> /// <summary>Resolves a read/subscribe RawPath to its <b>catalog-validated</b> definition, or a miss.</summary>
private readonly EquipmentTagRefResolver<SqlTagDefinition> _resolver; private readonly EquipmentTagRefResolver<SqlTagDefinition> _resolver;
/// <summary>The factory the CALLER passed, or null to take the dialect's. Kept separate from
/// <see cref="_factory"/> so a re-derived dialect cannot silently displace a test's injected factory.</summary>
private readonly DbProviderFactory? _explicitFactory;
/// <summary>Re-parses a <c>DriverConfig</c> into every config-derived dependency. Null when the driver
/// was constructed directly, which keeps the constructor-supplied trio authoritative.</summary>
private readonly Func<string, SqlDriverBinding>? _rebind;
/// <summary> /// <summary>
/// The read path. Built once, in the constructor, and <b>not</b> injected: its <c>resolve</c> /// The read path. Built once, in the constructor, and <b>not</b> injected: its <c>resolve</c>
/// delegate must read this driver's live authored table, which does not exist before the driver does. /// delegate must read this driver's live authored table, which does not exist before the driver does.
/// </summary> /// </summary>
private readonly SqlPollReader _reader; private SqlPollReader _reader;
/// <summary>Polled subscriptions. The driver supplies the reader + change bridge; the engine owns the loop.</summary> /// <summary>Polled subscriptions. The driver supplies the reader + change bridge; the engine owns the loop.</summary>
private readonly PollGroupEngine _poll; private readonly PollGroupEngine _poll;
@@ -133,6 +145,11 @@ public sealed class SqlDriver
/// explicitly by tests that need to observe or delay connection creation. /// explicitly by tests that need to observe or delay connection creation.
/// </param> /// </param>
/// <param name="logger">Optional; defaults to a no-op logger.</param> /// <param name="logger">Optional; defaults to a no-op logger.</param>
/// <param name="rebind">
/// Optional re-parse of a <c>DriverConfig</c> into every config-derived dependency, supplied by the
/// factory so <see cref="InitializeAsync"/> can adopt a changed config. Null (direct construction —
/// every test) keeps the constructor-supplied options, dialect and connection string.
/// </param>
/// <exception cref="ArgumentNullException">A required reference argument is null.</exception> /// <exception cref="ArgumentNullException">A required reference argument is null.</exception>
/// <exception cref="ArgumentException"><paramref name="driverInstanceId"/> or <paramref name="connectionString"/> is blank.</exception> /// <exception cref="ArgumentException"><paramref name="driverInstanceId"/> or <paramref name="connectionString"/> is blank.</exception>
public SqlDriver( public SqlDriver(
@@ -141,7 +158,8 @@ public sealed class SqlDriver
ISqlDialect dialect, ISqlDialect dialect,
string connectionString, string connectionString,
DbProviderFactory? factory = null, DbProviderFactory? factory = null,
ILogger<SqlDriver>? logger = null) ILogger<SqlDriver>? logger = null,
Func<string, SqlDriverBinding>? rebind = null)
{ {
ArgumentNullException.ThrowIfNull(options); ArgumentNullException.ThrowIfNull(options);
ArgumentNullException.ThrowIfNull(dialect); ArgumentNullException.ThrowIfNull(dialect);
@@ -149,15 +167,47 @@ public sealed class SqlDriver
if (string.IsNullOrWhiteSpace(connectionString)) if (string.IsNullOrWhiteSpace(connectionString))
throw new ArgumentException("A Sql driver needs a resolved connection string.", nameof(connectionString)); throw new ArgumentException("A Sql driver needs a resolved connection string.", nameof(connectionString));
_options = options;
_driverInstanceId = driverInstanceId; _driverInstanceId = driverInstanceId;
_dialect = dialect;
_factory = factory ?? dialect.Factory;
_connectionString = connectionString;
_logger = logger ?? NullLogger<SqlDriver>.Instance; _logger = logger ?? NullLogger<SqlDriver>.Instance;
Endpoint = DescribeEndpoint(connectionString); _explicitFactory = factory;
_rebind = rebind;
_resolver = new EquipmentTagRefResolver<SqlTagDefinition>(Lookup); _resolver = new EquipmentTagRefResolver<SqlTagDefinition>(Lookup);
// Everything below this line is derived from config and is re-derived wholesale on a reinitialize.
ApplyBinding(options, dialect, connectionString);
_poll = new PollGroupEngine(
reader: ReadAsync,
onChange: (handle, tagRef, snapshot) =>
OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)),
onError: HandlePollError,
backoffCap: PollBackoffCap);
}
/// <summary>
/// Adopts a config-derived trio — options, dialect, connection string — and rebuilds everything
/// downstream of it (the provider factory, the endpoint description, and the poll reader, which
/// captures all three). Called from the constructor and again from <see cref="InitializeAsync"/>
/// when a changed config arrives.
/// <para><b>All-or-nothing on purpose.</b> The reason Sql was excluded from the first #516 pass is
/// that adopting options alone would poll a NEW tag set through the OLD connection and dialect. Doing
/// it in one method means a future field derived from config gets added here rather than being
/// forgotten on the reinit path.</para>
/// <para>Not thread-safe by design: only the init path calls it, before any poll group is running.
/// The poll engine and the tag resolver survive across a rebind — the engine holds a delegate to
/// <c>ReadAsync</c> and the resolver reads the live tag table, so neither captures the old trio.</para>
/// </summary>
/// <param name="options">The typed options to adopt.</param>
/// <param name="dialect">The provider seam to adopt.</param>
/// <param name="connectionString">The resolved connection string to adopt. Never logged.</param>
[MemberNotNull(nameof(_options), nameof(_dialect), nameof(_factory), nameof(_connectionString), nameof(_reader))]
private void ApplyBinding(SqlDriverOptions options, ISqlDialect dialect, string connectionString)
{
_options = options;
_dialect = dialect;
_factory = _explicitFactory ?? dialect.Factory;
_connectionString = connectionString;
Endpoint = DescribeEndpoint(connectionString);
_reader = new SqlPollReader( _reader = new SqlPollReader(
_factory, _factory,
connectionString, connectionString,
@@ -168,12 +218,16 @@ public sealed class SqlDriver
nullIsBad: options.NullIsBad, nullIsBad: options.NullIsBad,
resolve: rawPath => _resolver.TryResolve(rawPath, out var definition) ? definition : null, resolve: rawPath => _resolver.TryResolve(rawPath, out var definition) ? definition : null,
logger: _logger); logger: _logger);
_poll = new PollGroupEngine( }
reader: ReadAsync,
onChange: (handle, tagRef, snapshot) => /// <summary>True when the supplied DriverConfig JSON carries a real body. The bootstrapper always passes
OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), /// a populated document; some unit tests pass <c>"{}"</c> or an empty string to exercise lifecycle shape
onError: HandlePollError, /// without a config — those keep the constructor-supplied trio.</summary>
backoffCap: PollBackoffCap); private static bool HasConfigBody(string? driverConfigJson)
{
if (string.IsNullOrWhiteSpace(driverConfigJson)) return false;
var trimmed = driverConfigJson.Trim();
return trimmed is not "{}" and not "[]";
} }
/// <inheritdoc /> /// <inheritdoc />
@@ -187,7 +241,7 @@ public sealed class SqlDriver
/// the connection string names both. This is the only rendering of the connection string that may /// the connection string names both. This is the only rendering of the connection string that may
/// appear in a log, a health message, or the Admin UI. /// appear in a log, a health message, or the Admin UI.
/// </summary> /// </summary>
public string Endpoint { get; } public string Endpoint { get; private set; } = "";
/// <summary>Host identifier surfaced through <see cref="IHostConnectivityProbe"/> — the endpoint description.</summary> /// <summary>Host identifier surfaced through <see cref="IHostConnectivityProbe"/> — the endpoint description.</summary>
public string HostName => Endpoint; public string HostName => Endpoint;
@@ -215,17 +269,20 @@ public sealed class SqlDriver
/// <summary> /// <summary>
/// Builds the authored RawPath table and proves the database is reachable. /// Builds the authored RawPath table and proves the database is reachable.
/// <para><paramref name="driverConfigJson"/> is <b>not</b> re-parsed here: the driver serves the /// <para><paramref name="driverConfigJson"/> <b>is</b> re-parsed here when the driver was built by
/// typed <see cref="SqlDriverOptions"/> it was constructed with. That premise — "config parsing /// the factory (Gitea #516). The original comment on this method claimed it was not, on the premise
/// belongs to the factory, which builds a fresh instance" — was <b>false when written</b> /// that "config parsing belongs to the factory, which builds a fresh instance" — that was false when
/// (Gitea #516): the host reinitialized the EXISTING child in place, so every config edit was /// written, because the host reinitialized the EXISTING child in place and every config edit was
/// silently discarded and the deployment still sealed green. It is true <i>now</i>, because /// silently discarded while the deployment still sealed green.</para>
/// <c>DriverSpawnPlanner</c> routes a changed <c>DriverConfig</c> through a stop + respawn.</para> /// <para>The re-parse is <b>all-or-nothing</b> via <see cref="ApplyBinding"/>: options, the
/// <para>This driver deliberately does <b>not</b> also re-parse in place, unlike Modbus / AbLegacy / /// <see cref="ISqlDialect"/> and the resolved connection string are re-derived by one
/// OpcUaClient. It builds more than options from config — the <see cref="ISqlDialect"/> and the /// <c>ParseBinding</c> call and adopted together. That is what makes it safe here — adopting options
/// resolved connection string are both injected at construction — so adopting a new /// alone would poll a NEW tag set through the OLD database, which is why this driver was excluded
/// <see cref="SqlDriverOptions"/> alone would run a NEW tag set against the OLD connection. Half a /// from the first #516 pass. It happens BEFORE <c>BuildTagTable</c>, so the tag table and the
/// re-parse is worse than none; the respawn rebuilds all three together.</para> /// connection it is polled over always come from the same revision.</para>
/// <para>A driver constructed directly (every unit test) gets no rebinder and keeps its
/// constructor-supplied trio, so <c>"{}"</c>-passing lifecycle tests are unaffected.
/// <c>DriverSpawnPlanner</c>'s stop + respawn remains the outer guarantee.</para>
/// <para>The table is built first because it is pure and cannot fail; the liveness check is the only /// <para>The table is built first because it is pure and cannot fail; the liveness check is the only
/// I/O, and on failure this method records <see cref="DriverState.Faulted"/> <b>and rethrows</b> — /// I/O, and on failure this method records <see cref="DriverState.Faulted"/> <b>and rethrows</b> —
/// <c>DriverInstanceActor</c> reads a throw as InitializeFailed and lands in Reconnecting with its /// <c>DriverInstanceActor</c> reads a throw as InitializeFailed and lands in Reconnecting with its
@@ -243,6 +300,18 @@ public sealed class SqlDriver
public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken)
{ {
WriteHealth(new DriverHealth(DriverState.Initializing, null, null)); WriteHealth(new DriverHealth(DriverState.Initializing, null, null));
// #516: adopt a changed config BEFORE the tag table is built, so the table and the connection it
// will be polled over come from the same revision. ApplyBinding takes the options, dialect and
// 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))
{
var binding = _rebind(driverConfigJson);
ApplyBinding(binding.Options, binding.Dialect, binding.ConnectionString);
}
BuildTagTable(); BuildTagTable();
try try
@@ -81,6 +81,43 @@ public static class SqlDriverFactoryExtensions
public static SqlDriver CreateInstance(string driverInstanceId, string driverConfigJson) public static SqlDriver CreateInstance(string driverInstanceId, string driverConfigJson)
=> CreateInstance(driverInstanceId, driverConfigJson, loggerFactory: null); => CreateInstance(driverInstanceId, driverConfigJson, loggerFactory: null);
/// <summary>
/// Parses a Sql <c>DriverConfig</c> JSON document into <b>every</b> dependency the driver derives
/// from config: the typed options, the <see cref="ISqlDialect"/> the <c>provider</c> key selects, and
/// the connection string resolved from <c>connectionStringRef</c>.
/// <para>Returned together because they must be adopted together. Re-parsing options alone on a
/// reinitialize would run a NEW tag set against the OLD connection and dialect (Gitea #516) — a
/// half-applied config change is worse than a discarded one, because it looks like it worked.</para>
/// <para>Re-running this on reinitialize also re-resolves the connection string from its environment
/// variable, so a rotated credential is picked up without a process restart.</para>
/// </summary>
/// <param name="driverInstanceId">The central config DB's identity for this driver instance.</param>
/// <param name="driverConfigJson">The driver configuration JSON.</param>
/// <returns>The parsed options, dialect and resolved connection string.</returns>
/// <exception cref="InvalidOperationException">The config is malformed or names no connection string.</exception>
internal static SqlDriverBinding ParseBinding(string driverInstanceId, string driverConfigJson)
{
ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId);
ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson);
var dto = Deserialize(driverInstanceId, driverConfigJson);
if (string.IsNullOrWhiteSpace(dto.ConnectionStringRef))
{
throw new InvalidOperationException(
$"Sql driver config for '{driverInstanceId}' is missing the required connectionStringRef. " +
$"Author the NAME of a connection string (the value itself is supplied out-of-band through " +
$"the '{SqlConnectionStringResolver.EnvironmentVariablePrefix}<ref>' environment variable).");
}
var dialect = CreateDialect(dto.Provider, driverInstanceId);
// Resolved BEFORE the knob validation in CreateInstance, so that every subsequent throw is a path
// that HAS the secret in hand — which is exactly where a careless interpolation would leak it.
var connectionString = SqlConnectionStringResolver.Resolve(dto.ConnectionStringRef!);
return new SqlDriverBinding(BuildOptions(dto, driverInstanceId), dialect, connectionString, dto);
}
/// <summary> /// <summary>
/// Logger-aware overload — used by <see cref="Register"/>'s closure when wired through DI. /// Logger-aware overload — used by <see cref="Register"/>'s closure when wired through DI.
/// <para>Constructs the driver only: the driver builds its own <see cref="SqlPollReader"/>, because /// <para>Constructs the driver only: the driver builds its own <see cref="SqlPollReader"/>, because
@@ -102,25 +139,11 @@ public static class SqlDriverFactoryExtensions
public static SqlDriver CreateInstance( public static SqlDriver CreateInstance(
string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory) string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory)
{ {
ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); var binding = ParseBinding(driverInstanceId, driverConfigJson);
ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); var dto = binding.Dto;
var dialect = binding.Dialect;
var dto = Deserialize(driverInstanceId, driverConfigJson); var connectionString = binding.ConnectionString;
var options = binding.Options;
if (string.IsNullOrWhiteSpace(dto.ConnectionStringRef))
{
throw new InvalidOperationException(
$"Sql driver config for '{driverInstanceId}' is missing the required connectionStringRef. " +
$"Author the NAME of a connection string (the value itself is supplied out-of-band through " +
$"the '{SqlConnectionStringResolver.EnvironmentVariablePrefix}<ref>' environment variable).");
}
var dialect = CreateDialect(dto.Provider, driverInstanceId);
// Resolved BEFORE the knob validation below, so that every subsequent throw is a path that HAS the
// secret in hand — which is exactly where a careless interpolation would leak it.
var connectionString = SqlConnectionStringResolver.Resolve(dto.ConnectionStringRef!);
var options = BuildOptions(dto, driverInstanceId);
var logger = (ILogger?)loggerFactory?.CreateLogger(typeof(SqlDriverFactoryExtensions).FullName!) var logger = (ILogger?)loggerFactory?.CreateLogger(typeof(SqlDriverFactoryExtensions).FullName!)
?? NullLogger.Instance; ?? NullLogger.Instance;
@@ -143,7 +166,11 @@ public static class SqlDriverFactoryExtensions
dialect, dialect,
connectionString, connectionString,
factory: null, factory: null,
logger: loggerFactory?.CreateLogger<SqlDriver>()); logger: loggerFactory?.CreateLogger<SqlDriver>(),
// Hand the driver the SAME parse it was built with, so InitializeAsync can re-derive options,
// dialect and connection string together on a reinitialize (#516). Re-resolving the connection
// string also picks up a rotated credential without a process restart.
rebind: json => ParseBinding(driverInstanceId, json));
// Endpoint is the credential-free server/database rendering — the ONLY form of the connection string // Endpoint is the credential-free server/database rendering — the ONLY form of the connection string
// permitted outside the provider call. // permitted outside the provider call.
@@ -250,3 +277,21 @@ public static class SqlDriverFactoryExtensions
} }
} }
} }
/// <summary>
/// Everything <c>SqlDriver</c> derives from its <c>DriverConfig</c> JSON, returned as one value so a
/// reinitialize adopts all of it atomically.
/// <para>Partial adoption is the hazard this type exists to prevent: the dialect and the resolved
/// connection string are both config-derived, so adopting new <see cref="Options"/> alone would poll a
/// NEW tag set against the OLD database (Gitea #516).</para>
/// </summary>
/// <param name="Options">The typed driver options.</param>
/// <param name="Dialect">The provider seam the <c>provider</c> key selects.</param>
/// <param name="ConnectionString">The connection string resolved from <c>connectionStringRef</c>. Never logged.</param>
/// <param name="Dto">The raw parsed DTO, so a caller needing a key the options do not carry (e.g.
/// <c>allowWrites</c>, which the driver ignores but warns about) need not re-deserialize.</param>
public sealed record SqlDriverBinding(
SqlDriverOptions Options,
ISqlDialect Dialect,
string ConnectionString,
SqlDriverConfigDto Dto);
@@ -0,0 +1,67 @@
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests;
/// <summary>
/// Gitea #516 — FOCAS was excluded from the first pass because it derives MORE than options from config:
/// the <c>Backend</c> key selects its <see cref="IFocasClientFactory"/>, injected at construction. Adopting
/// a new <c>FocasDriverOptions</c> alone would poll a NEW device/tag set through the OLD backend, which
/// looks like it worked and does not.
/// <para><c>ParseBinding</c> now returns options + backend together and <c>InitializeAsync</c> adopts both
/// or neither. These tests pin the BACKEND moving — the half that was missing — because a test that only
/// checked options would have passed against the broken version.</para>
/// </summary>
[Trait("Category", "Unit")]
public sealed class FocasReinitConfigAdoptionTests
{
/// <summary>
/// Reinitializing onto <c>Backend: "unimplemented"</c> must make the driver adopt that backend, whose
/// <c>EnsureUsable()</c> throws by design. A driver still holding the constructor's <c>wire</c>
/// backend would not throw — so the throw is a direct read of which backend is live.
/// </summary>
[Fact]
public async Task Reinitialize_adopts_a_changed_backend_not_just_the_options()
{
var driver = FocasDriverFactoryExtensions.CreateInstance(
"focas-1", """{"backend":"wire","devices":[]}""");
var ex = await Record.ExceptionAsync(() => driver.ReinitializeAsync(
"""{"backend":"unimplemented","devices":[]}""", CancellationToken.None));
ex.ShouldBeOfType<NotSupportedException>(
"FocasDriver adopted a reinitialized config but kept its ORIGINAL backend — the half-applied "
+ "adoption (#516) that would poll a new device/tag set through the old backend");
}
/// <summary>The other direction: moving OFF the unimplemented backend must also take effect, so the
/// adoption is not a one-way latch.</summary>
[Fact]
public async Task Reinitialize_away_from_the_unimplemented_backend_also_takes_effect()
{
var driver = FocasDriverFactoryExtensions.CreateInstance(
"focas-1", """{"backend":"unimplemented","devices":[]}""");
// Sanity: the constructor-supplied backend is the throwing one.
(await Record.ExceptionAsync(() => driver.InitializeAsync("{}", CancellationToken.None)))
.ShouldBeOfType<NotSupportedException>();
var ex = await Record.ExceptionAsync(() => driver.ReinitializeAsync(
"""{"backend":"wire","devices":[]}""", CancellationToken.None));
ex.ShouldNotBeOfType<NotSupportedException>(
"FocasDriver stayed on the unimplemented backend after a reinitialize that selected 'wire'");
}
/// <summary>An empty/placeholder document keeps the constructor-supplied pair, so the lifecycle tests
/// that pass <c>"{}"</c> keep meaning what they meant.</summary>
[Fact]
public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_backend()
{
var driver = FocasDriverFactoryExtensions.CreateInstance(
"focas-1", """{"backend":"unimplemented","devices":[]}""");
(await Record.ExceptionAsync(() => driver.ReinitializeAsync("{}", CancellationToken.None)))
.ShouldBeOfType<NotSupportedException>();
}
}
@@ -0,0 +1,120 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts;
namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests;
/// <summary>
/// Gitea #516 — Sql was excluded from the first pass because it derives MORE than options from config
/// (its <see cref="ISqlDialect"/> and its resolved connection string are both config-derived), so
/// adopting a new <c>SqlDriverOptions</c> alone would poll a NEW tag set through the OLD database.
/// A half-applied config change is worse than a discarded one, because it looks like it worked.
/// <para>The fix is <c>ParseBinding</c> + <c>ApplyBinding</c>: one parse produces all three, and they are
/// adopted together. These tests pin the ATOMICITY, not just that a re-parse happens — that is the
/// property that made this driver special.</para>
/// </summary>
[Trait("Category", "Unit")]
public sealed class SqlReinitConfigAdoptionTests
{
private const string RefA = "SqlReinitTestA";
private const string RefB = "SqlReinitTestB";
/// <summary>
/// A reinitialize that changes <c>connectionStringRef</c> must move the driver onto the NEW database.
/// <c>Endpoint</c> is the credential-free rendering of the live connection string, so it is a direct
/// read of which connection the driver actually adopted — no log-string matching.
/// </summary>
[Fact]
public async Task Reinitialize_adopts_a_changed_connection_string_not_just_the_options()
{
using var _ = new ScopedEnvironmentVariable(
SqlConnectionStringResolver.EnvironmentVariablePrefix + RefA,
"Server=old-server;Database=OldDb;Integrated Security=true");
using var __ = new ScopedEnvironmentVariable(
SqlConnectionStringResolver.EnvironmentVariablePrefix + RefB,
"Server=new-server;Database=NewDb;Integrated Security=true");
var driver = SqlDriverFactoryExtensions.CreateInstance(
"sql-1", $$"""{"provider":"SqlServer","connectionStringRef":"{{RefA}}"}""", loggerFactory: null);
driver.Endpoint.ShouldContain("old-server");
// The database is unreachable, so init faults — irrelevant here. The assertion is which database it
// tried, i.e. whether the connection string moved with the options.
try
{
await driver.ReinitializeAsync(
$$"""{"provider":"SqlServer","connectionStringRef":"{{RefB}}"}""", CancellationToken.None);
}
catch
{
// Liveness failure against a non-existent server is expected.
}
driver.Endpoint.ShouldContain(
"new-server",
customMessage:
"SqlDriver adopted a reinitialized config but kept its ORIGINAL connection string — the exact "
+ "half-applied adoption (#516) that would poll a new tag set against the old database");
driver.Endpoint.ShouldNotContain("old-server");
}
/// <summary>
/// A config whose <c>connectionStringRef</c> names an unprovisioned environment variable must make
/// the whole reinitialize FAIL, leaving the previous binding intact — never a partial adoption where
/// new options are live against the old connection.
/// </summary>
[Fact]
public async Task A_reinitialize_that_cannot_resolve_its_connection_leaves_the_previous_binding_intact()
{
using var _ = new ScopedEnvironmentVariable(
SqlConnectionStringResolver.EnvironmentVariablePrefix + RefA,
"Server=old-server;Database=OldDb;Integrated Security=true");
var driver = SqlDriverFactoryExtensions.CreateInstance(
"sql-1", $$"""{"provider":"SqlServer","connectionStringRef":"{{RefA}}"}""", loggerFactory: null);
await Should.ThrowAsync<Exception>(() => driver.ReinitializeAsync(
"""{"provider":"SqlServer","connectionStringRef":"SqlReinitTestNeverProvisioned"}""",
CancellationToken.None));
driver.Endpoint.ShouldContain(
"old-server",
customMessage:
"a reinitialize that could not resolve its new connection must leave the previous binding whole");
}
/// <summary>An empty/placeholder document keeps the constructor-supplied binding, so the many lifecycle
/// tests that pass <c>"{}"</c> keep meaning what they meant.</summary>
[Fact]
public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_binding()
{
using var _ = new ScopedEnvironmentVariable(
SqlConnectionStringResolver.EnvironmentVariablePrefix + RefA,
"Server=old-server;Database=OldDb;Integrated Security=true");
var driver = SqlDriverFactoryExtensions.CreateInstance(
"sql-1", $$"""{"provider":"SqlServer","connectionStringRef":"{{RefA}}"}""", loggerFactory: null);
try { await driver.ReinitializeAsync("{}", CancellationToken.None); }
catch { /* liveness failure expected */ }
driver.Endpoint.ShouldContain("old-server");
}
/// <summary>Sets an environment variable for the duration of a test and restores it after. The resolver
/// reads process-global state, so a leaked variable would race every sibling test.</summary>
private sealed class ScopedEnvironmentVariable : IDisposable
{
private readonly string _name;
private readonly string? _previous;
public ScopedEnvironmentVariable(string name, string value)
{
_name = name;
_previous = Environment.GetEnvironmentVariable(name);
Environment.SetEnvironmentVariable(name, value);
}
public void Dispose() => Environment.SetEnvironmentVariable(_name, _previous);
}
}