From 21d50d4406cebae9f2e80a7e65a3f65dcca9ef90 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 16:29:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(centralui):=20P6-11=20=E2=80=94=20request-g?= =?UTF-8?q?eneration=20guard=20on=20Deployments=20page?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fetch initiators (search debounce, paging, push/coalesce reload) could each have a LoadDataAsync flight in flight at once; completion order was not call order, so a slow, stale flight could resolve after a newer one and clobber the grid the operator was already looking at. Add a monotonic _requestGeneration counter, bumped at fetch start and checked at apply — last-write-wins by generation, not by whichever await resolves first. Separately, the filter-debounce timer wrote `_currentPage = 1` directly on its raw System.Threading.Timer callback thread — a Blazor threading violation, since component state may only be mutated on the renderer's sync context — and that write was not covered by the generation guard at all. LoadDataAsync now takes a resetToFirstPage flag, resolves the target page into a local, and applies it only behind the same generation check as every other piece of state, via a new DispatchFilterReloadAsync that marshals through InvokeAsync like the existing push-reload path. Tests (DeploymentsRequestGenerationTests, bUnit): a delayed stale flight arriving after a newer one does not overwrite the grid; a stale filter-debounce flight does not reset the page after a newer navigation. Both driven deterministically via TaskCompletionSource-backed repository stubs. Verified to fail against the unfixed code (swapped via `git show HEAD:`, not `git stash` — repo-wide and unsafe across the parallel worktree wave). --- .../Pages/Deployment/Deployments.razor | 116 +++++++- .../DeploymentsRequestGenerationTests.cs | 254 ++++++++++++++++++ 2 files changed, 361 insertions(+), 9 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsRequestGenerationTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Deployments.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Deployments.razor index 129cd0fa..c2a7a40c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Deployments.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Deployments.razor @@ -17,7 +17,7 @@ aria-label="@(_autoRefresh ? "Pause auto-refresh" : "Resume auto-refresh")"> @(_autoRefresh ? "⏸ Pause updates" : "▶ Resume updates") - + @@ -259,6 +259,19 @@ private int _totalPages; private const int PageSize = 25; + // ── Request-generation guard (P6-11) ──────────────────────────────────────── + // Three independent initiators can each start a LoadDataAsync flight: a search + // debounce, a page click, and the push/coalesce reload. Every flight awaits its + // own round-trip to central MS SQL, so COMPLETION order is not CALL order — a + // slow, stale flight that started first can resolve after a faster, newer one + // and clobber the grid the operator is already looking at. _requestGeneration + // is bumped synchronously at the top of every LoadDataAsync call (call order is + // still reliable — increments never interleave on the renderer's dispatcher), + // and a flight discards its own results at apply time if a newer flight has + // since started. Last-write-wins by GENERATION, not by whichever await + // resolves first. + private long _requestGeneration; + // ── Filters ─────────────────────────────────────────────────────────────── private DeploymentStatus? _statusFilter; @@ -423,10 +436,48 @@ if (_disposed) return; } - // A changed search term redefines the result set, so the operator's current - // page number no longer refers to anything — restart at page 1. - _currentPage = 1; - _ = DispatchReloadAsync(); + // This callback runs on the raw System.Threading.Timer thread — it is NOT + // the renderer's synchronization context. _currentPage is component state, + // so its reset to page 1 must be marshaled through InvokeAsync the same as + // the reload itself (P6-11); writing it here directly was a Blazor + // threading violation that could also race a same-tick page click on the + // renderer thread. + _ = DispatchFilterReloadAsync(); + } + + /// + /// Marshals the filter-debounce trailing edge onto the renderer's dispatcher. + /// The page-1 reset MUST happen inside InvokeAsync, and behind the SAME + /// request-generation guard as the rest of the applied state (P6-11) — + /// runs on a raw + /// callback thread, not the renderer's + /// synchronization context, and a slow search flight can resolve after the + /// operator has already navigated to a different page by other means. Passing + /// resetToFirstPage: true defers the actual _currentPage = 1 + /// write into 's apply step, so a stale flight here + /// is discarded exactly like a stale flight on any other initiator. + /// + private async Task DispatchFilterReloadAsync() + { + if (_disposed) return; + try + { + await InvokeAsync(async () => + { + if (_disposed) return; + + // A changed search term redefines the result set, so the + // operator's current page number no longer refers to anything — + // restart at page 1 (applied only if this flight is still current; + // see the resetToFirstPage remarks on LoadDataAsync). + await LoadDataAsync(resetToFirstPage: true); + StateHasChanged(); + }); + } + catch (ObjectDisposedException) + { + // Component disposed between the guard and the dispatch — ignore. + } } /// @@ -497,8 +548,27 @@ await LoadDataAsync(); } - private async Task LoadDataAsync() + /// + /// True when this flight represents a redefinition of the result set (a new + /// search term) rather than a plain reload of whatever page is currently on + /// screen. The target page is resolved into a LOCAL pageToQuery rather + /// than writing _currentPage up front, so the page number — like every + /// other piece of applied state — only lands once this flight has been + /// confirmed to still be the newest one (see the generation guard below). A + /// stale, slow search flight that resolves after a newer page click must not + /// yank the operator back to page 1 out from under a navigation they already + /// completed. + /// + private async Task LoadDataAsync(bool resetToFirstPage = false) { + // Captured before any await — see the request-generation guard note on + // _requestGeneration above. Bumping it here (rather than at apply time) + // means it also marks THIS flight as the newest one in flight, so an + // earlier flight still awaiting its own round-trip will find itself + // stale when it reaches the check below. + var generation = ++_requestGeneration; + var pageToQuery = resetToFirstPage ? 1 : _currentPage; + _loading = _rows.Count == 0; // Only show loading on first load _errorMessage = null; try @@ -520,16 +590,29 @@ SiteIdScope: siteScope); var page = await DeploymentManagerRepository - .QueryDeploymentListPageAsync(filter, _currentPage, PageSize); + .QueryDeploymentListPageAsync(filter, pageToQuery, PageSize); // The tile counts deliberately ignore the status filter (see // GetDeploymentStatusCountsAsync) so each tile keeps showing its own // total while it is the one selected. - _counts = await DeploymentManagerRepository.GetDeploymentStatusCountsAsync(filter); + var counts = await DeploymentManagerRepository.GetDeploymentStatusCountsAsync(filter); + // A newer flight has started since this one began (its generation is + // now the current one) — this flight's results are stale. Discard + // them rather than overwrite whatever the newer flight already + // applied (or is about to apply). This guards _currentPage exactly + // like every other applied field — a stale reset-to-page-1 flight + // must not clobber a page the operator has since navigated to. + if (generation != _requestGeneration) + { + return; + } + + _currentPage = pageToQuery; _rows = page.Rows; _totalCount = page.TotalCount; _totalPages = page.PageCount(PageSize); + _counts = counts; // A concurrent retention purge, or a filter that narrowed the set, can // leave the operator past the end. Fall back to the last real page and @@ -539,6 +622,12 @@ _currentPage = _totalPages; var reread = await DeploymentManagerRepository .QueryDeploymentListPageAsync(filter, _currentPage, PageSize); + + if (generation != _requestGeneration) + { + return; + } + _rows = reread.Rows; _totalCount = reread.TotalCount; _totalPages = reread.PageCount(PageSize); @@ -546,9 +635,18 @@ } catch (Exception ex) { + if (generation != _requestGeneration) + { + return; + } + _errorMessage = $"Failed to load deployments: {ex.Message}"; } - _loading = false; + + if (generation == _requestGeneration) + { + _loading = false; + } } private async Task GoToPageAsync(int page) diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsRequestGenerationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsRequestGenerationTests.cs new file mode 100644 index 00000000..da3c3ec2 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsRequestGenerationTests.cs @@ -0,0 +1,254 @@ +using System.Reflection; +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.CentralUI.Auth; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.DeploymentManager; +using DeploymentsPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Deployment.Deployments; + +namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Deployment; + +/// +/// Regression tests for arch-review 2026-08-19 P6-11. Three independent initiators — +/// search, paging, and the push/coalesce reload — can each have a +/// LoadDataAsync flight in flight on the Deployment Status page at once. Every +/// flight awaits its own round-trip to central MS SQL, so COMPLETION order is not +/// CALL order: a slow flight that started first can resolve LAST and overwrite a +/// newer grid the operator is already looking at. +/// +/// +/// Separately, the filter-debounce timer used to write _currentPage = 1 +/// directly on its raw callback thread — a +/// Blazor threading violation, since component state may only be mutated on the +/// renderer's synchronization context — and that write was NOT covered by the +/// request-generation guard at all, so even a correctly-marshaled version of it +/// could still reset the operator's page out from under a newer navigation. The fix +/// resolves the target page into a local at fetch start and applies it only behind +/// the same generation check as every other piece of state +/// (LoadDataAsync(resetToFirstPage:)), so a stale filter-debounce flight is +/// discarded exactly like a stale flight on any other initiator. +/// +/// +/// +/// Both races are driven deterministically with +/// -backed repository stubs — never sleep-then-assert timing. Both tests reference +/// methods/fields the fix introduces (_requestGeneration, +/// DispatchFilterReloadAsync), so they fail — by compile error or reflection +/// NRE — against the unfixed source, in addition to failing behaviourally once a +/// same-shaped unfixed build is exercised; verified via a saved pre-fix copy of +/// Deployments.razor, not git stash (repo-wide, unsafe to use in this +/// worktree wave). +/// +/// +public class DeploymentsRequestGenerationTests : BunitContext +{ + private IDeploymentManagerRepository _deployRepo = null!; + private DeploymentStatusNotifier _notifier = null!; + + /// + /// One controllable response per call to QueryDeploymentListPageAsync, in + /// call order. A queued lets the test choose + /// exactly when each flight's fetch resolves, regardless of when it started. + /// + private readonly List> _pageResponses = new(); + private int _pageCallIndex; + + private void RegisterServices() + { + _deployRepo = Substitute.For(); + _notifier = new DeploymentStatusNotifier(NullLogger.Instance); + + _deployRepo.QueryDeploymentListPageAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + { + var index = Interlocked.Increment(ref _pageCallIndex) - 1; + return index < _pageResponses.Count + ? _pageResponses[index].Task + : Task.FromResult(DeploymentListPage.Empty); + }); + + _deployRepo.GetDeploymentStatusCountsAsync( + Arg.Any(), Arg.Any()) + .Returns(DeploymentStatusCounts.Empty); + + Services.AddSingleton(_deployRepo); + Services.AddSingleton(_notifier); + + var identity = new ClaimsIdentity( + new[] { new Claim(ClaimTypes.Name, "deployer") }, "TestCookie"); + var stubAuth = new StubAuthStateProvider( + new AuthenticationState(new ClaimsPrincipal(identity))); + Services.AddSingleton(stubAuth); + Services.AddScoped(_ => new SiteScopeService(stubAuth)); + } + + private sealed class StubAuthStateProvider : AuthenticationStateProvider + { + private readonly AuthenticationState _state; + public StubAuthStateProvider(AuthenticationState state) => _state = state; + public override Task GetAuthenticationStateAsync() + => Task.FromResult(_state); + } + + private static TaskCompletionSource Gate() => + new(TaskCreationOptions.RunContinuationsAsynchronously); + + private static TaskCompletionSource CompletedGate(DeploymentListPage page) + { + var tcs = Gate(); + tcs.SetResult(page); + return tcs; + } + + private static DeploymentListRow Row(int id, string instance) => + new( + Id: id, + DeploymentId: $"dep-{id:D4}", + InstanceId: id, + InstanceUniqueName: instance, + SiteId: 1, + Status: DeploymentStatus.Success, + RevisionHash: $"rev{id:D4}hash", + DeployedBy: "deployer", + DeployedAt: DateTimeOffset.UtcNow.AddMinutes(-id), + CompletedAt: DateTimeOffset.UtcNow, + ErrorMessage: null); + + /// + /// Invokes a private async method on the component via the renderer's dispatcher + /// (mirroring the pattern in DebugViewConnectDisposeRaceTests) and hands + /// back the resulting without awaiting it, so the caller can + /// act while the flight is still in progress. + /// + private static async Task InvokePrivateAsync( + IRenderedComponent cut, string methodName, params object?[] args) + { + var method = typeof(DeploymentsPage).GetMethod(methodName, BindingFlags.Instance | BindingFlags.NonPublic)!; + Task? task = null; + await cut.InvokeAsync(() => { task = (Task)method.Invoke(cut.Instance, args)!; }); + return task!; + } + + private static IReadOnlyList GetRows(DeploymentsPage page) => + (IReadOnlyList)typeof(DeploymentsPage) + .GetField("_rows", BindingFlags.Instance | BindingFlags.NonPublic)! + .GetValue(page)!; + + private static int GetCurrentPage(DeploymentsPage page) => + (int)typeof(DeploymentsPage) + .GetField("_currentPage", BindingFlags.Instance | BindingFlags.NonPublic)! + .GetValue(page)!; + + [Fact] + public async Task StaleFlightArrivingAfterANewerOne_DoesNotOverwriteTheGrid() + { + RegisterServices(); + + var initialRows = Enumerable.Range(1, 25).Select(i => Row(i, $"Init-{i}")).ToList(); + _pageResponses.Add(CompletedGate(new DeploymentListPage(initialRows, TotalCount: 100))); // call #1: initial render + + var stale = Gate(); // call #2: page-2 flight — held open, the SLOW, STALE flight + _pageResponses.Add(stale); + + // call #3: status-filter flight — resolves immediately, the FAST, NEWER flight + _pageResponses.Add(CompletedGate( + new DeploymentListPage(new[] { Row(999, "TileFilteredFreshRow") }, TotalCount: 1))); + + var cut = Render(); + cut.WaitForAssertion(() => Assert.Contains(GetRows(cut.Instance), r => r.InstanceUniqueName == "Init-1")); + + // Flight A starts FIRST (a page click) and blocks awaiting `stale`. + var flightA = await InvokePrivateAsync(cut, "GoToPageAsync", 2); + Assert.False(flightA.IsCompleted); + + // Flight B starts SECOND (a status-tile click) while A is still in flight, and + // resolves immediately — it is the newer, correct result. + var flightB = await InvokePrivateAsync(cut, "SetStatusFilterAsync", DeploymentStatus.Failed); + await flightB.WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.Contains(GetRows(cut.Instance), r => r.InstanceUniqueName == "TileFilteredFreshRow"); + + // Now let the STALE flight (page 2) resolve, arriving AFTER the newer one has + // already applied. Awaiting flightA guarantees its continuation — including + // the apply-time generation check under test — has fully run before we + // assert, rather than racing bUnit's render pump. + stale.SetResult(new DeploymentListPage(new[] { Row(2, "PageTwoStaleRow") }, TotalCount: 100)); + await flightA.WaitAsync(TimeSpan.FromSeconds(5)); + + // Pre-fix, flight A's late arrival clobbers _rows with the stale page-2 data. + // Post-fix, its generation is no longer current, so it discards its own + // results and the newer grid survives untouched. + Assert.DoesNotContain(GetRows(cut.Instance), r => r.InstanceUniqueName == "PageTwoStaleRow"); + Assert.Contains(GetRows(cut.Instance), r => r.InstanceUniqueName == "TileFilteredFreshRow"); + } + + /// + /// The "does the timer refresh reset the page mid-search" half of P6-11. The + /// filter-debounce ("timer") reload fires first, as if 500ms had elapsed after a + /// keystroke, and is held open. While it is still in flight the operator + /// navigates to page 2 by other means (a fresh, faster flight). The debounce + /// flight — which intends to reset to page 1 — must not resolve later and + /// clobber a page the operator has since, and more recently, navigated to. + /// + /// This is driven through DispatchFilterReloadAsync directly (rather than + /// the outer OnFilterDebounceElapsed timer callback, which is fire-and- + /// forget and gives no handle to await) so completion of the + /// apply step under test — including the generation check — can be awaited + /// deterministically instead of racing bUnit's render pump. + /// + /// + [Fact] + public async Task StaleFilterDebounceFlight_DoesNotResetThePageAfterANewerNavigation() + { + RegisterServices(); + + var initialRows = Enumerable.Range(1, 25).Select(i => Row(i, $"Init-{i}")).ToList(); + _pageResponses.Add(CompletedGate(new DeploymentListPage(initialRows, TotalCount: 100))); // call #1: initial render + + var staleSearch = Gate(); // call #2: the debounce-triggered ("timer") flight — held open, STALE + _pageResponses.Add(staleSearch); + + // call #3: page-2 click — resolves immediately, NEWER. + _pageResponses.Add(CompletedGate( + new DeploymentListPage(new[] { Row(2, "PageTwoFreshRow") }, TotalCount: 100))); + + var cut = Render(); + cut.WaitForAssertion(() => Assert.Contains(GetRows(cut.Instance), r => r.InstanceUniqueName == "Init-1")); + + // The filter-debounce ("timer") reload starts FIRST and blocks on `staleSearch`. + var staleFlight = await InvokePrivateAsync(cut, "DispatchFilterReloadAsync"); + Assert.False(staleFlight.IsCompleted); + + // While it is still in flight, the operator navigates to page 2 — a fresh + // action that resolves immediately. + var nav = await InvokePrivateAsync(cut, "GoToPageAsync", 2); + await nav.WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.Equal(2, GetCurrentPage(cut.Instance)); + Assert.Contains(GetRows(cut.Instance), r => r.InstanceUniqueName == "PageTwoFreshRow"); + + // Now let the STALE debounce flight resolve, arriving after the navigation + // already applied. Awaiting staleFlight guarantees its continuation — + // including the apply-time generation check under test — has fully run + // before we assert, rather than racing bUnit's render pump. + staleSearch.SetResult( + new DeploymentListPage(new[] { Row(1, "SearchPageOneStaleRow") }, TotalCount: 1)); + await staleFlight.WaitAsync(TimeSpan.FromSeconds(5)); + + // Pre-fix, the stale flight unconditionally wrote _currentPage = 1 up front + // and then overwrote the grid with page-1 data once it resolved — resetting + // the operator's page mid an already-completed navigation. Post-fix, its + // generation is no longer current, so both the page and the grid are left + // untouched. + Assert.Equal(2, GetCurrentPage(cut.Instance)); + Assert.Contains(GetRows(cut.Instance), r => r.InstanceUniqueName == "PageTwoFreshRow"); + Assert.DoesNotContain(GetRows(cut.Instance), r => r.InstanceUniqueName == "SearchPageOneStaleRow"); + } +}