diff --git a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md index 9d0de32d..4a0d7ace 100644 --- a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md @@ -537,3 +537,61 @@ added: `IReadOnlyDictionary? tagConfigByTag = null`. The seeder alarm-bearing raw tag that `Operator_restart_re_subscribes_alarm_refs_too` (a test the plan itself specifies) requires. It is trailing and optional, so it is the same source-compatible shape as `enabled`. + +### T5 — `DriverHostActorRestartTests` (operator-restart falsification, 03/U6's first half) + +**RED observed against a T3-reverted build** (the `SendDesiredSubscriptions(child, …)` call in +`SpawnChild` commented out, everything else on the branch): **all 4 FAILED, 0 passed**, each at the +bar the plan predicts — + +``` +Operator_restart_re_subscribes_the_respawned_child [FAIL] + factory.Created[1].SubscribeCount should be greater than or equal to 1 but was 0 +Operator_restart_re_subscribes_alarm_refs_too [FAIL] + factory.Created[1].SubscribeAlarmsCount should be greater than or equal to 1 but was 0 +Restart_of_a_driver_with_an_empty_value_set_still_sends_the_set [FAIL] + respawned.SubscribeAlarmsCount should be greater than or equal to 1 but was 0 +A_child_spawned_by_a_fresh_deploy_ends_with_the_NEW_refs [FAIL] + factory.Created[2].LastSubscribedRefs should be ["Plant/Modbus/dev1/temp"] but was null +``` + +The call was then restored; **4/4 pass**, and the class was run **20× in isolation, 20/20 green** (#500 +discipline for this assembly). + +**Two deviations, both found by running the tests rather than by reading:** + +1. ⚠️ **The plan's claim that the transient spawn-time set "is never acted on" is FALSE, and the + production comment that repeated it has been corrected.** The plan reasons that the child is still + `Connecting` when the stale set arrives, so it is only stored. But the child runs **concurrently** + with the rest of `HandleArtifactForApply` — `Context.ActorOf` returns and the child starts + processing while the host is still building `refsByDriver` — so a driver whose `InitializeAsync` + completes instantly reaches Connected and subscribes the stale set *before* the bulk push arrives. + Measured directly: `SubscribeCount == 2` on the respawned child, first with revision A's ref and + then with B's. What Akka's per-pair ordering does guarantee is **convergence**, and + `HandleSubscribeAsync` drops the prior subscription before establishing the new one, so the cost is + one extra subscribe round-trip on a *respawning* deploy — a real device driver, whose initialise + opens a socket, rarely wins that race. Accepted rather than "fixed": withholding the spawn-time send + is the 03/S16 defect, and the operator-Restart and supervision paths have no bulk push coming at + all. The `SpawnChild` comment now states this instead of the plan's claim, and the test asserts + convergence (the plan's own stated acceptance — "the child's live subscription carries the NEW + refs") rather than pinning the race. +2. **`A_child_spawned_by_a_fresh_deploy_ends_with_the_NEW_refs` had to be re-shaped.** The plan + describes revision B as one "which respawns nothing but re-pushes". It respawns: the raw tags are + folded into the merged `DriverConfig`, so a tag-set edit changes the `DriverInstanceSpec` and trips + the #516 config respawn. That is the *stronger* arrangement for this test's subject (a brand-new + child spawned while the authority still holds A), so the test keeps the same name and asserts the + respawned child converges on B — plus a subsequent operator Restart, which pins that the authority + is repopulated **whole** per apply rather than accumulated. +3. **`Restart_of_a_driver_with_no_authored_tags_sends_an_empty_set_not_nothing` was renamed + `Restart_of_a_driver_with_an_empty_value_set_still_sends_the_set`, with a real positive control.** + The seeder derives `DriverInstances` from the raw-tag rows, so a driver with *zero* tags is not + expressible, and — more importantly — an all-empty set has **no driver-side observable at all** + (the child stores it and, having no handle, does nothing), which would have made the test vacuous: + "it did not subscribe" passes equally against a build that sends the respawned child nothing, i.e. + against the defect itself. The subject is therefore a driver whose only raw tag is alarm-bearing: + its value half is genuinely empty while its alarm half is not, and **both halves ride in one + message** — so `SubscribeAlarmsCount >= 1` proves the message ARRIVED and `SubscribeCount == 0` + then proves the empty value half was honoured. The falsification run above confirms the control + works: with T3 reverted this test fails on the *positive control*, not on the absence. + **Not covered, and worth a reviewer's eye:** the `_desiredByDriver` union's `_children.Keys` arm + (a running child absent from BOTH ref maps) is not reachable through this seeder and is untested. diff --git a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json index 42819372..7023cf45 100644 --- a/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json +++ b/archreview/plans/R3-03-restart-resubscribe-watchdog-plan.md.tasks.json @@ -31,7 +31,7 @@ { "id": "T5", "subject": "DriverHostActorRestartTests: operator restart re-subscribes the respawned child (falsifiable; 03/S16, U6)", - "status": "pending", + "status": "done", "blockedBy": [ "T3", "T4" 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 84bf70b6..9b58ed51 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -2825,12 +2825,20 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS // that connected and then read nothing until the next deployment — the #488 periodic reconcile cannot // help, because it early-returns on an empty desired set. // - // ORDERING, deliberately: ApplyDriverSpecs (which spawns) runs BEFORE ApplyDesiredSubscriptions (which - // rebuilds _desiredByDriver), so a newly-spawned child is handed the PREVIOUS apply's set — or nothing, - // on the first deploy. That transient is superseded milliseconds later by the bulk push on the same - // sender→receiver pair (Akka guarantees per-pair ordering), and the child is still Connecting, where - // SetDesiredSubscriptions only STORES; the subscribe happens at Connected entry. So the stale set is - // never acted on. Sent after InitializeRequested above, which keeps init-before-subscribe ordering. + // ORDERING, and its one accepted cost: ApplyDriverSpecs (which spawns) runs BEFORE + // ApplyDesiredSubscriptions (which rebuilds _desiredByDriver), so on a deploy that RESPAWNS a child + // (#516 — the merged DriverConfig folds in the raw tags, so a tag-set edit changes the spec) the new + // child is handed the PREVIOUS apply's set, and nothing at all on the first deploy. That transient is + // superseded a moment later by the bulk push, on the same sender→receiver pair, so Akka's per-pair + // ordering guarantees the child CONVERGES on the new set — but it is NOT guaranteed to be inert on + // the way there: the child runs concurrently with the rest of this handler, so a driver whose + // InitializeAsync completes quickly can reach Connected and subscribe the stale set before the bulk + // push arrives, then re-subscribe. The cost is one extra subscribe/unsubscribe round-trip on a + // respawning deploy (HandleSubscribeAsync drops the prior subscription before establishing the new + // one, so it converges); a real device driver, whose initialise opens a socket, rarely wins that race. + // Deliberately not "fixed" by withholding the send: a child that never gets a set is the 03/S16 + // defect, and the operator-Restart and supervision paths have no bulk push coming at all. + // Sent after InitializeRequested above, which keeps init-before-subscribe ordering. SendDesiredSubscriptions(child, spec.DriverInstanceId); // v3 (WP7): if the (real, non-stub) driver consumes other tags' live values, spawn a mux-adapter diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorRestartTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorRestartTests.cs new file mode 100644 index 00000000..621213c1 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorRestartTests.cs @@ -0,0 +1,287 @@ +using Akka.Actor; +using Microsoft.EntityFrameworkCore; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Admin; +using ZB.MOM.WW.OtOpcUa.Commons.Types; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// The repo's first driver-child restart suite (archreview 03/S16, 03/U6): what a child obtained +/// by each of the three incarnation routes is actually handed. +/// +/// +/// +/// The falsification bar, and why this file exists at all. An actor-restart test must assert a +/// FUNCTION the new incarnation performs — it subscribed, it re-initialised — never that the +/// incarnation EXISTS. Existence is exactly what +/// DriverHostActorReconcileTests.RestartDriver_respawns_the_child_without_an_actor_name_collision +/// asserts (no name collision, factory created a second driver, one child remains), and that test +/// passes green against the 03/S16 defect: a respawned child that connects and then reads nothing +/// forever. It is kept, because its own subject — the InvalidActorNameException — is real; it +/// is simply the weaker half of a pair, and this file is the other half. +/// +/// +/// The three incarnation routes, and what re-provisions each. (1) Deploy reconcile — +/// ApplyDriverSpecs → SpawnChild, then the apply's bulk desired-subscription push. +/// (2) Operator Restart — HandleRestartDriver stops the child and calls +/// SpawnChild with the cached spec; before 03/S16 that was the whole story, and the new child +/// started with an EMPTY desired set that nothing re-sent until the next deployment (the #488 +/// periodic reconcile early-returns on an empty set, so it is structurally inert here). +/// (3) Supervision restart — Akka's default decider recreates the actor object around the +/// SAME driver instance without going through SpawnChild at all, so nothing re-sent +/// InitializeRequested either and the child sat in Connecting forever around a driver +/// its own PostStop had already shut down. A fourth route, Force Reconnect, re-uses the +/// live actor instance and needs nothing — a verified negative, pinned here so a future edit to the +/// restart path cannot break the path that was already correct. +/// +/// +/// The supervision crash seam, and why the obvious ones do not work. The seam used is the +/// driver's own OnDataChange carrying a NULL Snapshot +/// (): DataChangeEventArgs is +/// a positional record with no null validation, and DriverInstanceActor.OnDataChangeForward +/// dereferences msg.Snapshot unguarded on the ACTOR thread, so it NREs inside the child and +/// the default decider Restarts it. Rejected alternatives, each for a specific reason: a throwing +/// IDriverHealthPublisher (PublishHealthSnapshot wraps the publish in a +/// catch (Exception), so it never escapes); a throwing SubscribeAsync (also swallowed — +/// the stub already models it via SubscribeFailuresBeforeSuccess); Kill.Instance (the +/// default decider Stops on ActorKilledException, a different directive and therefore a +/// different code path); and an internal "throw this" test message (a production surface added +/// purely for a test). The fault is arbitrary by design — what is under test is the RECOVERY. If +/// DataChangeEventArgs ever gains null validation, the fallback is an explicit +/// OneForOneStrategy(Directive.Restart) on the host plus an internal test-only throw message. +/// +/// +public sealed class DriverHostActorRestartTests : RuntimeActorTestBase +{ + private static readonly RevisionHash RevA = RevisionHash.Parse(new string('a', 64)); + private static readonly RevisionHash RevB = RevisionHash.Parse(new string('b', 64)); + private static readonly NodeId TestNode = NodeId.Parse("node-1:4053"); + + private const string DriverId = "DI-1"; + private const string SpeedPath = "Plant/Modbus/dev1/speed"; + private const string TempPath = "Plant/Modbus/dev1/temp"; + + /// Seeds + applies a v3 deployment whose single Modbus driver is ENABLED, so a real child is + /// spawned around a , and waits until that child has connected + /// and established its first subscription. + private (IActorRef Host, RecordingSubscribableDriverFactory Factory) DeployOneDriver( + IDbContextFactory db, + (string Driver, string DriverName, string Device, string Tag, string DataType, bool Writable)[] rawTags, + IReadOnlyDictionary? tagConfigByTag = null) + { + var factory = new RecordingSubscribableDriverFactory("Modbus"); + var deploymentId = V3ArtifactSeeder.SeedV3Deployment( + db, RevA, rawTags, Array.Empty<(string, string, string, string, string?)>(), + enabled: true, tagConfigByTag: tagConfigByTag); + var (host, _, _) = V3ArtifactSeeder.SpawnHostAndApply(this, db, TestNode, deploymentId, RevA, factory); + return (host, factory); + } + + /// + /// The 03/S16 falsification. An operator's Restart is supposed to recover a sick driver; before + /// the fix it silently killed it — the respawned child came up with _desiredRefs empty, and the + /// only SetDesiredSubscriptions sender in the repo was the apply's bulk push, so the driver + /// subscribed to nothing until the next deployment dispatch. + /// + /// Deliberately NOT the pass criterion: that a second driver was created, or that one child + /// remains. Both are true against the defect. + /// + /// + [Fact] + public void Operator_restart_re_subscribes_the_respawned_child() + { + var db = NewInMemoryDbFactory(); + var (host, factory) = DeployOneDriver(db, new[] { (DriverId, "Modbus", "dev1", "speed", "Float", false) }); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(1); + factory.Created[0].SubscribeCount.ShouldBe(1); + factory.Created[0].LastSubscribedRefs.ShouldBe(new[] { SpeedPath }); + }, + duration: PresenceBudget); + + host.Tell(new RestartDriver("c1", DriverId, "tester", Guid.NewGuid())); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(2, "the restart did not respawn the child around a new driver"); + factory.Created[1].SubscribeCount.ShouldBeGreaterThanOrEqualTo( + 1, + "the RESPAWNED child never subscribed — it is Connected and reading nothing, which is the " + + "03/S16 defect (an operator pressed Restart to recover a driver and silently killed it)"); + factory.Created[1].LastSubscribedRefs.ShouldBe(new[] { SpeedPath }); + }, + duration: PresenceBudget); + } + + /// + /// The value refs and the alarm refs ride in ONE SetDesiredSubscriptions but are applied by two + /// different child code paths (the value set self-Tells Subscribe; the alarm set runs + /// SubscribeDesiredAlarms against the IAlarmSource feed, which stays gated shut until it + /// is called). Sending the message is therefore not evidence that both halves landed, so the alarm + /// half is pinned separately. + /// + [Fact] + public void Operator_restart_re_subscribes_alarm_refs_too() + { + var db = NewInMemoryDbFactory(); + var (host, factory) = DeployOneDriver( + db, + new[] + { + (DriverId, "Modbus", "dev1", "speed", "Float", false), + (DriverId, "Modbus", "dev1", "temp", "Float", false), + }, + // An alarm-bearing raw tag is EXCLUDED from the value set and carried in the alarm set instead. + new Dictionary + { + ["temp"] = """{"alarm":{"alarmType":"AlarmCondition","severity":500}}""", + }); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(1); + factory.Created[0].SubscribeAlarmsCount.ShouldBeGreaterThanOrEqualTo(1); + factory.Created[0].LastSubscribedAlarmRefs.ShouldBe(new[] { TempPath }); + }, + duration: PresenceBudget); + + host.Tell(new RestartDriver("c1", DriverId, "tester", Guid.NewGuid())); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(2); + factory.Created[1].SubscribeAlarmsCount.ShouldBeGreaterThanOrEqualTo( + 1, + "the respawned child never re-subscribed its alarm refs — the IAlarmSource feed stays " + + "gated shut, so no native alarm transition can ever reach the host again"); + factory.Created[1].LastSubscribedAlarmRefs.ShouldBe(new[] { TempPath }); + // The value half must survive the alarm half being present. + factory.Created[1].LastSubscribedRefs.ShouldBe(new[] { SpeedPath }); + }, + duration: PresenceBudget); + } + + /// + /// The ordering guard for the spawn-time send. ApplyDriverSpecs (which spawns) runs BEFORE + /// ApplyDesiredSubscriptions (which rebuilds the authority), so a child spawned by a fresh + /// deploy is momentarily handed the PREVIOUS apply's set. That transient must never be acted on: it + /// is superseded by the bulk push on the same sender→receiver pair (Akka guarantees per-pair + /// ordering) while the child is still Connecting, where SetDesiredSubscriptions only + /// STORES; the subscribe happens on the Connected entry, from whatever was stored last. + /// + /// Revision B swaps the driver's tag, so A's and B's sets are distinguishable. Note the raw tags + /// are folded into the merged DriverConfig, so a tag-set change DOES change the driver + /// spec and therefore respawns the child (#516) — which is precisely the arrangement under test: + /// a brand-new child, spawned while the authority still holds A, must end up subscribed to B. + /// + /// + /// What is asserted is CONVERGENCE, not inertness, and the difference was measured rather + /// than assumed. The child runs concurrently with the rest of the apply handler, so a driver + /// whose InitializeAsync completes instantly (this stub does) can reach Connected and + /// subscribe the transient stale set BEFORE the bulk push arrives — observed here as + /// SubscribeCount == 2. Akka's per-pair ordering still guarantees the final state is B's, + /// and HandleSubscribeAsync drops the prior subscription before establishing the new one, + /// so the cost is one extra round-trip on a respawning deploy. Pinning the count would pin a + /// race; the final refs are the invariant. + /// + /// + [Fact] + public void A_child_spawned_by_a_fresh_deploy_ends_with_the_NEW_refs() + { + var db = NewInMemoryDbFactory(); + var (host, factory) = DeployOneDriver(db, new[] { (DriverId, "Modbus", "dev1", "speed", "Float", false) }); + + AwaitAssert( + () => factory.Created[0].LastSubscribedRefs.ShouldBe(new[] { SpeedPath }), + duration: PresenceBudget); + + var revB = V3ArtifactSeeder.SeedV3Deployment( + db, RevB, + new[] { (DriverId, "Modbus", "dev1", "temp", "Float", false) }, + Array.Empty<(string, string, string, string, string?)>(), + enabled: true); + host.Tell(new Commons.Messages.Deploy.DispatchDeployment(revB, RevB, CorrelationId.NewId())); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(2, "the tag-set change re-spawns the child (#516 config respawn)"); + factory.Created[1].LastSubscribedRefs.ShouldBe( + new[] { TempPath }, + "the newly-spawned child is on the PREVIOUS revision's refs — the spawn-time set was not superseded"); + }, + duration: PresenceBudget); + + // A Restart on top of that must hand the new child B's set, not A's — i.e. the authority really is + // repopulated WHOLE on every apply rather than accumulated. + host.Tell(new RestartDriver("c1", DriverId, "tester", Guid.NewGuid())); + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(3); + factory.Created[2].LastSubscribedRefs.ShouldBe(new[] { TempPath }); + }, + duration: PresenceBudget); + } + + /// + /// The empty-set semantics are load-bearing and must not degrade into "skip the send": an EMPTY value + /// set is what tells the child to Unsubscribe and drop a stale handle, so it must be SENT. + /// + /// The subject is a driver whose only raw tag is alarm-bearing: alarm tags are excluded from the + /// value set and carried in the alarm one, so its value half is genuinely empty while its alarm + /// half is not — and both halves ride in ONE SetDesiredSubscriptions. That is what makes + /// the absence assertion non-vacuous: SubscribeAlarmsCount >= 1 proves the message + /// ARRIVED, and SubscribeCount == 0 then proves the empty value half was honoured rather + /// than the whole send being skipped. Asserting only "it did not subscribe" would pass equally + /// against a build that sent the respawned child nothing at all — the 03/S16 defect itself. + /// + /// + [Fact] + public void Restart_of_a_driver_with_an_empty_value_set_still_sends_the_set() + { + var db = NewInMemoryDbFactory(); + var (host, factory) = DeployOneDriver( + db, + new[] { (DriverId, "Modbus", "dev1", "temp", "Float", false) }, + new Dictionary + { + ["temp"] = """{"alarm":{"alarmType":"AlarmCondition","severity":500}}""", + }); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(1); + factory.Created[0].SubscribeAlarmsCount.ShouldBeGreaterThanOrEqualTo(1); + }, + duration: PresenceBudget); + + host.Tell(new RestartDriver("c1", DriverId, "tester", Guid.NewGuid())); + + AwaitAssert( + () => + { + factory.Created.Count.ShouldBe(2); + var respawned = factory.Created[1]; + respawned.InitializeCount.ShouldBe(1, "the respawned child was never initialised"); + // Positive control: the SetDesiredSubscriptions genuinely reached the respawned child. + respawned.SubscribeAlarmsCount.ShouldBeGreaterThanOrEqualTo( + 1, "the respawned child received no desired set at all — the absence below would be vacuous"); + respawned.LastSubscribedAlarmRefs.ShouldBe(new[] { TempPath }); + respawned.SubscribeCount.ShouldBe( + 0, "an EMPTY value set must route to Unsubscribe / no-op, never to a spurious subscribe"); + }, + duration: PresenceBudget); + } +}