diff --git a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json index 17b97de4..d743e7e6 100644 --- a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json +++ b/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", diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs index b6648b2d..f8c856c0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs +++ b/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("kind", "rebuild")); if (outcome.RebuildFailed || failedNodes > 0) { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorApplyFailureTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorApplyFailureTests.cs index 2d401aaf..3bb225fa 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorApplyFailureTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorApplyFailureTests.cs @@ -25,7 +25,10 @@ namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.OpcUa; /// public sealed class OpcUaPublishActorApplyFailureTests : RuntimeActorTestBase { - /// A rebuild whose sink throws increments otopcua.opcua.apply.failed (kind=rebuild). + /// 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. [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.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.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 dbFactory, params string[] equipmentIds) + private static Guid SeedEquipmentDeployment(IDbContextFactory dbFactory, params (string Id, string Name)[] equipment) { var artifact = JsonSerializer.SerializeToUtf8Bytes(new { - Equipment = equipmentIds.Select(id => new + Equipment = equipment.Select(e => new { - EquipmentId = id, - MachineCode = id.ToUpperInvariant(), + EquipmentId = e.Id, + MachineCode = e.Id.ToUpperInvariant(), UnsLineId = "line-1", - Name = id, + Name = e.Name, }).ToArray(), DriverInstances = Array.Empty(), ScriptedAlarms = Array.Empty(), }); + 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; } /// A sink whose RebuildAddressSpace throws (drives the applier's SafeRebuild catch → RebuildFailed). diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs index ed596971..a34c325b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/OpcUa/OpcUaPublishActorRebuildTests.cs +++ b/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 { - /// Tests that RebuildAddressSpace with dbFactory loads artifact, composes, and applies. + /// 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.) [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 - // RebuildAddressSpace. With 2 new equipment we expect one Rebuild call. - sink.RebuildCalls.ShouldBe(1); + // PureAdd: the equipment folders are materialised, no full rebuild, and the added parent + // (the shared line-1) is announced exactly once. + 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); } /// Tests that rebuild with no artifact is idempotent no-op. @@ -63,7 +69,10 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase sink.RebuildCalls.ShouldBe(0); } - /// Tests that second rebuild with same artifact is empty plan no-op. + /// 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. [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. - sink.RebuildCalls.ShouldBe(1); + // Same composition ⇒ plan IsEmpty ⇒ applier + materialise passes not run again. + sink.Calls.Count.ShouldBe(callsAfterFirst); + sink.RebuildCalls.ShouldBe(0); } /// Tests that rebuild without dbFactory falls back to raw sink rebuild. @@ -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)); - sinkM.Calls.ShouldContain("EV:eq-main/F/M1"); + AwaitAssert(() => sinkM.Calls.ShouldContain("EV:eq-main/F/M1"), duration: TimeSpan.FromSeconds(2)); + sinkM.RebuildCalls.ShouldBe(0); sinkM.Calls.ShouldNotContain("EV:eq-sa/F/S1"); } @@ -212,6 +225,100 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase ctx.SaveChanges(); } + /// 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 Calls queue: the "NA:" + /// announcement index is strictly after the "EF:" materialise index. + [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.Instance); + SeedDeployment(db, equipmentIds: new[] { "eq-1" }, driverIds: Array.Empty()); + + 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 + } + + /// 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. + [Fact] + public void Rebuild_kind_deploy_skips_the_node_added_announce() + { + var db = NewInMemoryDbFactory(); + var sink = new RecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.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); + } + + /// 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. + private static Guid SeedNamedEquipmentDeployment( + IDbContextFactory 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(), + Namespaces = Array.Empty(), + Tags = Array.Empty(), + ScriptedAlarms = Array.Empty(), + }); + + 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 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 } /// Seal a deployment whose area carries , with a line + one