fix(vtags): prune _planByVtag on child termination + crash-then-change test (H1b review follow-up)

2026-06-15 10:12:11 -04:00
parent ada01e1af8
commit ebf2f1dd7a
2 changed files with 42 additions and 4 deletions
@@ -39,8 +39,9 @@ public sealed class VirtualTagHostActor : ReceiveActor
    private readonly Dictionary<string, IActorRef> _children = new(StringComparer.Ordinal);
    // vtagId -> folder-scoped OPC UA NodeId the materialiser placed the variable at.
    private readonly Dictionary<string, string> _nodeIdByVtag = new(StringComparer.Ordinal);
-    // vtagId -> the plan its currently-spawned child was built from, so we can detect an in-place
-    // change (edited Expression/DependencyRefs) and stop+respawn the child to adopt it.
+    // vtagId -> the plan its live child was built from, so we can detect an in-place change (edited
+    // Expression/DependencyRefs) and stop+respawn the child to adopt it. Kept in lock-step with
+    // _children: rebuilt to the desired set at the end of every apply, and pruned in OnChildTerminated.
    private readonly Dictionary<string, EquipmentVirtualTagPlan> _planByVtag = new(StringComparer.Ordinal);

    /// <summary>Factory method to create Props for a VirtualTagHostActor.</summary>
@@ -161,8 +162,11 @@ public sealed class VirtualTagHostActor : ReceiveActor
        foreach (var id in stale)
        {
            _children.Remove(id);
-            // NodeId map is rebuilt on the next ApplyVirtualTags; leaving the mapping is harmless
-            // (no child will publish for it until respawned). A dead child is respawned on next apply.
+            // Keep _planByVtag in lock-step with _children so it never holds a plan for a dead child.
+            // (The next apply also resets it, and the change-detect guard short-circuits on the missing
+            // _children entry — so this is defensive consistency, not a live-bug fix.) NodeId map is
+            // rebuilt on the next ApplyVirtualTags; leaving that mapping is harmless. Dead child respawns.
+            _planByVtag.Remove(id);
            _log.Warning("VirtualTagHost: child for vtag {VirtualTagId} terminated; will respawn on next apply", id);
        }
    }
@@ -243,6 +243,40 @@ public sealed class VirtualTagHostActorTests : RuntimeActorTestBase
        mux.ExpectNoMsg(TimeSpan.FromMilliseconds(400));
    }

+    /// <summary>
+    /// H1b crash-then-change: after a child terminates unexpectedly AND the next apply carries a
+    /// CHANGED plan for the same vtagId, the replacement child must adopt the NEW plan (refs "B"),
+    /// not the stale one (refs "A"). This pins that OnChildTerminated prunes _planByVtag so the
+    /// change-detect guard can't be confused by a dead child's old plan.
+    /// </summary>
+    [Fact]
+    public void Respawn_after_termination_with_a_changed_plan_adopts_the_new_refs()
+    {
+        var publish = CreateTestProbe();
+        var mux = CreateTestProbe();
+        var host = Sys.ActorOf(VirtualTagHostActor.Props(publish.Ref, mux.Ref, new StubEvaluator()));
+
+        // First apply — child registers interest in "A".
+        host.Tell(new VirtualTagHostActor.ApplyVirtualTags(
+            new[] { PlanWithRefs("vt-1", "eq-1", "speed-rpm", "ctx.GetTag(\"A\")", "A") }));
+        mux.ExpectMsg<DependencyMuxActor.RegisterInterest>().TagRefs.ShouldContain("A");
+        var firstChild = mux.LastSender;
+
+        // Kill the child deterministically and drain its PostStop UnregisterInterest.
+        Watch(firstChild);
+        Sys.Stop(firstChild);
+        ExpectTerminated(firstChild);
+        mux.ExpectMsg<DependencyMuxActor.UnregisterInterest>(TimeSpan.FromSeconds(5));
+
+        // Re-apply with a CHANGED plan (refs "B"). The replacement must register the NEW refs.
+        host.Tell(new VirtualTagHostActor.ApplyVirtualTags(
+            new[] { PlanWithRefs("vt-1", "eq-1", "speed-rpm", "ctx.GetTag(\"B\")", "B") }));
+        var reg2 = mux.ExpectMsg<DependencyMuxActor.RegisterInterest>(TimeSpan.FromSeconds(5));
+        reg2.TagRefs.ShouldContain("B");
+        reg2.TagRefs.ShouldNotContain("A");
+        mux.LastSender.ShouldNotBe(firstChild);
+    }
+
    /// <summary>Deterministic no-op evaluator: keeps spawned children inert so tests drive the host's
    /// OnResult path directly via synthetic EvaluationResults.</summary>
    private sealed class StubEvaluator : IVirtualTagEvaluator