Fixes #3013

[erikdarlingdata]

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 here. 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 and every other health read stayed green while the alert pass went blind one condition at a time. Only a grep of the service log found the class.

This is option 1 of the three the issue lays out: count them. Options 2 (alert on the alerting) and 3 (separate deadline exhaustion from unreachable) are deliberately not done — see below.

Severity, stated honestly

The issue reads as though the elevated rate is ongoing. It is not, and that was caught independently twice: the sample behind the filing was taken while the last pre-treatment continuous-aggregate refresh was still finishing, and the monitoring session noticed the same contamination on its own about ninety minutes later. Two separate corrections, arrived at separately, agreeing.

At rest the rate is 2–3 per hour, measured four times across six hours on one dogfood store (2/hr, 3/hr, 2/hr, 3/hr, all at max_concurrent_sweeps = 4) — at or near the pre-incident noise floor. So this closes a blind spot; it is not a response to a live fire.

What justifies it is the incident series, not the current rate. During the store-side lock contention the service log's [ERROR rate rose 41 → 61 per hour — roughly 20x the measured floor — and every one of those lines was a store read from the alerting or analysis side. Over the same hours, actual collector failures in collection_log were falling: 23, then 7, then 5, then 2. Two populations moving in opposite directions, and the rising one appeared on no health surface at all.

The mechanism is deadline stratification. The alert pass runs on DarlingAlertReadAdapter.AlertPassCommandTimeoutSeconds while the collection sweep runs on budgets an order of magnitude longer, so as store latency rises the short-deadline consumers cross their limit first and at a rising rate while the long-deadline ones still complete. The first casualty of store contention is the alerting layer; the last is the thing alerting exists to report on.

The counter's introduction exposed a latent outage-shaped behaviour, and that is the strongest argument for it

Found in review of this PR, in the code path this PR instruments — pre-existing on dev, not introduced here.

DarlingWorker.EvaluateAlertsAsync read the latest CPU sample inside the same try as engine.EvaluateServerAsync. That read runs on the alert pass's own command deadline and is the pass's first store read, so under the store contention this issue documents it is the first thing to fail. When it did, the try unwound and the shared engine sweep was never invoked — so for that server, on that tick, not one of its thirteen check families was evaluated — CPU, blocking, deadlocks, poison waits, long-running queries, TempDB space, low disk, PVS pressure, file growth, anomalous jobs, failed jobs, database state, forced-plan failures, plus the blocking-wait gate nested inside blocking and the restart-survival watermark seed. One cheap read on the shortest deadline took all of them down with it, and nothing said so: the sweep's catch logged a single line naming the server, not the conditions that went unjudged.

That is this issue's thesis in its sharpest available form. #3013 argued that under contention the alerting layer fails first and silently while collection keeps working; this is the same mechanism one level deeper, where the alerting layer's own cheapest read silently disables the rest of the alerting layer. It was invisible for the same reason the original class was: it produces no collection_log row and no health-surface change.

Fixed here because the counter cannot be made honest without it — the numerator incremented on this path while the denominator did not, since EvaluateCoreAsync never ran to record its pass. The CPU read now has its own try and degrades to (null, null); AlertServerSnapshot already documents a null CPU pair as normal input and CheckCpuAsync gates on HasValue, so the tick loses its CPU alert and keeps the other twelve.

Worth stating plainly: an instrument whose introduction surfaces a latent outage-shaped behaviour in the layer it measures is a better case for the instrument than anything in the original issue. The blind spot was not only that failures went uncounted; it was that nobody could see a failed read taking a whole sweep with it.

Where the counter lives, and why

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 to be readable would be unavailable exactly when it has something to say. Both SKUs host their MCP surface in the same process as the alert pass, so there is nothing to persist it for.

Both scopes on the response, because neither substitutes for the other. The per-server count is the actionable unit and matches this tool's scope. The instance total is the only home the fleet-scoped store self-alerts have — disk pressure, compression-job health, store-job cadence and retention holds belong to no server, so a per-server-only figure would have given them no home at all, reproducing #3013's own defect one level down. They are held in a bucket that is a separate field rather than a sentinel key in the per-server map, so no server key can resolve to it however it is spelled.

Shape follows #3033/#3017's conventions with one deliberate difference. There, the output figures went flat on the collector row because the denominator (total_runs) was already on the row and nesting the numerator away from it would have split a ratio. Here neither number exists on the response yet, so a block is what keeps them together:

alert_read_health: {
  server_read_failures, server_alert_passes, instance_read_failures,
  last_failure_at, last_failure_read, counting_since, finding, note
}

last_failure_read is the actionable half — which condition went blind, not merely that one did. It is a compile-time constant per site, never the exception message: Npgsql renders both a deadline and an unreachable backend as the same seven words, so the message adds nothing the count does not already carry, and an exception message can carry host and database names that must not reach an MCP response.

last_failure_at is the currency term. This figure never ages out of a window, so without a stamp beside it a healed episode from days ago and one still in progress read identically — last_error's own lesson (#3010), on a different field.

Deliberately not a band, and not a status input. #3033 declined a membership set because on one collector it would have said "no action needed" about a collector that was in fact broken. The argument is stronger here: a threshold over blind alert reads would have to guess how many make alerting unhealthy, and a wrong guess on this surface fails by saying nothing is wrong. CollectorHealthClassifier.Classify still takes nine parameters and none of them is about alerting; that is pinned off the type.

Both windows named, and the one disclaimed

Measured: this process's lifetime, from counting_since to now. In memory; a restart takes it to zero.

Disclaimed: the fixed trailing seven days that total_runs, the three durations and #3033's rows_stored/runs_with_rows cover. alert_read_health is the only block on the response that is not on that window, and the note says so outright — a reader who assumed otherwise would read a zero as "none in seven days" when a process that started a minute ago can only report on a minute. On the web surface the same statement lives in the panel's own subtitle ("since this service started — NOT the trailing 7 days"), which is why it is a separate panel rather than six more tiles on Sweep Pressure: inheriting that panel's "trailing 7 days" subtitle would have made it assert a window it never measured.

Also disclaimed: it does not count alerts that failed to deliver, and makes no claim about them. That is get_alert_history's question, and it is answered separately below.

And the denominator is a denominator, not a rate. server_alert_passes exists so three failures over two hundred passes can be told from three over four. It is not a quotient of anything: a pass issues many reads, and a Darling sweep of a connected server runs two passes (the shared engine's conditions and the service's own store-polled self-alerts) where a Lite sweep runs one. The note says that too.

The population: 27 counted sites, 17 exempt, each with a reason

The population is store reads whose result feeds an alert decision. Not the one instance reported — swept by shape:

Where	Counted	Exempt
AlertEngine.cs (shared, both SKUs)	13 condition reads	4
DarlingSelfAlertEvaluator.cs	5 self-alert reads	7
DarlingWorker.cs (alert-pass members only)	9	6

The exemptions are stated at each site in source and are keyed by log message in the pin:

• Delivery-path failures (Alert resolution callback failed, Connection-change self-alert delivery failed) — a different fact with a different remedy, and option 2's territory.
• Writes (incident-occurrence persistence, resolution history rows, the store self-metrics sweep) — the counter is about reads.
• Occurrence-load bookkeeping — a lost occurrence total, not a condition the alert could no longer judge; the check still fires and resolves on its own evidence.
• The five fleet-scoped Evaluate* methods on the self-alert evaluator — they are handed their evidence as parameters and perform no store read; their catches cover the apply/deliver half. The reads that feed them are counted at their own sites in DarlingWorker.EvaluateCompressionJobHealthAsync. This is the one place where reading the log message alone would have got the population wrong.
• The disk-pressure volume read — a local DriveInfo read, not a store read; Under store contention the alert pass fails first and silently, so the self-alert that would report it is the first thing lost #3013's mechanism has no bearing on it.
• The pg_database_size read — context for the alert text, not the evidence the alert is judged on.
• The failed-jobs fetch — reads the monitored server's msdb over its own connection and its own timeout, not the store over the alert pass's deadline. It is a swallowed alert read, but not one this mechanism can produce, and pooling the two would put a target-side outage into a number the operator reads as store contention.

Cross-SKU parity: all four surfaces checked

get_collection_health exists on both SKUs, and #3033 found that the web dashboard's descriptors are a fourth surface that silently drops anything they do not list.

1. Darling MCP (DarlingMcpDataTools.cs) — block added; key derived with CultureInfo.InvariantCulture, matching that SKU's alert pass.
2. Lite MCP (McpHealthTools.cs) — same block, same field set; key derived with int.ToString(), matching Lite's alert pass. Each SKU mirrors its own pass rather than one spelling being imposed on the other, because changing Lite's alert key would re-key its suppression, badge and watermark state too. Both agreements are pinned from source, each from its own side.
3. Web dashboard (server-tabs.js) — a new "Alerting Reads" panel in the Collection Health fanout on both the SQL Server and PostgreSQL server tabs, as one shared ALERT_READ_PANEL const so the two cannot drift. Pinned by count against the SWEEP_STATS panel's, not by presence.
4. WPF Viewer grid — not touched, following Fixes #3017 #3033's split (own review class, The WPF Viewer computes the fleet deadlock total a third time, with the same structural-zero defect — and sharing the coverage type crosses an assembly boundary #3029).

The tool descriptions stay byte-identical across SKUs (10,172 chars each, both assembled from the shared FleetScopedReads const), pinned.

Lite gets the instrument for free because the counting lives in the shared engine. What does not transfer is the mechanism: Lite's alert reads hit a local DuckDB store, so the deadline stratification measured on a Postgres store does not apply there. The surface gap did, and this is a parity change rather than a port of the mechanism — said in the code comment so nobody later reads the Lite block as a claim about Lite's deadlines.

Option 3 (separating deadline exhaustion from unreachable): not done, and why

Exception while reading from stream is Npgsql's undocumented 30 s default CommandTimeout — a socket read timeout restarted by every backend message, so the bound is backend silence, not work. Duration separates the two bounds where the message cannot: about 10,000 ms for the alert pass's own deadline versus about 30,000 ms for the inherited default.

Distinguishing them cheaply at the catch site would need either a Stopwatch around every one of 27 reads — whose value is then a duration this surface would have to band — or a claim about which inner exception type Npgsql wraps each case in, which needs its own measurement against a genuinely contended store to verify. What was available for free, and is done, is recording which read failed: the operator learns which condition went blind without the surface asserting why. The duration discriminator is recorded here as the measurement a future option-3 change should be built on.

The issue's own "Not verified", now verified

Whether the alert deliveries were also missed, or only these particular reads.

Checked, and the answer is that deliveries were not missed. Over the contended window on the affected store, alert history carries 66 rows spread continuously across every hour — every hour of the window has rows, with no gap. Every row's send_error is null. Firing and resolution rows pair up throughout (Deadlocks Detected/Deadlocks Cleared, Blocking Detected/Blocking Cleared, High CPU/CPU Resolved, Collector Cost Regression/Cost Regression Cleared). The store's own Store Job Over Cadence self-alert fired three times inside the window — so the self-alert family kept delivering while the collection-health self-alert's read was failing.

That reduces the severity, and it is the honest reading: the alerting layer degraded partially. Specific condition reads went blind; the rest of the pass and the whole delivery path kept working.

Two limits on that check, both real. Alert history records what the engine decided to fire, so it structurally cannot show an alert that was never evaluated because its read failed — the absence of a Collection Stopped row proves nothing either way (collection was not stopped; collectors were succeeding). And alert_sent: false on the firing rows is the headless service's channel configuration, not a delivery failure — the interleaved alert_sent: true rows are what prove the path was alive.

The denominator's population, after review

Review found a real gap and it is fixed: the six PostgreSQL predictor checks each recorded a swallowed read and none recorded a pass, so a PostgreSQL target's denominator reported two passes where three ran. A guarded numerator over an unguarded denominator is worse than neither, because the pair still renders and now understates its own exposure.

Counted rather than excluded, and at GROUP granularity rather than per site. Excluding them was not viable: these are store reads on the alert path whose failure means that condition went unjudged, which is exactly #3013's population — dropping them would put PostgreSQL targets back in the blind spot the issue is about, on the fleet where the managed store actually lives. And one pass for the group rather than six, because that is the shape already in the tree: AlertEngine.EvaluateCoreAsync dispatches fourteen independently failure-isolated Check*Async calls and records one pass; EvaluateStoreAlertsAsync dispatches four and records one. Isolation granularity is not pass granularity. Six per-site passes would have counted per-check on PostgreSQL targets while counting per-group on SQL Server ones — the same category error, relocated. Recorded in EvaluatePostgresAlertsAsync after its null-store guard, for the same reason the engine records after its master switch.

One correction to how the gap was described, because it would misdirect a future change. It is not that the pair "reads as a ratio above 1" and therefore cries wolf. Failures-per-pass above 1 is the ordinary shape on every engine: a pass issues many reads, so a SQL Server target failing all fourteen engine reads in one sweep posts 14 against 2. That is not a defect, and it is precisely why this surface computes no ratio and says so — "a denominator for judging whether the failure count is large, not a rate." The actual defect was narrower and is what was fixed: a whole evaluation pass ran and was absent from the denominator, so the figure understated its own exposure by a third on PostgreSQL targets.

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 state that the denominator is comparable within a host and engine and not across them.

Pinned, both directions. EveryAlertEvaluationPass_RecordsItselfInTheDenominator asserts every pass entry point records itself and that 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. Entry points are an explicit (file, member) list with a count assertion, the mechanism AlertPassCommandTimeoutTests already uses, and deliberately never enumerated by log-line shape: that is what keeps the five fleet-scoped Evaluate* methods — which look like read sites from their log lines but are handed evidence as parameters — from slipping in.

The pins caught four bugs in themselves, all worth recording:

1. Its first run failed 2-against-3 on its own arithmetic: a negative lookbehind and a subtraction both excluding the declaration. The comment at the site now says why the count is asserted rather than the presence.
2. Red-proof variant "note keeps a stale pass inventory" came back green against a note whose SQL Server arm had been broken, because the assertion checked only that "runs three" appeared. One true clause is not a true inventory. Split into four separately-asserted arms; the variant now fails.
3. A scanner-control fixture accumulated into the shared unclassified list from the previous fixture, so Assert.Single saw two entries. The list is cleared between fixtures now — a control that fails for the wrong reason is not a control.
4. Red-proof variant "cancellation catch swallows quietly" first came back green with no test output at all, which turned out to be a silent compile failure the grep filter hid: at that site the rethrow is compiler-enforced. Re-targeted to a site where the pin is the only guard, where it fires. A variant that never ran looks exactly like a variant that passed.

The census matched one spelling of catch, and that was a gap

The re-review came back clean, and one of its verifying claims was wrong in a way worth chasing: it reported that DarlingWorker.cs's narrower catch types "all live outside the scanned alert-pass members." One does not. FetchFailedJobsAsync — which is in the scoped member list — swallows a failed msdb read in catch (SqlException ex) when (SqlServerPermissionErrors.IsPermissionDenied(ex.Number)), and the census regex matched catch (Exception only, so it could not see that block at all.

Nothing was miscounted today: that member is exempt for its own stated reason, and it now carries an exemption entry for both its arms instead of one. But the pin was the #2786 shape the rest of this file exists to avoid — a guard that names the arm it was written for — and a store read moved into a narrower catch inside any scoped member would have reported CLEAN. Found by auditing the caught types directly rather than trusting that they were all Exception.

s_catch now matches any caught type. OperationCanceledException is the one exclusion and it is proven rather than assumed: NoCancellationCatch_QuietlySwallowsAReadFailure asserts every such block in scope either rethrows or logs nothing at error level, with a floor on how many blocks it examined so it cannot pass vacuously. Two scanner controls were added beside the existing ones — a narrower type with a when filter must be reported, and a cancellation catch must not be a census subject at all.

One incidental finding worth recording: at the forced-plan site the rethrow is compiler-enforced — removing throw; there fails with CS0165 on an unassigned local, so that particular block cannot host the defect at all. The pin is the only guard at the sites where it isn't.

Presence cannot see placement

The denominator pin originally asserted that each pass entry point contains a RecordPass. That is not the whole claim. 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. Identical outcome to omitting the call, arriving through position instead of absence, and presence alone cannot see it.

The pin now asserts the pass is recorded before the reads begin. Three details that matter:

• Conditional, not universal. Not every pass entry point owns a try — AlertEngine.EvaluateCoreAsync dispatches fourteen checks that each own theirs — so where there is none the ordering claim does not apply. The arm counts how many entry points it actually reached and asserts a floor, so it cannot go vacuous for all three unnoticed.
• Matched as the try statement, not the substring: "try" occurs inside retry, entry and geometry, and a substring hit would compare the pass offset against an arbitrary identifier.
• Structural rather than behavioural, deliberately. 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. This is the honest closure of the "no functional test drives EvaluatePostgresAlertsAsync" gap: the ordering is now pinned, the runtime increment is still not.

Four review findings: three upheld, one declined

Reachability — upheld, and the smaller half was the arithmetic. 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 this issue measures it fails first — and when it did, the engine sweep was skipped, EvaluateCoreAsync never recorded its pass, and the caller's catch still recorded a failure. A third route to the same defect: not omission, not placement, but a pass site that is real, correctly placed, and never entered. Neither existing arm could see it.

The larger half is not the counter at all: a single failed CPU read aborted the whole shared sweep for that server that tick — blocking, deadlocks, poison waits, long-running queries, TempDB, low disk, PVS, file growth, jobs, database state and forced plans all unevaluated, because one cheap read on the shortest deadline threw first. That is this issue's own thesis in its sharpest form, and it is pre-existing rather than introduced here. The CPU read now has its own try and degrades to (null, null), which is safe rather than convenient: AlertServerSnapshot documents a null CPU pair as normal input and CheckCpuAsync gates on HasValue, so the tick loses its CPU alert and nothing else. Guarded by NoCountedRead_SharesATryWithThePassItWouldSkip, which brace-balances every try in the member and asserts the two statements sit in different ones.

Fleet inventory — upheld, and wrong in both directions. The enumeration omitted the collector-cost regression self-alert, which records a null-key failure. Measuring the other side found the mirror error: it named store disk pressure, whose two feed reads are both exempt (a local DriveInfo read; a pg_database_size read that is context for the alert text), so it can never contribute a failure. The list was simultaneously missing a real cause and advertising a phantom one.

An inventory maintained by hand in four places cannot fail when the set grows, so it is not maintained in four places any more: AlertReadFailureCounter.FleetScopedReads is one public const, and WindowNote, the class remarks and both tool descriptions concatenate it — legal inside a Description attribute because a const-string concatenation is a compile-time constant. TheFleetScopedInventory_MatchesWhatTheCounterActuallyRecords derives the set from source, asserts the count, asserts each site is represented, asserts the phantom stays gone, and asserts every tool file references the constant rather than repeating it.

Worth recording how nearly this was missed: the first grep for RecordReadFailure(null returned one site when there are two, because an earlier commit had wrapped one of those calls across lines. A line-bound pattern would have "proved" a single fleet-scoped condition. The pin matches Singleline for exactly that reason.

Failed jobs — declined as reported, and a different defect fixed. The reported consequence rests on SaveFailedJobWatermarkAsync throwing. Both implementations swallow — PgAlertStateStore and DuckDbAlertHistoryStore each log "Could not persist failed-job watermark" and do not rethrow — which is the contract this file's own remarks state. So a write failure cannot reach that catch and cannot be mislabelled. The structural observation is accurate and inert: the write is inside the read's try (measured: try at +8, save at +51) where the blocking, deadlock and database-state checks keep theirs after the catch (+55, +45, +139). Not moving it — relocating a store write in the shared engine to fix an unreachable mislabel is a behaviour change to both SKUs for no observable gain.

What was wrong is mine. The only thing that can throw into that catch is the fetcher, and on Lite FetchFailedJobsForAlertAsync has no catch at all — so on Lite it can. That fetcher reads the monitored server's msdb: precisely the population DarlingWorker.FetchFailedJobsAsync is exempted for, on the stated ground that pooling target-side reads with store reads puts a target outage into a number read as store contention. Counting it here while exempting it there was an inconsistency inside this PR. Now exempt, with the reasoning at the site.

A fourth finding, and an open question this PR does not answer

Review of the commit above caught my own comment claiming something the code does not do. The new RecordPass in EvaluatePostgresAlertsAsync said it was recorded after its 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 DarlingSelfAlertEvaluator.EvaluateStoreAlertsAsync returns before recording on the same check. This path is guarded only on _postgres/_alertDeliverer being non-null.

Measured: zero AlertsEnabled references in DarlingWorker.cs, against one in AlertEngine.cs and ten in DarlingSelfAlertEvaluator.cs — that contrast is the positive control. The only gates between the sweep body and the predictors are the null-runtime check, Engine == PostgreSql, and the null-store check, and EvaluatePostgresAlertsAsync reaches _alertDeliverer.DeliverAsync directly.

So the consequence is larger than the denominator: with alerting switched off, the PostgreSQL Tier 0 predictors still read, still evaluate and still deliver. Pre-existing, not introduced here.

The RecordPass is deliberately not gated. Gating only that line would make the denominator deny passes that genuinely ran and alerts that genuinely fired — a surface understating its own exposure, which is the defect this change exists to remove. The count stays truthful and the gap is named at the site.

The predictors are deliberately not gated either, and that is the open question. Whether the Tier 0 group is intentionally exempt from the master switch is a question about what alerting does on a live fleet, and a reporting change should not answer it by side effect. Nothing in the code claims the exemption — the method's own doc comment says recording and mute handling stay with the deliverer "exactly as for an engine-emitted alert", which reads like intended parity — but absence of a claim is not a decision. Routed as one question rather than assumed either way.

Verification

Real test files compiled into staged net10.0 xunit v3 hosts (AssemblyName Darling.Tests / Lite.Tests), with a pristine origin/dev control host built the same way for both SKUs:

	base (origin/dev)	this branch
Darling.Tests	1,309 total / 7 failed / 44 skipped	1,327 total / 7 failed / 44 skipped
Lite.Tests	327 total / 38 failed / 0 skipped	333 total / 38 failed / 0 skipped

The failure sets are byte-identical between base and branch (diffed, not eyeballed): 7 Darling failures are DarlingServiceInstallLocationTests (Windows paths, DPAPI, registry) and the 38 Lite failures are ParitySource.RepoRoot() walking up from the staging bin directory plus two McpAlertSettingsKeyTests — all harness and platform artifacts of running Windows suites on macOS, none of them reachable from anything this change touches. +18 Darling tests and +6 Lite tests, all discovered by name and all green.

Red-proofed twenty-seven ways across twenty-three distinct assertions, each variant rebuilt from source (a stale harness makes source mutations invisible) and restored by stashing the mutation only, with the fix re-asserted by content — 27 RecordReadFailure call sites present — after every restore rather than by a clean git status:

Variant	Fails	Assertion
Drop one increment (deadlocks)	3 tests	unclassified-block list; read-name count; functional per-server count
Drop RecordPass	2	ServerAlertPasses — the denominator
Fleet bucket folded into the per-server map	1	fleet/server separation
Lite's block removed	2	per-SKU presence; cross-SKU field-set equality
Panel on one server tab only	1	panel count vs. SWEEP_STATS'
Darling drops counting_since	1	Reading-member coverage, reflected off the record
…the same mutation, seen from Lite	1	cross-SKU field-set equality
Lite's read key drifts to InvariantCulture	1	key-derivation agreement with Lite's own pass
Lite left unwired at construction	1	construction wiring
Duplicate read name	1	name uniqueness (a different arm of the same pin)
Note drops the delivery disclaimer	1	window-note phrase set
FormatFinding always returns a sentence	1	a clean reading carries no finding
An exempt site starts counting	2	counted/exempt totals in both directions
Lite's tool description drifts one word	1	byte-identical descriptions
PostgreSQL predictor group records no pass	1	pass-entry-point arm
A RecordPass placed where there is no pass	3	entry-point-count arm, plus both census arms
Note keeps a stale pass inventory	1	the four inventory arms, asserted separately
The engine's own pass unrecorded	3	entry-point arm, plus both functional pins
A swallowed read behind a narrower caught type	3	census classification — the gap widening closed
A cancellation catch that logs and does not rethrow	1	the proven-exclusion pin
That pin's scan pattern broken so it reaches nothing	1	its examined-blocks floor
The pass recorded inside the try	1	pass-ordering arm
The ordering arm's try pattern broken	1	its reached-entry-points floor
The CPU read back inside the sweep's try	3	the reachability arm, plus both census arms
The failed-jobs target read counted again	2	both census arms
The disk-pressure phantom restored to the inventory	1	the inventory's phantom arm
A third fleet-scoped site the inventory omits	1	the inventory's count arm
One SKU inlines its own copy of the fleet list	2	description byte-identity, plus the const-reference tie

The census pin is red-proofed itself, not just used to red-proof the change: TheScanner_FindsAPlantedCatchBlockAndRejectsAPlantedProseOne runs three fixtures through the identical Classify call — a counted block, an exempt block, and a stray block that must be reported — plus proof that a catch (Exception ex) written in a comment is not a catch block, which matters because this change added fourteen prose exemption comments that a naive scanner would have counted.

The Darling.Tests control that decides whether the healthy case means anything (AHealthyPass_CountsItselfAndLeavesTheFailureCountAtZero) exercises the case worth worrying about — four checks enabled against an adapter that answers — and asserts the reads actually happened, so its zero is a zero from a pass that looked rather than one that was gated off.

Five commits, and the last four came out of reviewing my own diff rather than out of CI: a null-conditional on a non-nullable field that could never short-circuit; counting_since overstating its own floor (it is the counter's first touch, early in host startup, not the process's first instruction); the two new timestamps serializing as raw DateTime where every other timestamp on this response goes out as round-trip "o"; and — the one that mattered — three comments crediting #2966 for the last_error currency lesson when it is #3010's, measured on the managed PostgreSQL fleet and pinned by LastErrorCurrencyTests. git grep for the number found #2966 attached to something unrelated, which is exactly what the house rule about citing figures from closed issues exists to catch. The fifth commit is the tail of that one: two XML doc comments still said counting_since was "process start" a line away from the corrected citation. Every red-proof variant in the table below was re-run against the final head, not only the ones whose target files those commits changed, and all are still red with the tree clean and 27 call sites intact after each restore. One of them changed shape usefully in the process: the "panel on one server tab only" mutation now matches its witness TWICE rather than once, because the two tabs' panel blocks are byte-identical — which is the shared-const property that variant exists to protect, arriving as a precondition failure rather than as an assertion.

No counted site can fire on cancellation, checked across all 27 rather than sampled: every one is either preceded by an OperationCanceledException catch that rethrows, or carries a when filter excluding it. That property is load-bearing rather than incidental — a broad catch (Exception) at any of them would make an ordinary service stop or a tripped budget register as a swallowed store read, inflating precisely the number this change exists to make trustworthy, and it would do so on every shutdown.

Verified out of the built assembly rather than the source file: reflection over PerformanceMonitor.Alerting.dll reports WindowNote at 1,536 chars with all eight load-bearing phrases present and no trace of the retracted #2966, and Reading carrying exactly the six members the two surfaces render. The staleness control is the note-mutating red-proof variant above — mutating that constant's content turned the harness red, so the harness demonstrably sees changes in that file (a rebuild alone proves nothing, since .NET builds deterministically).

Scrubcheck clean at exit 0 over the diff's added lines (104,897 bytes on the first commit, 22,140 on the follow-up), 9/9 internal controls fired; the planted-marker control through the identical invocation returns exit 1 and finds all three plants. A whole-file sweep reports one host name in AlertEngine.cs — pre-existing on origin/dev, in a comment this change does not touch, zero occurrences in added lines.

Not verified

• No live-store round trip. No get_collection_health call was made against a running service carrying this build, on either SKU, so no client has received this payload. What is verified, by rendering the block through the real Reading, the real FormatFinding and the real WindowNote: the snake_case field names survive serialization verbatim (McpHelpers.JsonOptions is new() { WriteIndented = false } — read from source, no naming policy, so a camelCase rewrite of server_read_failures was a live risk and is ruled out); a clean reading renders "finding":null rather than a reassuring sentence; counting_since and last_failure_at both emit round-trip "o"; and a fleet-scoped failure lands in instance_read_failures while leaving server_read_failures at zero. The serializer options were replicated rather than referenced, because McpHelpers is internal — so that one line is read from source, not executed. What remains unverified is that the tool's own initializer produces this exact object (source-pinned against the Reading record and the other SKU, not run) and anything requiring a store.
• The counter has never been observed non-zero in production. Every functional test drives it through a throwing fake. The 41→61/hr population that justifies the change was measured in the service log, not through this instrument, and nothing here proves the instrument would have counted exactly those lines.
• The web panel is unrendered. No browser, no human, no accessibility check. Six tiles were added to a tab that already has several panels; last_failure_read has no ARIA relation to the counts it qualifies, and the panel carries no severity hint by design. Two mechanical risks were checked rather than assumed: every format token the descriptor names (int, reltime, text) exists in util.js's FORMATTERS registry, so no tile falls through to the em-dash default on a typo; and the round-trip "o" timestamps this change now emits carry seven fractional digits, which is outside what ISO 8601 requires a parser to accept — tested in V8, new Date("2026-09-05T08:00:00.0000000Z") parses and truncates to milliseconds, so relTime renders a relative string rather than an em-dash. That risk was introduced by this PR's own timestamp-format change and would have silently blanked two of the six tiles.
• The full note is not on the web surface — only the window statement, in the subtitle. A web reader gets no delivery disclaimer and no "deliberately not persisted" statement, and the finding sentence is not rendered there at all (prose does not fit a stat tile).
• The tool description grew 7,726 → 10,172 chars on both SKUs. Whether that addition earns its token cost is not measured. Fixes #3017 #3033 flagged the same thing about its own growth and it has now happened twice on this one tool.
• The exemption for the failed-jobs msdb fetch is a judgement, not a measurement. It is a swallowed alert read that this counter does not count. If an operator ever wants "every swallowed alert read" rather than "every swallowed alert read of the store," that decision has to be revisited.
• server_alert_passes counts two passes per sweep on Darling and one on Lite. That is documented in the note and in the counter's own doc comment, but it does mean the denominator is not comparable across SKUs, and nothing enforces that a reader notices.
• No per-pass counter, deliberately. The issue's wording asks for one, and a naive implementation would be wrong: two pass types run back-to-back on the same server on Darling, so a bucket reset at each pass boundary would report the engine pass's zero over the self-alert pass's failure one line earlier — the exact "reports healthy while degraded" shape this issue is about. The cumulative count plus last_failure_at gives the recency answer without that hazard.
• The third pass has never been observed incrementing at runtime. EvaluatePostgresAlertsAsync needs a ServerRuntime on a PostgreSQL engine and a reachable store to reach its RecordPass, so no functional test drives it. Its presence, its position relative to the reads, and its placement after the null-store guard are all pinned from source; that the increment actually lands on a live PostgreSQL target is not. This is the weakest link in the denominator fix.
• ReadInstance() and ServerKeys() have no production caller. Both are exercised only by the pin that proves the fleet bucket is held apart from every server — ServerKeys() is how that test can assert the empty string is not a key at all. They are kept as the observability surface for that invariant and as the shape a fleet-level reader would need; nothing on either SKU calls them today, and their doc comments state a purpose rather than claiming one does.
• A server with zero collection-log rows never sees the block. Both SKUs' tools return the unavailable envelope before reaching the response body when the collector rollup is empty, so a brand-new or never-collected server reports nothing about its alerting reads. That early return predates this change and is shared with the whole payload; moving the block above it would change the envelope's shape, which other pins read. In practice the gap is narrow — a server that has never collected has barely any alert reads to fail, the fleet-scoped total is on every other server's response, and the per-server figure for such a server would be near zero anyway — but it is a real hole and it is not fixed here.
• The Store Job Over Cadence alerts quoted above name the store's own background jobs, and are read as evidence the self-alert family kept delivering. They are not proof that every self-alert family did.

Closes #3013. Fixes keywords are a no-op on dev merges, so this needs closing by hand.

[claude]

Reviewed this PR (in-memory swallowed-read counter for the alert pass, surfaced on both SKUs' get_collection_health). Overall this is careful, well-tested work — the fleet-bucket/per-server separation, the source-scanning census tests, and the Lite/Darling parity tests (byte-identical tool descriptions, matching field sets via reflection off Reading, matching server-key derivation) all hold up under inspection. No T-SQL touched, so the style guide doesn't apply here; no injection/secrets/file/network concerns — the only I/O this touches (DriveInfo, pg_database_size) is pre-existing and correctly kept out of the new counter's scope.

One real gap, left as an inline comment: the six PostgreSQL-predictor read sites in DarlingWorker.cs (EvaluatePostgresAlertsAsync, EvaluatePgCpuAsync/PgDeadlocksAsync/PgBlockingAsync/PgLongRunningQueryAsync/PgPoisonWaitAsync) call RecordReadFailure on failure but never call the matching RecordPass, so ServerAlertPasses undercounts real read activity specifically for PostgreSQL-monitored targets — the opposite failure mode from #3013 (misleadingly alarming rather than invisible), and not caught by the PR's own census tests since those only check counted-vs-exempt, not pass/failure pairing.

Everything else (thread-safety posture on AlertReadFailureCounter, the exemption table's reasoning for "handed evidence as parameters" catches, the JS panel's lazy build() closures avoiding the TDZ issue I initially suspected) checked out on closer inspection.

[claude]

Reviewed the diff (AlertReadFailureCounter.cs + wiring in AlertEngine.cs, DarlingSelfAlertEvaluator.cs, DarlingWorker.cs, both MCP tools, MainWindow.xaml.cs, server-tabs.js, and both test files). No T-SQL touched, so the collector style rules don't apply here.

Things I specifically checked and verified against source (not just the PR description's claims):

• Key-derivation parity: Darling's RecordReadFailure/ReadFor calls all use ServerId.ToString(CultureInfo.InvariantCulture), matching DarlingSelfAlertEvaluator.Key() and the alert pass in DarlingWorker. Lite uses plain .ToString() on both the write side (MainWindow.AlertEngine.cs) and read side (McpHealthTools.cs). No key-derivation mismatch that would silently produce a false-healthy zero.
• Denominator placement: RecordPass is called after the AlertsEnabled master-switch gate in AlertEngine.EvaluateCoreAsync, after the connectivity gate in DarlingSelfAlertEvaluator.EvaluateStoreAlertsAsync, and after the null-store guard in DarlingWorker.EvaluatePostgresAlertsAsync — so a pass that never touched the store never dilutes the denominator, consistent with the PR's stated intent.
• Fleet-bucket routing: the only two RecordReadFailure(null, ...) call sites (collector-cost regression self-alert in DarlingSelfAlertEvaluator, and store background-job health reads in DarlingWorker.EvaluateCompressionJobHealthAsync) are genuinely fleet-scoped (confirmed via the "FLEET-level (not per-server)" doc comment on EvaluateCollectorCostAsync), not misrouted per-server failures.
• Exemption census: spot-checked every catch block in AlertEngine.cs and DarlingSelfAlertEvaluator.cs and confirmed all use literal catch (Exception ...), so the regex-based scanner in AlertReadFailureSurfaceTests.cs can't silently skip a block by catching a narrower type. The DarlingWorker.cs catches on other (non-alert) exception types (SqlException, PostgresException, etc.) all live outside the scanned alert-pass members, so they're correctly out of scope rather than accidentally missed.
• Cross-SKU parity: diffed both get_collection_health tool description strings — confirmed byte-identical after the addition. The alert_read_health block's field set, shape, and window-note text match between DarlingMcpDataTools.cs and McpHealthTools.cs.
• JS forward reference: server-tabs.js references ALERT_READ_PANEL (defined at the bottom of the file, line ~2214) from inside SERVER_TABS/POSTGRES_TABS, which are declared earlier (lines 529/1363). This isn't a TDZ bug because the reference lives inside each tab's build: (server, ctx) => [...] closure, which only executes when a tab is opened — by then the whole module has finished evaluating. Same pattern already used for SWEEP_STATS pre-existing this PR.
• No double-counting: the outer catch in DarlingWorker.EvaluateAlertsAsync can only fire from ReadLatestCpuAsync or something escaping the engine sweep, since every per-check catch inside AlertEngine logs-and-returns rather than rethrowing (the only rethrows are OperationCanceledException when the token is actually cancelled).

Nothing I'd block on. This is a well-scoped, well-tested addition (source-scanned census with a positive/negative scanner control, red-proofed mutations, cross-SKU field/description parity pins) and I couldn't find a gap in either the counting logic or the Lite/Darling parity.

[claude]

Reviewed the diff (non-test production code: AlertReadFailureCounter.cs, AlertEngine.cs, DarlingSelfAlertEvaluator.cs, DarlingWorker.cs, both MCP tool surfaces, MainWindow.xaml.cs, server-tabs.js). This is C#/JS — no .sql files touched, so the T-SQL style conventions don't apply here.

Overall the change is careful and the census/pin machinery is genuinely thorough. I found one real correctness gap and two smaller consistency issues; left them as inline comments:

1. DarlingWorker.EvaluateAlertsAsync — a failure in ReadLatestCpuAsync (a store read on the alert-pass deadline) prevents engine.EvaluateServerAsync from ever running, so AlertEngine.EvaluateCoreAsync's RecordPass never fires for that attempt, yet the outer catch still calls RecordReadFailure. This is the exact "numerator without a matching denominator" bug this PR's c1a68d1 commit fixed for the PostgreSQL predictor group — but EveryAlertEvaluationPass_RecordsItselfInTheDenominator's fixed 3-entry-point list doesn't cover this path, so a regression here wouldn't be caught.
2. DarlingSelfAlertEvaluator.EvaluateCollectorCostAsync records its swallowed read under a null key (fleet-scoped), but the WindowNote/XML docs/both tool descriptions all enumerate exactly four fleet-scoped self-alerts ("disk pressure, compression-job health, store-job cadence, retention holds") as the only things landing in instance_read_failures outside any server. This is a fifth, undocumented one.
3. AlertEngine.CheckFailedJobsAsync — unlike every other check, the store write (SaveFailedJobWatermarkAsync) sits inside the same try/catch as the read, so a write failure there gets mislabeled as a swallowed "failed jobs" read by the new counter, contradicting the counter's own stated read-vs-write boundary (writes are explicitly exempted everywhere else in this change).

Lite/Darling parity looks solid otherwise: key derivation matches each SKU's own alert pass (invariant vs. default culture ToString()), the tool descriptions are identical apart from the SKU-specific paragraph, and the web panel is a single shared ALERT_READ_PANEL const used on both tabs.

[claude]

Review summary

Reviewed the net diff against origin/dev (11 files, +2052/-9). This is a pure C#/JS change — no T-SQL, so the T-SQL style rules don't apply here.

What it does: adds AlertReadFailureCounter, a shared in-memory, thread-safe (Interlocked + ConcurrentDictionary) counter that both SKUs' alert passes feed on every swallowed store read, and surfaces it as a new alert_read_health block on both SKUs' get_collection_health MCP tool plus a new Darling web-dashboard panel.

Lite/Darling parity: good. Checked both McpHealthTools.cs and DarlingMcpDataTools.cs — the new block, the finding/note composition, and the (now byte-identical, tested) tool descriptions match. The web-dashboard panel is Darling-only, which is correct since Lite has no web dashboard (per CONTRIBUTING.md). AlertEngine.cs (shared library) changes apply to both SKUs identically. The two AlertReadFailureSurfaceTests files pin the cross-file/cross-SKU agreement (server-key derivation, field sets, description bytes) from source rather than by assertion on today's values, which is a solid guard against future drift.

Test-suite sanity check: manually re-derived the call-site census the tests assert (s_wholeFileScopes, WorkerCountedSites/WorkerExemptSites, the 27/s_exemptions.Count totals) against actual catch (Exception / RecordReadFailure( counts in AlertEngine.cs and DarlingSelfAlertEvaluator.cs — they reconcile (13+4=17, 5+7=12, counted totals sum to 27, exempt totals sum to 17 = s_exemptions.Count).

One finding posted inline on DarlingWorker.cs:3270: the new RecordPass call in EvaluatePostgresAlertsAsync is guarded only on _postgres/_alertDeliverer being non-null, not on _settings.AlertsEnabled — unlike the other two new RecordPass sites (the shared engine and DarlingSelfAlertEvaluator), both of which skip counting a pass when the master alerting switch is off. The inline comment's own justification ("for the same reason the engine records after its master switch") doesn't match what the guard actually checks. Looks like the Postgres predictor group has never respected the master switch (pre-existing, not introduced here), but the new denominator tracking inherits that gap silently.

No security, injection, or missing-index-DMV concerns — readName values are all compile-time constants (never exception messages or other server-derived text), and the new JSON fields go through JsonSerializer.Serialize rather than manual string building.

[claude]

Review

Scope: no T-SQL touched, so CONTRIBUTING.md's SQL style section and the missing-index-DMV rule dont apply here. This is a C# change (new AlertReadFailureCounter, wiring through AlertEngine, DarlingSelfAlertEvaluator, DarlingWorker, both get_collection_health MCP tools, and the web panel), reviewed for correctness, Lite/Darling parity, security, and performance.

Correctness

• Walked the new AlertReadFailureCounter.cs counter (RecordReadFailure/RecordPass/ReadFor/ReadInstance/FormatFinding) end to end. The fleet-bucket-vs-per-server split, the null/blank-key routing, and the zero-vs-absence handling in ReadFor are all consistent with what is documented and what the tests assert.
• Verified the reachability fix in DarlingWorker.EvaluateAlertsAsync: the CPU read is now isolated in its own try/catch before the sweep's try, so a failed CPU read no longer takes engine.EvaluateServerAsync (and its RecordPass) down with it. sqlCpu/totalCpu correctly default to (null, null) on that path and the second try no longer redeclares them.
• Verified RecordPass placement in all three pass entry points (AlertEngine.EvaluateCoreAsync, DarlingSelfAlertEvaluator.EvaluateStoreAlertsAsync, DarlingWorker.EvaluatePostgresAlertsAsync) sits after the relevant master-switch/guard early-return and before any store read, matching the 'presence isn't placement' test's intent.
• Cross-checked the call-site census claims (13 engine sites/4 exempt, 5 self-alert sites/7 exempt, 9 worker sites/6 exempt = 27/17 total) against the actual diff hunks by hand -- they match.
• EvaluateCollectorCostAsync's pre-existing catch (Exception ex) when (ex is not OperationCanceledException) correctly means the new RecordReadFailure(null, ...) call added inside it will not fire on ordinary shutdown/budget cancellation -- worth confirming since a broader catch (Exception) there would have made cancellation look like a store-read failure in the exact metric this PR is trying to make trustworthy.

Lite/Darling parity

• Key derivation intentionally differs per SKU (Darling: InvariantCulture; Lite: default ToString()), and each side is pinned against its own alert pass's actual key expression via source-scan tests rather than assuming agreement -- the right way to keep two independently-evolving SKUs from silently drifting.
• Both get_collection_health tool descriptions, both MCP response shapes, and the two web-dashboard tabs (SERVER_TABS/POSTGRES_TABS) all got the new alert_read_health block/panel, with tests asserting field-set equality and panel-count equality rather than presence alone.

Security

• No new external input surface -- readName values are compile-time constants at every call site, and the only 'user'-influenced value reaching a key is an already-resolved internal server ID, not raw request input. No SQL, file, or process I/O added by this change.

Performance

• Counter uses Interlocked increments and a ConcurrentDictionary per-server map; no locks on the hot alert-evaluation path. LastFailureRead/_instanceLastFailureRead aren't volatile, but the class's own doc comment already disclaims atomicity across the fields on a single Reading, and this is diagnostics-only data, so it isn't worth a change.

No correctness bugs, parity drift, or security issues found. This PR ships an unusually large amount of self-verification (the census tests, the red-proof table, the reflection-based field-set checks), and it holds up under a manual pass over the actual diff.