fix(dashboard): generation-tagged feed subscribers survive pump teardown races; drain-timeout observability

src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/DashboardPageBase.cs

@@ -1,4 +1,5 @@
 using Microsoft.AspNetCore.Components;
+using Microsoft.Extensions.Logging;
 
 namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Components;
 
@@ -31,6 +32,10 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable
     [Inject]
     protected IDashboardSnapshotFeed SnapshotFeed { get; set; } = null!;
 
+    /// <summary>Logger used to report a snapshot subscription that ended or would not drain.</summary>
+    [Inject]
+    protected ILogger<DashboardPageBase>? Logger { get; set; }
+
     /// <summary>
     /// The most recent gateway metric snapshot. Synchronously seeded from
     /// <see cref="IDashboardSnapshotService.GetSnapshot"/> for the very first
@@ -62,9 +67,21 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable
                 await _watchTask.WaitAsync(WatchDrainTimeout).ConfigureAwait(false);
             }
         }
+        catch (TimeoutException)
+        {
+            // Accepted limitation: the abandoned loop still holds its feed subscription, so
+            // the feed's idle gate stays open until it does unwind. There is no way to force
+            // a detach — the loop is parked on a dispatcher that is not draining — so the
+            // warning is the operator's only signal that a circuit teardown wedged.
+            Logger?.LogWarning(
+                "Dashboard page {Page} did not release its snapshot subscription within {Timeout}; "
+                + "the shared snapshot feed stays active until it unwinds.",
+                GetType().Name,
+                WatchDrainTimeout);
+        }
         catch
         {
-            // Disposal-time errors (including a drain timeout) are best-effort.
+            // Other disposal-time errors are best-effort.
         }
 
         _watchCancellation.Dispose();
@@ -87,10 +104,15 @@ public abstract class DashboardPageBase : ComponentBase, IAsyncDisposable
         {
             // The page is going away.
         }
-        catch
+        catch (Exception error)
         {
-            // The feed is best-effort: the last rendered snapshot stays on screen and
+            // The feed is best-effort: the last rendered snapshot stays on screen and the
-            // the snapshot service keeps serving GetSnapshot() for the next page load.
+            // snapshot service keeps serving GetSnapshot() for the next page load. Logged
+            // once here, on the way out of the loop — never per snapshot.
+            Logger?.LogWarning(
+                error,
+                "Live snapshot updates ended for dashboard page {Page}; it keeps the last rendered snapshot.",
+                GetType().Name);
         }
     }
 }

src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSnapshotFeed.cs

@@ -12,18 +12,30 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard;
 /// snapshot cost by the number of open pages.
 /// </summary>
 /// <remarks>
-/// The pump is idle-gated: it starts when the subscriber count goes 0 → 1 and is cancelled
+/// <para>
-/// and awaited when it goes 1 → 0, so an unwatched gateway runs no timer and builds no
+/// The pump is idle-gated: it starts when the first subscriber arrives and is cancelled and
+/// awaited when the last one leaves, so an unwatched gateway runs no timer and builds no
 /// snapshots. Successive pumps are chained through <c>_pumpTask</c>, so a rapid
 /// unsubscribe/resubscribe restarts a fresh pump without ever running two enumerations at
 /// once.
+/// </para>
+/// <para>
+/// Every subscriber is tagged with the pump generation it joined under, and a dying pump only
+/// ever detaches its own generation. A pump ends its generation the instant its source fails
+/// or completes — before the (possibly slow) enumerator disposal — so a subscriber arriving
+/// while a pump unwinds starts a fresh generation instead of silently attaching to a dead
+/// pump that is about to detach everybody and leave nobody watching.
+/// </para>
 /// </remarks>
 public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
 {
+    /// <summary>Generation value meaning "no pump is accepting subscribers".</summary>
+    private const long NoGeneration = 0;
+
     private readonly IDashboardSnapshotService _snapshotService;
     private readonly ILogger<DashboardSnapshotFeed> _logger;
     private readonly object _gate = new();
-    private readonly List<Channel<DashboardSnapshot>> _subscribers = [];
+    private readonly List<Subscription> _subscribers = [];
 
     /// <summary>
     /// The most recent pump, completed while idle. A starting pump awaits its predecessor
@@ -31,9 +43,15 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
     /// </summary>
     private Task _pumpTask = Task.CompletedTask;
 
-    /// <summary>Cancellation for the live pump; null when no pump is running or one is being torn down.</summary>
+    /// <summary>Cancellation for the live pump; null when no generation is accepting subscribers.</summary>
     private CancellationTokenSource? _pumpCancellation;
 
+    /// <summary>The generation new subscribers join, or <see cref="NoGeneration"/> when no pump is live.</summary>
+    private long _generation = NoGeneration;
+
+    /// <summary>Last generation handed out; only ever incremented under <c>_gate</c>.</summary>
+    private long _lastGeneration = NoGeneration;
+
     /// <summary>Initializes a new instance of the <see cref="DashboardSnapshotFeed"/> class.</summary>
     /// <param name="snapshotService">Snapshot source to multicast.</param>
     /// <param name="logger">Optional logger for pump faults.</param>
@@ -60,7 +78,7 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
                 SingleWriter = false,
             });
 
-        Subscribe(channel);
+        Subscription subscription = Subscribe(channel);
         try
         {
             await foreach (DashboardSnapshot snapshot in channel.Reader
@@ -74,47 +92,40 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
         {
             // Untokened on purpose: teardown must run to completion even when this
             // subscriber is unwinding because its own token fired.
-            await UnsubscribeAsync(channel).ConfigureAwait(false);
+            await UnsubscribeAsync(subscription).ConfigureAwait(false);
         }
     }
 
-    private void Subscribe(Channel<DashboardSnapshot> channel)
+    private Subscription Subscribe(Channel<DashboardSnapshot> channel)
     {
         lock (_gate)
         {
-            _subscribers.Add(channel);
+            // A live generation is joined; otherwise this subscriber starts one. Keying on
-            if (_subscribers.Count != 1)
+            // "is a generation live" rather than "is this the first subscriber" is what makes
-            {
+            // a subscriber arriving while a pump unwinds start a fresh pump for itself.
-                return;
+            long generation = _pumpCancellation is null ? StartPumpLocked() : _generation;
-            }
+            Subscription subscription = new(channel, generation);
-
+            _subscribers.Add(subscription);
-            CancellationTokenSource cancellation = new();
+            return subscription;
-            Task previous = _pumpTask;
-            _pumpCancellation = cancellation;
-
-            // Task.Run, not a direct call: an async iterator runs synchronously up to its
-            // first suspension, and the first pull of the underlying watch can read the API
-            // key table. That must not run on the subscribing circuit's thread, let alone
-            // while this lock is held.
-            _pumpTask = Task.Run(() => PumpAsync(previous, cancellation, cancellation.Token));
         }
     }
 
-    private async Task UnsubscribeAsync(Channel<DashboardSnapshot> channel)
+    private async Task UnsubscribeAsync(Subscription subscription)
     {
         CancellationTokenSource? cancellation;
         Task pump;
         lock (_gate)
         {
-            if (!_subscribers.Remove(channel) || _subscribers.Count != 0)
+            if (!_subscribers.Remove(subscription) || _subscribers.Count != 0)
             {
-                // Either the pump already dropped this channel (it completed or faulted
+                // Either the pump already detached this subscription (it completed or
-                // and reset itself), or other viewers are still watching.
+                // faulted), or other viewers are still watching.
                 return;
             }
 
             cancellation = _pumpCancellation;
             _pumpCancellation = null;
+            _generation = NoGeneration;
             pump = _pumpTask;
         }
 
@@ -124,7 +135,7 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
         }
         catch (ObjectDisposedException)
         {
-            // The pump reset itself and disposed its own cancellation source first.
+            // The pump ended on its own and disposed its cancellation source first.
         }
 
         try
@@ -138,33 +149,90 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
         }
     }
 
-    private async Task PumpAsync(Task previous, CancellationTokenSource cancellation, CancellationToken cancellationToken)
+    /// <summary>
+    /// Starts a pump generation. Must be called while holding <c>_gate</c>; the caller adds
+    /// the subscribers that belong to the returned generation.
+    /// </summary>
+    /// <returns>The new generation identifier.</returns>
+    private long StartPumpLocked()
+    {
+        long generation = ++_lastGeneration;
+        CancellationTokenSource cancellation = new();
+        Task previous = _pumpTask;
+        _generation = generation;
+        _pumpCancellation = cancellation;
+
+        // Task.Run, not a direct call: an async iterator runs synchronously up to its
+        // first suspension, and the first pull of the underlying watch can read the API
+        // key table. That must not run on the subscribing circuit's thread, let alone
+        // while this lock is held.
+        _pumpTask = Task.Run(() => PumpAsync(generation, previous, cancellation, cancellation.Token));
+        return generation;
+    }
+
+    private async Task PumpAsync(
+        long generation,
+        Task previous,
+        CancellationTokenSource cancellation,
+        CancellationToken cancellationToken)
     {
         try
         {
             // Never overlap with the enumeration this pump replaces.
             await previous.ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
 
-            await foreach (DashboardSnapshot snapshot in _snapshotService
+            // Enumerated by hand rather than with await foreach so the generation can be
+            // ended the moment the source fails or completes — await foreach would run the
+            // enumerator's disposal first, and a subscriber arriving during that disposal
+            // would join a generation that is already doomed.
+            IAsyncEnumerator<DashboardSnapshot> snapshots = _snapshotService
                 .WatchSnapshotsAsync(cancellationToken)
-                .ConfigureAwait(false))
+                .GetAsyncEnumerator(cancellationToken);
+            try
             {
-                Broadcast(snapshot);
+                while (true)
+                {
+                    bool moved;
+                    try
+                    {
+                        moved = await snapshots.MoveNextAsync().ConfigureAwait(false);
+                    }
+                    catch
+                    {
+                        EndGeneration(generation);
+                        throw;
                     }
 
-            // The source completed on its own; hand the completion to the subscribers
+                    if (!moved)
-            // and re-arm so the next one starts a fresh enumeration.
+                    {
-            Reset(cancellation, error: null);
+                        EndGeneration(generation);
+                        break;
+                    }
+
+                    Broadcast(generation, snapshots.Current);
+                }
+            }
+            finally
+            {
+                await snapshots.DisposeAsync().ConfigureAwait(false);
+            }
+
+            // The source completed on its own; hand the completion to this generation's
+            // subscribers and re-arm so the next one starts a fresh enumeration.
+            Reset(generation, error: null);
         }
         catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
         {
-            // Last subscriber left: the unsubscribing caller already detached the
+            // The production DashboardSnapshotService swallows cancellation and yield-breaks,
-            // channels and cleared the pump state.
+            // so normal teardown exits through the fall-through above (with an ownership-checked
+            // Reset that finds no subscribers); an implementation that propagates the token
+            // instead exits here. Both shapes end the generation exactly once.
+            EndGeneration(generation);
         }
         catch (Exception error)
         {
             _logger.LogWarning(error, "Dashboard snapshot feed stopped; the next subscriber restarts it.");
-            Reset(cancellation, error);
+            Reset(generation, error);
         }
         finally
         {
@@ -172,45 +240,101 @@ public sealed class DashboardSnapshotFeed : IDashboardSnapshotFeed
         }
     }
 
-    private void Broadcast(DashboardSnapshot snapshot)
+    private void Broadcast(long generation, DashboardSnapshot snapshot)
     {
         lock (_gate)
         {
-            foreach (Channel<DashboardSnapshot> subscriber in _subscribers)
+            foreach (Subscription subscriber in _subscribers)
             {
+                if (subscriber.Generation != generation)
+                {
+                    continue;
+                }
+
                 // Bounded/DropOldest: always accepted unless the channel is completed.
-                subscriber.Writer.TryWrite(snapshot);
+                subscriber.Channel.Writer.TryWrite(snapshot);
             }
         }
     }
 
     /// <summary>
-    /// Detaches every subscriber and clears the pump state so the next subscriber starts a
+    /// Stops handing <paramref name="generation"/> to new subscribers. Called the instant a
-    /// new enumeration. The detached subscribers observe <paramref name="error"/> (or a
+    /// pump's source fails or completes, before its enumerator is disposed.
-    /// clean end of stream) from their own <c>WatchAsync</c>.
     /// </summary>
-    /// <param name="cancellation">The calling pump's cancellation source, used as its ownership token.</param>
+    /// <param name="generation">The generation that has ended.</param>
-    /// <param name="error">Failure to surface, or null when the source completed cleanly.</param>
+    private void EndGeneration(long generation)
-    private void Reset(CancellationTokenSource cancellation, Exception? error)
     {
-        Channel<DashboardSnapshot>[] detached;
         lock (_gate)
         {
-            if (!ReferenceEquals(_pumpCancellation, cancellation))
+            EndGenerationLocked(generation);
+        }
+    }
+
+    /// <summary>Clears the live-pump state if <paramref name="generation"/> still owns it.</summary>
+    /// <param name="generation">The generation that has ended.</param>
+    private void EndGenerationLocked(long generation)
+    {
+        if (_generation != generation)
         {
-                // A newer pump (or an in-flight teardown) owns the state now: its
-                // subscribers must not be detached by this pump's exit.
             return;
         }
 
-            detached = _subscribers.ToArray();
+        _generation = NoGeneration;
-            _subscribers.Clear();
         _pumpCancellation = null;
     }
 
-        foreach (Channel<DashboardSnapshot> subscriber in detached)
+    /// <summary>
+    /// Detaches the subscribers of a finished generation and re-arms the feed. Subscribers of
+    /// any other generation are left alone — they belong to a pump that is still running (or
+    /// about to), so a dying pump must not take them down with it.
+    /// </summary>
+    /// <param name="generation">The generation whose subscribers are being detached.</param>
+    /// <param name="error">Failure to surface, or null when the source completed cleanly.</param>
+    private void Reset(long generation, Exception? error)
     {
-            subscriber.Writer.TryComplete(error);
+        List<Channel<DashboardSnapshot>> detached = [];
+        lock (_gate)
+        {
+            for (int index = _subscribers.Count - 1; index >= 0; index--)
+            {
+                if (_subscribers[index].Generation != generation)
+                {
+                    continue;
+                }
+
+                detached.Add(_subscribers[index].Channel);
+                _subscribers.RemoveAt(index);
+            }
+
+            EndGenerationLocked(generation);
+
+            if (_subscribers.Count > 0 && _pumpCancellation is null)
+            {
+                // Belt and braces: subscribers left with no live pump would be frozen for
+                // good, because only a subscriber that finds no generation starts one.
+                long restarted = StartPumpLocked();
+                foreach (Subscription subscriber in _subscribers)
+                {
+                    subscriber.Generation = restarted;
                 }
             }
         }
+
+        foreach (Channel<DashboardSnapshot> channel in detached)
+        {
+            channel.Writer.TryComplete(error);
+        }
+    }
+
+    /// <summary>One viewer's delivery channel plus the pump generation serving it.</summary>
+    /// <param name="channel">Delivery channel for this viewer.</param>
+    /// <param name="generation">Pump generation this viewer joined under.</param>
+    private sealed class Subscription(Channel<DashboardSnapshot> channel, long generation)
+    {
+        /// <summary>Gets the viewer's delivery channel.</summary>
+        public Channel<DashboardSnapshot> Channel { get; } = channel;
+
+        /// <summary>Gets or sets the pump generation currently serving this viewer.</summary>
+        public long Generation { get; set; } = generation;
+    }
+}

src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotFeedTests.cs

@@ -201,6 +201,59 @@ public sealed class DashboardSnapshotFeedTests
         await DrainAsync(second, secondMove);
     }
 
+    /// <summary>
+    /// The race the generation tagging exists for: a page subscribes in the window between
+    /// the source failing and the dying pump detaching its subscribers. Without generations
+    /// the newcomer joined the doomed pump, was detached with its error, and no pump ever
+    /// restarted (only a first subscriber started one) — that page froze for good.
+    /// </summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task WatchAsync_WhenASubscriberJoinsWhileAFaultedPumpUnwinds_IsServedByAFreshPump()
+    {
+        FakeSnapshotService service = new();
+        service.HoldDisposal();
+        DashboardSnapshotFeed feed = new(service);
+
+        using CancellationTokenSource firstCancellation = new();
+        IAsyncEnumerator<DashboardSnapshot> first =
+            feed.WatchAsync(firstCancellation.Token).GetAsyncEnumerator(firstCancellation.Token);
+        Task<bool> firstMove = first.MoveNextAsync().AsTask();
+        await WaitUntilAsync(() => service.EnumerationCount >= 1);
+
+        service.Fault(new InvalidOperationException("simulated snapshot source failure"));
+
+        // The pump has observed the failure and is parked disposing the enumerator — the
+        // exact window in which a page used to attach itself to a doomed pump.
+        await service.DisposalReached.WaitAsync(TestTimeout);
+
+        using CancellationTokenSource secondCancellation = new();
+        IAsyncEnumerator<DashboardSnapshot> second =
+            feed.WatchAsync(secondCancellation.Token).GetAsyncEnumerator(secondCancellation.Token);
+
+        // An async iterator body runs synchronously up to its first await, so the
+        // subscription is registered by the time MoveNextAsync hands back its task.
+        Task<bool> secondMove = second.MoveNextAsync().AsTask();
+
+        service.ReleaseDisposal();
+
+        // The subscriber that was there when the source broke still learns about it...
+        InvalidOperationException failure =
+            await Assert.ThrowsAsync<InvalidOperationException>(() => firstMove.WaitAsync(TestTimeout));
+        Assert.Equal("simulated snapshot source failure", failure.Message);
+        await first.DisposeAsync();
+
+        // ...and the one that joined mid-unwind is served by a restarted enumeration
+        // instead of inheriting the failure.
+        await WaitUntilAsync(() => service.EnumerationCount >= 2);
+        await PushUntilAsync(service, secondMove);
+        Assert.True(await secondMove.WaitAsync(TestTimeout));
+        Assert.StartsWith("push-", second.Current.GatewayVersion, StringComparison.Ordinal);
+
+        await secondCancellation.CancelAsync();
+        await DrainAsync(second, secondMove);
+    }
+
     /// <summary>Builds a snapshot whose version string identifies it in assertions.</summary>
     /// <param name="version">Identity marker carried in <c>GatewayVersion</c>.</param>
     /// <returns>A snapshot carrying the supplied identity marker.</returns>
@@ -282,13 +335,19 @@ public sealed class DashboardSnapshotFeedTests
     private sealed class FakeSnapshotService : IDashboardSnapshotService
     {
         private readonly Channel<object> _pushes = Channel.CreateUnbounded<object>();
+        private readonly TaskCompletionSource _disposalReached = new(TaskCreationOptions.RunContinuationsAsynchronously);
+        private readonly TaskCompletionSource _disposalRelease = new(TaskCreationOptions.RunContinuationsAsynchronously);
         private int _enumerationCount;
         private int _completedEnumerationCount;
         private volatile bool _lastEnumerationWasCancelled;
+        private volatile bool _holdDisposal;
 
         /// <summary>Gets the number of times the feed started enumerating this source.</summary>
         public int EnumerationCount => Volatile.Read(ref _enumerationCount);
 
+        /// <summary>Gets a task that completes when a held enumerator disposal is reached.</summary>
+        public Task DisposalReached => _disposalReached.Task;
+
         /// <summary>Gets the number of enumerations that have finished (cancelled, faulted, or completed).</summary>
         public int CompletedEnumerationCount => Volatile.Read(ref _completedEnumerationCount);
 
@@ -303,11 +362,34 @@ public sealed class DashboardSnapshotFeedTests
         /// <param name="error">Exception to throw from the enumeration.</param>
         public void Fault(Exception error) => _pushes.Writer.TryWrite(error);
 
+        /// <summary>
+        /// Parks enumerator disposal until <see cref="ReleaseDisposal"/>, which holds the pump
+        /// in the window between observing the source's failure and detaching its subscribers.
+        /// </summary>
+        public void HoldDisposal() => _holdDisposal = true;
+
+        /// <summary>Releases a held enumerator disposal.</summary>
+        public void ReleaseDisposal() => _disposalRelease.TrySetResult();
+
         /// <inheritdoc />
         public DashboardSnapshot GetSnapshot() => CreateSnapshot("current");
 
         /// <inheritdoc />
-        public async IAsyncEnumerable<DashboardSnapshot> WatchSnapshotsAsync(
+        public IAsyncEnumerable<DashboardSnapshot> WatchSnapshotsAsync(CancellationToken cancellationToken)
+            => new GatedEnumerable(this);
+
+        private async Task OnDisposingAsync()
+        {
+            if (!_holdDisposal)
+            {
+                return;
+            }
+
+            _disposalReached.TrySetResult();
+            await _disposalRelease.Task.ConfigureAwait(false);
+        }
+
+        private async IAsyncEnumerable<DashboardSnapshot> EnumerateAsync(
             [EnumeratorCancellation] CancellationToken cancellationToken)
         {
             Interlocked.Increment(ref _enumerationCount);
@@ -329,5 +411,40 @@ public sealed class DashboardSnapshotFeedTests
                 Interlocked.Increment(ref _completedEnumerationCount);
             }
         }
+
+        /// <summary>
+        /// Wraps the iterator so disposal is a control point of its own: the feed ends a pump
+        /// generation when MoveNextAsync fails, which is strictly before this disposal runs.
+        /// </summary>
+        /// <param name="owner">The fake whose enumeration is being wrapped.</param>
+        private sealed class GatedEnumerable(FakeSnapshotService owner) : IAsyncEnumerable<DashboardSnapshot>
+        {
+            /// <summary>Creates a gated enumerator over the fake's enumeration.</summary>
+            /// <param name="cancellationToken">Token to observe for cancellation.</param>
+            /// <returns>The gated enumerator.</returns>
+            public IAsyncEnumerator<DashboardSnapshot> GetAsyncEnumerator(
+                CancellationToken cancellationToken = default)
+                => new GatedEnumerator(owner, owner.EnumerateAsync(cancellationToken).GetAsyncEnumerator(cancellationToken));
+        }
+
+        private sealed class GatedEnumerator(
+            FakeSnapshotService owner,
+            IAsyncEnumerator<DashboardSnapshot> inner) : IAsyncEnumerator<DashboardSnapshot>
+        {
+            /// <summary>Gets the current snapshot.</summary>
+            public DashboardSnapshot Current => inner.Current;
+
+            /// <summary>Advances the wrapped enumeration.</summary>
+            /// <returns>A task that yields whether another snapshot is available.</returns>
+            public ValueTask<bool> MoveNextAsync() => inner.MoveNextAsync();
+
+            /// <summary>Parks while the fake holds disposal, then disposes the wrapped enumeration.</summary>
+            /// <returns>A task that represents the asynchronous operation.</returns>
+            public async ValueTask DisposeAsync()
+            {
+                await owner.OnDisposingAsync().ConfigureAwait(false);
+                await inner.DisposeAsync().ConfigureAwait(false);
+            }
+        }
     }
 }