test(dcl+communication+storeandforward+kpihistory+host): close register row-40-class sleeps + row-32 provider leak (arch-review 2026-08-19 P8-8 + P8-9)

P8-8: replace the remaining wall-clock sleep-then-assert instances outside Communication.Tests' Medium-wave sweep with the same row-40 house pattern — continuous polling (AssertHoldsFor) for ABSENCE assertions instead of a single end-of-window sample, which cannot distinguish "stayed quiet throughout" from "hadn't happened yet when the one sample landed" and cannot catch a value that flips wrong and back before that sample. - DataConnectionActorBatchTests: QualityCounters_ArePushedOnTransitionOnly_AndCoalesced (:206/:214 class) converted to AssertHoldsFor; ResolutionProbes_BackOff... (:181/:184, a genuine real-time backoff-RATE assertion, not a sync-primitive stand-in) converted to AssertRoundsWithinWindow, which enforces the ceiling continuously (fails fast on a no-backoff regression) while the floor still necessarily waits out the window. - SiteAlarmLiveCacheServiceTests (:452-454, :470-472): converted to the project's own AssertHoldsFor (mirrors SiteAlarmAggregatorActorTests', same assembly). - StoreAndForwardServiceTests (:891-892): async AssertHoldsForAsync variant (the class runs on the async test thread). - KpiHistoryRecorderActorTests (:1043-1049): two skip-path FoldCount==0 absence checks converted. Each helper is a private per-file copy, matching the Medium-wave T17 precedent (SiteAlarmAggregatorActorTests.AssertHoldsFor) rather than a shared TestSupport utility. Proof of the mechanism: a standalone (non-repo) harness run 100 trials of a real background thread flipping a boolean true then back to false inside a 300ms window — Thread.Sleep(300)+one sample missed the violation 100/100 times (false pass), continuous 20ms polling caught it 100/100 times. Most of the actual conversions here guard monotonic counters (their single-sample variant would eventually see the final value too), so each converted test carries an inline honesty note on what specifically the conversion buys for that assertion (fail-fast vs. true flip-back detection). P8-9: ScadaBridgeTelemetryTests.EmitHelpers_DoNotThrow called SetQueueDepthProvider(() => 5) against the process-global row-32 provider slot with no clear and no [Collection] — a landmine for any future same-process reader of scadabridge.store_and_forward.queue.depth. Fixed with compare-and-clear in a finally via the existing (production, untouched) ClearQueueDepthProvider. Searched this assembly and the whole test tree for a second same-process reader that asserts on the value: none exists today (MetricsEndpointTests scrapes /metrics but only asserts on the response shape, and the only value-asserting test, QueueDepthGaugeTests, lives in StoreAndForward.Tests — a separate test-host process the leak cannot reach), so no [Collection] was added; the test's doc comment records the search and flags it for re-check if that changes. Test-only change, zero production edits. Full suites green: DCL.Tests 342/342, Communication.Tests 729/729, StoreAndForward.Tests 135/135, KpiHistory.Tests 54/54, Host.Tests 522/522. Each converted test additionally looped 20x in isolation with zero flakes.
2026-08-19 16:33:24 -04:00
parent b34ec10f8b
commit 428b8db024
5 changed files with 196 additions and 19 deletions
@@ -136,6 +136,30 @@ public class SiteAlarmLiveCacheServiceTests : TestKit
        return service;
    }

+    /// <summary>
+    /// Row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave
+    /// <c>SiteAlarmAggregatorActorTests.AssertHoldsFor</c> treatment in this same test
+    /// project): proves every check in <paramref name="checks"/> keeps holding throughout
+    /// <paramref name="window"/> by polling, rather than a single blind
+    /// <c>Thread.Sleep(window)</c> followed by one end-of-window sample. A bare
+    /// sleep-then-assert cannot distinguish "the mechanism correctly stayed quiet" from "the
+    /// mechanism just hadn't run yet when the one sample was taken", and it can't catch a
+    /// value that flips wrong and then flips back before that single sample. This fails
+    /// immediately the first tick any check goes false, and only returns once the full
+    /// window has elapsed with every check still true.
+    /// </summary>
+    private static void AssertHoldsFor(TimeSpan window, params (Func<bool> Holds, string Because)[] checks)
+    {
+        var deadline = DateTime.UtcNow + window;
+        while (true)
+        {
+            foreach (var (holds, because) in checks)
+                Assert.True(holds(), because);
+            if (DateTime.UtcNow >= deadline) return;
+            Thread.Sleep(20);
+        }
+    }
+
    [Fact]
    public void Seed_FanOut_Publishes_The_Instances_That_Failed_To_Answer()
    {
@@ -450,8 +474,8 @@ public class SiteAlarmLiveCacheServiceTests : TestKit
        sub.Dispose();
        AwaitAssert(() => Assert.False(service.IsLive(SiteId)), TimeSpan.FromSeconds(10));
        var startsAfterStop = factory.GetOrCreateCount;
-        Thread.Sleep(300);
-        Assert.Equal(startsAfterStop, factory.GetOrCreateCount); // no self-heal restart fired
+        AssertHoldsFor(TimeSpan.FromMilliseconds(300),
+            (() => factory.GetOrCreateCount == startsAfterStop, "no self-heal restart fired"));
    }

    // ── WP2.3: IsLive tracks the STREAM, not merely "a snapshot was published once" ──
@@ -468,8 +492,8 @@ public class SiteAlarmLiveCacheServiceTests : TestKit
        // the cache is not live — the page must keep polling. Pre-fix IsLive flipped true
        // here and the page grafted a never-updating snapshot over fresh poll data.
        AwaitCondition(() => service.GetCurrentAlarms(SiteId) is not null, TimeSpan.FromSeconds(5));
-        Thread.Sleep(400);
-        Assert.False(service.IsLive(SiteId));
+        AssertHoldsFor(TimeSpan.FromMilliseconds(400),
+            (() => !service.IsLive(SiteId), "must stay not-live until the site accepts the stream"));

        // The site accepts it → live.
        AwaitCondition(() => factory.Client.Connects.Count >= 1, TimeSpan.FromSeconds(5));
@@ -177,11 +177,73 @@ public class DataConnectionActorBatchTests : TestKit
        ExpectMsg<TagValueUpdate>(u => u.Quality == QualityCode.Bad, TimeSpan.FromSeconds(5));
        ExpectMsg<SubscribeTagsResponse>(TimeSpan.FromSeconds(5));

+        // This is a genuine real-time rate assertion (how many retry rounds a backoff
+        // schedule produces inside a fixed wall-clock window), not a synchronization
+        // primitive standing in for "wait until X happens" — proving the floor (>= 2
+        // rounds) is unavoidably tied to actually waiting out the window. What the old
+        // blind Thread.Sleep(2000) + single end-of-window sample could NOT catch is a
+        // regression to full-rate (no-backoff) retrying: it would still land somewhere
+        // in [2, 8] by luck if the window elapsed while under load, or it would blow
+        // past 8 well before the 2s mark and nobody would know until the final sample.
+        // AssertRoundsWithinWindow polls throughout and fails immediately the first time
+        // the ceiling is breached, so a no-backoff regression is caught fast instead of
+        // being masked by (or racing) the fixed window.
        var initialCalls = adapter.SubscribeBatches.Count;
-        Thread.Sleep(2000);
-        var rounds = adapter.SubscribeBatches.Count - initialCalls;
+        AssertRoundsWithinWindow(TimeSpan.FromSeconds(2),
+            () => adapter.SubscribeBatches.Count - initialCalls,
+            minInclusive: 2, maxInclusive: 8,
+            context: "dead-device probe backoff (floor 150ms, ceiling 600ms)");
+    }

-        Assert.InRange(rounds, 2, 8);
+    /// <summary>
+    /// Row-40 house pattern adapted for a RATE assertion: proves a monotonically-increasing
+    /// count stays within <c>[minInclusive, maxInclusive]</c> across the whole
+    /// <paramref name="window"/>, rather than a single blind sleep-then-sample at the end.
+    /// The upper bound is enforced continuously (fails fast the instant it is exceeded,
+    /// well before the window elapses); the lower bound can only be confirmed once the
+    /// full window has elapsed, since proving "at least N happened by wall-clock time T"
+    /// requires actually waiting until T.
+    /// </summary>
+    private static void AssertRoundsWithinWindow(
+        TimeSpan window, Func<int> currentRounds, int minInclusive, int maxInclusive, string context)
+    {
+        var deadline = DateTime.UtcNow + window;
+        while (true)
+        {
+            var rounds = currentRounds();
+            Assert.True(rounds <= maxInclusive,
+                $"{context}: {rounds} rounds already fired, exceeding the ceiling of {maxInclusive} " +
+                "before the window even elapsed — backoff is not slowing retries down.");
+            if (DateTime.UtcNow >= deadline)
+            {
+                Assert.True(rounds >= minInclusive,
+                    $"{context}: only {rounds} rounds fired in the window (need >= {minInclusive}).");
+                return;
+            }
+            Thread.Sleep(20);
+        }
+    }
+
+    /// <summary>
+    /// Row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave
+    /// <c>SiteAlarmAggregatorActorTests.AssertHoldsFor</c> treatment): proves
+    /// <paramref name="holds"/> keeps being true for the whole <paramref name="window"/> by
+    /// polling, rather than a single blind <c>Thread.Sleep(window)</c> followed by one
+    /// end-of-window sample. A bare sleep-then-assert cannot distinguish "the mechanism
+    /// correctly stayed quiet" from "the mechanism just hadn't run yet when the one sample
+    /// was taken", and it can't catch a value that flips wrong and then flips back before
+    /// that single sample. This fails immediately the first time <paramref name="holds"/>
+    /// goes false, and only returns once the full window has elapsed with it still true.
+    /// </summary>
+    private static void AssertHoldsFor(TimeSpan window, Func<bool> holds, string because)
+    {
+        var deadline = DateTime.UtcNow + window;
+        while (true)
+        {
+            Assert.True(holds(), because);
+            if (DateTime.UtcNow >= deadline) return;
+            Thread.Sleep(20);
+        }
    }

    [Fact]
@@ -203,16 +265,18 @@ public class DataConnectionActorBatchTests : TestKit
        for (var i = 0; i < 20; i++)
            adapter.ValueCallback!("tag1", new TagValue(i, QualityCode.Good, DateTimeOffset.UtcNow));

-        Thread.Sleep(400);
-        Assert.DoesNotContain(_health.ReceivedCalls(), c => c.GetMethodInfo().Name == "UpdateTagQuality");
+        AssertHoldsFor(TimeSpan.FromMilliseconds(400),
+            () => !_health.ReceivedCalls().Any(c => c.GetMethodInfo().Name == "UpdateTagQuality"),
+            "20 unchanged-quality repeats must never push UpdateTagQuality");

        // One genuine transition → exactly one coalesced push.
        adapter.ValueCallback!("tag1", new TagValue(1, QualityCode.Bad, DateTimeOffset.UtcNow));
        AwaitCondition(
            () => _health.ReceivedCalls().Count(c => c.GetMethodInfo().Name == "UpdateTagQuality") == 1,
            TimeSpan.FromSeconds(3));
-        Thread.Sleep(300);
-        Assert.Single(_health.ReceivedCalls(), c => c.GetMethodInfo().Name == "UpdateTagQuality");
+        AssertHoldsFor(TimeSpan.FromMilliseconds(300),
+            () => _health.ReceivedCalls().Count(c => c.GetMethodInfo().Name == "UpdateTagQuality") == 1,
+            "the coalesced push must not repeat");
    }

    [Fact]
@@ -15,9 +15,37 @@ public class ScadaBridgeTelemetryTests
        Assert.Equal("ZB.MOM.WW.ScadaBridge", ScadaBridgeTelemetry.MeterName);
    }

+    /// <summary>
+    /// Register row 32 class: <c>SetQueueDepthProvider</c> writes the SAME process-global
+    /// slot <c>QueueDepthGaugeCollection</c> (StoreAndForward.Tests) exists to serialize
+    /// access to — this test was mutating it with no clear and no <c>[Collection]</c>,
+    /// leaving a stray <c>() =&gt; 5</c> provider registered in the process for the rest of
+    /// this assembly's run. Since the meter/instrument state is a single static field on
+    /// <c>ScadaBridgeTelemetry</c> (Commons), the leak is scoped to whatever process this
+    /// assembly's test host runs in — it cannot reach StoreAndForward.Tests (a separate
+    /// assembly with its own test-host process), but it IS visible within Host.Tests, whose
+    /// <c>MetricsEndpointTests</c> boots a real in-process host (<c>WebApplicationFactory</c>
+    /// shares this AppDomain, not a child process) and scrapes <c>/metrics</c>, which
+    /// enumerates every registered gauge including this one.
+    ///
+    /// Honesty note on what was and wasn't proven: a search of this assembly (and the whole
+    /// test tree) found no test today that asserts on the VALUE of
+    /// <c>scadabridge.store_and_forward.queue.depth</c> from Host.Tests or any other
+    /// same-process reader — <c>MetricsEndpointTests</c> only asserts the endpoint returns
+    /// Prometheus-shaped text, not this series' content, and the only test that DOES assert
+    /// on the value (<c>QueueDepthGaugeTests</c>) lives in StoreAndForward.Tests, a distinct
+    /// test-host process the leak cannot reach. So there is no currently-reproducible flake
+    /// to demonstrate here (no fork of this test with the leak left in has an assertion to
+    /// break) — the defect is a landmine for a FUTURE same-process reader, not a live one.
+    /// The fix (compare-and-clear in <c>finally</c>) closes the landmine regardless; a
+    /// <c>[Collection]</c> is intentionally not added since no second same-process reader of
+    /// this slot exists to serialize against as of this writing — re-run this search before
+    /// removing the guard below if that changes.
+    /// </summary>
    [Fact]
    public void EmitHelpers_DoNotThrow()
    {
+        Func<long> queueDepthProvider = () => 5;
        var ex = Record.Exception(() =>
        {
            ScadaBridgeTelemetry.RecordDeploymentApplied();
@@ -27,9 +55,20 @@ public class ScadaBridgeTelemetryTests
            ScadaBridgeTelemetry.LiveAlarmAggregatorStarted();
            ScadaBridgeTelemetry.LiveAlarmAggregatorStopped();
            ScadaBridgeTelemetry.RecordLiveAlarmStreamReconnect();
-            ScadaBridgeTelemetry.SetQueueDepthProvider(() => 5);
+            ScadaBridgeTelemetry.SetQueueDepthProvider(queueDepthProvider);
        });

-        Assert.Null(ex);
+        try
+        {
+            Assert.Null(ex);
+        }
+        finally
+        {
+            // Compare-and-clear: only removes OUR delegate from the process-global slot, so
+            // a legitimate concurrent registrant (a real StoreAndForwardService started
+            // elsewhere in this process) is never stomped — mirrors
+            // ScadaBridgeTelemetry.ClearQueueDepthProvider's own doc'd contract.
+            ScadaBridgeTelemetry.ClearQueueDepthProvider(queueDepthProvider);
+        }
    }
 }
@@ -497,6 +497,28 @@ public class KpiHistoryRecorderActorTests : TestKit
            NullLogger<KpiHistoryRecorderActor>.Instance)));
    }

+    /// <summary>
+    /// Row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave
+    /// <c>SiteAlarmAggregatorActorTests.AssertHoldsFor</c> treatment): proves
+    /// <paramref name="holds"/> keeps being true for the whole <paramref name="window"/> by
+    /// polling, rather than a single blind <c>Thread.Sleep(window)</c> followed by one
+    /// end-of-window sample. A bare sleep-then-assert cannot distinguish "the mechanism
+    /// correctly stayed quiet" from "the mechanism just hadn't run yet when the one sample
+    /// was taken", and it can't catch a value that flips wrong and then flips back before
+    /// that single sample. This fails immediately the first time <paramref name="holds"/>
+    /// goes false, and only returns once the full window has elapsed with it still true.
+    /// </summary>
+    private static void AssertHoldsFor(TimeSpan window, Func<bool> holds, string because)
+    {
+        var deadline = DateTime.UtcNow + window;
+        while (true)
+        {
+            Assert.True(holds(), because);
+            if (DateTime.UtcNow >= deadline) return;
+            Thread.Sleep(20);
+        }
+    }
+
    [Fact]
    public void SampleTick_WritesHealthySourceSamples_AndThrowingSourceDoesNotAbortTick()
    {
@@ -1040,13 +1062,13 @@ public class KpiHistoryRecorderActorTests : TestKit
            interval: TimeSpan.FromMilliseconds(50));

        // …and then skip: no slices are ever folded.
-        Thread.Sleep(300);
-        Assert.Equal(0, repository.FoldCount);
+        AssertHoldsFor(TimeSpan.FromMilliseconds(300), () => repository.FoldCount == 0,
+            "watermark inside the lookback tail must skip the backfill entirely");

        // A second BackfillTick is a no-op (the done-latch is set).
        actor.Tell(KpiHistoryRecorderActor.BackfillTick.Instance);
-        Thread.Sleep(300);
-        Assert.Equal(0, repository.FoldCount);
+        AssertHoldsFor(TimeSpan.FromMilliseconds(300), () => repository.FoldCount == 0,
+            "the done-latch must make a second BackfillTick a no-op");
    }

    [Fact]
@@ -786,6 +786,29 @@ public class StoreAndForwardServiceTests : IAsyncLifetime, IDisposable
        while (Interlocked.CompareExchange(ref target, value, current) != current);
    }

+    /// <summary>
+    /// Register row-40 house pattern for an ABSENCE assertion (mirrors the Medium-wave
+    /// <c>SiteAlarmAggregatorActorTests.AssertHoldsFor</c> treatment, ported here async since
+    /// this class's facts run on the async test thread): proves <paramref name="holds"/> keeps
+    /// being true for the whole <paramref name="window"/> by polling, rather than a single
+    /// blind <c>await Task.Delay(window)</c> followed by one end-of-window sample. A bare
+    /// delay-then-assert cannot distinguish "the mechanism correctly stayed quiet" from "the
+    /// mechanism just hadn't run yet when the one sample was taken", and it can't catch a
+    /// value that flips wrong and then flips back before that single sample. This fails
+    /// immediately the first time <paramref name="holds"/> goes false, and only returns once
+    /// the full window has elapsed with it still true throughout.
+    /// </summary>
+    private static async Task AssertHoldsForAsync(TimeSpan window, Func<bool> holds, string because)
+    {
+        var deadline = DateTime.UtcNow + window;
+        while (true)
+        {
+            Assert.True(holds(), because);
+            if (DateTime.UtcNow >= deadline) return;
+            await Task.Delay(20);
+        }
+    }
+
    // ── Task 12: deferToSweep enqueue mode ──

    /// <summary>
@@ -888,8 +911,13 @@ public class StoreAndForwardServiceTests : IAsyncLifetime, IDisposable
        service.TriggerSweep(); // the clobbering kick

        var stop = service.StopAsync();
-        await Task.Delay(300);
-        Assert.False(stop.IsCompleted); // pre-fix: StopAsync already returned (awaited the no-op)
+        // The real sweep is blocked on `release` behind `entered`, so `stop` is genuinely
+        // unable to complete during this window (not merely "hasn't yet") — proving it stays
+        // incomplete throughout, not just at one end-of-delay sample, is what actually
+        // distinguishes "waited for the real sweep" from "pre-fix: awaited the no-op and
+        // returned instantly, then this single sample raced whether that had happened yet".
+        await AssertHoldsForAsync(TimeSpan.FromMilliseconds(300), () => !stop.IsCompleted,
+            "pre-fix: StopAsync already returned (awaited the no-op)");

        release.TrySetResult();
        await stop.WaitAsync(TimeSpan.FromSeconds(5)); // drains the real sweep promptly once released