Top Cpu Queries' Max Dop is the newest plan's reading with the cross-plan maximum kept as a dated history, so a MAXDOP-1 instance stops reporting 16 from a plan compiled before the pin (#3648)

[erikdarlingdata]

The lie

The High CPU card's Top Cpu Queries (and the bad_actor_query detail behind a BAD_ACTOR finding) reported a Max Dop per query that was computed as

MAX(max_dop) AS max_dop … FROM v_query_stats … GROUP BY database_name, query_hash

max_dop is sys.dm_exec_query_stats.max_dop — a per-plan high-water mark: the highest degree of parallelism that one cached plan has ever run at since it entered the cache. Grouping by query_hash folds every plan the statement text had inside the window into one row and keeps the largest counter, so the field reported the highest DOP any plan for that text ever ran at — not the DOP the plan spending the CPU is running at, and with nothing saying which plan, when, or whether that plan still exists. The reader also coerced a NULL reading to 0 and the card rendered it as a number, although the DMV never reports 0 (a serial plan is 1).

Evidence

On a live page today (#3648) the #1 row said Max Dop 16 on an instance whose MAXDOP is 1 — configured 1, in use 1, values_match true, zero config changes across 59 snapshots in 14 days — and whose stored plan for that exact query_hash is serial: NonParallelPlanReason="MaxDOPSetToOne", EstimatedAvailableDegreeOfParallelism="1", no Parallelism operators. The 16 belonged to a plan compiled before the instance was pinned to 1, still sitting in the cache with its old counter. Row #3 on the same card said Max Dop 1 for a hash with one serial plan — the same field behaving honestly, so the inconsistency the reader saw was within the field's semantics, not a bug in one row. A reader recommended a MAXDOP 1 Query Store hint off the field — the exact decision the card exists to support — and retracted it after reading the plan the card should have summarized.

The fix — state the basis or don't state the number (#3574 / #3632 shape)

Both SKUs' top_cpu_queries and bad_actor_query reads now rank each hash's rows newest-first inside a windowed CTE (ROW_NUMBER() OVER (PARTITION BY database_name, query_hash ORDER BY collection_time DESC, creation_time DESC NULLS LAST, delta_worker_time DESC NULLS LAST)) and carry the hash-wide MAX(max_dop) OVER (...) onto every row, so the outer aggregate can say which reading is current and when the maximum was last seen:

field	meaning
max_dop	the newest plan's reading — the row with the latest collection_time (ties: the later-compiled plan, then the one that spent the CPU). null when the DMV row carried none. Existing consumers that read max_dop now get the honest number.
max_dop_any_plan	the cross-plan maximum — what the field used to be, now labelled as history
max_dop_any_plan_last_seen	the latest snapshot that carried that maximum (ISO-8601, null when unknown)
plan_count	COUNT(DISTINCT query_plan_hash) in the group
dop_note	the one sentence a renderer can print verbatim, composed once in PerformanceMonitor.Common.QueryDopProvenance so both SKUs say the byte-identical thing; null when the history agrees with the headline

Exact dop_note format, fired only when a plan in the group ran at a higher DOP than the newest plan reports:

DOP 1 (a parallel plan ran at 16 until 2026-09-04; 3 plans in window)
DOP unknown for the newest plan (a parallel plan ran at 16 until 2026-09-04; 2 plans in window)
DOP 1 (a parallel plan ran at 16; 1 plan in window)        ← no last-seen available; singular count

New columns are appended after the old ones so the existing reader ordinals are untouched, and the SQL text is byte-identical between Lite's inline cmd.CommandText and Darling's TopCpuQueriesSql / BadActorDetailSql constants — the NULLS LAST is explicit because DuckDB and Postgres default DESC null placement differently and an implicit default would make the two SKUs pick different rows on the same data. The windowed CTE reads the same row set the old GROUP BY scanned (same window predicates, same delta_worker_time > 0 filter on the top-CPU read), adding one sort; the bad-actor read is one hash and is trivially cheap.

The comments in both collectors and in the shared helper name the DMV semantics, why hash-grouping lied, and the live consequence.

Reader census (MAX(max_dop) grouped by hash, published without a plan dimension?)

• Fixed: Lite DrillDownCollector.Queries.cs CollectTopCpuQueries + CollectBadActorDetail; Darling PgDrillDownCollector.Queries.cs TopCpuQueriesSql + BadActorDetailSql. These are the top_cpu_queries / bad_actor_query records on the analysis card, in get_analysis_facts / analyze_server drill-down payloads, and through Top CPU Queries renders as a two-column field jumble on Slack — one section block per query, not seven interleaved fields #3644's summary line.
• Already honest — left: queries_at_spike (both SKUs) reads v_query_snapshots.dop — the live request's actual DOP from dm_exec_requests, not the plan-cache high-water mark. The issue's "sibling at ~592" is in fact BadActorDetailSql, fixed above. Query Store readers (ViewerDataService.QueryStore, LocalDataService.QueryStore, DarlingDataReader's QS read) group by (database, query_id, plan_id, …) — a plan dimension. ViewerDataService.ItemHistory and DarlingTrendReader / get_query_trend publish per-row max_dop with no cross-plan folding (and null it when aggregated).
• Left, documented, recommended follow-up: the MCP get_top_queries_by_cpu (Darling DarlingDataReader.TopQueriesSql / TopQueriesByHostObjectSql; Lite LocalDataService.QueryStats TopQueriesSql) and the viewer query-stats grids (both SKUs) group by (database, query_hash, host_object_name) and publish min_dop/max_dop with query_plan_hash = MAX(…) as a representative. The MCP tool's description already labels max_dop a lifetime max and points to analyze_query_plan, and its parallel_only / min_dop filters are defined on that lifetime max, so re-headlining there changes a documented filter contract and a TopQueryRow positional record consumed by two tool files and their tests — a lane of its own, not a rider on this one. The same four fields (plan_count, max_dop_any_plan, max_dop_any_plan_last_seen, dop_note) are the recommended shape.

What this does NOT do

• It does not touch PerformanceMonitor.Analysis/ (FactScorer / InferenceEngine / FactAdvice), DarlingMcpTools.cs, AnalysisNotificationService.FlattenInto, or WebhookAlertService — the Top CPU Queries renders as a two-column field jumble on Slack — one section block per query, not seven interleaved fields #3644 lane owns how record-shaped drill-downs render; the new fields flow through its summary line. Read-only note for that lane: ScalarText renders JsonValueKind.Null as "", so a null max_dop / dop_note currently appears as its label with an empty value rather than being omitted.
• It does not cross-check against server_config MAXDOP — QUERY_HIGH_DOP fires on a stale lifetime max_dop, not current parallelism #2705/Lite still raises QUERY_HIGH_DOP from a stale lifetime max_dop — #2705's guard landed on Darling only #2999 already do that for the QUERY_HIGH_DOP fact; the drill-down record now carries its own provenance instead.
• It does not change the MCP get_top_queries_by_cpu contract (see census).

Tests

• Lite.Tests/DrillDownDopProvenanceTests (real DuckDB, 6): the live page's fixture — one hash, an old parallel plan at 16 last seen 3 h before window end, a new serial plan at 1 at the newest snapshot → max_dop = 1, max_dop_any_plan = 16, plan_count = 2, max_dop_any_plan_last_seen = the parallel plan's last snapshot, exact dop_note, windowed total still spans both plans; the same through bad_actor_query; one serial plan → no note; NULL reading → JSON null on every DOP field, never 0; newest unknown + parallel history → "DOP unknown…"; both plans at the same newest snapshot → the later-compiled plan is the headline (the NULLS LAST tie-break).
• Darling.Tests/DrillDownDopProvenanceParityTests (5): both reads byte-identical across SKUs — Lite's inline text read from source, Darling's from the public constants — hosted on the Darling side because build.yml's darling filter reaches every Lite .cs while the lite filter does not reach Darling/PerformanceMonitor.Darling.Analysis (CrossAppGuardCiGateTests enforces this, and caught head 1). Lite.Tests/DrillDownDopProvenanceParityTests (4) pins Lite's own provenance shape, that the bare MAX(max_dop) AS max_dop is gone, that no unknown DOP is coerced to 0, and meta-pins Darling's byte-identity theory — the QueryHighDopStaleMaxDopParityTests pair shape.
• Lite.Tests/QueryDopProvenanceTests + Darling.Tests/QueryDopProvenanceTests (7 each, identical decision table): the dop_note format and every no-note arm.
• Darling.Tests/DrillDownDopProvenanceLiveTests (DARLING_TEST_PG-gated): the same three scenarios against real Postgres through PgDrillDownCollector.
• Executed here: Lite 30/30 green (incl. CrossAppGuardCiGateTests) on real DuckDB via the mactest harness (net10.0-windows build, Microsoft.WindowsDesktop.App dropped from the runtimeconfig); Darling 45/45 green including the live test (through LiveStoreCleanup, per the The other ~124 live-test finally blocks still clean up on the body's connection, so a failing test can still report its teardown's error #1902 ratchet) against a timescale/timescaledb:2.28.1-pg18 rig with a seeded query_stats, plus DarlingAnalysisPipelineTests (dialect / FROM-target / AllSql-count guards over the new CTE), CreationTimeClockFrameDisciplineTests, StoreSqlClockDisciplineTests. All touched projects build -c Release -p:EnableWindowsTargeting=true with 0 warnings.

Closes #3648

[claude]

Reviewed the core change (PgDrillDownCollector.Queries.cs, DrillDownCollector.Queries.cs, QueryDopProvenance.cs) plus the new test suites.

Correctness: the windowed CTE / ROW_NUMBER() OVER (... ORDER BY collection_time DESC, creation_time DESC NULLS LAST, delta_worker_time DESC NULLS LAST) + MAX(...) OVER shape checks out — max_dop_any_plan_last_seen is derived by matching each row's own max_dop against the partition-wide max_dop_any_plan, which is always satisfiable since the max is computed from the same NULL-excluding set. QueryDopProvenance.Note's decision table (headline vs. history, the <= guard, the "unknown headline + serial-only history" no-op case) matches its test tables in both Lite.Tests and Darling.Tests exactly. The NULLS LAST divergence between DuckDB/Postgres default null ordering is called out and handled explicitly, which is the right call given #3648's root cause was exactly this kind of unstated per-engine assumption.

creation_time clock-frame discipline: the new ORDER BY ... creation_time DESC NULLS LAST tie-break is a same-server relative ordering (both sides are v_query_stats rows for the same server_id), not a comparison against a UTC window bound, so it correctly doesn't trip CreationTimeClockFrameDisciplineTests's bare-comparison scan — worth double-checking that reasoning since creation_time is elsewhere treated as load-bearing to de-skew.

Lite/Darling parity: TopCpuQueriesSql/BadActorDetailSql are byte-identical between the two SKUs (verified by diffing the literals directly), matching DrillDownDopProvenanceParityTests' claim, and QueryDopProvenanceTests is duplicated verbatim across both test projects.

Non-blocking observation: ROW_NUMBER()'s three-way tie-break (collection_time, creation_time, delta_worker_time) leaves a fourth, unhandled level of non-determinism if two plans for the same hash ever tie on all three (e.g., two plans compiled and captured at the identical instant with identical CPU consumption) — the engine's arbitrary pick would be stable within one query execution but isn't guaranteed stable across repeated runs. Extremely unlikely in practice and not something the PR needs to solve, just flagging for the record.

No security, SQL-injection (all params bound), or perf concerns — the added CTE is one extra sort over the same filtered row set the old GROUP BY scanned.

[claude]

LGTM — verified the windowed-CTE/ROW_NUMBER ranking logic and QueryDopProvenance.Note's decision table against their tests, confirmed the two SKUs' SQL is byte-identical, checked the creation_time tie-break doesn't violate the clock-frame de-skew discipline, and confirmed no T-SQL/OPTION(RECOMPILE) or missing-index concerns apply (this touches Postgres/DuckDB analysis SQL, not SQL Server collectors).

[claude]

Reviewed the diff, the shared QueryDopProvenance composer, both SKUs' top_cpu_queries/bad_actor_query SQL, and the test suite (unit pins, real-DuckDB drill-down tests, live-Postgres test, and the byte-identity + shape parity guards on both sides).

Findings:

• Correctness: the windowed CTE's ROW_NUMBER() OVER (... ORDER BY collection_time DESC, creation_time DESC NULLS LAST, delta_worker_time DESC NULLS LAST) newest-plan pick, the MAX() OVER hash-wide maximum, and the max_dop_any_plan_last_seen derivation (MAX(CASE WHEN max_dop = max_dop_any_plan THEN collection_time END)) all check out — traced through by hand and confirmed against the TwoPlansAtTheSameNewestSnapshot test, which specifically exercises the DuckDB/Postgres NULLS LAST divergence that would otherwise make the two SKUs pick different rows on identical data. QueryDopProvenance.Note correctly gates on maxDopAnyPlan <= newest and on the null/serial-only case, matching its own decision-table tests.
• Lite/Darling parity: TopCpuQueriesSql/BadActorDetailSql are verified byte-identical to Lite's inline text (confirmed by reading both files directly, not just trusting the guard), and the parity is meta-pinned in both directions (Lite.Tests pins that Darling's guard still does the byte comparison; Darling.Tests does the comparison itself) per the repo's established CrossAppGuardCiGateTests pattern. build.yml's darling filter includes Lite/**/*.cs, so an edit to either side's SQL reaches the job that runs the comparison.
• NULL handling: confirmed max_dop now projects null rather than the old 0-on-IsDBNull coercion in both readers, and the DOP note never invents a parallel history from an all-null group.
• Security/OPTION(RECOMPILE): N/A — these are DuckDB/Postgres analysis-drilldown reads, not T-SQL collector queries; existing file convention (-- comments, AS-based aliasing) is followed consistently with the surrounding code.

No correctness, security, or parity issues found. Nice use of a live-Postgres test specifically to pin the NULLS LAST engine-default divergence rather than just string-matching the SQL.

[claude]

LGTM — verified the max_dop provenance logic (newest-plan ranking, hash-wide max, NULLS LAST tie-break), NULL-vs-0 handling, and Lite/Darling byte-identity parity across both drill-down reads; no correctness, security, or parity issues found.