fix(adminui): close a browse session that opens after its picker is gone (R04 L4)

Every picker and RawBrowseModal stored the OpenAsync token after the await; if the host was disposed (or the modal closed) meanwhile, its teardown had already run with no token, so the plant-side session lived until the reaper's idle TTL. BrowseOpenGuard.CloseIfOrphaned closes it at once; each host sets _disposed first thing in DisposeAsync (RawBrowseModal also treats !Visible as gone). Refs #745 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 17:51:34 -04:00
parent 1313b94097
commit e16146fa0c
9 changed files with 248 additions and 0 deletions
@@ -0,0 +1,44 @@
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Browsing;
+
+/// <summary>
+///     Closes a browse session that finished opening after its host went away (R04 L4). Every picker and
+///     <c>RawBrowseModal</c> awaited <see cref="IBrowserSessionService.OpenAsync"/> and then stored the token;
+///     if the host was disposed (or the modal closed) meanwhile, its teardown had already run with no token,
+///     so the session — an OPC UA client session, an MQTT connection, a SQL connection against the plant —
+///     stayed open until the reaper's idle TTL (2 min + one 30 s tick).
+/// </summary>
+public static class BrowseOpenGuard
+{
+    /// <summary>
+    ///     Call right after <see cref="IBrowserSessionService.OpenAsync"/> returns. When the host is gone it
+    ///     closes the new session (fire-and-forget, like the hosts' own teardown) and returns
+    ///     <see langword="true"/>, telling the caller not to adopt the token.
+    /// </summary>
+    /// <param name="result">The open result.</param>
+    /// <param name="hostGone">Whether the host was disposed (or closed) while the open was in flight.</param>
+    /// <param name="service">The browse service the session was opened on.</param>
+    /// <returns><see langword="true"/> when the caller must drop the result.</returns>
+    public static bool CloseIfOrphaned(BrowseOpenResult result, bool hostGone, IBrowserSessionService service)
+    {
+        if (!hostGone) return false;
+        if (result.Ok && result.Token != Guid.Empty)
+        {
+            // Never block (or throw into) a dead host's continuation on a slow remote close.
+            _ = CloseQuietlyAsync(service, result.Token);
+        }
+
+        return true;
+    }
+
+    private static async Task CloseQuietlyAsync(IBrowserSessionService service, Guid token)
+    {
+        try
+        {
+            await service.CloseAsync(token).ConfigureAwait(false);
+        }
+        catch
+        {
+            // The reaper still bounds a session whose close failed.
+        }
+    }
+}
@@ -89,6 +89,8 @@
    private string _built = "";

    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private bool _canBrowse;
    private string? _openError;
@@ -110,6 +112,8 @@
        {
            var json = GetConfigJson() ?? "{}";
            var result = await BrowserService.OpenAsync(DriverType, json, default);
+            // Host disposed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)) { return; }
            if (result.Ok) _token = result.Token;
            else _openError = result.Message;
        }
@@ -155,6 +159,7 @@

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow capture.
@@ -164,6 +164,8 @@
    private string _built = "";

    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private bool _canBrowse;
    private string? _openError;
@@ -185,6 +187,8 @@
        {
            var json = GetConfigJson() ?? "{}";
            var result = await BrowserService.OpenAsync(DriverType, json, default);
+            // Host disposed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)) { return; }
            if (result.Ok) _token = result.Token;
            else _openError = result.Message;
        }
@@ -229,6 +233,7 @@

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow capture.
@@ -111,6 +111,8 @@
    private string _attributeName = "";
    private string _built = "";
    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private bool _attrsLoading;
    private bool _canOperate;
@@ -134,6 +136,8 @@
        {
            var json = GetConfigJson() ?? "{}";
            var result = await BrowserService.OpenAsync("GalaxyMxGateway", json, default);
+            // Host disposed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)) { return; }
            if (result.Ok) _token = result.Token;
            else _openError = result.Message;
        }
@@ -216,6 +220,7 @@

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow remote.
@@ -65,6 +65,8 @@
    private string _nodeId = "";
    private string _built = "";
    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private bool _canOperate;
    private string? _openError;
@@ -85,6 +87,8 @@
        {
            var json = GetConfigJson() ?? "{}";
            var result = await BrowserService.OpenAsync("OpcUaClient", json, default);
+            // Host disposed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)) { return; }
            if (result.Ok) _token = result.Token;
            else _openError = result.Message;
        }
@@ -114,6 +118,7 @@

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow remote.
@@ -240,6 +240,8 @@ else
    private string _built = "";
    private string _selectedColumnNodeId = "";
    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private bool _canOperate;
    private string? _openError;
@@ -268,6 +270,8 @@ else
            // sole place it is read, and it dies with the local variable.
            var json = BuildBrowseConfig();
            var result = await BrowserService.OpenAsync(DriverTypeNames.Sql, json, default);
+            // Host disposed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)) { return; }
            if (result.Ok) { _token = result.Token; }
            else { _openError = result.Message; }
        }
@@ -429,6 +433,7 @@ else

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow database.
@@ -76,6 +76,8 @@
    private string _built = "";

    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private bool _canBrowse;
    private string? _openError;
@@ -97,6 +99,8 @@
        {
            var json = GetConfigJson() ?? "{}";
            var result = await BrowserService.OpenAsync(DriverType, json, default);
+            // Host disposed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)) { return; }
            if (result.Ok) _token = result.Token;
            else _openError = result.Message;
        }
@@ -134,6 +138,7 @@

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow capture.
@@ -300,6 +300,8 @@
    private string? _groupPrefix;

    private Guid _token = Guid.Empty;
+    // Set first thing in DisposeAsync; an open that lands afterwards closes its own session.
+    private volatile bool _disposed;
    private bool _opening;
    private string? _openError;

@@ -459,6 +461,8 @@
        try
        {
            var result = await BrowserService.OpenAsync(_driverType, _mergedConfig, default);
+            // Host disposed or modal closed mid-open: close the session nobody will (R04 L4).
+            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed || !Visible, BrowserService)) { return; }
            if (result.Ok)
            {
                _token = result.Token;
@@ -823,6 +827,7 @@

    public ValueTask DisposeAsync()
    {
+        _disposed = true;
        if (_token != Guid.Empty)
        {
            // Fire-and-forget — don't block circuit teardown on a slow remote.
@@ -0,0 +1,169 @@
+using System.Reflection;
+using System.Runtime.ExceptionServices;
+using Microsoft.AspNetCore.Components;
+using Microsoft.AspNetCore.Components.RenderTree;
+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.Drivers.Pickers;
+using ZB.MOM.WW.OtOpcUa.Commons.Browsing;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Pickers;
+
+// BL0006: the test renderer needs RenderTree types (same suppression as RawBrowseModalStaleOpenTests).
+#pragma warning disable BL0006
+
+/// <summary>
+///     R04 L4: a browse session that finishes opening after its picker (or the browse modal) went away is
+///     closed at once instead of living until the reaper's idle TTL.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class BrowseOpenGuardTests : IDisposable
+{
+    private readonly List<Renderer> _renderers = new();
+
+    public void Dispose()
+    {
+        foreach (var r in _renderers) r.Dispose();
+    }
+
+    [Fact]
+    public async Task Guard_closes_an_orphaned_session_and_leaves_a_live_one()
+    {
+        var svc = new RecordingBrowserService();
+        var token = Guid.NewGuid();
+
+        BrowseOpenGuard.CloseIfOrphaned(new BrowseOpenResult(true, null, token), hostGone: false, svc).ShouldBeFalse();
+        svc.Closed.ShouldBeEmpty();
+
+        BrowseOpenGuard.CloseIfOrphaned(new BrowseOpenResult(true, null, token), hostGone: true, svc).ShouldBeTrue();
+        await svc.WaitForCloseAsync();
+        svc.Closed.ShouldBe([token]);
+    }
+
+    [Fact]
+    public void Guard_never_closes_a_failed_open_and_swallows_a_failing_close()
+    {
+        var svc = new RecordingBrowserService();
+        BrowseOpenGuard.CloseIfOrphaned(new BrowseOpenResult(false, "refused", Guid.Empty), hostGone: true, svc).ShouldBeTrue();
+        svc.Closed.ShouldBeEmpty();
+
+        var throwing = new RecordingBrowserService { ThrowOnClose = true };
+        Should.NotThrow(() => BrowseOpenGuard.CloseIfOrphaned(new BrowseOpenResult(true, null, Guid.NewGuid()), true, throwing));
+    }
+
+    [Fact]
+    public async Task Picker_disposed_mid_open_closes_the_session_it_was_handed()
+    {
+        var gate = new TaskCompletionSource<BrowseOpenResult>();
+        var svc = new RecordingBrowserService { Open = () => gate.Task };
+        var picker = NewPicker(svc);
+
+        var open = InvokeOpen(picker);
+        await picker.DisposeAsync();
+        var token = Guid.NewGuid();
+        gate.SetResult(new BrowseOpenResult(true, null, token));
+        await open;
+
+        await svc.WaitForCloseAsync();
+        svc.Closed.ShouldBe([token]);
+        Token(picker).ShouldBe(Guid.Empty);
+    }
+
+    [Fact]
+    public async Task Picker_still_open_adopts_the_session()
+    {
+        // Positive control for the test above: without the dispose the token is adopted, not closed.
+        var token = Guid.NewGuid();
+        var svc = new RecordingBrowserService { Open = () => Task.FromResult(new BrowseOpenResult(true, null, token)) };
+        var picker = NewPicker(svc);
+
+        await InvokeOpen(picker);
+
+        Token(picker).ShouldBe(token);
+        svc.Closed.ShouldBeEmpty();
+    }
+
+    [Fact]
+    public void Every_browse_host_guards_its_open()
+    {
+        foreach (var path in new[]
+                 {
+                     "AbCipAddressPickerBody.razor", "FOCASAddressPickerBody.razor", "GalaxyAddressPickerBody.razor",
+                     "OpcUaClientAddressPickerBody.razor", "TwinCATAddressPickerBody.razor", "SqlAddressPickerBody.razor",
+                 })
+        {
+            var src = AdminUiSource.Read("Components", "Shared", "Drivers", "Pickers", path);
+            src.ShouldContain("BrowseOpenGuard.CloseIfOrphaned(result, _disposed, BrowserService)", customMessage: path);
+            src.ShouldContain("_disposed = true;", customMessage: path);
+        }
+
+        var modal = AdminUiSource.Read("Components", "Shared", "Raw", "RawBrowseModal.razor");
+        modal.ShouldContain("BrowseOpenGuard.CloseIfOrphaned(result, _disposed || !Visible, BrowserService)");
+    }
+
+    private OpcUaClientAddressPickerBody NewPicker(IBrowserSessionService svc)
+    {
+        var picker = new OpcUaClientAddressPickerBody();
+        typeof(OpcUaClientAddressPickerBody).GetProperty("BrowserService", BindingFlags.NonPublic | BindingFlags.Instance)!
+            .SetValue(picker, svc);
+        var renderer = new InertRenderer();
+        _renderers.Add(renderer);
+        renderer.Attach(picker);
+        return picker;
+    }
+
+    private static Task InvokeOpen(OpcUaClientAddressPickerBody picker) =>
+        (Task)typeof(OpcUaClientAddressPickerBody).GetMethod("OpenBrowseAsync", BindingFlags.NonPublic | BindingFlags.Instance)!
+            .Invoke(picker, null)!;
+
+    private static Guid Token(OpcUaClientAddressPickerBody picker) =>
+        (Guid)typeof(OpcUaClientAddressPickerBody).GetField("_token", BindingFlags.NonPublic | BindingFlags.Instance)!
+            .GetValue(picker)!;
+
+    private sealed class RecordingBrowserService : IBrowserSessionService
+    {
+        private readonly TaskCompletionSource _closed = new(TaskCreationOptions.RunContinuationsAsynchronously);
+        public List<Guid> Closed { get; } = new();
+        public bool ThrowOnClose { get; init; }
+        public Func<Task<BrowseOpenResult>> Open { get; init; } = () => Task.FromResult(new BrowseOpenResult(false, "n/a", Guid.Empty));
+
+        public Task WaitForCloseAsync() => _closed.Task.WaitAsync(TimeSpan.FromSeconds(10));
+
+        public bool CanBrowse(string driverType, string configJson) => true;
+        public Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, CancellationToken ct) => Open();
+        public Task<IReadOnlyList<BrowseNode>> RootAsync(Guid token, CancellationToken ct) => throw new NotImplementedException();
+        public Task<IReadOnlyList<BrowseNode>> ExpandAsync(Guid token, string nodeId, CancellationToken ct) => throw new NotImplementedException();
+        public Task<IReadOnlyList<AttributeInfo>> AttributesAsync(Guid token, string nodeId, CancellationToken ct) => throw new NotImplementedException();
+        public Task<int> RequestRebirthAsync(Guid token, string scope, CancellationToken ct) => throw new NotImplementedException();
+        public bool CanRequestRebirth(Guid token) => false;
+
+        public Task CloseAsync(Guid token)
+        {
+            if (ThrowOnClose) throw new InvalidOperationException("remote gone");
+            lock (Closed) Closed.Add(token);
+            _closed.TrySetResult();
+            return Task.CompletedTask;
+        }
+    }
+
+    private sealed class InertRenderer()
+        : Renderer(new ServiceCollection().BuildServiceProvider(), NullLoggerFactory.Instance)
+    {
+        public override Dispatcher Dispatcher { get; } = new InlineDispatcher();
+        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;
+    }
+
+    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<Task> workItem) => workItem();
+        public override Task<TResult> InvokeAsync<TResult>(Func<TResult> workItem) => Task.FromResult(workItem());
+        public override Task<TResult> InvokeAsync<TResult>(Func<Task<TResult>> workItem) => workItem();
+    }
+}