diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Topology.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Topology.razor index 22dcd817..1eae3e83 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Topology.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/Topology.razor @@ -160,6 +160,21 @@ private Timer? _liveUpdatesTimer; private static readonly TimeSpan LiveUpdatesInterval = TimeSpan.FromSeconds(15); + /// + /// Guards together with + /// (register row 103, the P6-6 lesser instance — mirrors DebugView's + /// _sessionLock). awaits + /// before calling ; the + /// circuit can be torn down inside that window, and Dispose may run on a + /// non-UI thread, so creating/replacing the timer and observing disposal must be one + /// atomic step — otherwise a timer created after Dispose already ran is never + /// disposed and ticks forever against a torn-down component. + /// + private readonly object _timerLock = new(); + + /// Set once has run; checked under . + private volatile bool _disposed; + private TreeView _tree = default!; private object? _selectedKey; private const string SelectedKeyStorage = "topology-tree-selected"; @@ -194,23 +209,51 @@ StartLiveUpdatesTimer(); } + /// + /// (Re)creates the live-update timer, or leaves none running when disposed / live + /// updates are off. Register row 103: creation and disposal of any timer this method + /// is replacing both happen under together with the + /// check, so a call that resumes after + /// already ran refuses to create a new timer instead of leaking one. + /// private void StartLiveUpdatesTimer() { - _liveUpdatesTimer?.Dispose(); - if (!_liveUpdates) return; - _liveUpdatesTimer = new Timer(_ => + Timer? old; + lock (_timerLock) { - InvokeAsync(async () => + old = _liveUpdatesTimer; + _liveUpdatesTimer = null; + if (!_disposed && _liveUpdates) + { + _liveUpdatesTimer = new Timer( + OnLiveUpdatesTick, null, LiveUpdatesInterval, LiveUpdatesInterval); + } + } + old?.Dispose(); + } + + private void OnLiveUpdatesTick(object? state) + { + if (_disposed) return; + InvokeAsync(async () => + { + if (_disposed || !_liveUpdates) return; + try { - if (!_liveUpdates) return; // Staleness is deliberately OFF the live loop: it only changes when a // template/instance is edited or redeployed, and recomputing it (a // full re-flatten per deployed instance) every 15s is the page's // dominant cost. The poll refreshes only the cheap deployed state. await LoadDeployedStateAsync(); + if (_disposed) return; StateHasChanged(); - }); - }, null, LiveUpdatesInterval, LiveUpdatesInterval); + } + catch (ObjectDisposedException) + { + // Component disposed between the guard and the dispatch — ignore + // (mirrors DebugView's SafeInvokeAsync / CentralUI-009). + } + }); } private void OnLiveUpdatesToggled(ChangeEventArgs e) @@ -222,14 +265,37 @@ } else { - _liveUpdatesTimer?.Dispose(); - _liveUpdatesTimer = null; + StopLiveUpdatesTimer(); } } + private void StopLiveUpdatesTimer() + { + Timer? timer; + lock (_timerLock) + { + timer = _liveUpdatesTimer; + _liveUpdatesTimer = null; + } + timer?.Dispose(); + } + + /// + /// Register row 103: the flag is set and the timer taken under the same lock + /// creates through, so a still-pending + /// either hands its new timer over here (never + /// created) or this method has nothing left to dispose — never both create AND leak. + /// public void Dispose() { - _liveUpdatesTimer?.Dispose(); + Timer? timer; + lock (_timerLock) + { + _disposed = true; + timer = _liveUpdatesTimer; + _liveUpdatesTimer = null; + } + timer?.Dispose(); } protected override async Task OnAfterRenderAsync(bool firstRender) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs index dd4b93dd..6dc7e139 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs @@ -110,6 +110,20 @@ public partial class TransportImport : ComponentBase, IDisposable private BundleSession? _session; private bool _uploadInProgress; + /// + /// Guards together with (register row + /// 103, the P6-6 lesser instance — mirrors DebugView's _sessionLock). + /// awaits , and the + /// circuit can be torn down inside that window; publishing the freshly-opened session + /// and observing disposal must be one atomic step so exactly one side (this method or + /// ) releases it — otherwise the session is neither published nor + /// abandoned and strands one of MaxConcurrentImportSessions for the full TTL. + /// + private readonly object _sessionLock = new(); + + /// Set once has run; checked under . + private volatile bool _disposed; + // Staging directory for in-flight bundle uploads. Lives under the system // temp directory rather than wwwroot/ because the file is never served to // a browser — it is only read by the in-process IBundleImporter. @@ -241,8 +255,25 @@ public partial class TransportImport : ComponentBase, IDisposable using var stream = new FileStream( _bundleTempPath, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 81920, useAsync: true); - _session = await BundleImporter.LoadAsync( + var loaded = await BundleImporter.LoadAsync( stream, passphrase, CancellationToken.None, ClientAddress.ClientAddress); + + // Register row 103 (P6-6 lesser instance): the component may have been + // disposed while LoadAsync was in flight (navigation away mid-upload). + // Publish and re-check disposal atomically under _sessionLock so exactly + // one side tears the freshly-opened session down. + bool abandoned; + lock (_sessionLock) + { + abandoned = _disposed; + if (!abandoned) _session = loaded; + } + if (abandoned) + { + try { BundleImporter.Abandon(loaded.SessionId); } catch { /* best-effort */ } + return; + } + _errorMessage = null; } catch (ArgumentException) @@ -921,8 +952,12 @@ public partial class TransportImport : ComponentBase, IDisposable /// private void ReleaseSession() { - var session = _session; - _session = null; + BundleSession? session; + lock (_sessionLock) + { + session = _session; + _session = null; + } if (session is null) return; try { @@ -938,10 +973,26 @@ public partial class TransportImport : ComponentBase, IDisposable /// Ensures neither the staged temp file nor the server-side bundle session /// survives circuit teardown. Blazor invokes Dispose when the user navigates /// away or the circuit ends, so an abandoned wizard cleans up automatically. + /// + /// Register row 103: the flag is set and the session taken under the same lock + /// publishes through, so a still + /// awaiting either hands the session over here + /// or abandons it itself — never neither (mirrors DebugView's P6-6 fix). + /// /// public void Dispose() { - ReleaseSession(); + BundleSession? session; + lock (_sessionLock) + { + _disposed = true; + session = _session; + _session = null; + } + if (session is not null) + { + try { BundleImporter.Abandon(session.SessionId); } catch { /* best-effort */ } + } DeleteBundleTempFile(); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportSessionDisposeRaceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportSessionDisposeRaceTests.cs new file mode 100644 index 00000000..3e10f925 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportSessionDisposeRaceTests.cs @@ -0,0 +1,227 @@ +using System.Reflection; +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.ScadaBridge.CentralUI.Auth; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase; +using ZB.MOM.WW.ScadaBridge.Security; +using ZB.MOM.WW.ScadaBridge.Transport; +using ZB.MOM.WW.ScadaBridge.Transport.Import; +using NSubstitute; +using TransportImportPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Design.TransportImport; + +namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Pages.Design; + +/// +/// Regression tests for register row 103 (the P6-6 lesser instance in +/// TransportImport). TryLoadAsync awaits +/// , then used to publish the result into +/// _session unconditionally. If the circuit was torn down inside that await +/// (user navigates away mid-upload), Dispose would have already run, found +/// _session still null, and released nothing — so the freshly-opened server-side +/// BundleSession stayed registered, stranding one of +/// TransportOptions.MaxConcurrentImportSessions (register row 43) for the full +/// session TTL. The fix publishes the session and re-checks disposal under one lock +/// (_sessionLock), mirroring DebugView's P6-6 fix, so exactly one side +/// (this method or Dispose) abandons it. +/// +public class TransportImportSessionDisposeRaceTests : BunitContext +{ + /// + /// A real -backed whose + /// can be held open on demand, so the dispose-during-load + /// window is deterministic rather than timing-dependent. Mirrors + /// DebugViewConnectDisposeRaceTests.GatedDebugStreamService. + /// + private sealed class GatedImporter : IBundleImporter + { + private readonly TaskCompletionSource _gate = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _entered = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly List _abandoned = new(); + + public BundleSessionStore Store { get; } = new( + Options.Create(new TransportOptions()), TimeProvider.System); + + /// Completes once has been entered. + public Task Entered => _entered.Task; + + /// Session ids passed to , in call order. + public IReadOnlyList Abandoned + { + get { lock (_abandoned) { return _abandoned.ToList(); } } + } + + public void ReleaseLoad() => _gate.TrySetResult(); + + public async Task LoadAsync( + Stream bundleStream, string? passphrase, CancellationToken ct = default, string? clientKey = null) + { + _entered.TrySetResult(); + await _gate.Task; + + var session = new BundleSession + { + SessionId = Guid.NewGuid(), + Manifest = new BundleManifest( + BundleFormatVersion: 1, + SchemaVersion: "1.0", + CreatedAtUtc: DateTimeOffset.UtcNow, + SourceEnvironment: "prod-cluster", + ExportedBy: "bob", + ScadaBridgeVersion: "1.0.0", + ContentHash: "sha256:0000", + Encryption: null, + Summary: new BundleSummary(0, 0, 0, 0, 0, 0, 0, 0), + Contents: Array.Empty()), + // Non-zero bytes so a zeroing assertion (if ever added) is meaningful. + DecryptedContent = new byte[] { 0x11, 0x22, 0x33, 0x44 }, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(30), + }; + return Store.Open(session); + } + + public Task PreviewAsync(Guid sessionId, CancellationToken ct = default) + => throw new NotSupportedException("Not exercised by these tests."); + + public Task ApplyAsync( + Guid sessionId, + IReadOnlyList resolutions, + string user, + CancellationToken ct = default, + BundleNameMap? nameMap = null) + => throw new NotSupportedException("Not exercised by these tests."); + + public void Abandon(Guid sessionId) + { + lock (_abandoned) { _abandoned.Add(sessionId); } + var session = Store.Get(sessionId); + if (session is { DecryptedContent.Length: > 0 }) + { + Array.Clear(session.DecryptedContent, 0, session.DecryptedContent.Length); + } + Store.Remove(sessionId); + } + } + + private readonly GatedImporter _importer = new(); + private readonly ISiteRepository _siteRepo = Substitute.For(); + private const string TestClientIp = "203.0.113.7"; + + public TransportImportSessionDisposeRaceTests() + { + JSInterop.Mode = JSRuntimeMode.Loose; + + Services.AddSingleton(_importer); + Services.AddSingleton(Substitute.For()); + Services.AddSingleton(new CircuitClientAddressProvider(TestClientIp)); + + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(Array.Empty()); + Services.AddSingleton(_siteRepo); + Services.AddSingleton>( + Options.Create(new TransportOptions + { + MaxBundleSizeMb = 10, + MaxUnlockAttemptsPerSession = 3, + })); + + var dbOptions = new DbContextOptionsBuilder() + .UseSqlite("DataSource=:memory:") + .ConfigureWarnings(w => w.Ignore(RelationalEventId.AmbientTransactionWarning)) + .Options; + var dbContext = new ScadaBridgeDbContext(dbOptions); + dbContext.Database.OpenConnection(); + dbContext.Database.EnsureCreated(); + Services.AddSingleton(dbContext); + + var claims = new List { new(JwtTokenService.UsernameClaimType, "alice") }; + var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "TestAuth")); + Services.AddSingleton(new TestAuthStateProvider(principal)); + Services.AddAuthorizationCore(); + } + + private static void SetField(object obj, string name, object? value) => + obj.GetType().GetField(name, BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public)! + .SetValue(obj, value); + + private static T GetField(object obj, string name) => + (T)obj.GetType().GetField(name, BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public)! + .GetValue(obj)!; + + /// Stages a temp bundle file and points the wizard's _bundleTempPath at it. + private static string StageBundleFile() + { + var dir = Path.Combine(Path.GetTempPath(), "scadabridge-transport-staging"); + Directory.CreateDirectory(dir); + var path = Path.Combine(dir, $"test-{Guid.NewGuid():N}.scadabundle"); + File.WriteAllBytes(path, new byte[] { 0x01, 0x02, 0x03 }); + return path; + } + + /// + /// Starts TryLoadAsync on the renderer's dispatcher (where it really runs) and + /// hands back its task without awaiting it, so the test can act inside the load window. + /// Mirrors DebugViewConnectDisposeRaceTests.StartConnectAsync. + /// + private static async Task StartTryLoadAsync(IRenderedComponent cut) + { + var method = typeof(TransportImportPage).GetMethod( + "TryLoadAsync", BindingFlags.Instance | BindingFlags.NonPublic)!; + Task? task = null; + await cut.InvokeAsync(() => { task = (Task)method.Invoke(cut.Instance, new object?[] { null })!; }); + return task!; + } + + [Fact] + public async Task TryLoadAsync_DisposedMidLoad_AbandonsTheJustOpenedSession() + { + var cut = Render(); + SetField(cut.Instance, "_bundleTempPath", StageBundleFile()); + + var tryLoad = await StartTryLoadAsync(cut); + await _importer.Entered.WaitAsync(TimeSpan.FromSeconds(5)); + + // The user navigates away while LoadAsync is still in flight. Dispose runs off + // the renderer's thread, exactly as a circuit teardown can. + cut.Instance.Dispose(); + + _importer.ReleaseLoad(); + await tryLoad.WaitAsync(TimeSpan.FromSeconds(10)); + + Assert.Single(_importer.Abandoned); + Assert.Null(GetField(cut.Instance, "_session")); + // The store must not still hold the session either — Abandon actually released it. + Assert.Null(_importer.Store.Get(_importer.Abandoned[0])); + } + + [Fact] + public async Task TryLoadAsync_ThenDispose_AbandonsTheSessionExactlyOnce() + { + var cut = Render(); + SetField(cut.Instance, "_bundleTempPath", StageBundleFile()); + + // Normal path: the load completes while the component is very much alive. + _importer.ReleaseLoad(); + var tryLoad = await StartTryLoadAsync(cut); + await tryLoad.WaitAsync(TimeSpan.FromSeconds(10)); + + Assert.Empty(_importer.Abandoned); + Assert.NotNull(GetField(cut.Instance, "_session")); + + cut.Instance.Dispose(); + Assert.Single(_importer.Abandoned); + + // A second Dispose (bUnit tears the component down again at test end) must not + // abandon an already-released session a second time. + cut.Instance.Dispose(); + Assert.Single(_importer.Abandoned); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyLiveUpdatesTimerDisposeRaceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyLiveUpdatesTimerDisposeRaceTests.cs new file mode 100644 index 00000000..57d871f7 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyLiveUpdatesTimerDisposeRaceTests.cs @@ -0,0 +1,194 @@ +using System.Reflection; +using System.Security.Claims; +using Bunit; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Deployment; +using ZB.MOM.WW.ScadaBridge.Communication; +using ZB.MOM.WW.ScadaBridge.DeploymentManager; +using ZB.MOM.WW.ScadaBridge.Security; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Services; +using TopologyPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Deployment.Topology; + +namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests; + +/// +/// Regression tests for register row 103 (the P6-6 lesser instance in +/// Topology). OnInitializedAsync awaits LoadDataAsync and only then +/// calls StartLiveUpdatesTimer. If the circuit was torn down inside that await +/// (user navigates away during the initial page load), the pre-fix Dispose would +/// have already run and found _liveUpdatesTimer still null — a no-op — so the +/// timer StartLiveUpdatesTimer creates once the await resumes is never disposed: +/// it ticks against a torn-down component every 15s until process restart. The fix +/// (mirroring DebugView's P6-6 fix and TransportImport's row-103 sibling +/// fix) creates/replaces the timer and observes disposal atomically under +/// _timerLock, so a (re)start that resumes after Dispose already ran +/// refuses to create a new timer instead of leaking one. +/// +/// +/// The race is reproduced deterministically by driving the exact call ORDER the async +/// gap allows — Dispose() then a late-arriving StartLiveUpdatesTimer() — +/// rather than by gating I/O and sleeping: StartLiveUpdatesTimer itself is +/// synchronous, so once we control ordering there is nothing left to wait on. This is +/// the "deterministic observation, never sleep-then-assert" style the shared plan +/// constraints require: the assertion is a direct field-state check, not a timer tick. +/// +/// +public class TopologyLiveUpdatesTimerDisposeRaceTests : BunitContext +{ + private readonly ITemplateEngineRepository _repo = Substitute.For(); + private readonly ISiteRepository _siteRepo = Substitute.For(); + private readonly IDeploymentManagerRepository _deployRepo = Substitute.For(); + private readonly IAuditService _audit = Substitute.For(); + + public TopologyLiveUpdatesTimerDisposeRaceTests() + { + Services.AddSingleton(_repo); + Services.AddSingleton(_siteRepo); + Services.AddSingleton(_deployRepo); + Services.AddSingleton(Substitute.For()); + Services.AddSingleton(_audit); + + var comms = new CommunicationService( + Options.Create(new CommunicationOptions()), + NullLogger.Instance); + Services.AddSingleton(comms); + + Services.AddSingleton(new OperationLockManager()); + Services.AddSingleton(Options.Create(new DeploymentManagerOptions + { + OperationLockTimeout = TimeSpan.FromSeconds(5) + })); + Services.AddSingleton(Options.Create(new CommunicationOptions())); + Services.AddSingleton>(NullLogger.Instance); + Services.AddScoped(); + Services.AddScoped(); + Services.AddSingleton( + new DeploymentStatusNotifier(NullLogger.Instance)); + Services.AddScoped(); + Services.AddScoped(); + Services.AddScoped(); + + AddTestAuth(); + + Services.AddScoped(); + Services.AddScoped(); + + JSInterop.Setup("treeviewStorage.load", _ => true).SetResult(null); + JSInterop.SetupVoid("treeviewStorage.save", _ => true); + + SeedEmptyRepos(); + } + + private void AddTestAuth() + { + var claims = new[] + { + new Claim(JwtTokenService.UsernameClaimType, "tester"), + new Claim(JwtTokenService.RoleClaimType, "Deployer") + }; + var user = new ClaimsPrincipal(new ClaimsIdentity(claims, "TestAuth")); + Services.AddSingleton(new TestAuthStateProvider(user)); + Services.AddAuthorizationCore(); + } + + private void SeedEmptyRepos() + { + _siteRepo.GetAllSitesAsync(Arg.Any()) + .Returns(Task.FromResult>(new List())); + _repo.GetAllTemplatesAsync(Arg.Any()) + .Returns(Task.FromResult>(new List