fix(opcua): carry the condition Comment across a rebuild; the restore report never rolls Time back (#641 review)

Native conditions support AddComment / commented Acknowledge, so the comment (and its ClientUserId) is condition state the rebuild dropped. And the deferred restore report re-stamped Time with the carried instant even when a client method call had reported a newer event in the restore->complete window; it now reports at the node's current Time. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 17:20:13 -04:00
parent 25f02e4102
commit 17dc171940
2 changed files with 45 additions and 6 deletions
@@ -74,7 +74,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    /// event at <see cref="CompleteAddressSpaceApply"/> (deferred until then so every notifier folder the
    /// apply wires receives it). Each entry pins the instance restored, so a condition replaced in between is
    /// not reported. Guarded by <c>Lock</c>.</summary>
-    private readonly List<(RealmKey Key, AlarmConditionState Alarm, DateTime Time)> _restoredConditionReports = new();
+    private readonly List<(RealmKey Key, AlarmConditionState Alarm)> _restoredConditionReports = new();
    /// <summary>Phase C: NodeId → resolved historian tagname for every variable materialised
    /// Historizing. Populated by <see cref="EnsureVariable"/> when a historian tagname is supplied; the
    /// (later) HistoryRead override resolves a HistoryRead request's NodeId against this map. Cleared on
@@ -1459,8 +1459,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    /// <param name="Time">The condition's <c>Time</c> — the instant its current state came about.</param>
    /// <param name="ActiveTransitionTime">When it last changed Active, if the SDK recorded it.</param>
    /// <param name="QualityTimestamp">The source timestamp of its Quality annotation, if present.</param>
+    /// <param name="Comment">The condition's <c>Comment</c> (a client's ack / AddComment text), if any.</param>
+    /// <param name="ClientUserId">Who left that comment, if recorded.</param>
    private sealed record NativeConditionCarryOver(
-        AlarmConditionDelta State, DateTime Time, DateTime? ActiveTransitionTime, DateTime? QualityTimestamp);
+        AlarmConditionDelta State, DateTime Time, DateTime? ActiveTransitionTime, DateTime? QualityTimestamp,
+        LocalizedText? Comment = null, string? ClientUserId = null);

    /// <summary>
    ///     Gitea #641 — read what a native condition node holds right now, so the rebuild can put it back.
@@ -1485,7 +1488,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        ReadConditionDelta(condition),
        condition.Time?.Value ?? DateTime.UtcNow,
        condition.ActiveState?.TransitionTime?.Value,
-        condition.Quality?.SourceTimestamp?.Value);
+        condition.Quality?.SourceTimestamp?.Value,
+        condition.Comment?.Value,
+        condition.ClientUserId?.Value);

    /// <summary>
    ///     Gitea #641 — put a carried state back onto the freshly materialised native condition. MUST be called
@@ -1532,10 +1537,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        alarm.SetSeverity(SystemContext, (EventSeverity)state.MappedSeverity);
        alarm.Message.Value = new LocalizedText(state.Message);
        alarm.Retain.Value = true;
+        if (alarm.Comment is not null && carried.Comment is not null) alarm.Comment.Value = carried.Comment;
+        if (alarm.ClientUserId is not null && carried.ClientUserId is not null) alarm.ClientUserId.Value = carried.ClientUserId;
        alarm.Time.Value = carried.Time;
        alarm.ReceiveTime.Value = carried.Time;

-        _restoredConditionReports.Add((conditionKey, alarm, carried.Time));
+        _restoredConditionReports.Add((conditionKey, alarm));
    }

    /// <summary>One materialised native-alarm condition together with the equipment folders wired as extra
@@ -3376,10 +3383,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        int reported = 0, discarded;
        lock (Lock)
        {
-            foreach (var (key, alarm, time) in _restoredConditionReports)
+            foreach (var (key, alarm) in _restoredConditionReports)
            {
                if (!_alarmConditions.TryGetValue(key, out var live) || !ReferenceEquals(live, alarm)) continue;
-                ReportConditionEvent(alarm, time);
+                // The node's CURRENT Time: the carried instant when nothing reported since the restore, or the
+                // newer instant of a report that did (a client method call on an SDK thread can land between the
+                // restore and here) — never roll the condition's Time back.
+                ReportConditionEvent(alarm, alarm.Time.Value);
                reported++;
            }

@@ -94,6 +94,35 @@ public sealed class NodeManagerNativeAlarmRebuildCarryOverTests : IDisposable
        await host.DisposeAsync();
    }

+    /// <summary>
+    ///     Gitea #641 review — the comment a client left with its acknowledgement is condition state too (native
+    ///     conditions support AddComment), and an event the condition reported AFTER the restore (a client method
+    ///     call landing on an SDK thread before the apply completes) must not have its newer <c>Time</c> rolled
+    ///     back by the deferred restore report.
+    /// </summary>
+    [Trait("Category", "Unit")]
+    [Fact]
+    public async Task The_comment_is_carried_and_a_newer_report_is_not_rolled_back()
+    {
+        var (host, nm) = await BootAsync();
+        var raisedAt = new DateTime(2026, 10, 8, 9, 0, 0, DateTimeKind.Utc);
+        Materialise(nm, native: true);
+        nm.WriteAlarmCondition(Alarm, Fault(acked: false), raisedAt, AddressSpaceRealm.Raw);
+        Condition(nm).Comment.Value = new LocalizedText("on it");
+
+        nm.RebuildAddressSpace();
+        Materialise(nm, native: true);
+        var later = raisedAt.AddMinutes(5);
+        nm.WriteAlarmQuality(Alarm, StatusCodes.BadNoCommunication, later, AddressSpaceRealm.Raw); // reports at `later`
+        nm.CompleteAddressSpaceApply();
+
+        var restored = Condition(nm);
+        restored.Comment.Value.Text.ShouldBe("on it");
+        restored.Time.Value.ShouldBe(later, "the restore report must not stamp the condition back to the raise time");
+
+        await host.DisposeAsync();
+    }
+
    /// <summary>Falsification control: a condition in the no-event position comes back exactly as a fresh
    /// materialise builds it, and reports nothing.</summary>
    [Trait("Category", "Unit")]