fix(transport): release abandoned BundleSessions instead of leaking a slot (row 43)
Only BundleImporter.ApplyAsync released an import session, so every path that opened one and did NOT apply held one of MaxConcurrentImportSessions (8) — and the DECRYPTED bundle content that T-007 deliberately zeroes on the apply paths — for the full BundleSessionTtlMinutes (30). Eight abandonments locked out all imports on the active node; observed live (R6: 3 back-to-back suite runs held 6 of 8 slots until central-b was restarted). New IBundleImporter.Abandon(sessionId): the same zero-then-remove release ApplyAsync performs, idempotent (double release / unknown id / expired id are no-ops) and only ever given the random-GUID id its caller opened, so it can never evict another circuit's session. Wired into every abandonment path: - Central UI wizard — ReleaseSession() from new upload, TryLoadAsync (before it overwrites _session), Back-to-Upload / ResetSessionState, the 3-strike lockout reset, and Dispose (navigation away / circuit teardown). - CLI transport preview — a preview never applies, so nothing released it at all; now released in a finally. - CLI transport import — covers the pre-apply aborts (blocker rows, unresolved site/connection mappings). Class doc's "an abandoned wizard does not leak" claim is true again (it only ever covered the staged plaintext temp file). Component-Transport.md + docs/components/Transport.md now record the explicit-release contract and that BundleSessionEvictionService is a TTL backstop, not an abandonment path. Tests: 4 bUnit tests over a REAL BundleSessionStore (dispose / back-to-upload / apply-then-dispose idempotency / concurrent-circuit isolation) — 3 of the 4 fail against the pre-fix page; 4 importer tests (release + zeroing, idempotency, cross-session isolation, and the slot-cap regression).
This commit is contained in:
Joseph Doherty
@@ -17,6 +17,7 @@ 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 TransportImportPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Design.TransportImport;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Pages.Design;
|
||||
@@ -688,6 +689,191 @@ public class TransportImportPageTests : BunitContext
|
||||
Assert.Empty(connChoices);
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
// Register row 43: an abandoned wizard must not strand its server-side
|
||||
// BundleSession. These tests swap the NSubstitute importer for one backed
|
||||
// by a REAL BundleSessionStore so the assertion is "the slot is actually
|
||||
// released", not "a mock was called".
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
|
||||
/// <summary>
|
||||
/// Minimal <see cref="IBundleImporter"/> over a real <see cref="BundleSessionStore"/>.
|
||||
/// <see cref="Abandon"/> and <see cref="ApplyAsync"/> mirror the production
|
||||
/// importer's zero-then-remove release; every other member is unused by these
|
||||
/// tests. Sessions are seeded directly with <see cref="OpenSession"/> because
|
||||
/// driving <c>InputFile</c> from bUnit is impractical (see the class remarks).
|
||||
/// </summary>
|
||||
private sealed class StoreBackedImporter : IBundleImporter
|
||||
{
|
||||
public BundleSessionStore Store { get; } = new(
|
||||
Microsoft.Extensions.Options.Options.Create(new TransportOptions()),
|
||||
TimeProvider.System);
|
||||
|
||||
public int AbandonCalls { get; private set; }
|
||||
|
||||
public BundleSession OpenSession(string sourceEnv = "prod-cluster")
|
||||
{
|
||||
var seed = BuildEncryptedSession(sourceEnv);
|
||||
var session = new BundleSession
|
||||
{
|
||||
SessionId = seed.SessionId,
|
||||
Manifest = seed.Manifest,
|
||||
// Non-zero bytes so the zeroing assertion is meaningful.
|
||||
DecryptedContent = new byte[] { 0x11, 0x22, 0x33, 0x44 },
|
||||
ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(30),
|
||||
};
|
||||
return Store.Open(session);
|
||||
}
|
||||
|
||||
public Task<BundleSession> LoadAsync(Stream bundleStream, string? passphrase, CancellationToken ct = default)
|
||||
=> throw new NotSupportedException("These tests seed sessions via OpenSession().");
|
||||
|
||||
public Task<ImportPreview> PreviewAsync(Guid sessionId, CancellationToken ct = default)
|
||||
=> Task.FromResult(new ImportPreview(sessionId, new List<ImportPreviewItem>()));
|
||||
|
||||
public Task<ImportResult> ApplyAsync(
|
||||
Guid sessionId,
|
||||
IReadOnlyList<ImportResolution> resolutions,
|
||||
string user,
|
||||
CancellationToken ct = default,
|
||||
BundleNameMap? nameMap = null)
|
||||
{
|
||||
Release(sessionId);
|
||||
return Task.FromResult(new ImportResult(
|
||||
BundleImportId: Guid.NewGuid(),
|
||||
Added: 1,
|
||||
Overwritten: 0,
|
||||
Skipped: 0,
|
||||
Renamed: 0,
|
||||
StaleInstanceIds: Array.Empty<int>(),
|
||||
AuditEventCorrelation: Guid.NewGuid().ToString()));
|
||||
}
|
||||
|
||||
public void Abandon(Guid sessionId)
|
||||
{
|
||||
AbandonCalls++;
|
||||
Release(sessionId);
|
||||
}
|
||||
|
||||
private void Release(Guid sessionId)
|
||||
{
|
||||
var session = Store.Get(sessionId);
|
||||
if (session is { DecryptedContent.Length: > 0 })
|
||||
{
|
||||
Array.Clear(session.DecryptedContent, 0, session.DecryptedContent.Length);
|
||||
}
|
||||
Store.Remove(sessionId);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Disposing_the_wizard_releases_its_bundle_session()
|
||||
{
|
||||
// Circuit teardown / navigation away. Before the fix, Dispose deleted only
|
||||
// the staged temp file, so the session (and its decrypted content) held one
|
||||
// of MaxConcurrentImportSessions for the full 30-minute TTL.
|
||||
var importer = new StoreBackedImporter();
|
||||
Services.AddSingleton<IBundleImporter>(importer);
|
||||
|
||||
var cut = Render<TransportImportPage>();
|
||||
var session = importer.OpenSession();
|
||||
var content = session.DecryptedContent;
|
||||
await cut.InvokeAsync(() => SetField(cut.Instance, "_session", session));
|
||||
|
||||
cut.Instance.Dispose();
|
||||
|
||||
Assert.Null(importer.Store.Get(session.SessionId));
|
||||
Assert.All(content, b => Assert.Equal(0, b));
|
||||
Assert.Null(GetField<object?>(cut.Instance, "_session"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task BackToUpload_releases_the_bundle_session()
|
||||
{
|
||||
// The wizard's only cancel affordance ("Back" on steps 2/3/5) routes through
|
||||
// BackToUpload → ResetSessionState, which used to null the local field only.
|
||||
var importer = new StoreBackedImporter();
|
||||
Services.AddSingleton<IBundleImporter>(importer);
|
||||
|
||||
var cut = Render<TransportImportPage>();
|
||||
var session = importer.OpenSession();
|
||||
var content = session.DecryptedContent;
|
||||
await cut.InvokeAsync(() =>
|
||||
{
|
||||
SetField(cut.Instance, "_session", session);
|
||||
SetField(cut.Instance, "_step", TransportImportPage.ImportWizardStep.Diff);
|
||||
});
|
||||
|
||||
await cut.InvokeAsync(() =>
|
||||
{
|
||||
var method = cut.Instance.GetType().GetMethod(
|
||||
"BackToUpload",
|
||||
System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)!;
|
||||
method.Invoke(cut.Instance, Array.Empty<object?>());
|
||||
});
|
||||
|
||||
Assert.Null(importer.Store.Get(session.SessionId));
|
||||
Assert.All(content, b => Assert.Equal(0, b));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Apply_then_dispose_releases_the_session_once_without_throwing()
|
||||
{
|
||||
// ApplyAsync already removes the session; the wizard's own release must be
|
||||
// idempotent so circuit teardown after a completed import is a silent no-op.
|
||||
var importer = new StoreBackedImporter();
|
||||
Services.AddSingleton<IBundleImporter>(importer);
|
||||
|
||||
var cut = Render<TransportImportPage>();
|
||||
var session = importer.OpenSession(sourceEnv: "prod-cluster");
|
||||
await cut.InvokeAsync(() =>
|
||||
{
|
||||
SetField(cut.Instance, "_session", session);
|
||||
SetField(cut.Instance, "_resolutions",
|
||||
new Dictionary<(string EntityType, string Name), ImportResolution>
|
||||
{
|
||||
[("Template", "Pump")] = new("Template", "Pump", ResolutionAction.Add, null),
|
||||
});
|
||||
SetField(cut.Instance, "_step", TransportImportPage.ImportWizardStep.Confirm);
|
||||
SetField(cut.Instance, "_confirmEnvironmentText", "prod-cluster");
|
||||
});
|
||||
|
||||
await cut.InvokeAsync(() => InvokeAsyncMethod(cut.Instance, "ApplyAsync"));
|
||||
Assert.Equal(
|
||||
TransportImportPage.ImportWizardStep.Result,
|
||||
GetField<TransportImportPage.ImportWizardStep>(cut.Instance, "_step"));
|
||||
Assert.Null(importer.Store.Get(session.SessionId));
|
||||
|
||||
// Must not throw, and must not resurrect or double-release anything.
|
||||
cut.Instance.Dispose();
|
||||
cut.Instance.Dispose();
|
||||
|
||||
Assert.Null(importer.Store.Get(session.SessionId));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Disposing_one_wizard_leaves_a_concurrent_circuits_session_open()
|
||||
{
|
||||
// Sessions are keyed by a GUID held only by the circuit that opened them,
|
||||
// so one operator abandoning their import must not evict another's.
|
||||
var importer = new StoreBackedImporter();
|
||||
Services.AddSingleton<IBundleImporter>(importer);
|
||||
|
||||
var first = Render<TransportImportPage>();
|
||||
var second = Render<TransportImportPage>();
|
||||
var mine = importer.OpenSession();
|
||||
var theirs = importer.OpenSession(sourceEnv: "other-cluster");
|
||||
await first.InvokeAsync(() => SetField(first.Instance, "_session", mine));
|
||||
await second.InvokeAsync(() => SetField(second.Instance, "_session", theirs));
|
||||
|
||||
first.Instance.Dispose();
|
||||
|
||||
Assert.Null(importer.Store.Get(mine.SessionId));
|
||||
var survivor = importer.Store.Get(theirs.SessionId);
|
||||
Assert.NotNull(survivor);
|
||||
Assert.Contains(survivor!.DecryptedContent, b => b != 0);
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
// Reflection helpers — the wizard's per-instance state is private (the
|
||||
// razor partial pattern). We poke at it via reflection rather than
|
||||
|
||||
@@ -502,6 +502,92 @@ public sealed class BundleImporterLoadTests
|
||||
() => rig.Importer.LoadAsync(stream, passphrase: null));
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
// Register row 43 — abandoned-session release.
|
||||
// ─────────────────────────────────────────────────────────────────────
|
||||
|
||||
[Fact]
|
||||
public async Task Abandon_releases_the_session_slot_and_zeroes_its_decrypted_content()
|
||||
{
|
||||
// Row 43: only ApplyAsync used to release a session, so any caller that
|
||||
// loaded a bundle and walked away held one of MaxConcurrentImportSessions
|
||||
// (plus the decrypted plaintext) for the full TTL.
|
||||
var rig = BuildRig();
|
||||
using var packed = PackEncryptedBundle(
|
||||
rig.Serializer, rig.ManifestBuilder, rig.Encryptor, SmallContent(), "correct");
|
||||
var session = await rig.Importer.LoadAsync(
|
||||
new MemoryStream(((MemoryStream)packed).ToArray()), passphrase: "correct");
|
||||
|
||||
var decrypted = session.DecryptedContent;
|
||||
Assert.NotEmpty(decrypted);
|
||||
Assert.Contains(decrypted, b => b != 0);
|
||||
Assert.NotNull(rig.SessionStore.Get(session.SessionId));
|
||||
|
||||
rig.Importer.Abandon(session.SessionId);
|
||||
|
||||
Assert.Null(rig.SessionStore.Get(session.SessionId));
|
||||
// Zeroed in place, so even a caller still holding the reference (the Razor
|
||||
// page keeps one for the Result step) can no longer read the plaintext.
|
||||
Assert.All(decrypted, b => Assert.Equal(0, b));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Abandon_is_idempotent_and_tolerates_unknown_ids()
|
||||
{
|
||||
// The Razor page calls Abandon from Dispose even after a successful Apply
|
||||
// already released the same id; a double release must be a silent no-op.
|
||||
var rig = BuildRig();
|
||||
using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent());
|
||||
var session = await rig.Importer.LoadAsync(packed, passphrase: null);
|
||||
|
||||
rig.Importer.Abandon(session.SessionId);
|
||||
rig.Importer.Abandon(session.SessionId);
|
||||
rig.Importer.Abandon(Guid.NewGuid());
|
||||
|
||||
Assert.Null(rig.SessionStore.Get(session.SessionId));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Abandon_does_not_release_another_callers_session()
|
||||
{
|
||||
// Sessions are keyed by a random GUID handed only to their opener, so one
|
||||
// operator abandoning their wizard must not evict a concurrent import.
|
||||
var rig = BuildRig();
|
||||
using var packedA = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent());
|
||||
using var packedB = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, EmptyContent());
|
||||
var a = await rig.Importer.LoadAsync(packedA, passphrase: null);
|
||||
var b = await rig.Importer.LoadAsync(packedB, passphrase: null);
|
||||
|
||||
rig.Importer.Abandon(a.SessionId);
|
||||
|
||||
Assert.Null(rig.SessionStore.Get(a.SessionId));
|
||||
var survivor = rig.SessionStore.Get(b.SessionId);
|
||||
Assert.NotNull(survivor);
|
||||
Assert.NotEmpty(survivor!.DecryptedContent);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Abandon_frees_a_concurrent_import_session_slot()
|
||||
{
|
||||
// The availability half of row 43: abandoned sessions used to exhaust the
|
||||
// cap and lock out every import on the node until the TTL expired.
|
||||
var rig = BuildRig(opts => opts.MaxConcurrentImportSessions = 2);
|
||||
using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent());
|
||||
var bundleBytes = ((MemoryStream)packed).ToArray();
|
||||
|
||||
var first = await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null);
|
||||
await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null);
|
||||
|
||||
var full = await Assert.ThrowsAsync<InvalidOperationException>(
|
||||
() => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null));
|
||||
Assert.Contains("Too many concurrent import sessions", full.Message);
|
||||
|
||||
rig.Importer.Abandon(first.SessionId);
|
||||
|
||||
var third = await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null);
|
||||
Assert.NotNull(rig.SessionStore.Get(third.SessionId));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Builds a zip directly so the test can write a manifest whose
|
||||
/// <c>ContentHash</c> or <c>BundleFormatVersion</c> intentionally
|
||||
|
||||
Reference in New Issue
Block a user