fix(transport)+test: close deferred-work register rows 58, 60, 65 (Low-tail sweep)

Row 65 — trigger FIRED, fixed. BundleUnlockAttempt rows were pruned only by nodes actively serving unlocks, so a cluster that stopped importing entirely left its last rows in place forever. The row's own trigger was "Transport gains a central scheduler a prune tick could ride" — it has one now: BundleSessionEvictionService, the central-only 1-minute session sweep added by the transport-session fixes. It now also ticks the new IBundleUnlockAttemptWindow.PruneAsync. Both drivers share the ONE 15-minute CAS cadence claim, so ticking on a 1-minute sweep costs at most one bounded DELETE per quarter hour and the two can never prune concurrently. PruneAsync is contractually non-throwing (unlike TryRegisterAttemptAsync, which throws and lets the limiter own the fail-open decision) — a background sweep has no caller to report to and must survive a database blip. Both Transport docs updated; three tests in FleetWideUnlockThrottleTests, incl. the retention sweep running with no unlock traffic anywhere in the test, which is the whole point. Row 58 — closed with a pin, and the OLD pin was found blind. The existing Deployments_Dispose_UnsubscribesFromNotifier is behavioural: it is satisfied by the _disposed guard alone, so deleting the `-=` from Dispose leaves it green while the singleton notifier keeps a delegate pinning the circuit-scoped component. Verified by mutation — with the `-=` removed the old test still passed and only the two new pins failed. Added: * a structural retention pin asserting the notifier's invocation list actually shrinks to 0 on Dispose; * a source scan pinning the row's stated TRIGGER ("a second page subscribes, or any subscriber lands without a matching Dispose unsubscribe") — every CentralUI file that subscribes must also unsubscribe and be IDisposable, with a non-empty guard so the scan cannot pass vacuously. Row 60 — closed with a pin, and the register's premise corrected. GlobalArtifactSnapshot does NOT hold EF-materialised entities: it is projected into pure Commons DTOs before the fan-out. The EF state that genuinely crosses into the parallel send is the tracked Site entities the lambda closes over, and that is safe only while lazy loading is off. Pinned in ConfigurationDatabase .Tests two ways — no proxies extension on the built DbContextOptions, and no EntityFrameworkCore.Proxies package reference anywhere in the repo (the structurally stronger half: UseLazyLoadingProxies() is not a compilable call without it). The real invariant is now spelled out at the fan-out itself. Row 59 — no product change; added the missing tripwire remark stating the constraint the safety argument rests on (serial by design, one scoped DbContext, do not Task.WhenAll; if it gets slow the fix is a scope PER SITE, not a shared scope under fan-out). Line numbers in the register have drifted to :528-545. Rows 36, 37, 64: verification only, no change — see the sweep report.
2026-08-17 03:42:31 -04:00
parent 07220bc655
commit 2807b5a3ea
11 changed files with 391 additions and 11 deletions
@@ -112,6 +112,118 @@ public class DeploymentsPushUpdateTests : BunitContext
            Arg.Any<DeploymentListFilter>(), Arg.Any<int>(), Arg.Any<int>(), Arg.Any<CancellationToken>());
    }

+    /// <summary>
+    /// Deferred-work register row 58: the notifier is a process SINGLETON that retains
+    /// delegates targeting circuit-scoped components, so an un-detached handler leaks the
+    /// whole component graph (and its circuit) for the process lifetime.
+    /// <para>
+    /// The sibling <see cref="Deployments_Dispose_UnsubscribesFromNotifier"/> pins the
+    /// observable behaviour, but it is satisfied by the <c>_disposed</c> guard ALONE —
+    /// delete the <c>-=</c> from <c>Dispose</c> and it still passes while the leak is
+    /// wide open. This test pins the retention property structurally instead: the
+    /// notifier's invocation list must actually shrink back to empty.
+    /// </para>
+    /// </summary>
+    [Fact]
+    public void Deployments_Dispose_ActuallyDetachesTheDelegate_NotJustGuardsIt()
+    {
+        RegisterServices();
+        var cut = Render<DeploymentsPage>();
+
+        Assert.Equal(1, SubscriberCount(_notifier));
+
+        cut.Instance.Dispose();
+
+        Assert.Equal(0, SubscriberCount(_notifier));
+    }
+
+    /// <summary>
+    /// Reads the compiler-generated backing field of the field-like
+    /// <c>StatusChanged</c> event and counts its invocation list. Reflection is the only
+    /// way to observe retention — the notifier deliberately exposes no subscriber count,
+    /// and a behavioural probe cannot distinguish "detached" from "attached but guarded".
+    /// </summary>
+    private static int SubscriberCount(DeploymentStatusNotifier notifier)
+    {
+        var field = typeof(DeploymentStatusNotifier).GetField(
+            "StatusChanged", BindingFlags.Instance | BindingFlags.NonPublic);
+        Assert.NotNull(field);
+
+        var handlers = (Delegate?)field!.GetValue(notifier);
+        return handlers?.GetInvocationList().Length ?? 0;
+    }
+
+    /// <summary>
+    /// Deferred-work register row 58's TRIGGER, pinned: "a second page subscribes, or any
+    /// subscriber lands without a matching <c>Dispose</c> unsubscribe". Row 58 is
+    /// acceptable only while every subscriber detaches itself; a new page that subscribes
+    /// and forgets is a silent circuit leak that no existing test would catch, because
+    /// every other pin is written against the one known subscriber.
+    /// <para>
+    /// A source scan rather than reflection: the subscription is a <c>+=</c> operator
+    /// inside a method body, which is not visible on the type's reflected surface.
+    /// Mirrors the source-scan convention in <c>Commons.Tests/ArchitecturalConstraintTests</c>.
+    /// </para>
+    /// </summary>
+    [Fact]
+    public void EveryCentralUiSubscriberToTheNotifier_UnsubscribesAndIsDisposable()
+    {
+        var sourceDir = GetCentralUiSourceDirectory();
+        var sourceFiles = Directory
+            .GetFiles(sourceDir, "*.razor", SearchOption.AllDirectories)
+            .Concat(Directory.GetFiles(sourceDir, "*.cs", SearchOption.AllDirectories))
+            .Where(f => !f.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}",
+                            StringComparison.Ordinal)
+                        && !f.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}",
+                            StringComparison.Ordinal));
+
+        var subscribers = new List<string>();
+        foreach (var file in sourceFiles)
+        {
+            var content = File.ReadAllText(file);
+            if (!content.Contains("StatusChanged +=", StringComparison.Ordinal))
+            {
+                continue;
+            }
+
+            subscribers.Add(file);
+
+            Assert.True(
+                content.Contains("StatusChanged -=", StringComparison.Ordinal),
+                $"{Path.GetFileName(file)} subscribes to IDeploymentStatusNotifier.StatusChanged but never "
+                + "unsubscribes. The notifier is a singleton, so the delegate pins this circuit-scoped "
+                + "component for the process lifetime (deferred-work register row 58).");
+
+            Assert.True(
+                content.Contains("IDisposable", StringComparison.Ordinal)
+                || content.Contains("IAsyncDisposable", StringComparison.Ordinal),
+                $"{Path.GetFileName(file)} subscribes to IDeploymentStatusNotifier.StatusChanged but does not "
+                + "implement IDisposable/IAsyncDisposable, so its unsubscribe can never run.");
+        }
+
+        // Guard the guard: if the scan silently stops matching (file moved, API renamed)
+        // the loop above would vacuously pass and the trigger would go unwatched.
+        Assert.NotEmpty(subscribers);
+    }
+
+    /// <summary>Walks up from the test output directory to the CentralUI project source root.</summary>
+    private static string GetCentralUiSourceDirectory()
+    {
+        var dir = new DirectoryInfo(AppContext.BaseDirectory);
+        while (dir != null)
+        {
+            var candidate = Path.Combine(dir.FullName, "src", "ZB.MOM.WW.ScadaBridge.CentralUI");
+            if (Directory.Exists(candidate))
+            {
+                return candidate;
+            }
+
+            dir = dir.Parent;
+        }
+
+        throw new InvalidOperationException("Could not locate the ZB.MOM.WW.ScadaBridge.CentralUI source directory.");
+    }
+
    /// <summary>
    /// Regression test for CentralUI-022. The notifier is a process singleton:
    /// it can read its subscriber list and begin invoking
@@ -48,6 +48,77 @@ public class ServiceCollectionExtensionsTests
 #pragma warning restore EF1001
    }

+    /// <summary>
+    /// Deferred-work register row 60: <c>ArtifactDeploymentService</c> builds its per-site
+    /// deploy commands serially ("DbContext is not thread-safe") and then fans the SEND out
+    /// with <c>Task.WhenAll</c>, with EF-materialised <c>Site</c> entities captured by the
+    /// parallel lambda. That is safe for exactly one reason: with lazy loading off, reading
+    /// a property on a materialised entity is a plain field read that never re-enters the
+    /// DbContext. Turn lazy-loading proxies on and every one of those reads becomes a
+    /// database round-trip on the shared context from N threads at once — the row's named
+    /// trigger, and a failure mode that surfaces as sporadic "A second operation was
+    /// started on this context instance" rather than a clean break.
+    /// <para>
+    /// Pinned here rather than at the call site because the hazard is a DI/configuration
+    /// change made somewhere else entirely, by someone who will never read
+    /// <c>ArtifactDeploymentService</c>.
+    /// </para>
+    /// </summary>
+    [Fact]
+    public void AddConfigurationDatabase_DoesNotEnableLazyLoading()
+    {
+        var services = new ServiceCollection();
+        services.AddConfigurationDatabase("Server=unused;Database=unused;");
+        using var sp = services.BuildServiceProvider();
+
+        var options = sp.GetRequiredService<DbContextOptions<ScadaBridgeDbContext>>();
+
+        Assert.DoesNotContain(
+            options.Extensions,
+            e => e.GetType().Name.Contains("Proxies", StringComparison.Ordinal));
+
+        // Belt and braces, and the structurally stronger half: UseLazyLoadingProxies() is
+        // not even a compilable call without the Proxies package, so asserting the package
+        // is absent from the whole repo closes the hazard at the source rather than at one
+        // registration. (Mirrors Csproj_ShouldNotHaveForbiddenPackageReferences in
+        // Commons.Tests/ArchitecturalConstraintTests.)
+        var repoRoot = GetRepositoryRoot();
+        var projectFiles = Directory
+            .GetFiles(repoRoot, "*.csproj", SearchOption.AllDirectories)
+            .Concat(Directory.GetFiles(repoRoot, "Directory.Packages.props", SearchOption.AllDirectories))
+            .Where(f => !f.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}",
+                            StringComparison.Ordinal)
+                        && !f.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}",
+                            StringComparison.Ordinal))
+            .ToList();
+
+        Assert.NotEmpty(projectFiles);
+        foreach (var file in projectFiles)
+        {
+            Assert.DoesNotContain(
+                "EntityFrameworkCore.Proxies",
+                File.ReadAllText(file),
+                StringComparison.Ordinal);
+        }
+    }
+
+    /// <summary>Walks up from the test output directory to the repository root.</summary>
+    private static string GetRepositoryRoot()
+    {
+        var dir = new DirectoryInfo(AppContext.BaseDirectory);
+        while (dir != null)
+        {
+            if (File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.ScadaBridge.slnx")))
+            {
+                return dir.FullName;
+            }
+
+            dir = dir.Parent;
+        }
+
+        throw new InvalidOperationException("Could not locate the repository root (ZB.MOM.WW.ScadaBridge.slnx).");
+    }
+
    // The no-arg overload is [Obsolete(error: true)], so it cannot be referenced directly
    // from source — that is the compile-time guard. Invoke it via reflection to verify the
    // runtime defence-in-depth behaviour.
@@ -252,6 +252,70 @@ public sealed class FleetWideUnlockThrottleTests : IDisposable
        Assert.Equal(2, await ctx.BundleUnlockAttempts.AsNoTracking().CountAsync());
    }

+    [Fact]
+    public async Task ScheduledPrune_ClearsAgedRows_WithNoUnlockTrafficAtAll()
+    {
+        // Register row 65: the prune used to ride the unlock path ONLY, so a cluster that
+        // stopped importing entirely left its last rows in place indefinitely. The
+        // periodic sweep now drives PruneAsync directly — no second unlock attempt is made
+        // anywhere in this test, which is the whole point.
+        var clock = new TestClock(new DateTimeOffset(2026, 6, 1, 12, 0, 0, TimeSpan.Zero));
+        var window = new SqlBundleUnlockAttemptWindow(
+            _provider.GetRequiredService<IServiceScopeFactory>(), clock);
+
+        Assert.True(await window.TryRegisterAttemptAsync(
+            "10.0.0.1", 10, BundleUnlockRateLimiter.DefaultWindow));
+
+        // Past both the 2-hour retention horizon and the 15-minute prune cadence.
+        clock.Advance(TimeSpan.FromHours(3));
+        await window.PruneAsync(BundleUnlockRateLimiter.DefaultWindow);
+
+        using var scope = _provider.CreateScope();
+        var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+        Assert.Empty(await ctx.BundleUnlockAttempts.AsNoTracking().ToListAsync());
+    }
+
+    [Fact]
+    public async Task ScheduledPrune_RespectsTheRetentionHorizonAndTheCadenceClaim()
+    {
+        // Two negative controls in one: a row inside the retention horizon survives, and
+        // the shared cadence claim means a burst of ticks cannot prune more often than
+        // PruneMinInterval (so ticking on the 1-minute session sweep is cheap).
+        var clock = new TestClock(new DateTimeOffset(2026, 6, 1, 12, 0, 0, TimeSpan.Zero));
+        var window = new SqlBundleUnlockAttemptWindow(
+            _provider.GetRequiredService<IServiceScopeFactory>(), clock);
+
+        Assert.True(await window.TryRegisterAttemptAsync(
+            "10.0.0.1", 10, BundleUnlockRateLimiter.DefaultWindow));
+
+        // Recent row, past the cadence: pruning runs but must delete nothing.
+        clock.Advance(TimeSpan.FromMinutes(30));
+        for (var i = 0; i < 5; i++)
+        {
+            await window.PruneAsync(BundleUnlockRateLimiter.DefaultWindow);
+        }
+
+        using var scope = _provider.CreateScope();
+        var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+        Assert.Equal(1, await ctx.BundleUnlockAttempts.AsNoTracking().CountAsync());
+    }
+
+    [Fact]
+    public async Task ScheduledPrune_DoesNotThrow_WhenTheStoreIsUnavailable()
+    {
+        // The sweep that drives this has no caller to report to, so PruneAsync's contract
+        // is "never throw" — a database blip must not kill the background service loop.
+        var services = new ServiceCollection();
+        services.AddScoped<IBundleUnlockAttemptRepository>(
+            _ => throw new InvalidOperationException("database unavailable"));
+        using var broken = services.BuildServiceProvider();
+
+        var window = new SqlBundleUnlockAttemptWindow(
+            broken.GetRequiredService<IServiceScopeFactory>(), TimeProvider.System);
+
+        await window.PruneAsync(BundleUnlockRateLimiter.DefaultWindow);
+    }
+
    /// <summary>Minimal mutable-clock <see cref="TimeProvider"/> for deterministic windows.</summary>
    private sealed class TestClock : TimeProvider
    {
@@ -219,7 +219,7 @@ public sealed class BundleSessionObservabilityTests
        // against this test's own store (register row 74) — see ReadGaugeDelta.
        var clock = new TestClock(DateTimeOffset.UtcNow);
        var store = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger<BundleSessionStore>.Instance);
-        var service = new BundleSessionEvictionService(store, NullLogger<BundleSessionEvictionService>.Instance);
+        var service = new BundleSessionEvictionService(store, InertUnlockWindow.Instance, NullLogger<BundleSessionEvictionService>.Instance);

        await service.StartAsync(CancellationToken.None);
        try
@@ -259,8 +259,8 @@ public sealed class BundleSessionObservabilityTests
        var clock = new TestClock(DateTimeOffset.UtcNow);
        var oldStore = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger<BundleSessionStore>.Instance);
        var newStore = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger<BundleSessionStore>.Instance);
-        var oldService = new BundleSessionEvictionService(oldStore, NullLogger<BundleSessionEvictionService>.Instance);
-        var newService = new BundleSessionEvictionService(newStore, NullLogger<BundleSessionEvictionService>.Instance);
+        var oldService = new BundleSessionEvictionService(oldStore, InertUnlockWindow.Instance, NullLogger<BundleSessionEvictionService>.Instance);
+        var newService = new BundleSessionEvictionService(newStore, InertUnlockWindow.Instance, NullLogger<BundleSessionEvictionService>.Instance);

        await oldService.StartAsync(CancellationToken.None);
        await newService.StartAsync(CancellationToken.None);
@@ -423,6 +423,24 @@ public sealed class BundleSessionObservabilityTests
        public void Advance(TimeSpan by) => _now = _now.Add(by);
    }

+    /// <summary>
+    /// Do-nothing <see cref="IBundleUnlockAttemptWindow"/> for the eviction-service tests.
+    /// The service also ticks the unlock-attempt retention prune (register row 65), but
+    /// these tests exercise only its gauge lifecycle — never <c>ExecuteAsync</c> — so the
+    /// window is present purely to satisfy the constructor. The prune tick itself is
+    /// covered by <c>FleetWideUnlockThrottleTests</c> against the real SQL-backed window.
+    /// </summary>
+    private sealed class InertUnlockWindow : IBundleUnlockAttemptWindow
+    {
+        public static readonly InertUnlockWindow Instance = new();
+
+        public Task<bool> TryRegisterAttemptAsync(
+            string clientKey, int maxAttemptsPerWindow, TimeSpan window, CancellationToken ct = default)
+            => Task.FromResult(true);
+
+        public Task PruneAsync(TimeSpan window, CancellationToken ct = default) => Task.CompletedTask;
+    }
+
    /// <summary>Captures formatted log entries so the cap-rejection Warning can be asserted.</summary>
    private sealed class CapturingLogger : ILogger<BundleSessionStore>
    {