fix(centralui): close register row 103 — take-once session/timer discipline for TransportImport + Topology (P6-6 lesser instances)

Mirrors the Medium wave's DebugView P6-6 fix (_sessionLock + _disposed,
publish-then-recheck-disposal under one lock) for the two TTL-bounded/
unbounded lesser instances the Medium wave deferred:

- TransportImport.razor.cs: TryLoadAsync awaits IBundleImporter.LoadAsync and
  used to publish the result into _session unconditionally. A circuit torn
  down mid-await left Dispose() with nothing to release (session still null),
  so the freshly-opened server-side BundleSession stayed registered, stranding
  one of MaxConcurrentImportSessions (register row 43) for the full session
  TTL. Now publishes and re-checks disposal atomically under _sessionLock, so
  exactly one side (TryLoadAsync or Dispose) abandons the session.

- Topology.razor: OnInitializedAsync awaits LoadDataAsync before calling
  StartLiveUpdatesTimer. A circuit torn down mid-await left Dispose() with no
  timer to stop yet, so the timer StartLiveUpdatesTimer creates once the await
  resumes was never disposed — it ticked against a torn-down component every
  15s until process restart. StartLiveUpdatesTimer/StopLiveUpdatesTimer/
  Dispose now create/replace/take the timer atomically under _timerLock
  together with a _disposed check, so a late-arriving (re)start refuses to
  create a timer once disposed.

Tests (bUnit, both proven to fail against the unfixed code):
- TransportImportSessionDisposeRaceTests: dispose-during-load abandons the
  just-opened session immediately; dispose-after-successful-load abandons
  exactly once.
- TopologyLiveUpdatesTimerDisposeRaceTests: a late StartLiveUpdatesTimer call
  after Dispose leaves no live timer; Dispose stops the timer exactly once
  (idempotent double-dispose); OnLiveUpdatesToggled(off) + Dispose leaves no
  live timer. All assert on deterministic field/call-order state, no sleeps.

Register row 103 stays open for integration to close per the plan's shared
constraints (register edits happen at integration only).
This commit is contained in:
Joseph Doherty
2026-08-19 16:22:00 -04:00
parent f1ef95f4da
commit 6d0d8b6258
4 changed files with 552 additions and 14 deletions
@@ -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;
/// <summary>
/// Regression tests for register row 103 (the P6-6 lesser instance in
/// <c>TransportImport</c>). <c>TryLoadAsync</c> awaits
/// <see cref="IBundleImporter.LoadAsync"/>, then used to publish the result into
/// <c>_session</c> unconditionally. If the circuit was torn down inside that await
/// (user navigates away mid-upload), <c>Dispose</c> would have already run, found
/// <c>_session</c> still null, and released nothing — so the freshly-opened server-side
/// <c>BundleSession</c> stayed registered, stranding one of
/// <c>TransportOptions.MaxConcurrentImportSessions</c> (register row 43) for the full
/// session TTL. The fix publishes the session and re-checks disposal under one lock
/// (<c>_sessionLock</c>), mirroring <c>DebugView</c>'s P6-6 fix, so exactly one side
/// (this method or <c>Dispose</c>) abandons it.
/// </summary>
public class TransportImportSessionDisposeRaceTests : BunitContext
{
/// <summary>
/// A real <see cref="BundleSessionStore"/>-backed <see cref="IBundleImporter"/> whose
/// <see cref="LoadAsync"/> can be held open on demand, so the dispose-during-load
/// window is deterministic rather than timing-dependent. Mirrors
/// <c>DebugViewConnectDisposeRaceTests.GatedDebugStreamService</c>.
/// </summary>
private sealed class GatedImporter : IBundleImporter
{
private readonly TaskCompletionSource _gate = new(TaskCreationOptions.RunContinuationsAsynchronously);
private readonly TaskCompletionSource _entered = new(TaskCreationOptions.RunContinuationsAsynchronously);
private readonly List<Guid> _abandoned = new();
public BundleSessionStore Store { get; } = new(
Options.Create(new TransportOptions()), TimeProvider.System);
/// <summary>Completes once <see cref="LoadAsync"/> has been entered.</summary>
public Task Entered => _entered.Task;
/// <summary>Session ids passed to <see cref="Abandon"/>, in call order.</summary>
public IReadOnlyList<Guid> Abandoned
{
get { lock (_abandoned) { return _abandoned.ToList(); } }
}
public void ReleaseLoad() => _gate.TrySetResult();
public async Task<BundleSession> 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<ManifestContentEntry>()),
// 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<ImportPreview> PreviewAsync(Guid sessionId, CancellationToken ct = default)
=> throw new NotSupportedException("Not exercised by these tests.");
public Task<ImportResult> ApplyAsync(
Guid sessionId,
IReadOnlyList<ImportResolution> 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<ISiteRepository>();
private const string TestClientIp = "203.0.113.7";
public TransportImportSessionDisposeRaceTests()
{
JSInterop.Mode = JSRuntimeMode.Loose;
Services.AddSingleton<IBundleImporter>(_importer);
Services.AddSingleton(Substitute.For<IAuditService>());
Services.AddSingleton(new CircuitClientAddressProvider(TestClientIp));
_siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(Array.Empty<Site>());
Services.AddSingleton(_siteRepo);
Services.AddSingleton<IOptions<TransportOptions>>(
Options.Create(new TransportOptions
{
MaxBundleSizeMb = 10,
MaxUnlockAttemptsPerSession = 3,
}));
var dbOptions = new DbContextOptionsBuilder<ScadaBridgeDbContext>()
.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<Claim> { new(JwtTokenService.UsernameClaimType, "alice") };
var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "TestAuth"));
Services.AddSingleton<AuthenticationStateProvider>(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<T>(object obj, string name) =>
(T)obj.GetType().GetField(name, BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public)!
.GetValue(obj)!;
/// <summary>Stages a temp bundle file and points the wizard's <c>_bundleTempPath</c> at it.</summary>
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;
}
/// <summary>
/// Starts <c>TryLoadAsync</c> 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 <c>DebugViewConnectDisposeRaceTests.StartConnectAsync</c>.
/// </summary>
private static async Task<Task> StartTryLoadAsync(IRenderedComponent<TransportImportPage> 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<TransportImportPage>();
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<object?>(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<TransportImportPage>();
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<object?>(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);
}
}
@@ -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;
/// <summary>
/// Regression tests for register row 103 (the P6-6 lesser instance in
/// <c>Topology</c>). <c>OnInitializedAsync</c> awaits <c>LoadDataAsync</c> and only then
/// calls <c>StartLiveUpdatesTimer</c>. If the circuit was torn down inside that await
/// (user navigates away during the initial page load), the pre-fix <c>Dispose</c> would
/// have already run and found <c>_liveUpdatesTimer</c> still null — a no-op — so the
/// timer <c>StartLiveUpdatesTimer</c> creates once the await resumes is never disposed:
/// it ticks against a torn-down component every 15s until process restart. The fix
/// (mirroring <c>DebugView</c>'s P6-6 fix and <c>TransportImport</c>'s row-103 sibling
/// fix) creates/replaces the timer and observes disposal atomically under
/// <c>_timerLock</c>, so a (re)start that resumes after <c>Dispose</c> already ran
/// refuses to create a new timer instead of leaking one.
///
/// <para>
/// The race is reproduced deterministically by driving the exact call ORDER the async
/// gap allows — <c>Dispose()</c> then a late-arriving <c>StartLiveUpdatesTimer()</c> —
/// rather than by gating I/O and sleeping: <c>StartLiveUpdatesTimer</c> 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.
/// </para>
/// </summary>
public class TopologyLiveUpdatesTimerDisposeRaceTests : BunitContext
{
private readonly ITemplateEngineRepository _repo = Substitute.For<ITemplateEngineRepository>();
private readonly ISiteRepository _siteRepo = Substitute.For<ISiteRepository>();
private readonly IDeploymentManagerRepository _deployRepo = Substitute.For<IDeploymentManagerRepository>();
private readonly IAuditService _audit = Substitute.For<IAuditService>();
public TopologyLiveUpdatesTimerDisposeRaceTests()
{
Services.AddSingleton(_repo);
Services.AddSingleton(_siteRepo);
Services.AddSingleton(_deployRepo);
Services.AddSingleton(Substitute.For<ZB.MOM.WW.ScadaBridge.DeploymentManager.IFlatteningPipeline>());
Services.AddSingleton(_audit);
var comms = new CommunicationService(
Options.Create(new CommunicationOptions()),
NullLogger<CommunicationService>.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<ILogger<DeploymentService>>(NullLogger<DeploymentService>.Instance);
Services.AddScoped<ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening.DiffService>();
Services.AddScoped<ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening.RevisionHashService>();
Services.AddSingleton<IDeploymentStatusNotifier>(
new DeploymentStatusNotifier(NullLogger<DeploymentStatusNotifier>.Instance));
Services.AddScoped<DeploymentService>();
Services.AddScoped<AreaService>();
Services.AddScoped<InstanceService>();
AddTestAuth();
Services.AddScoped<ZB.MOM.WW.ScadaBridge.CentralUI.Components.Shared.IDialogService,
ZB.MOM.WW.ScadaBridge.CentralUI.Components.Shared.DialogService>();
Services.AddScoped<ZB.MOM.WW.ScadaBridge.CentralUI.Auth.SiteScopeService>();
JSInterop.Setup<string?>("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<AuthenticationStateProvider>(new TestAuthStateProvider(user));
Services.AddAuthorizationCore();
}
private void SeedEmptyRepos()
{
_siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyList<Site>>(new List<Site>()));
_repo.GetAllTemplatesAsync(Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyList<Template>>(new List<Template>()));
_repo.GetAllInstancesAsync(Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyList<Instance>>(new List<Instance>()));
_repo.GetAreasBySiteIdAsync(Arg.Any<int>(), Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyList<Area>>(new List<Area>()));
}
private static void InvokePrivate(object obj, string name) =>
obj.GetType().GetMethod(name, BindingFlags.Instance | BindingFlags.NonPublic)!
.Invoke(obj, null);
private static T? GetField<T>(object obj, string name) =>
(T?)obj.GetType().GetField(name, BindingFlags.Instance | BindingFlags.NonPublic)!.GetValue(obj);
[Fact]
public void Render_StartsALiveTimer_Sanity()
{
// Baseline: the normal init path (fast/empty repos complete synchronously) leaves
// a running timer, so the race tests below start from a known non-null state.
var cut = Render<TopologyPage>();
Assert.NotNull(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
cut.Instance.Dispose();
}
[Fact]
public void StartLiveUpdatesTimer_ResumingAfterDispose_LeavesNoLiveTimer()
{
// Simulates: OnInitializedAsync is suspended awaiting LoadDataAsync; the user
// navigates away, so Dispose() runs (and, since no timer exists yet, has nothing
// to stop); the await then resumes and calls StartLiveUpdatesTimer() — the exact
// shape register row 103 flags for Topology (the P6-6 lesser instance).
var cut = Render<TopologyPage>();
// The normal init path already started a timer; dispose it first so the field is
// null going into the race exactly as it would be if OnInitializedAsync's await
// was still pending when Dispose ran (no timer created yet).
cut.Instance.Dispose();
Assert.Null(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
// The late-arriving continuation's call, happening strictly AFTER Dispose().
InvokePrivate(cut.Instance, "StartLiveUpdatesTimer");
// Post-fix: refuses to create a timer once disposed — nothing is leaked.
// Pre-fix: this assignment is unconditional, so the field would be non-null here
// (a live Timer nothing will ever dispose).
Assert.Null(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
}
[Fact]
public void Dispose_StopsTheLiveTimer_ExactlyOnce()
{
var cut = Render<TopologyPage>();
Assert.NotNull(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
cut.Instance.Dispose();
Assert.Null(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
// A second Dispose (bUnit tears the component down again at test end) must not
// throw (e.g. from double-disposing the same Timer) and must leave the slot null.
var ex = Record.Exception(() => cut.Instance.Dispose());
Assert.Null(ex);
Assert.Null(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
}
[Fact]
public void OnLiveUpdatesToggled_Off_ThenDispose_LeavesNoLiveTimer()
{
var cut = Render<TopologyPage>();
Assert.NotNull(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
// Turn live updates off via the toolbar handler (StopLiveUpdatesTimer path).
var toggleMethod = typeof(TopologyPage).GetMethod(
"OnLiveUpdatesToggled", BindingFlags.Instance | BindingFlags.NonPublic)!;
var changeArgsType = typeof(Microsoft.AspNetCore.Components.ChangeEventArgs);
var changeArgs = (Microsoft.AspNetCore.Components.ChangeEventArgs)Activator.CreateInstance(changeArgsType)!;
changeArgs.Value = false;
toggleMethod.Invoke(cut.Instance, new object?[] { changeArgs });
Assert.Null(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
var ex = Record.Exception(() => cut.Instance.Dispose());
Assert.Null(ex);
Assert.Null(GetField<Timer?>(cut.Instance, "_liveUpdatesTimer"));
}
}