fix(adminui): snapshot already old at dialog open is stale immediately (#857)
The Controls panel primed its per-node view from the snapshot store with ReceivedUtc = now, so a node that stopped publishing before the dialog opened looked live for the first 30 s stale window. InMemoryDriverStatusSnapshotStore now records each snapshot's LOCAL receive time (TimeProvider-injectable) and exposes it via GetAllWithReceiveTime(); the panel primes through the pure seam DriverStatusByNode.Prime/PrimedReceiveTime, which keeps the store's receive time (clamped to now; still skew-proof). A store that does not track one (default interface impl -> null) falls back to the publish time only when that is already older than the stale window, else now. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk
This commit is contained in:
+4
-4
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user