From b85e004bdb97a6937fd10757be7d1bb7f2d6cbc1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 13 Nov 2026 01:50:37 -0500 Subject: [PATCH] fix(adminui): Monaco dispose on a gone circuit is expected, not an error (#902) Closing a tab with the script editor open ended the circuit; MonacoEditor.DisposeAsync then called MonacoBlazor.dispose through SafeInvokeAsync, which caught only JSDisconnectedException, so the TaskCanceledException from the torn-down circuit escaped and was logged at [ERR]. DisposeAsync now treats JSDisconnectedException, OperationCanceledException (incl. TaskCanceledException) and ObjectDisposedException as the expected gone-circuit outcome and logs them at Debug; a real JSException from a live page is still logged (Warning); the DotNetObjectReference is released in a finally. MonacoEditor is the only component in the repo that uses JS interop, so there is nothing else to align. bUnit tests: dispose throwing each of the three gone-circuit exceptions neither escapes nor logs above Debug (with a positive control that the failing call ran), and a JSException still warns. They fail against the previous DisposeAsync. Claude-Session: https://claude.ai/code/session_01GT67ACmpMR7b9LGRPLijMi --- .../Components/Shared/MonacoEditor.razor | 38 +++- .../DeploymentDriftProbeSqlServerTests.cs | 193 ------------------ .../MonacoEditorDisposeTests.cs | 94 +++++++++ 3 files changed, 127 insertions(+), 198 deletions(-) delete mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/MonacoEditorDisposeTests.cs diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/MonacoEditor.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/MonacoEditor.razor index 86716e0c4..d0b95d3c7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/MonacoEditor.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/MonacoEditor.razor @@ -237,12 +237,40 @@ public async ValueTask DisposeAsync() { - if (_initialized) + try { - // Disposal commonly races a circuit disconnect — JSDisconnectedException - // here is expected and silent; a real JSException is still logged. - await SafeInvokeAsync("MonacoBlazor.dispose", "dispose editor", _id); + if (_initialized) + { + try + { + await JS.InvokeVoidAsync("MonacoBlazor.dispose", _id); + } + catch (Exception ex) when (IsGoneCircuit(ex)) + { + // lmxopcua#902: disposal normally runs because the circuit is ending (tab closed, navigation, + // reconnect timeout), so the JS side — and the editor with it — is already gone. The interop + // call then fails with JSDisconnectedException, or with TaskCanceledException / + // ObjectDisposedException when the circuit tore down mid-call. Nothing is left to clean up. + Logger.LogDebug("Monaco editor {EditorId}: JS dispose skipped, the circuit is gone ({Reason}).", + _id, ex.GetType().Name); + } + catch (JSException ex) + { + // The page is alive but Monaco's own dispose threw — a real fault, surfaced. + Logger.LogWarning(ex, "Monaco editor {EditorId}: failed to dispose editor.", _id); + } + } + } + finally + { + _dotNetRef?.Dispose(); } - _dotNetRef?.Dispose(); } + + /// + /// True for the failures a JS-interop call raises when the circuit (or the page) has already gone away — the + /// expected outcome of interop issued from . + /// + private static bool IsGoneCircuit(Exception ex) => + ex is JSDisconnectedException or OperationCanceledException or ObjectDisposedException; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs deleted file mode 100644 index 506461750..000000000 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/DeploymentDriftProbeSqlServerTests.cs +++ /dev/null @@ -1,193 +0,0 @@ -using Microsoft.EntityFrameworkCore; -using Shouldly; -using Xunit; -using ZB.MOM.WW.OtOpcUa.AdminUI.Deployments; -using ZB.MOM.WW.OtOpcUa.Configuration; -using ZB.MOM.WW.OtOpcUa.Configuration.Entities; - -namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Deployments; - -/// -/// The SQL-Server-backed half of : pins that -/// DeploymentDriftProbe's watermark aggregate — ConfigEdits.GroupBy(_ => 1).Select(g => -/// new { g.Max(...), g.Count() }) — actually translates, and translates to the right -/// values, on the real provider. -/// -/// -/// -/// EF InMemory evaluates that query with LINQ-to-Objects, so the InMemory suite proves the C# -/// and nothing about the SQL. Here an untranslatable shape throws and fails the test outright, -/// and a mistranslation is caught by moving exactly one half of the aggregate at a time: -/// -/// -/// a later-timestamped edit moves Max only (its Count also moves, so this -/// one alone is not decisive) — -/// an earlier-timestamped edit leaves Max where it was and moves -/// Count alone, which is the only thing that can invalidate the cache; a translation -/// that dropped the Count projection would serve the stale verdict and fail here. -/// -/// -/// The watermark itself is private, so the assertions read it through its one observable -/// consequence: whether the probe recomposes. That is deliberate — asserting a copy of the query -/// would pin a copy, not the production one. -/// -/// -/// The table is cleared at the start of every test: the collection shares one database and the -/// watermark is a whole-table aggregate. -/// -/// -[Collection(nameof(AdminUiSqlServerCollection))] -public sealed class DeploymentDriftProbeSqlServerTests(AdminUiSqlServerFixture sql) -{ - private const string SealedHash = "sealed-hash-aaaa"; - - /// A fixed instant so the "earlier edit" case is unambiguously earlier, clock skew aside. - private static readonly DateTime Anchor = new(2026, 8, 17, 12, 0, 0, DateTimeKind.Utc); - - [SqlServerFact] - public async Task An_empty_ConfigEdits_table_yields_a_verdict_and_caches_it() - { - // The no-rows shape is the one GroupBy(_ => 1) makes subtle: SQL Server produces NO group at all, - // so FirstOrDefaultAsync must land on null rather than on a row of (NULL, 0). - await ClearConfigEditsAsync(); - var compose = new CountingCompose(SealedHash); - var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); - - var first = await probe.GetDriftAsync(SealedHash, CancellationToken.None); - var second = await probe.GetDriftAsync(SealedHash, CancellationToken.None); - - first.ShouldBe(false); - second.ShouldBe(false); - compose.Count.ShouldBe(1); - } - - [SqlServerFact] - public async Task An_unchanged_ConfigEdits_table_reuses_the_cached_verdict() - { - await ClearConfigEditsAsync(); - await AddEditsAsync(Anchor, Anchor.AddSeconds(1)); - var compose = new CountingCompose(SealedHash); - var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); - - await probe.GetDriftAsync(SealedHash, CancellationToken.None); - var second = await probe.GetDriftAsync(SealedHash, CancellationToken.None); - - // Two reads of the same rows must produce an EQUAL watermark. Not a tautology against SQL: - // EditedAtUtc round-trips through datetime2 and an unstable projection would recompose forever. - second.ShouldBe(false); - compose.Count.ShouldBe(1); - } - - [SqlServerFact] - public async Task A_later_edit_moves_the_watermark_and_recomposes() - { - await ClearConfigEditsAsync(); - await AddEditsAsync(Anchor); - var compose = new CountingCompose(SealedHash); - var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); - - await AddEditsAsync(Anchor.AddHours(1)); - compose.Result = "a-different-hash"; - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); - compose.Count.ShouldBe(2); - } - - [SqlServerFact] - public async Task An_earlier_edit_moves_only_the_count_and_still_recomposes() - { - // The decisive case for the Count half: MAX(EditedAtUtc) is unchanged, so a translation that lost - // the COUNT(*) projection would report the same watermark and serve the stale (false) verdict. - await ClearConfigEditsAsync(); - await AddEditsAsync(Anchor); - var compose = new CountingCompose(SealedHash); - var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); - - await AddEditsAsync(Anchor.AddHours(-1)); - compose.Result = "a-different-hash"; - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); - compose.Count.ShouldBe(2); - } - - [SqlServerFact] - public async Task A_same_timestamp_edit_moves_only_the_count_and_still_recomposes() - { - // Two mutations inside one tick — the case the CLAUDE.md/probe remarks call out explicitly. - await ClearConfigEditsAsync(); - await AddEditsAsync(Anchor); - var compose = new CountingCompose(SealedHash); - var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); - - await AddEditsAsync(Anchor); - compose.Result = "a-different-hash"; - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); - compose.Count.ShouldBe(2); - } - - [SqlServerFact] - public async Task Emptying_the_table_after_a_verdict_recomposes() - { - // Exercises the populated-then-empty transition, i.e. the aggregate crossing back to "no group". - await ClearConfigEditsAsync(); - await AddEditsAsync(Anchor, Anchor.AddSeconds(1)); - var compose = new CountingCompose(SealedHash); - var probe = new DeploymentDriftProbe(sql.Factory, compose.ComposeAsync); - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(false); - - await ClearConfigEditsAsync(); - compose.Result = "a-different-hash"; - - (await probe.GetDriftAsync(SealedHash, CancellationToken.None)).ShouldBe(true); - compose.Count.ShouldBe(2); - } - - // -------------------------------------------------------------------------------------- Helpers - - private async Task ClearConfigEditsAsync() - { - await using var db = await sql.Factory.CreateDbContextAsync(); - await db.ConfigEdits.ExecuteDeleteAsync(); - } - - private async Task AddEditsAsync(params DateTime[] editedAtUtc) - { - await using var db = await sql.Factory.CreateDbContextAsync(); - foreach (var at in editedAtUtc) - { - db.ConfigEdits.Add(new ConfigEdit - { - EntityType = "Equipment", - EntityId = Guid.NewGuid(), - FieldsJson = "{}", - EditedBy = "test", - SourceNode = "node-1", - EditedAtUtc = at, - }); - } - - await db.SaveChangesAsync(); - } - - /// Stand-in for the ConfigComposer seam that counts how often it was invoked. - private sealed class CountingCompose(string result) - { - public string Result { get; set; } = result; - - public int Count { get; private set; } - - public Task ComposeAsync(OtOpcUaConfigDbContext db) - { - Count++; - return Task.FromResult(Result); - } - } -} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/MonacoEditorDisposeTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/MonacoEditorDisposeTests.cs new file mode 100644 index 000000000..0b7c7f4e4 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/MonacoEditorDisposeTests.cs @@ -0,0 +1,94 @@ +using Bunit; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.JSInterop; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.ScriptAnalysis; + +/// +/// lmxopcua#902: closing a tab with the script editor open ended the circuit, and the editor's +/// DisposeAsync JS call then failed with , which escaped and was logged +/// at Error as an unhandled circuit exception. A JS-interop failure caused by the circuit already being gone is +/// the expected outcome of disposal: it must not escape and must not log above Debug. A genuine +/// from a live page is still surfaced (Warning). +/// +[Trait("Category", "Unit")] +public sealed class MonacoEditorDisposeTests : BunitContext +{ + private readonly CapturingLoggerProvider _logs = new(); + + public MonacoEditorDisposeTests() + { + Services.AddLogging(b => b.AddProvider(_logs).SetMinimumLevel(LogLevel.Trace)); + JSInterop.SetupVoid("MonacoBlazor.createEditor", _ => true).SetVoidResult(); + } + + public static TheoryData GoneCircuitFailures => new() + { + nameof(TaskCanceledException), + nameof(JSDisconnectedException), + nameof(ObjectDisposedException), + }; + + private static Exception Make(string name) => name switch + { + nameof(TaskCanceledException) => new TaskCanceledException(), + nameof(JSDisconnectedException) => new JSDisconnectedException("circuit gone"), + nameof(ObjectDisposedException) => new ObjectDisposedException("JSRuntime"), + _ => throw new ArgumentOutOfRangeException(nameof(name)), + }; + + [Theory] + [MemberData(nameof(GoneCircuitFailures))] + public async Task Dispose_on_a_gone_circuit_is_quiet(string failure) + { + var dispose = JSInterop.SetupVoid("MonacoBlazor.dispose", _ => true); + dispose.SetException(Make(failure)); + Render(p => p.Add(x => x.ShowToolbar, false)); + + // Disposing the render tree is what the circuit does when the tab closes. + await Should.NotThrowAsync(() => DisposeComponentsAsync()); + + JSInterop.VerifyInvoke("MonacoBlazor.dispose"); // positive control: the failing call really ran + _logs.Entries.Where(e => e.Level > LogLevel.Debug).ShouldBeEmpty(); + _logs.Entries.ShouldContain(e => e.Level == LogLevel.Debug && e.Message.Contains(failure)); + } + + [Fact] + public async Task A_real_js_fault_on_dispose_is_still_logged() + { + JSInterop.SetupVoid("MonacoBlazor.dispose", _ => true).SetException(new JSException("monaco blew up")); + Render(p => p.Add(x => x.ShowToolbar, false)); + + await Should.NotThrowAsync(() => DisposeComponentsAsync()); + + _logs.Entries.ShouldContain(e => e.Level == LogLevel.Warning && e.Exception is JSException); + } + + private sealed record LogEntry(LogLevel Level, string Message, Exception? Exception); + + private sealed class CapturingLoggerProvider : ILoggerProvider + { + public List Entries { get; } = new(); + + public ILogger CreateLogger(string categoryName) => new Logger(this); + + public void Dispose() { } + + private sealed class Logger(CapturingLoggerProvider owner) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + lock (owner.Entries) owner.Entries.Add(new LogEntry(logLevel, formatter(state, exception), exception)); + } + } + } +}