Repository navigation
Let a zero-output collector's own notes answer for its zero, instead of asserting it is an event collector at rest (#3160) - #3162
Conversation
…of asserting it is an event collector at rest (#3160) FormatOutputFinding told an operator that a collector storing zero rows "stores a row only when an event occurs ... zero is the correct resting state on a well-behaved target and needs no action". That is right for deadlocks and blocked_process_report and wrong for query_store, which is not an event collector and stored zero rows on 11,728 consecutive runs on a read-replica fleet, every one carrying an empty-enumeration note. The conclusion happened to be correct there; the rationale was not, and the identical sentence over a query_store that had genuinely stopped would read as reassurance. note_count is the fourth term. Runs that recorded what they found get a finding that reports HOW MANY did and defers to last_note; runs that recorded nothing keep the category reading, now with the precondition it rests on stated out loud. The formatter takes the COUNT and never the note TEXT, so the note keeps exactly one home in FormatCollectionNote rather than gaining a second copy that can drift. Not a collector-name list, which is what #2511 exists to refuse: it would go stale in the direction that makes it pass, because the next periodic collector to break gets the event-collector sentence until somebody remembers to add it. Both new pins drive the two readings out of one collector name to assert that no list is consulted. Neither branch is quieter than the text it replaces, and the sweep that found #3030, #3109 and #3154 reads rows_stored, which nothing here touches. Both SKUs' get_collection_health descriptions taught two zero readings and now teach three.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed the change to Logic: The new denial → noted → category branch order is correct, and the invariant it leans on ( Parity: Confirmed byte-identical between SKUs where the PR claims it: the Test correctness: Manually traced both new Minor/unrelated: All 7 touched files ( No correctness bugs, security issues, or performance regressions found — this is a purely in-process C#/display-text change with no new I/O, no new SQL, and no external input. |
…ch the declaration by shape (#3160) BothSkusRowTypes_DocumentTheOutputMembers_Identically covered RowsStored, RunsWithRows and ProductiveRunPercent. OutputFinding is the member this change edits in both SKUs, and its two doc comments diverged once while that was being written - so the drift this pin exists for is demonstrated on the member the pin did not cover. Hand-fixing that divergence left no mechanism to catch the next one. Adding the case alone would not have worked. DocComment matched a declaration against two literal spellings, `public long X { get; set; }` and `public double X =>`, and OutputFinding is `public string? X =>` - so the theory would have failed its own re-anchoring precondition rather than comparing anything. The helper now matches the SHAPE, so it covers the row type's fourth member and any fifth without being hand-edited first.
|
Reviewed the diff (ServerHealthBands.cs, DarlingDataReader.cs, DarlingMcpDataTools.cs, LocalDataService.CollectionHealth.cs, McpHealthTools.cs, and the two test suites). Correctness: Lite/Darling parity: Security/performance: No input handling, SQL, or hot-path code touched — this is pure display-text formatting in a low-frequency health-reporting path. No concerns. Nit: left an inline comment — this PR incidentally prepends a UTF-8 BOM to all 7 files it touches, inconsistent with the rest of the codebase. Harmless (stripped transparently on read) but worth cleaning up. Nothing blocking. The mutation-testing table and the noted/unnoted positive-control test design are good — the new tests would actually catch a regression here. |
None of these seven carried a BOM before this branch, and the tree is ~97% BOM-less, so the effect was to move seven files into the minority for no reason anybody chose. Functionally inert - the compiler, File.ReadAllText and the ReadRepoFile pins all strip it during decode - but it renders as a whole-file rewrite in tools that key on the first line. Removed as bytes rather than through a text decode, because a BOM strip must not be able to touch line endings: all seven keep their CRLF exactly (1454, 2170, 1570, 662, 722, 639 and 334 pairs) with no bare LF, and the text either side of the three bytes is identical.
|
Reviewed this PR (the merged diff spanning all three commits: the new Correctness — Lite/Darling parity — checked byte-for-byte:
Security / performance — no T-SQL, no external input handling, no new I/O in this change; it's pure formatting/doc/test logic over already-validated counts. No missing-index or other DMV concerns apply here since no collector queries were touched. No defects found. The one thing I couldn't do in this sandbox is actually execute |
|
Reviewed the diff ( Correctness: The new branch order (denial → noted → category) in Parity: Every call site of Security/perf: No new I/O, no string interpolation of untrusted input (all format args are No findings to flag. |
|
Reviewed for correctness, Lite/Darling parity, security, and performance. No blocking issues found. Correctness — Lite/Darling parity — confirmed byte-identical:
Security/Performance — no new input handling, no SQL touched (NoteCount was already populated pre-PR), purely a display-string formatter change. No regression concerns. Minor/no-op note — commit 1 adds a UTF-8 BOM to all seven touched files, commit 3 removes it again byte-for-byte (confirmed CRLF/line-count preserved). Net diff is BOM-free, so nothing to fix, but worth flagging for anyone reviewing commit-by-commit rather than the squashed diff, since it makes commit 1 look like a whole-file rewrite in tools that key off the first line. |
Both get_collection_health tool descriptions conflicted: this branch rewrote the output_finding sentence from two zero readings to three, while #3152 added last_failure_elapsed_ms and the deadline reading beside it. The two edits are to disjoint parts of one long string, so both survive - dev's text is the base and the three-reading block replaces its two-reading sentence. Both SKUs' descriptions remain byte-identical, which their parity pin requires.
|
Reviewed. This is a well-scoped, thoroughly self-documented change — I verified the substantive claims rather than taking them on faith:
Left one non-blocking nit inline on a test fixture magic number. No correctness, parity, or security issues found otherwise. |
… from note_count is COUNT(error_message) over SUCCESS runs, so a fixture passing the total run count asserted a row with more notes than successes - a state no query can return. FormatOutputFinding does not validate the count, so the test passed while describing an unreachable input. The factory now rejects such a count, because it is the one place every fixture passes through.
|
Reviewed the diff. No Found one real bug (posted inline): Lite's new Everything else looks solid:
Minor nit, not blocking: |
…ng's The previous change fixed the Darling twin only, which left the two test files disagreeing about whether a note count above the success count is representable - the drift this file's parity discipline exists to prevent. Lite's fixture now passes the success count and its Row helper rejects a larger one, as Darling's does.
|
Reviewed the diff. This is a well-tested, narrowly-scoped fix ( Two minor nits left as inline comments, both in the test fixtures added by the fixup commits:
Neither blocks merge — both are cleanup nits in test-only code. |
…Lite's counts The added line comments restated the block comment directly above them and were the only // comments in either file, which CONTRIBUTING asks against and neither file otherwise contains. The new information is one clause, so it belongs in that block. Lite's fixture counts are now named as Darling's are, since the two files are meant to match line-for-line and the guard referring to a bare 63_448 was the asymmetry left by adding the guard to Lite in its own style rather than the twin's.
|
Reviewed the Correctness. Branch order is sound: early return on Lite/Darling parity — checked byte-for-byte, all consistent:
The Other checks: no T-SQL touched by this PR, so the collector-query style rules don't apply here. No new input/network/file/process handling, so no security surface. No perf concern — this is pure string formatting with no added queries. No stray Nothing blocking. Nice, precise fix. |
…tually holds note_count above success_count is producible, not unrepresentable: success_count also excludes legacy abandoned-by-note rows, which carry a non-null error_message on a SUCCESS status and are therefore counted by note_count. Those rows having aged out of retention makes the tighter bound true of current data rather than structurally true, so the guard as written rejected a shape production can emit. note_count <= total_runs is what FormatOutputFinding rests on, both aggregates coming from one GROUP BY.
|
Reviewed the diff (ServerHealthBands.cs, both SKUs' MCP tool/reader files, both test suites) against CONTRIBUTING.md's C# and Lite/Darling parity conventions. No correctness, parity, or security issues found. What I checked specifically:
Nothing to flag — this is a clean, well-tested fix. |
1 participant
CollectorHealthClassifier.FormatOutputFindinggave an operator the right conclusion via a rationale that does not apply, and the identical sentence would have appeared over a genuinely broken collector.For any zero-row collector with no current denial it asserted: "For one that stores a row only when an event occurs — a deadlock, a blocked-process report, a blocking chain, a held xmin — zero is the correct resting state on a well-behaved target and needs no action." That is right for
deadlocksandblocked_process_report.query_storeis not an event collector and got it anyway — zero rows on 11,728 consecutive runs on a read-replica fleet, every one carrying an empty-enumeration note.What changed
note_countis the fourth term, and the branch order is denial, then noted, then category.A collector whose runs recorded what they found gets a finding that reports how many did and defers to
last_note. A collector whose runs recorded nothing keeps the category reading — now stating the precondition it rests on ("No run recorded a note, which is what that reading rests on.") instead of asserting a category outright.The formatter takes the count and never the note text. The note's prose keeps exactly one home in
FormatCollectionNote; a finding that rendered it too would be a second copy of a sentence whose whole value is being accurate about one server's answer, and the copy that drifts is never the one being read.TheFindingSignature_TakesTheNoteCount_AndNoNoteTextpins the absence of a string parameter as the property rather than an accident.note_count > 0is safe to point a reader atlast_notefor, and that was checked rather than assumed:note_countisCOUNT(error_message)over SUCCESS runs andlast_noteisnote_rank = 1, whoseORDER BYputs noted runs first — so a positive count always has a note to read.The closing caveat is now one constant shared by both zero-output branches, for the reason
OutputWindowNoteis one.Not a collector-name list
That was the other option and it is what #2511 exists to refuse — "The answer is derived, never transcribed." A list goes stale in the direction that makes it pass: the next periodic collector to break gets the event-collector sentence until somebody remembers to add it. Both new pins drive the two readings out of one collector name, which is the assertion that no list is consulted.
The property kept is that a deliberate zero and a broken zero stay distinguishable. Neither branch is quieter than the text it replaces, and the "SUCCESS with zero rows" sweep that surfaced #3030, #3109 and #3154 reads
rows_stored, which nothing here touches.Both SKUs' tool descriptions taught two readings and there are now three
get_collection_health's description saidoutput_finding"says which of the two zero readings applies". Byte-identical in both SKUs, and it would have been wrong the moment this shipped. It now teaches the third and names the deliberate case.BothSkusToolDescriptions_TeachThatACorrectZeroExistsand the cross-SKU ordinal/wiring pins still hold.The
OutputFindingdoc comment is updated in both SKUs and verified byte-identical whitespace-collapsed — the drift classBothSkusRowTypes_DocumentTheOutputMembers_Identicallyexists for.Tests
Two new cases per SKU. The discriminating one differs from its sibling in exactly one input —
note_count— the wayTheThreeReadings_AreSeparableFromOneRowdiffers in exactlydenialIsNewest, and it uses the unnoted row as the positive control for its twoDoesNotContainassertions so a check passing by matching nothing cannot hide in the pair.Mutation-tested; every mutation is caught only by the pin that claims it:
TheNotedZero_DefersToTheNote_AndTheUnnotedZeroKeepsTheCategoryReadingTheFindingSignature_TakesTheNoteCount_AndNoNoteTextThe last one compiles at every existing call site, so it is the realistic silent version of that mistake rather than an obviously breaking one.
The parity pin now covers the member this change edits
BothSkusRowTypes_DocumentTheOutputMembers_IdenticallycoveredRowsStored,RunsWithRowsandProductiveRunPercent.OutputFindingis the member edited in both SKUs here, and its two doc comments diverged once while this was being written — so the drift the pin exists for is demonstrated on the one member the pin did not cover. Fixing that by hand left nothing to catch the next one.Adding the
[InlineData]case alone would not have worked, which is why this is a code change and not a one-liner.DocCommentmatched a declaration against two literal spellings —public long X { get; set; }andpublic double X =>— andOutputFindingispublic string? X =>. The theory would have failed its own re-anchoring precondition rather than comparing anything. The helper now matches the declaration by shape, so it covers the row type's fourth member and any fifth without being hand-edited first, the same reasonOrdinalMapbeside it is derived.Verified member-specifically rather than by the theory merely failing: diverging one word in the Lite copy alone reds
BothSkusRowTypes_DocumentTheOutputMembers_Identically(OutputFinding)whileRowsStored,RunsWithRowsandProductiveRunPercentall still pass. Restored and confirmed by content, not by diffstat.There is no Lite twin of this theory — it lives only in the Darling suite — so the change is Darling-only.
Verification note
The 17 real cases in the Darling suite were run locally, including the cross-SKU pins that read the Lite sources from disk — so the Lite production changes are verified. The Lite suite's own two new cases are clones of the Darling ones and were not run locally (WPF target); CI runs them.
Relationship to #3161
#3161 records that a collector definition cannot contribute to
collection_log.error_messageat all — every note value is runner-authored — and recommends not building that seam yet. This change needs none of it: the note it defers to is already runner-authored, andNoteCountwas already on the row. It also respects that issue's constraint, which is the part that overlaps: the finding carries no stored verdict. It is re-derived from the row's counts on every read, and it reports a count while pointing at the note rather than rendering a conclusion that would go stale the moment somebody fixed the condition.CHANGELOG entry text
Not committed here, per the batching convention: