fix(kpihistory)+fix(notificationoutbox)+fix(sitecallaudit)+fix(host): P5-5 — rate-fold interval triple made self-consistent + cross-validated

Defect: the hourly KPI rollup fold (KpiHistoryRepository.FoldHourlyRollupsAsync) sums every Rate-classified sample recorded in an hour, correct only if each sample is a non-overlapping delta covering exactly one recorder tick (KpiHistoryOptions.SampleInterval). That invariant was an unenforced silent assumption across three independently-owned config keys — SampleInterval, NotificationOutboxOptions.DeliveredKpiWindow, SiteCallAuditOptions.KpiInterval — so widening DeliveredKpiWindow to e.g. 15 min would silently over-count the rollup ~15x. Robust fix: NotificationOutboxKpiSampleSource and SiteCallAuditKpiSampleSource now rescale their Rate-classified samples (deliveredLastInterval, failedLastInterval) by SampleInterval / (DeliveredKpiWindow or KpiInterval) before recording, so the rollup's per-hour Sum is correct regardless of how the other two keys are tuned — the live dashboard tile's own cutoff/window is untouched. Both sources gain a project reference to KpiHistory to read SampleInterval; no cycle (KpiHistory references only Commons). Eager cross-validation (defense-in-depth): KpiRateFoldIntervalCrossValidator (Host, the one composition root referencing all three component projects) fails boot naming all three config keys when they diverge, protecting the separate (still-real) invariant that the recorded/charted KPI-history sample equals the live tile at the same instant — which the rescale trades away whenever the scale factor isn't 1. Rides KpiHistoryOptions' existing ValidateOnStart trigger. Also: NotificationOutboxOptions had no registered validator at all before this (the gap this task's "whichever options class owns DeliveredKpiWindow" file scope pointed at) — added NotificationOutboxOptionsValidator mirroring SiteCallAuditOptionsValidator's positive-duration/count convention, wired with ValidateOnStart. Tests: new rescale tests in both sample-source suites prove the fold is numerically correct at a non-default SampleInterval (30/40 counted over a 15/20-minute window correctly divides down to a 1-minute-tick delta); new KpiRateFoldIntervalCrossValidatorTests prove divergent config fails with all three keys named; new NotificationOutboxOptionsValidatorTests. Every new test proven to fail against the unfixed computation first (temporarily short-circuited the scale factor / cross-check to the pre-fix behavior, confirmed failure, restored) — not via git stash (repo-wide, shared across concurrent worktrees this session) but via file-copy swap. Full suites green: Host.Tests (527), ConfigurationDatabase.Tests (401), KpiHistory.Tests (54), NotificationOutbox.Tests (163), SiteCallAudit.Tests (67).
2026-08-19 16:39:50 -04:00
parent 1cea01d924
commit abbbcd1a8d
13 changed files with 613 additions and 20 deletions
@@ -0,0 +1,92 @@
+using Microsoft.Extensions.Options;
+using ZB.MOM.WW.ScadaBridge.KpiHistory;
+using ZB.MOM.WW.ScadaBridge.NotificationOutbox;
+using ZB.MOM.WW.ScadaBridge.SiteCallAudit;
+
+namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
+
+/// <summary>
+/// P5-5 (arch-review Low): <see cref="KpiRateFoldIntervalCrossValidator"/> is the
+/// boot-time guardrail for the "rate-fold interval triple" —
+/// <see cref="KpiHistoryOptions.SampleInterval"/>,
+/// <see cref="NotificationOutboxOptions.DeliveredKpiWindow"/>, and
+/// <see cref="SiteCallAuditOptions.KpiInterval"/>. The sample sources now rescale
+/// their Rate-classified samples onto <c>SampleInterval</c>, so the hourly rollup
+/// fold's correctness no longer depends on this equality — but the
+/// recorded/charted sample no longer equals the live dashboard tile unless it
+/// holds, so this validator still fails the host at boot on any divergence,
+/// naming all three configuration keys.
+/// </summary>
+public class KpiRateFoldIntervalCrossValidatorTests
+{
+    private static ValidateOptionsResult Validate(
+        KpiHistoryOptions kpiHistoryOptions,
+        NotificationOutboxOptions notificationOutboxOptions,
+        SiteCallAuditOptions siteCallAuditOptions)
+    {
+        var validator = new KpiRateFoldIntervalCrossValidator(
+            Options.Create(notificationOutboxOptions),
+            Options.Create(siteCallAuditOptions));
+        return validator.Validate(Options.DefaultName, kpiHistoryOptions);
+    }
+
+    [Fact]
+    public void AllThreeEqual_AtDocumentedDefaults_IsValid()
+    {
+        var result = Validate(
+            new KpiHistoryOptions(), new NotificationOutboxOptions(), new SiteCallAuditOptions());
+
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void AllThreeEqual_AtANonDefaultCommonValue_IsValid()
+    {
+        var common = TimeSpan.FromMinutes(5);
+        var result = Validate(
+            new KpiHistoryOptions { SampleInterval = common },
+            new NotificationOutboxOptions { DeliveredKpiWindow = common },
+            new SiteCallAuditOptions { KpiInterval = common });
+
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void DeliveredKpiWindowDiverges_FailsAndNamesAllThreeKeys()
+    {
+        var result = Validate(
+            new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) },
+            new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(15) },
+            new SiteCallAuditOptions { KpiInterval = TimeSpan.FromMinutes(1) });
+
+        Assert.True(result.Failed);
+        Assert.Contains("ScadaBridge:KpiHistory:SampleInterval", result.FailureMessage);
+        Assert.Contains("ScadaBridge:NotificationOutbox:DeliveredKpiWindow", result.FailureMessage);
+        Assert.Contains("ScadaBridge:SiteCallAudit:KpiInterval", result.FailureMessage);
+    }
+
+    [Fact]
+    public void KpiIntervalDiverges_FailsAndNamesAllThreeKeys()
+    {
+        var result = Validate(
+            new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) },
+            new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(1) },
+            new SiteCallAuditOptions { KpiInterval = TimeSpan.FromMinutes(20) });
+
+        Assert.True(result.Failed);
+        Assert.Contains("ScadaBridge:KpiHistory:SampleInterval", result.FailureMessage);
+        Assert.Contains("ScadaBridge:NotificationOutbox:DeliveredKpiWindow", result.FailureMessage);
+        Assert.Contains("ScadaBridge:SiteCallAudit:KpiInterval", result.FailureMessage);
+    }
+
+    [Fact]
+    public void BothDeliveredKpiWindowAndKpiIntervalDiverge_Fails()
+    {
+        var result = Validate(
+            new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) },
+            new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(15) },
+            new SiteCallAuditOptions { KpiInterval = TimeSpan.FromMinutes(20) });
+
+        Assert.True(result.Failed);
+    }
+}
@@ -3,6 +3,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Kpi;
 using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Notifications;
+using ZB.MOM.WW.ScadaBridge.KpiHistory;
 using ZB.MOM.WW.ScadaBridge.NotificationOutbox.Kpi;

 namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests.Kpi;
@@ -22,8 +23,28 @@ public class NotificationOutboxKpiSampleSourceTests
        DeliveredKpiWindow = TimeSpan.FromMinutes(1),
    };

+    // Matches Options.DeliveredKpiWindow (1 minute) so the default-config tests below see
+    // an identity rescale factor (1.0) and their pre-existing raw-count assertions hold.
+    private static readonly KpiHistoryOptions DefaultKpiHistoryOptions = new()
+    {
+        SampleInterval = TimeSpan.FromMinutes(1),
+    };
+
    private static NotificationOutboxKpiSampleSource CreateSource(INotificationOutboxRepository repository) =>
-        new(repository, Microsoft.Extensions.Options.Options.Create(Options));
+        CreateSource(repository, DefaultKpiHistoryOptions);
+
+    private static NotificationOutboxKpiSampleSource CreateSource(
+        INotificationOutboxRepository repository, KpiHistoryOptions kpiHistoryOptions) =>
+        CreateSource(repository, Options, kpiHistoryOptions);
+
+    private static NotificationOutboxKpiSampleSource CreateSource(
+        INotificationOutboxRepository repository,
+        NotificationOutboxOptions options,
+        KpiHistoryOptions kpiHistoryOptions) =>
+        new(
+            repository,
+            Microsoft.Extensions.Options.Options.Create(options),
+            Microsoft.Extensions.Options.Options.Create(kpiHistoryOptions));

    [Fact]
    public void Source_IsNotificationOutbox()
@@ -131,6 +152,45 @@ public class NotificationOutboxKpiSampleSourceTests
        AssertSample(samples, "oldestPendingAgeSeconds", KpiScopes.Node, "node-a", 60);
    }

+    // P5-5 (arch-review Low): the KPI-history rollup fold sums every deliveredLastInterval
+    // sample recorded in an hour, which over-counts unless each sample is a non-overlapping
+    // delta covering exactly one recorder tick (KpiHistoryOptions.SampleInterval). Before the
+    // fix, this class recorded the RAW DeliveredKpiWindow-windowed count verbatim — with a
+    // 15-minute DeliveredKpiWindow sampled every 1-minute SampleInterval, 60 overlapping
+    // 15-minute counts summed to ~15x the true hourly total. Proven against the unfixed
+    // computation: temporarily short-circuiting DeliveredIntervalScaleFactor to always return
+    // 1.0 (i.e. "record the raw count verbatim", the pre-P5-5 behavior) makes this test fail
+    // (30 != 2); restoring the real scale factor passes it.
+    [Fact]
+    public async Task CollectAsync_RescalesDeliveredLastInterval_OntoKpiHistorySampleInterval()
+    {
+        var repository = Substitute.For<INotificationOutboxRepository>();
+        repository.ComputeKpisAsync(Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>())
+            .Returns(new NotificationKpiSnapshot(
+                QueueDepth: 0, StuckCount: 0, ParkedCount: 0,
+                DeliveredLastInterval: 30, OldestPendingAge: null));
+        repository.ComputePerSiteKpisAsync(Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>())
+            .Returns(Array.Empty<SiteNotificationKpiSnapshot>());
+        repository.ComputePerNodeKpisAsync(Arg.Any<DateTimeOffset>(), Arg.Any<DateTimeOffset>(), Arg.Any<CancellationToken>())
+            .Returns(Array.Empty<NodeNotificationKpiSnapshot>());
+
+        // DeliveredKpiWindow (15 min) is 15x KpiHistoryOptions.SampleInterval (1 min) — a
+        // non-default SampleInterval configuration deliberately chosen to prove the rescale
+        // math, not just the identity (factor == 1) default-config case the other tests use.
+        var options = new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromMinutes(15) };
+        var kpiHistoryOptions = new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) };
+        var source = CreateSource(repository, options, kpiHistoryOptions);
+
+        var samples = await source.CollectAsync(CapturedAt);
+
+        var delivered = Assert.Single(samples, s => s.Metric == "deliveredLastInterval");
+        // 30 counted over a 15-minute window, rescaled onto a 1-minute recorder tick: 30 / 15 = 2.
+        // Summing 60 such 1-minute-tick samples across an hour therefore reconstructs the true
+        // hourly total instead of the ~15x over-count a raw (unscaled) 15-minute count would
+        // produce if simply summed 60 times.
+        Assert.Equal(2.0, delivered.Value, precision: 6);
+    }
+
    [Fact]
    public async Task CollectAsync_OmitsOldestPendingAge_WhenNull()
    {
@@ -0,0 +1,92 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests;
+
+/// <summary>
+/// P5-5 (arch-review Low): <see cref="NotificationOutboxOptions"/> had no
+/// registered validator before this — the dispatch/purge/KPI cadences and the
+/// delivered-throughput KPI window (<see cref="NotificationOutboxOptions.DeliveredKpiWindow"/>)
+/// could be zero/negative with no boot-time signal, which would spin an Akka
+/// scheduler or — because <c>NotificationOutboxKpiSampleSource</c> divides by
+/// <c>DeliveredKpiWindow</c> to rescale onto <c>KpiHistoryOptions.SampleInterval</c>
+/// — divide by zero. <see cref="NotificationOutboxOptions.MaxParallelDeliveries"/>
+/// is validated on its BASE property, not the clamped
+/// <see cref="NotificationOutboxOptions.ResolvedMaxParallelDeliveries"/>, so a
+/// misconfigured non-positive value fails fast instead of silently clamping to 1.
+/// </summary>
+public class NotificationOutboxOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(NotificationOutboxOptions options) =>
+        new NotificationOutboxOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new NotificationOutboxOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroDispatchInterval_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { DispatchInterval = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("DispatchInterval", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroDispatchBatchSize_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { DispatchBatchSize = 0 });
+        Assert.True(result.Failed);
+        Assert.Contains("DispatchBatchSize", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroStuckAgeThreshold_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { StuckAgeThreshold = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("StuckAgeThreshold", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroTerminalRetention_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { TerminalRetention = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("TerminalRetention", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroPurgeInterval_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { PurgeInterval = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("PurgeInterval", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroDeliveredKpiWindow_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("DeliveredKpiWindow", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeDeliveredKpiWindow_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { DeliveredKpiWindow = TimeSpan.FromSeconds(-1) });
+        Assert.True(result.Failed);
+        Assert.Contains("DeliveredKpiWindow", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroMaxParallelDeliveries_IsRejected()
+    {
+        var result = Validate(new NotificationOutboxOptions { MaxParallelDeliveries = 0 });
+        Assert.True(result.Failed);
+        Assert.Contains("MaxParallelDeliveries", result.FailureMessage);
+    }
+}
@@ -5,6 +5,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
 using ZB.MOM.WW.ScadaBridge.Commons.Types;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Kpi;
+using ZB.MOM.WW.ScadaBridge.KpiHistory;
 using ZB.MOM.WW.ScadaBridge.SiteCallAudit.Kpi;

 namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.Kpi;
@@ -30,9 +31,21 @@ public class SiteCallAuditKpiSampleSourceTests
        KpiInterval = kpiInterval ?? TimeSpan.FromMinutes(1),
    };

+    // Matches Options().KpiInterval (1 minute) so the default-config tests below see an
+    // identity rescale factor (1.0) and their pre-existing raw-count assertions hold.
+    private static readonly KpiHistoryOptions DefaultKpiHistoryOptions = new()
+    {
+        SampleInterval = TimeSpan.FromMinutes(1),
+    };
+
    private static SiteCallAuditKpiSampleSource CreateSource(
-        ISiteCallAuditRepository repo, SiteCallAuditOptions? options = null) =>
-        new(repo, OptionsWrap(options ?? Options()));
+        ISiteCallAuditRepository repo,
+        SiteCallAuditOptions? options = null,
+        KpiHistoryOptions? kpiHistoryOptions = null) =>
+        new(
+            repo,
+            OptionsWrap(options ?? Options()),
+            Microsoft.Extensions.Options.Options.Create(kpiHistoryOptions ?? DefaultKpiHistoryOptions));

    private static IOptions<SiteCallAuditOptions> OptionsWrap(SiteCallAuditOptions o) =>
        Microsoft.Extensions.Options.Options.Create(o);
@@ -205,6 +218,48 @@ public class SiteCallAuditKpiSampleSourceTests
        Assert.Equal(3, repo.Calls.Count);
    }

+    // ---------------------------------------------------------------------
+    // P5-5 (arch-review Low): failedLastInterval/deliveredLastInterval are rescaled onto
+    // KpiHistoryOptions.SampleInterval before recording, so the central hourly rollup fold's
+    // per-hour Sum stays correct regardless of how KpiInterval is tuned. Before the fix, this
+    // class recorded the RAW KpiInterval-windowed counts verbatim; with a 20-minute
+    // KpiInterval sampled every 1-minute SampleInterval, summing 60 overlapping 20-minute
+    // counts across an hour would over-count ~20x. Proven against the unfixed computation:
+    // temporarily short-circuiting IntervalScaleFactor to always return 1.0 (i.e. "record the
+    // raw count verbatim", the pre-P5-5 behavior) makes this test fail (40/1 != 2/0.05);
+    // restoring the real scale factor passes it.
+    // ---------------------------------------------------------------------
+
+    [Fact]
+    public async Task CollectAsync_RescalesFailedAndDeliveredLastInterval_OntoKpiHistorySampleInterval()
+    {
+        var repo = new StubRepo
+        {
+            Global = new SiteCallKpiSnapshot(
+                BufferedCount: 0,
+                ParkedCount: 0,
+                FailedLastInterval: 1,
+                DeliveredLastInterval: 40,
+                OldestPendingAge: null,
+                StuckCount: 0),
+        };
+
+        // KpiInterval (20 min) is 20x KpiHistoryOptions.SampleInterval (1 min) — a
+        // non-default SampleInterval configuration deliberately chosen to prove the rescale
+        // math, not just the identity (factor == 1) default-config case the other tests use.
+        var options = Options(kpiInterval: TimeSpan.FromMinutes(20));
+        var kpiHistoryOptions = new KpiHistoryOptions { SampleInterval = TimeSpan.FromMinutes(1) };
+
+        var samples = await CreateSource(repo, options, kpiHistoryOptions).CollectAsync(CapturedAt);
+
+        // 1 failure / 40 deliveries counted over a 20-minute window, rescaled onto a
+        // 1-minute recorder tick: divide by 20. Summing 60 such 1-minute-tick samples across
+        // an hour therefore reconstructs the true hourly totals instead of the ~20x
+        // over-count a raw (unscaled) 20-minute count would produce if simply summed 60 times.
+        AssertHas(samples, "failedLastInterval", KpiScopes.Global, null, 0.05);
+        AssertHas(samples, "deliveredLastInterval", KpiScopes.Global, null, 2.0);
+    }
+
    // ---------------------------------------------------------------------
    // 6. Multiple sites/nodes are all emitted with distinct ScopeKeys.
    // ---------------------------------------------------------------------