analyze_server: an empty analysis window is "unavailable", not an all-clear (#3524)

Darling/Darling.Tests/AnalyzeWindowEmptyLivePostgresTests.cs

@@ -0,0 +1,149 @@
+/*
+ * 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 System.Text.Json;
+using System.Threading;
+using System.Threading.Tasks;
+using Npgsql;
+using PerformanceMonitor.Collectors;
+using PerformanceMonitor.Common;
+using PerformanceMonitor.Darling.Analysis;
+using PerformanceMonitor.Darling.Service.Mcp;
+using PerformanceMonitor.Darling.Storage;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// Gated (DARLING_TEST_PG) proof of #3524: <c>analyze_server</c> must not answer "all metrics are
+/// within normal ranges" when the analysis window collected NOTHING. The pipeline's data-span gate
+/// measures LIFETIME history, so a server whose collection died (broken credential, unreachable
+/// target) still passes it — and the zero-facts pass used to return a bare <c>[]</c> that the tool
+/// rendered as a true-negative all-clear. Both sides of the distinction are asserted, on the REAL
+/// 24h gate rather than a zeroed one, because the bug lives precisely in the gap between "enough
+/// history" and "an empty window".
+/// </summary>
+[Collection("live-postgres")]
+public sealed class AnalyzeWindowEmptyLivePostgresTests
+{
+    private const string ServerName = "darling-analyze-window-empty-e2e";
+    private const string StaleWait = "WE3524_STALE_WAIT";
+    private const string BenignWait = "WE3524_BENIGN_WAIT";
+    private static readonly int ServerId = ServerIdHelper.GetDeterministicHashCode(ServerName);
+
+    private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+
+    [Fact]
+    public async Task ADeadCollectorWindow_IsUnavailable_AndAWindowWithFactsKeepsTheAllClear()
+    {
+        var cs = ConnectionString;
+        Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the live window-empty analysis test.");
+
+        var ct = TestContext.Current.CancellationToken;
+        using var connection = new NpgsqlConnection(cs);
+        await connection.OpenAsync(ct);
+        await PgMigrations.MigrateAsync(connection, ct);
+        await DeleteRowsAsync(connection, ct);
+
+        await using var postgres = NpgsqlDataSource.Create(cs!);
+
+        var bodySucceeded = false;
+        try
+        {
+            await RegisterServerAsync(connection, ct);
+            var service = new DarlingAnalysisService(postgres);
+
+            /* ── the dead-collector shape: 25 hours of history (the real 24h gate PASSES) whose
+               newest row is five hours old, so the tool's default 4h window holds nothing. */
+            await PlantWaitAsync(connection, Naive(DateTime.UtcNow.AddHours(-30)), StaleWait, 60_000L, ct);
+            await PlantWaitAsync(connection, Naive(DateTime.UtcNow.AddHours(-5)), StaleWait, 60_000L, ct);
+
+            var deadWindow = await DarlingMcpTools.AnalyzeServer(service, postgres, ServerName);
+            using (var doc = JsonDocument.Parse(deadWindow))
+            {
+                Assert.Equal("unavailable", doc.RootElement.GetProperty("status").GetString());
+
+                /* The #2506 persistence disclosure survives the new envelope — an anchored
+                   empty-window run still owes the caller that context, and this unanchored one
+                   reports the ordinary answer. */
+                Assert.True(doc.RootElement.GetProperty("hints").GetProperty("persisted").GetBoolean());
+            }
+            Assert.Contains("get_collection_health", deadWindow, StringComparison.Ordinal);
+            Assert.Contains("NOT an all-clear", deadWindow, StringComparison.Ordinal);
+            Assert.DoesNotContain("within normal ranges", deadWindow, StringComparison.Ordinal);
+
+            /* The service says WHICH kind of nothing this was: the window, not the lifetime span. */
+            Assert.NotNull(service.WindowEmptyMessage);
+            Assert.Null(service.InsufficientDataMessage);
+
+            /* ── the control, and the reason the fix is a distinction rather than a rewording: the
+               SAME server with one benign wait INSIDE the window has facts to score, finds nothing
+               wrong, and keeps the genuine true-negative all-clear. */
+            await PlantWaitAsync(connection, Naive(DateTime.UtcNow.AddMinutes(-30)), BenignWait, 100L, ct);
+
+            var healthyWindow = await DarlingMcpTools.AnalyzeServer(service, postgres, ServerName);
+            using (var doc = JsonDocument.Parse(healthyWindow))
+            {
+                Assert.Equal("empty", doc.RootElement.GetProperty("status").GetString());
+            }
+            Assert.Contains("All metrics are within normal ranges", healthyWindow, StringComparison.Ordinal);
+            Assert.Null(service.WindowEmptyMessage);
+
+            bodySucceeded = true;
+        }
+        finally
+        {
+            await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) =>
+                await DeleteRowsAsync(cleanup, cleanupCt));
+        }
+    }
+
+    /// <summary>Naive-UTC, the Kind every timestamp column in this store is bound with.</summary>
+    private static DateTime Naive(DateTime value) => DateTime.SpecifyKind(value, DateTimeKind.Unspecified);
+
+    private static async Task RegisterServerAsync(NpgsqlConnection connection, CancellationToken ct)
+    {
+        using var command = new NpgsqlCommand(@"
+INSERT INTO servers (server_id, server_name, display_name, is_enabled, sql_major_version, created_date, modified_date)
+VALUES ($1, $2, $3, TRUE, 15, $4, $4)
+ON CONFLICT (server_id) DO UPDATE SET is_enabled = TRUE, sql_major_version = 15;", connection);
+        command.Parameters.AddWithValue(ServerId);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue(Naive(DateTime.UtcNow));
+        await command.ExecuteNonQueryAsync(ct);
+    }
+
+    private static async Task PlantWaitAsync(
+        NpgsqlConnection connection, DateTime at, string waitType, long waitMs, CancellationToken ct)
+    {
+        using var command = new NpgsqlCommand(@"
+INSERT INTO wait_stats
+    (collection_id, collection_time, server_id, server_name, wait_type, delta_waiting_tasks, delta_wait_time_ms)
+VALUES ($1, $2, $3, $4, $5, $6, $7)", connection);
+        command.Parameters.AddWithValue(CollectionIdGenerator.Next());
+        command.Parameters.AddWithValue(at);
+        command.Parameters.AddWithValue(ServerId);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue(waitType);
+        command.Parameters.AddWithValue(50L);
+        command.Parameters.AddWithValue(waitMs);
+        await command.ExecuteNonQueryAsync(ct);
+    }
+
+    private static async Task DeleteRowsAsync(NpgsqlConnection connection, CancellationToken ct)
+    {
+        using var cleanup = new NpgsqlCommand(
+            $"DELETE FROM wait_stats WHERE server_id = {ServerId}; " +
+            $"DELETE FROM analysis_findings WHERE server_id = {ServerId}; " +
+            $"DELETE FROM analysis_muted WHERE server_id = {ServerId}; " +
+            $"DELETE FROM servers WHERE server_id = {ServerId};", connection);
+        await cleanup.ExecuteNonQueryAsync(ct);
+    }
+}

Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs

@@ -133,6 +133,15 @@ public sealed class DarlingAnalysisService
     /// </summary>
     public string? InsufficientDataMessage { get; private set; }
 
+    /// <summary>
+    /// Set after AnalyzeAsync when the server PASSED the data-span gate but the analysis window itself
+    /// produced zero facts (#3524). Null otherwise. The gate measures TOTAL history, so a server whose
+    /// collection died still sails through it and lands on an empty window — which is a dead collector
+    /// or an unreachable target, not a healthy server. Callers must not render an empty findings list
+    /// as an all-clear while this is set; nothing was measured.
+    /// </summary>
+    public string? WindowEmptyMessage { get; private set; }
+
     /// <summary>
     /// How the last pass ended EARLY, or null when it ran through (#2430). Set inside the pass's own
     /// catch, so <see cref="AnalysisAbandonKind.None"/> here means a genuine fault: the pass reached the
@@ -216,6 +225,7 @@ public async Task<List<AnalysisFinding>> AnalyzeAsync(AnalysisContext context)
 
         IsAnalyzing = true;
         InsufficientDataMessage = null;
+        WindowEmptyMessage = null;
         EndedEarlyAs = null;
 
         try
@@ -257,6 +267,21 @@ cannot unwind from inside it — these boundary checks are what turn the token i
 
             if (facts.Count == 0)
             {
+                /* #3524: the span gate above passed on LIFETIME history, so an empty WINDOW here means
+                   collection stopped producing rows for it — not that the server is healthy. Say so,
+                   instead of returning a bare [] that reads exactly like "analyzed and found nothing". */
+                WindowEmptyMessage =
+                    $"No facts were collected in the analysis window " +
+                    $"({context.TimeRangeStart:yyyy-MM-dd HH:mm} to {context.TimeRangeEnd:yyyy-MM-dd HH:mm} UTC) " +
+                    $"even though this server has {dataSpanHours:F1} hours of total collected history. " +
+                    "Collection appears to have stopped or broken for this window, so nothing was measured " +
+                    "— this is NOT an all-clear.";
+
+                _logger?.LogWarning(
+                    "[DarlingAnalysisService] No facts in the analysis window for {Server} ({Start} to {End}) " +
+                    "despite {Span:F1}h of total history — collection may be down",
+                    context.ServerName, context.TimeRangeStart, context.TimeRangeEnd, dataSpanHours);
+
                 LastAnalysisTime = DateTime.UtcNow;
                 return [];
             }

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs

@@ -80,10 +80,33 @@ make every ordinary run look anchored to the engine — which is exactly the set
                 ? null
                 : "as_of was supplied, so this analysis ran over a PAST window and is exploratory: the findings below are complete but were NOT written to the store. A finding row carries the time the analysis RAN, and the reads that consume those rows (get_analysis_findings, the viewer's Recommendations tab) treat the newest analysis_time as this server's CURRENT state — so persisting a backdated run would make last week's findings today's headline and would inflate the occurrence stats of any live incident sharing a story path. Re-run without as_of to analyze and persist the present.";
 
+            if (analysisService.WindowEmptyMessage != null)
+            {
+                /* #3524: zero facts in the window is a DEAD-COLLECTOR shape, not a clean bill of
+                   health — the data-span gate passes on lifetime history, so a server whose
+                   collection broke last week lands here, and the "empty" all-clear below would tell
+                   the caller in prose that all metrics are normal when nothing was measured at all.
+                   Same miss vocabulary as get_analysis_facts' zero-facts case; same hints block as
+                   the all-clear, because an anchored empty-window run still owes the caller the
+                   persistence disclosure. */
+                return McpHelpers.Status(
+                    "unavailable",
+                    analysisService.WindowEmptyMessage +
+                    " Check get_collection_health to see when collectors last succeeded and why they stopped.",
+                    new
+                    {
+                        analysis_time = analysisService.LastAnalysisTime?.ToString("o"),
+                        persisted = anchor is null,
+                        persistence_note = persistenceNote
+                    });
+            }
+
             if (findings.Count == 0)
             {
                 /* A successful analysis that found nothing wrong: a true negative ("all clear"),
-                   surfaced with the shared miss vocabulary so callers branch on it uniformly. */
+                   surfaced with the shared miss vocabulary so callers branch on it uniformly. Facts
+                   WERE collected and scored this time — the window-collected-nothing case returned
+                   above as unavailable instead (#3524). */
                 return McpHelpers.Status(
                     "empty",
                     "No significant findings. All metrics are within normal ranges.",

Lite.Tests/AnalysisWindowEmptyTests.cs

@@ -0,0 +1,148 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor Lite.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using System.IO;
+using System.Text.Json;
+using System.Threading.Tasks;
+using DuckDB.NET.Data;
+using PerformanceMonitorLite.Analysis;
+using PerformanceMonitorLite.Database;
+using PerformanceMonitorLite.Mcp;
+using PerformanceMonitorLite.Models;
+using PerformanceMonitorLite.Services;
+using Xunit;
+
+namespace PerformanceMonitorLite.Tests;
+
+/// <summary>
+/// #3524: <c>analyze_server</c> must not answer "all metrics are within normal ranges" when the
+/// analysis window collected NOTHING. The pipeline's data-span gate measures LIFETIME history, so a
+/// server whose collection died (broken credential, unreachable target) still passes it — and the
+/// zero-facts pass used to return a bare <c>[]</c> that the tool rendered as a true-negative
+/// all-clear. Both sides of the distinction are asserted, on the REAL 24h gate rather than a zeroed
+/// one, because the bug lives precisely in the gap between "enough history" and "an empty window".
+/// </summary>
+public sealed class AnalysisWindowEmptyTests : IClassFixture<SharedDuckDbFixture>, IDisposable
+{
+    private const string StaleWait = "WE3524_STALE_WAIT";
+    private const string BenignWait = "WE3524_BENIGN_WAIT";
+
+    private readonly string _tempDir;
+    private readonly DuckDbInitializer _duckDb;
+    private readonly ServerManager _serverManager;
+    private readonly int _serverId;
+    private long _nextId = -3_524_000;
+    private DuckDBConnection? _seedConn;
+
+    public AnalysisWindowEmptyTests(SharedDuckDbFixture fixture)
+    {
+        fixture.ResetData();
+        _duckDb = fixture.DuckDb;
+
+        _tempDir = Path.Combine(Path.GetTempPath(), "AnalysisWindowEmptyTests_" + Guid.NewGuid().ToString("N")[..8]);
+        var configDir = Path.Combine(_tempDir, "config");
+        Directory.CreateDirectory(configDir);
+
+        /* Windows auth so AddServer never touches the credential store — no DPAPI side effects. */
+        _serverManager = new ServerManager(configDir);
+        var server = new ServerConnection { ServerName = "TestServer", DisplayName = "TestServer" };
+        _serverManager.AddServer(server);
+
+        _serverId = RemoteCollectorService.GetDeterministicHashCode(
+            RemoteCollectorService.GetServerNameForStorage(server));
+    }
+
+    public void Dispose()
+    {
+        _seedConn?.Dispose();
+        try { if (Directory.Exists(_tempDir)) Directory.Delete(_tempDir, recursive: true); }
+        catch { /* best-effort cleanup */ }
+    }
+
+    /// <summary>
+    /// The dead-collector shape: 25 hours of history (the real 24h gate PASSES) whose newest row is
+    /// five hours old, so the tool's default 4h window holds nothing. That must come back as
+    /// <c>unavailable</c> with the pointer at collection health — never as the all-clear prose an
+    /// operator (or their agent) would read as "this server is fine".
+    /// </summary>
+    [Fact]
+    public async Task ADeadCollectorWindow_IsUnavailable_NotAnAllClear()
+    {
+        await PlantWaitAsync(DateTime.UtcNow.AddHours(-30), StaleWait, 60_000L);
+        await PlantWaitAsync(DateTime.UtcNow.AddHours(-5), StaleWait, 60_000L);
+
+        var service = new AnalysisService(_duckDb);
+        var result = await McpAnalysisTools.AnalyzeServer(service, _serverManager);
+
+        using var doc = JsonDocument.Parse(result);
+        Assert.Equal("unavailable", doc.RootElement.GetProperty("status").GetString());
+        Assert.Contains("get_collection_health", result, StringComparison.Ordinal);
+        Assert.Contains("NOT an all-clear", result, StringComparison.Ordinal);
+        Assert.DoesNotContain("within normal ranges", result, StringComparison.Ordinal);
+
+        /* The service says WHICH kind of nothing this was: the window, not the lifetime span. */
+        Assert.NotNull(service.WindowEmptyMessage);
+        Assert.Null(service.InsufficientDataMessage);
+
+        /* The #2506 persistence disclosure survives the new envelope — an anchored empty-window run
+           still owes the caller that context, and this unanchored one reports the ordinary answer. */
+        Assert.True(doc.RootElement.GetProperty("hints").GetProperty("persisted").GetBoolean());
+    }
+
+    /// <summary>
+    /// The control, and the reason the fix is a distinction rather than a rewording: the SAME server
+    /// with one benign wait INSIDE the window has facts to score, finds nothing wrong, and keeps the
+    /// genuine true-negative all-clear.
+    /// </summary>
+    [Fact]
+    public async Task AWindowWithFactsButNoFindings_KeepsTheAllClear()
+    {
+        await PlantWaitAsync(DateTime.UtcNow.AddHours(-30), StaleWait, 60_000L);
+        await PlantWaitAsync(DateTime.UtcNow.AddHours(-5), StaleWait, 60_000L);
+        await PlantWaitAsync(DateTime.UtcNow.AddMinutes(-30), BenignWait, 100L);
+
+        var service = new AnalysisService(_duckDb);
+        var result = await McpAnalysisTools.AnalyzeServer(service, _serverManager);
+
+        using var doc = JsonDocument.Parse(result);
+        Assert.Equal("empty", doc.RootElement.GetProperty("status").GetString());
+        Assert.Contains("All metrics are within normal ranges", result, StringComparison.Ordinal);
+        Assert.Null(service.WindowEmptyMessage);
+    }
+
+    private async Task<DuckDBConnection> SeedConnectionAsync()
+    {
+        if (_seedConn is null)
+        {
+            _seedConn = _duckDb.CreateConnection();
+            await _seedConn.OpenAsync();
+        }
+        return _seedConn;
+    }
+
+    private async Task PlantWaitAsync(DateTime at, string waitType, long deltaWaitMs)
+    {
+        using var readLock = _duckDb.AcquireReadLock();
+        var conn = await SeedConnectionAsync();
+        using var cmd = conn.CreateCommand();
+        cmd.CommandText = @"
+INSERT INTO wait_stats
+    (collection_id, collection_time, server_id, server_name, wait_type,
+     waiting_tasks_count, wait_time_ms, signal_wait_time_ms,
+     delta_waiting_tasks, delta_wait_time_ms, delta_signal_wait_time_ms)
+VALUES ($1, $2, $3, 'TestServer', $4, 50, $5, 0, 50, $5, 0)";
+        void P(object v) => cmd.Parameters.Add(new DuckDBParameter { Value = v });
+        P(_nextId--);
+        P(at);
+        P(_serverId);
+        P(waitType);
+        P(deltaWaitMs);
+        await cmd.ExecuteNonQueryAsync();
+    }
+}