From ef41a43102a5316c4b0c3ad8d12f16c8b72723a7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 21 Jul 2026 03:22:40 -0400 Subject: [PATCH 1/2] fix(drivers): fail the apply when the artifact could not be read (#486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Drivers/DriverHostActor.cs | 23 +++++ .../DriverHostActorArtifactCacheTests.cs | 8 +- .../Drivers/DriverHostActorTests.cs | 17 ++++ .../DriverHostActorUnreadableArtifactTests.cs | 95 +++++++++++++++++++ 4 files changed, 140 insertions(+), 3 deletions(-) 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 4c382570..cc0c598f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -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("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); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorArtifactCacheTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorArtifactCacheTests.cs index 77f9972b..a24ac9cf 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorArtifactCacheTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorArtifactCacheTests.cs @@ -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(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(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(); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorTests.cs index 2429e610..8f192617 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorTests.cs @@ -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; } + + /// 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. + private static readonly byte[] EmptyButRealArtifact = JsonSerializer.SerializeToUtf8Bytes(new + { + RawFolders = Array.Empty(), + DriverInstances = Array.Empty(), + Devices = Array.Empty(), + TagGroups = Array.Empty(), + Tags = Array.Empty(), + }); } 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 5749b2fc..1036d4a3 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 @@ -120,6 +120,57 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas duration: Timeout); } + /// + /// Issue #486 — an apply that read no artifact applied nothing, so it must NOT be reported as + /// Applied. Reporting success also advanced _currentRevision, and because + /// HandleDispatchFromSteady short-circuits on a revision match, the node could never be + /// healed by re-dispatching that revision. + /// + [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(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 + } + + /// 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. + [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(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(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Applied); + AwaitAssert( + () => AskDiagnostics(actor).Drivers.Select(d => d.Name).ShouldContain("drv-2"), + duration: Timeout); + AskDiagnostics(actor).CurrentRevision.ShouldBe(RevB); + } + /// Spawns the host with the subscribing factory, dispatches and /// waits for the Applied ACK so the child + the initial SubscribeBulk pass are in place. private (IActorRef Actor, Akka.TestKit.TestProbe Coordinator) SpawnHostAndApply( @@ -161,6 +212,50 @@ public sealed class DriverHostActorUnreadableArtifactTests : RuntimeActorTestBas }, })); + /// 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). + 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(), + Tags = new[] + { + new { TagId = "t-speed", DeviceId = "dev-1", TagGroupId = (string?)null, Name = "speed", DataType = "Double", AccessLevel = 1, TagConfig = "{}" }, + }, + }); + + /// Replaces a seeded deployment's artifact in place — the ConfigDb becoming readable again + /// without the revision changing. + private static void SetArtifact( + IDbContextFactory 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(); + } + /// Seeds a Sealed deployment whose ArtifactBlob is zero-length — byte-for-byte what the /// host's ?? Array.Empty<byte>() hands downstream when the row cannot be found. private static DeploymentId SeedUnreadableDeployment(IDbContextFactory db, RevisionHash rev) => From 689fc06e853026d72ac781231ed02c55260abf83 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 21 Jul 2026 03:27:54 -0400 Subject: [PATCH 2/2] docs(486): live-gate the failed-apply fix on docker-dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression check first, since this changes ACK semantics: a normal deploy still records Applied on all six nodes. Then the same induction as #485 — the node now records Failed with a reason while the five nodes that read the real artifact record Applied (pre-fix all six claimed Applied), and the log shows it staying on the PREVIOUS revision rather than the dispatched one, which is what lets a retry land. Values kept flowing throughout (59603, current), and the next real deploy applied normally. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW --- docs/plans/2026-07-21-issue-485-live-gate.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/plans/2026-07-21-issue-485-live-gate.md b/docs/plans/2026-07-21-issue-485-live-gate.md index 34d98729..e766a389 100644 --- a/docs/plans/2026-07-21-issue-485-live-gate.md +++ b/docs/plans/2026-07-21-issue-485-live-gate.md @@ -61,3 +61,16 @@ not a data-plane one: the node keeps serving its last good configuration through ACK `Failed` and leave `_currentRevision` alone when the artifact could not be read, so the normal deploy retry heals it. Tracked separately rather than folded in here, because it changes deploy ACK semantics fleet-wide. + +## Addendum — issue #486 fix, live-gated (2026-07-21) + +The finding above is now fixed (`DriverHostActor.ApplyAndAck` fails the apply when `ReconcileDrivers` +returns null) and gated on the same rig with the same induction. + +| # | Check | Result | Evidence | +|---|---|---|---| +| 8 | Regression — a normal deploy still ACKs Applied fleet-wide | ✅ | The change alters ACK semantics, so this is the risk that matters. A genuine deploy recorded `Status = Applied` on **all six** nodes. | +| 9 | An unreadable artifact now ACKs Failed, with a reason | ✅ | central-2 recorded `Status = Failed` + `the deployment artifact could not be read (ConfigDb unreachable, or the row is missing/empty); nothing was applied`. The five nodes that read the real artifact recorded Applied — so the fleet view now distinguishes them. Pre-fix, all six claimed Applied. | +| 10 | The revision is NOT advanced | ✅ | `apply of a855dd9e… FAILED — … Staying on revision 5bc0d9f3…` — the **previous** revision, not the dispatched `583122b7…`. This is what stops `HandleDispatchFromSteady` from short-circuiting a retry of that revision. | +| 11 | The node keeps serving throughout (#485 still holds) | ✅ | The #485 guard fired first (`keeping the 5 running driver(s)`), and `ns=2;s=pymodbus/plc/HR200X` read **59603 @ 07:27:28**, live and current, after the Failed apply. Failing the apply reports the truth; it does not tear anything down. | +| 12 | The node recovers on the next real deploy | ✅ | The following deployment applied on central-2 (`children=5`) and recorded Applied on all six nodes. |