Report the window get_query_store_top actually served (#2364)

CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - **Server Inventory's `Last Updated` was a config-snapshot time wearing a freshness label** ([#2359]) - `server_properties` ships with `FrequencyMinutes 0`, which the schedule table defines as "collect once on server load only", so the column showed the last SERVICE START and every actively-monitored server on a long-running install looked days stale. Renamed to `Inventory As Of`, and a new `Last Collected` column carries the real heartbeat from `v_collection_log` - the same signal `list_servers` uses. Darling only: Lite reads live and stamps the value at read time, so its column was never misleading. Reported by @ghauan, who correctly pushed back when the first fix did not explain their case.
+- **get_query_store_top reported a window it could not serve** ([#2364]) - it reads raw `query_store_stats`, which is dropped at 4 days on a store with the rollups armed, so a 30-day request returned at most four days with `hours_back: 720` echoed back unchanged. It cannot be routed to a rollup the way #2353 was: the corrected CAGGs carry no `query_id` or `plan_id`, and plan identity is the whole point of the tool. So the response now carries `effective_start`, `effective_hours_back`, `truncated` and a note naming why, and the empty path no longer blames Query Store configuration for a window it never read. Found while scoping #2357 with @carlei1978.
 - **Server Inventory's Last Updated looked broken on decommissioned servers** ([#2359]) - the grid lists every REGISTERED server and never read `servers.is_enabled`, so a server removed from monitoring kept the `collection_time` it had when collection stopped. Accurate, and read by everyone as a stale freshness column. Measured on a 61-server fleet the split was total: 19 disabled servers last collected within five minutes of each other on the day they were removed, 42 enabled servers all fresh within a minute. A new Monitoring column now says Active or Stopped. The disabled rows are KEPT, not filtered - this is the FinOps tab and a decommissioned server's cost history is what it exists to show. Reported by @ghauan.
 - **Extended-length paths no longer slip past the install-location guards** ([#2348]) - `\\?\UNC\server\share` is the long spelling of a REAL share and `\\?\C:\Users\bob` of a REAL profile, but the wholesale `\\?\` exclusion waved both through undiagnosed, because skipping a check is not the same as passing it. Both implementations now strip the prefix BEFORE classifying, so the long spelling gets the same verdict as the short one, and `\\?\C:\PerformanceMonitorDarling` - an ordinary local root written the long way - is still correctly left alone. The shared decision table and the cross-language parity test hold the service and `install-darling.ps1` to it together.
 - **get_query_trend silently truncated any window past 4 days** ([#2353]) - it read only the raw `query_stats` table, whose raw tier is dropped at 4 days independently of the collector's much longer advertised retention, while echoing `hours_back: 168` back unchanged. Worse than a short array: the empty path asserted "No history found ... within the last 168 hours" over a span it never read, which for a query whose history had aged out is a false statement rather than an incomplete one. It now routes to the hourly rollup by the age of the window's OLDEST point measured from wall clock, and reports `source`, `effective_hours_back`, `bucket` and `truncated` so the response describes the data instead of the request. Columns the rollup does not carry come back NULL, never zero - on an aggregate row a zero would read as "none observed". Reported by @carlei1978.
@@ -2822,6 +2823,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2340]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2340
 [#2344]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2344
 [#2362]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2362
+[#2364]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2364
 [#2347]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2347
 [#2359]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2359
 [#2348]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2348

Darling/Darling.Tests/QueryStoreTopWindowTests.cs

@@ -0,0 +1,135 @@
+/*
+ * 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.IO;
+using System.Runtime.CompilerServices;
+using PerformanceMonitor.Darling.Service.Mcp;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #2364: <c>get_query_store_top</c> reports the window it actually served.
+///
+/// <para><b>The defect.</b> It reads raw <c>query_store_stats</c>, and on a store with the rollups armed that
+/// table is dropped at four days — measured: a 4d 13h span over 17,162,516 rows under a
+/// <c>policy_retention {"drop_after": "4 days"}</c>. A caller asking for 30 days therefore got at most four,
+/// with <c>hours_back: 720</c> echoed back unchanged and nothing marking the difference.</para>
+///
+/// <para><b>Why this could not be fixed the way #2353 was.</b> That one routed to the hourly rollup. Here there
+/// is no route: <c>QueryStoreTopSql</c> groups by <c>database_name, query_id, plan_id, query_hash,
+/// replica_role</c> and the corrected CAGG groups by <c>database_name, module_name, query_hash</c> — no
+/// <c>query_id</c>, no <c>plan_id</c>. Plan identity is the entire purpose of this tool, and a rollup grained to
+/// it would approach the size of the raw data. So the fix is honesty, not routing.</para>
+/// </summary>
+public class QueryStoreTopWindowTests
+{
+    private static string ReaderSource => ReadRepoFile(Path.Combine(
+        "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingDataReader.cs"));
+
+    private static string ToolSource => ReadRepoFile(Path.Combine(
+        "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpDataTools.cs"));
+
+    /// <summary>
+    /// The floor probe is bounded on BOTH sides of the window. Bounding the partitioning column is what lets
+    /// TimescaleDB exclude chunks; an unbounded <c>MIN</c> would read the retention window to answer a question
+    /// about it.
+    /// </summary>
+    [Fact]
+    public void TheWindowFloorProbe_IsBoundedOnBothSides()
+    {
+        var sql = DarlingDataReader.QueryStoreWindowFloorSql;
+
+        Assert.Contains("MIN(collection_time)", sql, StringComparison.Ordinal);
+        Assert.Contains("FROM query_store_stats", sql, StringComparison.Ordinal);
+        Assert.Contains("collection_time >=", sql, StringComparison.Ordinal);
+        Assert.Contains("collection_time <=", sql, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// <b>The floor must come from a probe, never from the returned rows.</b> The result set is the top N by
+    /// COST, so its timestamps say nothing about how far back the read reached — the most expensive query in a
+    /// month may have run this morning. Deriving the window from the rows would produce a confident, wrong
+    /// answer, which is worse than the silence it replaces.
+    /// </summary>
+    [Fact]
+    public void TheEffectiveWindow_ComesFromTheProbe_NotTheRows()
+    {
+        Assert.Contains("GetQueryStoreWindowFloorAsync", ToolSource, StringComparison.Ordinal);
+
+        var at = ToolSource.IndexOf("var effectiveStart =", StringComparison.Ordinal);
+        Assert.True(at >= 0, "the tool no longer computes an effective window (#2364)");
+
+        var line = ToolSource[at..ToolSource.IndexOf(';', at)];
+        Assert.Contains("floor", line, StringComparison.Ordinal);
+
+        /* Not from the projection: rows are ordered by cost, and LastExecutionTime is a per-query fact. */
+        Assert.DoesNotContain("rows.Min(", ToolSource, StringComparison.Ordinal);
+        Assert.DoesNotContain("rows.Max(", ToolSource, StringComparison.Ordinal);
+    }
+
+    /// <summary>The payload describes the data, not just the request.</summary>
+    [Fact]
+    public void ThePayload_CarriesTheServedWindow()
+    {
+        Assert.Contains("effective_start", ToolSource, StringComparison.Ordinal);
+        Assert.Contains("effective_hours_back", ToolSource, StringComparison.Ordinal);
+        Assert.Contains("truncated", ToolSource, StringComparison.Ordinal);
+
+        /* hours_back is still echoed — the caller needs to see what it asked for beside what it got. */
+        Assert.Contains("hours_back,", ToolSource, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// The empty path must not assert absence over a span it never read. The old message said Query Store "may
+    /// not be enabled", which for a window reaching past raw retention is a confident wrong diagnosis — an agent
+    /// acts on it by going to look at a Query Store configuration that is fine.
+    /// </summary>
+    [Fact]
+    public void TheEmptyPath_NamesTheWindowSearched_AndDoesNotOnlyBlameConfiguration()
+    {
+        var at = ToolSource.IndexOf("No Query Store rows for this server", StringComparison.Ordinal);
+        Assert.True(at >= 0, "the empty message no longer names the window searched (#2364)");
+
+        var message = ToolSource[at..Math.Min(ToolSource.Length, at + 700)];
+
+        Assert.Contains("hours_back", message, StringComparison.Ordinal);
+        Assert.Contains("4 days", message, StringComparison.Ordinal);
+        Assert.Contains("shorter window", message, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// The tool still reads RAW, and that is deliberate rather than an oversight — pinned so a well-meaning
+    /// "route it to the rollup like #2353" edit has to confront why it cannot.
+    /// </summary>
+    [Fact]
+    public void TheTool_StillReadsRaw_BecauseTheRollupCannotCarryPlanIdentity()
+    {
+        Assert.Contains("FROM query_store_stats", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal);
+        Assert.DoesNotContain("query_store_stats_corrected", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal);
+        Assert.DoesNotContain("query_store_stats_hourly", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal);
+
+        /* plan_id is the reason: it is a grouping key here and absent from every rollup. */
+        Assert.Contains("plan_id", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal);
+    }
+
+    private static string ReadRepoFile(string relative, [CallerFilePath] string thisFile = "")
+    {
+        for (var dir = new DirectoryInfo(Path.GetDirectoryName(thisFile)!); dir is not null; dir = dir.Parent)
+        {
+            var candidate = Path.Combine(dir.FullName, relative);
+            if (File.Exists(candidate))
+            {
+                return File.ReadAllText(candidate);
+            }
+        }
+
+        throw new FileNotFoundException($"Could not locate {relative} walking up from {thisFile}");
+    }
+}

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs

@@ -879,6 +879,41 @@ public static async Task<List<TopProcedureRow>> GetTopProceduresByCpuAsync(
 
     /* ─────────────────────────── query store ─────────────────────────── */
 
+    /// <summary>
+    /// The oldest <c>collection_time</c> this server actually has inside the requested window (#2364).
+    ///
+    /// <para><b>Why a separate probe rather than reading the returned rows.</b> <see cref="QueryStoreTopSql"/>
+    /// returns the top N by COST, not by time, so the timestamps on those rows say nothing about how far back
+    /// the window reaches — the most expensive query in a month might have run this morning. The window floor is
+    /// a property of the tier, not of the result set, and has to be asked for separately.</para>
+    ///
+    /// <para>Bounded on both sides, so it prunes chunks and answers from an ordered scan that stops at the first
+    /// row rather than reading the window. $1 server_id, $2/$3 window (naive UTC).</para>
+    /// </summary>
+    public const string QueryStoreWindowFloorSql = """
+        SELECT MIN(collection_time)
+        FROM query_store_stats
+        WHERE server_id = $1
+        AND   collection_time >= $2
+        AND   collection_time <= $3
+        """;
+
+    /// <summary>
+    /// Reads <see cref="QueryStoreWindowFloorSql"/>. Null when the window holds nothing at all, which the caller
+    /// reports as "nothing was read" rather than as an absence of activity.
+    /// </summary>
+    public static async Task<DateTime?> GetQueryStoreWindowFloorAsync(
+        NpgsqlDataSource postgres, int serverId, DateTime startUtc, DateTime endUtc,
+        CancellationToken cancellationToken = default)
+    {
+        await using var command = postgres.CreateCommand(QueryStoreWindowFloorSql);
+        DarlingMcpReadParameters.AddInt(command, serverId);
+        DarlingMcpReadParameters.AddTimestamp(command, startUtc);
+        DarlingMcpReadParameters.AddTimestamp(command, endUtc);
+        var value = await command.ExecuteScalarAsync(cancellationToken);
+        return value is DateTime dt ? dt : null;
+    }
+
     /// <summary>
     /// Top Query Store groups over the window — a focused projection of the viewer's
     /// <c>QueryStoreTopSql</c> (the columns Lite's get_query_store_top returns): group by

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs

@@ -663,9 +663,27 @@ public static async Task<string> GetQueryStoreTop(
         try
         {
             var now = DateTime.UtcNow;
-            var rows = await DarlingDataReader.GetQueryStoreTopAsync(postgres, resolved.ServerId, now.AddHours(-hours_back), now, top, database_name);
+            var requestedStart = now.AddHours(-hours_back);
+            var rows = await DarlingDataReader.GetQueryStoreTopAsync(postgres, resolved.ServerId, requestedStart, now, top, database_name);
+
+            /* #2364: what the window ACTUALLY holds. The rows above are the top N by COST, so their timestamps
+               say nothing about how far back the read reached -- the most expensive query in a month may have
+               run this morning. raw query_store_stats is dropped at 4 days on a store with the rollups armed,
+               and this tool has no rollup to fall back to (the corrected CAGGs carry no query_id or plan_id,
+               and plan identity is the whole point of this tool). So the honest move is to report the window
+               that was served rather than echo the one that was asked for. */
+            var floor = await DarlingDataReader.GetQueryStoreWindowFloorAsync(postgres, resolved.ServerId, requestedStart, now);
+            var effectiveStart = floor ?? requestedStart;
+            var truncated = floor is DateTime f && f > requestedStart.AddMinutes(90);
+
             if (rows.Count == 0)
-                return McpHelpers.Status("unavailable", "No Query Store data available. Query Store may not be enabled on target databases.");
+                return McpHelpers.Status(
+                    "unavailable",
+                    $"No Query Store rows for this server in the {hours_back}-hour window searched. Query Store " +
+                    "may not be enabled on the target databases -- or the window reaches past what the raw tier " +
+                    "retains (query_store_stats is dropped at 4 days when the rollups are armed), in which case " +
+                    "nothing was read for the older part of it. Try a shorter window before concluding the " +
+                    "queries did not run.");
 
             var result = rows.Select(r => new
             {
@@ -693,6 +711,16 @@ to tell them apart. NULL when the server did not attribute the row. */
             {
                 server = resolved.ServerName,
                 hours_back,
+                /* #2364: what was served, beside what was asked for. hours_back alone was a request echoed
+                   back as though it described the data. */
+                effective_start = effectiveStart.ToString("o"),
+                effective_hours_back = Math.Round((now - effectiveStart).TotalHours, 1),
+                truncated,
+                truncation_note = truncated
+                    ? "The window reaches further back than this server's raw query_store_stats retains, so the "
+                      + "older part of it was not read. This tool reads the raw tier only: the corrected rollups "
+                      + "carry no query_id or plan_id, and plan identity is what it exists to return."
+                    : null,
                 queries = result
             }, McpHelpers.JsonOptions);
         }