dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(adminui): guard RawBrowseModal's resolve against a superseded open

Browse Source 
Blazor does not serialize OnParametersSetAsync against its own outstanding
task, and the modal's `_open` bool cannot tell two opens apart. ResetAndResolveAsync
awaits LoadMergedProbeConfigAsync and ResolveGroupContextAsync and then wrote
_driverType/_mergedConfig/_effectiveDeviceId/_groupPrefix/_canBrowse/_disabledReason
with no staleness re-check, so a close-reopen onto another device let the first
open's late continuation install its device's browse context under the displayed
modal — "Connect & browse" would silently dial the wrong controller and the commit
would land under the wrong tag-group.

Ports the per-open `_openGeneration` counter proven in the two sibling modals
(RawManualTagEntryModal, RawTagModal, 2026-08-05) to every post-await write here:
after both awaits, in the catch's error banner, and in the finally. 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 RawBrowseModalStaleOpenTests facts, each observed red before the guard
and each re-falsified afterwards by reverting only its own guard. The modal calls
StateHasChanged on the open path, which throws without a render handle, so the test
attaches an inert Renderer subclass (file-local BL0006 pragma); [Parameter]s are set
by reflection per the BL0005 convention.

AdminUI suite 1117/1117 green.
This commit is contained in:
Joseph Doherty
2026-08-09 21:07:07 -04:00
parent 0125634400
commit 87637e4e2c
3 changed files with 450 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/2026-08-05-raw-modal-picker-live-config-design.md
+15
View File
@@ -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
src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor
+42 -4
View File
@@ -279,6 +279,15 @@
[Parameter] public EventCallback OnSaved { get; set; }
private bool _open; // guards re-seeding on unrelated re-renders
/// <summary>
/// Identifies WHICH open a post-await write belongs to. <see cref="_open"/> is a bool and cannot
/// tell two opens apart, and Blazor does not serialize <c>OnParametersSetAsync</c> 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 &amp; browse" at a controller nobody is looking at.
/// </summary>
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();
}
}
}
/// <summary>
/// Whether the open that captured <paramref name="generation"/> has been superseded — by a later
/// open, or by the modal being closed outright (a close raises no new generation, so
/// <see cref="Visible"/> is the half that catches it).
/// </summary>
/// <param name="generation">The <see cref="_openGeneration"/> value captured when the open started.</param>
/// <returns><see langword="true"/> when no state may be written for that open.</returns>
private bool IsStale(int generation) => generation != _openGeneration || !Visible;
private async Task OpenBrowseAsync()
{
_opening = true;
tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs
+393
View File
@@ -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;
/// <summary>
/// Pins that a superseded open of the browse modal can never write its device's browse context under
/// a different device's displayed modal.
/// </summary>
/// <remarks>
/// <para>
/// Blazor does not serialize <c>OnParametersSetAsync</c> against its own outstanding task, and the
/// modal's <c>_open</c> 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
/// <c>_driverType</c> / <c>_mergedConfig</c> / <c>_groupPrefix</c> — and "Connect &amp; browse"
/// would then silently dial the wrong controller and commit under the wrong group. Same race, same
/// fix as the two sibling modals (<c>RawTagModal</c>, <c>RawManualTagEntryModal</c>, 2026-08-05).
/// </para>
/// <para>
/// AdminUI has no bUnit, so the component is driven directly as a class: its injected properties,
/// its <c>[Parameter]</c>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 <c>StateHasChanged</c> on the open path, which throws
/// unless the component holds a render handle — hence the inert renderer below.
/// </para>
/// </remarks>
// 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<InertRenderer> _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<string>(modal, "_mergedConfig").ShouldBe(ConfigB);
Field<string>(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<string?>(modal, "_groupPrefix").ShouldBeNull();
Field<string>(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<bool>(modal, "_resolving").ShouldBeTrue();
deviceBResolve.SetResult();
await liveOpen;
Field<bool>(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<bool>(modal, "_canBrowse").ShouldBeTrue();
Field<string?>(modal, "_disabledReason").ShouldBeNull();
}
/// <inheritdoc/>
public void Dispose()
{
foreach (var renderer in _renderers) { renderer.Dispose(); }
}
/// <summary>Builds a modal with its three injected dependencies set and a render handle attached.</summary>
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;
}
/// <summary>An export reader over a factory that fails loudly — for tests that must never reach it.</summary>
private static RawTagCsvExportReader NullExportReader()
=> new(new GatedDbContextFactory(dbName: null, gate: Task.CompletedTask));
/// <summary>Seeds a lone tag-group row into a fresh InMemory database and returns its name.</summary>
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;
}
/// <summary>
/// Sets the modal's <c>[Parameter]</c>s. By reflection because BL0005 forbids assigning component
/// parameters from outside the component, and this project treats warnings as errors.
/// </summary>
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);
}
}
/// <summary>Sets one of the <c>@inject</c>-generated private properties.</summary>
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);
}
/// <summary>Drives the open path — the lifecycle method the resolve hangs off.</summary>
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)!;
}
/// <summary>Reads one of the modal's private resolve-state fields.</summary>
private static T Field<T>(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)!;
}
/// <summary>
/// An <see cref="IDbContextFactory{TContext}"/> whose context creation can be held open, so a
/// test can suspend a resolve inside <c>ResolveGroupContextAsync</c> and interleave a second open.
/// A null database name means "must never be called".
/// </summary>
private sealed class GatedDbContextFactory(string? dbName, Task gate)
: IDbContextFactory<OtOpcUaConfigDbContext>
{
/// <inheritdoc/>
public OtOpcUaConfigDbContext CreateDbContext()
{
dbName.ShouldNotBeNull("the group-context resolve was reached by a test that forbids it.");
return RawTreeTestDb.CreateNamed(dbName!);
}
/// <inheritdoc/>
public async Task<OtOpcUaConfigDbContext> CreateDbContextAsync(CancellationToken ct = default)
{
await gate;
return CreateDbContext();
}
}
/// <summary>
/// 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.
/// </summary>
private sealed class FakeBrowserSessionService : IBrowserSessionService
{
/// <inheritdoc/>
public bool CanBrowse(string driverType, string configJson) => true;
/// <inheritdoc/>
public Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, CancellationToken ct)
=> throw new NotImplementedException();
/// <inheritdoc/>
public Task<IReadOnlyList<BrowseNode>> RootAsync(Guid token, CancellationToken ct)
=> throw new NotImplementedException();
/// <inheritdoc/>
public Task<IReadOnlyList<BrowseNode>> ExpandAsync(Guid token, string nodeId, CancellationToken ct)
=> throw new NotImplementedException();
/// <inheritdoc/>
public Task<IReadOnlyList<AttributeInfo>> AttributesAsync(Guid token, string nodeId, CancellationToken ct)
=> throw new NotImplementedException();
/// <inheritdoc/>
public Task<int> RequestRebirthAsync(Guid token, string scope, CancellationToken ct)
=> throw new NotImplementedException();
/// <inheritdoc/>
public bool CanRequestRebirth(Guid token) => false;
/// <inheritdoc/>
public Task CloseAsync(Guid token) => throw new NotImplementedException();
}
/// <summary>
/// The smallest renderer that gives a component a working render handle: <c>StateHasChanged</c>
/// 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.
/// </summary>
private sealed class InertRenderer()
: Renderer(new ServiceCollection().BuildServiceProvider(), NullLoggerFactory.Instance)
{
/// <inheritdoc/>
public override Dispatcher Dispatcher { get; } = new InlineDispatcher();
/// <summary>Assigns the component a render handle, as a root component would receive.</summary>
public void Attach(IComponent component) => AssignRootComponentId(component);
/// <inheritdoc/>
protected override void HandleException(Exception exception)
=> ExceptionDispatchInfo.Capture(exception).Throw();
/// <inheritdoc/>
protected override Task UpdateDisplayAsync(in RenderBatch renderBatch) => Task.CompletedTask;
}
/// <summary>
/// Runs everything on the calling thread. The real dispatcher would refuse a
/// <c>StateHasChanged</c> raised from a test thread (or from a resolve continuation), which is
/// exactly what these tests do.
/// </summary>
private sealed class InlineDispatcher : Dispatcher
{
/// <inheritdoc/>
public override bool CheckAccess() => true;
/// <inheritdoc/>
public override Task InvokeAsync(Action workItem)
{
workItem();
return Task.CompletedTask;
}
/// <inheritdoc/>
public override Task InvokeAsync(Func<Task> workItem) => workItem();
/// <inheritdoc/>
public override Task<TResult> InvokeAsync<TResult>(Func<TResult> workItem)
=> Task.FromResult(workItem());
/// <inheritdoc/>
public override Task<TResult> InvokeAsync<TResult>(Func<Task<TResult>> workItem) => workItem();
}
}