fix(centralui): P6-11 — request-generation guard on Deployments page
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:<path>`, not `git stash` — repo-wide and unsafe across the parallel worktree wave).
This commit is contained in:
Joseph Doherty
+254
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Regression tests for arch-review 2026-08-19 P6-11. Three independent initiators —
|
||||
/// search, paging, and the push/coalesce reload — can each have a
|
||||
/// <c>LoadDataAsync</c> 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.
|
||||
///
|
||||
/// <para>
|
||||
/// Separately, the filter-debounce timer used to write <c>_currentPage = 1</c>
|
||||
/// directly on its raw <see cref="System.Threading.Timer"/> 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
|
||||
/// (<c>LoadDataAsync(resetToFirstPage:)</c>), so a stale filter-debounce flight is
|
||||
/// discarded exactly like a stale flight on any other initiator.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// Both races are driven deterministically with <see cref="TaskCompletionSource{T}"/>
|
||||
/// -backed repository stubs — never sleep-then-assert timing. Both tests reference
|
||||
/// methods/fields the fix introduces (<c>_requestGeneration</c>,
|
||||
/// <c>DispatchFilterReloadAsync</c>), 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
|
||||
/// <c>Deployments.razor</c>, not <c>git stash</c> (repo-wide, unsafe to use in this
|
||||
/// worktree wave).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public class DeploymentsRequestGenerationTests : BunitContext
|
||||
{
|
||||
private IDeploymentManagerRepository _deployRepo = null!;
|
||||
private DeploymentStatusNotifier _notifier = null!;
|
||||
|
||||
/// <summary>
|
||||
/// One controllable response per call to <c>QueryDeploymentListPageAsync</c>, in
|
||||
/// call order. A queued <see cref="TaskCompletionSource{T}"/> lets the test choose
|
||||
/// exactly when each flight's fetch resolves, regardless of when it started.
|
||||
/// </summary>
|
||||
private readonly List<TaskCompletionSource<DeploymentListPage>> _pageResponses = new();
|
||||
private int _pageCallIndex;
|
||||
|
||||
private void RegisterServices()
|
||||
{
|
||||
_deployRepo = Substitute.For<IDeploymentManagerRepository>();
|
||||
_notifier = new DeploymentStatusNotifier(NullLogger<DeploymentStatusNotifier>.Instance);
|
||||
|
||||
_deployRepo.QueryDeploymentListPageAsync(
|
||||
Arg.Any<DeploymentListFilter>(), Arg.Any<int>(), Arg.Any<int>(), Arg.Any<CancellationToken>())
|
||||
.Returns(_ =>
|
||||
{
|
||||
var index = Interlocked.Increment(ref _pageCallIndex) - 1;
|
||||
return index < _pageResponses.Count
|
||||
? _pageResponses[index].Task
|
||||
: Task.FromResult(DeploymentListPage.Empty);
|
||||
});
|
||||
|
||||
_deployRepo.GetDeploymentStatusCountsAsync(
|
||||
Arg.Any<DeploymentListFilter>(), Arg.Any<CancellationToken>())
|
||||
.Returns(DeploymentStatusCounts.Empty);
|
||||
|
||||
Services.AddSingleton(_deployRepo);
|
||||
Services.AddSingleton<IDeploymentStatusNotifier>(_notifier);
|
||||
|
||||
var identity = new ClaimsIdentity(
|
||||
new[] { new Claim(ClaimTypes.Name, "deployer") }, "TestCookie");
|
||||
var stubAuth = new StubAuthStateProvider(
|
||||
new AuthenticationState(new ClaimsPrincipal(identity)));
|
||||
Services.AddSingleton<AuthenticationStateProvider>(stubAuth);
|
||||
Services.AddScoped(_ => new SiteScopeService(stubAuth));
|
||||
}
|
||||
|
||||
private sealed class StubAuthStateProvider : AuthenticationStateProvider
|
||||
{
|
||||
private readonly AuthenticationState _state;
|
||||
public StubAuthStateProvider(AuthenticationState state) => _state = state;
|
||||
public override Task<AuthenticationState> GetAuthenticationStateAsync()
|
||||
=> Task.FromResult(_state);
|
||||
}
|
||||
|
||||
private static TaskCompletionSource<DeploymentListPage> Gate() =>
|
||||
new(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
|
||||
private static TaskCompletionSource<DeploymentListPage> 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);
|
||||
|
||||
/// <summary>
|
||||
/// Invokes a private async method on the component via the renderer's dispatcher
|
||||
/// (mirroring the pattern in <c>DebugViewConnectDisposeRaceTests</c>) and hands
|
||||
/// back the resulting <see cref="Task"/> without awaiting it, so the caller can
|
||||
/// act while the flight is still in progress.
|
||||
/// </summary>
|
||||
private static async Task<Task> InvokePrivateAsync(
|
||||
IRenderedComponent<DeploymentsPage> 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<DeploymentListRow> GetRows(DeploymentsPage page) =>
|
||||
(IReadOnlyList<DeploymentListRow>)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<DeploymentsPage>();
|
||||
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");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// <para>
|
||||
/// This is driven through <c>DispatchFilterReloadAsync</c> directly (rather than
|
||||
/// the outer <c>OnFilterDebounceElapsed</c> timer callback, which is fire-and-
|
||||
/// forget and gives no <see cref="Task"/> 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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<DeploymentsPage>();
|
||||
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");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user