diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/ApiMethodForm.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/ApiMethodForm.razor index 56d1dc2a..1779fa92 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/ApiMethodForm.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/ApiMethodForm.razor @@ -11,6 +11,7 @@ @inject IInboundApiKeyAdmin ApiKeyAdmin @inject ScriptAnalysis.ScriptAnalysisService AnalysisService @inject NavigationManager NavigationManager +@implements IDisposable
@@ -377,6 +378,14 @@ private void GoBack() => NavigationManager.NavigateTo("/design/external-systems"); + /// Cancels an in-flight Test Run when the component is torn down. + public void Dispose() + { + _runCts?.Cancel(); + _runCts?.Dispose(); + _runCts = null; + } + private void ToggleTestRunPanel() => _showTestRun = !_showTestRun; private async Task RunInSandboxAsync() diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SharedScriptForm.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SharedScriptForm.razor index ef725ea7..7c47ea2c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SharedScriptForm.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/SharedScriptForm.razor @@ -11,6 +11,7 @@ @inject ScriptAnalysis.ScriptAnalysisService AnalysisService @inject AuthenticationStateProvider AuthStateProvider @inject NavigationManager NavigationManager +@implements IDisposable
@@ -241,6 +242,14 @@ NavigationManager.NavigateTo("/design/shared-scripts"); } + /// Cancels an in-flight Test Run when the component is torn down. + public void Dispose() + { + _runCts?.Cancel(); + _runCts?.Dispose(); + _runCts = null; + } + private void CheckCompilation() { var syntaxError = ValidateSyntaxLocally(_formCode); diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs index 6b6a0882..9f5c687f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs @@ -242,7 +242,7 @@ public class ScriptAnalysisService /// The sandbox run request including code, kind, parameters, and optional instance binding. /// Cancellation token for the caller; combined with an internal timeout CTS. /// A describing the outcome, return value, console output, and any error. - public async Task RunInSandboxAsync(SandboxRunRequest request, CancellationToken ct) + public virtual async Task RunInSandboxAsync(SandboxRunRequest request, CancellationToken ct) { if (string.IsNullOrWhiteSpace(request.Code)) { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/ScriptFormSandboxDisposalTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/ScriptFormSandboxDisposalTests.cs new file mode 100644 index 00000000..f4efdd8c --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Design/ScriptFormSandboxDisposalTests.cs @@ -0,0 +1,286 @@ +using System.Reflection; +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.DependencyInjection; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.CentralUI.ScriptAnalysis; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Security; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Security; +using ZB.MOM.WW.ScadaBridge.TemplateEngine; +using ApiMethodFormPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Design.ApiMethodForm; +using SharedScriptFormPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Design.SharedScriptForm; + +namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Design; + +/// +/// Regression tests for arch-review P6-12. ApiMethodForm and +/// SharedScriptForm run a Test Run sandbox script via +/// AnalysisService.RunInSandboxAsync(request, _runCts.Token) but — unlike +/// TemplateScriptDialog, which cancels/disposes _runCts from an +/// Dispose() — neither form implemented +/// at all. An abandoned run's _runCts was never +/// cancelled, so the sandbox kept the run alive until its own internal script +/// timeout; when it finally completed, the continuation touched a torn-down +/// component's _running/_runResult fields. The fix mirrors +/// TemplateScriptDialog exactly: @implements IDisposable plus a +/// Dispose() that cancels + disposes _runCts. +/// +/// holds a run open on a +/// gate so the dispose-mid-run window is +/// deterministic (never Task.Delay ordering) — same pattern as +/// DebugViewConnectDisposeRaceTests.GatedDebugStreamService. Overriding +/// requires ScriptAnalysisService.RunInSandboxAsync to be virtual — +/// a test-only seam added alongside this fix (mirroring the pre-existing +/// virtual on DebugStreamService.StartStreamAsync/StopStream), +/// not itself part of the P6-12 defect or its fix. +/// +/// +/// Proof of failure: stashing only the two .razor changes (leaving +/// the virtual seam in place) and re-running +/// +/// and +/// +/// fails both: with no on the form, (cut.Instance as +/// IDisposable) is null, so the disposal no-ops and the token the +/// sandbox observed is never cancelled. +/// +/// +public class ApiMethodFormSandboxDisposalTests : BunitContext +{ + private (IRenderedComponent Cut, GatedScriptAnalysisService Sandbox) RenderPage() + { + // ApiMethodForm mounts a MonacoEditor unconditionally; let bUnit answer its + // JS interop calls loosely (same convention as DebugView's disposal tests). + JSInterop.Mode = JSRuntimeMode.Loose; + + var repo = Substitute.For(); + Services.AddSingleton(repo); + + var keyAdmin = Substitute.For(); + keyAdmin.ListAsync(Arg.Any()) + .Returns(Array.Empty()); + Services.AddSingleton(keyAdmin); + + var sandbox = new GatedScriptAnalysisService(); + Services.AddSingleton(sandbox); + + var claims = new[] + { + new Claim(JwtTokenService.UsernameClaimType, "tester"), + new Claim(JwtTokenService.RoleClaimType, "Designer"), + }; + var user = new ClaimsPrincipal(new ClaimsIdentity(claims, "TestAuth")); + Services.AddSingleton(new TestAuthStateProvider(user)); + Services.AddAuthorizationCore(); + + return (Render(), sandbox); + } + + /// + /// Starts RunInSandboxAsync() on the renderer's dispatcher (where it really + /// runs) and hands back its task without awaiting it, so the test can act inside the + /// run window. + /// + private static async Task StartRunAsync(IRenderedComponent cut) + { + var run = typeof(ApiMethodFormPage).GetMethod( + "RunInSandboxAsync", BindingFlags.Instance | BindingFlags.NonPublic)!; + Task? task = null; + await cut.InvokeAsync(() => { task = (Task)run.Invoke(cut.Instance, null)!; }); + return task!; + } + + private static object? GetField(ApiMethodFormPage page, string name) => + typeof(ApiMethodFormPage).GetField(name, BindingFlags.Instance | BindingFlags.NonPublic)! + .GetValue(page); + + [Fact] + public async Task Dispose_MidRun_CancelsTheSandboxRunToken() + { + var (cut, sandbox) = RenderPage(); + + var run = await StartRunAsync(cut); + await sandbox.Entered.WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.False(sandbox.LastToken.IsCancellationRequested, + "the run must still be in flight, unabandoned, before dispose"); + + // The user navigates away while the run is still in flight. Dispose runs off + // the renderer's thread, exactly as a circuit teardown can. + (cut.Instance as IDisposable)?.Dispose(); + + Assert.True(sandbox.LastToken.IsCancellationRequested, + "Dispose() must cancel _runCts so the sandbox stops the abandoned run " + + "instead of parking it until the script timeout"); + Assert.Null(GetField(cut.Instance, "_runCts")); + + // Unblock the gated run so nothing is left hanging past this test. + sandbox.ReleaseRun(); + await run.WaitAsync(TimeSpan.FromSeconds(5)); + } + + [Fact] + public async Task Dispose_MidRun_ContinuationIsANoOp() + { + var (cut, sandbox) = RenderPage(); + + var run = await StartRunAsync(cut); + await sandbox.Entered.WaitAsync(TimeSpan.FromSeconds(5)); + + (cut.Instance as IDisposable)?.Dispose(); + + try + { + // The cancelled run's continuation must resolve on its own (no + // Task.Delay-based waiting) and must not surface an unhandled exception — + // RunInSandboxAsync's own catch(OperationCanceledException) swallows it. + var ex = await Record.ExceptionAsync(() => run.WaitAsync(TimeSpan.FromSeconds(5))); + Assert.Null(ex); + + // No state mutation: the disposed run must not have been (mis)recorded as + // an "Unexpected: ..." runtime error — it stays exactly as it started. + Assert.Null(GetField(cut.Instance, "_runResult")); + Assert.False((bool)GetField(cut.Instance, "_running")!); + } + finally + { + // Safety net so an unfixed run (never cancelled) doesn't hang past this test. + sandbox.ReleaseRun(); + } + } +} + +public class SharedScriptFormSandboxDisposalTests : BunitContext +{ + private (IRenderedComponent Cut, GatedScriptAnalysisService Sandbox) RenderPage() + { + // SharedScriptForm mounts a MonacoEditor unconditionally; let bUnit answer its + // JS interop calls loosely (same convention as DebugView's disposal tests). + JSInterop.Mode = JSRuntimeMode.Loose; + + var repo = Substitute.For(); + var audit = Substitute.For(); + Services.AddSingleton(repo); + Services.AddSingleton(audit); + Services.AddScoped(); + + var sandbox = new GatedScriptAnalysisService(); + Services.AddSingleton(sandbox); + + var claims = new[] + { + new Claim(JwtTokenService.UsernameClaimType, "tester"), + new Claim(JwtTokenService.RoleClaimType, "Designer"), + }; + var user = new ClaimsPrincipal(new ClaimsIdentity(claims, "TestAuth")); + Services.AddSingleton(new TestAuthStateProvider(user)); + Services.AddAuthorizationCore(); + + return (Render(), sandbox); + } + + private static async Task StartRunAsync(IRenderedComponent cut) + { + var run = typeof(SharedScriptFormPage).GetMethod( + "RunInSandboxAsync", BindingFlags.Instance | BindingFlags.NonPublic)!; + Task? task = null; + await cut.InvokeAsync(() => { task = (Task)run.Invoke(cut.Instance, null)!; }); + return task!; + } + + private static object? GetField(SharedScriptFormPage page, string name) => + typeof(SharedScriptFormPage).GetField(name, BindingFlags.Instance | BindingFlags.NonPublic)! + .GetValue(page); + + [Fact] + public async Task Dispose_MidRun_CancelsTheSandboxRunToken() + { + var (cut, sandbox) = RenderPage(); + + var run = await StartRunAsync(cut); + await sandbox.Entered.WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.False(sandbox.LastToken.IsCancellationRequested, + "the run must still be in flight, unabandoned, before dispose"); + + (cut.Instance as IDisposable)?.Dispose(); + + Assert.True(sandbox.LastToken.IsCancellationRequested, + "Dispose() must cancel _runCts so the sandbox stops the abandoned run " + + "instead of parking it until the script timeout"); + Assert.Null(GetField(cut.Instance, "_runCts")); + + sandbox.ReleaseRun(); + await run.WaitAsync(TimeSpan.FromSeconds(5)); + } + + [Fact] + public async Task Dispose_MidRun_ContinuationIsANoOp() + { + var (cut, sandbox) = RenderPage(); + + var run = await StartRunAsync(cut); + await sandbox.Entered.WaitAsync(TimeSpan.FromSeconds(5)); + + (cut.Instance as IDisposable)?.Dispose(); + + try + { + var ex = await Record.ExceptionAsync(() => run.WaitAsync(TimeSpan.FromSeconds(5))); + Assert.Null(ex); + + Assert.Null(GetField(cut.Instance, "_runResult")); + Assert.False((bool)GetField(cut.Instance, "_running")!); + } + finally + { + sandbox.ReleaseRun(); + } + } +} + +/// +/// A whose sandbox run can be held open on demand +/// via a gate, so the dispose-mid-run window is +/// deterministic across both script forms' tests. Overriding +/// requires it to be +/// virtual — see the class-level remarks on +/// . +/// +internal sealed class GatedScriptAnalysisService : ScriptAnalysisService +{ + private readonly TaskCompletionSource _gate = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _entered = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public GatedScriptAnalysisService() + : base( + Substitute.For(), + new MemoryCache(new MemoryCacheOptions()), + new ServiceCollection().BuildServiceProvider()) + { + } + + /// Completes once has been entered. + public Task Entered => _entered.Task; + + /// The cancellation token supplied to the most recent call. + public CancellationToken LastToken { get; private set; } + + /// Completes the held-open run with a successful, inert result. + public void ReleaseRun() => _gate.TrySetResult( + new SandboxRunResult(true, "1", "int", "", null, SandboxErrorKind.None, 1, null)); + + public override async Task RunInSandboxAsync(SandboxRunRequest request, CancellationToken ct) + { + LastToken = ct; + _entered.TrySetResult(); + using var reg = ct.Register(() => _gate.TrySetCanceled(ct)); + return await _gate.Task; + } +}