Node records Applied (and advances _currentRevision) for a deployment whose artifact it could not read #486

New Issue

2026-07-21T03:02:26-04:00

dohertj2 commented

2026-07-21 03:02:26 -04:00

Found during the #485 live gate (docs/plans/2026-07-21-issue-485-live-gate.md). Pre-existing — not introduced by the #485 fixes, though those fixes make the skip path more reachable.

What happens

DriverHostActor.ApplyAndAck advances the revision and records success before it knows whether anything was applied:

var appliedBlob = ReconcileDrivers(deploymentId);   // null when the artifact could not be read
_currentRevision = revision;                        // advances regardless
UpsertNodeDeploymentState(deploymentId, NodeDeploymentStatus.Applied, failureReason: null);
SendAck(deploymentId, ApplyAckOutcome.Applied, failureReason: null, correlation);

ReconcileDrivers returns null on both the (long-standing) throw path and the empty-artifact path added for #485. Observed live on central-2:

[07:00:07 WRN] DriverHost central-2:4053: artifact ... read back empty; skipping reconcile ...
[07:00:07 INF] DriverHost central-2:4053: applied deployment 8c36a673... (rev 03b33904..., children=5)

NodeDeploymentState for that deployment reads Status = Applied for central-2.

Why it matters

The node claims a revision whose configuration it never applied. Because HandleDispatchFromSteady short-circuits on a revision match (dispatch matches current rev; immediate ACK), re-dispatching that same revision is a no-op — the node cannot self-heal. Only a later deployment carrying a different revision recovers it.

The fleet view is also wrong: the coordinator and the AdminUI both believe the node is converged when it is not.

Scope

Reporting/convergence only, not data plane. Thanks to the #485 fixes the node keeps serving its last known-good address space, drivers and subscriptions throughout — verified live (values kept advancing across the event). So this is a lying status, not an outage.

Suggested fix

When ReconcileDrivers returns null, treat the apply as failed: leave _currentRevision alone, write NodeDeploymentStatus.Failed with the reason, and SendAck(..., ApplyAckOutcome.Failed, ...). The normal deploy retry path then heals the node, and the short-circuit stops hiding the gap.

Kept out of the #485 fixes deliberately — it changes deploy ACK semantics fleet-wide (a node that today reports Applied would start reporting Failed), so it wants its own change and its own live gate.

Found during the #485 live gate (`docs/plans/2026-07-21-issue-485-live-gate.md`). **Pre-existing** — not introduced by the #485 fixes, though those fixes make the skip path more reachable. ## What happens `DriverHostActor.ApplyAndAck` advances the revision and records success *before* it knows whether anything was applied: ```csharp var appliedBlob = ReconcileDrivers(deploymentId); // null when the artifact could not be read _currentRevision = revision; // advances regardless UpsertNodeDeploymentState(deploymentId, NodeDeploymentStatus.Applied, failureReason: null); SendAck(deploymentId, ApplyAckOutcome.Applied, failureReason: null, correlation); ``` `ReconcileDrivers` returns `null` on both the (long-standing) throw path and the empty-artifact path added for #485. Observed live on central-2: ``` [07:00:07 WRN] DriverHost central-2:4053: artifact ... read back empty; skipping reconcile ... [07:00:07 INF] DriverHost central-2:4053: applied deployment 8c36a673... (rev 03b33904..., children=5) ``` `NodeDeploymentState` for that deployment reads `Status = Applied` for central-2. ## Why it matters The node claims a revision whose configuration it never applied. Because `HandleDispatchFromSteady` short-circuits on a revision match (`dispatch matches current rev; immediate ACK`), **re-dispatching that same revision is a no-op** — the node cannot self-heal. Only a later deployment carrying a *different* revision recovers it. The fleet view is also wrong: the coordinator and the AdminUI both believe the node is converged when it is not. ## Scope Reporting/convergence only, not data plane. Thanks to the #485 fixes the node keeps serving its last known-good address space, drivers and subscriptions throughout — verified live (values kept advancing across the event). So this is a lying status, not an outage. ## Suggested fix When `ReconcileDrivers` returns `null`, treat the apply as failed: leave `_currentRevision` alone, write `NodeDeploymentStatus.Failed` with the reason, and `SendAck(..., ApplyAckOutcome.Failed, ...)`. The normal deploy retry path then heals the node, and the short-circuit stops hiding the gap. Kept out of the #485 fixes deliberately — it changes deploy ACK semantics fleet-wide (a node that today reports Applied would start reporting Failed), so it wants its own change and its own live gate.

dohertj2 referenced this issue

2026-07-21 03:02:43 -04:00

Transient ConfigDb error during artifact load empties the served address space (logs claim "rebuild becomes no-op") #485

dohertj2 referenced this issue from a commit

2026-07-21 03:28:00 -04:00

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

dohertj2 closed this issue

2026-07-21 03:28:00 -04:00

dohertj2 commented

2026-07-21 03:28:14 -04:00

Fixed and live-gated on master (7808faa7). Evidence appended to docs/plans/2026-07-21-issue-485-live-gate.md.

A null blob from ReconcileDrivers now 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 the part that matters — it is what makes a retry actually land instead of being waved through by the revision short-circuit.

Live results on docker-dev, same induction as #485:

Regression first, since this changes ACK semantics fleet-wide: a normal deploy still records Applied on all six nodes.
Status = Failed on central-2 with the reason, while the five nodes that read the real artifact record Applied — the fleet view now distinguishes them. Pre-fix all six claimed Applied.
apply of a855dd9e... FAILED — ... Staying on revision 5bc0d9f3... — the previous revision, not the dispatched 583122b7....
The node kept serving throughout: the #485 guard fired first (keeping the 5 running driver(s)) and the Modbus tag read 59603 with a current timestamp after the Failed apply. This reports the truth; it does not tear anything down.
The next real deploy applied normally on central-2.

Tests were RED-first (both failed with "should be Failed but was Applied"). The one that carries the weight is the retry test: after a failed apply, re-dispatching the same revision now genuinely applies — its recovered artifact adds a second driver specifically so a short-circuited ACK, which has no side effects, cannot satisfy it.

Four existing tests changed, both changes 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 exactly 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.

Fixed and live-gated on `master` (`7808faa7`). Evidence appended to `docs/plans/2026-07-21-issue-485-live-gate.md`. A null blob from `ReconcileDrivers` now 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 the part that matters — it is what makes a retry actually land instead of being waved through by the revision short-circuit. Live results on docker-dev, same induction as #485: - **Regression first**, since this changes ACK semantics fleet-wide: a normal deploy still records Applied on **all six** nodes. - `Status = Failed` on central-2 with the reason, while the five nodes that read the real artifact record Applied — the fleet view now distinguishes them. Pre-fix all six claimed Applied. - `apply of a855dd9e... FAILED — ... Staying on revision 5bc0d9f3...` — the **previous** revision, not the dispatched `583122b7...`. - The node kept serving throughout: the #485 guard fired first (`keeping the 5 running driver(s)`) and the Modbus tag read 59603 with a current timestamp after the Failed apply. This reports the truth; it does not tear anything down. - The next real deploy applied normally on central-2. Tests were RED-first (both failed with "should be Failed but was Applied"). The one that carries the weight is the retry test: after a failed apply, re-dispatching the **same** revision now genuinely applies — its recovered artifact adds a second driver specifically so a short-circuited ACK, which has no side effects, cannot satisfy it. Four existing tests changed, both changes 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 exactly 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.

dohertj2 referenced this issue

2026-07-21 03:36:41 -04:00

feat(drivers): periodic reconcile of desired vs actual driver subscriptions #488

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#486