diff --git a/docs/plans/2026-08-05-raw-modal-picker-live-config-design.md b/docs/plans/2026-08-05-raw-modal-picker-live-config-design.md index 473fee14..03fe1955 100644 --- a/docs/plans/2026-08-05-raw-modal-picker-live-config-design.md +++ b/docs/plans/2026-08-05-raw-modal-picker-live-config-design.md @@ -114,6 +114,21 @@ through a spec review + code review. Things the reviews changed or decided: (out of this branch's scope). Recorded in `deferment.md` §0.1. Lower stakes there — the browse modal resolves the device it was opened for and shows its own error banner — but the shape is identical. + - **Ported 2026-08-09.** The `_openGeneration` counter now guards **every** post-await write in + `ResetAndResolveAsync`: the merged-config write, the group-context write + (`_groupPrefix`/`_effectiveDeviceId` — a second await the siblings do not have), the `catch`'s + error banner, and the `finally`'s `_resolving` + `StateHasChanged`. The `finally` is the one + departure from the sibling shape and it is load-bearing: this modal resolves inside a + `try`/`finally`, so a stale `return` still runs the tail and would drop the **live** open's + spinner mid-resolve. Pinned by four facts in `RawBrowseModalStaleOpenTests` + (`A_stale_open_never_feeds_the_browser_another_devices_config`, + `A_stale_group_resolve_never_retargets_the_displayed_open`, + `A_stale_open_completing_late_never_clears_the_live_opens_loading_state`, + `A_stale_resolve_failure_never_paints_its_error_over_the_live_open`), each observed red before + the guard and each re-falsified afterwards by reverting **only its own** guard. Driving this + modal as a class needs one thing the siblings did not: it calls `StateHasChanged` on the open + path, which throws without a render handle, so the test attaches an inert `Renderer` subclass + (file-local `#pragma warning disable BL0006`). - **Component-class tests must not set `[Parameter]` properties directly**: analyzer BL0005 is an error under `TreatWarningsAsErrors`. The two test files legitimately solve it two ways (file-local pragma vs a reflection `SetParameters` helper); the reflection helper is the preferred convention diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor index 1e57c8c3..de50996f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor @@ -279,6 +279,15 @@ [Parameter] public EventCallback OnSaved { get; set; } private bool _open; // guards re-seeding on unrelated re-renders + + /// + /// Identifies WHICH open a post-await write belongs to. is a bool and cannot + /// tell two opens apart, and Blazor does not serialize OnParametersSetAsync against its own + /// outstanding task — so a close-reopen onto another device overlaps the first open's resolve, and + /// last-completion-wins would point "Connect & browse" at a controller nobody is looking at. + /// + private int _openGeneration; + private bool _resolving; private bool _canBrowse; private string? _disabledReason; @@ -348,6 +357,8 @@ private async Task ResetAndResolveAsync() { + var generation = ++_openGeneration; + _token = Guid.Empty; _opening = false; _openError = null; @@ -374,6 +385,10 @@ // Merged Driver+Device config — endpoint lives in DeviceConfig in v3, so the browser must dial // the merged blob (same merge the driver-probe/test-connect path uses). var merged = await Svc.LoadMergedProbeConfigAsync(DeviceId); + // A superseded open must never install its device under the displayed modal — see + // _openGeneration. Every post-await write below is behind this check. + if (IsStale(generation)) { return; } + if (merged is { } m) { _driverType = m.DriverType; @@ -384,6 +399,8 @@ if (!string.IsNullOrEmpty(TagGroupId)) { var ctx = await ExportReader.ResolveGroupContextAsync(TagGroupId!); + if (IsStale(generation)) { return; } // same race, second await: this one retargets the commit + if (ctx is { } c) { _groupPrefix = c.GroupPath; @@ -401,16 +418,37 @@ } catch (Exception ex) { - _canBrowse = false; - _disabledReason = $"Could not load device config: {ex.Message}"; + // A superseded open that FAILS is the same defect wearing the catch block: its diagnostic + // names a device the operator is no longer looking at, and it would disable browsing for the + // one they are. + if (!IsStale(generation)) + { + _canBrowse = false; + _disabledReason = $"Could not load device config: {ex.Message}"; + } } finally { - _resolving = false; - StateHasChanged(); + // The tail state belongs to an open too: `return`ing out of the try above still runs this, so + // an unguarded finally would drop the LIVE open's spinner mid-resolve and render the + // half-reset panel underneath it. + if (!IsStale(generation)) + { + _resolving = false; + StateHasChanged(); + } } } + /// + /// Whether the open that captured has been superseded — by a later + /// open, or by the modal being closed outright (a close raises no new generation, so + /// is the half that catches it). + /// + /// The value captured when the open started. + /// when no state may be written for that open. + private bool IsStale(int generation) => generation != _openGeneration || !Visible; + private async Task OpenBrowseAsync() { _opening = true; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs new file mode 100644 index 00000000..eeaddab0 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs @@ -0,0 +1,393 @@ +using System.Reflection; +using System.Runtime.ExceptionServices; +using Microsoft.AspNetCore.Components; +using Microsoft.AspNetCore.Components.RenderTree; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Browsing; +using ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared.Raw; +using ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.Commons.Browsing; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Raw; + +/// +/// Pins that a superseded open of the browse modal can never write its device's browse context under +/// a different device's displayed modal. +/// +/// +/// +/// Blazor does not serialize OnParametersSetAsync against its own outstanding task, and the +/// modal's _open bool cannot tell two opens apart. Its resolve awaits the merged probe +/// config and the target group context, so without a post-await staleness re-check a +/// close-reopen onto another device lets the first open's late continuation install its +/// _driverType / _mergedConfig / _groupPrefix — and "Connect & browse" +/// would then silently dial the wrong controller and commit under the wrong group. Same race, same +/// fix as the two sibling modals (RawTagModal, RawManualTagEntryModal, 2026-08-05). +/// +/// +/// AdminUI has no bUnit, so the component is driven directly as a class: its injected properties, +/// its [Parameter]s (BL0005 forbids setting those from outside the component, and this +/// project treats warnings as errors) and its private resolve state are reached by reflection. +/// Unlike the siblings this modal calls StateHasChanged on the open path, which throws +/// unless the component holds a render handle — hence the inert renderer below. +/// +/// +// BL0006 warns off Microsoft.AspNetCore.Components.RenderTree, which is right for production code and +// unavoidable here: subclassing Renderer is the only way to hand a component a render handle without +// taking a bUnit dependency, and Renderer's abstract render method is typed in that namespace. The surface +// used is that one parameter — no RenderTree type is inspected, stored or asserted on. +#pragma warning disable BL0006 +public sealed class RawBrowseModalStaleOpenTests : IDisposable +{ + private const string ConfigA = """{"endpoint":"10.0.0.1:44818","slot":0}"""; + private const string ConfigB = """{"endpoint":"10.0.0.2:44818","slot":1}"""; + + private readonly List _renderers = new(); + + [Fact] + public async Task A_stale_open_never_feeds_the_browser_another_devices_config() + { + var deviceAResolve = new TaskCompletionSource(); + var svc = new FakeRawTreeService + { + OnLoadMergedProbeConfigAsync = async deviceId => + { + if (deviceId == "dev-A") { await deviceAResolve.Task; return ("AbCip", ConfigA); } + return ("TwinCAT", ConfigB); + }, + }; + + var modal = NewModal(svc, new FakeBrowserSessionService(), NullExportReader()); + + // Quick close-reopen onto another device while dev-A's resolve is still in flight. + SetParameters(modal, visible: true, deviceId: "dev-A", tagGroupId: null); + var staleOpen = OpenAsync(modal); + + SetParameters(modal, visible: false, deviceId: "dev-A", tagGroupId: null); + await OpenAsync(modal); + + SetParameters(modal, visible: true, deviceId: "dev-B", tagGroupId: null); + await OpenAsync(modal); + + deviceAResolve.SetResult(); + await staleOpen; + + Field(modal, "_mergedConfig").ShouldBe(ConfigB); + Field(modal, "_driverType").ShouldBe("TwinCAT"); + } + + [Fact] + public async Task A_stale_group_resolve_never_retargets_the_displayed_open() + { + var dbName = SeedGroup("TG-A", deviceId: "dev-A", name: "GroupA"); + var groupResolve = new TaskCompletionSource(); + + var svc = new FakeRawTreeService + { + OnLoadMergedProbeConfig = deviceId => + deviceId == "dev-A" ? ("AbCip", ConfigA) : null, + }; + + var modal = NewModal( + svc, + new FakeBrowserSessionService(), + new RawTagCsvExportReader(new GatedDbContextFactory(dbName, groupResolve.Task))); + + // The merged-config resolve completes inline here, so this open suspends inside the GROUP + // resolve — the second await, which a fix that guards only the first one leaves unprotected. + SetParameters(modal, visible: true, deviceId: "dev-A", tagGroupId: "TG-A"); + var staleOpen = OpenAsync(modal); + + SetParameters(modal, visible: false, deviceId: "dev-A", tagGroupId: "TG-A"); + await OpenAsync(modal); + + // Re-opened with no device and no target group: the displayed open commits at a device root it + // has not resolved yet, so both writes the stale continuation makes are observable. + SetParameters(modal, visible: true, deviceId: "", tagGroupId: null); + await OpenAsync(modal); + + groupResolve.SetResult(); + await staleOpen; + + Field(modal, "_groupPrefix").ShouldBeNull(); + Field(modal, "_effectiveDeviceId").ShouldBe(""); + } + + [Fact] + public async Task A_stale_open_completing_late_never_clears_the_live_opens_loading_state() + { + var deviceAResolve = new TaskCompletionSource(); + var deviceBResolve = new TaskCompletionSource(); + var svc = new FakeRawTreeService + { + OnLoadMergedProbeConfigAsync = async deviceId => + { + if (deviceId == "dev-A") { await deviceAResolve.Task; return ("AbCip", ConfigA); } + await deviceBResolve.Task; + return ("TwinCAT", ConfigB); + }, + }; + + var modal = NewModal(svc, new FakeBrowserSessionService(), NullExportReader()); + + SetParameters(modal, visible: true, deviceId: "dev-A", tagGroupId: null); + var staleOpen = OpenAsync(modal); + + SetParameters(modal, visible: false, deviceId: "dev-A", tagGroupId: null); + await OpenAsync(modal); + + SetParameters(modal, visible: true, deviceId: "dev-B", tagGroupId: null); + var liveOpen = OpenAsync(modal); + + // The stale run finishes while the live one is still resolving. Its tail state — the spinner + // flag and the render it forces — belongs to an open that is no longer displayed: letting the + // `finally` run would drop the live open's spinner and paint the not-yet-resolved + // "Browsing unavailable" panel over a device that is still loading. + deviceAResolve.SetResult(); + await staleOpen; + + Field(modal, "_resolving").ShouldBeTrue(); + + deviceBResolve.SetResult(); + await liveOpen; + + Field(modal, "_resolving").ShouldBeFalse(); + } + + [Fact] + public async Task A_stale_resolve_failure_never_paints_its_error_over_the_live_open() + { + var deviceAResolve = new TaskCompletionSource(); + var svc = new FakeRawTreeService + { + OnLoadMergedProbeConfigAsync = async deviceId => + { + if (deviceId == "dev-A") { await deviceAResolve.Task; return ("AbCip", ConfigA); } + return ("TwinCAT", ConfigB); + }, + }; + + var modal = NewModal(svc, new FakeBrowserSessionService(), NullExportReader()); + + SetParameters(modal, visible: true, deviceId: "dev-A", tagGroupId: null); + var staleOpen = OpenAsync(modal); + + SetParameters(modal, visible: false, deviceId: "dev-A", tagGroupId: null); + await OpenAsync(modal); + + SetParameters(modal, visible: true, deviceId: "dev-B", tagGroupId: null); + await OpenAsync(modal); + + // A superseded open that FAILS is the same defect wearing the catch block: its diagnostic names + // a device the operator is no longer looking at, and disables browsing for the one they are. + deviceAResolve.SetException(new InvalidOperationException("dev-A unreachable")); + await staleOpen; + + Field(modal, "_canBrowse").ShouldBeTrue(); + Field(modal, "_disabledReason").ShouldBeNull(); + } + + /// + public void Dispose() + { + foreach (var renderer in _renderers) { renderer.Dispose(); } + } + + /// Builds a modal with its three injected dependencies set and a render handle attached. + private RawBrowseModal NewModal( + IRawTreeService svc, IBrowserSessionService browser, RawTagCsvExportReader exportReader) + { + var modal = new RawBrowseModal(); + Inject(modal, "Svc", svc); + Inject(modal, "BrowserService", browser); + Inject(modal, "ExportReader", exportReader); + + var renderer = new InertRenderer(); + _renderers.Add(renderer); + renderer.Attach(modal); + + return modal; + } + + /// An export reader over a factory that fails loudly — for tests that must never reach it. + private static RawTagCsvExportReader NullExportReader() + => new(new GatedDbContextFactory(dbName: null, gate: Task.CompletedTask)); + + /// Seeds a lone tag-group row into a fresh InMemory database and returns its name. + private static string SeedGroup(string tagGroupId, string deviceId, string name) + { + var dbName = $"rawbrowse-{Guid.NewGuid():N}"; + using var db = RawTreeTestDb.CreateNamed(dbName); + db.TagGroups.Add(new TagGroup { TagGroupId = tagGroupId, DeviceId = deviceId, Name = name }); + db.SaveChanges(); + return dbName; + } + + /// + /// Sets the modal's [Parameter]s. By reflection because BL0005 forbids assigning component + /// parameters from outside the component, and this project treats warnings as errors. + /// + private static void SetParameters( + RawBrowseModal modal, bool visible, string deviceId, string? tagGroupId) + { + Set("Visible", visible); + Set("DeviceId", deviceId); + Set("TagGroupId", tagGroupId); + + void Set(string name, object? value) + { + var property = typeof(RawBrowseModal).GetProperty(name); + property.ShouldNotBeNull($"the [Parameter] {name} was renamed or removed."); + property!.SetValue(modal, value); + } + } + + /// Sets one of the @inject-generated private properties. + private static void Inject(RawBrowseModal modal, string name, object value) + { + var property = typeof(RawBrowseModal).GetProperty( + name, + BindingFlags.NonPublic | BindingFlags.Instance); + + property.ShouldNotBeNull($"the injected {name} property was renamed or removed."); + + property!.SetValue(modal, value); + } + + /// Drives the open path — the lifecycle method the resolve hangs off. + private static async Task OpenAsync(RawBrowseModal modal) + { + var method = typeof(RawBrowseModal).GetMethod( + "OnParametersSetAsync", + BindingFlags.NonPublic | BindingFlags.Instance); + + method.ShouldNotBeNull("the open path moved off OnParametersSetAsync — retarget this driver."); + + await (Task)method!.Invoke(modal, null)!; + } + + /// Reads one of the modal's private resolve-state fields. + private static T Field(RawBrowseModal modal, string name) + { + var field = typeof(RawBrowseModal).GetField(name, BindingFlags.NonPublic | BindingFlags.Instance); + field.ShouldNotBeNull($"the modal's {name} field was renamed or removed."); + return (T)field!.GetValue(modal)!; + } + + /// + /// An whose context creation can be held open, so a + /// test can suspend a resolve inside ResolveGroupContextAsync and interleave a second open. + /// A null database name means "must never be called". + /// + private sealed class GatedDbContextFactory(string? dbName, Task gate) + : IDbContextFactory + { + /// + public OtOpcUaConfigDbContext CreateDbContext() + { + dbName.ShouldNotBeNull("the group-context resolve was reached by a test that forbids it."); + return RawTreeTestDb.CreateNamed(dbName!); + } + + /// + public async Task CreateDbContextAsync(CancellationToken ct = default) + { + await gate; + return CreateDbContext(); + } + } + + /// + /// Browse gate stand-in: every driver+config can browse, so a test asserting that a stale run did + /// not disable browsing has a true baseline to be falsified against. Nothing else is reachable — + /// these tests never open a session. + /// + private sealed class FakeBrowserSessionService : IBrowserSessionService + { + /// + public bool CanBrowse(string driverType, string configJson) => true; + + /// + public Task OpenAsync(string driverType, string configJson, CancellationToken ct) + => throw new NotImplementedException(); + + /// + public Task> RootAsync(Guid token, CancellationToken ct) + => throw new NotImplementedException(); + + /// + public Task> ExpandAsync(Guid token, string nodeId, CancellationToken ct) + => throw new NotImplementedException(); + + /// + public Task> AttributesAsync(Guid token, string nodeId, CancellationToken ct) + => throw new NotImplementedException(); + + /// + public Task RequestRebirthAsync(Guid token, string scope, CancellationToken ct) + => throw new NotImplementedException(); + + /// + public bool CanRequestRebirth(Guid token) => false; + + /// + public Task CloseAsync(Guid token) => throw new NotImplementedException(); + } + + /// + /// The smallest renderer that gives a component a working render handle: StateHasChanged + /// throws without one, and this modal calls it on the open path. Renders are built and then + /// discarded — the tests read the modal's state, not its markup. + /// + private sealed class InertRenderer() + : Renderer(new ServiceCollection().BuildServiceProvider(), NullLoggerFactory.Instance) + { + /// + public override Dispatcher Dispatcher { get; } = new InlineDispatcher(); + + /// Assigns the component a render handle, as a root component would receive. + public void Attach(IComponent component) => AssignRootComponentId(component); + + /// + protected override void HandleException(Exception exception) + => ExceptionDispatchInfo.Capture(exception).Throw(); + + /// + protected override Task UpdateDisplayAsync(in RenderBatch renderBatch) => Task.CompletedTask; + } + + /// + /// Runs everything on the calling thread. The real dispatcher would refuse a + /// StateHasChanged raised from a test thread (or from a resolve continuation), which is + /// exactly what these tests do. + /// + private sealed class InlineDispatcher : Dispatcher + { + /// + public override bool CheckAccess() => true; + + /// + public override Task InvokeAsync(Action workItem) + { + workItem(); + return Task.CompletedTask; + } + + /// + public override Task InvokeAsync(Func workItem) => workItem(); + + /// + public override Task InvokeAsync(Func workItem) + => Task.FromResult(workItem()); + + /// + public override Task InvokeAsync(Func> workItem) => workItem(); + } +}