feat(runtime): announce added nodes after the materialise passes on non-rebuild applies (R2-07 T4b)

archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json

@@ -35,7 +35,7 @@
     {
       "id": "T4b",
       "subject": "Phase 1: OpcUaPublishActor calls AnnounceAddedNodes after the Materialise passes on non-rebuild applies (high-risk)",
-      "status": "pending",
+      "status": "completed",
       "blockedBy": [
         "T2",
         "T3",

src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs

@@ -357,6 +357,16 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers
             // values once the first dependency update arrives; until then variables show BadWaitingForInitialData.
             failedNodes += _applier.MaterialiseEquipmentVirtualTags(composition);
 
+            // R2-07 T4b — on a NON-rebuild apply (PureAdd / AttributeOnly), the Materialise passes above
+            // just created exactly the added nodes WITHOUT tearing anything down, so announce them with a
+            // Part 3 GeneralModelChangeEvent(NodeAdded) per affected parent — model-aware clients then
+            // re-browse and pick up the new nodes while every existing MonitoredItem stays alive. Ordering
+            // is correct by construction: Apply returned before the passes ran, and this announce runs after
+            // them, so the nodes exist when clients re-browse. After a full rebuild the announcement is moot
+            // (subscriptions are dead anyway) — the guard skips it. AnnounceAddedNodes is Safe-wrapped
+            // internally, so a faulting announce can never break the deploy.
+            if (!outcome.RebuildCalled) _applier.AnnounceAddedNodes(plan);
+
             OtOpcUaTelemetry.OpcUaSinkWrite.Add(1, new KeyValuePair<string, object?>("kind", "rebuild"));
             if (outcome.RebuildFailed || failedNodes > 0)
             {

tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorApplyFailureTests.cs

@@ -25,7 +25,10 @@ namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.OpcUa;
 /// </summary>
 public sealed class OpcUaPublishActorApplyFailureTests : RuntimeActorTestBase
 {
-    /// <summary>A rebuild whose sink throws increments otopcua.opcua.apply.failed (kind=rebuild).</summary>
+    /// <summary>A rebuild whose sink throws increments otopcua.opcua.apply.failed (kind=rebuild). R2-07 — a
+    /// pure-add no longer rebuilds, so the degraded-rebuild path is exercised via a rebuild-forcing change:
+    /// a first PureAdd deploy establishes the applied composition, then a rename (ChangedEquipment ⇒ Rebuild)
+    /// drives the throwing SafeRebuild.</summary>
     [Fact]
     public void Rebuild_when_sink_throws_increments_apply_failed_meter()
     {
@@ -33,10 +36,17 @@ public sealed class OpcUaPublishActorApplyFailureTests : RuntimeActorTestBase
         var db = NewInMemoryDbFactory();
         var sink = new ThrowOnRebuildSink();
         var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
-        SeedEquipmentDeployment(db, "eq-1");
+        var dep1 = SeedEquipmentDeployment(db, ("eq-1", "eq-1"));
 
         var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, dbFactory: db, applier: applier));
-        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
+        // First deploy: PureAdd — no rebuild, so the throwing sink is not hit yet.
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep1)));
+        Thread.Sleep(200);
+        recorder.Total.ShouldBe(0);
+
+        // Second deploy: eq-1 RENAMED ⇒ ChangedEquipment ⇒ Rebuild ⇒ the sink's RebuildAddressSpace throws.
+        var dep2 = SeedEquipmentDeployment(db, ("eq-1", "eq-1-renamed"));
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep2)));
 
         AwaitAssert(() =>
         {
@@ -53,7 +63,7 @@ public sealed class OpcUaPublishActorApplyFailureTests : RuntimeActorTestBase
         var db = NewInMemoryDbFactory();
         var sink = new NoopSink();
         var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
-        SeedEquipmentDeployment(db, "eq-1");
+        SeedEquipmentDeployment(db, ("eq-1", "eq-1"));
 
         var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, dbFactory: db, applier: applier));
         actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
@@ -75,25 +85,26 @@ public sealed class OpcUaPublishActorApplyFailureTests : RuntimeActorTestBase
         if (last is not null) throw last;
     }
 
-    private static void SeedEquipmentDeployment(IDbContextFactory<OtOpcUaConfigDbContext> dbFactory, params string[] equipmentIds)
+    private static Guid SeedEquipmentDeployment(IDbContextFactory<OtOpcUaConfigDbContext> dbFactory, params (string Id, string Name)[] equipment)
     {
         var artifact = JsonSerializer.SerializeToUtf8Bytes(new
         {
-            Equipment = equipmentIds.Select(id => new
+            Equipment = equipment.Select(e => new
             {
-                EquipmentId = id,
+                EquipmentId = e.Id,
-                MachineCode = id.ToUpperInvariant(),
+                MachineCode = e.Id.ToUpperInvariant(),
                 UnsLineId = "line-1",
-                Name = id,
+                Name = e.Name,
             }).ToArray(),
             DriverInstances = Array.Empty<object>(),
             ScriptedAlarms = Array.Empty<object>(),
         });
 
+        var id = Guid.NewGuid();
         using var ctx = dbFactory.CreateDbContext();
         ctx.Deployments.Add(new Deployment
         {
-            DeploymentId = Guid.NewGuid(),
+            DeploymentId = id,
             RevisionHash = new string('a', 64),
             Status = DeploymentStatus.Sealed,
             CreatedBy = "test",
@@ -101,6 +112,7 @@ public sealed class OpcUaPublishActorApplyFailureTests : RuntimeActorTestBase
             ArtifactBlob = artifact,
         });
         ctx.SaveChanges();
+        return id;
     }
 
     /// <summary>A sink whose RebuildAddressSpace throws (drives the applier's SafeRebuild catch → RebuildFailed).</summary>

tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs

@@ -18,7 +18,10 @@ namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.OpcUa;
 
 public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
 {
-    /// <summary>Tests that RebuildAddressSpace with dbFactory loads artifact, composes, and applies.</summary>
+    /// <summary>R2-07 — RebuildAddressSpace with dbFactory loads the artifact, composes, and applies. Two
+    /// brand-new equipment are a PureAdd: the Materialise passes create the equipment folders WITHOUT a full
+    /// rebuild (existing subscriptions survive), and the added parents are announced via NodeAdded. (Was:
+    /// pre-R2-07 this asserted one full rebuild.)</summary>
     [Fact]
     public void RebuildAddressSpace_with_dbFactory_loads_artifact_composes_and_applies()
     {
@@ -37,10 +40,13 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
 
         AwaitAssert(() =>
         {
-            // Add path: Equipment + Driver + Alarm — but only Equipment/Alarm topology triggers
+            // PureAdd: the equipment folders are materialised, no full rebuild, and the added parent
-            // RebuildAddressSpace. With 2 new equipment we expect one Rebuild call.
+            // (the shared line-1) is announced exactly once.
-            sink.RebuildCalls.ShouldBe(1);
+            sink.Calls.ShouldContain("EF:eq-1");
+            sink.Calls.ShouldContain("EF:eq-2");
+            sink.Calls.ShouldContain("NA:line-1");
         }, duration: TimeSpan.FromSeconds(2));
+        sink.RebuildCalls.ShouldBe(0);
     }
 
     /// <summary>Tests that rebuild with no artifact is idempotent no-op.</summary>
@@ -63,7 +69,10 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
         sink.RebuildCalls.ShouldBe(0);
     }
 
-    /// <summary>Tests that second rebuild with same artifact is empty plan no-op.</summary>
+    /// <summary>Tests that a second rebuild with the same artifact is an empty-plan no-op. The first deploy
+    /// (one new equipment) is a PureAdd — no full rebuild, but it DOES materialise the folder; the second
+    /// deploy of the identical composition diffs to an empty plan and the actor short-circuits, adding no
+    /// further sink calls.</summary>
     [Fact]
     public void Second_rebuild_with_same_artifact_is_empty_plan_no_op()
     {
@@ -76,12 +85,15 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
             sink: sink, dbFactory: db, applier: applier));
 
         actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
-        AwaitAssert(() => sink.RebuildCalls.ShouldBe(1), duration: TimeSpan.FromSeconds(2));
+        AwaitAssert(() => sink.Calls.ShouldContain("EF:eq-1"), duration: TimeSpan.FromSeconds(2));
+        sink.RebuildCalls.ShouldBe(0);          // PureAdd — no full rebuild
+        var callsAfterFirst = sink.Calls.Count;
 
         actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
         Thread.Sleep(200);
-        // Same composition ⇒ plan IsEmpty ⇒ applier not called again.
+        // Same composition ⇒ plan IsEmpty ⇒ applier + materialise passes not run again.
-        sink.RebuildCalls.ShouldBe(1);
+        sink.Calls.Count.ShouldBe(callsAfterFirst);
+        sink.RebuildCalls.ShouldBe(0);
     }
 
     /// <summary>Tests that rebuild without dbFactory falls back to raw sink rebuild.</summary>
@@ -125,9 +137,10 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
 
         siteActor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
 
-        AwaitAssert(() => sinkA.RebuildCalls.ShouldBe(1), duration: TimeSpan.FromSeconds(2));
+        // PureAdd (equipment + tag) ⇒ no full rebuild; the materialise passes still run the cluster slice.
         // t-sa (EquipmentId "eq-sa", FolderPath "F", Name "S1") → folder-scoped variable "eq-sa/F/S1".
-        sinkA.Calls.ShouldContain("EV:eq-sa/F/S1");
+        AwaitAssert(() => sinkA.Calls.ShouldContain("EV:eq-sa/F/S1"), duration: TimeSpan.FromSeconds(2));
+        sinkA.RebuildCalls.ShouldBe(0);
         // t-main (MAIN cluster) must NOT leak onto the SITE-A node.
         sinkA.Calls.ShouldNotContain("EV:eq-main/F/M1");
 
@@ -145,8 +158,8 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
 
         mainActor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
 
-        AwaitAssert(() => sinkM.RebuildCalls.ShouldBe(1), duration: TimeSpan.FromSeconds(2));
+        AwaitAssert(() => sinkM.Calls.ShouldContain("EV:eq-main/F/M1"), duration: TimeSpan.FromSeconds(2));
-        sinkM.Calls.ShouldContain("EV:eq-main/F/M1");
+        sinkM.RebuildCalls.ShouldBe(0);
         sinkM.Calls.ShouldNotContain("EV:eq-sa/F/S1");
     }
 
@@ -212,6 +225,100 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
         ctx.SaveChanges();
     }
 
+    /// <summary>R2-07 T4b — a PureAdd deploy (here a single new equipment) does NOT rebuild, and the
+    /// NodeAdded announcement is raised AFTER the Materialise passes have created the node (so the node
+    /// exists when a re-browsing client arrives). Proven via the ordered <c>Calls</c> queue: the "NA:"
+    /// announcement index is strictly after the "EF:" materialise index.</summary>
+    [Fact]
+    public void Pure_add_announces_node_after_materialising_and_never_rebuilds()
+    {
+        var db = NewInMemoryDbFactory();
+        var sink = new RecordingSink();
+        var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
+        SeedDeployment(db, equipmentIds: new[] { "eq-1" }, driverIds: Array.Empty<string>());
+
+        var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, dbFactory: db, applier: applier));
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId()));
+
+        AwaitAssert(() => sink.Calls.ShouldContain("NA:line-1"), duration: TimeSpan.FromSeconds(2));
+        sink.RebuildCalls.ShouldBe(0);
+
+        var calls = sink.Calls.ToList();
+        var materialiseIdx = calls.IndexOf("EF:eq-1");
+        var announceIdx = calls.IndexOf("NA:line-1");
+        materialiseIdx.ShouldBeGreaterThanOrEqualTo(0);
+        announceIdx.ShouldBeGreaterThan(materialiseIdx); // announced AFTER the node was materialised
+    }
+
+    /// <summary>R2-07 T4b — when a deploy forces a full rebuild (here a renamed equipment ⇒ ChangedEquipment
+    /// ⇒ Rebuild) even though it ALSO adds an equipment, the post-materialise announce hook is SKIPPED (the
+    /// rebuild made subscriptions moot). Proven by capturing the NodeAdded count after the first pure-add
+    /// deploy and asserting the rebuild-forcing second deploy adds none.</summary>
+    [Fact]
+    public void Rebuild_kind_deploy_skips_the_node_added_announce()
+    {
+        var db = NewInMemoryDbFactory();
+        var sink = new RecordingSink();
+        var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
+
+        var dep1 = SeedNamedEquipmentDeployment(db, ("eq-1", "Pump-1"));
+        var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, dbFactory: db, applier: applier));
+
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep1)));
+        AwaitAssert(() => sink.Calls.Count(c => c.StartsWith("NA:")).ShouldBe(1), duration: TimeSpan.FromSeconds(2));
+        sink.RebuildCalls.ShouldBe(0);
+        var naAfterFirst = sink.Calls.Count(c => c.StartsWith("NA:"));
+
+        // Second deploy: eq-1 RENAMED (ChangedEquipment ⇒ Rebuild) AND a new eq-2 (an add). The rebuild
+        // fires, so the announce hook is skipped despite eq-2 being added.
+        var dep2 = SeedNamedEquipmentDeployment(db, ("eq-1", "Pump-1-RENAMED"), ("eq-2", "Pump-2"));
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep2)));
+
+        AwaitAssert(() => sink.RebuildCalls.ShouldBe(1), duration: TimeSpan.FromSeconds(2));
+        // No new NodeAdded announcement was raised for the rebuild-kind deploy.
+        sink.Calls.Count(c => c.StartsWith("NA:")).ShouldBe(naAfterFirst);
+    }
+
+    /// <summary>Seal a deployment carrying the given (equipmentId, name) rows under a shared line, returning
+    /// the new DeploymentId so a test can target THAT artifact. Renaming an equipment across two such
+    /// deployments produces a ChangedEquipment delta (Rebuild), while adding a row is a PureAdd.</summary>
+    private static Guid SeedNamedEquipmentDeployment(
+        IDbContextFactory<OtOpcUaConfigDbContext> dbFactory,
+        params (string Id, string Name)[] equipment)
+    {
+        var artifact = JsonSerializer.SerializeToUtf8Bytes(new
+        {
+            UnsAreas = new[] { new { UnsAreaId = "area-1", ClusterId = "c1", Name = "Area" } },
+            UnsLines = new[] { new { UnsLineId = "line-1", UnsAreaId = "area-1", Name = "Line" } },
+            Equipment = equipment.Select(e => new
+            {
+                EquipmentId = e.Id,
+                DriverInstanceId = (string?)null,
+                UnsLineId = "line-1",
+                Name = e.Name,
+                MachineCode = e.Id.ToUpperInvariant(),
+            }).ToArray(),
+            DriverInstances = Array.Empty<object>(),
+            Namespaces = Array.Empty<object>(),
+            Tags = Array.Empty<object>(),
+            ScriptedAlarms = Array.Empty<object>(),
+        });
+
+        var id = Guid.NewGuid();
+        using var ctx = dbFactory.CreateDbContext();
+        ctx.Deployments.Add(new Deployment
+        {
+            DeploymentId = id,
+            RevisionHash = new string('d', 64),
+            Status = DeploymentStatus.Sealed,
+            CreatedBy = "test",
+            SealedAtUtc = DateTime.UtcNow,
+            ArtifactBlob = artifact,
+        });
+        ctx.SaveChanges();
+        return id;
+    }
+
     private static void SeedDeployment(
         IDbContextFactory<OtOpcUaConfigDbContext> dbFactory,
         string[] equipmentIds,
@@ -265,9 +372,11 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
         var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(
             sink: sink, dbFactory: db, applier: applier));
 
-        // First deploy: the area folder is materialised with the OLD name (one rebuild).
+        // First deploy: the area folder is materialised with the OLD name. R2-07 — this is now a PureAdd
+        // (area + line + equipment all added), so NO full rebuild; the folder is materialised directly.
         actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep1)));
-        AwaitAssert(() => sink.RebuildCalls.ShouldBe(1), duration: TimeSpan.FromSeconds(2));
+        AwaitAssert(() => sink.Calls.ShouldContain("EF:area-1"), duration: TimeSpan.FromSeconds(2));
+        sink.RebuildCalls.ShouldBe(0);
 
         // Second deploy: ONLY the area Name changed — a rename. The actor must reach the apply path and
         // drive a surgical in-place folder rename (NOT a rebuild, NOT a no-op).
@@ -278,7 +387,7 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
         {
             sink.FolderRenameCalls.ShouldContain(("area-1", "Plant South"));
         }, duration: TimeSpan.FromSeconds(2));
-        sink.RebuildCalls.ShouldBe(1); // still 1 — the rename did NOT force a second full rebuild
+        sink.RebuildCalls.ShouldBe(0); // the rename did NOT force a full rebuild
     }
 
     /// <summary>Seal a deployment whose area carries <paramref name="areaName"/>, with a line + one