Repository navigation
Serve blocking severity, and share the deadlock aggregation (#2484) - #2497
Conversation
… places Third of the #2484 list, and the reason it needed its own change: the deadlock aggregation was internal to the Darling viewer, so the headless service could either duplicate it or move it. It moved to Common. There were already TWO copies -- the viewer's and Lite's -- computing victim counts and total wait from parsed graphs. A third would have been how three surfaces end up quietly disagreeing about one deadlock. The record now lives in Common and both SKUs use it; the viewer's internal entry point stays as a delegate so the nine pins written against it keep working, and because they now exercise the moved code they ARE the evidence the move changed nothing. Truncation was copied verbatim rather than rewritten to an equivalent, so not a single bucket boundary can shift. Lite keeps its own aggregation body: it parses into DeadlockProcessDetail where Darling uses DeadlockProcessNode. Only the record is shared. Unifying the parsers is a real question and not this change's. What the read adds over what already shipped: get_blocking_trend and get_deadlock_trend count incidents. Ten one-second blocks and one ten-minute block are the same count and are not the same problem. This is severity -- total, max and process-weighted average -- which is the half nobody could reach outside the desktop viewer. The empty answer checks BOTH capture paths. Either can be off on a given server: the blocked-process report needs its XE session, the DMV snapshot needs its collector. Probing one would report "never captured" for a server capturing fine through the other, and probing neither would let a silent capture gap read as a clean bill of health -- which is the worst outcome here, because "no blocking" is what an operator wants to hear. Total wait sums EVERY process, not just victims, which is why victim_count is a separate field rather than something derivable from it. Pinned. Census 103 -> 104 tools, 78 -> 79 shared, Darling-unique unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// </summary> | ||
| public const string HasAnyBlockingCaptureSql = """ | ||
| SELECT 1 | ||
| WHERE EXISTS (SELECT 1 FROM v_blocked_process_reports WHERE server_id = $1) |
There was a problem hiding this comment.
HasAnyBlockingCaptureAsync only probes two of the three capture paths this endpoint reports on.
get_blocking_stats combines blocking-duration data (from v_blocked_process_reports / v_dmv_blocking_snapshots) and deadlock severity (from v_deadlocks, populated by its own independent XE session — see DeadlockXeSessionName / RemoteCollectorService.Deadlocks.cs, a collector that's entirely separate from the blocked-process-report and DMV-blocking-snapshot paths). But HasAnyBlockingCaptureSql only checks the first two:
SELECT 1
WHERE EXISTS (SELECT 1 FROM v_blocked_process_reports WHERE server_id = $1)
OR EXISTS (SELECT 1 FROM v_dmv_blocking_snapshots WHERE server_id = $1)Concretely: a server whose blocking XE/DMV capture has worked at some point in history, but whose deadlock collector is disabled or has never run, will get everCaptured = true for every quiet window — even one with zero deadlocks captured ever. The response then reads "empty" / "This server HAS captured blocking before, so the window is genuinely clear" — a false clean bill of health for the deadlock half of the payload. That's exactly the failure mode this PR's own description calls out as "the worst outcome" for blocking (an operator hearing "no blocking" and stopping there) — except here it applies unnoticed to deadlocks, because deadlock capture health was never checked.
Suggest either checking v_deadlocks here too (OR EXISTS (SELECT 1 FROM v_deadlocks WHERE server_id = $1)), or determining blocking/deadlock availability independently and wording the unavailable/empty message per-section. Same gap exists in Lite/Services/LocalDataService.BlockingStats.cs's HasAnyBlockingCaptureAsync (identical SQL shape, same missing check) — so it isn't a Lite/Darling parity drift, but a bug shared by both.
| command.CommandText = @" | ||
| SELECT 1 | ||
| WHERE EXISTS (SELECT 1 FROM v_blocked_process_reports WHERE server_id = $1) | ||
| OR EXISTS (SELECT 1 FROM v_dmv_blocking_snapshots WHERE server_id = $2)"; |
There was a problem hiding this comment.
Same gap as Darling's twin (DarlingDataReader.HasAnyBlockingCaptureAsync): this only checks v_blocked_process_reports and v_dmv_blocking_snapshots, but get_blocking_stats (in Lite/Mcp/McpHealthTools.cs) also reports deadlock severity from v_deadlocks, which is populated by its own independent XE session (DeadlockXeSessionName, a collector entirely separate from blocking capture). A server with blocking-capture history but a disabled/broken deadlock collector will get a false "empty"/"genuinely clear" verdict on every quiet window instead of "unavailable" — exactly the silent-capture-gap-reads-as-clean-bill-of-health failure this PR is designed to avoid, just uncovered for the deadlock half. Consider adding OR EXISTS (SELECT 1 FROM v_deadlocks WHERE server_id = $3) (with a third bound parameter) or splitting the empty/unavailable verdict per section.
|
Reviewed the diff (Common aggregator extraction + new What looks solid:
One correctness issue, present identically in both apps (left inline comments on each): No SQL injection, secrets, or missing-index concerns found. Didn't spot other Lite/Darling drift beyond the shared bug above. |
I sliced the old method out starting at its SIGNATURE, so its doc block
stayed behind and stacked onto the delegate that replaced it. XML docs take
the last one, so tooling looked right and only a human reading the file was
misled -- which is exactly what the hygiene guard says.
The guard also says not to just delete the first block, because it usually
belongs to a member an insertion pushed it away from. Here it belongs to
code that MOVED, so deleting is right -- but only after checking sentence by
sentence that Common carries it. One did not: the tie between this
truncation and the count trend's DATE_TRUNC('minute', deadlock_time), which
is why a severity chart and a count chart line up bucket for bucket. Carried
over first, then the head deleted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| /// </summary> | ||
| public const string HasAnyBlockingCaptureSql = """ | ||
| SELECT 1 | ||
| WHERE EXISTS (SELECT 1 FROM v_blocked_process_reports WHERE server_id = $1) | ||
| OR EXISTS (SELECT 1 FROM v_dmv_blocking_snapshots WHERE server_id = $1) |
There was a problem hiding this comment.
HasAnyBlockingCaptureSql only probes v_blocked_process_reports and v_dmv_blocking_snapshots — it never checks v_deadlocks. But the caller (GetBlockingStats below) gates the empty vs unavailable verdict on blocking.Count == 0 && deadlocks.Count == 0, i.e. this single boolean is used to certify both halves of the payload as "genuinely clear" or "never captured".
Concretely: if blocking capture has historical rows (so everCaptured is true) but v_deadlocks has never had a row for this server (deadlock XE session/trace never enabled — a real, independent failure mode from the two sources this check covers), and the current window happens to have zero blocking events too, this returns:
"This server HAS captured blocking before, so the window is genuinely clear."
...which asserts a clean bill of health for deadlocks that was never actually verified. That's exactly the failure mode this PR's own design principle warns against (PR description: "probing neither would let a silent capture gap read as a clean bill of health... that is the worst outcome"), just reintroduced for the deadlock half of this same endpoint.
Worth adding an EXISTS (SELECT 1 FROM v_deadlocks WHERE server_id = $1) arm (or a separate probe) so the verdict actually reflects both capture pipelines this endpoint reports on. Same gap exists in Lite's HasAnyBlockingCaptureAsync (Lite/Services/LocalDataService.BlockingStats.cs) for parity.
| public async Task<bool> HasAnyBlockingCaptureAsync(int serverId) | ||
| { | ||
| using var connection = await OpenConnectionAsync(); | ||
| using var command = connection.CreateCommand(); | ||
|
|
||
| command.CommandText = @" | ||
| SELECT 1 | ||
| WHERE EXISTS (SELECT 1 FROM v_blocked_process_reports WHERE server_id = $1) | ||
| OR EXISTS (SELECT 1 FROM v_dmv_blocking_snapshots WHERE server_id = $2)"; | ||
|
|
||
| command.Parameters.Add(new DuckDBParameter { Value = serverId }); | ||
| command.Parameters.Add(new DuckDBParameter { Value = serverId }); | ||
| return await command.ExecuteScalarAsync() is not null and not DBNull; |
There was a problem hiding this comment.
Same capture-availability gap as Darling's twin (DarlingDataReader.HasAnyBlockingCaptureAsync): this checks v_blocked_process_reports and v_dmv_blocking_snapshots only, never v_deadlocks. Since McpHealthTools.GetBlockingStats gates its empty/unavailable verdict on both blocking.Count == 0 && deadlocks.Count == 0, a server whose blocking capture has historical rows but whose deadlock capture never produced a single row would still get told "this server HAS captured blocking before, so the window is genuinely clear" — asserting a clean bill of health for deadlocks that was never checked. See the parallel comment on the Darling side for the full scenario.
Separately, this method binds serverId to two distinct parameters ($1 and $2) for the same value, where the sibling GetBlockingDurationStatsAsync a few lines below reuses a single $1 across multiple occurrences in the same query. Not a bug, just an inconsistency with the established convention in this file.
|
Reviewed the diff (headless Blocking Stats endpoint + the deadlock-severity aggregator move to Overall: solid, careful parity work. The move of One correctness finding left as inline comments on both No SQL injection, secrets, or missing-index-DMV concerns — all new reads are parameterized, and I didn't flag anything folklore-related. |
…ignored Review finding, and a real one. The empty verdict fires only when the blocking series AND the deadlock series are both empty, but the probe behind it checked only the two BLOCKING sources. So a server whose deadlock capture had never run, but whose blocking capture was fine, would be told the window was "genuinely clear" -- on the strength of a source that says nothing about deadlocks. That is precisely the failure this probe was written to prevent, and I wrote the probe reasoning about capture paths while forgetting that one of the two series it covers comes from a third one. The doc comment even argued for checking both paths rather than one; it was counting the wrong "both". Now all three, on both SKUs. There was no live test for this tool -- only the pure aggregator had one -- which is why nothing caught it. There is one now, and it pins the case in the direction that would otherwise flip: a deadlock outside the window and no blocking rows at all must read as a clear WINDOW, not as never-captured, because telling an operator to go fix collection that is working is the same defect pointing the other way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good catch, and a real one — fixed in both SKUs. The verdict fires only when the blocking series and the deadlock series are both empty, but the probe behind it checked only the two blocking sources. A server whose deadlock capture had never run, with blocking capture working fine, would have been told the window was "genuinely clear" on the strength of a source that says nothing about deadlocks. That is exactly the failure the probe was written to prevent. The doc comment even argued the case for checking both paths rather than one — it was counting the wrong "both". Now all three. There was no live test for this tool, only for the pure aggregator, which is why nothing caught it. |
| public sealed record DeadlockSeverityStatsPoint( | ||
| DateTime Time, int VictimCount, long TotalWaitMs, long MaxWaitMs, double AvgWaitMs); | ||
|
|
||
| public sealed partial class ViewerDataService |
There was a problem hiding this comment.
Minor cleanup nit: now that AggregateDeadlockSeverity just delegates to DeadlockSeverityAggregator.Aggregate in Common, the using System.Linq; at the top of this file (line 11) is no longer used anywhere in it — the .OrderBy/.Select calls that needed it moved to PerformanceMonitor.Common/DeadlockSeverityAggregator.cs. There's also a leftover double blank line here (30-31) where the removed DeadlockSeverityStatsPoint record used to sit. Neither breaks the build (only CS4014 is WarningsAsErrors in this project), just tidiness.
Review summaryWent through the full diff (Darling MCP tool + Postgres reads, Lite MCP tool + DuckDB reads, the shared No correctness, parity, or security issues found. Specifically checked and confirmed OK:
One trivial nit left as an inline comment: |
#2497 landed on dev while this was open and added its own HasAnyBlockingCaptureAsync - same name, same signature, and on Lite the same partial class, so the merge stopped compiling. The collision is worth more than a rename, because the two probes answer opposite-facing questions and reaching for the wrong one is precisely the failure both changes exist to prevent. #2497's probe asks whether an EVENT was ever captured, over the three data tables. That is right for get_blocking_stats, whose verdict is about severity. It is wrong for a trend, because a server collected perfectly for months that simply never blocked has no event rows - so it would be told its blocking has never been captured, which is the reassuring-answer failure inverted. This one asks whether the COLLECTOR ever ran, over collection_log, because for a trend the denominator is whether we LOOKED, not whether we found something. So: HasAnyBlockingCollectorRunAsync / HasAnyDeadlockCollectorRunAsync, named for what they actually ask, with a doc note on each pointing at its neighbour and saying why they are not interchangeable. On Lite they now live one file apart; a reader reaching for the nearer one by name should be stopped by the comment rather than by a wrong answer in production. Merged rather than rebased: another session is committing to this branch, and rewriting its history to fix a conflict it did not cause is a worse trade than a merge commit. #2485 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebasing onto dev picked up #2497's get_blocking_stats, which is a windowed read and took the anchor under the same rule as everything else. The changelog still said 52/45. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebasing onto dev picked up #2497's get_blocking_stats, which is a windowed read and took the anchor under the same rule as everything else. The changelog still said 52/45. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…anything get_blocking_stats shipped in erikdarlingdata#2497 with an event-existence probe: had a blocking or deadlock row ever been stored. That is wrong on an edge table. A server collected perfectly for months that simply never blocked has no rows at all, so the probe answers no and the read tells a healthy server this is NOT a clean bill of health and to go check its XE session -- a false alarm that sends someone to fix collection that is working. erikdarlingdata#2500 got this right for the trends and left a note saying the event probe was nonetheless correct for get_blocking_stats, because that verdict is about severity. That reasoning does not survive contact with a healthy server: zero severity is the NORMAL state, so the same false alarm fires. The note is corrected along with the code. The denominator is now whether we LOOKED: a successful run of either blocking capture path, or of the deadlock collector, in collection_log. Both paths because either can be off alone; deadlocks because the verdict covers that series too. The live test now pins the case the first design got backwards -- a collector that RAN and saw nothing, with no event rows anywhere, must read as a clear window. That is the ordinary state of a well-behaved server and it was the one state the original probe could not describe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third item on #2484, and the one that needed its own PR: the deadlock aggregation was
internalto the Darling viewer, so the headless service could either duplicate it or move it.The move is the substance
There were already two copies computing victim counts and total wait from parsed graphs — the viewer's and Lite's. A third would be how three surfaces end up quietly disagreeing about one deadlock.
DeadlockSeverityStatsPointand the aggregation now live inPerformanceMonitor.Common.AggregateDeadlockSeveritystays as a delegate, so the nine existing pins written against it keep working — and because they now exercise the moved code, they are the evidence the move changed nothing.TruncateToMinutewas copied verbatim rather than rewritten to an equivalent, so not a single bucket boundary can shift.DeadlockProcessDetailwhere Darling usesDeadlockProcessNode. Only the record is shared. Unifying the parsers is a real question and not this change's.What the read adds
get_blocking_trendandget_deadlock_trendcount incidents. Ten one-second blocks and one ten-minute block are the same count and are not the same problem. This is severity — total, max, and process-weighted average — the half that existed only in the desktop viewer.The empty answer checks both capture paths
Either can be off on a given server: the blocked-process report needs its XE session running, the DMV snapshot needs its collector enabled.
That second failure is the worst outcome here, because "no blocking" is exactly what an operator wants to hear. So
unavailablesays outright this is NOT a clean bill of health and names both prerequisites.Verification
DeadlockSeverityAggregatorTests— run locally, 2/2 passing, against the realCommonassembly via anet10.0shim. Pins that total wait sums every process (4500, not 1500) rather than only victims, that the average is process-weighted, that buckets come back in time order when fed out of order, and that an unparseable graph contributes nothing rather than a zero bucket — an empty bucket would read as "a deadlock happened and cost nothing", and absent evidence is not evidence of calm.fanout()— one read, two charts. Counts are deliberately not charted beside durations: a count and a millisecond on one y-domain is the two-units mistake the CPU chart's dropped idle series exists to avoid.Census 103 → 104 tools, 78 → 79 shared, Darling-unique unchanged.