From d32d89c340cfd8d4a57a5ce77df85f2bc983231f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 27 Jul 2026 19:32:46 -0400 Subject: [PATCH] =?UTF-8?q?fix(drivers):=20stop=20silently=20discarding=20?= =?UTF-8?q?driver=20config=20edits=20(=C2=A78.3,=20#516)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five drivers ignored the config handed to them on reinitialize, serving the options their constructor captured — and the deployment sealed green anyway. Fixed on both halves, as chosen. Seam (load-bearing): DriverSpawnPlanner routes a changed DriverConfig to ToStop + ToSpawn instead of an in-place delta, making the factory the single parse authority. This REVERSES the deliberate decision documented at DriverSpawnPlan.cs:49-50 ("a pure DriverConfig change stays an in-place delta — no reconnect"). That reasoning was correct about the resilience pipeline and wrong about the driver. The accepted price is a reconnect per config edit. Per-driver, and this split was not anticipated: - Re-parse in place — Modbus, AbLegacy, OpcUaClient. ParseOptions extracted from each factory, called from InitializeAsync behind a HasConfigBody guard so "{}" still keeps the constructor options. - Respawn-only, deliberately NOT re-parsed — Sql and FOCAS. Each builds more than options from config (Sql's dialect + resolved connection string, FOCAS's client-factory backend, both injected at construction), so adopting new options alone would run a NEW tag set against an OLD connection. Half a re-parse is worse than none. SqlDriver's doc-comment asserted the opposite premise — "config parsing belongs to the factory, which builds a fresh instance" — which was false when written and is true only now. Two green seals removed from ApplyChildDelta: it overwrote the cached Spec synchronously BEFORE the child dequeued the message, so the host believed the new config was live and the next reconcile computed no delta, sealing the drift permanently; and it Tell'd with no Receive registered, so a failed reinit — including Galaxy's deliberate NotSupportedException — dead-lettered. Exposed (not created) by the change: a factory throw is a CONFIG error, and SpawnChild catches it and silently substitutes a stub. Only a brand-new driver could reach that before; an ordinary config edit can now. Raised Warning -> Error with an actionable message. It still does not fail the deployment — doing so would let one malformed driver block a fleet deploy, so that is a follow-up rather than a drive-by. Tests: every pre-existing reinit test in these suites passes "{}", the exact input a guarded re-parser treats as "keep constructor options" — blind to this defect by construction. New tests pass CHANGED json; the Modbus one was verified falsifiable by deleting the re-parse (goes red). Two tests asserted the old behaviour and were rewritten, including the positive control in DriverHostActorUnreadableArtifactTests, whose observable moved from UnsubscribeAsync to ShutdownAsync now that teardown happens by stopping the child rather than emptying its desired set. Remaining full-suite failures are pre-existing and environmental, verified identical on the pre-change tree: Host.IntegrationTests (3) and Driver.AbLegacy.IntegrationTests (4, docker fixture-gated). --- deferment.md | 57 ++++++++++++++- .../AbLegacyDriver.cs | 20 +++++- .../AbLegacyDriverFactoryExtensions.cs | 22 ++++-- .../FocasDriver.cs | 14 +++- .../ModbusDriver.cs | 21 +++++- .../ModbusDriverFactoryExtensions.cs | 22 ++++-- .../OpcUaClientDriver.cs | 21 +++++- .../OpcUaClientDriverFactoryExtensions.cs | 25 +++++-- .../ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs | 12 +++- .../Drivers/DriverHostActor.cs | 63 ++++++++++++++-- .../Drivers/DriverSpawnPlan.cs | 39 ++++++---- .../AbLegacyReinitConfigAdoptionTests.cs | 49 +++++++++++++ .../ModbusReinitConfigAdoptionTests.cs | 72 +++++++++++++++++++ .../OpcUaClientReinitConfigAdoptionTests.cs | 50 +++++++++++++ .../DriverHostActorUnreadableArtifactTests.cs | 24 +++++-- .../Drivers/DriverSpawnPlannerTests.cs | 36 +++++++--- .../Drivers/StubDrivers.cs | 10 ++- 17 files changed, 500 insertions(+), 57 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReinitConfigAdoptionTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusReinitConfigAdoptionTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientReinitConfigAdoptionTests.cs diff --git a/deferment.md b/deferment.md index b2801ebc..4c29ad33 100644 --- a/deferment.md +++ b/deferment.md @@ -77,7 +77,10 @@ Every gap below is in the **authoring/dispatch layer, not driver runtime code**. - `TagConfigEditorMap.cs:25-28` + `TagConfigValidator.cs:27-28` — "`DriverTypeNames.Sql` deliberately absent until Task 11"; the constant exists (`DriverTypeNames.cs:57`) and the values are identical, so behaviour is correct. - `RawDriverTypeDialog.razor:2-4, 59-60` — "its factory lands in Wave C". - `CsvColumnMap.cs:322-324` — hardcodes `CalculationDriverType = "Calculation"` "Not yet a `DriverTypeNames` constant"; it has been one since `DriverTypeNames.cs:54`. -- `SqlDriver.cs:218-220` — justifies not re-parsing config on the premise "the factory builds a fresh instance", **disproved** by `DriverInstanceActor.cs:316` + `DriverHostActor.cs:2565`. Should be corrected alongside #516. +- ~~`SqlDriver.cs:218-220` — justifies not re-parsing config on the premise "the factory builds a fresh + instance", **disproved** by `DriverInstanceActor.cs:316` + `DriverHostActor.cs:2565`.~~ ✅ **Fixed** — + the comment now says the premise was false when written and is true *now* because a config change + respawns. See §9. --- @@ -350,7 +353,8 @@ appear in §7.1 as well. 2. ~~**#518 + #507 together** — neither fix is observable alone.~~ ✅ **Done.** The framing was right that they had to be handled together, but wrong about the resolution: **#507 was not revivable**, so it was deleted rather than fixed. See §9. -3. **#516** — silent config discard on 5 drivers; then re-scope #489, which it partly subsumes. +3. ~~**#516** — silent config discard on 5 drivers; then re-scope #489, which it partly subsumes.~~ + ✅ **Done.** See §9. #489 is still open and should now be re-scoped. 4. **G-4** — extend the existing picker-parity test to `DriverConfigModal` + `DeviceModal`; it would have caught G-1 and G-2 for free, and this class has now recurred twice. 5. **Bookkeeping sweep** — mark the 8 stale `.tasks.json` files complete so the 19-task AdminUI plan @@ -369,7 +373,7 @@ the work, so this register never disagrees with the tree. |---|---|---| | 1. ACL enforcement decision | ✅ **Done** — decided *make non-enforcement explicit*; Gitea **#520** tracks the wire-up | `d1e88dc4` | | 2. #518 + #507 | ✅ **Done** — `IRediscoverable` consumed as a re-browse prompt; #507's injection path **deleted**; `IHostConnectivityProbe` half split to Gitea **#521** | `09a401b8`, `97c9f4b4` | -| 3. #516 config discard | ⏳ Not started — decided *both* (per-driver re-parse **and** seam respawn) | — | +| 3. #516 config discard | ✅ **Done** — seam respawn + re-parse on 3 drivers; Sql/FOCAS deliberately respawn-only | `2dc19f30` | | 4. G-4 dispatch-map parity | ⏳ Not started | — | | 5. Bookkeeping sweep | ⏳ Not started | — | | 6. Tier truth | ⏳ Not started — decided *document the truth, defer the code decision* | — | @@ -461,6 +465,53 @@ Two traps worth keeping: `DriverHostStatus` table was re-created deliberately in the v3 initial migration, so deleting on inference would be wrong. Split to Gitea **#521** with both options costed. +### 2026-07-27 — §8.3 #516 driver config edits silently discarded + +Decision: **both** — per-driver re-parse *and* the seam respawn. Reading the drivers split the +"per-driver" half in two, which the register did not anticipate: + +- **Re-parse in place** (`Modbus`, `AbLegacy`, `OpcUaClient`) — `ParseOptions` extracted from each + factory, called from `InitializeAsync` behind a `HasConfigBody` guard so `"{}"` still keeps the + constructor options. +- **Respawn-only, deliberately NOT re-parsed** (`Sql`, `FOCAS`) — each builds **more than options** + from config: Sql's `ISqlDialect` + resolved connection string, FOCAS's client-factory backend, both + injected at construction. Adopting new options alone would run a **new tag set against an old + connection**. Half a re-parse is worse than none. Their doc-comments now say so. + +**The seam respawn is the load-bearing half.** `DriverSpawnPlanner` now routes a changed +`DriverConfig` to `ToStop` + `ToSpawn`, making the factory the single parse authority. +`ToApplyDelta` is consequently always empty from the reconcile path. This **reverses the deliberate +decision documented at `DriverSpawnPlan.cs:49-50`** ("a pure DriverConfig change stays an in-place +delta — no reconnect"). That reasoning was right about resilience and wrong about the driver; the +accepted price is a reconnect on every config edit. + +Two seals removed from `ApplyChildDelta`: it overwrote the cached `Spec` **synchronously, before the +child had dequeued the message**, so the host immediately believed the new config was live and the +next reconcile computed no delta — sealing the drift permanently; and it `Tell`d with no +`Receive` registered, so a failed reinit (including Galaxy's deliberate +`NotSupportedException`) dead-lettered. + +**A new visibility gap this change exposed, not created:** a factory throw is a *config* error +(`TryCreate` is pure parsing; device I/O happens later), and `SpawnChild` catches it and silently +substitutes a stub. Previously only a brand-new driver could hit it; now an ordinary config edit can. +Raised from `Warning` to `Error` with an actionable message. It still does **not** fail the +deployment — making it do so would let one malformed driver block a fleet deploy, so that is a +deliberate follow-up rather than a drive-by change. + +**Tests.** Every pre-existing reinit test in the five suites passes `"{}"` — precisely the input a +guarded re-parser treats as "keep the constructor options", so they were blind to this defect *by +construction*. The new tests pass **changed** JSON. The Modbus one was verified falsifiable by +deleting the re-parse line (goes red). Two existing tests asserted the old behaviour and were +rewritten: `DriverSpawnPlannerTests` (two cases), and +`DriverHostActorUnreadableArtifactTests.Dropping_a_drivers_last_tag_does_clear_its_subscription` — +a **positive control** for a sibling absence assertion, whose observable moved from `UnsubscribeAsync` +to `ShutdownAsync` because the teardown now happens by stopping the child rather than emptying its +desired set. Same event, different route; the control still calibrates the settle window. + +Full-solution run: only pre-existing environment failures remain — `Host.IntegrationTests` (3, +verified identical on the pre-change tree) and `Driver.AbLegacy.IntegrationTests` (4, docker +fixture-gated, and notably these **fail rather than skip**, unlike every other fixture-gated suite). + --- *Generated 2026-07-27 against `master` @ `90bdaa44` by five parallel source-verification agents. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 94add6bd..faadaafc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -13,7 +13,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscovery, ISubscribable, IHostConnectivityProbe, IPerCallHostResolver, IDisposable, IAsyncDisposable { - private readonly AbLegacyDriverOptions _options; + /// Mutable so can adopt a re-parsed config on reinitialize + /// (#516). Only ever written on the init path, before any reader/session uses it. + private AbLegacyDriverOptions _options; private readonly string _driverInstanceId; private readonly IAbLegacyTagFactory _tagFactory; private readonly ILogger _logger; @@ -94,12 +96,28 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover /// public string DriverType => "AbLegacy"; + /// True when the supplied DriverConfig JSON carries a real body. The bootstrapper always + /// passes a populated document; some unit tests pass "{}" or an empty string to exercise + /// lifecycle shape without a config — those keep the constructor-supplied options. + private static bool HasConfigBody(string? driverConfigJson) + { + if (string.IsNullOrWhiteSpace(driverConfigJson)) return false; + var trimmed = driverConfigJson.Trim(); + return trimmed is not "{}" and not "[]"; + } + /// public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { _health = new DriverHealth(DriverState.Initializing, null, null); try { + // #516: re-parse the supplied DriverConfig so a config change delivered through the IDriver + // contract is honoured. Without this the driver keeps serving the options it was CONSTRUCTED + // with, so an operator's edit is discarded while the deployment still seals green. + if (HasConfigBody(driverConfigJson)) + _options = AbLegacyDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); + foreach (var device in _options.Devices) { var addr = AbLegacyHostAddress.TryParse(device.HostAddress) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs index fdd3dd57..cf55b578 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriverFactoryExtensions.cs @@ -49,6 +49,23 @@ public static class AbLegacyDriverFactoryExtensions /// Optional logger factory for the driver instance. /// A configured instance. internal static AbLegacyDriver CreateInstance(string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory) + { + return new AbLegacyDriver( + ParseOptions(driverInstanceId, driverConfigJson), driverInstanceId, + tagFactory: null, + logger: loggerFactory?.CreateLogger()); + } + + /// + /// Parses an AB Legacy DriverConfig JSON document into typed options. Extracted so + /// can re-parse a CHANGED config on reinitialize + /// (Gitea #516) instead of serving the options it was constructed with — which silently discarded + /// every operator edit while the deployment still sealed green. + /// + /// The unique driver instance identifier. + /// The driver configuration as a JSON string. + /// The parsed . + internal static AbLegacyDriverOptions ParseOptions(string driverInstanceId, string driverConfigJson) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); @@ -82,10 +99,7 @@ public static class AbLegacyDriverFactoryExtensions Timeout = PositiveTimeoutOrDefault(dto.TimeoutMs, 2_000), }; - return new AbLegacyDriver( - options, driverInstanceId, - tagFactory: null, - logger: loggerFactory?.CreateLogger()); + return options; } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 1c3b654c..0d834c59 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -100,7 +100,19 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// public string DriverType => "FOCAS"; - /// + /// + /// Opens the configured CNC handles and builds the authored tag table. + /// is deliberately not re-parsed here, unlike + /// Modbus / AbLegacy / OpcUaClient (Gitea #516). This driver builds more than options from config — + /// its IFocasClientFactory is selected from the Backend key and injected at + /// construction — so adopting a new FocasDriverOptions alone would run a NEW device/tag set + /// against the OLD backend. Half a re-parse is worse than none. + /// A config change is covered by the stop + respawn in DriverSpawnPlanner, which + /// rebuilds options and backend together through the factory. + /// + /// The driver configuration JSON (see remarks — not re-parsed). + /// Cancellation token for the operation. + /// A completed task. public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null)); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 12854f7c..3d972795 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -23,7 +23,9 @@ public sealed class ModbusDriver { // ---- instance fields (grouped at top for auditability) ---- - private readonly ModbusDriverOptions _options; + /// Mutable so can adopt a re-parsed config on reinitialize + /// (#516). Only ever written on the init path, before any reader/transport uses it. + private ModbusDriverOptions _options; private readonly Func _transportFactory; private readonly string _driverInstanceId; private readonly ILogger _logger; @@ -188,12 +190,29 @@ public sealed class ModbusDriver /// public string DriverType => "Modbus"; + /// True when the supplied DriverConfig JSON carries a real body. The bootstrapper always + /// passes a populated document; some unit tests pass "{}" or an empty string to exercise + /// lifecycle shape without a config — those keep the constructor-supplied options. + private static bool HasConfigBody(string? driverConfigJson) + { + if (string.IsNullOrWhiteSpace(driverConfigJson)) return false; + var trimmed = driverConfigJson.Trim(); + return trimmed is not "{}" and not "[]"; + } + /// public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { WriteHealth(new DriverHealth(DriverState.Initializing, null, null)); try { + // #516: re-parse the supplied DriverConfig so a config change delivered through the IDriver + // contract is honoured. Without this the driver keeps serving the options it was CONSTRUCTED + // with, so an operator's edit is discarded while the deployment still seals green. An empty / + // placeholder document (the "{}" some unit tests pass) keeps the constructor-supplied options. + if (HasConfigBody(driverConfigJson)) + _options = ModbusDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); + _transport = _transportFactory(_options); await _transport.ConnectAsync(cancellationToken).ConfigureAwait(false); // Build the RawPath → definition table from the authored raw tags. Each entry's TagConfig diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs index 93fdb4a7..ba587ce6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs @@ -44,6 +44,23 @@ public static class ModbusDriverFactoryExtensions /// Optional logger factory for creating loggers per driver instance. /// The constructed instance. public static ModbusDriver CreateInstance(string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory) + { + return new ModbusDriver( + ParseOptions(driverInstanceId, driverConfigJson), driverInstanceId, + transportFactory: null, + logger: loggerFactory?.CreateLogger()); + } + + /// + /// Parses a Modbus DriverConfig JSON document into typed options. Extracted from + /// so + /// can re-parse a CHANGED config on reinitialize (Gitea #516) rather than serving the options it was + /// constructed with — which silently discarded every edit while the deployment still sealed green. + /// + /// The unique identifier for the driver instance. + /// The JSON configuration string for the driver. + /// The parsed . + internal static ModbusDriverOptions ParseOptions(string driverInstanceId, string driverConfigJson) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); @@ -106,10 +123,7 @@ public static class ModbusDriverFactoryExtensions }, }; - return new ModbusDriver( - options, driverInstanceId, - transportFactory: null, - logger: loggerFactory?.CreateLogger()); + return options; } private static T ParseEnum(string? raw, string? tagName, string driverInstanceId, string field) where T : struct, Enum diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index d9ac8555..26dece00 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -54,7 +54,9 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit _secretResolver = secretResolver ?? NullSecretResolver.Instance; } - private readonly OpcUaClientDriverOptions _options; + /// Mutable so can adopt a re-parsed config on reinitialize + /// (#516). Only ever written on the init path, before any reader/session uses it. + private OpcUaClientDriverOptions _options; private readonly ISecretResolver _secretResolver; private readonly string _driverInstanceId; // ---- IAlarmSource state ---- @@ -156,12 +158,29 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit /// public string DriverType => "OpcUaClient"; + /// True when the supplied DriverConfig JSON carries a real body. The bootstrapper always + /// passes a populated document; some unit tests pass "{}" or an empty string to exercise + /// lifecycle shape without a config — those keep the constructor-supplied options. + private static bool HasConfigBody(string? driverConfigJson) + { + if (string.IsNullOrWhiteSpace(driverConfigJson)) return false; + var trimmed = driverConfigJson.Trim(); + return trimmed is not "{}" and not "[]"; + } + /// public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { _health = new DriverHealth(DriverState.Initializing, null, null); try { + // #516: re-parse the supplied DriverConfig so a config change delivered through the IDriver + // contract is honoured. Without this the driver keeps serving the options it was CONSTRUCTED + // with — the endpoint, security policy and tag set were all frozen at construction, and only + // secret rotation was picked up. + if (HasConfigBody(driverConfigJson)) + _options = OpcUaClientDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); + // Enforce the Equipment-vs-SystemPlatform choice at startup per driver-specs.md // §8 "Namespace Assignment" — a misconfigured remote fails draft validation here, // not as a runtime surprise. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriverFactoryExtensions.cs index 9da3a22f..dfd53972 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriverFactoryExtensions.cs @@ -63,16 +63,31 @@ public static class OpcUaClientDriverFactoryExtensions public static OpcUaClientDriver CreateInstance( string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory = null, ISecretResolver? secretResolver = null) + { + return new OpcUaClientDriver( + ParseOptions(driverInstanceId, driverConfigJson), driverInstanceId, + loggerFactory?.CreateLogger(), + secretResolver ?? NullSecretResolver.Instance); + } + + /// + /// Parses an OpcUaClient DriverConfig JSON document into typed options. Extracted so + /// can re-parse a CHANGED config on reinitialize + /// (Gitea #516) instead of serving the options it was constructed with. Note the driver's own + /// doc-comment previously claimed "resolving on every InitializeAsync picks up rotations" — that was + /// true of SECRET rotation only; the endpoint, security policy and tag set were all frozen at + /// construction. + /// + /// The unique driver instance identifier. + /// The driver configuration as a JSON string. + /// The parsed . + internal static OpcUaClientDriverOptions ParseOptions(string driverInstanceId, string driverConfigJson) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); - var options = JsonSerializer.Deserialize(driverConfigJson, JsonOptions) + return JsonSerializer.Deserialize(driverConfigJson, JsonOptions) ?? throw new InvalidOperationException( $"OpcUaClient driver config for '{driverInstanceId}' deserialised to null"); - - return new OpcUaClientDriver( - options, driverInstanceId, loggerFactory?.CreateLogger(), - secretResolver ?? NullSecretResolver.Instance); } } 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 e99c3355..6af0b649 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -216,8 +216,16 @@ public sealed class SqlDriver /// /// Builds the authored RawPath table and proves the database is reachable. /// is not re-parsed here: the driver serves the - /// typed it was constructed with, exactly as ModbusDriver does - /// (config parsing belongs to the factory, which builds a fresh instance). + /// typed it was constructed with. That premise — "config parsing + /// belongs to the factory, which builds a fresh instance" — was false when written + /// (Gitea #516): the host reinitialized the EXISTING child in place, so every config edit was + /// silently discarded and the deployment still sealed green. It is true now, because + /// DriverSpawnPlanner routes a changed DriverConfig through a stop + respawn. + /// This driver deliberately does not also re-parse in place, unlike Modbus / AbLegacy / + /// OpcUaClient. It builds more than options from config — the and the + /// resolved connection string are both injected at construction — so adopting a new + /// alone would run a NEW tag set against the OLD connection. Half a + /// re-parse is worse than none; the respawn rebuilds all three together. /// 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 and rethrows — /// DriverInstanceActor reads a throw as InitializeFailed and lands in Reconnecting with its diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index cb2152ee..82d5267a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -244,6 +244,11 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers /// value maps so stale condition state never leaks across redeploys. private readonly NativeAlarmProjector _nativeAlarmProjector = new(); + /// In-flight sends, keyed by correlation, so + /// can advance the cached spec only once the child confirms it adopted + /// the config (#516). Bounded by the number of driver children with a delta in flight. + private readonly Dictionary _pendingDelta = new(); + /// The composition from the most-recent apply (set at the END of /// ). Null until the first apply. private AddressSpaceComposition? _lastComposition; @@ -868,6 +873,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers Receive(ForwardNativeAlarm); Receive(OnDriverConnectivityChanged); Receive(HandleDeltaApplied); + Receive(HandleApplyResult); Receive(HandleRestartDriver); Receive(HandleReconnectDriver); Receive(HandleRouteNodeWrite); @@ -901,6 +907,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers Receive(ForwardNativeAlarm); Receive(OnDriverConnectivityChanged); Receive(HandleDeltaApplied); + Receive(HandleApplyResult); Receive(HandleRestartDriver); Receive(HandleReconnectDriver); Receive(HandleRouteNodeWrite); @@ -1353,6 +1360,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers // A late DeltaApplied (an apply completed just before the DB went Stale) — re-register the driver's // mux adapter anyway; it simply re-reads the driver's current refs (harmless, no DB access). Receive(HandleDeltaApplied); + Receive(HandleApplyResult); Receive(_ => { /* PubSub ack */ }); Timers.StartPeriodicTimer("retry-db", RetryConfigDbConnection.Instance, ReconnectInterval); } @@ -2007,7 +2015,15 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers try { driver = _driverFactory.TryCreate(spec.DriverType, spec.DriverInstanceId, spec.DriverConfig); } catch (Exception ex) { - _log.Warning(ex, "DriverHost {Node}: factory for {Type} threw on {Id}; stubbing", + // A factory throw is a CONFIG error, not a connectivity one — TryCreate is pure parsing; + // device I/O happens later in InitializeAsync. Logged at Error because the node silently + // degrades to a stub that answers nothing, and since #516 routed DriverConfig changes + // through a respawn this path is now reachable by an ordinary operator edit rather than + // only by a brand-new driver. It still does NOT fail the deployment — making it do so is + // a deliberate follow-up, since it would let one malformed driver block a fleet deploy. + _log.Error(ex, + "DriverHost {Node}: factory for {Type} REJECTED the config for {Id} — the driver is " + + "stubbed and will serve nothing until the config is fixed and redeployed", _localNode, spec.DriverType, spec.DriverInstanceId); } if (driver is null) @@ -2087,15 +2103,54 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers Context.Stop(adapter); } + /// + /// Sends an in-place config delta to a running child. + /// Unreachable from the reconcile path since #516 — a DriverConfig change is now a + /// stop + respawn (), so ToApplyDelta is always empty. + /// Kept because the seam is still exercised by DriverInstanceActor's own mid-connect config + /// adoption. + /// Two seals were removed here. This used to overwrite the cached + /// Spec SYNCHRONOUSLY, before the child had even dequeued the message — so the host + /// immediately believed the new config was live, the NEXT reconcile computed no delta against it, + /// and any drift was sealed permanently. It also Telld with no Receive<ApplyResult> + /// handler registered, so a FAILED reinit — including Galaxy's deliberate + /// NotSupportedException — dead-lettered and was never surfaced. + /// private void ApplyChildDelta(DriverInstanceSpec spec) { if (!_children.TryGetValue(spec.DriverInstanceId, out var entry)) return; - entry.Actor.Tell(new DriverInstanceActor.ApplyDelta(spec.DriverConfig, CorrelationId.NewId())); - // Store the full new spec — a delta can change Name, Enabled, ClusterId, etc. in addition to config. - _children[spec.DriverInstanceId] = entry with { Spec = spec }; + var correlation = CorrelationId.NewId(); + _pendingDelta[correlation] = spec; + entry.Actor.Tell(new DriverInstanceActor.ApplyDelta(spec.DriverConfig, correlation), Self); _log.Debug("DriverHost {Node}: ApplyDelta queued for {Id}", _localNode, spec.DriverInstanceId); } + /// + /// A child's reply to . On success the cached Spec is advanced — + /// only now, when the child has actually adopted the config, so a failed reinit leaves the host + /// believing the OLD config is live and the next reconcile re-attempts the change instead of + /// silently treating the drift as applied. A failure is logged at Error rather than swallowed. + /// + private void HandleApplyResult(DriverInstanceActor.ApplyResult msg) + { + if (msg.Success) + { + if (_pendingDelta.Remove(msg.Correlation, out var spec) + && _children.TryGetValue(spec.DriverInstanceId, out var entry)) + { + // A delta can change Name, Enabled, ClusterId etc. alongside the config. + _children[spec.DriverInstanceId] = entry with { Spec = spec }; + } + return; + } + + _pendingDelta.Remove(msg.Correlation, out var failed); + _log.Error( + "DriverHost {Node}: driver {Id} REJECTED an in-place config change ({Reason}) — the host keeps " + + "the previous config so the next reconcile re-attempts it", + _localNode, failed?.DriverInstanceId ?? "", msg.Reason ?? "no reason given"); + } + /// /// A driver child finished applying an in-place (its /// completed). Re-register THAT driver's dependency-consumer diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverSpawnPlan.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverSpawnPlan.cs index 5399691f..37ba98ab 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverSpawnPlan.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverSpawnPlan.cs @@ -7,7 +7,12 @@ namespace ZB.MOM.WW.OtOpcUa.Runtime.Drivers; /// spawn / ApplyDelta / stop on its child actors accordingly. /// /// Specs with no current child — create a new actor. -/// Specs whose child exists but config JSON or type differs. +/// +/// In-place config deltas. Always empty since #516 — a DriverConfig change is now a +/// stop + respawn so the factory is the single parse authority. Retained because +/// DriverInstanceActor still handles ApplyDelta on its own config-adoption path +/// (a config arriving mid-connect), and removing the list would hide that seam. +/// /// DriverInstanceIds currently running but missing from the new artifact, or now disabled. public sealed record DriverSpawnPlan( IReadOnlyList ToSpawn, @@ -42,23 +47,33 @@ public static class DriverSpawnPlanner toStop.Add(id); continue; } - // A driver TYPE change can't be reinitialized in-place (factory-bound) — stop + respawn. - // A RESILIENCE-CONFIG change likewise forces a respawn: the CapabilityInvoker (and its - // resolved options) is bound to the child actor at spawn time, so the only way a changed - // ResilienceConfig takes effect is to rebuild the child. The factory invalidates the stale - // cached pipelines on the respawn's Create call. (A pure DriverConfig change stays an - // in-place delta — no reconnect — because it doesn't touch the resilience pipeline.) + // ANY of the three config surfaces changing forces a stop + respawn, so the FACTORY is the + // single parse authority for everything a driver is built from. + // + // • DriverType — factory-bound, can't be reinitialized in place. + // • ResilienceConfig — the CapabilityInvoker (and its resolved options) binds to the child + // actor at spawn time; the factory invalidates the stale cached pipelines on respawn. + // • DriverConfig — was an in-place ApplyDelta until #516. It is now a respawn. + // + // #516: the in-place delta silently DISCARDED the edit on five drivers (Modbus, FOCAS, + // OpcUaClient, AbLegacy, Sql), whose InitializeAsync served the options captured by their + // constructor and never looked at the driverConfigJson they were handed. The deployment still + // sealed green. Those five now re-parse (belt), and this respawn is the braces: several + // drivers build MORE than options from config — Sql derives its dialect + connection string + // and FOCAS its client-factory backend, both injected at construction — so an in-place + // re-parse of options ALONE would apply a new tag set against an old connection. Only a + // respawn rebuilds all of it. + // + // The accepted cost is a reconnect on every DriverConfig edit, replacing the prior + // no-reconnect in-place path. That is deliberate: a correct reconnect beats a silent no-op. if (!string.Equals(snap.DriverType, spec.DriverType, StringComparison.Ordinal) - || !string.Equals(snap.ResilienceConfig, spec.ResilienceConfig, StringComparison.Ordinal)) + || !string.Equals(snap.ResilienceConfig, spec.ResilienceConfig, StringComparison.Ordinal) + || !string.Equals(snap.LastConfigJson, spec.DriverConfig, StringComparison.Ordinal)) { toStop.Add(id); toSpawn.Add(spec); continue; } - if (!string.Equals(snap.LastConfigJson, spec.DriverConfig, StringComparison.Ordinal)) - { - toDelta.Add(spec); - } } foreach (var (id, spec) in targetById) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReinitConfigAdoptionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReinitConfigAdoptionTests.cs new file mode 100644 index 00000000..4aaa3feb --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReinitConfigAdoptionTests.cs @@ -0,0 +1,49 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Gitea #516 — AbLegacyDriver.InitializeAsync used to serve the options its CONSTRUCTOR +/// captured and never look at the driverConfigJson it was handed, so an operator's config edit +/// was silently discarded while the deployment still sealed green. +/// Every pre-existing reinit test in this suite passes "{}" — the exact input the guarded +/// re-parser treats as "keep the constructor options" — so they were blind to the defect. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyReinitConfigAdoptionTests +{ + /// + /// The driver validates every device's HostAddress during init. Reinitializing with a config + /// whose device address is structurally invalid must therefore THROW — a driver still serving the + /// constructor's valid device list would validate that instead and succeed, which is precisely how + /// the discarded edit stayed invisible. + /// + [Fact] + public async Task Reinitialize_adopts_a_changed_device_list() + { + var driver = AbLegacyDriverFactoryExtensions.CreateInstance( + "ab1", + """{"devices":[{"hostAddress":"ab://10.0.0.1:44818/1,0","plcFamily":"Slc500"}]}""", + loggerFactory: null); + + await Should.ThrowAsync( + () => driver.ReinitializeAsync( + """{"devices":[{"hostAddress":"not-a-valid-ab-address","plcFamily":"Slc500"}]}""", + CancellationToken.None), + "AbLegacyDriver kept its constructor-supplied device list after a reinitialize with a changed config (#516)"); + } + + /// An empty/placeholder document must still keep the constructor options, so the many + /// lifecycle tests that pass "{}" keep meaning what they meant. + [Fact] + public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_options() + { + var driver = AbLegacyDriverFactoryExtensions.CreateInstance( + "ab1", + """{"devices":[{"hostAddress":"ab://10.0.0.1:44818/1,0","plcFamily":"Slc500"}]}""", + loggerFactory: null); + + await Should.NotThrowAsync(() => driver.ReinitializeAsync("{}", CancellationToken.None)); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusReinitConfigAdoptionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusReinitConfigAdoptionTests.cs new file mode 100644 index 00000000..98481200 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusReinitConfigAdoptionTests.cs @@ -0,0 +1,72 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// Gitea #516 — used to serve the options its CONSTRUCTOR +/// captured and never look at the driverConfigJson it was handed, so an operator's config edit +/// was silently discarded while the deployment still sealed green. +/// This test passes a CHANGED config on purpose. Every pre-existing reinit test in this +/// suite passes "{}" — exactly the input the guarded re-parser treats as "keep the constructor +/// options" — so those tests were blind to the defect by construction. +/// +[Trait("Category", "Unit")] +public sealed class ModbusReinitConfigAdoptionTests +{ + /// The transport factory receives the options the driver actually decided to use, so it is a + /// direct read of which config won — no log-string matching. + [Fact] + public async Task Reinitialize_with_a_changed_host_rebuilds_the_transport_against_the_new_host() + { + var seen = new List(); + var options = ModbusDriverFactoryExtensions.ParseOptions("m1", """{"host":"10.0.0.1","port":502}"""); + var driver = new ModbusDriver(options, "m1", opts => + { + seen.Add($"{opts.Host}:{opts.Port}"); + return new NeverConnectingTransport(); + }); + + try { await driver.InitializeAsync("""{"host":"10.0.0.1","port":502}""", CancellationToken.None); } + catch { /* connect failure is expected and irrelevant */ } + + try { await driver.ReinitializeAsync("""{"host":"10.0.0.99","port":1502}""", CancellationToken.None); } + catch { /* ditto */ } + + seen.Count.ShouldBeGreaterThanOrEqualTo(2); + seen[^1].ShouldBe("10.0.0.99:1502", + "ModbusDriver kept its constructor-supplied host after a reinitialize with a changed config (#516)"); + } + + /// A placeholder / empty document must still keep the constructor options — the guard exists so + /// the many lifecycle tests that pass "{}" keep meaning what they meant. + [Fact] + public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_options() + { + var seen = new List(); + var options = ModbusDriverFactoryExtensions.ParseOptions("m1", """{"host":"10.0.0.1","port":502}"""); + var driver = new ModbusDriver(options, "m1", opts => + { + seen.Add($"{opts.Host}:{opts.Port}"); + return new NeverConnectingTransport(); + }); + + try { await driver.ReinitializeAsync("{}", CancellationToken.None); } + catch { /* connect failure is expected */ } + + seen.ShouldAllBe(s => s == "10.0.0.1:502"); + } + + private sealed class NeverConnectingTransport : IModbusTransport + { + public bool IsConnected => false; + public Task ConnectAsync(CancellationToken cancellationToken) + => throw new IOException("test transport never connects"); + public Task DisconnectAsync(CancellationToken cancellationToken) => Task.CompletedTask; + public Task SendAsync(byte unitId, byte[] pdu, CancellationToken cancellationToken) + => throw new IOException("test transport never connects"); + public void Dispose() { } + public ValueTask DisposeAsync() => ValueTask.CompletedTask; + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientReinitConfigAdoptionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientReinitConfigAdoptionTests.cs new file mode 100644 index 00000000..5299fdb5 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientReinitConfigAdoptionTests.cs @@ -0,0 +1,50 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests; + +/// +/// Gitea #516 — OpcUaClientDriver.InitializeAsync used to serve the options its CONSTRUCTOR +/// captured. Its own comment claimed "resolving on every InitializeAsync … picks up rotations", which +/// was true of SECRET rotation only: the endpoint, the security policy and the tag set were all frozen +/// at construction, so an operator's edit was discarded while the deployment still sealed green. +/// +[Trait("Category", "Unit")] +public sealed class OpcUaClientReinitConfigAdoptionTests +{ + /// + /// ValidateNamespaceKind runs at the top of init and rejects TargetNamespaceKind=Equipment + /// with an empty UNS mapping table. Reinitializing INTO that shape must therefore throw — a driver + /// still serving the constructor's valid options would validate those and succeed, which is exactly + /// how the discarded edit stayed invisible. + /// + [Fact] + public async Task Reinitialize_adopts_a_changed_config() + { + var driver = OpcUaClientDriverFactoryExtensions.CreateInstance( + "opc1", + """{"endpointUrl":"opc.tcp://10.0.0.1:4840","targetNamespaceKind":"Equipment","unsMappingTable":{"a":"b"}}"""); + + await Should.ThrowAsync( + () => driver.ReinitializeAsync( + """{"endpointUrl":"opc.tcp://10.0.0.1:4840","targetNamespaceKind":"Equipment","unsMappingTable":{}}""", + CancellationToken.None), + "OpcUaClientDriver kept its constructor-supplied options after a reinitialize with a changed config (#516)"); + } + + /// An empty/placeholder document must still keep the constructor options. + [Fact] + public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_options() + { + var driver = OpcUaClientDriverFactoryExtensions.CreateInstance( + "opc1", + """{"endpointUrl":"opc.tcp://10.0.0.1:4840","targetNamespaceKind":"Equipment","unsMappingTable":{"a":"b"}}"""); + + // "{}" keeps the (valid) constructor options, so ValidateNamespaceKind passes and the driver gets as + // far as the connect — which fails against an unreachable endpoint. Anything BUT the validation + // throw proves the constructor options survived. + var ex = await Record.ExceptionAsync(() => driver.ReinitializeAsync("{}", CancellationToken.None)); + (ex is InvalidOperationException ioe && ioe.Message.Contains("UnsMappingTable", StringComparison.OrdinalIgnoreCase)) + .ShouldBeFalse("an empty document must keep the constructor-supplied options, not re-validate an empty config"); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorUnreadableArtifactTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorUnreadableArtifactTests.cs index 1036d4a3..a3f5f1bc 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorUnreadableArtifactTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorUnreadableArtifactTests.cs @@ -78,10 +78,17 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas factory.LastSubscribedRefs.ShouldNotBeNull()!.ShouldContain(SpeedRef); } - /// Positive control for the subscription half: a READABLE artifact that keeps the driver but - /// drops its last tag genuinely does clear the live subscription — the child receives an empty desired - /// set and unsubscribes. This proves both that the unsubscribe is observable through this factory and - /// that it lands well within , so the absence asserted above is real. + /// + /// Positive control for the subscription half: a READABLE artifact that drops the driver's last tag + /// genuinely does tear the live subscription down, and does so well within + /// — without this, the absence asserted above would pass on a race. + /// The observable changed with #516. Dropping a tag changes the driver's + /// DriverConfig, which used to be an in-place delta: the child received an empty desired set + /// and called UnsubscribeAsync. A config change is now a stop + respawn, so the old child is + /// STOPPED (taking its subscription with it via ShutdownAsync) and a fresh one spawns with + /// nothing to subscribe to. The teardown is therefore observed as a shutdown, not an unsubscribe — + /// the same event, reached by a different route. + /// [Fact] public void Dropping_a_drivers_last_tag_does_clear_its_subscription() { @@ -90,13 +97,13 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas var (actor, coordinator) = SpawnHostAndApply(db, SeedV3Deployment(db, RevA), factory); AwaitAssert(() => factory.LastSubscribedRefs.ShouldNotBeNull()!.ShouldContain(SpeedRef), duration: Timeout); - factory.UnsubscribeCount.ShouldBe(0); + factory.ShutdownCount.ShouldBe(0); actor.Tell(new DispatchDeployment(SeedTaglessDriverDeployment(db, RevB), RevB, CorrelationId.NewId())); coordinator.ExpectMsg(Timeout); - AwaitAssert(() => factory.UnsubscribeCount.ShouldBe(1), duration: SettleWindow); - AskDiagnostics(actor).Drivers.Select(d => d.Name).ShouldContain("drv-1"); // the driver itself stayed + AwaitAssert(() => factory.ShutdownCount.ShouldBeGreaterThanOrEqualTo(1), duration: SettleWindow); + AskDiagnostics(actor).Drivers.Select(d => d.Name).ShouldContain("drv-1"); // respawned under the same id } /// Positive control: the guard keys on "the read gave us nothing", not on "the new config has @@ -318,6 +325,9 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas /// Number of UnsubscribeAsync calls — non-zero means a live handle was torn down. public int UnsubscribeCount => Volatile.Read(ref _driver.UnsubscribeCount); + /// Number of ShutdownAsync calls — non-zero means a driver child was stopped. + public int ShutdownCount => Volatile.Read(ref _driver.ShutdownCount); + /// public IDriver? TryCreate(string driverType, string driverInstanceId, string driverConfigJson) => string.Equals(driverType, supportedType, StringComparison.Ordinal) ? _driver : null; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverSpawnPlannerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverSpawnPlannerTests.cs index f9e18d4a..541d5a1f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverSpawnPlannerTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverSpawnPlannerTests.cs @@ -40,9 +40,18 @@ public sealed class DriverSpawnPlannerTests plan.ToStop.ShouldBeEmpty(); } - /// Verifies that different configuration is routed to ApplyDelta. + /// + /// A changed DriverConfig forces a stop + respawn (#516), NOT an in-place delta. + /// This test previously asserted the opposite. The in-place path silently discarded the edit + /// on five drivers whose InitializeAsync served constructor-captured options and never read + /// the driverConfigJson they were handed — and the deployment still sealed green. Routing + /// through the factory makes it the single parse authority, which matters most for the drivers that + /// build MORE than options from config (Sql's dialect + connection string, FOCAS's client-factory + /// backend) where re-parsing options alone would apply a new tag set against an old connection. + /// The accepted cost is a reconnect on every config edit. + /// [Fact] - public void Different_config_routes_to_ApplyDelta() + public void Different_config_routes_to_stop_and_respawn() { var current = new Dictionary { @@ -52,9 +61,11 @@ public sealed class DriverSpawnPlannerTests var plan = DriverSpawnPlanner.Compute(current, target); - plan.ToApplyDelta.Single().DriverInstanceId.ShouldBe("a"); - plan.ToSpawn.ShouldBeEmpty(); - plan.ToStop.ShouldBeEmpty(); + plan.ToStop.Single().ShouldBe("a"); + plan.ToSpawn.Single().DriverInstanceId.ShouldBe("a"); + plan.ToSpawn.Single().DriverConfig.ShouldBe("{\"host\":\"new\"}"); + // The whole point: nothing is left on the in-place path that could discard the edit. + plan.ToApplyDelta.ShouldBeEmpty(); } /// Verifies that removed drivers are routed to ToStop. @@ -166,11 +177,14 @@ public sealed class DriverSpawnPlannerTests } /// - /// A pure DriverConfig change with an UNCHANGED ResilienceConfig stays an in-place delta (no - /// reconnect) — the resilience pipeline is untouched, so there's no reason to respawn. + /// A pure DriverConfig change respawns even when the ResilienceConfig is UNCHANGED. + /// This asserted the opposite until #516. The reasoning then was "the resilience pipeline is + /// untouched, so there's no reason to respawn" — correct about resilience, wrong about the driver: + /// five drivers ignored the config the in-place delta handed them, and the deployment sealed green + /// anyway. The reconnect is the accepted price of the edit actually taking effect. /// [Fact] - public void DriverConfig_change_with_unchanged_ResilienceConfig_stays_ApplyDelta() + public void DriverConfig_change_respawns_even_when_ResilienceConfig_is_unchanged() { var current = new Dictionary { @@ -180,8 +194,8 @@ public sealed class DriverSpawnPlannerTests var plan = DriverSpawnPlanner.Compute(current, target); - plan.ToApplyDelta.Single().DriverInstanceId.ShouldBe("a"); - plan.ToSpawn.ShouldBeEmpty(); - plan.ToStop.ShouldBeEmpty(); + plan.ToStop.Single().ShouldBe("a"); + plan.ToSpawn.Single().DriverInstanceId.ShouldBe("a"); + plan.ToApplyDelta.ShouldBeEmpty(); } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/StubDrivers.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/StubDrivers.cs index 2ab4213d..b9780be4 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/StubDrivers.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/StubDrivers.cs @@ -50,9 +50,17 @@ internal class StubDriver : IDriver return Task.CompletedTask; } + /// Number of calls — a child actor stopping raises it, so a test + /// can observe a driver child being torn down (e.g. by the #516 config-change respawn). + public int ShutdownCount; + /// Shuts down the driver. /// Cancellation token for the operation. - public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask; + public Task ShutdownAsync(CancellationToken cancellationToken) + { + Interlocked.Increment(ref ShutdownCount); + return Task.CompletedTask; + } /// Gets the health status of the driver. public DriverHealth GetHealth() => new(DriverState.Healthy, DateTime.UtcNow, null); /// Gets the memory footprint of the driver.