Report the window get_query_store_top actually served (#2364)

[erikdarlingdata]

Closes #2364. Found while scoping #2357 with @carlei1978, who supplied the tool and window that made it findable.

The defect

get_query_store_top reads raw query_store_stats, and on a store with the rollups armed that table is dropped at four days. Measured on the use2 fleet:

oldest                 newest                 span            rows
2026-08-15 00:04:25    2026-08-17 17:36:55    4 days 13:49    17,162,516
policy_retention  {"drop_after": "4 days", "hypertable_id": 29}

So a 30-day request returned at most four days, echoed hours_back: 720 back unchanged, and marked nothing.

Why this could not be fixed the way #2353 was

That one routed to the hourly rollup. Here there is no route, and the reason is structural:

QueryStoreTopSql   GROUP BY database_name, query_id, plan_id, query_hash, replica_role
corrected CAGG     GROUP BY server_id, server_name, database_name, module_name, query_hash, bucket

No query_id, no plan_id. Plan identity is the entire purpose of a tool whose own description is "Pull the full Query Store entry including plan_id and forced-plan history before considering a force", and a rollup grained to plan identity would approach the size of the raw data.

So the fix is honesty, not routing — and a test pins that, so a well-meaning "route it like #2353" edit has to confront why it cannot.

The floor comes from a probe, never from the rows

This distinction is load-bearing. 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.

QueryStoreWindowFloorSql is a bounded MIN(collection_time), bounded on both sides so it prunes chunks and stops at the first row.

The empty path

It used to say Query Store "may not be enabled". For a window that simply reached past retention that is a confident wrong diagnosis, and an agent acts on it by going to inspect a configuration that is fine. It now names the window searched, the four-day raw horizon, and suggests a shorter window before concluding the queries did not run.

Tests

That the probe is bounded on both sides; that the effective window comes from the probe and not from rows.Min/rows.Max; that the payload carries effective_start, effective_hours_back and truncated while still echoing hours_back; that the empty message names the window and the horizon; and that the tool still reads raw, with plan_id present as the reason.

Verification

Builds clean, 0 warnings. My own doc-hygiene scan caught a stacked <summary> where the insertion had stranded QueryStoreTopSql's doc block — reattached to its own member rather than deleted, since the stranded block was the real documentation.

[claude]

truncated/truncation_note conflate two different causes of "the floor is later than requested": raw-tier retention having dropped the older rows, and there simply being no data that far back (server recently onboarded, or Query Store recently enabled on the source instance). The floor probe (MIN(collection_time) in the window) can't distinguish these — it just says "the earliest row I can see starts here" — but the note asserts one specific cause ("reaches further back than this server's raw query_store_stats retains") regardless of which is true.

That's the same "confident wrong diagnosis" problem this PR is fixing in the other direction: a server monitored for only 2 days that gets a 30-day request will report truncated: true with a note blaming the 4-day raw retention window, when actually no data ever existed before onboarding. Worth softening the wording to something cause-agnostic ("data prior to this point was not read — either it aged out of the raw tier, or collection had not started yet") unless there's a reliable way to tell the two apart (e.g. comparing against the server's first-collected timestamp).

[claude]

The empty-path message hardcodes "dropped at 4 days" as the raw retention horizon, but per the PR description that's a policy_retention {"drop_after": "4 days"} value measured on one fleet (use2), not a constant enforced anywhere in this code. If a deployment's TimescaleDB retention policy is configured differently (or changes later), this diagnostic text — read by an agent deciding whether to keep digging — becomes actively wrong. Note the success-path truncation_note a few lines below avoids this by not naming a specific duration; this message should probably do the same, or derive the figure from timescaledb_information.jobs/policy_retention instead of stating it as fact.

[claude]

This re-derives the (server_id, start, end) triple that DarlingMcpReadParameters.AddWindow already binds — and GetQueryStoreTopAsync right below uses AddWindow(command, serverId, startUtc, endUtc) for the exact same three parameters against the same table. Per the helper's own doc comment, it was "lifted into one place so ... readers bind identically without re-declaring them per file" — this call site re-declares them. Suggest:

Suggested change

	await using var command = postgres.CreateCommand(QueryStoreWindowFloorSql);
	DarlingMcpReadParameters.AddInt(command, serverId);
	DarlingMcpReadParameters.AddTimestamp(command, startUtc);
	DarlingMcpReadParameters.AddTimestamp(command, endUtc);
	AddWindow(command, serverId, startUtc, endUtc);

[claude]

Correction to my suggestion above: this file has its own private AddWindow/AddInt/AddTimestamp at the bottom of the class (~line 1286), which is what the bare AddWindow(...) calls elsewhere in this file (e.g. GetQueryStoreTopAsync) actually resolve to — not DarlingMcpReadParameters.AddWindow. The two classes have identically-named/-shaped helpers by design (DarlingMcpReadParameters's doc comment: "the SAME bindings ... DarlingDataReader defines privately ... lifted into one place" for the newer domain readers). So the suggested fix (AddWindow(command, serverId, startUtc, endUtc)) is still correct and still removes the duplication — it just resolves to this file's own private helper, the same one its neighbor uses, rather than the external one. Explicitly qualifying DarlingMcpReadParameters.AddInt/AddTimestamp here works but is the odd one out next to every other reader in this file.

[claude]

Review summary

Solid, well-tested fix for the underlying defect (raw query_store_stats truncation on stores with rollups armed) — the reasoning in the PR description and the test file (QueryStoreTopWindowTests.cs) is sound, and the "floor from a probe, never from the rows" design is the right call given the top-N-by-cost shape of the result set.

Left three inline comments on DarlingMcpDataTools.cs/DarlingDataReader.cs:

• truncated/truncation_note attribute any shorter-than-requested floor to raw-tier retention specifically, but the probe can't actually distinguish "aged out of retention" from "never collected" (recent onboarding, QS recently enabled on the source server). That's the same failure mode — a confident wrong diagnosis — this PR fixes in the other direction.
• The empty-path message hardcodes "dropped at 4 days," which per the PR description is a policy_retention value measured on one fleet, not a constant guaranteed by any code here.
• GetQueryStoreWindowFloorAsync manually re-derives the (server_id, start, end) parameter triple instead of calling this file's existing private AddWindow helper, which its neighbor GetQueryStoreTopAsync already uses for the identical purpose.

One more, not inline-able as a single line: in GetQueryStoreTop (DarlingMcpDataTools.cs), the window-floor probe runs before the rows.Count == 0 check, so on the empty path (Query Store not enabled — plausibly the single most common outcome for a fleet-wide sweep) it does an extra Postgres round trip whose result is immediately discarded. Worth moving the floor probe after the empty-rows return, or short-circuiting when rows.Count == 0.

No Lite/Darling parity issue: Lite backfills Query Store history directly from the source server's own QS retention (see CollectionBackgroundService.RunQueryStoreBackfillIfDueAsync) rather than reading a TimescaleDB hypertable with a drop_after policy, so the specific defect this PR fixes doesn't apply there.