Fix query_store probe re-examination and query_stats/procedure_stats drain stalls

[erikdarlingdata]

What this fixes

query_store's probe re-examination (#3189, remeasured after #3216). #3216 widened the
liveness-touch guard's write interval but left the verdict SELECT unconditional — measured
again post-#3216 on the fleet's outlier server, the probe was still 47-52% of query_store's wall time,
essentially where #3189 left it. QueryStoreProbeCache (new) memoizes a resolved-and-current
verdict for the same TTL the touch guard already treats as an acceptable staleness bound, keyed
on (server, database, id, hash) so an in-place plan/text rewrite is always a cache miss.
Recurrence measured on the fleet's outlier server (two non-overlapping ~1h windows, two hours apart): 77.3% of
plan ids and 80.1% of query ids recur — the basis for expecting a real hit rate, not an
assumption.

query_stats/procedure_stats drain stalls. Both collectors read cached-plan XML via a
bare GetString, no per-row size bound. Root-caused via get_collector_stall_probes — 78 real
stalls on the fleet's outlier server over 2 days, every one showing a completely idle target (single-digit
runnable_tasks, zero work-queue, zero pending IO) at the moment of the stall — plus a comment
already in DarlingCollectorRunner.cs (#2880) noting unrelated collectors' ExecuteReaderAsync
calls degrade 3-152x in the same window. That's a process-wide GC pause from Large-Object-Heap
allocation, not target contention: three fresh 87-91s events carried the same ~27-28MB payload
as a 26-second run minutes earlier on the same server. QueryPlanXmlCaptureLimits (new) caps
captured plan XML at 512 KB per row, sized from the real distribution (heavy-tailed: top 5% of
rows carry 56% of bytes, median ~28KB) — ships NULL instead of an oversized plan. Measured end
to end on the fleet's outlier server, 4 trials each: unguarded 1.48-2.68s vs. guarded 1.12-1.40s, every guarded
trial faster than every unguarded one, ~36% wall-clock reduction.

Follow-ups filed, not built here

• [DESIGN] Eventually collect plan XML that QueryPlanXmlCaptureLimits caps, without reintroducing the batched-large-fetch problem it fixes #3392 — a plan over the 512 KB cap ships NULL forever, not just this cycle. Proposes a small
  backlog table plus a low-frequency, deliberately one-plan-at-a-time sweep (batching large
  fetches is the exact mechanism this PR fixes, so the sweep can't reintroduce it). De-risked by
  measuring that the fleet's outlier server's currently-over-cap plans have compile ages of 13.6 hours to 144.6
  days — not the volatile handles a naive reading of plan_handle would assume.
• query_store's plan fetch shares query_stats' pre-fix anti-pattern (uncapped GetString, no per-row size bound) — worth measuring #3393 — query_store's own plan fetch (FetchAndStorePlansAsync) reads plan XML the same
  uncapped way, with a larger aggregate budget (up to 64 MB) than query_stats's worst observed
  case. Live evidence checked (3 samples) is inconclusive, so this is filed for measurement, not
  as a confirmed incident.

Testing

Both new/changed collector-definition and cache-logic test files were executed for real via a
throwaway net10.0 console harness (Lite.Tests/Darling.Tests are Windows-only, WPF-referencing
projects and can't run on macOS) — 39/39 passing, including three pre-existing IL-scanning
structural pins (ProbeDenominatorTests, FetchStoreConnectionBorrowTests,
PlanFetchCarryoverKeyTests) that directly exercise the call sites this PR modifies. Full
solution builds clean (dotnet build PerformanceMonitor.sln -p:EnableWindowsTargeting=true).

🤖 Generated with Claude Code

https://claude.ai/code/session_01WTi8131EmrYtg3f3VVYCta

[claude]

Traced through this wiring (and the text-side twin at ~3775) against QueryStoreProbeCache/QueryStoreFetchProbe — the tuple-shape mismatch between references (PlanId/PlanHash) and SelectNeedingProbe's (Id, Hash) tuple is just a positional implicit conversion, not a bug, and the missing/hashById bookkeeping looks correct for cache hit, miss, hash-mismatch, and expiry cases.

One gap though: QueryStoreProbeCacheTests only unit-tests QueryStoreProbeCache in isolation, and the IL-scanning pins this PR's description calls out (ProbeDenominatorTests, FetchStoreConnectionBorrowTests, PlanFetchCarryoverKeyTests) are structural — they check that certain calls/setters exist in the compiled IL, not that the cache is wired correctly at runtime. Nothing in the test suite actually exercises FetchAndStorePlansAsync/FetchAndStoreQueryTextAsync with a cache-warm second call to confirm a cache hit really skips TouchAndProbePlansAsync/TouchAndProbeTextsAsync and still drains missing correctly. Given this codebase's heavy investment in regression-pinning tests for exactly this kind of call-site behavior, this integration path (probably needs DARLING_TEST_PG) seems like a natural candidate for one.

[claude]

Minor: this doc comment's <para> tags are unbalanced (8 opens vs 5 closes) — the paragraphs starting "Why this exists:", "512 KB is a measured choice…", and "Why the cap's win is bigger…" each open a <para> that's never closed before the next <para>/</summary>. Doesn't break the build since none of the csproj files in this chain set GenerateDocumentationFile, but worth tidying for consistency with the rest of the file's well-formed doc comments.

[claude]

Reviewed the probe-cache and plan-XML-size-cap changes against CONTRIBUTING.md's conventions.

Correctness. Traced QueryStoreProbeCache's wiring into FetchAndStorePlansAsync/FetchAndStoreQueryTextAsync closely, including the tuple-shape difference between (PlanId, PlanHash)/(QueryId, QueryHash) and SelectNeedingProbe's (Id, Hash) parameter — that's a positional implicit tuple conversion, not a bug. The missing/carryover bookkeeping looks correct across cache hit, miss, TTL expiry, and in-place-rewrite (hash-mismatch) cases. Left one comment noting the new runtime wiring only has unit tests for QueryStoreProbeCache in isolation, not an integration test exercising the actual skip-the-round-trip behavior inside the runner.

Lite/Darling parity. No drift: the deferred plan/text fetch-and-probe machinery (QueryStoreProbeCache, DarlingCollectorRunner, _planFetchCarryover) is Darling-only architecture with no Lite counterpart to keep in sync — confirmed there's nothing equivalent under Lite/. The plan-XML size cap (QueryPlanXmlCaptureLimits) lives in the shared PerformanceMonitor.Collectors project, so both QueryStatsCollector and ProcedureStatsCollector pick it up identically for both apps, and Lite.Tests was updated accordingly.

T-SQL style. The CASE WHEN DATALENGTH(...) > ... THEN NULL ELSE ... END fragments follow the column_name = expression convention and uppercase keywords. No new statement/batch was introduced, so no additional OPTION(RECOMPILE) is needed per the collector-query rule.

Security. No injectable input — MaxCapturedPlanXmlBytes is a compile-time constant, not user data.

Left two inline notes, both minor: a test-coverage gap on the new cache wiring, and an unbalanced-<para>-tags nit in QueryPlanXmlCaptureLimits.cs's doc comment (cosmetic only — no project in this chain sets GenerateDocumentationFile, so it won't surface as a build warning).

[erikdarlingdata]

Reviewed from the custom-alerting-implementation session (at Erik's request). Strong, well-evidenced fix and the core logic is correct — but CI is red on two real issues, both blockers, and both are artifacts of validating on macOS (the Darling.Tests Windows/live-PG project can't run there). Neither reflects on the analysis itself.

🔴 Blocker 1 — a client server identifier is in tracked source (public repo)

The doc comments cite the client host by its real short-name+ordinal — 2× in QueryStoreProbeCache.cs, 5× in QueryPlanXmlCaptureLimits.cs. FleetIdentifierScrubTests.NoTrackedSourceFileNamesAServerByShortNameAndOrdinal exists to stop exactly that from reaching open source, and it's failing on it. Scrub the identifier everywhere — the measurements are the best part of this PR, so keep them; just swap the name for a generic placeholder ("a field box" / "the measured fleet") or a synthetic slug.

🔴 Blocker 2 — a Darling-side query-shape pin wasn't updated

ProcedureStatsPlanCadenceTests.TheGatedQuery_KeepsTheRowShapeTheWriterAndPayloadDimensionExpect string-strips the old query_plan_xml = tqp.query_plan fragment (ProcedureStatsPlanCadenceTests.cs:91-92) to derive the no-plan shape, then Assert.Equal(gated, stripped). The new CASE … DATALENGTH … fragment no longer matches that .Replace(), so it fails with "strings differ." The Lite-side definition tests were updated; this Darling-side pin was missed. Update the two .Replace() strings to the new fragment — ideally derive them from the collector constant so they can't drift again. (Only ProcedureStats fails; QueryStats has no equivalent Darling cadence pin.)

✅ Core logic — verified, not just the write-up

• The plan-XML cap is applied server-side (DATALENGTH(tqp.query_plan) > cap → NULL in T-SQL), so the oversized value is nulled on the SQL Server and never crosses the wire or materializes as a client LOH allocation — that genuinely addresses the GC-pause root cause rather than masking it. And since dm_exec_text_query_plan returns nvarchar(max), DATALENGTH = the exact .NET UTF-16 byte count, so 512 KB is a precise bound. The "read twice ≠ double TVF invocation" note is correct (OUTER APPLY materializes the column once).
• QueryStoreProbeCache is thread-safe (ConcurrentDictionary), hash-keyed so an in-place rewrite is a miss by construction, TTL = GuardHours (doesn't loosen the touch guard's liveness contract), and the runner wiring Confirms only Resolved && !HashStale verdicts while treating cache hits as resolved. Correct.
• Good parity — both collectors, in the shared Collectors project (Lite benefits too); follow-ups [DESIGN] Eventually collect plan XML that QueryPlanXmlCaptureLimits caps, without reintroducing the batched-large-fetch problem it fixes #3392/query_store's plan fetch shares query_stats' pre-fix anti-pattern (uncapped GetString, no per-row size bound) — worth measuring #3393 filed for the known gaps.

Minor (non-blocking)

The cache is deliberately unbounded, which is defensible — but a high-plan-churn target (often exactly the ones with these symptoms) accrues stale (id, oldhash) entries beyond the ~175k estimate. Inert and sweep-able; worth a line in the #3392-adjacent follow-up rather than a change here.

Both blockers are mechanical — fix them and this is a merge. Happy to pair on either.

[erikdarlingdata]

Both blockers fixed (force-pushed):

Blocker 1 (leaked identifier). Scrubbed both files — the real server name is gone from QueryStoreProbeCache.cs, QueryPlanXmlCaptureLimits.cs, and the commit message (amended, not just a follow-up commit, so it's not recoverable from an old SHA on this branch). Verified against the actual guard, not just a manual grep: ran FleetIdentifierScrubTests for real via a throwaway net10.0 harness (both .cs files here are plain-net10.0-reachable) — both tests pass clean against the current tree.

Blocker 2 (cadence test). ProcedureStatsPlanCadenceTests.TheGatedQuery_KeepsTheRowShapeTheWriterAndPayloadDimensionExpect — updated the .Replace() strings to the new CASE WHEN DATALENGTH(...) > ... THEN NULL ELSE ... END fragment, derived from QueryPlanXmlCaptureLimits.MaxCapturedPlanXmlBytes per your suggestion rather than a repeated literal. Ran the actual file for real (renamed the harness assembly to Darling.Tests to pick up the InternalsVisibleTo grant) — that test passes, along with 53 of the file's other 54 tests (the one miss is a [Theory]/[InlineData] method my harness's plain reflection runner can't parameterize — unrelated to this change, not something the fix touches).

Thanks for catching both — especially the identifier, that was a real mistake on my part.

[claude]

Reviewed the diff (3ab91a7..HEAD). This is unusually well-documented and well-tested for its size; I traced the two changes in detail and didn't find correctness, parity, or security issues to flag.

QueryPlanXmlCaptureLimits / the DATALENGTH guard (QueryStatsCollector.cs, ProcedureStatsCollector.cs)

• Lives in the shared PerformanceMonitor.Collectors project, so Lite and Darling both get the cap automatically — no parity drift.
• CASE WHEN DATALENGTH(tqp.query_plan) > 524288 THEN NULL ELSE tqp.query_plan END follows the column_name = expression style and correctly ships NULL (not a placeholder) when over cap, consistent with how QueryStorePlanMap's content-less markers already work.
• Verified the fragment is well-formed both as a top-level splice (QueryStatsCollector) and inside the nested sp_executesql dynamic-SQL string (ProcedureStatsCollector's standard path) — no embedded quotes to break the nesting.
• PlanSelectFragment correctly moved from const to static readonly since it's no longer a compile-time constant expression once it concatenates QueryPlanXmlCaptureLimits.MaxCapturedPlanXmlBytes.
• Tests in both Lite.Tests and Darling.Tests were updated to assert the new guarded shape via the shared constant rather than a repeated literal.

QueryStoreProbeCache + DarlingCollectorRunner integration (Darling-only, by design — Lite's store is a local DuckDB lookup, not a network round trip, so it has no analogous cost to cache away)

• Traced the tuple-typed reference plumbing ((long PlanId, string? PlanHash) in the runner vs. (long Id, string? Hash) in the cache API) — these are the same underlying ValueTuple types with different compile-time element-name aliases, so the cross-calls are sound, not a type mismatch.
• Cache-confirmed ids are correctly excluded from the store round trip and still correctly cleared from missing (paid debt); only misses reach TouchAndProbePlansAsync/TouchAndProbeTextsAsync.
• Keying on (server, database, id, hash) correctly forces a cache miss on any in-place plan/text rewrite.
• The liveness-touch tradeoff (skipping the round trip means skipping that cycle's last_seen touch attempt) is bounded by reusing QueryStoreLivenessTouchGuard.GuardHours as the cache TTL, so it doesn't loosen the guard's own staleness promise — checked this against QueryStoreLivenessTouchGuard's doc comment and it's an accurate characterization.
• QueryStoreProbeCacheTests.cs covers the important edges well: TTL boundary (< not <=), hash-mismatch-is-a-miss, server/database scoping, null-hash-as-a-valid-key, and confirmation overwrite/extension.
• The PerItemPlanProbeIds/PerItemTextProbeIds metric now reflects post-cache round-trip size rather than raw reference count — this is a deliberate, documented change and still matches the field's stated contract ("handed to the touch/probe round trip"), so the ~0.61ms/reference denominator stays honest.

No missing-index DMV suggestions, no secrets/credentials, no new file/network/process surface. Nothing else stood out as a bug or a Lite/Darling divergence.