Part of #2874 - #2966
Merged
Merged
Part of #2874#2966
Conversation
|
Reviewed the diff (test-only change in One thing worth flagging: the PR description says this closes out "the last pin in the
By contrast, This isn't just a doc nit: the bare Worth either correcting the PR description's inventory, or opening a tracking issue for migrating those two files onto |
|
Reviewed. This is a clean, test-only change ( Traced the three new
No leftover references to the removed private regex/wrapper, doc-comment No Lite/Darling parity concern — this scanner infrastructure (Npgsql command-timeout pins) has no Lite counterpart. No production code, T-SQL, or security-relevant surface touched. Nothing to flag. |
erikdarlingdata
added a commit
that referenced
this pull request
Sep 5, 2026
… prose The count of pins routing through CommandDeadlineScanner was stated in doc comments and checked by nothing, so it went stale three times: #2938 wrote five, #2940 made it six, #2966 seven, and this branch nine. The enumerated list beside the count rotted the same way, which is the worse half - a reader looking for the adopters found four names and no hint that others existed. CommandDeadlineScannerAdoptionTests globs the pins and re-derives the set from the tree, against a declared adopter list and a declared abstainer list. The abstainer half is what earns the test: a new *CommandTimeoutTests.cs arriving with its own private copy of the rule lands in neither list and fails asking which it is, which is how the two holdouts consolidated here came to exist. StartupCommandTimeoutTests is the one declared abstainer, because it judges every site relationally against its own bootstrap constant and the shared scanner cannot express which constant a site must take. The membership test reads STRIPPED source, so prose naming the method does not count as routing through it - every file in this family discusses the shared judgement at length, and a raw read would pass on the commentary. The prose counts in CommandPlaneCommandTimeoutTests and CSharpSourceWalkerTests are dropped rather than corrected, since a number nothing asserts is what failed here. CommandDeadlineScanner.cs's own stale count is left alone to avoid colliding with the deletion in flight against that file.
This was referenced Sep 5, 2026
Merged
erikdarlingdata
added a commit
that referenced
this pull request
Sep 5, 2026
#2981) CommandDeadlineScanner's class summary opened with a count and a roster of the pins routing through it. Nothing checked either, and both drifted: the count was written five, then six, then seven, then nine over #2938, #2940, #2966 and #2972, and the enumerated list never moved at all. CommandDeadlineScannerAdoptionTests already re-derives the adopting set from the tree and declares the deliberate abstainers beside it, so the summary now points there instead of restating it. Nothing left in the comment is countable, so there is no figure left to go stale. The same file quoted the MCP read surface's census as "112 of the MCP surface's 119 sites" to argue that the assignment spelling dominates. That numerator no longer holds - every site in McpReadCommandTimeoutTests' scope is assignment-spelled today - so the claim keeps its point and hands the census back to the pin that maintains it. The adoption pin's own rationale counted the names in the roster it replaced. That figure described a list this change removes, so it goes too.
Merged
erikdarlingdata
added a commit
that referenced
this pull request
Sep 5, 2026
…nd name a test class that exists The three comments crediting #2966 for last_error's missing timestamp were crediting the wrong issue: #2966 is about pin-count adoption, and nothing in the tree connects it to this. The lesson is #3010's, measured on the managed PostgreSQL fleet and pinned by LastErrorCurrencyTests - which also declines to make its own predicate a band input, for the same reason this block is not one. Lite's block pointed at LiteAlertReadSurfaceTests, which does not exist; the pin is Lite.Tests' AlertReadFailureSurfaceTests. And the two stacked comment blocks over last_failure_at are one.
erikdarlingdata
added a commit
that referenced
this pull request
Sep 5, 2026
* Count the alerting layer's swallowed store reads, and put the count where collection health is Every condition check in the alert pass wraps its store read in log-and-skip: on a failure it writes one [ERROR line and returns, because firing on absent evidence fabricates an alert and resolving on it fabricates a recovery. That posture is correct and is unchanged. What was missing is that a swallowed read reached no surface a person reads - it is not a collector run, so it writes no collection_log row, so get_collection_health stayed green while the alert pass went blind one condition at a time. Only a grep of the service log found the class. Twenty-seven such sites across the shared engine, Darling's self-alert evaluator and the worker's PostgreSQL predictor passes now record the failure on a process-lifetime counter, naming which read went blind. Both SKUs' get_collection_health carries an alert_read_health block: the per-server count, the alert-pass count it sits over, the instance total (which is the only home the fleet-scoped store self-alerts have), the newest failure's timestamp and read name, and counting_since. The web dashboard's Collection Health fanout gains a panel on both server tabs, with its own subtitle because these figures are not the trailing seven days every sibling panel on that tab reports. In memory and deliberately not persisted: what it counts is a failure to read the store, so a counter that had to write the store would be unavailable exactly when it has something to say. Not a band and not a status input, for #3017's reason one level down - a threshold here would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this surface fails by saying nothing is wrong. * Say what counting_since is precisely, and drop a null-conditional that can never fire The worker's counter field is non-nullable and initialized to the process instance, so the null-conditional at its eight call sites could never short-circuit and read as if the field might be absent. The engine's and the self-alert evaluator's fields stay nullable — they take the counter as an optional constructor argument so a test can build one that counts nothing. counting_since is when this counter FIRST counted, which is early in the host's own startup rather than literally the process's first instruction. Both tool descriptions say so identically and the shared note matches; a surface that overstates its own floor by a few hundred milliseconds is a small version of the defect this block exists to remove. Also documented at the seam: a null key reads as no server rather than as the fleet bucket, and the fleet bucket reaches every reader through instance_read_failures instead. RecordPass takes a nullable key for symmetry with RecordReadFailure, and no caller passes null today. * Serialize the two new timestamps the way every other timestamp on this response is last_success, last_error_at and last_denied_at on the collector rows all go out as round-trip "o". A raw DateTime serializes to an ISO string too, but with trailing zeros trimmed, so last_failure_at and counting_since would have carried different precision guarantees from their neighbours on one payload for no reason. Applied identically on both SKUs. * Cite the issue that actually taught the last_error currency lesson, and name a test class that exists The three comments crediting #2966 for last_error's missing timestamp were crediting the wrong issue: #2966 is about pin-count adoption, and nothing in the tree connects it to this. The lesson is #3010's, measured on the managed PostgreSQL fleet and pinned by LastErrorCurrencyTests - which also declines to make its own predicate a band input, for the same reason this block is not one. Lite's block pointed at LiteAlertReadSurfaceTests, which does not exist; the pin is Lite.Tests' AlertReadFailureSurfaceTests. And the two stacked comment blocks over last_failure_at are one. * Stop the two remaining doc comments overstating when counting begins CountingSince is the first touch of the counter, which for the process instance is early in the host's startup rather than the process's first instruction. The shipped note and both tool descriptions already said so; these two XML comments still said "process start", one line from the citation they were sitting beside. The note's own text is unchanged - only where its source lines break. Verified by dumping the constant out of the built assembly: 1,536 characters before and after. * Record the PostgreSQL predictor group as the third alert pass, and pin pass/failure pairing Review found it: the six PostgreSQL predictor checks each recorded a swallowed read and none recorded a pass, so a PostgreSQL target reported two passes for three while its failures landed in the numerator normally. A guarded numerator over an unguarded denominator is worse than neither, because the pair still renders and now understates its own exposure. One pass for the whole group rather than one per check. The six are independently failure-isolated exactly as the shared engine's fourteen Check*Async calls are, and those fourteen are one pass: isolation granularity is not pass granularity. Recorded after the null-store guard, for the same reason the engine records after its master switch. The note and both tool descriptions said "two passes per sweep", which stopped being true the moment a third existed. They now give the inventory per host and per target engine and say the denominator is comparable within a host and engine and not across them. EveryAlertEvaluationPass_RecordsItselfInTheDenominator is the guard the census tests did not provide: they proved every swallowed read is counted and said nothing about whether the pass that issued it is. It asserts per entry point AND asserts the tree-wide RecordPass count equals the number of entry points, so a fourth pass that forgets to record fails, and a RecordPass placed somewhere that is not a pass fails too. * Assert each arm of the pass inventory separately, after red-proofing found one clause enough The first draft of the inventory check asserted only that "runs three" appeared in the note. Red-proofing it against a note whose SQL Server arm had been broken came back GREEN: one true clause is not a true inventory. Each of the three arms and the cross-engine disclaimer is now a separate assertion, so breaking any one of them fails. * Census any caught type, not just Exception, and prove the one exclusion is safe The scanner matched `catch (Exception` only, so a swallowed read behind a narrower type inside a scoped member reported CLEAN. That is not hypothetical here: FetchFailedJobsAsync swallows a failed msdb read in `catch (SqlException ex) when (IsPermissionDenied(...))` and the census could not see it. Nothing was miscounted today, because that member is exempt for its own stated reason and now has an entry for both arms rather than one - but the gap was the #2786 shape the rest of this file was written to avoid, found by auditing the caught types rather than trusting that they were all Exception. OperationCanceledException stays out, and no longer on faith: NoCancellationCatch_QuietlySwallowsAReadFailure asserts every such block in scope either rethrows or logs nothing at error level, with a floor on how many it examined so the assertion cannot pass vacuously. A block that logged an error and returned would be a swallowed read hiding behind the one type the census does not look at. Two scanner controls added beside the existing ones: a narrower caught type WITH a `when` filter must be reported, and a cancellation catch must not be a census subject at all. * Assert the pass is recorded BEFORE the reads, not merely somewhere in the method Presence cannot see placement. A RecordPass moved inside the try - after the reads rather than before them - records the pass only on cycles that succeeded, so a cycle whose read failed would add to the numerator and nothing to the denominator. That is the same defect as omitting the call entirely, arriving through position instead of absence. Asserted structurally rather than behaviourally on purpose: reaching these bodies at runtime needs a live store, and a test that opens a socket to prove a static ordering is a flaky test proving a fact the source already settles. The arm is conditional, because not every pass entry point owns a try - AlertEngine's EvaluateCoreAsync dispatches fourteen checks that each own theirs - and it counts how many entry points it actually reached, with a floor, so it cannot go vacuous for all three unnoticed. Matched as the try STATEMENT rather than the substring, since "try" occurs inside retry, entry and geometry. * Isolate the CPU read from the sweep, single-source the fleet set, and stop counting a target read Three findings from review, judged on their own merits and measured at the site rather than reasoned from shape. REACHABILITY, and the largest of the three. EvaluateAlertsAsync read the latest CPU sample inside the same try as engine.EvaluateServerAsync. That read runs on the alert-pass deadline and is the pass's first store read, so under the contention #3013 measures it fails first - and when it did, the engine sweep was skipped entirely, so EvaluateCoreAsync never recorded its pass while the caller's catch still recorded a failure. Numerator up, denominator unchanged, worst exactly when the counter matters most. A third route to the defect the PostgreSQL group had: not omission and not placement but a pass site that is real, correctly placed, and never entered. The larger half is not the arithmetic. A single failed CPU read aborted the whole shared sweep for that server that tick, so blocking, deadlocks, poison waits, long-running queries, TempDB, low disk, PVS, file growth, jobs, database state and forced plans went unevaluated. The snapshot already documents a null CPU pair as normal input and CheckCpuAsync gates on HasValue, so the read now has its own try and degrades to (null, null): the tick loses its CPU alert and nothing else. NoCountedRead_SharesATryWithThePassItWouldSkip is the guard, brace-balancing every try in the member because the whole claim is that these two statements are in different ones. FLEET INVENTORY, wrong in both directions. The list named store disk pressure, whose two feed reads are both exempt so it can never contribute a failure, and omitted the collector-cost regression self-alert, which does. Either way an operator reading it hunts the wrong thing. The set is now one public const that WindowNote, the class remarks and both tool descriptions concatenate - a const-string concatenation is a compile-time constant, so it cannot grow in one place and go stale in three - with the set derived from source and checked against it. FAILED JOBS, declining the reported defect and fixing a different one. The claim was that a throwing SaveFailedJobWatermarkAsync would be mislabelled a read; both implementations swallow it without rethrowing, so that is unreachable and the write-inside-the-try asymmetry is inert. What IS wrong is that the fetcher reads the monitored server's msdb - the population DarlingWorker's own fetcher is exempted for - so counting it here contradicted that exemption and would put a target-side outage into a number read as store contention. Exempt, with the reasoning at the site. * Stop the PostgreSQL pass comment claiming a master-switch parity the code does not have The comment said the pass is recorded after the guard "for the same reason the engine records after its master switch". It is not. The shared engine records after EvaluateServerAsync's !_settings.AlertsEnabled early return and the self-alert evaluator returns before recording on the same check; this path is guarded only on _postgres/_alertDeliverer being non-null, and DarlingWorker holds no reference to AlertsEnabled anywhere in the file. Measured rather than assumed: zero AlertsEnabled references in DarlingWorker.cs against one in AlertEngine.cs and ten in DarlingSelfAlertEvaluator.cs, and the only gates between the sweep body and the predictors are the null-runtime check, the engine check and the null-store check. So with alerting switched off the PostgreSQL predictors still read, still evaluate and still reach _alertDeliverer. The denominator is left alone deliberately. Gating only this line would make the count deny passes that genuinely ran and alerts that genuinely fired - a surface lying about its own exposure, which is the defect this whole change exists to remove. Whether the Tier 0 predictors are deliberately exempt from the master switch is a question about alert delivery, not something to settle inside a counter, so the count stays truthful and the gap is named at the site.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The collection-sweep pin was asking the
#2874family's deadline question its own way.CommandDeadlineScanner(#2938) is the shared judgement, and this pin kept a privates_setsTimeoutand aSetsTheSweepDeadlinewrapper instead. Same divergence #2913 fixed forCSharpSourceWalker: private copies of one judgement, already drifted, only some carrying the hardening.This does not close the family, and the inventory is worth stating exactly — re-derived after merging
origin/dev, which brought in #2940. Measured bygrep -l 'CommandDeadlineScanner.SetsAnExplicitDeadline' *.cs, six pins had adopted the shared judgement before this change —StorageCommandTimeoutTests,ViewerCommandTimeoutTests,AnalysisPassCommandTimeoutTests,FactCollectorCommandTimeoutTests,AlertPassCommandTimeoutTestsandCommandPlaneCommandTimeoutTests— and this makes seven.Two holdouts remain and are NOT touched here:
StragglerCommandTimeoutTests.cs(privates_setsTimeoutat line 100, used at line 548) andMcpReadCommandTimeoutTests.cs(line 74, used at lines 158, 497, 505, 534 and 539). Both still carry exactly the single-span rule this change argues is unsound, so both plausibly hold the same live false-accept blind spots this fixed for the sweep. They are left for separate assessment rather than folded in: McpRead's surface is ~119 sites, so re-deriving its census is a scope call, not a rider on this one.This is a behaviour change, not a tidy-up
The private copy was a SINGLE span — a bare
CommandTimeout\s*=over the two statements from the construction. The shared judgement asks in two halves, and each half closes a false ACCEPT the single span cannot see:usingheader's block, which spends both counted statements on the sibling. It is in the same scope and in the very next statement, so no statement count or scope bound can exclude it — only the NAME the assignment qualifies can.Both arrive as permanent fixtures in
TheDeadlineScanner_DoesNotBorrowANeighboursDeadline, with a positive control so the theory cannot pass by rejecting everything.The censuses were RE-DERIVED, not adjusted
The two-span rule is stricter in one direction: the assignment half requires name-qualification against the bound variable, so a site assigning
CommandTimeoutthrough a differently-named reference would newly report untimed. That would be a finding, not a number to tune — so both constants were re-derived by enumerating the real sources through the pin's owns_sweepMembers,MemberBodyands_commandCtor, and asking both rules at every site.ExpectedSweepCommandSites= 13, unchanged.ExpectedCopyWriterSites= 4, unchanged. Zero sites disagree between the old rule and the shared one, so the offender list stays empty and nothing was reclassified.All thirteen are answered by the name-bound half, none by the initializer half — so the stricter rule is not merely reachable for this pin's shapes, it is the only path that carries them. Two do it through receivers not called
command:isDueandupsert, both_postgres!.CreateCommandsites inTryRefreshPgStatementTextAsync, which theBoundNamewalk resolves through ausingheader declaration and a plain declaration respectively.What deliberately did NOT change
The scanner holds no construction regex — it takes an offset the caller found — so adoption cannot narrow construction matching.
s_commandCtorstays the pin's own, qualifiednew Npgsql.NpgsqlCommand(alternative and bare.CreateCommandmethod group included. Kept for the same reason: the twelve(file, member)pairs of member scoping, which exist because.Servicespans budget regimes; the store-versus-target receiver classifier, which has no equivalent in the shared helper and which.Storageand.Viewernever needed; and the COPY half, whoseNpgsqlBinaryImporter.Timeoutis a different property on a different type that the scanner knows nothing about.Verification
Windows-only suite, so CI is the arbiter. Locally the REAL
.csfiles were compiled by absolute path into a throwawaynet10.0xunit host withAssemblyName=Darling.Tests, staged inside the repo tree: 34 tests, 0 failed, 0 skipped, 0 not run, up from 31 — the three added fixtures. The realnet10.0-windowsproject also rebuilds withEnableWindowsTargeting=true: 0 errors, and no doc-commentcrefleft dangling by the deleted regex.Proved red first, one mutation at a time, each confirmed applied by anchor count and content hash and each confirmed to have relinked the assembly, so no green came from a stale binary:
Expected: False, Actual: True— the two false accepts, reproduced — while the census stays green, which is the same evidence that the thirteen real sites are unmoved.EverySweepBodyCommand_IsBuiltAgainstTheStore_NotAMonitoredTargetand both of its unit fixtures.No
CHANGELOG.mdentry — that file is the coordinator's, and this entry is reported to them. No production code, no schema bump, no migration rung.Re-verified after merging
origin/dev(which brought #2940 and #2965): both censuses unchanged at 13 and 4, zero old-versus-new disagreements, 34 tests green, and the real project still rebuilds with 0 errors.ServiceCommandDeadlines.CollectionSweepSecondsis still 10 andDarlingWorker.SweepWatchdogSecondsstill 60, so the band assertion is unaffected.