Unify the collector-health ROW banding + fix the daily-collector STALE/FAILING false-positive (#1573)

CHANGELOG.md

@@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - **Darling scheduler: a service restart no longer starves long-frequency collectors - NextDue is seeded from the persisted collection watermark, not a full-interval offset** ([#1577]) - a daily collector (`index_object_stats`, CollectorScheduleDefaults frequency 1440 min) read as FAILING across most of the fleet even though every run it *did* complete was a SUCCESS; field-confirmed on DARLING01, where after a day with ~4 deploy restarts it had not run on 4 of 5 servers in ~27h. Root cause: on EVERY connect, `TryConnectAsync` seeded each collector's next-due as `now + CadencePhaseOffset(serverId, frequencySeconds)` (and `RecomputeNextDue`'s new-entry branch did the same) - a deterministic per-server phase (`(uint)serverId % periodSeconds`) that for a daily collector is *anywhere up to ~24h*. The persisted last-collection watermark was never consulted for scheduling, so each restart re-phased every collector up to a full interval forward, discarding when it actually last ran. Minute collectors were invisible to this (their offset is < 60s); long-frequency collectors were **starved indefinitely in a restart-heavy window** - each restart pushed the next run past the following restart. The full-interval offset (introduced as [#1553]'s anti-lockstep jitter) is simply too coarse for long cadences, and it also meant a FRESH install waited up to 24h for a daily collector's first run. **Fix:** the seed sites now read each collector's `MAX(collection_time)` in ONE batched round-trip per server and decide the next-due via a new pure `ComputeSeededNextDue(lastRunUtc, frequencyMinutes, nowUtc, jitter)`: a recently-run collector waits out the REMAINING interval (`lastRun + interval`), an overdue one runs promptly, and a never-run one runs shortly after connect - overdue/never-run stamped at `now + a small per-server jitter` capped at `min(interval, 150s)` (mirroring the analysis-phase jitter) to de-cluster the fleet WITHOUT the full-interval defer. A restart now RESUMES the real cadence instead of re-phasing it, a daily collector actually collects daily across restarts, and a fresh install runs it minutes after first connect (then daily). The watermark read is failure-isolated (a store hiccup falls back to a prompt jittered run, never aborting the connect), the new-entry branch of the reload recompute gets the same treatment (lazily, at most one extra round-trip per server that gains a collector), and the steady-state advance in `RunDueCollectorsAsync` (already the exact interval) is unchanged. Distinct from #1573, which fixes the *banding* (a daily collector shouldn't read FAILING at 25h); both are needed - with only #1573 the collector still wouldn't run, with only this the banding would still mis-flag the legitimate gap. Darling service only, no store schema change; the DarlingWorker field-hardening invariants (fire-and-track Retired flag, outer-thread-only sweep state, never-dispose gate, never await-all) are untouched. Verified: the Darling service builds clean in Release and the full `Darling.Tests` suite is green (**2403 passed / 0 failed / 138 gated-live skipped**), including a pure decision-table for the seed policy (daily recently-run waits ~23h; daily overdue runs promptly; 1-min recently-run keeps its ~30s feel; daily never-run seeds within ~150s - the fresh-install fix; the overdue boundary is inclusive) and the jitter cap (deterministic per FNV id, capped at `min(interval, 150s)`), plus a gated-live pin that the batched read returns `MAX(collection_time)` per collector relabeled Kind=Utc.
-
+- **Collector health: a healthy DAILY collector no longer bands STALE/FAILING, and no longer inflates the Overview "collectors failing" count** ([#1578]) - the per-collector health status used FLAT thresholds (STALE past 4h since last success, FAILING past 24h) that assumed a frequent (~1-min) collector. `index_object_stats` is a DAILY collector (`CollectorScheduleDefaults` cadence 1440 min) that succeeds every run, so between its once-a-day successes it always crossed 4h and then 24h and read STALE then FAILING - a real field false-positive. Because the Overview card's "Collectors: N failing" count reuses the SAME row banding (it counts `HealthStatus == "FAILING"`), a healthy daily collector also showed the fleet Overview a phantom failing collector. The staleness thresholds are now RELATIVE to each collector's own cadence: **FAILING when hours-since-success > max(24, 2 x freqHours)** and **STALE when > max(4, 1.5 x freqHours)**. The floors are the original flat values, so every FREQUENT collector bands byte-for-byte identically (a 1-min collector still goes stale at 4h and fails at 24h; an hourly collector too, since 1.5h/2h are under the floors); only a slow collector relaxes - a daily collector now goes stale at 36h and fails at 48h, so `index_object_stats` at 27h reads HEALTHY. On-load config snapshots (`server_config` / `database_config` / `database_scoped_config` / `trace_flags` / `server_properties`) stay staleness-exempt (banded by failure rate only), and NEVER_RUN / NO_PERMISSIONS / WARNING are unchanged. The banding was **three byte-identical copies** that nothing pinned together (the #1573 core) - Lite's grid, the Darling viewer's grid + Overview count, and the service's `get_collection_health` MCP tool + web fleet failing-count reader; all three now delegate to ONE pure classifier, `CollectorHealthClassifier` in `PerformanceMonitor.Common` (alongside the #1562 per-server card classifier), which also centralizes the on-load set so the surfaces cannot drift. Each surface resolves the collector's cadence from the shared `CollectorScheduleDefaults` default (no consumer cheaply has a per-install schedule override at the row level, so all three use the same default - which is itself the parity guarantee); an unknown collector name falls to the 24h/4h floors, i.e. the old flat behavior. A mirrored decision-table test in Lite.Tests and Darling.Tests pins the whole table - including that `index_object_stats`' default 1440-min cadence yields HEALTHY at 27h, the exact bug - so the two SKUs can never drift.
 - **Darling: the retention purge's batched DELETE no longer breaks on compressed chunks** ([#1567]) - the purge's row-capped `ctid IN (SELECT ctid ... LIMIT 10000)` batching idiom cannot execute against a TimescaleDB table with a compressed chunk in range: reading the `ctid` system column through transparent decompression is unsupported ("transparent decompression only supports tableoid system column", reproduced on the pinned 2.28.1), so the whole statement errored, the per-table isolation swallowed it as a warning, and the table silently kept its expired rows. In production that broke the DELETE fallback exactly when it matters - on a hypertable with compressed chunks whose `drop_chunks` transiently failed - and on plain-PostgreSQL stores pointed at a Timescale-compressed table. The batcher now deletes the OLDEST one-day slice of expired rows per statement (a plain time-range predicate rides TimescaleDB's supported DML decompression; the per-DML decompression cap is lifted on the purge connection, a placeholder-safe SET on stores without the extension). A day is one chunk on a hypertable and one day's arrival volume on a plain table - the same bounded-work goal the row cap served. Found via #1564: the gated purge tests flaked order-dependently because the shared fixture's compression policy jobs run immediately on creation and compressed the test chunks mid-suite; those tests now assert only their own-scoped contract (global activity counts stay order-dependent), carry a capturing logger so any purge warning lands in the failure text instead of a null logger, and a new deterministic E2E compresses a chunk synchronously and proves the DELETE path clears expired rows inside it.
 
 - **Darling: the viewer's store pool is now capped too, and the postgres.exe process count is documented for what it actually is** ([#1566]) — round-4 field verification correctly flagged that the [#1559] "≤24 backends" claim didn't hold: the process count oscillated to 44 during checkpoint/catch-up waves. Two truths behind that: **(1)** the peaks are overwhelmingly **TimescaleDB background policy workers**, not client connections — the managed conf deliberately sizes `timescaledb.max_background_workers` to hypertables + 2 (≈38), and every running compression/retention job is its own postgres.exe, so the count legitimately surges during policy waves and recedes (the field box's OS free memory was rock-stable through every swing); **(2)** the claim was also genuinely incomplete — the co-located **viewer built its connection string independently and rode Npgsql's default pool of 100**; it is now capped at 10 (a read-only UI seat on 30/60-second timers). The README's troubleshooting section gains the process-anatomy note with the `pg_stat_activity GROUP BY backend_type` decomposition query and the reminder that Windows charges the shared buffer segment to every attached process's working set, so per-process memory numbers cannot be summed.
@@ -421,7 +421,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#1571]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1571
 [#1574]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1574
 [#1577]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1577
-[#1536]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1536
+[#1578]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1578[#1536]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1536
 [#1534]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1534
 [#1533]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1533
 [#1532]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1532

Darling/Darling.Tests/CollectorHealthClassifierTests.cs

@@ -0,0 +1,182 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using PerformanceMonitor.Collectors;
+using PerformanceMonitor.Common;
+using PerformanceMonitor.Darling.Viewer;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// Decision-table pins for the shared <see cref="CollectorHealthClassifier"/> (#1573) - the one banding
+/// the Lite grid, this viewer's grid + Overview "collectors failing" count, and the service's
+/// get_collection_health / web fleet reader all delegate to. This SAME table is pinned identically in
+/// Lite.Tests so the two SKUs' collector-health banding cannot drift.
+///
+/// <para>
+/// The load-bearing case is the motivating bug: index_object_stats is a DAILY collector
+/// (<see cref="CollectorScheduleDefaults"/> cadence 1440 min) that succeeds every run, yet the old flat
+/// 4h-STALE / 24h-FAILING thresholds flagged it STALE past 4h and FAILING past 24h. The thresholds are
+/// now relative to each collector's cadence (FAILING = max(24, 2 x freqHours); STALE = max(4, 1.5 x
+/// freqHours)), with floors that keep every FREQUENT collector byte-for-byte identical.
+/// </para>
+/// </summary>
+public sealed class CollectorHealthClassifierTests
+{
+    private const int OneMinute = 1;      // wait_stats, cpu_utilization, etc. - the fastest cadence.
+    private const int Hourly = 60;        // database_size_stats - still floor-bound (1.5h/2h < 4h/24h).
+    private const int Daily = 1440;       // index_object_stats - the collector that relaxes.
+    private const int OnLoadFreq = 0;     // config snapshots run on connect, not on the loop.
+
+    [Theory]
+    /* NEVER_RUN wins first, whatever else is set (including on-load). */
+    [InlineData(0, 0, 0, 0, 999, OneMinute, false, CollectorHealthClassifier.NeverRun)]
+    [InlineData(0, 0, 0, 0, 0, OnLoadFreq, true, CollectorHealthClassifier.NeverRun)]
+
+    /* NO_PERMISSIONS (only permission denials) is checked before the on-load branch. */
+    [InlineData(5, 0, 0, 5, 999, OneMinute, false, CollectorHealthClassifier.NoPermissions)]
+    [InlineData(5, 0, 0, 5, 999, OnLoadFreq, true, CollectorHealthClassifier.NoPermissions)]
+
+    /* On-load collectors are staleness-exempt: banded by failure rate only, never STALE/FAILING. */
+    [InlineData(10, 10, 0, 0, 500, OnLoadFreq, true, CollectorHealthClassifier.Healthy)]
+    [InlineData(10, 7, 3, 0, 500, OnLoadFreq, true, CollectorHealthClassifier.Warning)]   // 30% > 20%
+
+    /* Frequent (1-min) collector - the floors mean these are IDENTICAL to the old flat thresholds. */
+    [InlineData(10, 10, 0, 0, 2, OneMinute, false, CollectorHealthClassifier.Healthy)]    // < 4h
+    [InlineData(10, 7, 3, 0, 2, OneMinute, false, CollectorHealthClassifier.Warning)]     // recent, 30% fail
+    [InlineData(10, 10, 0, 0, 5, OneMinute, false, CollectorHealthClassifier.Stale)]      // > 4h, < 24h
+    [InlineData(10, 10, 0, 0, 25, OneMinute, false, CollectorHealthClassifier.Failing)]   // > 24h
+    [InlineData(5, 0, 5, 0, 999, OneMinute, false, CollectorHealthClassifier.Failing)]    // ran, never a success (999 sentinel)
+
+    /* Hourly (60-min) collector - 1.5h/2h are both under the 4h/24h floors, so still floor-bound. */
+    [InlineData(10, 10, 0, 0, 5, Hourly, false, CollectorHealthClassifier.Stale)]         // > 4h floor
+    [InlineData(10, 10, 0, 0, 25, Hourly, false, CollectorHealthClassifier.Failing)]      // > 24h floor
+
+    /* Daily (1440-min) collector - the FIX. STALE line 36h, FAILING line 48h. */
+    [InlineData(30, 30, 0, 0, 27, Daily, false, CollectorHealthClassifier.Healthy)]       // THE BUG: was FAILING, now HEALTHY
+    [InlineData(30, 30, 0, 0, 35, Daily, false, CollectorHealthClassifier.Healthy)]       // still under the 36h stale line
+    [InlineData(30, 30, 0, 0, 37, Daily, false, CollectorHealthClassifier.Stale)]         // > 36h, < 48h
+    [InlineData(30, 30, 0, 0, 47, Daily, false, CollectorHealthClassifier.Stale)]         // still stale, not failing
+    [InlineData(30, 30, 0, 0, 49, Daily, false, CollectorHealthClassifier.Failing)]       // > 48h
+    [InlineData(10, 7, 3, 0, 1, Daily, false, CollectorHealthClassifier.Warning)]         // recent, 30% fail -> WARNING not STALE
+    public void Classify_BandsTheDecisionTable(
+        long totalRuns, long successCount, long errorCount, long permissionDeniedCount,
+        double hoursSinceLastSuccess, int frequencyMinutes, bool isOnLoad, string expected)
+    {
+        Assert.Equal(expected, CollectorHealthClassifier.Classify(
+            totalRuns, successCount, errorCount, permissionDeniedCount,
+            hoursSinceLastSuccess, frequencyMinutes, isOnLoad));
+    }
+
+    [Theory]
+    [InlineData(OneMinute, 24.0)]     // floor
+    [InlineData(Hourly, 24.0)]        // 2 x 1h = 2h < 24h floor
+    [InlineData(720, 24.0)]           // 2 x 12h = 24h, exactly the floor
+    [InlineData(Daily, 48.0)]         // 2 x 24h = 48h relaxes past the floor
+    public void FailingThresholdHours_IsMaxOfFloorAndTwiceCadence(int frequencyMinutes, double expected) =>
+        Assert.Equal(expected, CollectorHealthClassifier.FailingThresholdHours(frequencyMinutes), 3);
+
+    [Theory]
+    [InlineData(OneMinute, 4.0)]      // floor
+    [InlineData(Hourly, 4.0)]         // 1.5 x 1h = 1.5h < 4h floor
+    [InlineData(Daily, 36.0)]         // 1.5 x 24h = 36h relaxes past the floor
+    public void StaleThresholdHours_IsMaxOfFloorAndOnePointFiveCadence(int frequencyMinutes, double expected) =>
+        Assert.Equal(expected, CollectorHealthClassifier.StaleThresholdHours(frequencyMinutes), 3);
+
+    [Theory]
+    [InlineData("server_config", true)]
+    [InlineData("database_config", true)]
+    [InlineData("database_scoped_config", true)]
+    [InlineData("trace_flags", true)]
+    [InlineData("server_properties", true)]
+    [InlineData("SERVER_CONFIG", true)]        // case-insensitive
+    [InlineData("wait_stats", false)]
+    [InlineData("index_object_stats", false)]
+    public void IsOnLoadCollector_MatchesTheFreqZeroSet(string collectorName, bool expected) =>
+        Assert.Equal(expected, CollectorHealthClassifier.IsOnLoadCollector(collectorName));
+
+    /// <summary>The on-load set is exactly the FrequencyMinutes == 0 entries in the shared schedule table -
+    /// the invariant that lets a caller resolve cadence and on-load-ness from the same source.</summary>
+    [Fact]
+    public void OnLoadSet_EqualsTheFreqZeroScheduleEntries()
+    {
+        foreach (var (name, entry) in CollectorScheduleDefaults.All)
+        {
+            Assert.Equal(entry.FrequencyMinutes == 0, CollectorHealthClassifier.IsOnLoadCollector(name));
+        }
+    }
+
+    /* --- The bug, reproduced through the viewer's real CollectorHealthRow (it resolves the cadence from
+       CollectorScheduleDefaults by name). --- */
+
+    [Fact]
+    public void IndexObjectStats_DefaultCadence_IsDaily()
+    {
+        // Pins the exact input that produced the field false-positive: a 1440-min (daily) collector.
+        Assert.Equal(1440, CollectorScheduleDefaults.All["index_object_stats"].FrequencyMinutes);
+    }
+
+    [Fact]
+    public void CollectorHealthRow_IndexObjectStats_At27Hours_IsHealthyNotFailing()
+    {
+        /* The motivating bug: index_object_stats succeeds daily; 27h since last success is well within a
+           daily cadence. Under the old flat 24h-FAILING threshold this read FAILING (and inflated the
+           Overview "collectors failing" count); it now reads HEALTHY. */
+        var row = new CollectorHealthRow
+        {
+            CollectorName = "index_object_stats",
+            TotalRuns = 7,
+            SuccessCount = 7,
+            LastSuccessTime = DateTime.UtcNow.AddHours(-27),
+        };
+        Assert.Equal("HEALTHY", row.HealthStatus);
+    }
+
+    [Fact]
+    public void CollectorHealthRow_IndexObjectStats_At37Hours_IsStale()
+    {
+        var row = new CollectorHealthRow
+        {
+            CollectorName = "index_object_stats",
+            TotalRuns = 7,
+            SuccessCount = 7,
+            LastSuccessTime = DateTime.UtcNow.AddHours(-37),
+        };
+        Assert.Equal("STALE", row.HealthStatus);
+    }
+
+    [Fact]
+    public void CollectorHealthRow_IndexObjectStats_At49Hours_IsFailing()
+    {
+        var row = new CollectorHealthRow
+        {
+            CollectorName = "index_object_stats",
+            TotalRuns = 7,
+            SuccessCount = 7,
+            LastSuccessTime = DateTime.UtcNow.AddHours(-49),
+        };
+        Assert.Equal("FAILING", row.HealthStatus);
+    }
+
+    [Fact]
+    public void CollectorHealthRow_FrequentCollector_BandsUnchanged()
+    {
+        /* A 1-min collector is unaffected by the relative thresholds (the floors dominate): 30h -> FAILING,
+           exactly as before the #1573 change. */
+        var row = new CollectorHealthRow
+        {
+            CollectorName = "wait_stats",
+            TotalRuns = 100,
+            SuccessCount = 100,
+            LastSuccessTime = DateTime.UtcNow.AddHours(-30),
+        };
+        Assert.Equal("FAILING", row.HealthStatus);
+    }
+}