fix(drivers): stop silently discarding driver config edits (§8.3, #516)
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<ApplyResult> 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).
This commit is contained in:
+17
-7
@@ -78,10 +78,17 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas
|
||||
factory.LastSubscribedRefs.ShouldNotBeNull()!.ShouldContain(SpeedRef);
|
||||
}
|
||||
|
||||
/// <summary>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 <see cref="SettleWindow"/>, so the absence asserted above is real.</summary>
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="SettleWindow"/> — without this, the absence asserted above would pass on a race.
|
||||
/// <para><b>The observable changed with #516.</b> Dropping a tag changes the driver's
|
||||
/// <c>DriverConfig</c>, which used to be an in-place delta: the child received an empty desired set
|
||||
/// and called <c>UnsubscribeAsync</c>. A config change is now a stop + respawn, so the old child is
|
||||
/// STOPPED (taking its subscription with it via <c>ShutdownAsync</c>) 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.</para>
|
||||
/// </summary>
|
||||
[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<ApplyAck>(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
|
||||
}
|
||||
|
||||
/// <summary>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
|
||||
/// <summary>Number of <c>UnsubscribeAsync</c> calls — non-zero means a live handle was torn down.</summary>
|
||||
public int UnsubscribeCount => Volatile.Read(ref _driver.UnsubscribeCount);
|
||||
|
||||
/// <summary>Number of <c>ShutdownAsync</c> calls — non-zero means a driver child was stopped.</summary>
|
||||
public int ShutdownCount => Volatile.Read(ref _driver.ShutdownCount);
|
||||
|
||||
/// <inheritdoc />
|
||||
public IDriver? TryCreate(string driverType, string driverInstanceId, string driverConfigJson) =>
|
||||
string.Equals(driverType, supportedType, StringComparison.Ordinal) ? _driver : null;
|
||||
|
||||
@@ -40,9 +40,18 @@ public sealed class DriverSpawnPlannerTests
|
||||
plan.ToStop.ShouldBeEmpty();
|
||||
}
|
||||
|
||||
/// <summary>Verifies that different configuration is routed to ApplyDelta.</summary>
|
||||
/// <summary>
|
||||
/// A changed <c>DriverConfig</c> forces a stop + respawn (#516), NOT an in-place delta.
|
||||
/// <para>This test previously asserted the opposite. The in-place path silently discarded the edit
|
||||
/// on five drivers whose <c>InitializeAsync</c> served constructor-captured options and never read
|
||||
/// the <c>driverConfigJson</c> 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.</para>
|
||||
/// <para>The accepted cost is a reconnect on every config edit.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Different_config_routes_to_ApplyDelta()
|
||||
public void Different_config_routes_to_stop_and_respawn()
|
||||
{
|
||||
var current = new Dictionary<string, DriverChildSnapshot>
|
||||
{
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
/// <summary>Verifies that removed drivers are routed to ToStop.</summary>
|
||||
@@ -166,11 +177,14 @@ public sealed class DriverSpawnPlannerTests
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// <para>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.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void DriverConfig_change_with_unchanged_ResilienceConfig_stays_ApplyDelta()
|
||||
public void DriverConfig_change_respawns_even_when_ResilienceConfig_is_unchanged()
|
||||
{
|
||||
var current = new Dictionary<string, DriverChildSnapshot>
|
||||
{
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,9 +50,17 @@ internal class StubDriver : IDriver
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
|
||||
/// <summary>Number of <see cref="ShutdownAsync"/> 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).</summary>
|
||||
public int ShutdownCount;
|
||||
|
||||
/// <summary>Shuts down the driver.</summary>
|
||||
/// <param name="cancellationToken">Cancellation token for the operation.</param>
|
||||
public Task ShutdownAsync(CancellationToken cancellationToken) => Task.CompletedTask;
|
||||
public Task ShutdownAsync(CancellationToken cancellationToken)
|
||||
{
|
||||
Interlocked.Increment(ref ShutdownCount);
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
/// <summary>Gets the health status of the driver.</summary>
|
||||
public DriverHealth GetHealth() => new(DriverState.Healthy, DateTime.UtcNow, null);
|
||||
/// <summary>Gets the memory footprint of the driver.</summary>
|
||||
|
||||
Reference in New Issue
Block a user