fix(opcua): an identical re-deploy after a degraded REBUILD repairs instead of reporting clean (#601 review)

A degraded rebuild advances _lastApplied (deliberately), so re-deploying the same revision diffed to an empty plan and reported a CLEAN outcome - the #601 're-deploy re-applies' repair path sealed green over an address space still known partial. The publish actor now remembers that the last advance was degraded and, on an empty plan, re-ensures the whole composition (idempotent, no rebuild), reporting clean only when that pass is clean. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 17:44:12 -04:00
parent 82ef89416f
commit fe7603ec6e
3 changed files with 102 additions and 8 deletions
@@ -157,6 +157,15 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers
    /// protecting. Distinguishes "the artifact went missing under a running server" (issue #485 — warn, and
    /// hold what is materialised) from "nothing has been deployed here yet" (a quiet no-op at boot).</summary>
    private bool _hasAppliedComposition;
+    /// <summary>
+    ///     Gitea #601 review — <see cref="_lastApplied"/> was advanced by a DEGRADED rebuild (a rebuild advances
+    ///     even with swallowed failures, see <see cref="HandleRebuildCore"/>). A re-deploy of the same revision
+    ///     then diffs to an EMPTY plan; without this flag that empty plan reported clean, so the "re-deploy
+    ///     re-applies" repair path sealed green over an address space still known to be partial. While set, an
+    ///     empty plan re-materialises the whole composition (idempotent ensures) and reports clean only when
+    ///     that pass is itself clean.
+    /// </summary>
+    private bool _lastAppliedDegraded;
    private DbHealthProbeActor.DbHealthStatus? _lastDbHealth;
    private RedundancyStateChanged? _lastSnapshot;
    private (bool Ok, DateTime At)? _probeAboutMe;
@@ -565,6 +574,28 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers

            if (plan.IsEmpty)
            {
+                if (_lastAppliedDegraded)
+                {
+                    // #601 review — the composition is unchanged but the last apply of it was a degraded
+                    // rebuild: re-ensure every node (each Ensure* no-ops on a node that stands) so the
+                    // re-deploy actually repairs what that rebuild could not place.
+                    var repairFailed = _applier.MaterialiseHierarchy(composition)
+                        + MaterialiseNodes(plan, composition, fullComposition: true);
+                    if (repairFailed > 0)
+                    {
+                        OtOpcUaTelemetry.OpcUaApplyFailed.Add(1, new KeyValuePair<string, object?>("kind", "nodes"));
+                        _log.Error(
+                            "OpcUaPublish: re-apply of a degraded composition is STILL degraded (correlation={Correlation}, failedNodes={FailedNodes})",
+                            msg.Correlation, repairFailed);
+                        return (true, $"re-apply of a previously degraded address space is still degraded (failedNodes={repairFailed})");
+                    }
+
+                    _lastAppliedDegraded = false;
+                    _log.Info("OpcUaPublish: re-apply repaired the previously degraded address space (correlation={Correlation})",
+                        msg.Correlation);
+                    return (false, null);
+                }
+
                _log.Debug("OpcUaPublish: rebuild requested but plan is empty (correlation={Correlation})",
                    msg.Correlation);
                return (false, null);
@@ -610,6 +641,9 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers
            {
                _lastApplied = composition;
                _hasAppliedComposition = true;
+                // #601 review — advanced despite failures only on a rebuild; remember it so an identical
+                // re-deploy repairs instead of short-circuiting clean on the empty plan.
+                _lastAppliedDegraded = outcome.RebuildFailed || failedNodes > 0;
            }
            else
            {
@@ -112,7 +112,49 @@ public sealed class OpcUaPublishActorApplyOutcomeTests : RuntimeActorTestBase
        ExpectNoMsg(TimeSpan.FromMilliseconds(200));
    }

-    private static byte[] BuildEquipmentArtifact(string equipmentId) =>
+    [Fact]
+    public void Redeploying_the_same_composition_after_a_degraded_REBUILD_repairs_it_instead_of_reporting_clean()
+    {
+        // A degraded REBUILD advances _lastApplied (deliberately — see HandleRebuildCore), so a re-deploy of
+        // the same revision diffs to an EMPTY plan. Reporting that empty plan as clean would let the
+        // "re-deploy re-applies" repair path (#601) seal green over an address space still known partial.
+        var sink = new FaultingSink(throwOnEnsureFolder: false);
+        var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
+        var actor = Sys.ActorOf(OpcUaPublishActor.PropsForTests(sink: sink, applier: applier));
+
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(
+            CorrelationId.NewId(), DeploymentId.NewId(), BuildEquipmentArtifact("eq-1", "First"), ReportOutcome: true));
+        ExpectMsg<OpcUaPublishActor.RebuildCompleted>(PresenceBudget).Degraded.ShouldBeFalse();
+
+        // A changed equipment row routes to a full rebuild; its re-materialise faults.
+        sink.ThrowOnEnsureFolder = true;
+        var changed = BuildEquipmentArtifact("eq-1", "Second");
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), DeploymentId.NewId(), changed, ReportOutcome: true));
+        var degraded = ExpectMsg<OpcUaPublishActor.RebuildCompleted>(PresenceBudget);
+        degraded.Degraded.ShouldBeTrue();
+        sink.RebuildCount.ShouldBe(1);
+
+        // Same revision again, sink still faulting: must NOT claim clean.
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), DeploymentId.NewId(), changed, ReportOutcome: true));
+        ExpectMsg<OpcUaPublishActor.RebuildCompleted>(PresenceBudget).Degraded.ShouldBeTrue();
+
+        // Fault gone: the same revision re-materialises the whole composition and only THEN reports clean.
+        sink.ThrowOnEnsureFolder = false;
+        var foldersBefore = sink.EnsureFolderCount;
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), DeploymentId.NewId(), changed, ReportOutcome: true));
+        ExpectMsg<OpcUaPublishActor.RebuildCompleted>(PresenceBudget).Degraded.ShouldBeFalse();
+        sink.EnsureFolderCount.ShouldBeGreaterThan(foldersBefore);
+
+        // And now it is genuinely clean, so a further identical deploy is the ordinary empty no-op.
+        var foldersAfterRepair = sink.EnsureFolderCount;
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), DeploymentId.NewId(), changed, ReportOutcome: true));
+        ExpectMsg<OpcUaPublishActor.RebuildCompleted>(PresenceBudget).Degraded.ShouldBeFalse();
+        sink.EnsureFolderCount.ShouldBe(foldersAfterRepair);
+    }
+
+    private static byte[] BuildEquipmentArtifact(string equipmentId) => BuildEquipmentArtifact(equipmentId, equipmentId);
+
+    private static byte[] BuildEquipmentArtifact(string equipmentId, string name) =>
        JsonSerializer.SerializeToUtf8Bytes(new
        {
            UnsAreas = new[] { new { UnsAreaId = "area-1", ClusterId = "c1", Name = "Area" } },
@@ -124,7 +166,7 @@ public sealed class OpcUaPublishActorApplyOutcomeTests : RuntimeActorTestBase
                    EquipmentId = equipmentId,
                    DriverInstanceId = (string?)null,
                    UnsLineId = "line-1",
-                    Name = equipmentId,
+                    Name = name,
                    MachineCode = equipmentId.ToUpperInvariant(),
                },
            },
@@ -137,16 +179,20 @@ public sealed class OpcUaPublishActorApplyOutcomeTests : RuntimeActorTestBase
    /// <summary>A sink that optionally throws from <c>EnsureFolder</c> (a swallowed per-node fault).</summary>
    private sealed class FaultingSink(bool throwOnEnsureFolder) : IOpcUaAddressSpaceSink
    {
+        public bool ThrowOnEnsureFolder { get; set; } = throwOnEnsureFolder;
+        public int EnsureFolderCount;
+        public int RebuildCount;
        public void WriteValue(string nodeId, object? value, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { }
        public void WriteAlarmQuality(string alarmNodeId, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm) { }
        public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime occurredUtc, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { }
        public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, AddressSpaceRealm realm, bool isNative = false) { }
        public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName, AddressSpaceRealm realm = AddressSpaceRealm.Uns)
        {
-            if (throwOnEnsureFolder) throw new InvalidOperationException("simulated sink fault");
+            EnsureFolderCount++;
+            if (ThrowOnEnsureFolder) throw new InvalidOperationException("simulated sink fault");
        }
        public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, AddressSpaceRealm realm, string? historianTagname = null, bool isArray = false, uint? arrayLength = null, bool refuseArrayOnScalar = false) { }
-        public void RebuildAddressSpace() { }
+        public void RebuildAddressSpace() => RebuildCount++;
        public void RaiseNodesAddedModelChange(string affectedNodeId, AddressSpaceRealm realm = AddressSpaceRealm.Uns) { }
        public void WireAlarmNotifiers(string alarmNodeId, AddressSpaceRealm alarmRealm, IReadOnlyList<string> notifierFolderNodeIds, AddressSpaceRealm notifierFolderRealm) { }
        public void AddReference(string sourceNodeId, AddressSpaceRealm sourceRealm, string targetNodeId, AddressSpaceRealm targetRealm, string referenceType = "Organizes") { }
@@ -845,7 +845,10 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
    ///     <c>_lastApplied</c>. A rebuild already re-materialised the whole composition, and the applier's
    ///     one-way rebuild ratchet is the resync mechanism for anything it could not place — holding the
    ///     composition back here would make every subsequent deploy re-diff the entire address space as added.
-    ///     Proven by the third apply of the same composition being an empty-plan no-op.
+    ///     Proven by the third apply of the same composition NOT rebuilding again (it diffs to an empty plan).
+    ///     Gitea #601 review: that empty plan is no longer a silent no-op, though — the previous apply was a
+    ///     degraded rebuild, so the identical re-deploy re-ensures the whole composition (idempotently, no
+    ///     rebuild) and thereby repairs the node the rebuild could not place.
    /// </summary>
    [Fact]
    public void Rebuild_with_failures_still_advances_last_applied()
@@ -869,13 +872,24 @@ public sealed class OpcUaPublishActorRebuildTests : RuntimeActorTestBase
        var afterSecond = sink.Calls.Count;

        // Re-deploy the SAME composition. _lastApplied advanced despite the failure, so this diffs to an
-        // empty plan and adds no sink calls at all.
+        // empty plan — no second rebuild (an un-advanced composition would re-diff ChangedEquipment and
+        // rebuild again) — but the degraded rebuild is remembered, so the failed variable is re-ensured.
        sink.FailEnsureVariableFor = null;
        var dep3 = SeedUnsRawDeployment(db, equipmentName: "station1-renamed", referencedTag: "a");
        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep3)));
-        Thread.Sleep(300);
+        AwaitAssert(
+            () => sink.Calls.Skip(afterSecond).ShouldContain("EV:Plant/Modbus/dev1/b"),
+            duration: PresenceBudget);
+        sink.RebuildCalls.ShouldBe(1);

-        sink.Calls.Count.ShouldBe(afterSecond);
+        // Repaired ⇒ genuinely clean now, so a FOURTH identical deploy is the ordinary empty-plan no-op.
+        Thread.Sleep(200);
+        var afterRepair = sink.Calls.Count;
+        var dep4 = SeedUnsRawDeployment(db, equipmentName: "station1-renamed", referencedTag: "a");
+        actor.Tell(new OpcUaPublishActor.RebuildAddressSpace(CorrelationId.NewId(), new DeploymentId(dep4)));
+        Thread.Sleep(300);
+        sink.Calls.Count.ShouldBe(afterRepair);
+        sink.RebuildCalls.ShouldBe(1);
    }

    /// <summary>Seal a deployment whose raw tree is <c>Plant/Modbus/dev1</c> carrying one tag per name in