Merge #857 Controls panel: snapshot already old at dialog open is stale immediately

Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk
2026-11-11 09:50:35 -05:00
parent b4ab403c7b 2bd3a6c3c8
commit fdc39009fc
5 changed files with 198 additions and 15 deletions
@@ -274,11 +274,11 @@
        try
        {
            // Subscribe BEFORE priming so a publish racing the read is not missed; a primed copy that lands after
-            // a newer racing live one is dropped by Upsert (it keeps the later PublishedUtc per node).
+            // a newer racing live one is dropped by Upsert (it keeps the later PublishedUtc per node). Primed
+            // copies keep the store's own receive time (#857): stamping them "now" showed a node that stopped
+            // publishing before the dialog opened as live for the first StaleAfter.
            StatusStore.SnapshotChanged += OnSnapshotChanged;
-            var primedUtc = DateTime.UtcNow;
-            foreach (var snap in StatusStore.GetAll())
-                _byNode.Upsert(snap, primedUtc);
+            _byNode.Prime(StatusStore.GetAllWithReceiveTime(), DateTime.UtcNow, StaleAfter);

            // Prime + subscribe the resilience store (same in-process read pattern as the health store).
            ResilienceStore.SnapshotChanged += OnResilienceChanged;
@@ -1,3 +1,4 @@
+using ZB.MOM.WW.OtOpcUa.AdminUI.Hubs;
 using ZB.MOM.WW.OtOpcUa.Commons.Messages.Drivers;

 namespace ZB.MOM.WW.OtOpcUa.AdminUI.Hosts;
@@ -67,6 +68,43 @@ public sealed class DriverStatusByNode
        return true;
    }

+    /// <summary>
+    ///     Primes the view from the snapshot store's current contents (lmxopcua#857). Each snapshot keeps the
+    ///     store's own receive time (<see cref="PrimedReceiveTime"/>) rather than "now", so a node that stopped
+    ///     publishing before the dialog opened is stale immediately instead of looking live for a full
+    ///     <paramref name="staleAfter"/> window. Returns true when the view changed.
+    /// </summary>
+    /// <param name="stored">The store's entries (<see cref="IDriverStatusSnapshotStore.GetAllWithReceiveTime"/>).</param>
+    /// <param name="nowUtc">The local time the view is primed at.</param>
+    /// <param name="staleAfter">The view's stale window (only used when the store has no receive time).</param>
+    public bool Prime(IEnumerable<StoredDriverHealth> stored, DateTime nowUtc, TimeSpan staleAfter)
+    {
+        var changed = false;
+        foreach (var entry in stored)
+            changed |= Upsert(entry.Snapshot, PrimedReceiveTime(entry, nowUtc, staleAfter));
+        return changed;
+    }
+
+    /// <summary>
+    ///     The receive time a snapshot primed from the store is aged from (lmxopcua#857). Prefers the store's
+    ///     LOCAL receive time — same clock as live updates, so still skew-proof — clamped to
+    ///     <paramref name="nowUtc"/>. A store that does not track receive times falls back to the publisher's
+    ///     <see cref="DriverHealthChanged.PublishedUtc"/> only when that is already older than the stale window
+    ///     (so a long-silent node is stale at once; a peer clock running behind can at worst dim a live row until
+    ///     its next publish), else to <paramref name="nowUtc"/> (the pre-#857 behaviour).
+    /// </summary>
+    /// <param name="stored">The store entry.</param>
+    /// <param name="nowUtc">The local time the view is primed at.</param>
+    /// <param name="staleAfter">The view's stale window.</param>
+    public static DateTime PrimedReceiveTime(StoredDriverHealth stored, DateTime nowUtc, TimeSpan staleAfter)
+    {
+        if (stored.ReceivedUtc is { } received)
+            return received > nowUtc ? nowUtc : received;
+
+        var published = stored.Snapshot.PublishedUtc;
+        return nowUtc - published > staleAfter ? published : nowUtc;
+    }
+
    /// <summary>
    ///     A point-in-time copy of every node's entry, ordered by node id (ordinal, case-insensitive) with the
    ///     unlabelled (pre-#847) entry last.
@@ -27,6 +27,17 @@ public interface IDriverStatusSnapshotStore
    /// <returns>One last-known snapshot per driver instance PER reporting node (lmxopcua#847).</returns>
    IReadOnlyCollection<DriverHealthChanged> GetAll();

+    /// <summary>
+    ///     <see cref="GetAll"/> plus, per snapshot, when THIS process stored it (local clock — lmxopcua#857). A
+    ///     consumer that primes a view from the store (the Controls dialog's <c>DriverStatusPanel</c>) needs the
+    ///     snapshot's real age: stamping a primed copy "received now" hid a node that had stopped publishing long
+    ///     before the dialog opened behind a full stale window of apparent freshness. The default implementation
+    ///     reports no receive time (<see langword="null"/>) for a store that does not track one.
+    /// </summary>
+    /// <returns>One entry per (driver instance, reporting node), as <see cref="GetAll"/>.</returns>
+    IReadOnlyCollection<StoredDriverHealth> GetAllWithReceiveTime() =>
+        GetAll().Select(s => new StoredDriverHealth(s, null)).ToArray();
+
    /// <summary>
    ///     Evicts every snapshot whose driver instance is not in <paramref name="configuredInstanceIds"/>
    ///     <b>and</b> whose last publish is older than <paramref name="olderThanUtc"/>. The stores have no other
@@ -53,3 +64,9 @@ public interface IDriverStatusSnapshotStore
    /// </summary>
    event Action<DriverHealthChanged>? SnapshotChanged;
 }
+
+/// <summary>A stored driver-health snapshot and when the store received it (lmxopcua#857).</summary>
+/// <param name="Snapshot">The last-known snapshot.</param>
+/// <param name="ReceivedUtc">When this process's store received it (local clock); <see langword="null"/> when the
+///     store does not track receive times.</param>
+public sealed record StoredDriverHealth(DriverHealthChanged Snapshot, DateTime? ReceivedUtc);
@@ -16,14 +16,23 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Hubs;
 /// </remarks>
 public sealed class InMemoryDriverStatusSnapshotStore : IDriverStatusSnapshotStore
 {
-    private readonly ConcurrentDictionary<(string DriverInstanceId, string NodeId), DriverHealthChanged> _byInstance = new();
+    // Each entry carries the LOCAL receive time (lmxopcua#857) so a view primed from the store can age a snapshot
+    // from when this process actually got it, not from when the view happened to read it.
+    private readonly ConcurrentDictionary<(string DriverInstanceId, string NodeId), StoredDriverHealth> _byInstance = new();
    private readonly ILogger<InMemoryDriverStatusSnapshotStore>? _logger;
+    private readonly TimeProvider _time;
    private readonly FailureLogThrottle _failureLog = new();

    /// <summary>Creates the store.</summary>
    /// <param name="logger">Optional logger for a subscriber that throws during a raise.</param>
-    public InMemoryDriverStatusSnapshotStore(ILogger<InMemoryDriverStatusSnapshotStore>? logger = null)
-        => _logger = logger;
+    /// <param name="time">Clock that stamps each snapshot's local receive time; <see cref="TimeProvider.System"/>
+    ///     when null.</param>
+    public InMemoryDriverStatusSnapshotStore(
+        ILogger<InMemoryDriverStatusSnapshotStore>? logger = null, TimeProvider? time = null)
+    {
+        _logger = logger;
+        _time = time ?? TimeProvider.System;
+    }

    /// <summary>Raised whenever a driver status snapshot is stored or updated.</summary>
    public event Action<DriverHealthChanged>? SnapshotChanged;
@@ -33,7 +42,7 @@ public sealed class InMemoryDriverStatusSnapshotStore : IDriverStatusSnapshotSto
    /// <inheritdoc />
    public void Upsert(DriverHealthChanged snapshot)
    {
-        _byInstance[KeyOf(snapshot)] = snapshot;
+        _byInstance[KeyOf(snapshot)] = new StoredDriverHealth(snapshot, _time.GetUtcNow().UtcDateTime);
        // Capture-then-invoke so a concurrent unsubscribe can't null the delegate mid-raise; one
        // subscriber at a time so a throwing one cannot starve the rest (or escape onto the bridge actor).
        SafeMulticast.Invoke(SnapshotChanged, snapshot,
@@ -44,11 +53,11 @@ public sealed class InMemoryDriverStatusSnapshotStore : IDriverStatusSnapshotSto
    public int EvictUnconfiguredStale(IReadOnlySet<string> configuredInstanceIds, DateTime olderThanUtc)
    {
        var removed = 0;
-        foreach (var (key, snapshot) in _byInstance)
+        foreach (var (key, entry) in _byInstance)
        {
-            if (configuredInstanceIds.Contains(key.DriverInstanceId) || snapshot.PublishedUtc >= olderThanUtc) continue;
-            // Remove only the exact snapshot inspected: a fresh publish that raced in is kept.
-            if (_byInstance.TryRemove(new KeyValuePair<(string, string), DriverHealthChanged>(key, snapshot))) removed++;
+            if (configuredInstanceIds.Contains(key.DriverInstanceId) || entry.Snapshot.PublishedUtc >= olderThanUtc) continue;
+            // Remove only the exact entry inspected: a fresh publish that raced in is kept.
+            if (_byInstance.TryRemove(new KeyValuePair<(string, string), StoredDriverHealth>(key, entry))) removed++;
        }

        return removed;
@@ -58,8 +67,9 @@ public sealed class InMemoryDriverStatusSnapshotStore : IDriverStatusSnapshotSto
    public bool TryGet(string driverInstanceId, out DriverHealthChanged snapshot)
    {
        DriverHealthChanged? latest = null;
-        foreach (var (key, s) in _byInstance)
+        foreach (var (key, entry) in _byInstance)
        {
+            var s = entry.Snapshot;
            if (!string.Equals(key.DriverInstanceId, driverInstanceId, StringComparison.Ordinal)) continue;
            if (latest is null || s.PublishedUtc > latest.PublishedUtc) latest = s;
        }
@@ -69,5 +79,8 @@ public sealed class InMemoryDriverStatusSnapshotStore : IDriverStatusSnapshotSto
    }

    /// <inheritdoc />
-    public IReadOnlyCollection<DriverHealthChanged> GetAll() => _byInstance.Values.ToArray();
+    public IReadOnlyCollection<DriverHealthChanged> GetAll() => _byInstance.Values.Select(e => e.Snapshot).ToArray();
+
+    /// <inheritdoc />
+    public IReadOnlyCollection<StoredDriverHealth> GetAllWithReceiveTime() => _byInstance.Values.ToArray();
 }
@@ -129,7 +129,9 @@ public sealed class DriverStatusByNodeTests
        var src = AdminUiSource.Read("Components", "Shared", "Drivers", "DriverStatusPanel.razor");

        src.ShouldContain("new DriverStatusByNode(DriverInstanceId)");
-        src.ShouldContain("StatusStore.GetAll()");
+        src.ShouldContain("_byNode.Prime(StatusStore.GetAllWithReceiveTime(), DateTime.UtcNow, StaleAfter)");
+        // Priming must not stamp the store's snapshots "received now" (#857).
+        src.ShouldNotContain("StatusStore.GetAll()");
        src.ShouldNotContain("StatusStore.TryGet(");
        src.ShouldContain("@foreach (var n in nodes)");
        src.ShouldContain("@n.Label");
@@ -137,4 +139,117 @@ public sealed class DriverStatusByNodeTests
        // The old single-snapshot field must be gone, or a half-migrated panel could still render one node.
        src.ShouldNotContain("_snapshot");
    }
+
+    // ---- lmxopcua#857: a snapshot already old when the dialog opens is stale immediately ----
+
+    private static readonly TimeSpan StaleAfter = TimeSpan.FromSeconds(30);
+
+    /// <summary>A hand-driven clock — the test project has no <c>FakeTimeProvider</c> package reference.</summary>
+    private sealed class ManualTimeProvider(DateTime startUtc) : TimeProvider
+    {
+        public DateTime UtcNow { get; set; } = startUtc;
+
+        public override DateTimeOffset GetUtcNow() => new(UtcNow, TimeSpan.Zero);
+    }
+
+    [Fact]
+    public void The_store_records_its_local_receive_time_per_node()
+    {
+        var clock = new ManualTimeProvider(T0);
+        var store = new InMemoryDriverStatusSnapshotStore(time: clock);
+        store.Upsert(Snap("drv", "Healthy", T0.AddHours(-3), "central-1")); // peer clock far behind: irrelevant
+        clock.UtcNow = T0.AddSeconds(7);
+        store.Upsert(Snap("drv", "Healthy", T0.AddHours(-3), "central-2"));
+
+        var all = store.GetAllWithReceiveTime();
+        all.Single(e => e.Snapshot.ReportingNodeId == "central-1").ReceivedUtc.ShouldBe(T0);
+        all.Single(e => e.Snapshot.ReportingNodeId == "central-2").ReceivedUtc.ShouldBe(T0.AddSeconds(7));
+        store.GetAll().Count.ShouldBe(2);
+    }
+
+    [Fact]
+    public void A_snapshot_the_store_received_long_before_the_dialog_opened_is_stale_at_once()
+    {
+        // The docker-dev repro: central-2 stopped 1m30s before the panel was reopened. It must dim immediately,
+        // while central-1 (still publishing) stays live.
+        var clock = new ManualTimeProvider(T0);
+        var store = new InMemoryDriverStatusSnapshotStore(time: clock);
+        store.Upsert(Snap("drv", "Healthy", T0, "central-2"));
+        clock.UtcNow = T0.AddSeconds(88);
+        store.Upsert(Snap("drv", "Healthy", T0.AddSeconds(88), "central-1"));
+
+        var openedAt = T0.AddSeconds(90);
+        var view = new DriverStatusByNode("drv");
+        view.Prime(store.GetAllWithReceiveTime(), openedAt, StaleAfter).ShouldBeTrue();
+
+        var entries = view.Entries;
+        entries.Single(e => e.NodeId == "central-2").IsStale(openedAt, StaleAfter).ShouldBeTrue();
+        entries.Single(e => e.NodeId == "central-1").IsStale(openedAt, StaleAfter).ShouldBeFalse();
+    }
+
+    [Fact]
+    public void A_fresh_snapshot_loaded_on_open_is_not_stale_even_when_the_peer_clock_is_far_behind()
+    {
+        // Positive control + skew-proofing: the age comes from the store's LOCAL clock, so a publisher whose clock
+        // is an hour behind does not dim a snapshot this process received a second ago.
+        var clock = new ManualTimeProvider(T0);
+        var store = new InMemoryDriverStatusSnapshotStore(time: clock);
+        store.Upsert(Snap("drv", "Healthy", T0.AddHours(-1), "central-1"));
+
+        var openedAt = T0.AddSeconds(1);
+        var view = new DriverStatusByNode("drv");
+        view.Prime(store.GetAllWithReceiveTime(), openedAt, StaleAfter);
+
+        var entry = view.Entries.ShouldHaveSingleItem();
+        entry.ReceivedUtc.ShouldBe(T0);
+        entry.IsStale(openedAt, StaleAfter).ShouldBeFalse();
+        entry.IsStale(T0.AddSeconds(31), StaleAfter).ShouldBeTrue(); // and it does age out on the normal schedule
+    }
+
+    [Fact]
+    public void A_live_update_after_priming_un_stales_the_node()
+    {
+        var view = new DriverStatusByNode("drv");
+        view.Prime([new StoredDriverHealth(Snap("drv", "Healthy", T0, "central-2"), T0)], T0.AddSeconds(90), StaleAfter);
+        view.Entries.Single().IsStale(T0.AddSeconds(90), StaleAfter).ShouldBeTrue();
+
+        view.Upsert(Snap("drv", "Healthy", T0.AddSeconds(91), "central-2"), T0.AddSeconds(91)).ShouldBeTrue();
+        view.Entries.Single().IsStale(T0.AddSeconds(92), StaleAfter).ShouldBeFalse();
+    }
+
+    [Fact]
+    public void A_store_receive_time_in_the_future_is_clamped_to_now()
+    {
+        var stored = new StoredDriverHealth(Snap("drv", "Healthy", T0, "central-1"), T0.AddMinutes(5));
+        DriverStatusByNode.PrimedReceiveTime(stored, T0, StaleAfter).ShouldBe(T0);
+    }
+
+    [Fact]
+    public void Without_a_store_receive_time_an_old_publish_seeds_the_age_and_a_recent_one_is_now()
+    {
+        var old = new StoredDriverHealth(Snap("drv", "Healthy", T0.AddSeconds(-90), "central-2"), null);
+        DriverStatusByNode.PrimedReceiveTime(old, T0, StaleAfter).ShouldBe(T0.AddSeconds(-90));
+
+        // Positive control: a recent publish (or a peer clock running ahead) is treated as received now.
+        var recent = new StoredDriverHealth(Snap("drv", "Healthy", T0.AddSeconds(-5), "central-1"), null);
+        DriverStatusByNode.PrimedReceiveTime(recent, T0, StaleAfter).ShouldBe(T0);
+        var ahead = new StoredDriverHealth(Snap("drv", "Healthy", T0.AddMinutes(10), "central-1"), null);
+        DriverStatusByNode.PrimedReceiveTime(ahead, T0, StaleAfter).ShouldBe(T0);
+    }
+
+    [Fact]
+    public void The_default_interface_receive_time_is_null_for_a_store_that_does_not_track_one()
+    {
+        IDriverStatusSnapshotStore store = new GetAllOnlyStore([Snap("drv", "Healthy", T0, "central-1")]);
+        store.GetAllWithReceiveTime().ShouldHaveSingleItem().ReceivedUtc.ShouldBeNull();
+    }
+
+    private sealed class GetAllOnlyStore(IReadOnlyCollection<DriverHealthChanged> all) : IDriverStatusSnapshotStore
+    {
+        public event Action<DriverHealthChanged>? SnapshotChanged { add { } remove { } }
+        public void Upsert(DriverHealthChanged snapshot) { }
+        public bool TryGet(string driverInstanceId, out DriverHealthChanged snapshot) { snapshot = null!; return false; }
+        public IReadOnlyCollection<DriverHealthChanged> GetAll() => all;
+        public int EvictUnconfiguredStale(IReadOnlySet<string> configuredInstanceIds, DateTime olderThanUtc) => 0;
+    }
 }