fix(drivers): fail the apply when the artifact could not be read (#486)

ApplyAndAck advanced _currentRevision and recorded NodeDeploymentState=Applied immediately after ReconcileDrivers, which returns null when the artifact could not be read. So a node that applied NOTHING reported success, claimed a revision whose configuration it never applied, and — because HandleDispatchFromSteady short-circuits on a revision match — could never be healed by re-dispatching that revision. Only a later, different revision recovered it. Now a null blob fails the apply: the revision is left where it was, NodeDeploymentState records Failed with the reason, and the coordinator gets a Failed ACK. Leaving the revision alone is what makes the retry actually land instead of being waved through. This is about telling the truth, not about tearing anything down — the node keeps serving its last-known-good address space, drivers and subscriptions throughout (#485). Tests, RED-first (both verified failing with "should be Failed but was Applied"): the ACK/revision assertion, plus the one that matters — after a failed apply, re-dispatching the SAME revision now genuinely applies. Its recovered artifact adds a second driver precisely so a short-circuited ACK (which has no side effects) cannot satisfy it. Four existing tests changed, and both changes are deliberate: - DriverHostActorTests.SeedDeployment never set ArtifactBlob at all. Its three consumers are about the apply/ACK/state machine, not the artifact, and now need a genuine apply — so the helper seeds a well-formed artifact declaring no drivers. That is what the composer emits for an empty configuration, and is precisely what "bytes we could not read" is NOT. - EmptyArtifact_IsNotCached asserted "the apply still succeeds — an empty artifact is a legitimate no-op deployment". That premise is the bug. Flipped to Failed; the test's actual subject (nothing is cached) is untouched and now holds for two independent reasons. Closes #486 Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
2026-07-21 03:22:40 -04:00
parent 5627d325eb
commit ef41a43102
4 changed files with 140 additions and 3 deletions
@@ -1579,6 +1579,29 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers
        try
        {
            var appliedBlob = ReconcileDrivers(deploymentId);
+
+            // Issue #486 — a null blob means the artifact could not be read (unreachable ConfigDb, or the
+            // empty bytes a missing row yields), so NOTHING was applied. Reporting Applied here used to
+            // advance _currentRevision too, and HandleDispatchFromSteady short-circuits on a revision
+            // match — so the node claimed a configuration it never applied AND could never be healed by
+            // re-dispatching that revision. Failing the apply keeps the revision where it was, which is
+            // what makes the retry land instead of being waved through. The node keeps serving its
+            // last-known-good address space, drivers and subscriptions throughout (issue #485) — this is
+            // about telling the truth, not about tearing anything down.
+            if (appliedBlob is null)
+            {
+                const string reason =
+                    "the deployment artifact could not be read (ConfigDb unreachable, or the row is missing/empty); nothing was applied";
+                UpsertNodeDeploymentState(deploymentId, NodeDeploymentStatus.Failed, reason);
+                SendAck(deploymentId, ApplyAckOutcome.Failed, reason, correlation);
+                OtOpcUaTelemetry.DeploymentApplied.Add(1, new KeyValuePair<string, object?>("outcome", "reject"));
+                span?.SetStatus(ActivityStatusCode.Error, reason);
+                _log.Error(
+                    "DriverHost {Node}: apply of {Id} FAILED — {Reason}. Staying on revision {Rev} and continuing to serve it; re-dispatch once the ConfigDb is readable",
+                    _localNode, deploymentId, reason, _currentRevision);
+                return;
+            }
+
            _currentRevision = revision;
            UpsertNodeDeploymentState(deploymentId, NodeDeploymentStatus.Applied, failureReason: null);
            SendAck(deploymentId, ApplyAckOutcome.Applied, failureReason: null, correlation);
@@ -129,10 +129,12 @@ public sealed class DriverHostActorArtifactCacheTests : RuntimeActorTestBase

        actor.Tell(new DispatchDeployment(deploymentId, RevA, CorrelationId.NewId()));

-        // The apply still succeeds — an empty artifact is a legitimate no-op deployment.
-        coordinator.ExpectMsg<ApplyAck>(TimeSpan.FromSeconds(5)).Outcome.ShouldBe(ApplyAckOutcome.Applied);
+        // Since #486 the apply FAILS rather than reporting a success it did not achieve: empty bytes are
+        // what an unreadable artifact looks like, not a legitimate no-op deployment. (A configuration that
+        // genuinely declares nothing still serialises to a JSON document with empty arrays.)
+        coordinator.ExpectMsg<ApplyAck>(TimeSpan.FromSeconds(5)).Outcome.ShouldBe(ApplyAckOutcome.Failed);

-        // But nothing is cached from it.
+        // The point of this test is unchanged, and now holds for two independent reasons: nothing is cached.
        Thread.Sleep(500);
        cache.Stores.ShouldBeEmpty();
    }
@@ -262,8 +262,25 @@ public sealed class DriverHostActorTests : RuntimeActorTestBase
            Status = status,
            CreatedBy = "test",
            SealedAtUtc = status == DeploymentStatus.Sealed ? DateTime.UtcNow : null,
+            // A REAL (if empty-of-content) artifact. These tests are about the apply/ACK/state machine,
+            // not about the artifact, and used to omit the blob entirely — but since #486 an unreadable
+            // artifact (which a missing blob is indistinguishable from) correctly FAILS the apply, so an
+            // artifact-less row would no longer exercise the success path they are asserting on. A
+            // configuration that declares no drivers is the genuine "nothing to run" deployment.
+            ArtifactBlob = EmptyButRealArtifact,
        });
        ctx.SaveChanges();
        return id;
    }
+
+    /// <summary>A well-formed artifact declaring no drivers — what the composer emits for a configuration
+    /// with nothing in it, as distinct from bytes that could not be read.</summary>
+    private static readonly byte[] EmptyButRealArtifact = JsonSerializer.SerializeToUtf8Bytes(new
+    {
+        RawFolders = Array.Empty<object>(),
+        DriverInstances = Array.Empty<object>(),
+        Devices = Array.Empty<object>(),
+        TagGroups = Array.Empty<object>(),
+        Tags = Array.Empty<object>(),
+    });
 }
@@ -120,6 +120,57 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas
            duration: Timeout);
    }

+    /// <summary>
+    ///     Issue #486 — an apply that read no artifact applied nothing, so it must NOT be reported as
+    ///     Applied. Reporting success also advanced <c>_currentRevision</c>, and because
+    ///     <c>HandleDispatchFromSteady</c> short-circuits on a revision match, the node could never be
+    ///     healed by re-dispatching that revision.
+    /// </summary>
+    [Fact]
+    public void Dispatch_whose_artifact_reads_back_empty_acks_Failed_and_leaves_the_revision_alone()
+    {
+        var db = NewInMemoryDbFactory();
+        var factory = new SubscribingDriverFactory("Modbus");
+        var (actor, coordinator) = SpawnHostAndApply(db, SeedV3Deployment(db, RevA), factory);
+        AskDiagnostics(actor).CurrentRevision.ShouldBe(RevA);
+
+        actor.Tell(new DispatchDeployment(SeedUnreadableDeployment(db, RevB), RevB, CorrelationId.NewId()));
+        var ack = coordinator.ExpectMsg<ApplyAck>(Timeout);
+
+        ack.Outcome.ShouldBe(ApplyAckOutcome.Failed);
+        ack.FailureReason.ShouldNotBeNullOrWhiteSpace();
+
+        var snapshot = AskDiagnostics(actor);
+        snapshot.CurrentRevision.ShouldBe(RevA);                     // never applied ⇒ never claimed
+        snapshot.Drivers.Select(d => d.Name).ShouldContain("drv-1"); // …and #485 still holds
+    }
+
+    /// <summary>Issue #486 — the point of failing the apply: the SAME revision can be dispatched again and
+    /// now actually applies, instead of being waved through by the "already at this rev" short-circuit. The
+    /// recovered artifact adds a second driver, so a short-circuited ACK (which has no side effects) cannot
+    /// satisfy this test — drv-2 only appears if the artifact was genuinely re-read and reconciled.</summary>
+    [Fact]
+    public void A_failed_apply_is_retried_not_short_circuited_once_the_artifact_is_readable_again()
+    {
+        var db = NewInMemoryDbFactory();
+        var factory = new SubscribingDriverFactory("Modbus");
+        var (actor, coordinator) = SpawnHostAndApply(db, SeedV3Deployment(db, RevA), factory);
+
+        var deploymentId = SeedUnreadableDeployment(db, RevB);
+        actor.Tell(new DispatchDeployment(deploymentId, RevB, CorrelationId.NewId()));
+        coordinator.ExpectMsg<ApplyAck>(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Failed);
+
+        // The ConfigDb recovers: the row now carries a real artifact, under the SAME revision.
+        SetArtifact(db, deploymentId, TwoDriverArtifact());
+        actor.Tell(new DispatchDeployment(deploymentId, RevB, CorrelationId.NewId()));
+
+        coordinator.ExpectMsg<ApplyAck>(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Applied);
+        AwaitAssert(
+            () => AskDiagnostics(actor).Drivers.Select(d => d.Name).ShouldContain("drv-2"),
+            duration: Timeout);
+        AskDiagnostics(actor).CurrentRevision.ShouldBe(RevB);
+    }
+
    /// <summary>Spawns the host with the subscribing factory, dispatches <paramref name="deploymentId"/> and
    /// waits for the Applied ACK so the child + the initial SubscribeBulk pass are in place.</summary>
    private (IActorRef Actor, Akka.TestKit.TestProbe Coordinator) SpawnHostAndApply(
@@ -161,6 +212,50 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas
            },
        }));

+    /// <summary>A readable artifact carrying drv-1 AND drv-2, so an apply that genuinely happens is
+    /// distinguishable from a short-circuited ACK (which spawns nothing).</summary>
+    private static byte[] TwoDriverArtifact() => JsonSerializer.SerializeToUtf8Bytes(new
+    {
+        RawFolders = new[] { new { RawFolderId = "rf-plant", ParentRawFolderId = (string?)null, Name = "Plant", ClusterId = "c1" } },
+        DriverInstances = new[]
+        {
+            new { DriverInstanceId = "drv-1", RawFolderId = "rf-plant", Name = "Modbus", DriverType = "Modbus", DriverConfig = "{}", ClusterId = "c1", Enabled = true },
+            new { DriverInstanceId = "drv-2", RawFolderId = "rf-plant", Name = "Modbus2", DriverType = "Modbus", DriverConfig = "{}", ClusterId = "c1", Enabled = true },
+        },
+        Devices = new[] { new { DeviceId = "dev-1", DriverInstanceId = "drv-1", Name = "dev1", DeviceConfig = "{}" } },
+        TagGroups = Array.Empty<object>(),
+        Tags = new[]
+        {
+            new { TagId = "t-speed", DeviceId = "dev-1", TagGroupId = (string?)null, Name = "speed", DataType = "Double", AccessLevel = 1, TagConfig = "{}" },
+        },
+    });
+
+    /// <summary>Replaces a seeded deployment's artifact in place — the ConfigDb becoming readable again
+    /// without the revision changing.</summary>
+    private static void SetArtifact(
+        IDbContextFactory<OtOpcUaConfigDbContext> db, DeploymentId deploymentId, byte[] artifact)
+    {
+        // ArtifactBlob is init-only, so the row is replaced rather than mutated: delete and re-add under
+        // the SAME id + revision. Separate SaveChanges calls so the in-memory provider does not see a
+        // delete and an insert of the same key in one change set.
+        using var ctx = db.CreateDbContext();
+        var row = ctx.Deployments.Single(d => d.DeploymentId == deploymentId.Value);
+        var rev = row.RevisionHash;
+        ctx.Deployments.Remove(row);
+        ctx.SaveChanges();
+
+        ctx.Deployments.Add(new Deployment
+        {
+            DeploymentId = deploymentId.Value,
+            RevisionHash = rev,
+            Status = DeploymentStatus.Sealed,
+            CreatedBy = "test",
+            SealedAtUtc = DateTime.UtcNow,
+            ArtifactBlob = artifact,
+        });
+        ctx.SaveChanges();
+    }
+
    /// <summary>Seeds a Sealed deployment whose <c>ArtifactBlob</c> is zero-length — byte-for-byte what the
    /// host's <c>?? Array.Empty&lt;byte&gt;()</c> hands downstream when the row cannot be found.</summary>
    private static DeploymentId SeedUnreadableDeployment(IDbContextFactory<OtOpcUaConfigDbContext> db, RevisionHash rev) =>